ref(replay): Use beforeAddBreadcrumb hook instead of scope listener (#10600)

Currently, replay captures breadcrumbs by setting up a scope listener on
the current scope. However, this is flawed because this means it will
not pick up breadcrumbs that are added to the isolation scope (which
will be the default going forward, in v8).

This PR changes this to use the `beforeAddBreadcrumb` hook instead to
listen to this.

CAVEATS: This means we only capture breadcrumbs added via
`Sentry.addBreadcrumb()`. If somebody adds a breadcrumb directly to a
scope, the hook is not triggered. I _think_ this is OK, but something to
be aware of - if we want to change this, we'd need to trigger the hook
on the scope, which may be a bit iffy/tricky. But for the most part,
this shouldn't matter, hopefully - this is also non-critical, meaning if
we miss a breadcrumb (for whatever reason) it's not _that_ bad, it will
simply not be visible in the Replay.

Author: mydea

packages/react/src/redux.ts

@@ -1,5 +1,5 @@
 /* eslint-disable @typescript-eslint/no-explicit-any */
-import { getClient, getCurrentScope, getGlobalScope } from '@sentry/core';
+import { addBreadcrumb, getClient, getCurrentScope, getGlobalScope } from '@sentry/core';
 import type { Scope } from '@sentry/types';
 import { addNonEnumerableProperty } from '@sentry/utils';
 
@@ -121,7 +121,7 @@ function createReduxEnhancer(enhancerOptions?: Partial<SentryEnhancerOptions>):
         /* Action breadcrumbs */
         const transformedAction = options.actionTransformer(action);
         if (typeof transformedAction !== 'undefined' && transformedAction !== null) {
-          scope.addBreadcrumb({
+          addBreadcrumb({
             category: ACTION_BREADCRUMB_CATEGORY,
             data: transformedAction,
             type: ACTION_BREADCRUMB_TYPE,

packages/react/test/redux.test.ts

@@ -1,17 +1,16 @@
 import * as Sentry from '@sentry/browser';
+import * as SentryCore from '@sentry/core';
 import * as Redux from 'redux';
 
 import { createReduxEnhancer } from '../src/redux';
 
-const mockAddBreadcrumb = jest.fn();
 const mockSetContext = jest.fn();
 const mockGlobalScopeAddEventProcessor = jest.fn();
 
 jest.mock('@sentry/core', () => ({
   ...jest.requireActual('@sentry/core'),
   getCurrentScope() {
     return {
-      addBreadcrumb: mockAddBreadcrumb,
       setContext: mockSetContext,
     };
   },
@@ -21,15 +20,22 @@ jest.mock('@sentry/core', () => ({
     };
   },
   addEventProcessor: jest.fn(),
+  addBreadcrumb: jest.fn(),
 }));
 
 afterEach(() => {
-  mockAddBreadcrumb.mockReset();
   mockSetContext.mockReset();
   mockGlobalScopeAddEventProcessor.mockReset();
 });
 
 describe('createReduxEnhancer', () => {
+  let mockAddBreadcrumb: jest.SpyInstance;
+
+  beforeEach(() => {
+    mockAddBreadcrumb = SentryCore.addBreadcrumb as unknown as jest.SpyInstance;
+    mockAddBreadcrumb.mockReset();
+  });
+
   it('logs redux action as breadcrumb', () => {
     const enhancer = createReduxEnhancer();
 

packages/replay/src/coreHandlers/handleScope.ts renamed to packages/replay/src/coreHandlers/handleBreadcrumbs.ts

@@ -1,4 +1,5 @@
-import type { Breadcrumb, Scope } from '@sentry/types';
+import { getClient } from '@sentry/core';
+import type { Breadcrumb } from '@sentry/types';
 import { normalize } from '@sentry/utils';
 
 import { CONSOLE_ARG_MAX_SIZE } from '../constants';
@@ -7,61 +8,55 @@ import type { ReplayFrame } from '../types/replayFrame';
 import { createBreadcrumb } from '../util/createBreadcrumb';
 import { addBreadcrumbEvent } from './util/addBreadcrumbEvent';
 
-let _LAST_BREADCRUMB: null | Breadcrumb = null;
-
 type BreadcrumbWithCategory = Required<Pick<Breadcrumb, 'category'>>;
 
-function isBreadcrumbWithCategory(breadcrumb: Breadcrumb): breadcrumb is BreadcrumbWithCategory {
-  return !!breadcrumb.category;
-}
+/**
+ * Handle breadcrumbs that Sentry captures, and make sure to capture relevant breadcrumbs to Replay as well.
+ */
+export function handleBreadcrumbs(replay: ReplayContainer): void {
+  const client = getClient();
 
-export const handleScopeListener: (replay: ReplayContainer) => (scope: Scope) => void =
-  (replay: ReplayContainer) =>
-  (scope: Scope): void => {
-    if (!replay.isEnabled()) {
-      return;
-    }
+  if (!client || !client.on) {
+    return;
+  }
 
-    const result = handleScope(scope);
+  client.on('beforeAddBreadcrumb', breadcrumb => beforeAddBreadcrumb(replay, breadcrumb));
+}
 
-    if (!result) {
-      return;
-    }
+function beforeAddBreadcrumb(replay: ReplayContainer, breadcrumb: Breadcrumb): void {
+  if (!replay.isEnabled() || !isBreadcrumbWithCategory(breadcrumb)) {
+    return;
+  }
 
+  const result = normalizeBreadcrumb(breadcrumb);
+  if (result) {
     addBreadcrumbEvent(replay, result);
-  };
-
-/**
- * An event handler to handle scope changes.
- */
-export function handleScope(scope: Scope): Breadcrumb | null {
-  // TODO (v8): Remove this guard. This was put in place because we introduced
-  // Scope.getLastBreadcrumb mid-v7 which caused incompatibilities with older SDKs.
-  // For now, we'll just return null if the method doesn't exist but we should eventually
-  // get rid of this guard.
-  const newBreadcrumb = scope.getLastBreadcrumb && scope.getLastBreadcrumb();
-
-  // Listener can be called when breadcrumbs have not changed, so we store the
-  // reference to the last crumb and only return a crumb if it has changed
-  if (_LAST_BREADCRUMB === newBreadcrumb || !newBreadcrumb) {
-    return null;
   }
+}
 
-  _LAST_BREADCRUMB = newBreadcrumb;
-
+/** Exported only for tests. */
+export function normalizeBreadcrumb(breadcrumb: Breadcrumb): Breadcrumb | null {
   if (
-    !isBreadcrumbWithCategory(newBreadcrumb) ||
-    ['fetch', 'xhr', 'sentry.event', 'sentry.transaction'].includes(newBreadcrumb.category) ||
-    newBreadcrumb.category.startsWith('ui.')
+    !isBreadcrumbWithCategory(breadcrumb) ||
+    [
+      // fetch & xhr are handled separately,in handleNetworkBreadcrumbs
+      'fetch',
+      'xhr',
+      // These two are breadcrumbs for emitted sentry events, we don't care about them
+      'sentry.event',
+      'sentry.transaction',
+    ].includes(breadcrumb.category) ||
+    // We capture UI breadcrumbs separately
+    breadcrumb.category.startsWith('ui.')
   ) {
     return null;
   }
 
-  if (newBreadcrumb.category === 'console') {
-    return normalizeConsoleBreadcrumb(newBreadcrumb);
+  if (breadcrumb.category === 'console') {
+    return normalizeConsoleBreadcrumb(breadcrumb);
   }
 
-  return createBreadcrumb(newBreadcrumb);
+  return createBreadcrumb(breadcrumb);
 }
 
 /** exported for tests only */
@@ -116,3 +111,7 @@ export function normalizeConsoleBreadcrumb(
     },
   });
 }
+
+function isBreadcrumbWithCategory(breadcrumb: Breadcrumb): breadcrumb is BreadcrumbWithCategory {
+  return !!breadcrumb.category;
+}

packages/replay/src/util/addGlobalListeners.ts

@@ -1,29 +1,27 @@
 import type { BaseClient } from '@sentry/core';
-import { getCurrentScope } from '@sentry/core';
 import { addEventProcessor, getClient } from '@sentry/core';
 import type { Client, DynamicSamplingContext } from '@sentry/types';
 import { addClickKeypressInstrumentationHandler, addHistoryInstrumentationHandler } from '@sentry/utils';
 
 import { handleAfterSendEvent } from '../coreHandlers/handleAfterSendEvent';
 import { handleBeforeSendEvent } from '../coreHandlers/handleBeforeSendEvent';
+import { handleBreadcrumbs } from '../coreHandlers/handleBreadcrumbs';
 import { handleDomListener } from '../coreHandlers/handleDom';
 import { handleGlobalEventListener } from '../coreHandlers/handleGlobalEvent';
 import { handleHistorySpanListener } from '../coreHandlers/handleHistory';
 import { handleNetworkBreadcrumbs } from '../coreHandlers/handleNetworkBreadcrumbs';
-import { handleScopeListener } from '../coreHandlers/handleScope';
 import type { ReplayContainer } from '../types';
 
 /**
  * Add global listeners that cannot be removed.
  */
 export function addGlobalListeners(replay: ReplayContainer): void {
   // Listeners from core SDK //
-  const scope = getCurrentScope();
   const client = getClient();
 
-  scope.addScopeListener(handleScopeListener(replay));
   addClickKeypressInstrumentationHandler(handleDomListener(replay));
   addHistoryInstrumentationHandler(handleHistorySpanListener(replay));
+  handleBreadcrumbs(replay);
   handleNetworkBreadcrumbs(replay);
 
   // Tag all (non replay) events that get sent to Sentry with the current

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

@@ -0,0 +1,135 @@
+import { CONSOLE_ARG_MAX_SIZE } from '../../../src/constants';
+import { normalizeBreadcrumb, normalizeConsoleBreadcrumb } from '../../../src/coreHandlers/handleBreadcrumbs';
+
+describe('Unit | coreHandlers | handleBreadcrumbs', () => {
+  describe('normalizeBreadcrumb', () => {
+    it.each([undefined, 'ui.click', 'ui.scroll', 'fetch', 'xhr', 'sentry.event', 'sentry.transaction'])(
+      'returns null if breadcrumb has category=%p',
+      category => {
+        const actual = normalizeBreadcrumb({ category });
+        expect(actual).toBeNull();
+      },
+    );
+
+    it('returns breadcrumb when category is valid', () => {
+      const breadcrumb = { category: 'other' };
+      const actual = normalizeBreadcrumb(breadcrumb);
+      expect(actual).toEqual({
+        timestamp: expect.any(Number),
+        category: 'other',
+        type: 'default',
+      });
+    });
+
+    it('timestamp takes precedence', () => {
+      const breadcrumb = { category: 'other', timestamp: 123456 };
+      const actual = normalizeBreadcrumb(breadcrumb);
+      expect(actual).toEqual({
+        timestamp: 123456,
+        category: 'other',
+        type: 'default',
+      });
+    });
+
+    it('handles console breadcrumb', () => {
+      const breadcrumb = {
+        category: 'console',
+        message: 'test',
+        data: {
+          arguments: ['a'.repeat(CONSOLE_ARG_MAX_SIZE + 10), 'b'.repeat(CONSOLE_ARG_MAX_SIZE + 10)],
+        },
+      };
+      const actual = normalizeBreadcrumb(breadcrumb);
+      expect(actual).toEqual({
+        timestamp: expect.any(Number),
+        category: 'console',
+        message: 'test',
+        type: 'default',
+        data: {
+          arguments: [`${'a'.repeat(CONSOLE_ARG_MAX_SIZE)}…`, `${'b'.repeat(CONSOLE_ARG_MAX_SIZE)}…`],
+          _meta: { warnings: ['CONSOLE_ARG_TRUNCATED'] },
+        },
+      });
+    });
+  });
+
+  describe('normalizeConsoleBreadcrumb', () => {
+    it('handles console messages with no arguments', () => {
+      const breadcrumb = { category: 'console', message: 'test' };
+      const actual = normalizeConsoleBreadcrumb(breadcrumb);
+
+      expect(actual).toMatchObject({ category: 'console', message: 'test' });
+    });
+
+    it('handles console messages with empty arguments', () => {
+      const breadcrumb = { category: 'console', message: 'test', data: { arguments: [] } };
+      const actual = normalizeConsoleBreadcrumb(breadcrumb);
+
+      expect(actual).toMatchObject({ category: 'console', message: 'test', data: { arguments: [] } });
+    });
+
+    it('handles console messages with simple arguments', () => {
+      const breadcrumb = {
+        category: 'console',
+        message: 'test',
+        data: { arguments: [1, 'a', true, null, undefined] },
+      };
+      const actual = normalizeConsoleBreadcrumb(breadcrumb);
+
+      expect(actual).toMatchObject({
+        category: 'console',
+        message: 'test',
+        data: {
+          arguments: [1, 'a', true, null, undefined],
+        },
+      });
+    });
+
+    it('truncates large strings', () => {
+      const breadcrumb = {
+        category: 'console',
+        message: 'test',
+        data: {
+          arguments: ['a'.repeat(CONSOLE_ARG_MAX_SIZE + 10), 'b'.repeat(CONSOLE_ARG_MAX_SIZE + 10)],
+        },
+      };
+      const actual = normalizeConsoleBreadcrumb(breadcrumb);
+
+      expect(actual).toMatchObject({
+        category: 'console',
+        message: 'test',
+        data: {
+          arguments: [`${'a'.repeat(CONSOLE_ARG_MAX_SIZE)}…`, `${'b'.repeat(CONSOLE_ARG_MAX_SIZE)}…`],
+          _meta: { warnings: ['CONSOLE_ARG_TRUNCATED'] },
+        },
+      });
+    });
+
+    it('truncates large JSON objects', () => {
+      const bb = { bb: 'b'.repeat(CONSOLE_ARG_MAX_SIZE + 10) };
+      const c = { c: 'c'.repeat(CONSOLE_ARG_MAX_SIZE + 10) };
+
+      const breadcrumb = {
+        category: 'console',
+        message: 'test',
+        data: {
+          arguments: [{ aa: 'yes' }, bb, c],
+        },
+      };
+      const actual = normalizeConsoleBreadcrumb(breadcrumb);
+
+      expect(actual).toMatchObject({
+        category: 'console',
+        message: 'test',
+        data: {
+          arguments: [
+            { aa: 'yes' },
+            `${JSON.stringify(bb, null, 2).slice(0, CONSOLE_ARG_MAX_SIZE)}…`,
+            `${JSON.stringify(c, null, 2).slice(0, CONSOLE_ARG_MAX_SIZE)}…`,
+          ],
+          _meta: { warnings: ['CONSOLE_ARG_TRUNCATED'] },
+        },
+      });
+    });
+  });
+});