Improve SentryHttpFailedRequestHandler issue grouping #4724
Conversation
…tion stack trace where possible, to improve Issue grouping.
ericsampson
requested review from
Flash0ver and
jamescrosswell
as code owners
| /// <param name="ranges">Collection of ranges to check.</param> | ||
| /// <param name="statusCode">Status code to check.</param> | ||
| /// <returns>True if any range contains the given status code.</returns> | ||
| internal static bool ContainsStatusCode(this IEnumerable<HttpStatusCodeRange> ranges, int statusCode) |
There was a problem hiding this comment.
Couldn't use the standard method name Contains because in that case the implicit conversion operators in HttpStatusCodeRange resulted in these extension methods not getting called.
| _hub?.DidNotReceive().CaptureEvent(Arg.Any<SentryEvent>()); | ||
| _hub?.DidNotReceiveWithAnyArgs().CaptureEvent(null!); |
There was a problem hiding this comment.
These tests weren't actually going to catch failures due to the use of DidNotReceive().CaptureEvent(Arg.Any<SentryEvent>());, which behaves in a subtle footgun manner for capturing methods that have optional arguments.
It's much safer to use DidNotReceiveWithAnyArgs and ReceivedWithAnyArgs in these scenarios, unless you're careful to specify an explicit value for every method parameter (whether Any or a specific value)
I made the same tweak throughout the rest of this test file.
| #if NET6_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.SetRemoteStackTrace(exception, Environment.StackTrace); |
|
This looks awesome @ericsampson - we're a bit tight on time but will definitely do our best to get this reviewed and included in version 6... it's a slight change in behaviour so might be good to do this in the major release. |
|
Totally understood @jamescrosswell! Thanks. Would it be worthwhile to start just by approving all the GH workflows to run to make sure that they don't surface any issues.
1b) Should I add a release note line?
Best, |
|
|
||
| var @event = new SentryEvent(exception); | ||
| var hint = new SentryHint(HintTypes.HttpResponseMessage, response); | ||
| exception.SetSentryMechanism(MechanismType); |
There was a problem hiding this comment.
everything below here in the file is only appears changed due to indenting
2 participants
Resolves #3582
With the current SentryHttpFailedRequestHandler, the captured exception stack trace is always the same two frames; this leads to all events getting lumped into a single Issue, even though the application code path (and remote API) are totally different. As you can imagine, this is less than ideal :)
This PR addresses this issue by capturing a full stack trace on .NET 6+ using
ExceptionDispatchInfo.SetRemoteStackTrace.Tests have been expanded and improved. The test of the new functionality (
HandleResponse_StackTraceIncludesCallerContext) was verified to fail using the existing code, and pass using the new code.Very similar changes could likely be made to the
SentryGraphQLHttpFailedRequestHandler.csto give the same improvement, but I didn't want to bundle too many changes together into one PR.I'd love to get this into 5.16.3 or 6.0.0 if possible, depending on the release timing :)
Thanks!