[draft] feat: Add metrics to genqclient #258
Conversation
ra-grover
requested review from
benjaminjkraft,
csilvers,
dnerdy and
StevenACoffman
as code owners
ra-grover
changed the title
|
@benjaminjkraft Sorry that I haven't ✅ all the steps in the PR description, I just wanted to gauge the interest from the maintainers if they are interested in something like this. I am willing to complete the description/test cases once I have a nod from maintainers. |
There was a problem hiding this comment.
Thanks for opening this to discuss!
I'm pretty hesitant to pull in one particular metrics collection framework -- many users won't want this as they may be using other tools instead. Unless there's a single framework we can use that will work for everyone (maybe OpenTelemetry is getting close to being that? I don't really know), I'm not sure it makes sense to pull specific frameworks into genqlient. Instead, we should just provide extension points so that users can put in their own tooling.
In theory, my hope is that the existing Client interface is sufficient to do that. Would it work for your use cases to wrap the client? You would have your own struct that implements graphql.Client and contains a graphql.Client returned from genqlient's constructor. Its MakeRequest method that would log the on-request metrics you want, then call out to the wrapped client, then log the on-response metrics, then return. If you see issues with that approach, we could look at modifying or extending the existing interface (ideally by adding fields to Request or Response, since that's not a breaking change, whereas adding methods is).
Potentially we could even look at putting such wrapped clients in a separate package in genqlient, but I think probably best is for that to get prototyped in a separate package, and we can link to that to start with and merge if it becomes popular. That way you don't have to worry about our opinions so much as you iterate :-) .
|
Closing this since for housekeeping purposes -- happy to continue to discuss here or in an issue if it's helpful! |
This updates both gqlparser and gqlgen dependencies. This was prompted by an error report in gqlparser. gqlparser also preserves comments now, so some test cases might need tweaking here. Per @tduong2049 in vektah/gqlparser#269 ### What happened? > I am using [genqlient](https://github.com/Khan/genqlient/) to generate GraphQL operations into Go code. It uses gqlparser to validate a provided graph before generating code. > In this case, I am using a supergraph composed from multiple subgraphs via Rover CLI. > Prior to [v2.5.2](https://github.com/vektah/gqlparser/releases/tag/v2.5.2), gqlparser parses the supergraph and returns no errors. > Upon upgrading to v2.5.2+, I get the following error when running `go run github.com/Khan/genqlient`: > `invalid schema: Argument extension for directive join__type cannot be null.` > Possibly related to #258? ### What did you expect? No parsing errors when running: `go run github.com/Khan/genqlient` on a supergraph. However, a validation error is returned when parsing `@join__type` directives. ### Minimal graphql.schema and models to reproduce ``` type AircraftManufacturer @join__type(graph: AIRCRAFT, key: "uuid") { uuid: ID! name: String! } ``` Signed-off-by: Steve Coffman <[email protected]> --------- Signed-off-by: Steve Coffman <[email protected]>
Adds prom metrics for GenqClient :
HistrogramVecfor latency partitioned by operation, code and method.CounterVecfor number of requests partitioned by operation, code and methodCounterVecfor errors (resp.Errors)I have: