fix(replay): Fully stop & restart session when it expires #8834

mydea · 2023-08-17T15:15:31Z

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

mydea · 2023-08-17T15:16:34Z

packages/replay/src/replay.ts


+    // 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;


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 🤔

Ah right, since try/catch inside of an async function is sugar on top of promises.

mydea · 2023-08-17T15:16:51Z

packages/replay/src/replay.ts


    // 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;


making sure we are also unpaused here, when starting new.

mydea · 2023-08-17T15:17:39Z

packages/replay/src/replay.ts

   */
  private async _runFlush(): Promise<void> {
-    if (!this.session || !this.eventBuffer) {
+    const replayId = this.getSessionId();


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.

mydea · 2023-08-17T15:18:13Z

packages/replay/src/replay.ts


+    // if this changed in the meanwhile, e.g. because the session was refreshed or similar, we abort here
+    if (replayId !== this.getSessionId()) {
+      return;


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

mydea · 2023-08-17T15:23:23Z

packages/replay/src/replay.ts

      this._maybeSaveSession();

+      // Note this empties the event buffer regardless of outcome of sending replay
+      const recordingData = await this.eventBuffer.finish();


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.

mydea · 2023-08-17T15:24:12Z

packages/replay/src/types/replay.ts

-   * 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;


Only now did I notice that we can literally replace this everywhere by just checking segmentId === 0 🤦

github-actions · 2023-08-17T15:32:49Z

size-limit report 📦

Path	Size
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped)	75.29 KB (-0.1% 🔽)
@sentry/browser (incl. Tracing) - Webpack (gzipped)	31.29 KB (+0.06% 🔺)
@sentry/browser - Webpack (gzipped)	21.89 KB (+0.07% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped)	70 KB (-0.14% 🔽)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped)	28.4 KB (-0.07% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped)	20.47 KB (-0.05% 🔽)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed)	221.17 KB (-0.17% 🔽)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed)	85.82 KB (+0.01% 🔺)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed)	60.67 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped)	31.28 KB (-0.05% 🔽)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped)	75.32 KB (-0.1% 🔽)
@sentry/react - Webpack (gzipped)	21.92 KB (+0.07% 🔺)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped)	93.18 KB (-0.08% 🔽)
@sentry/nextjs Client - Webpack (gzipped)	50.85 KB (+0.04% 🔺)

packages/replay/src/session/loadOrCreateSession.ts

+  }
+
+  logInfoNextTick('[Replay] Session in sessionStorage is expired, creating new one...');
+  return createSession(sessionOptions, { previousSessionId: existingSession.id });


packages/replay/src/replay.ts

   * called outside of constructor.
   */
-  public initializeSampling(): void {
+  public initializeSampling(previousSessionId?: string): void {


billyvg · 2023-08-18T12:47:24Z

packages/replay/src/replay.ts

    try {
      logInfo(
-        `[Replay] Stopping Replay${reason ? ` triggered by ${reason}` : ''}`,
+        `[Replay] Stopping Replay${reason ? ` triggered by ${reason}` : ''} ${new Date().toISOString()} ${


Should we keep the date string here?

billyvg · 2023-08-18T13:10:47Z

packages/replay/src/replay.ts


+    // if this changed in the meanwhile, e.g. because the session was refreshed or similar, we abort here
+    if (replayId !== this.getSessionId()) {
+      return;


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

mydea requested a review from billyvg August 17, 2023 15:15

mydea self-assigned this Aug 17, 2023

mydea mentioned this pull request Aug 17, 2023

feat(replay): Re-sample sessions after they expire #8407

Closed

mydea commented Aug 17, 2023

View reviewed changes

Base automatically changed from fn/replay-restart-buffer to develop August 18, 2023 06:12

mydea force-pushed the fn/isolate-eventBuffer branch from bc72625 to da7f8d3 Compare August 18, 2023 06:17

mydea marked this pull request as ready for review August 18, 2023 06:17

github-advanced-security bot found potential problems Aug 18, 2023

View reviewed changes

billyvg approved these changes Aug 18, 2023

View reviewed changes

mydea force-pushed the fn/isolate-eventBuffer branch 2 times, most recently from f70f354 to 4b185e9 Compare September 1, 2023 09:15

mydea added 2 commits September 11, 2023 09:37

fix(replay): Fully stop & restart session when it expires

befae60

simplify checkAndHandleExpiredSession check

7bc5c80

mydea force-pushed the fn/isolate-eventBuffer branch from b571601 to 7bc5c80 Compare September 11, 2023 07:37

PR cleanup

7b34dd7

mydea merged commit 30c4540 into develop Sep 11, 2023

mydea deleted the fn/isolate-eventBuffer branch September 11, 2023 08:56

Uh oh!

fix(replay): Fully stop & restart session when it expires #8834

fix(replay): Fully stop & restart session when it expires #8834

Uh oh!

Conversation

mydea commented Aug 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

Check failure

Check failure

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Aug 17, 2023 •

edited

Loading