fix(core): Sample errors after beforeSend while preserving session updates - #22671
Conversation
…nd` returns `null`
…nd` returns `null`
e0abd98 to
b002e45
Compare
size-limit report 📦
|
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
The sampling fix is correct. For beforeSend, we shouldn't increase error counts for filtered events (see comment).
| // Mechanism is passed via hint so it's available even when the event | ||
| // is dropped before event processors run (e.g. sampleRate sampling). | ||
| // We also add it via an event processor above. | ||
| captureException(ex, { |
There was a problem hiding this comment.
q: given the change in client.ts where we also merge the hint's mechanism: do we need this?
There was a problem hiding this comment.
Yes, we need it here for when _prepareEvent is skipped.
eventFromException defaults every exception to handled: true, and e.g. handled: false (from the global error handler) is only merged onto the event inside _prepareEvent, which is skipped on the sampleRate drop path -> removing this also breaks this test "marks session as unhandled when unhandled error is sampled out by sampleRate"
There was a problem hiding this comment.
ah thanks for explaining, makes sense!
| // Reflects crashes inside release health sessions, regardless of beforeSend dropping the event. | ||
| const session = currentScope.getSession() || isolationScope.getSession(); | ||
| if (isError && session) { | ||
| this._updateSessionFromEvent(session, preparedEvent); | ||
| } |
There was a problem hiding this comment.
h: events dropped via beforeSend should not update a session. See Sessions spec and the internal slack convo we had about this topic a few weeks ago (will send you the link).
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| const client = new TestClient(options); | ||
| setCurrentClient(client); | ||
| describe('edge case: sampled-out error that beforeSend would have filtered', () => { | ||
| test('updates session even though beforeSend would have dropped the error', () => { |
There was a problem hiding this comment.
tests the edge-case that was mentioned in a Slack convo (Sampled out + would have been filtered by beforeSend)
s1gr1d
changed the title
beforeSend returns nullsampleRate drops error event
s1gr1d
changed the title
sampleRate drops error eventbeforeSend while preserving session updates
| `Discarding event because it's not included in the random sample (sampling rate = ${sampleRate})`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Bug: Sentry.lastEventId() is now set for events that are dropped by sampleRate, breaking user feedback integrations like showReportDialog() which rely on a sent event.
Severity: MEDIUM
Suggested Fix
Move the setLastEventId call to after the sampleRate check within the _prepareEvent function. This ensures Sentry.lastEventId() is only set for events that are actually sent, restoring the expected behavior for features that rely on it.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/core/src/client.ts#L1483-L1485
Potential issue: The `setLastEventId` call in `_prepareEvent` now executes before the
`sampleRate` check. Consequently, `Sentry.lastEventId()` stores the ID for all error
events, including those that are subsequently dropped due to sampling and never sent to
Sentry. This behavior breaks integrations like `showReportDialog()`, which depend on
`lastEventId()` to reference an event that was successfully transmitted in order to
associate user feedback with it.
There was a problem hiding this comment.
this is a known limitation. I made it more obvious: b653230
fc24c42 to
f711e80
Compare
| throw _makeDoNotSendEventError( | ||
| `Discarding event because it's not included in the random sample (sampling rate = ${sampleRate})`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Bug: The sampleRate check is now performed after event processing, causing unnecessary work for events that are ultimately sampled out and discarded.
Severity: MEDIUM
Suggested Fix
This change was intentional to support session tracking. A potential improvement could be a two-phase processing system: handle session-relevant data, make the sampling decision, and only then execute the full, expensive processing pipeline for events that will be sent, if the spec allows.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/core/src/client.ts#L1487-L1490
Potential issue: The order of operations for event capturing has been changed to `event
processors → beforeSend → session update → sampleRate → send`. This means that for any
event that is ultimately dropped due to sampling, the SDK still performs all the
processing work upfront. This includes merging scope data, running integrations,
executing potentially expensive user-defined `event processors` and `beforeSend`
callbacks, and data normalization. This introduces a performance regression,
particularly impactful for applications with low `sampleRate` values, where the majority
of events are processed only to be discarded.
2 participants
When
sampleRatecauses an error event to be dropped, the_processEventmethod returned early before reaching_updateSessionFromEvent, so the session is never marked crashed/errored. This increased crash-free session rates.This PR moves error event sampling after event processors and
beforeSend, while keeping the session update before the sampling decision.The updated pipeline is:
event processors → beforeSend → session update → sampleRate → sendThis follows the session update filtering spec.
Closes #22615