diff --git a/packages/core/src/client.ts b/packages/core/src/client.ts index f9a20a930705..fea394ef9435 100644 --- a/packages/core/src/client.ts +++ b/packages/core/src/client.ts @@ -51,11 +51,11 @@ import { prepareEvent } from './utils/prepareEvent'; import { makePromiseBuffer, type PromiseBuffer, SENTRY_BUFFER_FULL_ERROR } from './utils/promisebuffer'; import { safeMathRandom } from './utils/randomSafeContext'; import { reparentChildSpans, shouldIgnoreSpan } from './utils/should-ignore-span'; -import { showSpanDropWarning } from './utils/spanUtils'; import { safeUnref } from './utils/timer'; import { convertSpanJsonToTransactionEvent, convertTransactionEventToSpanJson } from './utils/transactionEvent'; import { maybeWarnAboutIgnoredTransactionOptions } from './utils/warnAboutIgnoredTransactionOptions'; import { resolveDataCollectionOptions } from './utils/data-collection/resolveDataCollectionOptions'; +import { applyBeforeSendSpanCallback } from './tracing/spans/beforeSendSpan'; const ALREADY_SEEN_ERROR = "Not capturing exception because it's already been captured."; const MISSING_RELEASE_FOR_SESSION_ERROR = 'Discarded session because of missing or non-string release'; @@ -1729,13 +1729,9 @@ function processBeforeSend( // 1.2 If a `beforeSendSpan` callback is defined, process the root span if (beforeSendSpan) { - const processedRootSpanJson = beforeSendSpan(rootSpanJson); - if (!processedRootSpanJson) { - showSpanDropWarning(); - } else { - // update event with processed root span values - processedEvent = merge(event, convertSpanJsonToTransactionEvent(processedRootSpanJson)); - } + const processedRootSpanJson = applyBeforeSendSpanCallback(rootSpanJson, beforeSendSpan); + // update event with processed root span values + processedEvent = merge(event, convertSpanJsonToTransactionEvent(processedRootSpanJson)); } // 2. Process child spans @@ -1756,13 +1752,7 @@ function processBeforeSend( // 2.b If a `beforeSendSpan` callback is defined, process the child span if (beforeSendSpan) { - const processedSpan = beforeSendSpan(span); - if (!processedSpan) { - showSpanDropWarning(); - processedSpans.push(span); - } else { - processedSpans.push(processedSpan); - } + processedSpans.push(applyBeforeSendSpanCallback(span, beforeSendSpan)); } else { processedSpans.push(span); } diff --git a/packages/core/src/tracing/spans/beforeSendSpan.ts b/packages/core/src/tracing/spans/beforeSendSpan.ts index 65b4703fc4cb..1d1126d7f883 100644 --- a/packages/core/src/tracing/spans/beforeSendSpan.ts +++ b/packages/core/src/tracing/spans/beforeSendSpan.ts @@ -1,6 +1,8 @@ +import { DEBUG_BUILD } from '../../debug-build'; import type { BeforeSendStaticSpanCallback, BeforeSendStreamedSpanCallback } from '../../types/options'; -import type { StreamedSpanJSON } from '../../types/span'; +import type { SpanJSON, StreamedSpanJSON } from '../../types/span'; import { addNonEnumerableProperty } from '../../utils/object'; +import { consoleSandbox, debug } from '../../utils/debug-logger'; /** * A wrapper to use the static, transaction-based span format in your `beforeSendSpan` callback. @@ -53,3 +55,34 @@ export function withStreamedSpan( export function isStaticBeforeSendSpanCallback(callback: unknown): callback is BeforeSendStaticSpanCallback { return !!callback && typeof callback === 'function' && '_static' in callback && !!callback._static; } + +let hasShownSpanDropWarning = false; +/** + * Apply a user-provided beforeSendSpan callback to a span JSON. + */ +export function applyBeforeSendSpanCallback( + span: T, + beforeSendSpan: (span: T) => T, +): T { + try { + const modifedSpan = beforeSendSpan(span); + if (!modifedSpan) { + if (!hasShownSpanDropWarning) { + consoleSandbox(() => { + // eslint-disable-next-line no-console + console.warn( + '[Sentry] Returning null from `beforeSendSpan` is disallowed. To drop certain spans, configure the respective integrations directly or use `ignoreSpans`.', + ); + }); + hasShownSpanDropWarning = true; + } + return span; + } + return modifedSpan; + } catch (error) { + // Spans are captured synchronously when they end, so a throwing callback would otherwise + // propagate into whatever user code ended the span. + DEBUG_BUILD && debug.error('The `beforeSendSpan` callback threw an error, sending the span unmodified:', error); + return span; + } +} diff --git a/packages/core/src/tracing/spans/captureSpan.ts b/packages/core/src/tracing/spans/captureSpan.ts index 16aee9c8bb65..736e28786fe4 100644 --- a/packages/core/src/tracing/spans/captureSpan.ts +++ b/packages/core/src/tracing/spans/captureSpan.ts @@ -15,13 +15,12 @@ import type { SerializedStreamedSpan, Span, SpanAttributeValue, SpanJSON, Stream import { getCombinedScopeData } from '../../utils/scopeData'; import { INTERNAL_getSegmentSpan, - showSpanDropWarning, spanToJSON, spanToStreamedSpanJSON, streamedSpanJsonToSerializedSpan, } from '../../utils/spanUtils'; import { getCapturedScopesOnSpan } from '../utils'; -import { isStaticBeforeSendSpanCallback } from './beforeSendSpan'; +import { applyBeforeSendSpanCallback, isStaticBeforeSendSpanCallback } from './beforeSendSpan'; import { spanJsonToSerializedStreamedSpan } from './spanJsonToStreamedSpan'; import { scopeContextsToSpanAttributes } from './scopeContextAttributes'; import { DEFAULT_ENVIRONMENT } from '../../constants'; @@ -199,22 +198,7 @@ export function captureStandaloneSpanWithStaticCallback( } }); - const processedSpan = beforeSendSpan(spanJSON) || (showSpanDropWarning(), spanJSON); + const processedSpan = applyBeforeSendSpanCallback(spanJSON, beforeSendSpan); return spanJsonToSerializedStreamedSpan(processedSpan); } - -/** - * Apply a user-provided beforeSendSpan callback to a span JSON. - */ -export function applyBeforeSendSpanCallback( - span: StreamedSpanJSON, - beforeSendSpan: (span: StreamedSpanJSON) => StreamedSpanJSON, -): StreamedSpanJSON { - const modifedSpan = beforeSendSpan(span); - if (!modifedSpan) { - showSpanDropWarning(); - return span; - } - return modifedSpan; -} diff --git a/packages/core/src/utils/spanUtils.ts b/packages/core/src/utils/spanUtils.ts index 84dc4b57039a..6e1d94ddfc0f 100644 --- a/packages/core/src/utils/spanUtils.ts +++ b/packages/core/src/utils/spanUtils.ts @@ -30,15 +30,12 @@ import { addNonEnumerableProperty } from '../utils/object'; import { generateSpanId } from '../utils/propagationContext'; import { timestampInSeconds } from '../utils/time'; import { generateSentryTraceHeader, generateTraceparentHeader } from '../utils/tracing'; -import { consoleSandbox } from './debug-logger'; import { _getSpanForScope } from './spanOnScope'; // These are aligned with OpenTelemetry trace flags export const TRACE_FLAG_NONE = 0x0; export const TRACE_FLAG_SAMPLED = 0x1; -let hasShownSpanDropWarning = false; - /** * Convert a span to a trace context, which can be sent as the `trace` context in an event. * By default, this will only include trace_id, span_id & parent_span_id. @@ -442,21 +439,6 @@ export function getActiveSpan(): Span | undefined { return _getSpanForScope(getCurrentScope()); } -/** - * Logs a warning once if `beforeSendSpan` is used to drop spans. - */ -export function showSpanDropWarning(): void { - if (!hasShownSpanDropWarning) { - consoleSandbox(() => { - // eslint-disable-next-line no-console - console.warn( - '[Sentry] Returning null from `beforeSendSpan` is disallowed. To drop certain spans, configure the respective integrations directly or use `ignoreSpans`.', - ); - }); - hasShownSpanDropWarning = true; - } -} - /** * Updates the name of the given span and ensures that the span name is not * overwritten by the Sentry SDK. diff --git a/packages/core/test/lib/client.test.ts b/packages/core/test/lib/client.test.ts index fc34bb12f045..34b3097aab67 100644 --- a/packages/core/test/lib/client.test.ts +++ b/packages/core/test/lib/client.test.ts @@ -1684,7 +1684,9 @@ describe('Client', () => { test('does not discard span and warn when returning null from `beforeSendSpan', () => { const consoleWarnSpy = vi.spyOn(console, 'warn').mockImplementation(() => undefined); - const beforeSendSpan = withStaticSpan(vi.fn(() => null as unknown as SpanJSON)); + // @ts-expect-error - intentionally violating the type signature here + const beforeSendSpan = withStaticSpan(vi.fn(() => null)); + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendSpan }); const client = new TestClient(options); @@ -1724,6 +1726,57 @@ describe('Client', () => { consoleWarnSpy.mockRestore(); }); + test("doesn't throw if the `beforeSendSpan` callback throws", () => { + const debugErrorSpy = vi.spyOn(debugLoggerModule.debug, 'error').mockImplementation(() => undefined); + const error = new Error('beforeSendSpan is broken'); + const beforeSendSpan = withStaticSpan( + vi.fn(() => { + throw error; + }), + ); + + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendSpan, debug: true }); + const client = new TestClient(options); + + const transaction: Event = { + transaction: '/dogs/are/great', + type: 'transaction', + spans: [ + { + description: 'first span', + span_id: '9e15bf99fbe4bc80', + start_timestamp: 1591603196.637835, + trace_id: '86f39e84263a4de99c326acab3bfe3bd', + data: {}, + status: 'ok', + }, + { + description: 'second span', + span_id: 'aa554c1f506b0783', + start_timestamp: 1591603196.637835, + trace_id: '86f39e84263a4de99c326acab3bfe3bd', + data: {}, + status: 'ok', + }, + ], + }; + + expect(() => client.captureEvent(transaction)).not.toThrow(); + + expect(beforeSendSpan).toHaveBeenCalledTimes(3); + + const capturedEvent = TestClient.instance!.event!; + expect(capturedEvent.spans).toHaveLength(2); + expect(client['_outcomes']).toEqual({}); + + expect(debugErrorSpy).toHaveBeenCalledTimes(3); + expect(debugErrorSpy).toHaveBeenCalledWith( + 'The `beforeSendSpan` callback threw an error, sending the span unmodified:', + error, + ); + debugErrorSpy.mockRestore(); + }); + test('calls `beforeSend` and logs info about invalid return value', () => { const invalidValues = [undefined, false, true, [], 1]; expect.assertions(invalidValues.length * 3); diff --git a/packages/core/test/lib/tracing/spans/captureSpan.test.ts b/packages/core/test/lib/tracing/spans/captureSpan.test.ts index e639da7ff67b..93d029f6a206 100644 --- a/packages/core/test/lib/tracing/spans/captureSpan.test.ts +++ b/packages/core/test/lib/tracing/spans/captureSpan.test.ts @@ -1,7 +1,8 @@ import { describe, expect, it, vi } from 'vitest'; -import type { Contexts, StreamedSpanJSON } from '../../../../src'; +import type { Contexts, Span, StreamedSpanJSON } from '../../../../src'; import { captureSpan, + debug, SEMANTIC_ATTRIBUTE_SENTRY_ENVIRONMENT, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, @@ -13,13 +14,18 @@ import { SEMANTIC_ATTRIBUTE_USER_ID, SEMANTIC_ATTRIBUTE_USER_IP_ADDRESS, SEMANTIC_ATTRIBUTE_USER_USERNAME, + spanStreamingIntegration, startInactiveSpan, startSpan, withStaticSpan, withScope, } from '../../../../src'; -import { safeSetSpanJSONAttributes } from '../../../../src/tracing/spans/captureSpan'; +import { + captureStandaloneSpanWithStaticCallback, + safeSetSpanJSONAttributes, +} from '../../../../src/tracing/spans/captureSpan'; import { scopeContextsToSpanAttributes } from '../../../../src/tracing/spans/scopeContextAttributes'; +import type { TestClientOptions } from '../../../mocks/client'; import { getDefaultTestClientOptions, TestClient } from '../../../mocks/client'; import { SENTRY_SEGMENT_ID, @@ -540,6 +546,137 @@ describe('captureSpan', () => { consoleWarnSpy.mockRestore(); }); + + it('keeps the span and logs an error if the beforeSendSpan callback throws', () => { + const debugErrorSpy = vi.spyOn(debug, 'error').mockImplementation(() => undefined); + const error = new Error('beforeSendSpan is broken'); + // A v10 callback that was not migrated to the streamed format throws like this, because + // `data` doesn't exist on a `StreamedSpanJSON`. + const beforeSendSpan = vi.fn(() => { + throw error; + }); + + const client = new TestClient( + getDefaultTestClientOptions({ + dsn: 'https://dsn@ingest.f00.f00/1', + tracesSampleRate: 1, + traceLifecycle: 'stream', + beforeSendSpan: beforeSendSpan as unknown as TestClientOptions['beforeSendSpan'], + }), + ); + + const span = withScope(scope => { + scope.setClient(client); + const span = startInactiveSpan({ name: 'my-span', attributes: { 'sentry.op': 'http.client' } }); + span.end(); + return span; + }); + + const serialized = captureSpan(span, client); + + expect(serialized.name).toBe('my-span'); + expect(serialized.attributes['sentry.op']).toEqual({ type: 'string', value: 'http.client' }); + expect(debugErrorSpy).toHaveBeenCalledWith( + 'The `beforeSendSpan` callback threw an error, sending the span unmodified:', + error, + ); + + debugErrorSpy.mockRestore(); + }); + + it("doesn't let a throwing beforeSendSpan callback propagate out of span.end()", () => { + const debugErrorSpy = vi.spyOn(debug, 'error').mockImplementation(() => undefined); + const client = new TestClient( + getDefaultTestClientOptions({ + dsn: 'https://dsn@ingest.f00.f00/1', + tracesSampleRate: 1, + traceLifecycle: 'stream', + integrations: [spanStreamingIntegration()], + beforeSendSpan: (() => { + throw new Error('beforeSendSpan is broken'); + }) as unknown as TestClientOptions['beforeSendSpan'], + }), + ); + + // Spans are captured synchronously from the `afterSpanEnd` hook, so a throwing callback + // would otherwise surface in user code that ended the span. + expect(() => + withScope(scope => { + scope.setClient(client); + client.init(); + startSpan({ name: 'my-span' }, () => undefined); + }), + ).not.toThrow(); + + debugErrorSpy.mockRestore(); + }); + }); +}); + +describe('captureStandaloneSpanWithStaticCallback', () => { + it('applies a static beforeSendSpan callback', () => { + const beforeSendSpan = withStaticSpan(vi.fn(span => span)); + + const client = new TestClient( + getDefaultTestClientOptions({ + dsn: 'https://dsn@ingest.f00.f00/1', + tracesSampleRate: 1, + release: '1.0.0', + environment: 'staging', + traceLifecycle: 'static', + beforeSendSpan, + }), + ); + + const span = withScope(scope => { + scope.setClient(client); + const span = startInactiveSpan({ name: 'my-span', attributes: { 'sentry.op': 'http.client' } }); + span.end(); + return span; + }); + + // @ts-expect-error - this is fine because withStaticSpan intentionally lies about its return type + const serialized = captureStandaloneSpanWithStaticCallback(span, client, beforeSendSpan); + + expect(beforeSendSpan).toHaveBeenCalledWith(expect.objectContaining({ span_id: span.spanContext().spanId })); + + expect(serialized.name).toBe('my-span'); + expect(serialized.attributes['sentry.op']).toEqual({ type: 'string', value: 'http.client' }); + }); + + it("doesn't throw if the beforeSendSpan callback throws", () => { + const debugErrorSpy = vi.spyOn(debug, 'error').mockImplementation(() => undefined); + + const error = new Error('beforeSendSpan is broken'); + const beforeSendSpan = vi.fn(() => { + throw error; + }); + + const client = new TestClient( + getDefaultTestClientOptions({ + dsn: 'https://dsn@ingest.f00.f00/1', + }), + ); + + const span = withScope(scope => { + let span: Span | undefined; + + expect(() => { + scope.setClient(client); + span = startInactiveSpan({ name: 'my-span', attributes: { 'sentry.op': 'http.client' } }); + span.end(); + }).not.toThrow(); + + return span; + }); + + expect(() => captureStandaloneSpanWithStaticCallback(span!, client, beforeSendSpan)).not.toThrow(); + expect(debugErrorSpy).toHaveBeenCalledWith( + 'The `beforeSendSpan` callback threw an error, sending the span unmodified:', + error, + ); + + debugErrorSpy.mockRestore(); }); });