feat(replay): Change the behavior of error-based sampling

WIP

Author: billyvg

packages/replay/src/integration.ts

@@ -169,24 +169,15 @@ Sentry.init({ replaysOnErrorSampleRate: ${errorSampleRate} })`,
   }
 
   /**
-   * We previously used to create a transaction in `setupOnce` and it would
-   * potentially create a transaction before some native SDK integrations have run
-   * and applied their own global event processor. An example is:
-   * https://github.com/getsentry/sentry-javascript/blob/b47ceafbdac7f8b99093ce6023726ad4687edc48/packages/browser/src/integrations/useragent.ts
-   *
-   * So we call `replay.setup` in next event loop as a workaround to wait for other
-   * global event processors to finish. This is no longer needed, but keeping it
-   * here to avoid any future issues.
+   * Setup and initialize replay container
    */
   public setupOnce(): void {
     if (!isBrowser()) {
       return;
     }
 
     this._setup();
-
-    // XXX: See method comments above
-    setTimeout(() => this.start());
+    this._initialize();
   }
 
   /**
@@ -226,6 +217,27 @@ Sentry.init({ replaysOnErrorSampleRate: ${errorSampleRate} })`,
     return this._replay.flushImmediate();
   }
 
+  /**
+   * Initializes replay.
+   */
+  protected _initialize(): void {
+    // Once upon a time, we tried to create a transaction in `setupOnce` and it would
+    // potentially create a transaction before some native SDK integrations have run
+    // and applied their own global event processor. An example is:
+    // https://github.com/getsentry/sentry-javascript/blob/b47ceafbdac7f8b99093ce6023726ad4687edc48/packages/browser/src/integrations/useragent.ts
+    //
+    // So we call `replay.initialize()` in next event loop as a workaround to wait for other
+    // global event processors to finish. This is no longer needed, but keeping it
+    // here to avoid any future issues.
+    setTimeout(() => {
+      if (!this._replay) {
+        return;
+      }
+
+      this._replay.initializeSampling();
+    });
+  }
+
   /** Setup the integration. */
   private _setup(): void {
     // Client is not available in constructor, so we need to wait until setupOnce

packages/replay/src/replay.ts

@@ -48,9 +48,10 @@ export class ReplayContainer implements ReplayContainerInterface {
   public session: Session | undefined;
 
   /**
-   * Recording can happen in one of two modes:
-   * * session: Record the whole session, sending it continuously
-   * * error: Always keep the last 60s of recording, and when an error occurs, send it immediately
+   * Recording can happen in one of three modes:
+   *   - session: Record the whole session, sending it continuously
+   *   - error: Always keep the last 60s of recording, and when an error occurs, send the replay
+   *   - buffer: Always keep the last 60s of recording, requires calling `capture()` to send the replay
    */
   public recordingMode: ReplayRecordingMode = 'session';
 
@@ -148,49 +149,72 @@ export class ReplayContainer implements ReplayContainerInterface {
   }
 
   /**
-   * Initializes the plugin.
-   *
-   * Creates or loads a session, attaches listeners to varying events (DOM,
-   * _performanceObserver, Recording, Sentry SDK, etc)
+   * Initializes the plugin based on configuration options. Should not be called outside of constructor
    */
-  public start(): void {
-    this.setInitialState();
+  public initializeSampling(): void {
+    const { errorSampleRate, sessionSampleRate } = this._options;
 
-    if (!this._loadAndCheckSession()) {
+    // if neither sample rate is > 0, then do nothing
+    if (errorSampleRate <= 0 && sessionSampleRate <= 0) {
+      return;
+    }
+
+    // Otherwise if there is _any_ sample rate set, try to load/create session
+    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;
     }
 
-    // If there is no session, then something bad has happened - can't continue
     if (!this.session) {
-      this._handleException(new Error('No session found'));
+      // This should not happen, something wrong has occurred
+      this._handleException(new Error('Unable to initialize and create session'));
       return;
     }
 
-    if (!this.session.sampled) {
-      // If session was not sampled, then we do not initialize the integration at all.
+    // Only call start if this session is sampled for session-based replays
+    if (this.session && this.session.sampled === 'session') {
+      this._initializeRecording();
       return;
     }
 
-    // If session is sampled for errors, then we need to set the recordingMode
-    // to 'error', which will configure recording with different options.
+    // If not sampled as session-based, it will always be sampled as
+    // error-based if `errorSampleRate` is > 0
     if (this.session.sampled === 'error') {
+      // If session is sampled for errors, then we need to set the recordingMode
+      // to 'error', which will configure recording with different options.
       this.recordingMode = 'error';
+      this._initializeRecording();
     }
 
-    // setup() is generally called on page load or manually - in both cases we
-    // should treat it as an activity
-    this._updateSessionActivity();
+    // There should be no other cases
+  }
 
-    this.eventBuffer = createEventBuffer({
-      useCompression: this._options.useCompression,
-    });
+  /**
+   * Create and start a replay.
+   *
+   * Creates or loads a session, attaches listeners to varying events (DOM,
+   * _performanceObserver, Recording, Sentry SDK, etc)
+   */
+  public start(): void {
+    // TODO: Should we allow you to call start if there is an existing replay in progress?
 
-    this._addListeners();
+    const previousSessionId = this.session && this.session.id;
 
-    // Need to set as enabled before we start recording, as `record()` can trigger a flush with a new checkout
-    this._isEnabled = true;
+    const { session } = getSession({
+      timeouts: this.timeouts,
+      stickySession: Boolean(this._options.stickySession),
+      currentSession: this.session,
+      sessionSampleRate: 1,
+      errorSampleRate: 0,
+    });
 
-    this.startRecording();
+    session.previousSessionId = previousSessionId;
+    this.session = session;
+
+    this._initializeRecording();
   }
 
   /**
@@ -430,6 +454,30 @@ export class ReplayContainer implements ReplayContainerInterface {
     this._context.urls.push(url);
   }
 
+  /**
+   * Initialize and start all listeners to varying events (DOM,
+   * Performance Observer, Recording, Sentry SDK, etc)
+   */
+  private _initializeRecording(): void {
+    this.setInitialState();
+
+    // this method is generally called on page load or manually - in both cases
+    // we should treat it as an activity
+    this._updateSessionActivity();
+
+    this.eventBuffer = createEventBuffer({
+      useCompression: this._options.useCompression,
+    });
+
+    // TODO: we should probably remove listeners before adding listeners
+    this._addListeners();
+
+    // Need to set as enabled before we start recording, as `record()` can trigger a flush with a new checkout
+    this._isEnabled = true;
+
+    this.startRecording();
+  }
+
   /** A wrapper to conditionally capture exceptions. */
   private _handleException(error: unknown): void {
     __DEBUG_BUILD__ && logger.error('[Replay]', error);

packages/replay/src/types.ts

@@ -436,6 +436,7 @@ export interface ReplayContainer {
   isEnabled(): boolean;
   isPaused(): boolean;
   getContext(): InternalEventContext;
+  initializeSampling(): void;
   start(): void;
   stop(reason?: string): void;
   pause(): void;

packages/replay/test/integration/errorSampleRate.test.ts

@@ -525,13 +525,14 @@ it('sends a replay after loading the session multiple times', async () => {
     REPLAY_SESSION_KEY,
     `{"segmentId":0,"id":"fd09adfc4117477abc8de643e5a5798a","sampled":"error","started":${BASE_TIMESTAMP},"lastActivity":${BASE_TIMESTAMP}}`,
   );
-  const { mockRecord, replay } = await resetSdkMock({
+  const { mockRecord, replay, integration } = await resetSdkMock({
     replayOptions: {
       stickySession: true,
     },
     autoStart: false,
   });
-  replay.start();
+  // @ts-ignore this is protected, but we want to call it for this test
+  integration._initialize();
 
   jest.runAllTimers();
 
@@ -546,6 +547,7 @@ it('sends a replay after loading the session multiple times', async () => {
   await new Promise(process.nextTick);
   jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY);
   await new Promise(process.nextTick);
+  await new Promise(process.nextTick);
 
   expect(replay).toHaveSentReplay({
     recordingData: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP, type: 2 }, TEST_EVENT]),

packages/replay/test/integration/sampling.test.ts

@@ -1,12 +1,15 @@
-import { mockRrweb, mockSdk } from '../index';
+import { resetSdkMock } from '../mocks/resetSdkMock';
 import { useFakeTimers } from '../utils/use-fake-timers';
 
 useFakeTimers();
 
 describe('Integration | sampling', () => {
+  beforeEach(() => {
+    jest.clearAllMocks();
+  });
+
   it('does nothing if not sampled', async () => {
-    const { record: mockRecord } = mockRrweb();
-    const { replay } = await mockSdk({
+    const { mockRecord, replay } = await resetSdkMock({
       replayOptions: {
         stickySession: true,
       },
@@ -20,14 +23,57 @@ describe('Integration | sampling', () => {
     const spyAddListeners = jest.spyOn(replay, '_addListeners');
     jest.runAllTimers();
 
-    expect(replay.session?.sampled).toBe(false);
-    expect(replay.getContext()).toEqual(
-      expect.objectContaining({
-        initialTimestamp: expect.any(Number),
-        initialUrl: 'http://localhost/',
-      }),
-    );
+    expect(replay.session).toBe(undefined);
+
+    // This is what the `_context` member is initialized with
+    expect(replay.getContext()).toEqual({
+      errorIds: new Set(),
+      traceIds: new Set(),
+      urls: [],
+      earliestEvent: null,
+      initialTimestamp: expect.any(Number),
+      initialUrl: '',
+    });
     expect(mockRecord).not.toHaveBeenCalled();
     expect(spyAddListeners).not.toHaveBeenCalled();
+
+    // TODO: Should we initialize recordingMode to something else?
+    expect(replay.recordingMode).toBe('session');
+  });
+
+  it('samples for error based session', async () => {
+    const { mockRecord, replay, integration } = await resetSdkMock({
+      replayOptions: {
+        stickySession: true,
+      },
+      sentryOptions: {
+        replaysSessionSampleRate: 0.0,
+        replaysOnErrorSampleRate: 1.0,
+      },
+      autoStart: false, // Needs to be false in order to spy on replay
+    });
+
+    // @ts-ignore private API
+    const spyAddListeners = jest.spyOn(replay, '_addListeners');
+
+    // @ts-ignore protected
+    integration._initialize();
+
+    jest.runAllTimers();
+
+    expect(replay.session?.id).toBeDefined();
+
+    // This is what the `_context` member is initialized with
+    expect(replay.getContext()).toEqual({
+      errorIds: new Set(),
+      earliestEvent: expect.any(Number),
+      initialTimestamp: expect.any(Number),
+      initialUrl: 'http://localhost/',
+      traceIds: new Set(),
+      urls: ['http://localhost/'],
+    });
+    expect(replay.recordingMode).toBe('error');
+    expect(spyAddListeners).toHaveBeenCalledTimes(1);
+    expect(mockRecord).toHaveBeenCalledTimes(1);
   });
 });

packages/replay/test/integration/session.test.ts

@@ -118,6 +118,7 @@ describe('Integration | session', () => {
   it('creates a new session if user has been idle for more than SESSION_IDLE_DURATION and comes back to click their mouse', async () => {
     const initialSession = { ...replay.session } as Session;
 
+    expect(mockRecord).toHaveBeenCalledTimes(1);
     expect(initialSession?.id).toBeDefined();
     expect(replay.getContext()).toEqual(
       expect.objectContaining({

packages/replay/test/mocks/mockSdk.ts

@@ -68,6 +68,10 @@ export async function mockSdk({ replayOptions, sentryOptions, autoStart = true }
     public setupOnce(): void {
       // do nothing
     }
+
+    public initialize(): void {
+      return super._initialize();
+    }
   }
 
   const replayIntegration = new TestReplayIntegration({
@@ -91,7 +95,8 @@ export async function mockSdk({ replayOptions, sentryOptions, autoStart = true }
   replayIntegration['_setup']();
 
   if (autoStart) {
-    replayIntegration.start();
+    // Only exists in our mock
+    replayIntegration.initialize();
   }
 
   const replay = replayIntegration['_replay']!;

packages/replay/test/mocks/resetSdkMock.ts

@@ -1,3 +1,4 @@
+import type { Replay as ReplayIntegration } from '../../src';
 import type { ReplayContainer } from '../../src/replay';
 import type { RecordMock } from './../index';
 import { BASE_TIMESTAMP } from './../index';
@@ -9,6 +10,7 @@ export async function resetSdkMock({ replayOptions, sentryOptions, autoStart }:
   domHandler: DomHandler;
   mockRecord: RecordMock;
   replay: ReplayContainer;
+  integration: ReplayIntegration;
 }> {
   let domHandler: DomHandler;
 
@@ -27,7 +29,7 @@ export async function resetSdkMock({ replayOptions, sentryOptions, autoStart }:
   const { mockRrweb } = await import('./mockRrweb');
   const { record: mockRecord } = mockRrweb();
 
-  const { replay } = await mockSdk({
+  const { replay, integration } = await mockSdk({
     replayOptions,
     sentryOptions,
     autoStart,
@@ -43,5 +45,6 @@ export async function resetSdkMock({ replayOptions, sentryOptions, autoStart }:
     domHandler,
     mockRecord,
     replay,
+    integration,
   };
 }

packages/types/src/replay.ts

@@ -24,4 +24,4 @@ export type ReplayRecordingData = string | Uint8Array;
  * NOTE: These types are still considered Beta and subject to change.
  * @hidden
  */
-export type ReplayRecordingMode = 'session' | 'error';
+export type ReplayRecordingMode = 'session' | 'error' | 'buffer';