SentryGraphQLHttpFailedRequestHandler improved issue grouping.

[ericsampson]

Fixes #4759

• Fix issue grouping on GraphQL requests #4759

---

Applies the same improvements for GraphQL as in this PR for plain HTTP: #4724

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.94%. Comparing base (42243df) to head (b74ff29).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4762      +/-   ##
==========================================
+ Coverage   73.85%   73.94%   +0.08%     
==========================================
  Files         485      485              
  Lines       17689    17691       +2     
  Branches     3497     3497              
==========================================
+ Hits        13065    13081      +16     
+ Misses       3762     3750      -12     
+ Partials      862      860       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Flash0ver]

again: thank you so much for the improvement ... and well done on the testing part!

some small changes needed

• align code indentation, according to our dotnet format rules and workflow
• merge from main to update the CHANGELOG

other than that it's ✅ from my end

[Flash0ver]

@sentry review

[Flash0ver]

hmmm ... correct ... but

in SentryHttpFailedRequestHandler we essentially only read the Status-Code

here, in SentryGraphQLHttpFailedRequestHandler, we also try to extract JSON content from the HttpResponseMessage ... I'm a bit uncertain whether this will always succeed ... should the JSON Content from the Response be malformed then GraphQLContentExtractor.ExtractResponseContentAsync would throw a JsonReaderException

I guess this would be another issue/changeset to refactor this approach,
as this PR is not changing anything about the throw-behavior of this method,
but instead replaced the Stack-Trace of the Exception with a richer Stack-Trace for more detailed grouping in Sentry.
Technically, I do agree that this would be nice to have, to make the NET5_0_OR_GREATER path "throw-free".

@jamescrosswell what do you think?

[ericsampson]

For context I wanted to keep the changes targeted and minimal vs the current strategy (at least for a first PR), I figured it's safer to have the outer try as in the original to capture json extraction failures--that seems like "not an edge case".

[jamescrosswell]

Seems unrelated to this PR yeah... I'm not sure I see a problem. If the JSON is malformed, that would be caught and converted to a sentry event here, which I don't think is unexpected behaviour. Is there a better alternative?

[jamescrosswell]

So I think I'm getting confused now.

How is this:

var exception = new GraphQLHttpRequestException(errorMessage);
ExceptionDispatchInfo.Throw(ExceptionDispatchInfo.SetCurrentStackTrace(exception));

... different from this?

var exception = new GraphQLHttpRequestException(errorMessage);
throw exception;

I get that the behaviour for ExceptionDispatchInfo.Throw is different to throw when we're talking about exceptions that have been thrown elsewhere in the code and have a stack trace to preserve... in this case we're creating the exception and throwing it on the very next line though... so what is the value of ExceptionDispatchInfo.Throw vs a plain old throw in this scenario? Aren't the stack traces going to be almost identical?

[ericsampson]

It is different, I know it's not obvious--try it via the unit test and debugging, if you're interested. The one test should fail if you just throw

[Flash0ver]

indeed

via Sentry.Samples.GraphQL.Server and Sentry.Samples.GraphQL.Client.Http

    at Sentry.SentryGraphQLHttpFailedRequestHandler.DoEnsureSuccessfulResponse(HttpRequestMessage request, HttpResponseMessage response) in /../sentry-dotnet/src/Sentry/SentryGraphQLHttpFailedRequestHandler.cs:line 40
+   at Sentry.SentryFailedRequestHandler.HandleResponse(HttpResponseMessage response) in /../sentry-dotnet/src/Sentry/SentryFailedRequestHandler.cs:line 47
+   at Sentry.SentryGraphQLHttpMessageHandler.HandleResponse(HttpResponseMessage response, ISpan span, String method, String url) in /../sentry-dotnet/src/Sentry/SentryGraphQLHttpMessageHandler.cs:line 95
+   at Sentry.SentryMessageHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) in /../sentry-dotnet/src/Sentry/SentryMessageHandler.cs:line 91
+   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1.ExecutionContextCallback(Object s)
+   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
+   at ...
+   at System.Threading.ThreadPoolWorkQueue.Dispatch()
+   at System.Threading.PortableThreadPool.WorkerThread.WorkerThreadStart()
+   at System.Threading.Thread.StartCallback()
+--- End of stack trace from previous location ---
+   at Sentry.SentryGraphQLHttpFailedRequestHandler.DoEnsureSuccessfulResponse(HttpRequestMessage request, HttpResponseMessage response) in /../sentry-dotnet/src/Sentry/SentryGraphQLHttpFailedRequestHandler.cs:line 40

[Flash0ver]

however ....

there isn't really a difference in user frames ...
so in Sentry, I don't really see a difference in Grouping / Fingerprinting

🤔

[Flash0ver]

@ericsampson, could you give 6.0.0-rc.2 a try to confirm that #4724 indeed improves your scenario?