feat(replay): Use new afterSend hook to improve error linking (#7390)

---------

Co-authored-by: Lukas Stracke <[email protected]>

Author: mydea

packages/browser-integration-tests/suites/replay/errors/droppedError/test.ts

@@ -1,37 +1,25 @@
 import { expect } from '@playwright/test';
 
 import { sentryTest } from '../../../../utils/fixtures';
-import { getExpectedReplayEvent } from '../../../../utils/replayEventTemplates';
-import { getReplayEvent, shouldSkipReplayTest, waitForReplayRequest } from '../../../../utils/replayHelpers';
+import { envelopeRequestParser } from '../../../../utils/helpers';
+import { getReplaySnapshot, isReplayEvent, shouldSkipReplayTest } from '../../../../utils/replayHelpers';
 
-/*
- * This scenario currently shows somewhat unexpected behavior from the PoV of a user:
- * The error is dropped, but the recording is started and continued anyway.
- * If folks only sample error replays, this will lead to a lot of confusion as the resulting replay
- * won't contain the error that started it (possibly none or only additional errors that occurred later on).
- *
- * This is because in error-mode, we start recording as soon as replay's eventProcessor is called with an error.
- * If later event processors or beforeSend drop the error, the recording is already started.
- *
- * We'll need a proper SDK lifecycle hook (WIP) to fix this properly.
- * TODO: Once we have lifecycle hooks, we should revisit this test and make sure it behaves as expected.
- *       This means that the recording should not be started or stopped if the error that triggered it is not sent.
- */
 sentryTest(
-  '[error-mode] should start recording if an error occurred although the error was dropped',
-  async ({ getLocalTestPath, page }) => {
+  '[error-mode] should not start recording if an error occurred when the error was dropped',
+  async ({ getLocalTestPath, page, forceFlushReplay }) => {
     if (shouldSkipReplayTest()) {
       sentryTest.skip();
     }
 
-    const reqPromise0 = waitForReplayRequest(page, 0);
-    const reqPromise1 = waitForReplayRequest(page, 1);
-    const reqPromise2 = waitForReplayRequest(page, 2);
-
     let callsToSentry = 0;
 
     await page.route('https://dsn.ingest.sentry.io/**/*', route => {
-      callsToSentry++;
+      const req = route.request();
+      const event = envelopeRequestParser(req);
+
+      if (isReplayEvent(event)) {
+        callsToSentry++;
+      }
 
       return route.fulfill({
         status: 200,
@@ -43,30 +31,17 @@ sentryTest(
     const url = await getLocalTestPath({ testDir: __dirname });
 
     await page.goto(url);
-    await page.click('#go-background');
+    await forceFlushReplay();
     expect(callsToSentry).toEqual(0);
 
     await page.click('#error');
-    const req0 = await reqPromise0;
-
-    await page.click('#go-background');
-    await reqPromise1;
 
     await page.click('#log');
-    await page.click('#go-background');
-    await reqPromise2;
+    await forceFlushReplay();
 
-    // Note: The fact that reqPromise1/reqPromise2 are fulfilled prooves that the recording continues
-
-    const event0 = getReplayEvent(req0);
+    expect(callsToSentry).toEqual(0);
 
-    expect(event0).toEqual(
-      getExpectedReplayEvent({
-        contexts: { replay: { error_sample_rate: 1, session_sample_rate: 0 } },
-        // This is by design. A dropped error shouldn't be in this list.
-        error_ids: [],
-        replay_type: 'error',
-      }),
-    );
+    const replay = await getReplaySnapshot(page);
+    expect(replay.recordingMode).toBe('error');
   },
 );

packages/browser-integration-tests/suites/replay/errors/errorModeCustomTransport/init.js

@@ -0,0 +1,22 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+window.Replay = new Sentry.Replay({
+  flushMinDelay: 500,
+  flushMaxDelay: 500,
+});
+
+Sentry.init({
+  dsn: 'https://[email protected]/1337',
+  sampleRate: 1,
+  replaysSessionSampleRate: 0.0,
+  replaysOnErrorSampleRate: 1.0,
+  integrations: [window.Replay],
+  transport: options => {
+    const transport = new Sentry.makeXHRTransport(options);
+
+    delete transport.send.__sentry__baseTransport__;
+
+    return transport;
+  },
+});

packages/browser-integration-tests/suites/replay/errors/errorModeCustomTransport/test.ts

@@ -0,0 +1,42 @@
+import { expect } from '@playwright/test';
+
+import { sentryTest } from '../../../../utils/fixtures';
+import { getReplaySnapshot, shouldSkipReplayTest, waitForReplayRequest } from '../../../../utils/replayHelpers';
+
+sentryTest(
+  '[error-mode] should handle errors with custom transport',
+  async ({ getLocalTestPath, page, forceFlushReplay }) => {
+    if (shouldSkipReplayTest()) {
+      sentryTest.skip();
+    }
+
+    const promiseReq0 = waitForReplayRequest(page, 0);
+    const promiseReq1 = waitForReplayRequest(page, 1);
+
+    let callsToSentry = 0;
+
+    await page.route('https://dsn.ingest.sentry.io/**/*', route => {
+      callsToSentry++;
+
+      return route.fulfill({
+        // Only error out for error, then succeed
+        status: callsToSentry === 1 ? 422 : 200,
+      });
+    });
+
+    const url = await getLocalTestPath({ testDir: __dirname });
+
+    await page.goto(url);
+    await forceFlushReplay();
+    expect(callsToSentry).toEqual(0);
+
+    await page.click('#error');
+    await promiseReq0;
+
+    await forceFlushReplay();
+    await promiseReq1;
+
+    const replay = await getReplaySnapshot(page);
+    expect(replay.recordingMode).toBe('session');
+  },
+);

packages/browser-integration-tests/suites/replay/errors/errorNotSent/init.js

@@ -0,0 +1,15 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+window.Replay = new Sentry.Replay({
+  flushMinDelay: 500,
+  flushMaxDelay: 500,
+});
+
+Sentry.init({
+  dsn: 'https://[email protected]/1337',
+  sampleRate: 1,
+  replaysSessionSampleRate: 0.0,
+  replaysOnErrorSampleRate: 1.0,
+  integrations: [window.Replay],
+});

packages/browser-integration-tests/suites/replay/errors/errorNotSent/test.ts

@@ -0,0 +1,41 @@
+import { expect } from '@playwright/test';
+
+import { sentryTest } from '../../../../utils/fixtures';
+import { getReplaySnapshot, shouldSkipReplayTest } from '../../../../utils/replayHelpers';
+
+sentryTest(
+  '[error-mode] should handle errors that result in API error response',
+  async ({ getLocalTestPath, page, forceFlushReplay }) => {
+    if (shouldSkipReplayTest()) {
+      sentryTest.skip();
+    }
+
+    let callsToSentry = 0;
+
+    await page.route('https://dsn.ingest.sentry.io/**/*', route => {
+      callsToSentry++;
+
+      return route.fulfill({
+        status: 422,
+        contentType: 'application/json',
+      });
+    });
+
+    const url = await getLocalTestPath({ testDir: __dirname });
+
+    await page.goto(url);
+    await forceFlushReplay();
+    expect(callsToSentry).toEqual(0);
+
+    await page.click('#error');
+
+    await page.click('#log');
+    await forceFlushReplay();
+
+    // Only sent once, but since API failed we do not go into session mode
+    expect(callsToSentry).toEqual(1);
+
+    const replay = await getReplaySnapshot(page);
+    expect(replay.recordingMode).toBe('error');
+  },
+);

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

@@ -7,7 +7,7 @@ import type {
   Session,
 } from '@sentry/replay/build/npm/types/types';
 import type { eventWithTime } from '@sentry/replay/build/npm/types/types/rrweb';
-import type { Breadcrumb, Event, ReplayEvent } from '@sentry/types';
+import type { Breadcrumb, Event, ReplayEvent, ReplayRecordingMode } from '@sentry/types';
 import pako from 'pako';
 import type { Page, Request, Response } from 'playwright';
 
@@ -104,9 +104,13 @@ function isCustomSnapshot(event: RecordingEvent): event is RecordingEvent & { da
  * Note that due to how this works with playwright, this is a POJO copy of replay.
  * This means that we cannot access any methods on it, and also not mutate it in any way.
  */
-export async function getReplaySnapshot(
-  page: Page,
-): Promise<{ _isPaused: boolean; _isEnabled: boolean; _context: InternalEventContext; session: Session | undefined }> {
+export async function getReplaySnapshot(page: Page): Promise<{
+  _isPaused: boolean;
+  _isEnabled: boolean;
+  _context: InternalEventContext;
+  session: Session | undefined;
+  recordingMode: ReplayRecordingMode;
+}> {
   return await page.evaluate(() => {
     const replayIntegration = (window as unknown as Window & { Replay: { _replay: ReplayContainer } }).Replay;
     const replay = replayIntegration._replay;
@@ -116,6 +120,7 @@ export async function getReplaySnapshot(
       _isEnabled: replay.isEnabled(),
       _context: replay.getContext(),
       session: replay.session,
+      recordingMode: replay.recordingMode,
     };
 
     return replaySnapshot;

packages/browser/src/client.ts

@@ -146,7 +146,7 @@ export class BrowserClient extends BaseClient<BrowserClientOptions> {
       } else {
         // If beacon is not supported or if they are using the tunnel option
         // use our regular transport to send client reports to Sentry.
-        this._sendEnvelope(envelope);
+        void this._sendEnvelope(envelope);
       }
     } catch (e) {
       __DEBUG_BUILD__ && logger.error(e);

packages/core/src/baseclient.ts

@@ -20,6 +20,7 @@ import type {
   Transaction,
   TransactionEvent,
   Transport,
+  TransportMakeRequestResponse,
 } from '@sentry/types';
 import {
   addItemToEnvelope,
@@ -320,7 +321,10 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
         );
       }
 
-      this._sendEnvelope(env);
+      const promise = this._sendEnvelope(env);
+      if (promise) {
+        promise.then(sendResponse => this.emit('afterSendEvent', event, sendResponse), null);
+      }
     }
   }
 
@@ -330,7 +334,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   public sendSession(session: Session | SessionAggregates): void {
     if (this._dsn) {
       const env = createSessionEnvelope(session, this._dsn, this._options._metadata, this._options.tunnel);
-      this._sendEnvelope(env);
+      void this._sendEnvelope(env);
     }
   }
 
@@ -363,6 +367,12 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   /** @inheritdoc */
   public on(hook: 'beforeEnvelope', callback: (envelope: Envelope) => void): void;
 
+  /** @inheritdoc */
+  public on(
+    hook: 'afterSendEvent',
+    callback: (event: Event, sendResponse: TransportMakeRequestResponse | void) => void,
+  ): void;
+
   /** @inheritdoc */
   public on(hook: string, callback: unknown): void {
     if (!this._hooks[hook]) {
@@ -379,6 +389,9 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   /** @inheritdoc */
   public emit(hook: 'beforeEnvelope', envelope: Envelope): void;
 
+  /** @inheritdoc */
+  public emit(hook: 'afterSendEvent', event: Event, sendResponse: TransportMakeRequestResponse | void): void;
+
   /** @inheritdoc */
   public emit(hook: string, ...rest: unknown[]): void {
     if (this._hooks[hook]) {
@@ -624,9 +637,9 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   /**
    * @inheritdoc
    */
-  protected _sendEnvelope(envelope: Envelope): void {
+  protected _sendEnvelope(envelope: Envelope): PromiseLike<void | TransportMakeRequestResponse> | void {
     if (this._transport && this._dsn) {
-      this._transport.send(envelope).then(null, reason => {
+      return this._transport.send(envelope).then(null, reason => {
         __DEBUG_BUILD__ && logger.error('Error while sending event:', reason);
       });
     } else {

packages/core/src/transports/base.ts

@@ -109,6 +109,10 @@ export function createTransport(
     );
   }
 
+  // We use this to identifify if the transport is the base transport
+  // TODO (v8): Remove this again as we'll no longer need it
+  send.__sentry__baseTransport__ = true;
+
   return {
     send,
     flush,