feat(js): Capture an event when a 200 counts as an error #49060
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
Scope: Frontend
scttcper
approved these changes
May 12, 2023
lobsterkatie
deleted the
kmclb-capture-error-when-200-triggers-request-error
branch
lobsterkatie
added a commit
that referenced
this pull request
May 15, 2023
This is a follow-up to #49060, which made it so that we capture an event whenever a 200 response to a request made by our frontend API client counts as an error. Now that some of those events have come in, it's clear that at least one of the problems is that we're ending up with responses where `response.text()` returns `undefined`. This separates such cases out into their own issue, so that it can remain open (with a much more explanatory error message) while we debug the cause and the existing issue can be resolved. That way, if the existing issue regresses, we'll know that there's a second reason why 200s are counting as errors.
lobsterkatie
added a commit
that referenced
this pull request
May 17, 2023
This makes several improvements to the way our frontend API client handles errors coming from responses with a 200 status. - Since we are now capturing an event specifically about this situation[1], don't also send a `RequestError` to Sentry. - In the capturing of that event: - Stop logging redundant data in extras - Only log extras if the error is a different one than `responseText` being undefined, which we already know about - Add `errorReason` as a tag, so we can see a breakdown of the different values - Use error message as fingerprint, for simplicity and to force the non-"`ResponseText` is undefined" errors into a new group [1] #49060
This was referenced May 22, 2023
volokluev
pushed a commit
that referenced
this pull request
May 30, 2023
For reasons we don't yet understand, we get a lot of `RequestError` events with a 200 status, all coming from our frontend API client's `request` method. This captures an error when that happens, so that we can debug it and handle whatever is prompting such behavior more gracefully.
volokluev
pushed a commit
that referenced
this pull request
May 30, 2023
This is a follow-up to #49060, which made it so that we capture an event whenever a 200 response to a request made by our frontend API client counts as an error. Now that some of those events have come in, it's clear that at least one of the problems is that we're ending up with responses where `response.text()` returns `undefined`. This separates such cases out into their own issue, so that it can remain open (with a much more explanatory error message) while we debug the cause and the existing issue can be resolved. That way, if the existing issue regresses, we'll know that there's a second reason why 200s are counting as errors.
volokluev
pushed a commit
that referenced
this pull request
May 30, 2023
This makes several improvements to the way our frontend API client handles errors coming from responses with a 200 status. - Since we are now capturing an event specifically about this situation[1], don't also send a `RequestError` to Sentry. - In the capturing of that event: - Stop logging redundant data in extras - Only log extras if the error is a different one than `responseText` being undefined, which we already know about - Add `errorReason` as a tag, so we can see a breakdown of the different values - Use error message as fingerprint, for simplicity and to force the non-"`ResponseText` is undefined" errors into a new group [1] #49060
lobsterkatie
added a commit
that referenced
this pull request
May 30, 2023
Sometimes when our frontend client makes a request to our API, we get a 200 response which is nonetheless considered an error. In #49060, we added code to capture a separate event when this happens, as a first step towards investigating what was causing it. After a few refinements of that code, it's now clear that the problem is response bodies coming back with a 200 but no actual data. (According to the code there could be other reasons, but no instances of this have been recorded so far.) What we still don't know, though, is what downstream effect these empty responses have (if any), because we filter any resulting events out in `beforeSend`. This PR aims to fix that, by switching from recording a separate event to marking and allowing through any resulting errors. Key changes: - Only capture a separate event for 200-status error responses when the response body isn't empty. (It's possible that in real life this never happens, but according to the code it could, so I left it in.) - Add a new kind of `RequestError`, called `UndefinedResponseBodyError`, corresponding to a 200 status code and an empty body. Not only does this identify such errors, it also allows them to pass through our existing filters. - Add a helper in `beforeSend` to tag and fingerprint such errors, so that they're easy to find and so that they group together to prevent noise in the issue stream. Note: While empty response bodies show up both as `undefined` and as the empty string, the latter case only seems to happen when we hit `internal/health`, specifically when we land here[1]. Since this seems like the system working as designed, I'm letting those events continue to be filtered, and only special-casing the `undefined` case. [1] https://github.com/getsentry/sentry/blob/bac5c9dcd9b7ad9adb95f51330fe10bc277e6fa6/src/sentry/api/endpoints/system_health.py#L21-L22
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For reasons we don't yet understand, we get a lot of
RequestErrorevents with a 200 status, all coming from our frontend API client'srequestmethod. This captures an error when that happens, so that we can debug it and handle whatever is prompting such behavior more gracefully.Part of #49142