feat(replay): Update client report discard reason for invalid sessions #18796
Conversation
chargome
left a comment
chargome
left a comment
There was a problem hiding this comment.
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.
'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.
got it then if im not wrong then is network_error the appropriate drop reason ?
There was a problem hiding this comment.
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.
@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
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
chargome
changed the title
There was a problem hiding this comment.
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.
| dropReason = 'invalid'; | ||
| } else { | ||
| dropReason = 'send_error'; | ||
| } |
There was a problem hiding this comment.
Invalid drop reason not in EventDropReason type
Medium Severity
The drop reason 'invalid' is used when recording a dropped event for ReplayDurationLimitError, but 'invalid' is not a valid value in the EventDropReason type defined in packages/core/src/types-hoist/clientreport.ts. The valid reasons are: 'before_send', 'event_processor', 'network_error', 'queue_overflow', 'ratelimit_backoff', 'sample_rate', 'send_error', 'internal_sdk_error', 'buffer_overflow', and 'ignored'. The PR discussion mentioned using 'ignored' initially, suggesting this may be an oversight where either 'ignored' should be used, or 'invalid' needs to be added to the EventDropReason type.
2 participants
This PR distinguishes replays that get dropped due to session length vs. transport/ratelimit reasons in the client report.
depends on #18901
closes #18316