GraphQL over HTTP client integration #2538
Conversation
…dotnet into graphql-client
jamescrosswell
changed the title
| json = _extractor.ExtractResponseContentAsync(response).Result; | ||
| if (json is { } jsonElement) | ||
| { | ||
| if (jsonElement.TryGetProperty("errors", out var errorsElement)) |
There was a problem hiding this comment.
Just for my understanding, where would errors be coming from?
There was a problem hiding this comment.
That gets returned by the GraphQL Server... it's part of the GraphQL protocol. You can see it in the Response.Data context in the screenshot above and the server uses it to communicate some problems with the graphql. Although it was syntactically valid GraphQL (I think the client could pick up on most errors like that before sending anything through to the server) the field test was not a valid field for a Notes query so the server didn't know how to serve it up.
Unlike REST, GraphQL wasn't built specifically to sit on top of HTTP so it doesn't rely on HTTP Status codes to communicate error states... it puts that stuff in the payload instead.
One question I had actually is whether we wanted to use the GraphQL Error message as the description for the event in the case of an error, since this is probably more informative than showing 'Sentry.GraphQL.SentryGraphQLHttpFailedRequestException`.
And that triggers another question, which is what we should do if there are multiple errors communicated by the GraphQL Server in the payload? Currently we capture a single exception... which I think makes sense (one request, one stack trace, one exception). What would we use as the description of the error if there were multiple exceptions though? We could generate some kind of Aggregate exception in this case (SentryGraphQLHttpFailedRequestException is our own invention).
And that triggers yet another question: I noticed SentryGraphQLHttpFailedRequestException is the only custom exception in the whole solution... which makes me nervous. Do we have a reason for avoiding our own exception types? Is there some other preferred way to handle this?
…dotnet into graphql-client
There was a problem hiding this comment.
This looks great!
I'm just going to drop a couple of follow-ups here:
- Custom sentry-exceptions for GraphQL errors. I think that's fine. We might want to think about the naming.
Sentry.SentryGraphQLHttpFailedRequestExceptionis quite the mouthful on the actual event - Putting the error details in the exception instead of
"GraphQL Error"would improve the way the event is displayed on Sentry - Aggregating GraphQL errors. As you mentioned, going with the first error as the description sounds reasonable to me.
- We could capture the full request from Request.Data
Summary
To auto-instrument GraphQL, there are two separate packages we're looking at supporting (initially):
This pull requests enables an integration with GraphQL Client.
Requirements
Overview
graphql-client doesn't include any OpenTelemetry instrumentation so we've created a
SentryGraphQLHttpMessageHandlerandSentryGraphQLFailedRequestHandlerto instrument this.graphql-clientimplements GraphQL over HTTP so we shouldn't need to take any dependencies on thegraphql-clientnuget packages... we've done everything using standard JSON serializers.See here for an example of a trace/transaction with a GraphQL Error.
Limitations:
Example