🐛 [RUMF-997] dont take a FullSnapshot on view creation during session renew #1011
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A full snapshot was incorrectly taken when renewing the session because
`startRecording` is called before initializing the recorder. So when a
session is renewed, we first create a new view then we stop recording:
```
notify SESSION_RENEWED
-> notify VIEW_CREATED (trackViews)
-> takeFullSnapshot() (startRecording)
-> stopRecording() (recorderApi)
-> startRecording() (recorderApi)
```
This commit uses the newly introduced lifecycle event triggered before
SESSION_RENEWED to stop recording when the session is expired:
```
notify SESSION_EXPIRED
-> stopRecording()
notify SESSION_RENEWED
-> notify VIEW_CREATED
-> (nothing happens since recording is stopped)
-> startRecording()
```
to ensure a single fullsnapshot is taken when renewing the session
webNeat
approved these changes
Aug 24, 2021
| @@ -62,12 +62,14 @@ export function makeRecorderApi(startRecordingImpl: StartRecording): RecorderApi | |||
| session: RumSession, | |||
| parentContexts: ParentContexts | |||
| ) => { | |||
| lifeCycle.subscribe(LifeCycleEventType.SESSION_RENEWED, () => { | |||
| lifeCycle.subscribe(LifeCycleEventType.SESSION_EXPIRED, () => { | |||
| if (state.status === RecorderStatus.Started) { | |||
| stopStrategy() | |||
| state = { status: RecorderStatus.IntentToStart } | |||
There was a problem hiding this comment.
If a session expired, we might not need to restart it if it's now LITE no?
There was a problem hiding this comment.
I think what you're referring to is a few lines below in the startStrategy
browser-sdk/packages/rum/src/boot/recorderApi.ts
Lines 78 to 82 in b38ae1d
There was a problem hiding this comment.
Yes start does not necessarily start recording 😅 it is the function exposed to the customer, so if it is called when the session has no replay plan, it will just set the state to back IntentToStart.
amortemousque
approved these changes
Aug 25, 2021
| // This implementation is "good enough" for our use-cases. Improving this is not trivial, | ||
| // primarily because multiple instances of the SDK may be managing the same session cookie at | ||
| // the same time, for example when using Logs and RUM on the same page, or opening multiple tabs | ||
| // on the same domain. |
| @@ -62,12 +62,14 @@ export function makeRecorderApi(startRecordingImpl: StartRecording): RecorderApi | |||
| session: RumSession, | |||
| parentContexts: ParentContexts | |||
| ) => { | |||
| lifeCycle.subscribe(LifeCycleEventType.SESSION_RENEWED, () => { | |||
| lifeCycle.subscribe(LifeCycleEventType.SESSION_EXPIRED, () => { | |||
| if (state.status === RecorderStatus.Started) { | |||
| stopStrategy() | |||
| state = { status: RecorderStatus.IntentToStart } | |||
There was a problem hiding this comment.
I think what you're referring to is a few lines below in the startStrategy
browser-sdk/packages/rum/src/boot/recorderApi.ts
Lines 78 to 82 in b38ae1d
…n-view-creation-during-session-renew
BenoitZugmeyer
deleted the
benoit/dont-take-fs-on-view-creation-during-session-renew
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
When renewing the session from replay to lite (for example), a full snapshot was taken on the lite session, because the recording was stopped a tiny bit too late (after the new view creation)
Changes
This PR introduces a new lifecycle event
SESSION_EXPIREDto have a chance to do some cleanup before the session is renewed. It uses it to stop recording.Testing
Manual, e2e (no unit test because we don't have any test at this level: it involves the whole RUM and recording stack)
I have gone over the contributing documentation.