-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(replay): Introduce ReplayDurationLimitError and enhance error ha… #18796
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: develop
Are you sure you want to change the base?
feat(replay): Introduce ReplayDurationLimitError and enhance error ha… #18796
Conversation
chargome
left a comment
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.
Hey @harshit078 thanks for contributing! For the lack of better options in the drop reasons (see https://develop.sentry.dev/sdk/telemetry/client-reports/#envelope-item-payload) I think the most fitting is still send_error in this case.
| if (err instanceof RateLimitError) { | ||
| dropReason = 'ratelimit_backoff'; | ||
| } else if (err instanceof ReplayDurationLimitError) { | ||
| dropReason = 'sample_rate'; |
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.
'sample_rate' would not be the correct drop reason here (this is only used in case an event was dropped because of the configured sample rate).
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.
got it then if im not wrong then is network_error the appropriate drop reason ?
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.
No, let's work with ignored for now (We might add a new drop reason in the future!) – please also update the browser integration tests for this
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.
Make sense, I'll update it
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.
@harshit078 actually we'll introduce a new reason right away so this stays consistant. Mind if I take this PR over? The rest looks good already
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.
sure please go ahead
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| if (err instanceof RateLimitError) { | ||
| dropReason = 'ratelimit_backoff'; | ||
| } else if (err instanceof ReplayDurationLimitError) { | ||
| dropReason = 'sample_rate'; |
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.
Incorrect drop reason for duration limit exceeded
Medium Severity
The dropReason for ReplayDurationLimitError is set to 'sample_rate', but this is semantically incorrect. As noted in the PR discussion, 'sample_rate' is specifically meant for events dropped due to the configured sample rate (e.g., sessionSampleRate), not for replays that exceed the maximum duration limit. This will cause misleading analytics where duration-exceeded drops appear as sample-rate drops, making it harder to diagnose why replays are being discarded.
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test).Closes #18316
Description
'ratelimit_backoff' | 'send_error' | 'sample_rate';