fix(replay): Do not renew session in error mode #6948
Conversation
size-limit report 📦
|
| @@ -369,6 +369,52 @@ describe('Integration | errorSampleRate', () => { | |||
| ]), | |||
| }); | |||
| }); | |||
|
|
|||
| it('stops replay when session expires', async () => { | |||
There was a problem hiding this comment.
We only want this for the error mode, right? Do we have tests in the session recording mode that check the expected behavior there?
There was a problem hiding this comment.
I think this existing test should cover this: https://github.com/getsentry/sentry-javascript/blob/master/packages/replay/test/integration/session.test.ts#L106
packages/replay/src/replay.ts
Outdated
| // We know this is set, because it is always set in _loadSession | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| if (!this.session!.sampled) { | ||
| this.stop(); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Should this check be done in _loadSession?
There was a problem hiding this comment.
Actually moved this to a new method.
packages/replay/src/replay.ts
Outdated
| if (!this.session || !this.session.sampled) { | ||
| this.stop(); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Maybe have this call loadSession?
packages/replay/src/replay.ts
Outdated
| private _loadAndCheckSession(expiry?: number): boolean { | ||
| this._loadSession(expiry); | ||
|
|
||
| // We know this is set, because it is always set in _loadSession | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| if (!this.session!.sampled) { | ||
| this.stop(); | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
What are the cases where we want to load session but not check sampled?
There was a problem hiding this comment.
I think on start, because there we do some more stuff (incl. also checking this). but maybe actually we can also just use this there, looking at it again - I'll unify it!
mydea
force-pushed
the
fn/replay-error-mode
branch
from
256faac
to
2489b45
Compare
| @@ -228,6 +230,10 @@ export class ReplayContainer implements ReplayContainerInterface { | |||
| * does not support a teardown | |||
| */ | |||
| public stop(): void { | |||
| if (!this._isEnabled) { | |||
There was a problem hiding this comment.
Added this check to ensure we do not stop multiple times.
mydea
force-pushed
the
fn/replay-error-mode
branch
from
2489b45
to
fcb805a
Compare
Currently, when you have a replay session running because of an error, and the session expires (after 15min of inactivity or 60min of activity), we just create a new session (again in error mode) and continue recording.
This is confusing because from a users perspective, they see replays in error mode but with (potentially) no error connected to it.
This PR changes this behavior - when in error mode, and the session expires, we stop the replay.