SentryGraphQLHttpFailedRequestHandler improved issue grouping. by ericsampson · Pull Request #4762 · getsentry/sentry-dotnet

ericsampson · 2025-11-22T15:56:21Z

Fixes #4759

Fix issue grouping on GraphQL requests #4759

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

src/Sentry/SentryGraphQLHttpFailedRequestHandler.cs

test/Sentry.Tests/SentryGraphQlTestHelpers.cs

codecov · 2025-11-24T17:21:26Z

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.

…grouping

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

src/Sentry/SentryGraphQLHttpFailedRequestHandler.cs

CHANGELOG.md

test/Sentry.Tests/SentryGraphQlTestHelpers.cs

Flash0ver · 2025-11-26T12:49:42Z

@sentry review

Flash0ver · 2025-11-26T16:01:42Z

src/Sentry/SentryGraphQLHttpFailedRequestHandler.cs

+    // Add a full stack trace into the exception to improve Issue grouping,
+    // see https://github.com/getsentry/sentry-dotnet/issues/3582
+    ExceptionDispatchInfo.Throw(ExceptionDispatchInfo.SetCurrentStackTrace(exception));
+#else
+                    // Where SetCurrentStackTrace is not available, just throw the Exception
+                    throw exception;
+#endif


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?

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".

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?

Co-authored-by: Stefan Pölz <38893694+Flash0ver@users.noreply.github.com>

…grouping

jamescrosswell · 2025-11-26T21:34:46Z

src/Sentry/SentryGraphQLHttpFailedRequestHandler.cs

+#if NET5_0_OR_GREATER
+                    // Add a full stack trace into the exception to improve Issue grouping,
+                    // see https://github.com/getsentry/sentry-dotnet/issues/3582
+                    ExceptionDispatchInfo.Throw(ExceptionDispatchInfo.SetCurrentStackTrace(exception));


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?

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

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

however ....

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

🤔

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

@Flash0ver sent you and James an email just now with some info, it doesn't look like it's working how I expected/wanted 😭

That's OK - this problem space turns us all on our heads at some point.

Thanks heaps for looking into this @ericsampson and should we close this PR then?

I'd be interested in at least figuring out why it's turning out this way, for the HTTP Client case at least. It feels to me like maybe the Sentry server-side fingerprinting or something is transforming things in a way that is unexpected to me...

EnesEfeTokta

I think it looks okay. Good jobb. 💡

Improve the issue grouping of the SentryGraphQLHttpFailedRequestHandler

dd0359b

ericsampson requested review from Flash0ver and jamescrosswell as code owners November 22, 2025 15:56

changelog

e7cd5df

ericsampson mentioned this pull request Nov 22, 2025

SentryHttpMessageHandler issue grouping is too eager #3582

Closed

cursor bot reviewed Nov 22, 2025

View reviewed changes

src/Sentry/SentryGraphQLHttpFailedRequestHandler.cs Outdated Show resolved Hide resolved

ericsampson mentioned this pull request Nov 22, 2025

Fix issue grouping on GraphQL requests #4759

Open

Flash0ver reviewed Nov 24, 2025

View reviewed changes

src/Sentry/SentryGraphQLHttpFailedRequestHandler.cs Show resolved Hide resolved

Improve approach to not use an Inner exception per PR feedback.

c3faef8

ericsampson commented Nov 24, 2025

View reviewed changes

test/Sentry.Tests/SentryGraphQlTestHelpers.cs Show resolved Hide resolved

Merge branch 'main' into SentryGraphQLHttpFailedRequestHandler-issue-…

6692c2f

…grouping

Flash0ver reviewed Nov 26, 2025

View reviewed changes

src/Sentry/SentryGraphQLHttpFailedRequestHandler.cs Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

test/Sentry.Tests/SentryGraphQlTestHelpers.cs Show resolved Hide resolved

sentry bot reviewed Nov 26, 2025

View reviewed changes

ericsampson and others added 3 commits November 26, 2025 12:40

Update src/Sentry/SentryGraphQLHttpFailedRequestHandler.cs

3a8b858

Co-authored-by: Stefan Pölz <38893694+Flash0ver@users.noreply.github.com>

Merge branch 'main' into SentryGraphQLHttpFailedRequestHandler-issue-…

9301663

…grouping

fix changelog

b74ff29

jamescrosswell reviewed Nov 26, 2025

View reviewed changes

EnesEfeTokta reviewed Dec 8, 2025

View reviewed changes

Uh oh!

Conversation

ericsampson commented Nov 22, 2025 • edited by Flash0ver Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Flash0ver left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Flash0ver commented Nov 26, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

Flash0ver Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsampson Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamescrosswell Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

jamescrosswell Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

ericsampson Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flash0ver Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Flash0ver Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flash0ver Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

ericsampson Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

jamescrosswell Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

ericsampson Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

EnesEfeTokta left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ericsampson commented Nov 22, 2025 •

edited by Flash0ver

Loading

codecov bot commented Nov 24, 2025 •

edited

Loading

Flash0ver Nov 26, 2025 •

edited

Loading

ericsampson Nov 26, 2025 •

edited

Loading

ericsampson Nov 27, 2025 •

edited

Loading

Flash0ver Nov 27, 2025 •

edited

Loading

EnesEfeTokta left a comment •

edited

Loading