fix: teardown and restart recording on session id change #1411
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Size Change: +3.72 kB (+0.31%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
pauldambra
changed the title
| this._tryTakeFullSnapshot() | ||
| if (sessionIdChanged || windowIdChanged) { | ||
| this.stopRecording() | ||
| this.startIfEnabledOrStop() |
There was a problem hiding this comment.
Haven't looked into it but wondering if we need to consider anything here with regards to ingestion controls (sampling, feature flags, minimum duration) that might affect the new recording not starting
There was a problem hiding this comment.
sampling and feature flags should just work...
min duration is interesting... i don't know if we'd restart the counter - we should... 👀
There was a problem hiding this comment.
yep... session start is read from storage, most recent snapshot data from the buffer, so we're safe with min duration too 👍
| @@ -426,26 +427,6 @@ describe('SessionRecording', () => { | |||
| }) | |||
| }) | |||
|
|
|||
| it('when the first event is an incremental it does take a manual full snapshot', () => { | |||
There was a problem hiding this comment.
not manually fangling this any more so we don't need to test it happens
| $snapshot_data: [ | ||
| createFullSnapshot(), |
There was a problem hiding this comment.
throughout these tests we're no longer manually adding full snapshots so we don't have to assert their presence
sometimes we see the artificial full snapshot on session idle timeout end up in the previous session not the new session - no bueno
in an attempt to not keep adding conditionals here i want to explore if tearing down rrweb and restarting recording is more stable
locally this seems way more reliable, but I want to get #1416 in so I can retest without that noise