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

Author: mydea

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 && isErrorEvent(event)) {
+        promise.then(sendResponse => this.emit('afterSendErrorEvent', 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: 'afterSendErrorEvent',
+    callback: (event: ErrorEvent, 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: 'afterSendErrorEvent', event: ErrorEvent, 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

@@ -104,6 +104,9 @@ export function createTransport(
     );
   }
 
+  // We use this to identifify if the transport is the base transport
+  send.__sentry__baseTransport__ = true;
+
   return {
     send,
     flush,

packages/core/test/lib/base.test.ts

@@ -1634,6 +1634,108 @@ describe('BaseClient', () => {
     });
   });
 
+  describe('sendEvent', () => {
+    beforeEach(() => {
+      jest.useFakeTimers();
+    });
+
+    afterEach(() => {
+      jest.useRealTimers();
+    });
+
+    it('emits `afterSendErrorEvent` when sending an error', async () => {
+      const client = new TestClient(
+        getDefaultTestClientOptions({
+          dsn: PUBLIC_DSN,
+          enableSend: true,
+        }),
+      );
+
+      // @ts-ignore
+      const mockSend = jest.spyOn(client._transport, 'send');
+
+      const errorEvent: Event = { message: 'error' };
+
+      const callback = jest.fn();
+      client.on('afterSendErrorEvent', callback);
+
+      client.sendEvent(errorEvent);
+      jest.runAllTimers();
+      // Wait for two ticks
+      // note that for whatever reason, await new Promise(resolve => setTimeout(resolve, 0)) causes the test to hang
+      await undefined;
+      await undefined;
+
+      expect(mockSend).toBeCalledTimes(1);
+      expect(callback).toBeCalledTimes(1);
+      expect(callback).toBeCalledWith(errorEvent, {});
+    });
+
+    it('still triggers `afterSendErrorEvent` when transport.send rejects', async () => {
+      expect.assertions(3);
+
+      const client = new TestClient(
+        getDefaultTestClientOptions({
+          dsn: PUBLIC_DSN,
+          enableSend: true,
+        }),
+      );
+
+      // @ts-ignore
+      const mockSend = jest.spyOn(client._transport, 'send').mockImplementation(() => {
+        return Promise.reject('send error');
+      });
+
+      const errorEvent: Event = { message: 'error' };
+
+      const callback = jest.fn();
+      client.on('afterSendErrorEvent', callback);
+
+      client.sendEvent(errorEvent);
+      jest.runAllTimers();
+      // Wait for two ticks
+      // note that for whatever reason, await new Promise(resolve => setTimeout(resolve, 0)) causes the test to hang
+      await undefined;
+      await undefined;
+
+      expect(mockSend).toBeCalledTimes(1);
+      expect(callback).toBeCalledTimes(1);
+      expect(callback).toBeCalledWith(errorEvent, undefined);
+    });
+
+    it('passes the response to the hook', async () => {
+      expect.assertions(3);
+
+      const client = new TestClient(
+        getDefaultTestClientOptions({
+          dsn: PUBLIC_DSN,
+          enableSend: true,
+        }),
+      );
+
+      // @ts-ignore
+      const mockSend = jest.spyOn(client._transport, 'send').mockImplementation(() => {
+        return Promise.resolve({ statusCode: 200 });
+      });
+
+      const errorEvent: Event = { message: 'error' };
+
+      const callback = jest.fn();
+      client.on('afterSendErrorEvent', callback);
+
+      client.sendEvent(errorEvent);
+      jest.runAllTimers();
+      // Wait for two ticks
+      // note that for whatever reason, await new Promise(resolve => setTimeout(resolve, 0)) causes the test to hang
+      await undefined;
+      await undefined;
+
+      expect(mockSend).toBeCalledTimes(1);
+      expect(callback).toBeCalledTimes(1);
+      expect(callback).toBeCalledWith(errorEvent, { statusCode: 200 });
+    });
+  });
+
   describe('captureSession()', () => {
     test('sends sessions to the client', () => {
       expect.assertions(1);

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

@@ -1,33 +1,15 @@
 import { expect } from '@playwright/test';
 
 import { sentryTest } from '../../../../utils/fixtures';
-import { getExpectedReplayEvent } from '../../../../utils/replayEventTemplates';
-import { getReplayEvent, shouldSkipReplayTest, waitForReplayRequest } from '../../../../utils/replayHelpers';
+import { getReplaySnapshot, 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',
+  '[error-mode] should not start recording if an error occurred when the error was dropped',
   async ({ getLocalTestPath, page }) => {
     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 => {
@@ -47,26 +29,14 @@ sentryTest(
     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;
 
-    // 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/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/integration-tests/suites/replay/errors/errorModeCustomTransport/test.ts

@@ -0,0 +1,39 @@
+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 }) => {
+  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 page.click('#go-background');
+  expect(callsToSentry).toEqual(0);
+
+  await page.click('#error');
+  await promiseReq0;
+
+  await page.click('#go-background');
+  await promiseReq1;
+
+  const replay = await getReplaySnapshot(page);
+  expect(replay.recordingMode).toBe('session');
+});

packages/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/integration-tests/suites/replay/errors/errorNotSent/test.ts

@@ -0,0 +1,42 @@
+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 }) => {
+    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 page.click('#go-background');
+    expect(callsToSentry).toEqual(0);
+
+    await page.click('#error');
+
+    await page.click('#go-background');
+    await page.click('#log');
+    await page.click('#go-background');
+
+    // 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');
+  },
+);