Uh oh!
There was an error while loading. Please reload this page.
fix(replay): Fully stop & restart session when it expires - #8834
Conversation
| // We can't move `_isEnabled` after awaiting a flush, otherwise we can | ||
| // enter into an infinite loop when `stop()` is called while flushing. | ||
| this._isEnabled = false; |
There was a problem hiding this comment.
I moved this out of the try-catch, as otherwise this lead to weird race conditions. I guess by having this in the try-catch the timing semantics are slightly different. I had a bunch of cases where stop was then called twice at the exact same time, which was fixed by moving this out here 🤔
There was a problem hiding this comment.
Ah right, since try/catch inside of an async function is sugar on top of promises.
| // Need to set as enabled before we start recording, as `record()` can trigger a flush with a new checkout | ||
| this._isEnabled = true; | ||
| this._isPaused = false; |
There was a problem hiding this comment.
making sure we are also unpaused here, when starting new.
| */ | ||
| private async _runFlush(): Promise<void> { | ||
| if (!this.session || !this.eventBuffer) { | ||
| const replayId = this.getSessionId(); |
There was a problem hiding this comment.
capturing the replayId to use at the very top here. With this, if this runs out of sync while processing, we should still never send data to the wrong replay ID, at least.
| // if this changed in the meanwhile, e.g. because the session was refreshed or similar, we abort here | ||
| if (replayId !== this.getSessionId()) { | ||
| return; |
There was a problem hiding this comment.
Just bailing out here, i think that should be fine? If this changed in the meanwhile, if we flush again later we should have discarded all the stuff before already...?
There was a problem hiding this comment.
There may be an edgecase that we will have to deal with separately where we have an ongoing/suspended flush, and a new session is created w/ checkout snapshot that gets added to buffer and then discarded here
| this._maybeSaveSession(); | ||
| // Note this empties the event buffer regardless of outcome of sending replay | ||
| const recordingData = await this.eventBuffer.finish(); |
There was a problem hiding this comment.
Moving this down here, the idea is:
- We capture the timestamp, event context etc. first
- Then, if the buffer flushing takes a long time, we don't care, even if in the meanwhile the session was refreshed we can still send the stuff to the old session. Also timestamps should be less prone to be too late due to buffer flushing, and should actually be more correct because it is the time of the flush and not whenever the buffer finished.
| * If this is false, the session should not be refreshed when it was inactive. | ||
| * This can be the case if you had a buffered session which is now recording because an error happened. | ||
| */ | ||
| shouldRefresh: boolean; |
There was a problem hiding this comment.
Only now did I notice that we can literally replace this everywhere by just checking segmentId === 0 🤦
size-limit report 📦
|
bc72625 to
da7f8d3Compare| } | ||
| logInfoNextTick('[Replay] Session in sessionStorage is expired, creating new one...'); | ||
| return createSession(sessionOptions, { previousSessionId: existingSession.id }); |
Check failure
Code scanning / CodeQL
Insecure randomness
| * called outside of constructor. | ||
| */ | ||
| public initializeSampling(): void { | ||
| public initializeSampling(previousSessionId?: string): void { |
Check failure
Code scanning / CodeQL
Insecure randomness
| try { | ||
| logInfo( | ||
| `[Replay] Stopping Replay${reason ? ` triggered by ${reason}` : ''}`, | ||
| `[Replay] Stopping Replay${reason ? ` triggered by ${reason}` : ''} ${new Date().toISOString()} ${ |
There was a problem hiding this comment.
Should we keep the date string here?
| // if this changed in the meanwhile, e.g. because the session was refreshed or similar, we abort here | ||
| if (replayId !== this.getSessionId()) { | ||
| return; |
There was a problem hiding this comment.
There may be an edgecase that we will have to deal with separately where we have an ongoing/suspended flush, and a new session is created w/ checkout snapshot that gets added to buffer and then discarded here
f70f354 to
4b185e9Compareb571601 to
7bc5c80Compare
This is a draft based on top of #8813, once that is merged this can be pointed to develop.
This PR changes the behavior when a session is expired to fully stop & restart the replay.
This means we just re-sample based on sample rates and start a completely new session in that case.
Supersedes #8407