fix(core): Sample errors after beforeSend while preserving session updates - #22671
Open
s1gr1d wants to merge 5 commits into
Open
fix(core): Sample errors after beforeSend while preserving session updates#22671s1gr1d wants to merge 5 commits into
beforeSend while preserving session updates#22671s1gr1d wants to merge 5 commits into
Conversation
…nd` returns `null`
…nd` returns `null`
s1gr1d
force-pushed
the
sig/fix-session-error-rate
branch
from
July 27, 2026 10:40
e0abd98 to
b002e45
Compare
Contributor
size-limit report 📦
|
Lms24
reviewed
Jul 27, 2026
Lms24
left a comment
Lms24
left a comment
Member
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, { |
Member
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?
Member
Author
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"
Member
There was a problem hiding this comment.
ah thanks for explaining, makes sense!
Comment on lines
+1497
to
+1501
| // 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); | ||
| } |
Member
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}
s1gr1d
commented
Jul 27, 2026
| 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', () => { |
Member
Author
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)
3 tasks
s1gr1d
changed the title
beforeSend returns nullsampleRate drops error event
s1gr1d
changed the title
sampleRate drops error eventbeforeSend while preserving session updates
Comment on lines
+1483
to
+1485
| `Discarding event because it's not included in the random sample (sampling rate = ${sampleRate})`, | ||
| ); | ||
| } |
Contributor
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.
s1gr1d
force-pushed
the
sig/fix-session-error-rate
branch
from
July 28, 2026 12:53
fc24c42 to
f711e80
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
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