-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat(replay): Stop replays when a segment could not be sent #6765
Conversation
size-limit report 📦
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a look at the base transport and just that we're aware: In case we receive an error Http response from Sentry,
sentry-javascript/packages/core/src/transports/base.ts
Lines 79 to 83 in 9dcba78
// We don't want to throw on NOK responses, but we want to at least log them | |
if (response.statusCode !== undefined && (response.statusCode < 200 || response.statusCode >= 300)) { | |
__DEBUG_BUILD__ && logger.warn(`Sentry responded with status code ${response.statusCode} to sent event.`); | |
} | |
we're currently not retrying anything in Replay, as the transport doesn't throw in this case. I think, keeping it this way (i.e. don't retry) is probably the safer option as it prevents thrashing Sentry with requests e.g. if we're down. However, I think we should nevertheless stop the replay. WDYT?
Additionally, this adds a new client report category, retry_failed. I am not 100% sure if we can just add this like this or if this needs any changes on the backend?
I checked the dev spec for client reports:
In case a reason needs to be added, it also has to be added to the allowlist in snuba.
So I guess, we still need to do something here. Let's check with Relay folks if this would cause errors if we already add it or if it would simply be ignored for the time being.
Not sure about client reports but otherwise lgtm. |
bae6b5e
to
aba648a
Compare
Note: I will extract the client_report stuff out of this PR, as well as the "handle bad API response" stuff, to keep this more streamlined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now, thanks for extracting the two concerns.
After some investigation, we decided that we actually don't really need a queue for handling replay events, because this is already basically handled by flushing.
For context, the flow in replay is as follows:
Because of this, no segment can be sent before the previous segment has finished being sent.
So instead of adding a new queue, the only change of this PR is to ensure we actually
stop()
the replay when we exceed the retry limit. This will ensure that no following segment will be sent, and we don't get any inconsistent replays. Instead, the replay will just end with the last successful segment.Additionally, this adds a new client report category,retry_failed
. I am not 100% sure if we can just add this like this or if this needs any changes on the backend?closes #6639
closes #6671