Session immutability constraints are violated #19777
Description
Is there an existing issue for this?
- I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
- I have reviewed the documentation https://docs.sentry.io/
- I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases
How do you use Sentry?
Sentry Saas (sentry.io)
Which SDK are you using?
@sentry/browser
SDK Version
latest
Framework Version
N/A
Link to Sentry event
No response
Reproduction Example/SDK Setup
Sentry.init({...});
setTimeout(() => {
Sentry.setUser({...});
}, 2000)Steps to Reproduce
see above
Expected Result
Sessions' attributes like did, release, environment or timestamp MUST NOT be mutated after a session (identified by sid) was sent for the first time. Doing so, causes our session metrics extraction to incorrectly categorize future session updates, which leads to double-counted sessions and thus incorrect release health metrics (e.g. healthy, crashed, crash-free sessions).
See develop spec.
Actual Result
Today, our SDK does not enforce immutability. On the contrary, a recent change (my mistake, see #19341) even explicitly sends a session update after a user was set on the isolation scope. Though worth noting, anyone could call updateSession before, causing indirect mutations for sessions when we send status updates (exited, crashed, etc).
Additional Context
Ideally, we could update a session safely but sadly this is not supported by our current (and possibly future) data storage layer.
Solution Brainstorm
There are several strategies on how we can long-term fix this, but all of them come with problems and tradeoffs.
Important: Every proposed strategy includes enforcing immutability (e.g. in updateSession), treating session attributes as "frozen" once the first envelope was sent for a specific sid.
-
Do nothing about missing data (besides enforced immutability). The cheapest and spec-correct option. Impacts "Crash free users" and related metrics relying on
did. This was largely the behaviour prior to fix(browser): Ensure user id is consistently added to sessions #19341 but was raised by users as a bug (via support): User information no available in the release session #19317 -
Defer sending of first session by grace period (e.g. 5sec). Any data (like setUser) set in the meantime will be included in the first envelope. Still leaves a chance for too late data but increases chances. Largest con: Any kind of short page visits are not tracked at all anymore. Could decrease overall session count and leave some sessions completely untracked.
-
Restart session on user change. This is what the spec suggests but in reality, it leads to 1. double counted "sessions" (one very short one without
did, one longer one withdidwhere both actually resemble the same session) -
Defer sending + Restart (combines the pros of 2 and 3) but still leaves room for untracked sessions as well as double-counted sessions. The latter to a smaller extent than 3 alone.
-
Telemetry-driven first session send: Only send the first session envelope once the first telemetry item (e.g. error, transaction, log, metric) was sent. Might lead to longer grace periods than 2, but might also do the opposite. Leaves a much higher chance for untracked sessions, heavily skewing healthy session metrics. Especially for errors-only users.
-
Provide API for users to explicitly force our SDK to wait for sending a session until users deem the session ready to be sent. This could be a
Promise<User>if we want to make it explicit, or a() => Promise<void>callback. In either case we would await a promise and only afterwards send a session. I could see us implementing this down the line as a custom strategy but we need one of 1-5 to handle the default case.
Priority
React with 👍 to help prioritize this issue. Please use comments to provide useful context, avoiding +1 or me too, to help us triage it.