getsentry / getsentry/sentry-javascript
Session immutability constraints are violated
- Lenguaje dominante
- TypeScript
- Estrellas
- 8.7k
- Forks
- 1.8k
- Merge medio
- 1 d 17 h
- PR fusionados (30 d)
- 515
Descripción
### Is there an existing issue for this?
- [x] I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
- [x] I have reviewed the documentation https://docs.sentry.io/
- [x] 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
```js
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](https://develop.sentry.dev/sdk/telemetry/sessions/#attribute-immutability).
### 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`.
1. 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 #19341 but was raised by users as a bug (via support): https://github.com/getsentry/sentry-javascript/issues/19317
2. 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.
3. 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 with `did` where both actually resemble the same session)
4. 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.
5. 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.
6. 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` if we want to make it explicit, or a `() => Promise` 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.
Guía de contribución
Evaluación
Este issue todavía no se ha evaluado.