-
-
Notifications
You must be signed in to change notification settings - Fork 225
feat: Cache Unhandled session instead of sending it immediately
#4653
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: feat/session-type-unhandled
Are you sure you want to change the base?
feat: Cache Unhandled session instead of sending it immediately
#4653
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/session-type-unhandled #4653 +/- ##
===============================================================
+ Coverage 73.17% 73.20% +0.03%
===============================================================
Files 479 479
Lines 17349 17370 +21
Branches 3426 3431 +5
===============================================================
+ Hits 12695 12716 +21
Misses 3804 3804
Partials 850 850 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…sentry/sentry-dotnet into feat/cache-unhandled-session
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.
Bug: Session State Loss During Persistence
The PauseSession method calls PersistSession without passing the pendingUnhandled state. This causes sessions marked as pending unhandled to lose this status when persisted, leading to them being incorrectly recovered as Exited instead of Unhandled on app restart.
src/Sentry/GlobalSessionManager.cs#L43-L44
sentry-dotnet/src/Sentry/GlobalSessionManager.cs
Lines 43 to 44 in a7ff327
| // potential race conditions. | |
| private void PersistSession(SessionUpdate update, DateTimeOffset? pauseTimestamp = null, bool pendingUnhandled = false) |
src/Sentry/GlobalSessionManager.cs#L290-L305
sentry-dotnet/src/Sentry/GlobalSessionManager.cs
Lines 290 to 305 in a7ff327
| public void PauseSession() | |
| { | |
| if (_currentSession is not { } session) | |
| { | |
| _options.LogWarning("Attempted to pause a session, but a session has not been started."); | |
| return; | |
| } | |
| _options.LogInfo("Pausing session (SID: {0}; DID: {1}).", session.Id, session.DistinctId); | |
| var now = _clock.GetUtcNow(); | |
| _lastPauseTimestamp = now; | |
| PersistSession(session.CreateUpdate(false, now), now); | |
| } | |
| public IReadOnlyList<SessionUpdate> ResumeSession() |
Resolves #4632
Followup on #4633
Problem
Sessions that end in
Unhandledcan later on still crash. Updating a session with an end status is a terminal operation, see getsentry/sentry-docs#15086 and once updated, sessions must not receive further updates. We need to make sure the SDK finishes a session only when it is confident, that a session is actually finished.Proposal
Instead of updating (and ending) a session with
SessionEndStatus.Unhandledright away we persist the state to disk. We can make use of the fact that there is a separation betweenPersistedSessionUpdateandSessionUpdate.PersistedSessionUpdateis used locally only, the SDK does not send this to Sentry and we can add a flagPendingUnhandledto itSessionUpdateis the actual update the SDK sends to SentryThe flow is as follows:
Unhandledand the update is persisted to diskCrashed, overwriting the pending unhandled stateExitedwithUnhandled.TODO
Currently, the SDK relies on restarts of the application to send the
SessionEndStatus. Ideally, we would also hook into some shutdown mechanism, or at the very least, closing the SDK should also end any active sessions.