getsentry / getsentry/sentry-javascript

Session immutability constraints are violated

オープン
#19,777 コメント 2 件 リアクション 0 件 担当者 1 名 @Lms24 が担当を希望しています GitHub で見る
Browser Bug
主要言語
TypeScript
スター
8.7k
フォーク
1.8k
平均マージ
1日 17時間
マージ済み PR(30日)
515

説明

### 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.

コントリビューションガイド

コントリビューションガイドを開く

評価

この issue はまだ評価されていません。

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。