From dcf7f409cdc351a9189fa0a34f46278bee768d00 Mon Sep 17 00:00:00 2001 From: Martin Sonnberger Date: Wed, 2 Sep 2026 10:08:03 +0200 Subject: [PATCH 1/3] feat(core): Record `callback_error` client reports for throwing user callbacks Events, logs, metrics and root spans dropped because a user callback threw were reported with the same outcome as a legitimate filter (`before_send`, `event_processor`, `sample_rate`). A dedicated `callback_error` reason makes them distinguishable. Co-Authored-By: Claude Fable 5 --- .../drop-reasons/before-send-throws/test.ts | 2 +- .../event-processor-throws/test.ts | 4 +- .../traces-sampler-throws/test.ts | 2 +- packages/core/src/client.ts | 49 +++++++++++-------- packages/core/src/eventProcessors.ts | 7 ++- packages/core/src/logs/internal.ts | 10 ++-- packages/core/src/metrics/internal.ts | 11 +++-- packages/core/src/tracing/sampling.ts | 22 ++++++--- packages/core/src/tracing/trace.ts | 6 +-- packages/core/src/types/clientreport.ts | 1 + packages/core/src/utils/safeCallback.ts | 7 +++ packages/core/test/lib/client.test.ts | 16 +++--- .../core/test/lib/eventProcessors.test.ts | 13 ++--- packages/core/test/lib/logs/internal.test.ts | 2 +- .../core/test/lib/metrics/internal.test.ts | 2 +- .../core/test/lib/tracing/sampling.test.ts | 35 ++++++------- 16 files changed, 114 insertions(+), 75 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/before-send-throws/test.ts b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/before-send-throws/test.ts index e30038efc57b..80d5813b5f88 100644 --- a/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/before-send-throws/test.ts +++ b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/before-send-throws/test.ts @@ -14,7 +14,7 @@ test('records a client report and no extra error event when beforeSend throws', { category: 'error', quantity: 1, - reason: 'before_send', + reason: 'callback_error', }, ], }, diff --git a/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processor-throws/test.ts b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processor-throws/test.ts index 370cccc35410..0996c0841cf2 100644 --- a/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processor-throws/test.ts +++ b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/event-processor-throws/test.ts @@ -14,7 +14,7 @@ test('records a client report and no extra error event when an event processor t { category: 'error', quantity: 1, - reason: 'event_processor', + reason: 'callback_error', }, ], }, @@ -32,7 +32,7 @@ test('records a client report and no extra error event when an async event proce { category: 'error', quantity: 1, - reason: 'event_processor', + reason: 'callback_error', }, ], }, diff --git a/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/traces-sampler-throws/test.ts b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/traces-sampler-throws/test.ts index df6cbb3195b2..e511fc97e8a9 100644 --- a/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/traces-sampler-throws/test.ts +++ b/dev-packages/node-integration-tests/suites/client-reports/drop-reasons/traces-sampler-throws/test.ts @@ -14,7 +14,7 @@ test('records a client report and no error event when tracesSampler throws', asy { category: 'span', quantity: 1, - reason: 'sample_rate', + reason: 'callback_error', }, ], }, diff --git a/packages/core/src/client.ts b/packages/core/src/client.ts index 2de21ba37b41..83cc51c99f17 100644 --- a/packages/core/src/client.ts +++ b/packages/core/src/client.ts @@ -50,7 +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 { CALLBACK_ERROR, safeCallback } from './utils/safeCallback'; import { reparentChildSpans, shouldIgnoreSpan } from './utils/should-ignore-span'; import { safeUnref } from './utils/timer'; import { convertSpanJsonToTransactionEvent, convertTransactionEventToSpanJson } from './utils/transactionEvent'; @@ -1522,6 +1522,14 @@ export abstract class Client { const parsedSampleRate = typeof sampleRate === 'undefined' ? undefined : parseSampleRate(sampleRate); const dataCategory = getDataCategoryByType(event.type); + const recordDroppedEvent = (reason: EventDropReason): void => { + this.recordDroppedEvent(reason, dataCategory); + if (isTransaction) { + // the transaction itself counts as one span, plus all the child spans that are added + this.recordDroppedEvent(reason, 'span', 1 + (event.spans || []).length); + } + }; + return this._prepareEvent(event, hint, currentScope, isolationScope) .then(prepared => { if (prepared === null) { @@ -1539,13 +1547,7 @@ export abstract class Client { }) .then(processedEvent => { if (processedEvent === null) { - this.recordDroppedEvent('before_send', dataCategory); - if (isTransaction) { - const spans = event.spans || []; - // the transaction itself counts as one span, plus all the child spans that are added - const spanCount = 1 + spans.length; - this.recordDroppedEvent('before_send', 'span', spanCount); - } + recordDroppedEvent('before_send'); throw _makeDoNotSendEventError(`${beforeSendLabel} returned \`null\`, will not send event.`); } @@ -1587,6 +1589,11 @@ export abstract class Client { return processedEvent; }) .then(null, reason => { + if (reason === CALLBACK_ERROR) { + recordDroppedEvent('callback_error'); + throw _makeDoNotSendEventError('A user callback threw an error, will not send event.'); + } + if (_isDoNotSendEventError(reason) || _isInternalError(reason)) { throw reason; } @@ -1702,17 +1709,13 @@ function _validateBeforeSendResult( ): PromiseLike | Event | null { const invalidValueError = `${beforeSendLabel} must return \`null\` or a valid event.`; if (isThenable(beforeSendResult)) { - return beforeSendResult.then( - event => { - if (!isPlainObject(event) && event !== null) { - throw _makeInternalError(invalidValueError); - } - return event; - }, - e => { - throw _makeInternalError(`${beforeSendLabel} rejected with ${e}`); - }, - ); + // A rejection can only be `CALLBACK_ERROR` here, as `safeCallback` already handled the user callback rejecting + return beforeSendResult.then(event => { + if (!isPlainObject(event) && event !== null) { + throw _makeInternalError(invalidValueError); + } + return event; + }); } else if (!isPlainObject(beforeSendResult) && beforeSendResult !== null) { throw _makeInternalError(invalidValueError); } @@ -1743,7 +1746,9 @@ function processBeforeSend( return safeCallback( DEBUG_BUILD ? 'The `beforeSend` callback threw an error, dropping the event:' : '', () => beforeSend(errorEvent, hint), - () => null, + () => { + throw CALLBACK_ERROR; + }, ); } @@ -1818,7 +1823,9 @@ function processBeforeSend( return safeCallback( DEBUG_BUILD ? 'The `beforeSendTransaction` callback threw an error, dropping the event:' : '', () => beforeSendTransaction(processedEvent as TransactionEvent, hint), - () => null, + () => { + throw CALLBACK_ERROR; + }, ); } } diff --git a/packages/core/src/eventProcessors.ts b/packages/core/src/eventProcessors.ts index ef25375d7716..78946463f1b9 100644 --- a/packages/core/src/eventProcessors.ts +++ b/packages/core/src/eventProcessors.ts @@ -3,11 +3,12 @@ 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 { CALLBACK_ERROR, safeCallback } from './utils/safeCallback'; import { rejectedSyncPromise, resolvedSyncPromise } from './utils/syncpromise'; /** * Process an array of event processors, returning the processed event (or `null` if the event was dropped). + * Rejects with `CALLBACK_ERROR` if a processor throws. */ export function notifyEventProcessors( processors: EventProcessor[], @@ -40,7 +41,9 @@ function _notifyEventProcessors( const result = safeCallback( DEBUG_BUILD ? `${processorName} threw an error, dropping event:` : '', () => processor({ ...event }, hint), - () => null, + () => { + throw CALLBACK_ERROR; + }, ); DEBUG_BUILD && result === null && debug.log(`${processorName} dropped event`); diff --git a/packages/core/src/logs/internal.ts b/packages/core/src/logs/internal.ts index 3ad106dfbb32..64bbabad80e6 100644 --- a/packages/core/src/logs/internal.ts +++ b/packages/core/src/logs/internal.ts @@ -8,7 +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 { CALLBACK_ERROR, safeCallback } from '../utils/safeCallback'; import { getCombinedScopeData } from '../utils/scopeData'; import { getActiveSpan } from '../utils/spanUtils'; import { timestampInSeconds } from '../utils/time'; @@ -144,13 +144,17 @@ export function _INTERNAL_captureLog( client.emit('beforeCaptureLog', processedLog); const log = beforeSendLog - ? safeCallback( + ? 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, + () => CALLBACK_ERROR, ) : processedLog; + if (log === CALLBACK_ERROR) { + client.recordDroppedEvent('callback_error', 'log_item', 1); + return; + } if (!log) { client.recordDroppedEvent('before_send', 'log_item', 1); DEBUG_BUILD && debug.warn('beforeSendLog returned null, log will not be captured.'); diff --git a/packages/core/src/metrics/internal.ts b/packages/core/src/metrics/internal.ts index 621992b2ed70..443a5c508e80 100644 --- a/packages/core/src/metrics/internal.ts +++ b/packages/core/src/metrics/internal.ts @@ -8,7 +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 { CALLBACK_ERROR, safeCallback } from '../utils/safeCallback'; import { getCombinedScopeData } from '../utils/scopeData'; import { getActiveSpan } from '../utils/spanUtils'; import { timestampInSeconds } from '../utils/time'; @@ -183,13 +183,18 @@ export function _INTERNAL_captureMetric(beforeMetric: Metric, options?: Internal client.emit('processMetric', enrichedMetric); const processedMetric = beforeSendMetric - ? safeCallback( + ? safeCallback( DEBUG_BUILD ? 'The `beforeSendMetric` callback threw an error, dropping the metric:' : '', () => beforeSendMetric(enrichedMetric), - () => null, + () => CALLBACK_ERROR, ) : enrichedMetric; + if (processedMetric === CALLBACK_ERROR) { + client.recordDroppedEvent('callback_error', 'metric', 1); + return; + } + if (!processedMetric) { client.recordDroppedEvent('before_send', 'metric', 1); DEBUG_BUILD && debug.log('`beforeSendMetric` returned `null`, will not send metric.'); diff --git a/packages/core/src/tracing/sampling.ts b/packages/core/src/tracing/sampling.ts index 2b9e696fc3fe..41268d658af6 100644 --- a/packages/core/src/tracing/sampling.ts +++ b/packages/core/src/tracing/sampling.ts @@ -6,6 +6,14 @@ import { hasSpansEnabled } from '../utils/hasSpansEnabled'; import { parseSampleRate } from '../utils/parseSampleRate'; import { safeCallback } from '../utils/safeCallback'; +interface SamplingDecision { + sampled: boolean; + sampleRate?: number; + localSampleRateWasApplied?: boolean; + /** Set when the span was dropped for a reason other than the sampling decision itself. */ + dropReason?: 'callback_error'; +} + /** * Makes a sampling decision for the given options. * @@ -16,15 +24,17 @@ export function sampleSpan( options: Pick, samplingContext: SamplingContext, sampleRand: number, -): [sampled: boolean, sampleRate?: number, localSampleRateWasApplied?: boolean] { +): SamplingDecision { // nothing to do if span recording is not enabled if (!hasSpansEnabled(options)) { - return [false]; + return { sampled: false }; } const resolved = resolveSampleRate(options, samplingContext); if (!resolved) { - return [false]; + // `hasSpansEnabled` guarantees either `tracesSampleRate` or `tracesSampler` is set, so the only way to end up + // without a sample rate is a throwing `tracesSampler` with nothing to fall back to. + return { sampled: false, dropReason: 'callback_error' }; } const [sampleRate, localSampleRateWasApplied] = resolved; @@ -39,7 +49,7 @@ export function sampleSpan( sampleRate, )} of type ${JSON.stringify(typeof sampleRate)}.`, ); - return [false]; + return { sampled: false }; } // if the function returned 0 (or false), or if `tracesSampleRate` is 0, it's a sign the transaction should be dropped @@ -52,7 +62,7 @@ export function sampleSpan( : 'a negative sampling decision was inherited or tracesSampleRate is set to 0' }`, ); - return [false, parsedSampleRate, localSampleRateWasApplied]; + return { sampled: false, sampleRate: parsedSampleRate, localSampleRateWasApplied }; } // We always compare the sample rand for the current execution context against the chosen sample rate. @@ -69,7 +79,7 @@ export function sampleSpan( ); } - return [shouldSample, parsedSampleRate, localSampleRateWasApplied]; + return { sampled: shouldSample, sampleRate: parsedSampleRate, localSampleRateWasApplied }; } /** diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index b7ac40595830..f7584ef49353 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -495,8 +495,8 @@ function _startRootSpan( const currentPropagationContext = scope.getPropagationContext(); const _isTracingSuppressed = isTracingSuppressed(scope); - const [sampled, sampleRate, localSampleRateWasApplied] = _isTracingSuppressed - ? [false] + const { sampled, sampleRate, localSampleRateWasApplied, dropReason } = _isTracingSuppressed + ? { sampled: false } : sampleSpan( options, { @@ -522,7 +522,7 @@ function _startRootSpan( if (!sampled && client && !_isTracingSuppressed) { DEBUG_BUILD && debug.log('[Tracing] Discarding root span because its trace was not chosen to be sampled.'); - client.recordDroppedEvent('sample_rate', hasSpanStreamingEnabled(client) ? 'span' : 'transaction'); + client.recordDroppedEvent(dropReason || 'sample_rate', hasSpanStreamingEnabled(client) ? 'span' : 'transaction'); } setCapturedScopesOnSpan(rootSpan, scope, isolationScope); diff --git a/packages/core/src/types/clientreport.ts b/packages/core/src/types/clientreport.ts index 154b58c5705e..d9966813ba8e 100644 --- a/packages/core/src/types/clientreport.ts +++ b/packages/core/src/types/clientreport.ts @@ -2,6 +2,7 @@ import type { DataCategory } from './datacategory'; export type EventDropReason = | 'before_send' + | 'callback_error' | 'event_processor' | 'network_error' | 'queue_overflow' diff --git a/packages/core/src/utils/safeCallback.ts b/packages/core/src/utils/safeCallback.ts index 5b9079ca7c5d..e01e97037e28 100644 --- a/packages/core/src/utils/safeCallback.ts +++ b/packages/core/src/utils/safeCallback.ts @@ -2,6 +2,13 @@ import { DEBUG_BUILD } from '../debug-build'; import { debug } from './debug-logger'; import { isThenable } from './is'; +/** + * Lets a `safeCallback` fallback signal "the callback failed" as opposed to "the callback returned `null`", + * so the call site can report the drop as `callback_error`. Return it from synchronous call sites; throw it + * to abort a promise chain. + */ +export const CALLBACK_ERROR = Symbol.for('SentryCallbackError'); + /** * Invokes a user-provided callback (e.g. `beforeSend`, `tracesSampler`, an integration hook) so that * neither a synchronous throw nor a rejected promise escapes into the caller. On failure the error is diff --git a/packages/core/test/lib/client.test.ts b/packages/core/test/lib/client.test.ts index e43c35d2fe63..fae5ff453e18 100644 --- a/packages/core/test/lib/client.test.ts +++ b/packages/core/test/lib/client.test.ts @@ -2222,7 +2222,7 @@ describe('Client', () => { expect(TestClient.instance!.event).toBeUndefined(); expect(captureExceptionSpy).not.toHaveBeenCalled(); - expect(recordDroppedEventSpy).toHaveBeenCalledWith('event_processor', 'error'); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('callback_error', 'error'); expect(debugErrorSpy).toHaveBeenCalledWith('Event processor "?" threw an error, dropping event:', exception); }); @@ -2244,7 +2244,7 @@ describe('Client', () => { expect(TestClient.instance!.event).toBeUndefined(); expect(captureExceptionSpy).not.toHaveBeenCalled(); - expect(recordDroppedEventSpy).toHaveBeenCalledWith('event_processor', 'error'); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('callback_error', 'error'); expect(debugErrorSpy).toHaveBeenCalledWith('Event processor "?" threw an error, dropping event:', exception); }); @@ -2277,7 +2277,7 @@ describe('Client', () => { expect(TestClient.instance!.event).toBeUndefined(); expect(captureExceptionSpy).not.toHaveBeenCalled(); - expect(recordDroppedEventSpy).toHaveBeenCalledWith('event_processor', 'error'); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('callback_error', 'error'); }); test('a rejecting event processor stops the processor chain', async () => { @@ -2312,7 +2312,7 @@ describe('Client', () => { expect(TestClient.instance!.event).toBeUndefined(); expect(captureExceptionSpy).not.toHaveBeenCalled(); - expect(recordDroppedEventSpy).toHaveBeenCalledWith('event_processor', 'error'); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('callback_error', 'error'); }); test('client-level event processor that throws on all events does not capture a new event', () => { @@ -2348,7 +2348,7 @@ describe('Client', () => { expect(beforeSend).toHaveBeenCalledTimes(1); expect(TestClient.instance!.event).toBeUndefined(); expect(captureExceptionSpy).not.toHaveBeenCalled(); - expect(recordDroppedEventSpy).toHaveBeenCalledWith('before_send', 'error'); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('callback_error', 'error'); expect(recordDroppedEventSpy).toHaveBeenCalledTimes(1); expect(debugErrorSpy).toHaveBeenCalledWith( 'The `beforeSend` callback threw an error, dropping the event:', @@ -2373,7 +2373,7 @@ describe('Client', () => { expect(beforeSend).toHaveBeenCalledTimes(1); expect(TestClient.instance!.event).toBeUndefined(); expect(captureExceptionSpy).not.toHaveBeenCalled(); - expect(recordDroppedEventSpy).toHaveBeenCalledWith('before_send', 'error'); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('callback_error', 'error'); expect(debugErrorSpy).toHaveBeenCalledWith( 'The `beforeSend` callback threw an error, dropping the event:', exception, @@ -2416,8 +2416,8 @@ describe('Client', () => { expect(TestClient.instance!.event).toBeUndefined(); expect(captureExceptionSpy).not.toHaveBeenCalled(); - expect(recordDroppedEventSpy).toHaveBeenCalledWith('before_send', 'transaction'); - expect(recordDroppedEventSpy).toHaveBeenCalledWith('before_send', 'span', 3); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('callback_error', 'transaction'); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('callback_error', 'span', 3); expect(debugErrorSpy).toHaveBeenCalledWith( 'The `beforeSendTransaction` callback threw an error, dropping the event:', exception, diff --git a/packages/core/test/lib/eventProcessors.test.ts b/packages/core/test/lib/eventProcessors.test.ts index 5570788cdcaf..0f3a84ba12f2 100644 --- a/packages/core/test/lib/eventProcessors.test.ts +++ b/packages/core/test/lib/eventProcessors.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it, vi } from 'vitest'; import { notifyEventProcessors } from '../../src/eventProcessors'; +import { CALLBACK_ERROR } from '../../src/utils/safeCallback'; import type { EventProcessor } from '../../src/types/eventprocessor'; import * as debugLoggerModule from '../../src/utils/debug-logger'; @@ -24,7 +25,7 @@ describe('notifyEventProcessors', () => { expect(later).not.toHaveBeenCalled(); }); - it('drops the event when a processor throws synchronously', async () => { + it('rejects with `CALLBACK_ERROR` when a processor throws synchronously', async () => { const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error'); const error = new Error('boom'); const throwing: EventProcessor = () => { @@ -33,21 +34,21 @@ describe('notifyEventProcessors', () => { throwing.id = 'Throwing'; const later = vi.fn(event => event); - const result = await notifyEventProcessors([throwing, later], { message: 'hello' }, {}); + await expect(notifyEventProcessors([throwing, later], { message: 'hello' }, {})).rejects.toBe(CALLBACK_ERROR); - expect(result).toBeNull(); expect(later).not.toHaveBeenCalled(); expect(debugErrorSpy).toHaveBeenCalledWith('Event processor "Throwing" threw an error, dropping event:', error); }); - it('drops the event when a processor rejects', async () => { + it('rejects with `CALLBACK_ERROR` when a processor rejects', async () => { const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error'); const error = new Error('boom'); const later = vi.fn(event => event); - const result = await notifyEventProcessors([() => Promise.reject(error), later], { message: 'hello' }, {}); + await expect(notifyEventProcessors([() => Promise.reject(error), later], { message: 'hello' }, {})).rejects.toBe( + CALLBACK_ERROR, + ); - expect(result).toBeNull(); expect(later).not.toHaveBeenCalled(); expect(debugErrorSpy).toHaveBeenCalledWith('Event processor "?" threw an error, dropping event:', error); }); diff --git a/packages/core/test/lib/logs/internal.test.ts b/packages/core/test/lib/logs/internal.test.ts index d34df4ba16e6..fa3797d7dfac 100644 --- a/packages/core/test/lib/logs/internal.test.ts +++ b/packages/core/test/lib/logs/internal.test.ts @@ -389,7 +389,7 @@ describe('_INTERNAL_captureLog', () => { expect(() => _INTERNAL_captureLog({ level: 'info', message: 'test message' }, scope)).not.toThrow(); expect(beforeSendLog).toHaveBeenCalled(); - expect(recordDroppedEventSpy).toHaveBeenCalledWith('before_send', 'log_item', 1); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('callback_error', 'log_item', 1); expect(debugErrorSpy).toHaveBeenCalledWith( 'The `beforeSendLog` callback threw an error, dropping the log:', exception, diff --git a/packages/core/test/lib/metrics/internal.test.ts b/packages/core/test/lib/metrics/internal.test.ts index 95e2a4ccea97..3efda3259dfb 100644 --- a/packages/core/test/lib/metrics/internal.test.ts +++ b/packages/core/test/lib/metrics/internal.test.ts @@ -385,7 +385,7 @@ describe('_INTERNAL_captureMetric', () => { expect(() => _INTERNAL_captureMetric({ type: 'counter', name: 'test.metric', value: 1 }, { scope })).not.toThrow(); expect(beforeSendMetric).toHaveBeenCalled(); - expect(recordDroppedEventSpy).toHaveBeenCalledWith('before_send', 'metric', 1); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('callback_error', 'metric', 1); expect(debugErrorSpy).toHaveBeenCalledWith( 'The `beforeSendMetric` callback threw an error, dropping the metric:', exception, diff --git a/packages/core/test/lib/tracing/sampling.test.ts b/packages/core/test/lib/tracing/sampling.test.ts index 5caa3ea35470..1ef40b367bfc 100644 --- a/packages/core/test/lib/tracing/sampling.test.ts +++ b/packages/core/test/lib/tracing/sampling.test.ts @@ -14,35 +14,36 @@ describe('sampleSpan', () => { it('inherits the parent sampling decision', () => { const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error'); - expect(sampleSpan({ tracesSampler }, { name: 'test', attributes: {}, parentSampled: true }, 0.5)).toEqual([ - true, - 1, - undefined, - ]); - expect(sampleSpan({ tracesSampler }, { name: 'test', attributes: {}, parentSampled: false }, 0.5)).toEqual([ - false, - 0, - undefined, - ]); + expect(sampleSpan({ tracesSampler }, { name: 'test', attributes: {}, parentSampled: true }, 0.5)).toEqual({ + sampled: true, + sampleRate: 1, + }); + expect(sampleSpan({ tracesSampler }, { name: 'test', attributes: {}, parentSampled: false }, 0.5)).toEqual({ + sampled: false, + sampleRate: 0, + }); expect(debugErrorSpy).toHaveBeenCalledWith(expectedMessage, exception); }); it('falls back to `tracesSampleRate` without a parent decision', () => { const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error'); - expect(sampleSpan({ tracesSampler, tracesSampleRate: 0.6 }, { name: 'test', attributes: {} }, 0.5)).toEqual([ - true, - 0.6, - true, - ]); + expect(sampleSpan({ tracesSampler, tracesSampleRate: 0.6 }, { name: 'test', attributes: {} }, 0.5)).toEqual({ + sampled: true, + sampleRate: 0.6, + localSampleRateWasApplied: true, + }); expect(debugErrorSpy).toHaveBeenCalledWith(expectedMessage, exception); }); - it('does not sample when there is nothing to fall back to', () => { + it('does not sample and reports a `callback_error` drop when there is nothing to fall back to', () => { const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error'); const debugWarnSpy = vi.spyOn(debugLoggerModule.debug, 'warn'); - expect(sampleSpan({ tracesSampler }, { name: 'test', attributes: {} }, 0.5)).toEqual([false]); + expect(sampleSpan({ tracesSampler }, { name: 'test', attributes: {} }, 0.5)).toEqual({ + sampled: false, + dropReason: 'callback_error', + }); expect(debugErrorSpy).toHaveBeenCalledWith(expectedMessage, exception); expect(debugWarnSpy).not.toHaveBeenCalled(); }); From 53d1f8d1ff60c081ff4aaa97af5562dacdef8364 Mon Sep 17 00:00:00 2001 From: Martin Sonnberger Date: Thu, 3 Sep 2026 16:17:17 +0200 Subject: [PATCH 2/3] revert to tuple instead of object --- packages/core/src/tracing/sampling.ts | 20 +++------- packages/core/src/tracing/trace.ts | 4 +- .../core/test/lib/tracing/sampling.test.ts | 38 ++++++++++--------- 3 files changed, 29 insertions(+), 33 deletions(-) diff --git a/packages/core/src/tracing/sampling.ts b/packages/core/src/tracing/sampling.ts index 41268d658af6..e036629db9b0 100644 --- a/packages/core/src/tracing/sampling.ts +++ b/packages/core/src/tracing/sampling.ts @@ -6,14 +6,6 @@ import { hasSpansEnabled } from '../utils/hasSpansEnabled'; import { parseSampleRate } from '../utils/parseSampleRate'; import { safeCallback } from '../utils/safeCallback'; -interface SamplingDecision { - sampled: boolean; - sampleRate?: number; - localSampleRateWasApplied?: boolean; - /** Set when the span was dropped for a reason other than the sampling decision itself. */ - dropReason?: 'callback_error'; -} - /** * Makes a sampling decision for the given options. * @@ -24,17 +16,17 @@ export function sampleSpan( options: Pick, samplingContext: SamplingContext, sampleRand: number, -): SamplingDecision { +): [sampled: boolean, sampleRate?: number, localSampleRateWasApplied?: boolean, dropReason?: 'callback_error'] { // nothing to do if span recording is not enabled if (!hasSpansEnabled(options)) { - return { sampled: false }; + return [false]; } const resolved = resolveSampleRate(options, samplingContext); if (!resolved) { // `hasSpansEnabled` guarantees either `tracesSampleRate` or `tracesSampler` is set, so the only way to end up // without a sample rate is a throwing `tracesSampler` with nothing to fall back to. - return { sampled: false, dropReason: 'callback_error' }; + return [false, undefined, undefined, 'callback_error']; } const [sampleRate, localSampleRateWasApplied] = resolved; @@ -49,7 +41,7 @@ export function sampleSpan( sampleRate, )} of type ${JSON.stringify(typeof sampleRate)}.`, ); - return { sampled: false }; + return [false]; } // if the function returned 0 (or false), or if `tracesSampleRate` is 0, it's a sign the transaction should be dropped @@ -62,7 +54,7 @@ export function sampleSpan( : 'a negative sampling decision was inherited or tracesSampleRate is set to 0' }`, ); - return { sampled: false, sampleRate: parsedSampleRate, localSampleRateWasApplied }; + return [false, parsedSampleRate, localSampleRateWasApplied]; } // We always compare the sample rand for the current execution context against the chosen sample rate. @@ -79,7 +71,7 @@ export function sampleSpan( ); } - return { sampled: shouldSample, sampleRate: parsedSampleRate, localSampleRateWasApplied }; + return [shouldSample, parsedSampleRate, localSampleRateWasApplied]; } /** diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index f7584ef49353..fdfaf51d4057 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -495,8 +495,8 @@ function _startRootSpan( const currentPropagationContext = scope.getPropagationContext(); const _isTracingSuppressed = isTracingSuppressed(scope); - const { sampled, sampleRate, localSampleRateWasApplied, dropReason } = _isTracingSuppressed - ? { sampled: false } + const [sampled, sampleRate, localSampleRateWasApplied, dropReason] = _isTracingSuppressed + ? [false] : sampleSpan( options, { diff --git a/packages/core/test/lib/tracing/sampling.test.ts b/packages/core/test/lib/tracing/sampling.test.ts index 1ef40b367bfc..2f78a782a1e5 100644 --- a/packages/core/test/lib/tracing/sampling.test.ts +++ b/packages/core/test/lib/tracing/sampling.test.ts @@ -14,25 +14,27 @@ describe('sampleSpan', () => { it('inherits the parent sampling decision', () => { const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error'); - expect(sampleSpan({ tracesSampler }, { name: 'test', attributes: {}, parentSampled: true }, 0.5)).toEqual({ - sampled: true, - sampleRate: 1, - }); - expect(sampleSpan({ tracesSampler }, { name: 'test', attributes: {}, parentSampled: false }, 0.5)).toEqual({ - sampled: false, - sampleRate: 0, - }); + expect(sampleSpan({ tracesSampler }, { name: 'test', attributes: {}, parentSampled: true }, 0.5)).toEqual([ + true, + 1, + undefined, + ]); + expect(sampleSpan({ tracesSampler }, { name: 'test', attributes: {}, parentSampled: false }, 0.5)).toEqual([ + false, + 0, + undefined, + ]); expect(debugErrorSpy).toHaveBeenCalledWith(expectedMessage, exception); }); it('falls back to `tracesSampleRate` without a parent decision', () => { const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error'); - expect(sampleSpan({ tracesSampler, tracesSampleRate: 0.6 }, { name: 'test', attributes: {} }, 0.5)).toEqual({ - sampled: true, - sampleRate: 0.6, - localSampleRateWasApplied: true, - }); + expect(sampleSpan({ tracesSampler, tracesSampleRate: 0.6 }, { name: 'test', attributes: {} }, 0.5)).toEqual([ + true, + 0.6, + true, + ]); expect(debugErrorSpy).toHaveBeenCalledWith(expectedMessage, exception); }); @@ -40,10 +42,12 @@ describe('sampleSpan', () => { const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error'); const debugWarnSpy = vi.spyOn(debugLoggerModule.debug, 'warn'); - expect(sampleSpan({ tracesSampler }, { name: 'test', attributes: {} }, 0.5)).toEqual({ - sampled: false, - dropReason: 'callback_error', - }); + expect(sampleSpan({ tracesSampler }, { name: 'test', attributes: {} }, 0.5)).toEqual([ + false, + undefined, + undefined, + 'callback_error', + ]); expect(debugErrorSpy).toHaveBeenCalledWith(expectedMessage, exception); expect(debugWarnSpy).not.toHaveBeenCalled(); }); From efbaddbd1aed099d14e5f0a0c2639b5f4f86d650 Mon Sep 17 00:00:00 2001 From: Martin Sonnberger Date: Thu, 3 Sep 2026 17:41:42 +0200 Subject: [PATCH 3/3] fix(core): Report callback errors without sentinel values Treat callback failures as drops at the callback boundary. Event processors and beforeSend hooks now keep their normal Event | null return shape and report callback_error through dedicated callbacks instead of throwing or returning CALLBACK_ERROR. prepareEvent records processor drops, while Client records beforeSend drops. This keeps the sentinel from escaping those pipelines and avoids duplicate client reports. Co-Authored-By: Claude Fable 5.1 Co-Authored-By: OpenAI Codex --- packages/core/src/client.ts | 64 +++++++-------- packages/core/src/eventProcessors.ts | 27 ++++-- packages/core/src/utils/envelope.ts | 9 +- packages/core/src/utils/prepareEvent.ts | 31 +++++-- packages/core/src/utils/safeCallback.ts | 4 +- packages/core/test/lib/client.test.ts | 2 + .../core/test/lib/eventProcessors.test.ts | 50 +++++++++-- packages/core/test/lib/prepareEvent.test.ts | 82 +++++++++++++++++++ .../src/util/sendReplayRequest.ts | 2 - .../test/unit/util/prepareReplayEvent.test.ts | 25 +++++- 10 files changed, 234 insertions(+), 62 deletions(-) diff --git a/packages/core/src/client.ts b/packages/core/src/client.ts index 83cc51c99f17..12d02b348bb2 100644 --- a/packages/core/src/client.ts +++ b/packages/core/src/client.ts @@ -20,7 +20,7 @@ import type { EventDropReason, Outcome } from './types/clientreport'; import type { DataCategory } from './types/datacategory'; import type { DsnComponents } from './types/dsn'; import type { DynamicSamplingContext, Envelope } from './types/envelope'; -import type { ErrorEvent, Event, EventHint, EventType, TransactionEvent } from './types/event'; +import type { ErrorEvent, Event, EventHint, TransactionEvent } from './types/event'; import type { EventProcessor } from './types/eventprocessor'; import type { FeedbackEvent } from './types/feedback'; import type { Integration } from './types/integration'; @@ -41,7 +41,7 @@ import type { ResolvedDataCollection } from './types/datacollection'; import { createClientReportEnvelope } from './utils/clientreport'; import { consoleSandbox, debug } from './utils/debug-logger'; import { dsnToString, makeDsn } from './utils/dsn'; -import { addItemToEnvelope, createAttachmentEnvelopeItem } from './utils/envelope'; +import { addItemToEnvelope, createAttachmentEnvelopeItem, getDataCategoryByType } from './utils/envelope'; import { getPossibleEventMessages } from './utils/eventUtils'; import { isObjectLike, isParameterizedString, isPlainObject, isPrimitive, isThenable } from './utils/is'; import { merge } from './utils/merge'; @@ -50,7 +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 { CALLBACK_ERROR, safeCallback } from './utils/safeCallback'; +import { safeCallback } from './utils/safeCallback'; import { reparentChildSpans, shouldIgnoreSpan } from './utils/should-ignore-span'; import { safeUnref } from './utils/timer'; import { convertSpanJsonToTransactionEvent, convertTransactionEventToSpanJson } from './utils/transactionEvent'; @@ -1515,6 +1515,7 @@ export abstract class Client { const isError = isErrorEvent(event); const eventType = event.type || 'error'; const beforeSendLabel = `before send for type \`${eventType}\``; + let beforeSendDropReason: 'before_send' | 'callback_error' = 'before_send'; // 1.0 === 100% events are sent // 0.0 === 0% events are sent @@ -1522,18 +1523,9 @@ export abstract class Client { const parsedSampleRate = typeof sampleRate === 'undefined' ? undefined : parseSampleRate(sampleRate); const dataCategory = getDataCategoryByType(event.type); - const recordDroppedEvent = (reason: EventDropReason): void => { - this.recordDroppedEvent(reason, dataCategory); - if (isTransaction) { - // the transaction itself counts as one span, plus all the child spans that are added - this.recordDroppedEvent(reason, 'span', 1 + (event.spans || []).length); - } - }; - return this._prepareEvent(event, hint, currentScope, isolationScope) .then(prepared => { if (prepared === null) { - this.recordDroppedEvent('event_processor', dataCategory); throw _makeDoNotSendEventError('An event processor returned `null`, will not send event.'); } @@ -1542,13 +1534,21 @@ export abstract class Client { return prepared; } - const result = processBeforeSend(this, options, prepared, hint); + const result = processBeforeSend(this, options, prepared, hint, () => { + beforeSendDropReason = 'callback_error'; + }); return _validateBeforeSendResult(result, beforeSendLabel); }) .then(processedEvent => { if (processedEvent === null) { - recordDroppedEvent('before_send'); - throw _makeDoNotSendEventError(`${beforeSendLabel} returned \`null\`, will not send event.`); + this.recordDroppedEvent(beforeSendDropReason, dataCategory); + if (isTransaction) { + const spans = event.spans || []; + // the transaction itself counts as one span, plus all the child spans that are added + this.recordDroppedEvent(beforeSendDropReason, 'span', 1 + spans.length); + } + const dropMessage = beforeSendDropReason === 'callback_error' ? 'threw an error' : 'returned `null`'; + throw _makeDoNotSendEventError(`${beforeSendLabel} ${dropMessage}, will not send event.`); } const session = currentScope.getSession() || isolationScope.getSession(); @@ -1589,11 +1589,6 @@ export abstract class Client { return processedEvent; }) .then(null, reason => { - if (reason === CALLBACK_ERROR) { - recordDroppedEvent('callback_error'); - throw _makeDoNotSendEventError('A user callback threw an error, will not send event.'); - } - if (_isDoNotSendEventError(reason) || _isInternalError(reason)) { throw reason; } @@ -1696,10 +1691,6 @@ export abstract class Client { ): PromiseLike; } -function getDataCategoryByType(type: EventType | 'replay_event' | undefined): DataCategory { - return type === 'replay_event' ? 'replay' : type || 'error'; -} - /** * Verifies that return value of configured `beforeSend` or `beforeSendTransaction` is of expected type, and returns the value if so. */ @@ -1709,13 +1700,17 @@ function _validateBeforeSendResult( ): PromiseLike | Event | null { const invalidValueError = `${beforeSendLabel} must return \`null\` or a valid event.`; if (isThenable(beforeSendResult)) { - // A rejection can only be `CALLBACK_ERROR` here, as `safeCallback` already handled the user callback rejecting - return beforeSendResult.then(event => { - if (!isPlainObject(event) && event !== null) { - throw _makeInternalError(invalidValueError); - } - return event; - }); + return beforeSendResult.then( + event => { + if (!isPlainObject(event) && event !== null) { + throw _makeInternalError(invalidValueError); + } + return event; + }, + e => { + throw _makeInternalError(`${beforeSendLabel} rejected with ${e}`); + }, + ); } else if (!isPlainObject(beforeSendResult) && beforeSendResult !== null) { throw _makeInternalError(invalidValueError); } @@ -1730,6 +1725,7 @@ function processBeforeSend( options: ClientOptions, event: Event, hint: EventHint, + onCallbackError: () => void, ): PromiseLike | Event | null { const { beforeSend, @@ -1747,7 +1743,8 @@ function processBeforeSend( DEBUG_BUILD ? 'The `beforeSend` callback threw an error, dropping the event:' : '', () => beforeSend(errorEvent, hint), () => { - throw CALLBACK_ERROR; + onCallbackError(); + return null; }, ); } @@ -1824,7 +1821,8 @@ function processBeforeSend( DEBUG_BUILD ? 'The `beforeSendTransaction` callback threw an error, dropping the event:' : '', () => beforeSendTransaction(processedEvent as TransactionEvent, hint), () => { - throw CALLBACK_ERROR; + onCallbackError(); + return null; }, ); } diff --git a/packages/core/src/eventProcessors.ts b/packages/core/src/eventProcessors.ts index 78946463f1b9..26a39c839538 100644 --- a/packages/core/src/eventProcessors.ts +++ b/packages/core/src/eventProcessors.ts @@ -3,21 +3,23 @@ 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 { CALLBACK_ERROR, safeCallback } from './utils/safeCallback'; +import { safeCallback } from './utils/safeCallback'; import { rejectedSyncPromise, resolvedSyncPromise } from './utils/syncpromise'; +type EventProcessorDropReason = 'event_processor' | 'callback_error'; + /** * Process an array of event processors, returning the processed event (or `null` if the event was dropped). - * Rejects with `CALLBACK_ERROR` if a processor throws. */ export function notifyEventProcessors( processors: EventProcessor[], event: Event | null, hint: EventHint, index: number = 0, + onDrop?: (reason: EventProcessorDropReason) => void, ): PromiseLike { try { - const result = _notifyEventProcessors(event, hint, processors, index); + const result = _notifyEventProcessors(event, hint, processors, index, onDrop); return isThenable(result) ? result : resolvedSyncPromise(result); } catch (error) { return rejectedSyncPromise(error); @@ -29,6 +31,7 @@ function _notifyEventProcessors( hint: EventHint, processors: EventProcessor[], index: number, + onDrop?: (reason: EventProcessorDropReason) => void, ): Event | null | PromiseLike { const processor = processors[index]; @@ -37,20 +40,32 @@ function _notifyEventProcessors( } const processorName = `Event processor "${processor.id || '?'}"`; + let callbackError = false; const result = safeCallback( DEBUG_BUILD ? `${processorName} threw an error, dropping event:` : '', () => processor({ ...event }, hint), () => { - throw CALLBACK_ERROR; + callbackError = true; + return null; }, ); DEBUG_BUILD && result === null && debug.log(`${processorName} dropped event`); if (isThenable(result)) { - return result.then(final => _notifyEventProcessors(final, hint, processors, index + 1)); + return result.then(final => { + if (!final) { + onDrop?.(callbackError ? 'callback_error' : 'event_processor'); + return null; + } + return _notifyEventProcessors(final, hint, processors, index + 1, onDrop); + }); } - return _notifyEventProcessors(result, hint, processors, index + 1); + if (!result) { + onDrop?.(callbackError ? 'callback_error' : 'event_processor'); + return null; + } + return _notifyEventProcessors(result, hint, processors, index + 1, onDrop); } diff --git a/packages/core/src/utils/envelope.ts b/packages/core/src/utils/envelope.ts index 58b3fe353bd6..4eacc1919410 100644 --- a/packages/core/src/utils/envelope.ts +++ b/packages/core/src/utils/envelope.ts @@ -10,7 +10,7 @@ import type { EnvelopeItemType, EventEnvelopeHeaders, } from '../types/envelope'; -import type { Event } from '../types/event'; +import type { Event, EventType } from '../types/event'; import type { SdkInfo } from '../types/sdkinfo'; import type { SdkMetadata } from '../types/sdkmetadata'; import { dsnToString } from './dsn'; @@ -251,3 +251,10 @@ export function createEventEnvelopeHeaders( }), }; } + +/** + * Maps an event type to the data category used for client reports. + */ +export function getDataCategoryByType(type: EventType): DataCategory { + return type === 'replay_event' ? 'replay' : type || 'error'; +} diff --git a/packages/core/src/utils/prepareEvent.ts b/packages/core/src/utils/prepareEvent.ts index 2af0eed8233f..f8951b3b0c75 100644 --- a/packages/core/src/utils/prepareEvent.ts +++ b/packages/core/src/utils/prepareEvent.ts @@ -7,6 +7,7 @@ import type { Event, EventHint } from '../types/event'; import type { ClientOptions } from '../types/options'; import type { StackParser } from '../types/stacktrace'; import { getFilenameToDebugIdMap } from './debug-ids'; +import { getDataCategoryByType } from './envelope'; import { addExceptionMechanismToCapturedException, uuid4 } from './misc'; import { normalize } from './normalize'; import { applyScopeDataToEvent, applySpanToEvent, getCombinedScopeData } from './scopeData'; @@ -36,7 +37,8 @@ export type ExclusiveEventHintOrCaptureContext = * @param event The original event. * @param hint May contain additional information about the original exception. * @param scope A scope containing event metadata. - * @returns A new event with more information. + * @returns A new event with more information, or `null` if an event processor dropped it (or threw). In that case the + * drop has already been recorded on the client, so callers must not record it again. * @hidden */ export function prepareEvent( @@ -102,19 +104,30 @@ export function prepareEvent( // Skip event processors for internal exceptions to prevent recursion // oxlint-disable-next-line typescript/prefer-optional-chain const isInternalException = hint.data && (hint.data as { __sentry__: boolean }).__sentry__ === true; - const result = isInternalException + const result: PromiseLike = isInternalException ? resolvedSyncPromise(prepared) - : notifyEventProcessors(eventProcessors, prepared, hint); + : notifyEventProcessors(eventProcessors, prepared, hint, 0, reason => { + if (!client) { + return; + } + + client.recordDroppedEvent(reason, getDataCategoryByType(event.type)); + if (reason === 'callback_error' && event.type === 'transaction') { + client.recordDroppedEvent(reason, 'span', 1 + (event.spans || []).length); + } + }); return result.then(evt => { - if (evt) { - // We apply the debug_meta field only after all event processors have ran, so that if any event processors modified - // file names (e.g.the RewriteFrames integration) the filename -> debug ID relationship isn't destroyed. - // This should not cause any PII issues, since we're only moving data that is already on the event and not adding - // any new data - applyDebugMeta(evt); + if (!evt) { + return null; } + // We apply the debug_meta field only after all event processors have ran, so that if any event processors modified + // file names (e.g.the RewriteFrames integration) the filename -> debug ID relationship isn't destroyed. + // This should not cause any PII issues, since we're only moving data that is already on the event and not adding + // any new data + applyDebugMeta(evt); + if (typeof normalizeDepth === 'number' && normalizeDepth > 0) { return normalizeEvent(evt, normalizeDepth, normalizeMaxBreadth); } diff --git a/packages/core/src/utils/safeCallback.ts b/packages/core/src/utils/safeCallback.ts index e01e97037e28..15a5d7bd3f79 100644 --- a/packages/core/src/utils/safeCallback.ts +++ b/packages/core/src/utils/safeCallback.ts @@ -4,8 +4,8 @@ import { isThenable } from './is'; /** * Lets a `safeCallback` fallback signal "the callback failed" as opposed to "the callback returned `null`", - * so the call site can report the drop as `callback_error`. Return it from synchronous call sites; throw it - * to abort a promise chain. + * so the call site can report the drop as `callback_error`. Always return it, never throw it: a thrown + * sentinel would have to be caught at every boundary, and some of those (e.g. `prepareEvent`) are public. */ export const CALLBACK_ERROR = Symbol.for('SentryCallbackError'); diff --git a/packages/core/test/lib/client.test.ts b/packages/core/test/lib/client.test.ts index fae5ff453e18..3248dec5ccdb 100644 --- a/packages/core/test/lib/client.test.ts +++ b/packages/core/test/lib/client.test.ts @@ -2374,6 +2374,7 @@ describe('Client', () => { expect(TestClient.instance!.event).toBeUndefined(); expect(captureExceptionSpy).not.toHaveBeenCalled(); expect(recordDroppedEventSpy).toHaveBeenCalledWith('callback_error', 'error'); + expect(recordDroppedEventSpy).toHaveBeenCalledTimes(1); expect(debugErrorSpy).toHaveBeenCalledWith( 'The `beforeSend` callback threw an error, dropping the event:', exception, @@ -2416,6 +2417,7 @@ describe('Client', () => { expect(TestClient.instance!.event).toBeUndefined(); expect(captureExceptionSpy).not.toHaveBeenCalled(); + expect(recordDroppedEventSpy).toHaveBeenCalledTimes(2); expect(recordDroppedEventSpy).toHaveBeenCalledWith('callback_error', 'transaction'); expect(recordDroppedEventSpy).toHaveBeenCalledWith('callback_error', 'span', 3); expect(debugErrorSpy).toHaveBeenCalledWith( diff --git a/packages/core/test/lib/eventProcessors.test.ts b/packages/core/test/lib/eventProcessors.test.ts index 0f3a84ba12f2..3c6cd578c029 100644 --- a/packages/core/test/lib/eventProcessors.test.ts +++ b/packages/core/test/lib/eventProcessors.test.ts @@ -1,6 +1,5 @@ import { describe, expect, it, vi } from 'vitest'; import { notifyEventProcessors } from '../../src/eventProcessors'; -import { CALLBACK_ERROR } from '../../src/utils/safeCallback'; import type { EventProcessor } from '../../src/types/eventprocessor'; import * as debugLoggerModule from '../../src/utils/debug-logger'; @@ -18,14 +17,43 @@ describe('notifyEventProcessors', () => { it('stops when a processor returns null', async () => { const later = vi.fn(event => event); + const onDrop = vi.fn(); - const result = await notifyEventProcessors([() => null, later], { message: 'hello' }, {}); + const result = await notifyEventProcessors([() => null, later], { message: 'hello' }, {}, 0, onDrop); expect(result).toBeNull(); expect(later).not.toHaveBeenCalled(); + expect(onDrop).toHaveBeenCalledOnce(); + expect(onDrop).toHaveBeenCalledWith('event_processor'); }); - it('rejects with `CALLBACK_ERROR` when a processor throws synchronously', async () => { + it('stops and reports the drop when a processor returns undefined', async () => { + const processor = (() => undefined) as unknown as EventProcessor; + const later = vi.fn(event => event); + const onDrop = vi.fn(); + + const result = await notifyEventProcessors([processor, later], { message: 'hello' }, {}, 0, onDrop); + + expect(result).toBeNull(); + expect(later).not.toHaveBeenCalled(); + expect(onDrop).toHaveBeenCalledOnce(); + expect(onDrop).toHaveBeenCalledWith('event_processor'); + }); + + it('stops and reports the drop when a processor resolves undefined', async () => { + const processor = (() => Promise.resolve(undefined)) as unknown as EventProcessor; + const later = vi.fn(event => event); + const onDrop = vi.fn(); + + const result = await notifyEventProcessors([processor, later], { message: 'hello' }, {}, 0, onDrop); + + expect(result).toBeNull(); + expect(later).not.toHaveBeenCalled(); + expect(onDrop).toHaveBeenCalledOnce(); + expect(onDrop).toHaveBeenCalledWith('event_processor'); + }); + + it('resolves with `null` and reports a callback error when a processor throws synchronously', async () => { const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error'); const error = new Error('boom'); const throwing: EventProcessor = () => { @@ -33,23 +61,29 @@ describe('notifyEventProcessors', () => { }; throwing.id = 'Throwing'; const later = vi.fn(event => event); + const onDrop = vi.fn(); - await expect(notifyEventProcessors([throwing, later], { message: 'hello' }, {})).rejects.toBe(CALLBACK_ERROR); + await expect(notifyEventProcessors([throwing, later], { message: 'hello' }, {}, 0, onDrop)).resolves.toBeNull(); expect(later).not.toHaveBeenCalled(); + expect(onDrop).toHaveBeenCalledOnce(); + expect(onDrop).toHaveBeenCalledWith('callback_error'); expect(debugErrorSpy).toHaveBeenCalledWith('Event processor "Throwing" threw an error, dropping event:', error); }); - it('rejects with `CALLBACK_ERROR` when a processor rejects', async () => { + it('resolves with `null` and reports a callback error when a processor rejects', async () => { const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error'); const error = new Error('boom'); const later = vi.fn(event => event); + const onDrop = vi.fn(); - await expect(notifyEventProcessors([() => Promise.reject(error), later], { message: 'hello' }, {})).rejects.toBe( - CALLBACK_ERROR, - ); + await expect( + notifyEventProcessors([() => Promise.reject(error), later], { message: 'hello' }, {}, 0, onDrop), + ).resolves.toBeNull(); expect(later).not.toHaveBeenCalled(); + expect(onDrop).toHaveBeenCalledOnce(); + expect(onDrop).toHaveBeenCalledWith('callback_error'); expect(debugErrorSpy).toHaveBeenCalledWith('Event processor "?" threw an error, dropping event:', error); }); }); diff --git a/packages/core/test/lib/prepareEvent.test.ts b/packages/core/test/lib/prepareEvent.test.ts index bc2f64694e68..62ba08ee9d04 100644 --- a/packages/core/test/lib/prepareEvent.test.ts +++ b/packages/core/test/lib/prepareEvent.test.ts @@ -447,6 +447,88 @@ describe('prepareEvent', () => { }); }); + describe('dropped events', () => { + function createClient(eventProcessor: EventProcessor): Client { + return { + emit() { + // noop + }, + getEventProcessors() { + return [eventProcessor]; + }, + recordDroppedEvent: vi.fn(), + } as unknown as Client; + } + + it('records an `event_processor` drop and resolves `null` when a processor returns `null`', async () => { + const client = createClient(() => null); + + await expect( + prepareEvent({} as ClientOptions, { message: 'foo' }, { integrations: [] }, new Scope(), client), + ).resolves.toBeNull(); + + expect(client.recordDroppedEvent).toHaveBeenCalledWith('event_processor', 'error'); + }); + + it('records `callback_error` transaction and span drops when a processor throws', async () => { + const client = createClient(() => { + throw new Error('sorry'); + }); + + await expect( + prepareEvent( + {} as ClientOptions, + { + type: 'transaction', + transaction: '/checkout', + spans: [ + { + description: 'load cart', + span_id: '9e15bf99fbe4bc80', + start_timestamp: 1591603196.637835, + trace_id: '86f39e84263a4de99c326acab3bfe3bd', + }, + { + description: 'reserve inventory', + span_id: 'aa554c1f506b0783', + start_timestamp: 1591603196.637835, + trace_id: '86f39e84263a4de99c326acab3bfe3bd', + }, + ], + }, + { integrations: [] }, + new Scope(), + client, + ), + ).resolves.toBeNull(); + + expect(client.recordDroppedEvent).toHaveBeenCalledTimes(2); + expect(client.recordDroppedEvent).toHaveBeenCalledWith('callback_error', 'transaction'); + expect(client.recordDroppedEvent).toHaveBeenCalledWith('callback_error', 'span', 3); + }); + + it('records a `callback_error` drop and resolves `null` when a processor rejects', async () => { + const client = createClient(() => Promise.reject(new Error('sorry'))); + + await expect( + prepareEvent({} as ClientOptions, { type: 'replay_event' }, { integrations: [] }, new Scope(), client), + ).resolves.toBeNull(); + + expect(client.recordDroppedEvent).toHaveBeenCalledWith('callback_error', 'replay'); + }); + + it('resolves `null` without a client when a processor throws', async () => { + const scope = new Scope(); + scope.addEventProcessor(() => { + throw new Error('sorry'); + }); + + await expect( + prepareEvent({} as ClientOptions, { message: 'foo' }, { integrations: [] }, scope), + ).resolves.toBeNull(); + }); + }); + it('merges scope data', async () => { const breadcrumb1 = { message: '1', timestamp: 111 } as Breadcrumb; const breadcrumb2 = { message: '2', timestamp: 222 } as Breadcrumb; diff --git a/packages/replay-internal/src/util/sendReplayRequest.ts b/packages/replay-internal/src/util/sendReplayRequest.ts index 6dea2a9821eb..c06e688eb377 100644 --- a/packages/replay-internal/src/util/sendReplayRequest.ts +++ b/packages/replay-internal/src/util/sendReplayRequest.ts @@ -53,8 +53,6 @@ export async function sendReplayRequest({ const replayEvent = await prepareReplayEvent({ scope, client, replayId, event: baseEvent }); if (!replayEvent) { - // Taken from baseclient's `_processEvent` method, where this is handled for errors/transactions - client.recordDroppedEvent('event_processor', 'replay'); DEBUG_BUILD && debug.log('An event processor returned `null`, will not send event.'); return Promise.resolve({}); } diff --git a/packages/replay-internal/test/unit/util/prepareReplayEvent.test.ts b/packages/replay-internal/test/unit/util/prepareReplayEvent.test.ts index 576c1acbba2e..c57c3c405e9a 100644 --- a/packages/replay-internal/test/unit/util/prepareReplayEvent.test.ts +++ b/packages/replay-internal/test/unit/util/prepareReplayEvent.test.ts @@ -1,5 +1,5 @@ import type { ReplayEvent } from '@sentry/core'; -import { getClient, getCurrentScope, setCurrentClient } from '@sentry/core'; +import { getClient, getCurrentScope, Scope, setCurrentClient } from '@sentry/core'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { REPLAY_EVENT_NAME } from '../../../src/constants'; import { prepareReplayEvent } from '../../../src/util/prepareReplayEvent'; @@ -25,6 +25,29 @@ describe('Unit | util | prepareReplayEvent', () => { vi.clearAllMocks(); }); + it('resolves `null` and records a client report when an event processor throws', async () => { + const client = getClient()!; + const scope = new Scope(); + const recordDroppedEventSpy = vi.spyOn(client, 'recordDroppedEvent'); + scope.addEventProcessor(() => { + throw new Error('sorry'); + }); + + const event: ReplayEvent = { + type: REPLAY_EVENT_NAME, + timestamp: 1670837008.634, + error_ids: [], + trace_ids: [], + urls: [], + replay_id: 'replay-ID', + replay_type: 'session', + segment_id: 0, + }; + + await expect(prepareReplayEvent({ scope, client, replayId: 'replay-ID', event })).resolves.toBeNull(); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('callback_error', 'replay'); + }); + it('works', async () => { const client = getClient()!; const scope = getCurrentScope();