fix(replay): Move error sampling to before send

packages/browser-integration-tests/suites/replay/bufferMode/test.ts

@@ -299,117 +299,126 @@ sentryTest(
 
 // Doing this in buffer mode to test changing error sample rate after first
 // error happens.
-sentryTest('[buffer-mode] can sample on each error event', async ({ getLocalTestPath, page, browserName }) => {
-  // This was sometimes flaky on firefox/webkit, so skipping for now
-  if (shouldSkipReplayTest() || ['firefox', 'webkit'].includes(browserName)) {
-    sentryTest.skip();
-  }
-
-  let callsToSentry = 0;
-  const errorEventIds: string[] = [];
-  const reqPromise0 = waitForReplayRequest(page, 0);
-  const reqErrorPromise = waitForErrorRequest(page);
-
-  await page.route('https://dsn.ingest.sentry.io/**/*', route => {
-    const event = envelopeRequestParser(route.request());
-    // error events have no type field
-    if (event && !event.type && event.event_id) {
-      errorEventIds.push(event.event_id);
-    }
-    // We only want to count errors & replays here
-    if (event && (!event.type || isReplayEvent(event))) {
-      callsToSentry++;
+sentryTest(
+  '[buffer-mode] can sample on each error event',
+  async ({ getLocalTestPath, page, browserName, enableConsole }) => {
+    // This was sometimes flaky on firefox/webkit, so skipping for now
+    if (shouldSkipReplayTest() || ['firefox', 'webkit'].includes(browserName)) {
+      sentryTest.skip();
     }
 
-    return route.fulfill({
-      status: 200,
-      contentType: 'application/json',
-      body: JSON.stringify({ id: 'test-id' }),
+    enableConsole();
+
+    let callsToSentry = 0;
+    const errorEventIds: string[] = [];
+    const reqPromise0 = waitForReplayRequest(page, 0);
+    const reqErrorPromise0 = waitForErrorRequest(page);
+
+    await page.route('https://dsn.ingest.sentry.io/**/*', route => {
+      const event = envelopeRequestParser(route.request());
+      // error events have no type field
+      if (event && !event.type && event.event_id) {
+        errorEventIds.push(event.event_id);
+      }
+      // We only want to count errors & replays here
+      if (event && (!event.type || isReplayEvent(event))) {
+        callsToSentry++;
+      }
+
+      return route.fulfill({
+        status: 200,
+        contentType: 'application/json',
+        body: JSON.stringify({ id: 'test-id' }),
+      });
+    });
+
+    const url = await getLocalTestPath({ testDir: __dirname });
+
+    await page.goto(url);
+    // Start buffering and assert that it is enabled
+    expect(
+      await page.evaluate(() => {
+        const replayIntegration = (window as unknown as Window & { Replay: InstanceType<typeof Replay> }).Replay;
+        const replay = replayIntegration['_replay'];
+        replayIntegration.startBuffering();
+        return replay.isEnabled();
+      }),
+    ).toBe(true);
+
+    await page.click('#go-background');
+    await page.click('#error');
+    await new Promise(resolve => setTimeout(resolve, 1000));
+
+    // 1 unsampled error, no replay
+    const reqError0 = await reqErrorPromise0;
+    const errorEvent0 = envelopeRequestParser(reqError0);
+    expect(callsToSentry).toEqual(1);
+    expect(errorEvent0.tags?.replayId).toBeUndefined();
+
+    await page.evaluate(async () => {
+      const replayIntegration = (window as unknown as Window & { Replay: Replay }).Replay;
+      replayIntegration['_replay'].getOptions().errorSampleRate = 1.0;
     });
-  });
-
-  const url = await getLocalTestPath({ testDir: __dirname });
-
-  await page.goto(url);
-  // Start buffering and assert that it is enabled
-  expect(
-    await page.evaluate(() => {
-      const replayIntegration = (window as unknown as Window & { Replay: InstanceType<typeof Replay> }).Replay;
-      const replay = replayIntegration['_replay'];
-      replayIntegration.startBuffering();
-      return replay.isEnabled();
-    }),
-  ).toBe(true);
-
-  await page.click('#go-background');
-  await page.click('#error');
-  await new Promise(resolve => setTimeout(resolve, 1000));
-
-  // 1 error, no replay
-  await reqErrorPromise;
-  expect(callsToSentry).toEqual(1);
-
-  await page.evaluate(async () => {
-    const replayIntegration = (window as unknown as Window & { Replay: Replay }).Replay;
-    replayIntegration['_replay'].getOptions().errorSampleRate = 1.0;
-  });
-
-  // Error sample rate is now at 1.0, this error should create a replay
-  await page.click('#error2');
-
-  const req0 = await reqPromise0;
-
-  // 2 errors, 1 flush
-  await reqErrorPromise;
-  expect(callsToSentry).toEqual(3);
-
-  const event0 = getReplayEvent(req0);
-  const content0 = getReplayRecordingContent(req0);
-
-  expect(event0).toEqual(
-    getExpectedReplayEvent({
-      error_ids: errorEventIds,
-      replay_type: 'buffer',
-    }),
-  );
-
-  // The first event should have both, full and incremental snapshots,
-  // as we recorded and kept all events in the buffer
-  expect(content0.fullSnapshots).toHaveLength(1);
-  // We want to make sure that the event that triggered the error was
-  // recorded, as well as the first error that did not get sampled.
-  expect(content0.breadcrumbs).toEqual(
-    expect.arrayContaining([
-      {
-        ...expectedClickBreadcrumb,
-        message: 'body > button#error',
-        data: {
-          nodeId: expect.any(Number),
-          node: {
-            attributes: {
-              id: 'error',
+
+    // Error sample rate is now at 1.0, this error should create a replay
+    const reqErrorPromise1 = waitForErrorRequest(page);
+    await page.click('#error2');
+    // 1 unsampled error, 1 sampled error -> 1 flush
+    const req0 = await reqPromise0;
+    const reqError1 = await reqErrorPromise1;
+    const errorEvent1 = envelopeRequestParser(reqError1);
+    expect(callsToSentry).toEqual(3);
+    expect(errorEvent0.event_id).not.toEqual(errorEvent1.event_id);
+    expect(errorEvent1.tags?.replayId).toBeDefined();
+
+    const event0 = getReplayEvent(req0);
+    const content0 = getReplayRecordingContent(req0);
+
+    expect(event0).toEqual(
+      getExpectedReplayEvent({
+        error_ids: errorEventIds,
+        replay_type: 'buffer',
+      }),
+    );
+
+    // The first event should have both, full and incremental snapshots,
+    // as we recorded and kept all events in the buffer
+    expect(content0.fullSnapshots).toHaveLength(1);
+    // We want to make sure that the event that triggered the error was
+    // recorded, as well as the first error that did not get sampled.
+    expect(content0.breadcrumbs).toEqual(
+      expect.arrayContaining([
+        {
+          ...expectedClickBreadcrumb,
+          message: 'body > button#error',
+          data: {
+            nodeId: expect.any(Number),
+            node: {
+              attributes: {
+                id: 'error',
+              },
+              id: expect.any(Number),
+              tagName: 'button',
+              textContent: '***** *****',
             },
-            id: expect.any(Number),
-            tagName: 'button',
-            textContent: '***** *****',
           },
         },
-      },
-      {
-        ...expectedClickBreadcrumb,
-        message: 'body > button#error2',
-        data: {
-          nodeId: expect.any(Number),
-          node: {
-            attributes: {
-              id: 'error2',
+        {
+          ...expectedClickBreadcrumb,
+          message: 'body > button#error2',
+          data: {
+            nodeId: expect.any(Number),
+            node: {
+              attributes: {
+                id: 'error2',
+              },
+              id: expect.any(Number),
+              tagName: 'button',
+              textContent: '******* *****',
             },
-            id: expect.any(Number),
-            tagName: 'button',
-            textContent: '******* *****',
           },
         },
-      },
-    ]),
-  );
-});
+      ]),
+    );
+  },
+);

packages/browser-integration-tests/utils/fixtures.ts

@@ -29,6 +29,7 @@ export type TestFixtures = {
   getLocalTestPath: (options: { testDir: string }) => Promise<string>;
   getLocalTestUrl: (options: { testDir: string; skipRouteHandler?: boolean }) => Promise<string>;
   forceFlushReplay: () => Promise<string>;
+  enableConsole: () => void;
   runInChromium: (fn: (...args: unknown[]) => unknown, args?: unknown[]) => unknown;
   runInFirefox: (fn: (...args: unknown[]) => unknown, args?: unknown[]) => unknown;
   runInWebkit: (fn: (...args: unknown[]) => unknown, args?: unknown[]) => unknown;
@@ -109,6 +110,13 @@ const sentryTest = base.extend<TestFixtures>({
     `),
     );
   },
+
+  enableConsole: ({ page }, use) => {
+    return use(() =>
+      // eslint-disable-next-line no-console
+      page.on('console', msg => console.log(msg.text())),
+    );
+  },
 });
 
 export { sentryTest };

packages/replay/src/coreHandlers/handleAfterSendEvent.ts

@@ -1,10 +1,8 @@
 import { getCurrentHub } from '@sentry/core';
 import type { Event, Transport, TransportMakeRequestResponse } from '@sentry/types';
 
-import { UNABLE_TO_SEND_REPLAY } from '../constants';
 import type { ReplayContainer } from '../types';
 import { isErrorEvent, isTransactionEvent } from '../util/eventUtils';
-import { isSampled } from '../util/isSampled';
 
 type AfterSendEventCallback = (event: Event, sendResponse: TransportMakeRequestResponse | void) => void;
 
@@ -42,22 +40,18 @@ export function handleAfterSendEvent(replay: ReplayContainer): AfterSendEventCal
       return;
     }
 
-    // Add error to list of errorIds of replay
+    // Add error to list of errorIds of replay. This is ok to do even if not
+    // sampled because context will get reset at next checkout.
+    // XXX: There is also a race condition where it's possible to capture an
+    // error to Sentry before Replay SDK has loaded, but response returns after
+    // it was loaded, and this gets called.
     if (event.event_id) {
       replay.getContext().errorIds.add(event.event_id);
     }
 
-    // Trigger error recording
+    // If error event is tagged with replay id it means it was sampled (when in buffer mode)
     // Need to be very careful that this does not cause an infinite loop
-    if (
-      replay.recordingMode === 'buffer' &&
-      event.exception &&
-      event.message !== UNABLE_TO_SEND_REPLAY // ignore this error because otherwise we could loop indefinitely with trying to capture replay and failing
-    ) {
-      if (!isSampled(replay.getOptions().errorSampleRate)) {
-        return;
-      }
-
+    if (replay.recordingMode === 'buffer' && event.tags && event.tags.replayId) {
       setTimeout(() => {
         // Capture current event buffer as new replay
         void replay.sendBufferedReplayOrFlush();

packages/replay/src/coreHandlers/handleGlobalEvent.ts

@@ -6,6 +6,7 @@ import type { ReplayContainer } from '../types';
 import { isErrorEvent, isReplayEvent, isTransactionEvent } from '../util/eventUtils';
 import { isRrwebError } from '../util/isRrwebError';
 import { handleAfterSendEvent } from './handleAfterSendEvent';
+import { shouldSampleForBufferEvent } from './util/shouldSampleForBufferEvent';
 
 /**
  * Returns a listener to be added to `addGlobalEventProcessor(listener)`.
@@ -36,8 +37,16 @@ export function handleGlobalEventListener(
       return null;
     }
 
-    // Only tag transactions with replayId if not waiting for an error
-    if (isErrorEvent(event) || (isTransactionEvent(event) && replay.recordingMode === 'session')) {
+    // When in buffer mode, we decide to sample here.
+    // Later, in `handleAfterSendEvent`, if the replayId is set, we know that we sampled
+    // And convert the buffer session to a full session
+    const isErrorEventSampled = shouldSampleForBufferEvent(replay, event);
+
+    // Tag errors if it has been sampled in buffer mode, or if it is session mode
+    // Only tag transactions if in session mode
+    const shouldTagReplayId = isErrorEventSampled || replay.recordingMode === 'session';
+
+    if (shouldTagReplayId) {
       event.tags = { ...event.tags, replayId: replay.getSessionId() };
     }
 

packages/replay/src/coreHandlers/util/shouldSampleForBufferEvent.ts

@@ -0,0 +1,29 @@
+import type { Event } from '@sentry/types';
+
+import { UNABLE_TO_SEND_REPLAY } from '../../constants';
+import type { ReplayContainer } from '../../types';
+import { isSampled } from '../../util/isSampled';
+
+/**
+ * Determine if event should be sampled (only applies in buffer mode).
+ * When an event is captured by `hanldleGlobalEvent`, when in buffer mode
+ * we determine if we want to sample the error or not.
+ */
+export function shouldSampleForBufferEvent(replay: ReplayContainer, event: Event): boolean {
+  if (replay.recordingMode !== 'buffer') {
+    return false;
+  }
+
+  // ignore this error because otherwise we could loop indefinitely with
+  // trying to capture replay and failing
+  if (event.message === UNABLE_TO_SEND_REPLAY) {
+    return false;
+  }
+
+  // Require the event to be an error event & to have an exception
+  if (!event.exception || event.type) {
+    return false;
+  }
+
+  return isSampled(replay.getOptions().errorSampleRate);
+}

packages/replay/test/integration/coreHandlers/handleAfterSendEvent.test.ts

@@ -136,7 +136,7 @@ describe('Integration | coreHandlers | handleAfterSendEvent', () => {
 
     const mockSend = getCurrentHub().getClient()!.getTransport()!.send as unknown as jest.SpyInstance<any>;
 
-    const error1 = Error({ event_id: 'err1' });
+    const error1 = Error({ event_id: 'err1', tags: { replayId: 'replayid1' } });
 
     const handler = handleAfterSendEvent(replay);