fix(replay): Streamline session creation/refresh (#8813)

We've been using `_loadAndCheckSession` both in initial session setup as well as when checking for expiration of session. This leads to some not-so-optimized stuff, as we kind of have to do double duty in there (e.g. we constantly re-assign the session etc). This streamlines this by splitting this into: * `_initializeSessionForSampling()`: Only called in `initializeSampling()` * `_checkSession()`: Called everywhere else, assumes we have a session setup yet Only the former actually looks into sessionStorage, the latter can assume we always have a session already. This also extends the behavior so that if we fetch a `buffer` session from storage and segment_id > 0, we start the session in `session` mode. Without this, we could theoretically run into endless sessions if the user keeps refreshing and keeps having errors, leading to continuous switchovers from buffer>session mode.
getsentry · Aug 18, 2023 · 4a4df0d · 4a4df0d
1 parent a985738
commit 4a4df0d
Show file tree

Hide file tree

Showing 21 changed files with 950 additions and 435 deletions.
diff --git a/packages/browser-integration-tests/suites/replay/bufferModeReload/init.js b/packages/browser-integration-tests/suites/replay/bufferModeReload/init.js
@@ -0,0 +1,17 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+window.Replay = new Sentry.Replay({
+  flushMinDelay: 200,
+  flushMaxDelay: 200,
+  minReplayDuration: 0,
+});
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  sampleRate: 1,
+  replaysSessionSampleRate: 0.0,
+  replaysOnErrorSampleRate: 1.0,
+
+  integrations: [window.Replay],
+});
diff --git a/packages/browser-integration-tests/suites/replay/bufferModeReload/template.html b/packages/browser-integration-tests/suites/replay/bufferModeReload/template.html
@@ -0,0 +1,10 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8" />
+  </head>
+  <body>
+    <button onclick="console.log('Test log 1')" id="button1">Click me</button>
+    <button onclick="Sentry.captureException('test error')" id="buttonError">Click me</button>
+  </body>
+</html>
diff --git a/packages/browser-integration-tests/suites/replay/bufferModeReload/test.ts b/packages/browser-integration-tests/suites/replay/bufferModeReload/test.ts
@@ -0,0 +1,51 @@
+import { expect } from '@playwright/test';
+
+import { sentryTest } from '../../../utils/fixtures';
+import {
+  getReplaySnapshot,
+  shouldSkipReplayTest,
+  waitForReplayRequest,
+  waitForReplayRunning,
+} from '../../../utils/replayHelpers';
+
+sentryTest('continues buffer session in session mode after error & reload', async ({ getLocalTestPath, page }) => {
+  if (shouldSkipReplayTest()) {
+    sentryTest.skip();
+  }
+
+  const reqPromise1 = waitForReplayRequest(page, 0);
+
+  await page.route('https://dsn.ingest.sentry.io/**/*', route => {
+    return route.fulfill({
+      status: 200,
+      contentType: 'application/json',
+      body: JSON.stringify({ id: 'test-id' }),
+    });
+  });
+
+  const url = await getLocalTestPath({ testDir: __dirname });
+
+  await page.goto(url);
+
+  // buffer session captures an error & switches to session mode
+  await page.click('#buttonError');
+  await new Promise(resolve => setTimeout(resolve, 300));
+  await reqPromise1;
+
+  await waitForReplayRunning(page);
+  const replay1 = await getReplaySnapshot(page);
+
+  expect(replay1.recordingMode).toEqual('session');
+  expect(replay1.session?.sampled).toEqual('buffer');
+  expect(replay1.session?.segmentId).toBeGreaterThan(0);
+
+  // Reload to ensure the session is correctly recovered from sessionStorage
+  await page.reload();
+
+  await waitForReplayRunning(page);
+  const replay2 = await getReplaySnapshot(page);
+
+  expect(replay2.recordingMode).toEqual('session');
+  expect(replay2.session?.sampled).toEqual('buffer');
+  expect(replay2.session?.segmentId).toBeGreaterThan(0);
+});
diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts
@@ -18,7 +18,8 @@ import { handleKeyboardEvent } from './coreHandlers/handleKeyboardEvent';
 import { setupPerformanceObserver } from './coreHandlers/performanceObserver';
 import { createEventBuffer } from './eventBuffer';
 import { clearSession } from './session/clearSession';
-import { getSession } from './session/getSession';
+import { loadOrCreateSession } from './session/loadOrCreateSession';
+import { maybeRefreshSession } from './session/maybeRefreshSession';
 import { saveSession } from './session/saveSession';
 import type {
   AddEventResult,
@@ -228,28 +229,24 @@ export class ReplayContainer implements ReplayContainerInterface {
 
     // Otherwise if there is _any_ sample rate set, try to load an existing
     // session, or create a new one.
-    const isSessionSampled = this._loadAndCheckSession();
-
-    if (!isSessionSampled) {
-      // This should only occur if `errorSampleRate` is 0 and was unsampled for
-      // session-based replay. In this case there is nothing to do.
-      return;
-    }
+    this._initializeSessionForSampling();
 
     if (!this.session) {
       // This should not happen, something wrong has occurred
       this._handleException(new Error('Unable to initialize and create session'));
       return;
     }
 
-    if (this.session.sampled && this.session.sampled !== 'session') {
-      // If not sampled as session-based, then recording mode will be `buffer`
-      // Note that we don't explicitly check if `sampled === 'buffer'` because we
-      // could have sessions from Session storage that are still `error` from
-      // prior SDK version.
-      this.recordingMode = 'buffer';
+    if (this.session.sampled === false) {
+      // This should only occur if `errorSampleRate` is 0 and was unsampled for
+      // session-based replay. In this case there is nothing to do.
+      return;
     }
 
+    // If segmentId > 0, it means we've previously already captured this session
+    // In this case, we still want to continue in `session` recording mode
+    this.recordingMode = this.session.sampled === 'buffer' && this.session.segmentId === 0 ? 'buffer' : 'session';
+
     logInfoNextTick(
       `[Replay] Starting replay in ${this.recordingMode} mode`,
       this._options._experiments.traceInternals,
@@ -276,19 +273,20 @@ export class ReplayContainer implements ReplayContainerInterface {
 
     logInfoNextTick('[Replay] Starting replay in session mode', this._options._experiments.traceInternals);
 
-    const previousSessionId = this.session && this.session.id;
-
-    const { session } = getSession({
-      timeouts: this.timeouts,
-      stickySession: Boolean(this._options.stickySession),
-      currentSession: this.session,
-      // This is intentional: create a new session-based replay when calling `start()`
-      sessionSampleRate: 1,
-      allowBuffering: false,
-      traceInternals: this._options._experiments.traceInternals,
-    });
+    const session = loadOrCreateSession(
+      this.session,
+      {
+        timeouts: this.timeouts,
+        traceInternals: this._options._experiments.traceInternals,
+      },
+      {
+        stickySession: this._options.stickySession,
+        // This is intentional: create a new session-based replay when calling `start()`
+        sessionSampleRate: 1,
+        allowBuffering: false,
+      },
+    );
 
-    session.previousSessionId = previousSessionId;
     this.session = session;
 
     this._initializeRecording();
@@ -305,18 +303,19 @@ export class ReplayContainer implements ReplayContainerInterface {
 
     logInfoNextTick('[Replay] Starting replay in buffer mode', this._options._experiments.traceInternals);
 
-    const previousSessionId = this.session && this.session.id;
-
-    const { session } = getSession({
-      timeouts: this.timeouts,
-      stickySession: Boolean(this._options.stickySession),
-      currentSession: this.session,
-      sessionSampleRate: 0,
-      allowBuffering: true,
-      traceInternals: this._options._experiments.traceInternals,
-    });
+    const session = loadOrCreateSession(
+      this.session,
+      {
+        timeouts: this.timeouts,
+        traceInternals: this._options._experiments.traceInternals,
+      },
+      {
+        stickySession: this._options.stickySession,
+        sessionSampleRate: 0,
+        allowBuffering: true,
+      },
+    );
 
-    session.previousSessionId = previousSessionId;
     this.session = session;
 
     this.recordingMode = 'buffer';
@@ -427,7 +426,7 @@ export class ReplayContainer implements ReplayContainerInterface {
    * new DOM checkout.`
    */
   public resume(): void {
-    if (!this._isPaused || !this._loadAndCheckSession()) {
+    if (!this._isPaused || !this._checkSession()) {
       return;
     }
 
@@ -535,7 +534,7 @@ export class ReplayContainer implements ReplayContainerInterface {
     if (!this._stopRecording) {
       // Create a new session, otherwise when the user action is flushed, it
       // will get rejected due to an expired session.
-      if (!this._loadAndCheckSession()) {
+      if (!this._checkSession()) {
         return;
       }
 
@@ -634,7 +633,7 @@ export class ReplayContainer implements ReplayContainerInterface {
 
     // --- There is recent user activity --- //
     // This will create a new session if expired, based on expiry length
-    if (!this._loadAndCheckSession()) {
+    if (!this._checkSession()) {
       return;
     }
 
@@ -751,31 +750,63 @@ export class ReplayContainer implements ReplayContainerInterface {
 
   /**
    * Loads (or refreshes) the current session.
+   */
+  private _initializeSessionForSampling(): void {
+    // Whenever there is _any_ error sample rate, we always allow buffering
+    // Because we decide on sampling when an error occurs, we need to buffer at all times if sampling for errors
+    const allowBuffering = this._options.errorSampleRate > 0;
+
+    const session = loadOrCreateSession(
+      this.session,
+      {
+        timeouts: this.timeouts,
+        traceInternals: this._options._experiments.traceInternals,
+      },
+      {
+        stickySession: this._options.stickySession,
+        sessionSampleRate: this._options.sessionSampleRate,
+        allowBuffering,
+      },
+    );
+
+    this.session = session;
+  }
+
+  /**
+   * Checks and potentially refreshes the current session.
    * Returns false if session is not recorded.
    */
-  private _loadAndCheckSession(): boolean {
-    const { type, session } = getSession({
-      timeouts: this.timeouts,
-      stickySession: Boolean(this._options.stickySession),
-      currentSession: this.session,
-      sessionSampleRate: this._options.sessionSampleRate,
-      allowBuffering: this._options.errorSampleRate > 0 || this.recordingMode === 'buffer',
-      traceInternals: this._options._experiments.traceInternals,
-    });
+  private _checkSession(): boolean {
+    // If there is no session yet, we do not want to refresh anything
+    // This should generally not happen, but to be safe....
+    if (!this.session) {
+      return false;
+    }
+
+    const currentSession = this.session;
+
+    const newSession = maybeRefreshSession(
+      currentSession,
+      {
+        timeouts: this.timeouts,
+        traceInternals: this._options._experiments.traceInternals,
+      },
+      {
+        stickySession: Boolean(this._options.stickySession),
+        sessionSampleRate: this._options.sessionSampleRate,
+        allowBuffering: this._options.errorSampleRate > 0,
+      },
+    );
+
+    const isNew = newSession.id !== currentSession.id;
 
     // If session was newly created (i.e. was not loaded from storage), then
     // enable flag to create the root replay
-    if (type === 'new') {
+    if (isNew) {
       this.setInitialState();
+      this.session = newSession;
     }
 
-    const currentSessionId = this.getSessionId();
-    if (session.id !== currentSessionId) {
-      session.previousSessionId = currentSessionId;
-    }
-
-    this.session = session;
-
     if (!this.session.sampled) {
       void this.stop({ reason: 'session not refreshed' });
       return false;

diff --git a/packages/replay/src/session/Session.ts b/packages/replay/src/session/Session.ts
@@ -14,6 +14,7 @@ export function makeSession(session: Partial<Session> & { sampled: Sampled }): S
   const segmentId = session.segmentId || 0;
   const sampled = session.sampled;
   const shouldRefresh = typeof session.shouldRefresh === 'boolean' ? session.shouldRefresh : true;
+  const previousSessionId = session.previousSessionId;
 
   return {
     id,
@@ -22,5 +23,6 @@ export function makeSession(session: Partial<Session> & { sampled: Sampled }): S
     segmentId,
     sampled,
     shouldRefresh,
+    previousSessionId,
   };
 }
diff --git a/packages/replay/src/session/createSession.ts b/packages/replay/src/session/createSession.ts
@@ -15,10 +15,14 @@ export function getSessionSampleType(sessionSampleRate: number, allowBuffering:
  * that all replays will be saved to as attachments. Currently, we only expect
  * one of these Sentry events per "replay session".
  */
-export function createSession({ sessionSampleRate, allowBuffering, stickySession = false }: SessionOptions): Session {
+export function createSession(
+  { sessionSampleRate, allowBuffering, stickySession = false }: SessionOptions,
+  { previousSessionId }: { previousSessionId?: string } = {},
+): Session {
   const sampled = getSessionSampleType(sessionSampleRate, allowBuffering);
   const session = makeSession({
     sampled,
+    previousSessionId,
   });
 
   if (stickySession) {

diff --git a/packages/replay/src/session/getSession.ts b/packages/replay/src/session/getSession.ts