Skip to content

SentryGraphQLHttpFailedRequestHandler improved issue grouping. #4762

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ericsampson wants to merge 7 commits into
base: main
Choose a base branch
from
Open

SentryGraphQLHttpFailedRequestHandler improved issue grouping. #4762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
dd0359b
Improve the issue grouping of the SentryGraphQLHttpFailedRequestHandler
ericsampson Nov 22, 2025
e7cd5df
changelog
ericsampson Nov 22, 2025
c3faef8
Improve approach to not use an Inner exception per PR feedback.
ericsampson Nov 24, 2025
6692c2f
Merge branch 'main' into SentryGraphQLHttpFailedRequestHandler-issue-…
ericsampson Nov 24, 2025
3a8b858
Update src/Sentry/SentryGraphQLHttpFailedRequestHandler.cs
ericsampson Nov 26, 2025
9301663
Merge branch 'main' into SentryGraphQLHttpFailedRequestHandler-issue-…
ericsampson Nov 26, 2025
b74ff29
fix changelog
ericsampson Nov 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Changelog

## Unreleased

### Fixes

- Captured [GraphQL Client Errors](https://docs.sentry.io/platforms/dotnet/configuration/graphql-client-errors/) on .NET 5+ now include a full stack trace in order to improve Issue grouping ([#4762](https://github.com/getsentry/sentry-dotnet/pull/4762))

## 6.0.0-rc.2

### Features
Expand Down
14 changes: 12 additions & 2 deletions src/Sentry/SentryGraphQLHttpFailedRequestHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System.Runtime.ExceptionServices;
using Sentry.Internal;
using Sentry.Internal.Extensions;
using Sentry.Protocol;
Expand All @@ -24,15 +25,24 @@ protected internal override void DoEnsureSuccessfulResponse([NotNull] HttpReques
JsonElement? json = null;
try
{
json = GraphQLContentExtractor.ExtractResponseContentAsync(response, _options).Result;
json = GraphQLContentExtractor.ExtractResponseContentAsync(response, _options).GetAwaiter().GetResult();
if (json is { } jsonElement)
{
if (jsonElement.TryGetProperty("errors", out var errorsElement))
{
// We just show the first error... maybe there's a better way to do this when multiple errors exist.
// We should check what the Java code is doing.
var errorMessage = errorsElement[0].GetProperty("message").GetString() ?? "GraphQL Error";
throw new GraphQLHttpRequestException(errorMessage);
var exception = new GraphQLHttpRequestException(errorMessage);

#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));
Copy link
Collaborator

@jamescrosswell jamescrosswell Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@ericsampson ericsampson Nov 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@Flash0ver Flash0ver Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@Flash0ver Flash0ver Nov 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

however ....

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

🤔

Copy link
Member

@Flash0ver Flash0ver Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@ericsampson ericsampson Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@jamescrosswell jamescrosswell Dec 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@ericsampson ericsampson Dec 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#else
// Where SetCurrentStackTrace is not available, just throw the Exception
throw exception;
#endif
}
}
// No GraphQL errors, but we still might have an HTTP error status
Expand Down
98 changes: 91 additions & 7 deletions test/Sentry.Tests/SentryGraphQlHttpFailedRequestHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ namespace Sentry.Tests;
public class SentryGraphQlHttpFailedRequestHandlerTests
{
private const string ValidQuery = "query getAllNotes { notes { id } }";
private const string DefaultErrorMessage = "Bad query";
private const string DefaultErrorCode = "BAD_OP";

private static HttpResponseMessage ForbiddenResponse()
=> new(HttpStatusCode.Forbidden);

Expand All @@ -12,7 +15,7 @@ private static HttpResponseMessage InternalServerErrorResponse()
private HttpResponseMessage PreconditionFailedResponse()
=> new(HttpStatusCode.PreconditionFailed)
{
Content = SentryGraphQlTestHelpers.ErrorContent("Bad query", "BAD_OP")
Content = SentryGraphQlTestHelpers.ErrorContent(DefaultErrorMessage, DefaultErrorCode)
};

[Fact]
Expand Down Expand Up @@ -81,7 +84,7 @@ public void HandleResponse_NoMatchingTarget_DontCapture()
}

[Fact]
public void HandleResponse_NoError_BaseCapturesFailedRequests()
public void HandleResponse_NoGraphQLError_HttpHandlerFallbackCapturesFailedRequests()
{
// Arrange
var hub = Substitute.For<IHub>();
Expand All @@ -91,16 +94,26 @@ public void HandleResponse_NoError_BaseCapturesFailedRequests()
};
var sut = new SentryGraphQLHttpFailedRequestHandler(hub, options);

var response = InternalServerErrorResponse();
response.RequestMessage = new HttpRequestMessage();
// Response has valid JSON but no GraphQL errors - just HTTP error status
var response = new HttpResponseMessage(HttpStatusCode.InternalServerError)
{
Content = SentryGraphQlTestHelpers.JsonContent(new { data = "some response data" }),
RequestMessage = new HttpRequestMessage(HttpMethod.Post, "http://example.com/graphql")
};

// Act
SentryEvent @event = null;
hub.CaptureEvent(Arg.Do<SentryEvent>(e => @event = e), hint: Arg.Any<SentryHint>());
sut.HandleResponse(response);

// Assert
hub.Received(1).CaptureEvent(
Arg.Any<SentryEvent>(),
Arg.Any<Scope>(), Arg.Any<SentryHint>());

// Should fall back to HTTP handler, capturing HttpRequestException
@event.Exception.Should().BeOfType<HttpRequestException>();
@event.Exception!.Message.Should().NotBeNullOrWhiteSpace();
}

[Fact]
Expand All @@ -115,15 +128,21 @@ public void HandleResponse_Error_Capture()
var sut = new SentryGraphQLHttpFailedRequestHandler(hub, options);

var response = PreconditionFailedResponse();
response.RequestMessage = new HttpRequestMessage();
response.RequestMessage = SentryGraphQlTestHelpers.GetRequestQuery(ValidQuery);

// Act
SentryEvent @event = null;
hub.CaptureEvent(Arg.Do<SentryEvent>(e => @event = e), hint: Arg.Any<SentryHint>());
sut.HandleResponse(response);

// Assert
hub.Received(1).CaptureEvent(
Arg.Any<SentryEvent>(),
Arg.Any<Scope>(), Arg.Any<SentryHint>());

// Verify it's actually a GraphQL error, not HTTP error fallback
@event.Exception.Should().BeOfType<GraphQLHttpRequestException>();
@event.Exception!.Message.Should().Be(DefaultErrorMessage);
}

[Fact]
Expand All @@ -139,14 +158,15 @@ public void HandleResponse_Error_DontSendPii()

var response = PreconditionFailedResponse();
var uri = new Uri("http://admin:1234@localhost/test/path?query=string#fragment");
response.RequestMessage = new HttpRequestMessage(HttpMethod.Get, uri);
response.RequestMessage = SentryGraphQlTestHelpers.GetRequestQuery(ValidQuery, uri.ToString());

// Act
SentryEvent @event = null;
hub.CaptureEvent(Arg.Do<SentryEvent>(e => @event = e), hint: Arg.Any<SentryHint>());
sut.HandleResponse(response);

// Assert
@event.Exception.Should().BeOfType<GraphQLHttpRequestException>();
@event.Request.Url.Should().Be("http://localhost/test/path?query=string"); // No admin:1234
@event.Request.Data.Should().BeNull();
var responseContext = @event.Contexts[Response.Type] as Response;
Expand Down Expand Up @@ -188,6 +208,10 @@ public void HandleResponse_Error_CaptureRequestAndResponse()
{
@event.Should().NotBeNull();

// Ensure it's a GraphQL exception (not HTTP fallback)
@event.Exception.Should().BeOfType<GraphQLHttpRequestException>();
@event.Exception!.Message.Should().Be(DefaultErrorMessage);

// Ensure the mechanism is set
@event.Exception?.Data[Mechanism.MechanismKey].Should().Be(SentryGraphQLHttpFailedRequestHandler.MechanismType);
@event.Exception?.Data[Mechanism.HandledKey].Should().Be(false);
Expand All @@ -205,7 +229,7 @@ public void HandleResponse_Error_CaptureRequestAndResponse()
responseContext?.StatusCode.Should().Be((short)response.StatusCode);
responseContext?.BodySize.Should().Be(response.Content.Headers.ContentLength);
responseContext?.Data?.ToString().Should().Be(
SentryGraphQlTestHelpers.ErrorContent("Bad query", "BAD_OP").ReadAsJson().ToString()
SentryGraphQlTestHelpers.ErrorContent(DefaultErrorMessage, DefaultErrorCode).ReadAsJson().ToString()
);

@event.Contexts.Response.Headers.Should().ContainKey("myHeader");
Expand Down Expand Up @@ -249,4 +273,64 @@ public void HandleResponse_Error_ResponseAsHint()
hint.Items[HintTypes.HttpResponseMessage].Should().Be(response);
}
}

[Fact]
public void HandleResponse_GraphQLError_HasExceptionWithStackTrace()
{
// Arrange
var hub = Substitute.For<IHub>();
var options = new SentryOptions
{
CaptureFailedRequests = true
};
var sut = new SentryGraphQLHttpFailedRequestHandler(hub, options);

var response = PreconditionFailedResponse();
response.RequestMessage = SentryGraphQlTestHelpers.GetRequestQuery(ValidQuery);

// Act
SentryEvent @event = null;
hub.CaptureEvent(Arg.Do<SentryEvent>(e => @event = e), hint: Arg.Any<SentryHint>());
sut.HandleResponse(response);

// Assert
using (new AssertionScope())
{
@event.Should().NotBeNull();
@event.Exception.Should().NotBeNull();
@event.Exception!.StackTrace.Should().NotBeNullOrWhiteSpace();
}
}

#if NET5_0_OR_GREATER // This test is only valid on .NET 5+ where we can use SetCurrentStackTrace
[Fact]
public void HandleResponse_GraphQLError_ExceptionStackTraceHasCallerContext()
{
// Arrange
var hub = Substitute.For<IHub>();
var options = new SentryOptions
{
CaptureFailedRequests = true
};
var sut = new SentryGraphQLHttpFailedRequestHandler(hub, options);

var response = PreconditionFailedResponse();
response.RequestMessage = SentryGraphQlTestHelpers.GetRequestQuery(ValidQuery);

// Act
SentryEvent @event = null;
hub.CaptureEvent(Arg.Do<SentryEvent>(e => @event = e), hint: Arg.Any<SentryHint>());
sut.HandleResponse(response);

// Assert
using (new AssertionScope())
{
@event.Should().NotBeNull();
@event.Exception.Should().NotBeNull();

// Exception's stack trace should include this test method name, proving we captured caller context on .NET 5+
@event.Exception!.StackTrace.Should().Contain(nameof(HandleResponse_GraphQLError_ExceptionStackTraceHasCallerContext));
}
}
#endif
}
17 changes: 10 additions & 7 deletions test/Sentry.Tests/SentryGraphQlTestHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,17 +46,20 @@ public static StringContent ResponesContent(string responseText) => JsonContent(

/// <summary>
/// e.g.
/// "[{"message":"Query does not contain operation \u0027getAllNotes\u0027.","extensions":{"code":"INVALID_OPERATION","codes":["INVALID_OPERATION"]}}]"
/// "{"errors": [{"message":"Query does not contain operation \u0027getAllNotes\u0027.","extensions":{"code":"INVALID_OPERATION","codes":["INVALID_OPERATION"]}}]}"
/// </summary>
public static StringContent ErrorContent(string errorMessage, string errorCode) => JsonContent(
new dynamic[]
new
{
new
errors = new dynamic[]
{
message = errorMessage,
extensions = new {
code = errorCode,
codes = new dynamic[]{ errorCode }
new
{
message = errorMessage,
extensions = new {
code = errorCode,
codes = new dynamic[]{ errorCode }
}
}
}
}
Expand Down
Loading