-
-
Notifications
You must be signed in to change notification settings - Fork 231
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.
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
dd0359b
e7cd5df
c3faef8
6692c2f
3a8b858
9301663
b74ff29
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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; | ||
|
|
@@ -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(); | ||
|
ericsampson marked this conversation as resolved.
|
||
| 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)); | ||
| #else | ||
|
ericsampson marked this conversation as resolved.
|
||
| // Where SetCurrentStackTrace is not available, just throw the Exception | ||
| throw exception; | ||
| #endif | ||
This comment was marked as outdated.
Sorry, something went wrong.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmmm ... correct ... but in here, in I guess this would be another issue/changeset to refactor this approach, @jamescrosswell what do you think?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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".
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| } | ||
| } | ||
| // No GraphQL errors, but we still might have an HTTP error status | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.