fix(core): Align beforeBreadcrumb and tracesSampler error fallbacks with spec

CHANGELOG.md

@@ -8,6 +8,10 @@
 
 ## Unreleased
 
+### Fixes
+
+- A throwing `beforeBreadcrumb` now drops the breadcrumb, and a throwing `tracesSampler` now falls back to the configured `tracesSampleRate` ([#6675](https://github.com/getsentry/sentry-react-native/pull/6675))
+
 ### Dependencies
 
 - Bump Cocoa SDK from v9.26.1 to v9.27.0 ([#6670](https://github.com/getsentry/sentry-react-native/pull/6670))

packages/core/src/js/sdk.tsx

@@ -111,6 +111,8 @@ export function init(passedOptions: ReactNativeOptions): void {
 
   const userBeforeBreadcrumb = safeFactory(userOptions.beforeBreadcrumb, {
     loggerMessage: 'The beforeBreadcrumb threw an error',
+    // Per the Callback Error Isolation spec, drop the breadcrumb when the callback throws.
+    onError: () => null,
   });
 
   // Exclude Dev Server and Sentry Dsn request from Breadcrumbs
@@ -175,7 +177,7 @@ export function init(passedOptions: ReactNativeOptions): void {
   }
 
   if ('tracesSampler' in options) {
-    options.tracesSampler = safeTracesSampler(options.tracesSampler);
+    options.tracesSampler = safeTracesSampler(options.tracesSampler, options.tracesSampleRate);
   }
 
   if (!('environment' in options)) {

packages/core/src/js/utils/safe.ts

@@ -8,13 +8,19 @@ type DangerTypesWithoutCallSignature = object | null | undefined;
  * Returns callback factory wrapped with try/catch
  * or the original passed value is it's not a function.
  *
- * If the factory fails original data are returned as it.
- * They might be partially modified by the failed function.
+ * If the factory fails the original data are returned as is (they might be
+ * partially modified by the failed function), unless an `onError` handler is
+ * provided to compute a different fallback value.
  */
 export function safeFactory<A extends [R, ...unknown[]], R, T extends DangerTypesWithoutCallSignature>(
   danger: ((...args: A) => R) | T,
   options: {
     loggerMessage?: string;
+    /**
+     * Computes the value returned when the wrapped function throws.
+     * Defaults to returning the first argument (the unmodified input).
+     */
+    onError?: (...args: A) => R;
   } = {},
 ): ((...args: A) => R) | T {
   if (typeof danger === 'function') {
@@ -26,7 +32,7 @@ export function safeFactory<A extends [R, ...unknown[]], R, T extends DangerType
           options.loggerMessage ? options.loggerMessage : `The ${danger.name} callback threw an error`,
           error,
         );
-        return args[0];
+        return options.onError ? options.onError(...args) : args[0];
       }
     };
   } else {
@@ -37,18 +43,24 @@ export function safeFactory<A extends [R, ...unknown[]], R, T extends DangerType
 type TracesSampler = Required<ReactNativeOptions>['tracesSampler'];
 
 /**
- * Returns sage tracesSampler that returns 0 if the original failed.
+ * Returns a safe tracesSampler that falls back to the configured `tracesSampleRate`
+ * if the original callback throws.
+ *
+ * Per the Callback Error Isolation spec the fallback MUST NOT substitute a hardcoded
+ * `0` or `1`; when no `tracesSampleRate` is configured the returned `undefined` lets
+ * the core sampling pipeline discard the transaction.
  */
 export function safeTracesSampler(
   tracesSampler: ReactNativeOptions['tracesSampler'],
+  tracesSampleRate: ReactNativeOptions['tracesSampleRate'],
 ): ReactNativeOptions['tracesSampler'] {
   if (tracesSampler) {
     return (...args: Parameters<TracesSampler>): ReturnType<TracesSampler> => {
       try {
         return tracesSampler(...args);
       } catch (error) {
-        debug.error('The tracesSampler callback threw an error', error);
-        return 0;
+        debug.error('The tracesSampler callback threw an error, falling back to tracesSampleRate', error);
+        return tracesSampleRate as ReturnType<TracesSampler>;
       }
     };
   } else {

packages/core/test/sdk.test.ts

@@ -408,17 +408,20 @@ describe('Tests the SDK functionality', () => {
       }).not.toThrow();
       expect(mockInitialScope).toHaveBeenCalledTimes(1);
     });
-    test('beforeBreadcrumb callback is safe after init', () => {
+    test('beforeBreadcrumb callback is safe after init and drops the breadcrumb on error', () => {
       const mockBeforeBreadcrumb = jest.fn(() => {
         throw 'Test error';
       });
 
       init({ beforeBreadcrumb: mockBeforeBreadcrumb });
 
+      let result: ReturnType<NonNullable<ReturnType<typeof usedOptions>>['beforeBreadcrumb']> | undefined;
       expect(() => {
-        usedOptions()?.beforeBreadcrumb?.({} as any);
+        result = usedOptions()?.beforeBreadcrumb?.({ message: 'test' } as any);
       }).not.toThrow();
       expect(mockBeforeBreadcrumb).toHaveBeenCalledTimes(1);
+      // Per the Callback Error Isolation spec the breadcrumb is dropped (null) on error.
+      expect(result).toBeNull();
     });
 
     test('integrations callback should not crash init', () => {
@@ -432,17 +435,21 @@ describe('Tests the SDK functionality', () => {
       expect(mockIntegrations).toHaveBeenCalledTimes(1);
     });
 
-    test('tracesSampler callback is safe after init', () => {
+    test('tracesSampler callback is safe after init and falls back to tracesSampleRate on error', () => {
       const mockTraceSampler = jest.fn(() => {
         throw 'Test error';
       });
 
-      init({ tracesSampler: mockTraceSampler });
+      init({ tracesSampler: mockTraceSampler, tracesSampleRate: 0.42 });
 
+      let result: number | boolean | undefined;
       expect(() => {
-        usedOptions()?.tracesSampler?.({} as any);
+        result = usedOptions()?.tracesSampler?.({} as any);
       }).not.toThrow();
       expect(mockTraceSampler).toHaveBeenCalledTimes(1);
+      // Per the Callback Error Isolation spec the sampler falls back to the configured
+      // tracesSampleRate rather than substituting a hardcoded 0.
+      expect(result).toBe(0.42);
     });
   });
 

packages/core/test/utils/safe.test.ts

@@ -33,11 +33,22 @@ describe('safe', () => {
       expect(mockFn).toHaveBeenCalledTimes(1);
       expect(actualResult).toEqual('foo');
     });
+    test('returns onError result if function failed and onError is provided', () => {
+      const mockFn = jest.fn(() => {
+        throw 'Test error';
+      });
+      const actualSafeFunction = safeFactory(<(foo: string) => string | null>mockFn, {
+        onError: () => null,
+      });
+      const actualResult = actualSafeFunction('foo');
+      expect(mockFn).toHaveBeenCalledTimes(1);
+      expect(actualResult).toBeNull();
+    });
   });
   describe('safeTracesSampler', () => {
     test('calls given function with correct args', () => {
       const mockFn = jest.fn();
-      const actualSafeFunction = safeTracesSampler(mockFn);
+      const actualSafeFunction = safeTracesSampler(mockFn, undefined);
       const expectedInheritOrSampleWith = function (fallbackSampleRate: number): number {
         return fallbackSampleRate;
       };
@@ -55,7 +66,7 @@ describe('safe', () => {
     });
     test('calls given function amd return its result', () => {
       const mockFn = jest.fn(() => 0.5);
-      const actualSafeFunction = safeTracesSampler(mockFn);
+      const actualSafeFunction = safeTracesSampler(mockFn, undefined);
       const actualResult = actualSafeFunction?.({
         name: 'foo',
         transactionContext: { name: 'foo' },
@@ -67,14 +78,29 @@ describe('safe', () => {
       expect(actualResult).toBe(0.5);
     });
     test('passes undefined trough', () => {
-      const actualSafeFunction = safeTracesSampler(undefined);
+      const actualSafeFunction = safeTracesSampler(undefined, undefined);
       expect(actualSafeFunction).not.toBeDefined();
     });
-    test('returns input object if function failed', () => {
+    test('falls back to the configured tracesSampleRate if the function failed', () => {
+      const mockFn = jest.fn(() => {
+        throw 'Test error';
+      });
+      const actualSafeFunction = safeTracesSampler(mockFn, 0.25);
+      const actualResult = actualSafeFunction?.({
+        name: 'foo',
+        transactionContext: { name: 'foo' },
+        inheritOrSampleWith: function (fallbackSampleRate: number): number {
+          return fallbackSampleRate;
+        },
+      });
+      expect(mockFn).toHaveBeenCalledTimes(1);
+      expect(actualResult).toEqual(0.25);
+    });
+    test('returns undefined if the function failed and no tracesSampleRate is configured', () => {
       const mockFn = jest.fn(() => {
         throw 'Test error';
       });
-      const actualSafeFunction = safeTracesSampler(mockFn);
+      const actualSafeFunction = safeTracesSampler(mockFn, undefined);
       const actualResult = actualSafeFunction?.({
         name: 'foo',
         transactionContext: { name: 'foo' },
@@ -83,7 +109,7 @@ describe('safe', () => {
         },
       });
       expect(mockFn).toHaveBeenCalledTimes(1);
-      expect(actualResult).toEqual(0);
+      expect(actualResult).toBeUndefined();
     });
   });
 });