fix(replay): Move error sampling to before send

billyvg · billyvg · commit 9d724a3a6f9d · 2023-05-05T17:31:47.000-04:00
In buffer mode, we were always tagging error events even if it was not sampled. This is because the sampling decision was moved from the initialization of the SDK to much further down the pipeline to right before flushing a replay. We now handle sampling of the error event before tagging event with replayId, and in the afterSend callback, we consider an error event as sampled if it has a replayId.
diff --git a/packages/browser-integration-tests/suites/replay/bufferMode/test.ts b/packages/browser-integration-tests/suites/replay/bufferMode/test.ts
@@ -303,122 +303,127 @@ 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 unsampled error, no replay
-  const reqError0 = await reqErrorPromise;
-  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;
-  });
-
-  // Error sample rate is now at 1.0, this error should create a replay
-  await page.click('#error2');
-
-  const req0 = await reqPromise0;
-
-  // 1 unsampled error, 1 sampled error -> 1 flush
-  const reqError1 = await reqErrorPromise;
-  const errorEvent1 = envelopeRequestParser(reqError1);
-  expect(callsToSentry).toEqual(3);
-  expect(errorEvent1.tags?.replayId).toBeDefined();
-
-  const event0 = getReplayEvent(req0);
-  const content0 = getReplayRecordingContent(req0);
-
-  expect(event0).toEqual(
-    getExpectedReplayEvent({
-      contexts: { replay: { error_sample_rate: 1, session_sample_rate: 0 } },
-      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({
+        contexts: { replay: { error_sample_rate: 1, session_sample_rate: 0 } },
+        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: '******* *****',
           },
         },
-      },
-    ]),
-  );
-});
+      ]),
+    );
+  },
+);
diff --git a/packages/browser-integration-tests/utils/fixtures.ts b/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 };
diff --git a/packages/replay/src/coreHandlers/handleAfterSendEvent.ts b/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();
diff --git a/packages/replay/src/coreHandlers/handleGlobalEvent.ts b/packages/replay/src/coreHandlers/handleGlobalEvent.ts
@@ -5,6 +5,8 @@ import { logger } from '@sentry/utils';
 import type { ReplayContainer } from '../types';
 import { isErrorEvent, isReplayEvent, isTransactionEvent } from '../util/eventUtils';
 import { isRrwebError } from '../util/isRrwebError';
+import { isSampled } from '../util/isSampled';
+import { shouldTrySampleEvent } from '../util/shouldTrySampleEvent';
 import { handleAfterSendEvent } from './handleAfterSendEvent';
 
 /**
@@ -36,8 +38,13 @@ export function handleGlobalEventListener(
       return null;
     }
 
-    // Only tag transactions with replayId if not waiting for an error
-    if (isErrorEvent(event) || (isTransactionEvent(event) && replay.recordingMode === 'session')) {
+    const isErrorEventSampled = shouldTrySampleEvent(replay, event) && isSampled(replay.getOptions().errorSampleRate);
+
+    // 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() };
     }
 
diff --git a/packages/replay/src/util/shouldTrySampleEvent.ts b/packages/replay/src/util/shouldTrySampleEvent.ts
@@ -0,0 +1,22 @@
+import type { Event } from '@sentry/types';
+
+import { UNABLE_TO_SEND_REPLAY } from '../constants';
+import type { ReplayContainer } from '../types';
+
+/**
+ * Determine if event should be sampled (only applies in buffer mode).
+ */
+export function shouldTrySampleEvent(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 have an exception
+  return !!event.exception;
+}
