feat(core): Isolate throwing user callbacks instead of capturing them as events

dev-packages/node-integration-tests/suites/client-reports/drop-reasons/before-send-throws/scenario.ts

@@ -0,0 +1,15 @@
+import * as Sentry from '@sentry/node';
+import { loggingTransport } from '@sentry-internal/node-integration-tests';
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  transport: loggingTransport,
+  beforeSend() {
+    throw new Error('beforeSend failed');
+  },
+});
+
+Sentry.captureException(new Error('this should get dropped because beforeSend throws'));
+
+// eslint-disable-next-line @typescript-eslint/no-floating-promises
+Sentry.flush();

dev-packages/node-integration-tests/suites/client-reports/drop-reasons/before-send-throws/test.ts

@@ -0,0 +1,24 @@
+import { afterAll, test } from 'vitest';
+import { cleanupChildProcesses, createRunner } from '../../../../utils/runner';
+
+afterAll(() => {
+  cleanupChildProcesses();
+});
+
+test('records a client report and no extra error event when beforeSend throws', async () => {
+  await createRunner(__dirname, 'scenario.ts')
+    .unignore('client_report')
+    .expect({
+      client_report: {
+        discarded_events: [
+          {
+            category: 'error',
+            quantity: 1,
+            reason: 'before_send',
+          },
+        ],
+      },
+    })
+    .start()
+    .completed();
+});

dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processor-throws/scenario.ts

@@ -0,0 +1,16 @@
+import * as Sentry from '@sentry/node';
+import { loggingTransport } from '@sentry-internal/node-integration-tests';
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  transport: loggingTransport,
+});
+
+Sentry.addEventProcessor(() => {
+  throw new Error('event processor failed');
+});
+
+Sentry.captureException(new Error('this should get dropped because the event processor throws'));
+
+// eslint-disable-next-line @typescript-eslint/no-floating-promises
+Sentry.flush();

dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processor-throws/test.ts

@@ -0,0 +1,24 @@
+import { afterAll, test } from 'vitest';
+import { cleanupChildProcesses, createRunner } from '../../../../utils/runner';
+
+afterAll(() => {
+  cleanupChildProcesses();
+});
+
+test('records a client report and no extra error event when an event processor throws', async () => {
+  await createRunner(__dirname, 'scenario.ts')
+    .unignore('client_report')
+    .expect({
+      client_report: {
+        discarded_events: [
+          {
+            category: 'error',
+            quantity: 1,
+            reason: 'event_processor',
+          },
+        ],
+      },
+    })
+    .start()
+    .completed();
+});

dev-packages/node-integration-tests/suites/client-reports/drop-reasons/traces-sampler-throws/scenario.ts

@@ -0,0 +1,17 @@
+import * as Sentry from '@sentry/node';
+import { loggingTransport } from '@sentry-internal/node-integration-tests';
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  transport: loggingTransport,
+  tracesSampler: () => {
+    throw new Error('tracesSampler failed');
+  },
+});
+
+Sentry.startSpan({ name: 'this should not be sampled because tracesSampler throws' }, () => {
+  // no-op
+});
+
+// eslint-disable-next-line @typescript-eslint/no-floating-promises
+Sentry.flush();

dev-packages/node-integration-tests/suites/client-reports/drop-reasons/traces-sampler-throws/test.ts

@@ -0,0 +1,24 @@
+import { afterAll, test } from 'vitest';
+import { cleanupChildProcesses, createRunner } from '../../../../utils/runner';
+
+afterAll(() => {
+  cleanupChildProcesses();
+});
+
+test('records a client report and no error event when tracesSampler throws', async () => {
+  await createRunner(__dirname, 'scenario.ts')
+    .unignore('client_report')
+    .expect({
+      client_report: {
+        discarded_events: [
+          {
+            category: 'span',
+            quantity: 1,
+            reason: 'sample_rate',
+          },
+        ],
+      },
+    })
+    .start()
+    .completed();
+});

packages/core/src/breadcrumbs.ts

@@ -1,6 +1,8 @@
 import { getClient, getIsolationScope } from './currentScopes';
+import { DEBUG_BUILD } from './debug-build';
 import type { Breadcrumb, BreadcrumbHint } from './types/breadcrumb';
 import { consoleSandbox } from './utils/debug-logger';
+import { safeCallback } from './utils/safeCallback';
 import { dateTimestampInSeconds } from './utils/time';
 
 /**
@@ -28,7 +30,11 @@ export function addBreadcrumb(breadcrumb: Breadcrumb, hint?: BreadcrumbHint): vo
   const timestamp = dateTimestampInSeconds();
   const mergedBreadcrumb = { timestamp, ...breadcrumb };
   const finalBreadcrumb = beforeBreadcrumb
-    ? consoleSandbox(() => beforeBreadcrumb(mergedBreadcrumb, hint))
+    ? safeCallback(
+        DEBUG_BUILD ? 'The `beforeBreadcrumb` callback threw an error, dropping the breadcrumb:' : '',
+        () => consoleSandbox(() => beforeBreadcrumb(mergedBreadcrumb, hint)),
+        () => null,
+      )
     : mergedBreadcrumb;
 
   if (finalBreadcrumb === null) return;

packages/core/src/client.ts

@@ -50,6 +50,7 @@ import { parseSampleRate } from './utils/parseSampleRate';
 import { prepareEvent } from './utils/prepareEvent';
 import { makePromiseBuffer, type PromiseBuffer, SENTRY_BUFFER_FULL_ERROR } from './utils/promisebuffer';
 import { safeMathRandom } from './utils/randomSafeContext';
+import { safeCallback } from './utils/safeCallback';
 import { reparentChildSpans, shouldIgnoreSpan } from './utils/should-ignore-span';
 import { safeUnref } from './utils/timer';
 import { convertSpanJsonToTransactionEvent, convertTransactionEventToSpanJson } from './utils/transactionEvent';
@@ -1738,7 +1739,12 @@ function processBeforeSend(
   let processedEvent = event;
 
   if (isErrorEvent(processedEvent) && beforeSend) {
-    return beforeSend(processedEvent, hint);
+    const errorEvent = processedEvent;
+    return safeCallback(
+      DEBUG_BUILD ? 'The `beforeSend` callback threw an error, dropping the event:' : '',
+      () => beforeSend(errorEvent, hint),
+      () => null,
+    );
   }
 
   if (isTransactionEvent(processedEvent)) {
@@ -1809,7 +1815,11 @@ function processBeforeSend(
           spanCountBeforeProcessing: spanCountBefore,
         };
       }
-      return beforeSendTransaction(processedEvent as TransactionEvent, hint);
+      return safeCallback(
+        DEBUG_BUILD ? 'The `beforeSendTransaction` callback threw an error, dropping the event:' : '',
+        () => beforeSendTransaction(processedEvent as TransactionEvent, hint),
+        () => null,
+      );
     }
   }
 

packages/core/src/eventProcessors.ts

@@ -3,6 +3,7 @@ import type { Event, EventHint } from './types/event';
 import type { EventProcessor } from './types/eventprocessor';
 import { debug } from './utils/debug-logger';
 import { isThenable } from './utils/is';
+import { safeCallback } from './utils/safeCallback';
 import { rejectedSyncPromise, resolvedSyncPromise } from './utils/syncpromise';
 
 /**
@@ -34,9 +35,15 @@ function _notifyEventProcessors(
     return event;
   }
 
-  const result = processor({ ...event }, hint);
+  const processorName = `Event processor "${processor.id || '?'}"`;
 
-  DEBUG_BUILD && result === null && debug.log(`Event processor "${processor.id || '?'}" dropped event`);
+  const result = safeCallback(
+    DEBUG_BUILD ? `${processorName} threw an error, dropping event:` : '',
+    () => processor({ ...event }, hint),
+    () => null,
+  );
+
+  DEBUG_BUILD && result === null && debug.log(`${processorName} dropped event`);
 
   if (isThenable(result)) {
     return result.then(final => _notifyEventProcessors(final, hint, processors, index + 1));

packages/core/src/logs/internal.ts

@@ -8,6 +8,7 @@ import type { Integration } from '../types/integration';
 import type { Log, SerializedLog } from '../types/log';
 import { consoleSandbox, debug } from '../utils/debug-logger';
 import { isParameterizedString } from '../utils/is';
+import { safeCallback } from '../utils/safeCallback';
 import { getCombinedScopeData } from '../utils/scopeData';
 import { getActiveSpan } from '../utils/spanUtils';
 import { timestampInSeconds } from '../utils/time';
@@ -142,8 +143,14 @@ export function _INTERNAL_captureLog(
 
   client.emit('beforeCaptureLog', processedLog);
 
-  // We need to wrap this in `consoleSandbox` to avoid recursive calls to `beforeSendLog`
-  const log = beforeSendLog ? consoleSandbox(() => beforeSendLog(processedLog)) : processedLog;
+  const log = beforeSendLog
+    ? safeCallback(
+        DEBUG_BUILD ? 'The `beforeSendLog` callback threw an error, dropping the log:' : '',
+        // We need to wrap this in `consoleSandbox` to avoid recursive calls to `beforeSendLog`
+        () => consoleSandbox(() => beforeSendLog(processedLog)),
+        () => null,
+      )
+    : processedLog;
   if (!log) {
     client.recordDroppedEvent('before_send', 'log_item', 1);
     DEBUG_BUILD && debug.warn('beforeSendLog returned null, log will not be captured.');

packages/core/src/metrics/internal.ts

@@ -8,6 +8,7 @@ import type { Integration } from '../types/integration';
 import type { Metric, SerializedMetric } from '../types/metric';
 import type { User } from '../types/user';
 import { debug } from '../utils/debug-logger';
+import { safeCallback } from '../utils/safeCallback';
 import { getCombinedScopeData } from '../utils/scopeData';
 import { getActiveSpan } from '../utils/spanUtils';
 import { timestampInSeconds } from '../utils/time';
@@ -181,9 +182,16 @@ export function _INTERNAL_captureMetric(beforeMetric: Metric, options?: Internal
 
   client.emit('processMetric', enrichedMetric);
 
-  const processedMetric = beforeSendMetric ? beforeSendMetric(enrichedMetric) : enrichedMetric;
+  const processedMetric = beforeSendMetric
+    ? safeCallback(
+        DEBUG_BUILD ? 'The `beforeSendMetric` callback threw an error, dropping the metric:' : '',
+        () => beforeSendMetric(enrichedMetric),
+        () => null,
+      )
+    : enrichedMetric;
 
   if (!processedMetric) {
+    client.recordDroppedEvent('before_send', 'metric', 1);
     DEBUG_BUILD && debug.log('`beforeSendMetric` returned `null`, will not send metric.');
     return;
   }

packages/core/src/tracing/sampling.ts

@@ -4,6 +4,7 @@ import type { SamplingContext } from '../types/samplingcontext';
 import { debug } from '../utils/debug-logger';
 import { hasSpansEnabled } from '../utils/hasSpansEnabled';
 import { parseSampleRate } from '../utils/parseSampleRate';
+import { safeCallback } from '../utils/safeCallback';
 
 /**
  * Makes a sampling decision for the given options.
@@ -21,37 +22,11 @@ export function sampleSpan(
     return [false];
   }
 
-  let localSampleRateWasApplied = undefined;
-
-  // we would have bailed already if neither `tracesSampler` nor `tracesSampleRate` were defined, so one of these should
-  // work; prefer the hook if so
-  let sampleRate;
-  if (typeof options.tracesSampler === 'function') {
-    sampleRate = options.tracesSampler({
-      ...samplingContext,
-      inheritOrSampleWith: fallbackSampleRate => {
-        // If we have an incoming parent sample rate, we'll just use that one.
-        // The sampling decision will be inherited because of the sample_rand that was generated when the trace reached the incoming boundaries of the SDK.
-        if (typeof samplingContext.parentSampleRate === 'number') {
-          return samplingContext.parentSampleRate;
-        }
-
-        // Fallback if parent sample rate is not on the incoming trace (e.g. if there is no baggage)
-        // This is to provide backwards compatibility if there are incoming traces from older SDKs that don't send a parent sample rate or a sample rand. In these cases we just want to force either a sampling decision on the downstream traces via the sample rate.
-        if (typeof samplingContext.parentSampled === 'boolean') {
-          return Number(samplingContext.parentSampled);
-        }
-
-        return fallbackSampleRate;
-      },
-    });
-    localSampleRateWasApplied = true;
-  } else if (samplingContext.parentSampled !== undefined) {
-    sampleRate = samplingContext.parentSampled;
-  } else if (typeof options.tracesSampleRate !== 'undefined') {
-    sampleRate = options.tracesSampleRate;
-    localSampleRateWasApplied = true;
+  const resolved = resolveSampleRate(options, samplingContext);
+  if (!resolved) {
+    return [false];
   }
+  const [sampleRate, localSampleRateWasApplied] = resolved;
 
   // Since this is coming from the user (or from a function provided by the user), who knows what we might get.
   // (The only valid values are booleans or numbers between 0 and 1.)
@@ -96,3 +71,57 @@ export function sampleSpan(
 
   return [shouldSample, parsedSampleRate, localSampleRateWasApplied];
 }
+
+/**
+ * Prefers `tracesSampler`. If it throws, falls back to the parent decision, then `tracesSampleRate`.
+ * Returns `undefined` when there is nothing to fall back to.
+ */
+function resolveSampleRate(
+  options: Pick<CoreOptions, 'tracesSampleRate' | 'tracesSampler'>,
+  samplingContext: SamplingContext,
+): [sampleRate: unknown, localSampleRateWasApplied?: boolean] | undefined {
+  const { tracesSampler, tracesSampleRate } = options;
+
+  if (typeof tracesSampler === 'function') {
+    const samplerResult = safeCallback(
+      DEBUG_BUILD
+        ? 'The `tracesSampler` callback threw an error, falling back to the parent sampling decision or `tracesSampleRate`:'
+        : '',
+      (): [unknown, boolean] => [
+        tracesSampler({
+          ...samplingContext,
+          inheritOrSampleWith: fallbackSampleRate => {
+            // If we have an incoming parent sample rate, we'll just use that one.
+            // The sampling decision will be inherited because of the sample_rand that was generated when the trace reached the incoming boundaries of the SDK.
+            if (typeof samplingContext.parentSampleRate === 'number') {
+              return samplingContext.parentSampleRate;
+            }
+
+            // Fallback if parent sample rate is not on the incoming trace (e.g. if there is no baggage)
+            // This is to provide backwards compatibility if there are incoming traces from older SDKs that don't send a parent sample rate or a sample rand. In these cases we just want to force either a sampling decision on the downstream traces via the sample rate.
+            if (typeof samplingContext.parentSampled === 'boolean') {
+              return Number(samplingContext.parentSampled);
+            }
+
+            return fallbackSampleRate;
+          },
+        }),
+        true,
+      ],
+      () => undefined,
+    );
+    if (samplerResult) {
+      return samplerResult;
+    }
+  }
+
+  if (samplingContext.parentSampled !== undefined) {
+    return [samplingContext.parentSampled];
+  }
+
+  if (typeof tracesSampleRate !== 'undefined') {
+    return [tracesSampleRate, true];
+  }
+
+  return undefined;
+}