diff --git a/docs/migration/v8-to-v9.md b/docs/migration/v8-to-v9.md index 56bf9c8bf9c4..588f2607bc15 100644 --- a/docs/migration/v8-to-v9.md +++ b/docs/migration/v8-to-v9.md @@ -68,6 +68,8 @@ Sentry.init({ }); ``` +- Dropping spans in the `beforeSendSpan` hook is no longer possible. +- The `beforeSendSpan` hook now receives the root span as well as the child spans. - In previous versions, we determined if tracing is enabled (for Tracing Without Performance) by checking if either `tracesSampleRate` or `traceSampler` are _defined_ at all, in `Sentry.init()`. This means that e.g. the following config would lead to tracing without performance (=tracing being enabled, even if no spans would be started): ```js @@ -243,6 +245,10 @@ The following outlines deprecations that were introduced in version 8 of the SDK ## General - **Returning `null` from `beforeSendSpan` span is deprecated.** + + Returning `null` from `beforeSendSpan` will now result in a warning being logged. + In v9, dropping spans is not possible anymore within this hook. + - **Passing `undefined` to `tracesSampleRate` / `tracesSampler` / `enableTracing` will be handled differently in v9** In v8, a setup like the following: diff --git a/packages/core/src/client.ts b/packages/core/src/client.ts index 86f0733a965c..d94eaa4270d1 100644 --- a/packages/core/src/client.ts +++ b/packages/core/src/client.ts @@ -52,9 +52,11 @@ import { logger } from './utils-hoist/logger'; import { checkOrSetAlreadyCaught, uuid4 } from './utils-hoist/misc'; import { SyncPromise, rejectedSyncPromise, resolvedSyncPromise } from './utils-hoist/syncpromise'; import { getPossibleEventMessages } from './utils/eventUtils'; +import { merge } from './utils/merge'; import { parseSampleRate } from './utils/parseSampleRate'; import { prepareEvent } from './utils/prepareEvent'; import { showSpanDropWarning } from './utils/spanUtils'; +import { convertSpanJsonToTransactionEvent, convertTransactionEventToSpanJson } from './utils/transactionEvent'; 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'; @@ -1004,41 +1006,54 @@ function processBeforeSend( hint: EventHint, ): PromiseLike | Event | null { const { beforeSend, beforeSendTransaction, beforeSendSpan } = options; + let processedEvent = event; - if (isErrorEvent(event) && beforeSend) { - return beforeSend(event, hint); + if (isErrorEvent(processedEvent) && beforeSend) { + return beforeSend(processedEvent, hint); } - if (isTransactionEvent(event)) { - if (event.spans && beforeSendSpan) { - const processedSpans: SpanJSON[] = []; - for (const span of event.spans) { - const processedSpan = beforeSendSpan(span); - if (processedSpan) { - processedSpans.push(processedSpan); - } else { - showSpanDropWarning(); - client.recordDroppedEvent('before_send', 'span'); + if (isTransactionEvent(processedEvent)) { + if (beforeSendSpan) { + // process root span + const processedRootSpanJson = beforeSendSpan(convertTransactionEventToSpanJson(processedEvent)); + if (!processedRootSpanJson) { + showSpanDropWarning(); + } else { + // update event with processed root span values + processedEvent = merge(event, convertSpanJsonToTransactionEvent(processedRootSpanJson)); + } + + // process child spans + if (processedEvent.spans) { + const processedSpans: SpanJSON[] = []; + for (const span of processedEvent.spans) { + const processedSpan = beforeSendSpan(span); + if (!processedSpan) { + showSpanDropWarning(); + processedSpans.push(span); + } else { + processedSpans.push(processedSpan); + } } + processedEvent.spans = processedSpans; } - event.spans = processedSpans; } if (beforeSendTransaction) { - if (event.spans) { + if (processedEvent.spans) { // We store the # of spans before processing in SDK metadata, // so we can compare it afterwards to determine how many spans were dropped - const spanCountBefore = event.spans.length; - event.sdkProcessingMetadata = { + const spanCountBefore = processedEvent.spans.length; + processedEvent.sdkProcessingMetadata = { ...event.sdkProcessingMetadata, spanCountBeforeProcessing: spanCountBefore, }; } - return beforeSendTransaction(event, hint); + return beforeSendTransaction(processedEvent as TransactionEvent, hint); } } - return event; + return processedEvent; } function isErrorEvent(event: Event): event is ErrorEvent { diff --git a/packages/core/src/envelope.ts b/packages/core/src/envelope.ts index c158eb15ec8e..5244c6625069 100644 --- a/packages/core/src/envelope.ts +++ b/packages/core/src/envelope.ts @@ -18,7 +18,6 @@ import type { SessionItem, SpanEnvelope, SpanItem, - SpanJSON, } from './types-hoist'; import { dsnToString } from './utils-hoist/dsn'; import { @@ -127,13 +126,17 @@ export function createSpanEnvelope(spans: [SentrySpan, ...SentrySpan[]], client? const beforeSendSpan = client && client.getOptions().beforeSendSpan; const convertToSpanJSON = beforeSendSpan ? (span: SentrySpan) => { - const spanJson = beforeSendSpan(spanToJSON(span) as SpanJSON); - if (!spanJson) { + const spanJson = spanToJSON(span); + const processedSpan = beforeSendSpan(spanJson); + + if (!processedSpan) { showSpanDropWarning(); + return spanJson; } - return spanJson; + + return processedSpan; } - : (span: SentrySpan) => spanToJSON(span); + : spanToJSON; const items: SpanItem[] = []; for (const span of spans) { diff --git a/packages/core/src/types-hoist/options.ts b/packages/core/src/types-hoist/options.ts index fdbab9e7603d..80847285a4ef 100644 --- a/packages/core/src/types-hoist/options.ts +++ b/packages/core/src/types-hoist/options.ts @@ -290,7 +290,7 @@ export interface ClientOptions SpanJSON | null; + beforeSendSpan?: (span: SpanJSON) => SpanJSON; /** * An event-processing callback for transaction events, guaranteed to be invoked after all other event diff --git a/packages/core/src/utils/spanUtils.ts b/packages/core/src/utils/spanUtils.ts index c4088fba4942..f012cb117267 100644 --- a/packages/core/src/utils/spanUtils.ts +++ b/packages/core/src/utils/spanUtils.ts @@ -286,7 +286,7 @@ export function showSpanDropWarning(): void { consoleSandbox(() => { // eslint-disable-next-line no-console console.warn( - '[Sentry] Deprecation warning: Returning null from `beforeSendSpan` will be disallowed from SDK version 9.0.0 onwards. The callback will only support mutating spans. To drop certain spans, configure the respective integrations directly.', + '[Sentry] Returning null from `beforeSendSpan` is disallowed. To drop certain spans, configure the respective integrations directly.', ); }); hasShownSpanDropWarning = true; diff --git a/packages/core/src/utils/transactionEvent.ts b/packages/core/src/utils/transactionEvent.ts new file mode 100644 index 000000000000..9ec233b4f078 --- /dev/null +++ b/packages/core/src/utils/transactionEvent.ts @@ -0,0 +1,57 @@ +import { SEMANTIC_ATTRIBUTE_EXCLUSIVE_TIME, SEMANTIC_ATTRIBUTE_PROFILE_ID } from '../semanticAttributes'; +import type { SpanJSON, TransactionEvent } from '../types-hoist'; +import { dropUndefinedKeys } from '../utils-hoist'; + +/** + * Converts a transaction event to a span JSON object. + */ +export function convertTransactionEventToSpanJson(event: TransactionEvent): SpanJSON { + const { trace_id, parent_span_id, span_id, status, origin, data, op } = event.contexts?.trace ?? {}; + + return dropUndefinedKeys({ + data: data ?? {}, + description: event.transaction, + op, + parent_span_id, + span_id: span_id ?? '', + start_timestamp: event.start_timestamp ?? 0, + status, + timestamp: event.timestamp, + trace_id: trace_id ?? '', + origin, + profile_id: data?.[SEMANTIC_ATTRIBUTE_PROFILE_ID] as string | undefined, + exclusive_time: data?.[SEMANTIC_ATTRIBUTE_EXCLUSIVE_TIME] as number | undefined, + measurements: event.measurements, + is_segment: true, + }); +} + +/** + * Converts a span JSON object to a transaction event. + */ +export function convertSpanJsonToTransactionEvent(span: SpanJSON): TransactionEvent { + const event: TransactionEvent = { + type: 'transaction', + timestamp: span.timestamp, + start_timestamp: span.start_timestamp, + transaction: span.description, + contexts: { + trace: { + trace_id: span.trace_id, + span_id: span.span_id, + parent_span_id: span.parent_span_id, + op: span.op, + status: span.status, + origin: span.origin, + data: { + ...span.data, + ...(span.profile_id && { [SEMANTIC_ATTRIBUTE_PROFILE_ID]: span.profile_id }), + ...(span.exclusive_time && { [SEMANTIC_ATTRIBUTE_EXCLUSIVE_TIME]: span.exclusive_time }), + }, + }, + }, + measurements: span.measurements, + }; + + return dropUndefinedKeys(event); +} diff --git a/packages/core/test/lib/client.test.ts b/packages/core/test/lib/client.test.ts index e394b49d2d22..afcb9db1ea0c 100644 --- a/packages/core/test/lib/client.test.ts +++ b/packages/core/test/lib/client.test.ts @@ -13,7 +13,7 @@ import { } from '../../src'; import type { BaseClient, Client } from '../../src/client'; import * as integrationModule from '../../src/integration'; -import type { Envelope, ErrorEvent, Event, TransactionEvent } from '../../src/types-hoist'; +import type { Envelope, ErrorEvent, Event, SpanJSON, TransactionEvent } from '../../src/types-hoist'; import * as loggerModule from '../../src/utils-hoist/logger'; import * as miscModule from '../../src/utils-hoist/misc'; import * as stringModule from '../../src/utils-hoist/string'; @@ -995,14 +995,14 @@ describe('Client', () => { }); test('calls `beforeSendSpan` and uses original spans without any changes', () => { - expect.assertions(2); + expect.assertions(3); const beforeSendSpan = jest.fn(span => span); const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendSpan }); const client = new TestClient(options); const transaction: Event = { - transaction: '/cats/are/great', + transaction: '/dogs/are/great', type: 'transaction', spans: [ { @@ -1023,25 +1023,81 @@ describe('Client', () => { }; client.captureEvent(transaction); - expect(beforeSendSpan).toHaveBeenCalledTimes(2); + expect(beforeSendSpan).toHaveBeenCalledTimes(3); const capturedEvent = TestClient.instance!.event!; expect(capturedEvent.spans).toEqual(transaction.spans); + expect(capturedEvent.transaction).toEqual(transaction.transaction); }); - test('calls `beforeSend` and uses the modified event', () => { - expect.assertions(2); - - const beforeSend = jest.fn(event => { - event.message = 'changed1'; - return event; + test('does not modify existing contexts for root span in `beforeSendSpan`', () => { + const beforeSendSpan = jest.fn((span: SpanJSON) => { + return { + ...span, + data: { + modified: 'true', + }, + }; }); - const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSend }); + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendSpan }); const client = new TestClient(options); - client.captureEvent({ message: 'hello' }); + const transaction: Event = { + transaction: '/animals/are/great', + type: 'transaction', + spans: [], + breadcrumbs: [ + { + type: 'ui.click', + }, + ], + contexts: { + trace: { + data: { + modified: 'false', + dropMe: 'true', + }, + span_id: '9e15bf99fbe4bc80', + trace_id: '86f39e84263a4de99c326acab3bfe3bd', + }, + app: { + data: { + modified: 'false', + }, + }, + }, + }; + client.captureEvent(transaction); - expect(beforeSend).toHaveBeenCalled(); - expect(TestClient.instance!.event!.message).toEqual('changed1'); + expect(beforeSendSpan).toHaveBeenCalledTimes(1); + const capturedEvent = TestClient.instance!.event!; + expect(capturedEvent).toEqual({ + transaction: '/animals/are/great', + breadcrumbs: [ + { + type: 'ui.click', + }, + ], + type: 'transaction', + spans: [], + environment: 'production', + event_id: '12312012123120121231201212312012', + start_timestamp: 0, + timestamp: 2020, + contexts: { + trace: { + data: { + modified: 'true', + }, + span_id: '9e15bf99fbe4bc80', + trace_id: '86f39e84263a4de99c326acab3bfe3bd', + }, + app: { + data: { + modified: 'false', + }, + }, + }, + }); }); test('calls `beforeSendTransaction` and uses the modified event', () => { @@ -1085,7 +1141,7 @@ describe('Client', () => { }); test('calls `beforeSendSpan` and uses the modified spans', () => { - expect.assertions(3); + expect.assertions(4); const beforeSendSpan = jest.fn(span => { span.data = { version: 'bravo' }; @@ -1095,7 +1151,7 @@ describe('Client', () => { const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendSpan }); const client = new TestClient(options); const transaction: Event = { - transaction: '/cats/are/great', + transaction: '/dogs/are/great', type: 'transaction', spans: [ { @@ -1117,12 +1173,13 @@ describe('Client', () => { client.captureEvent(transaction); - expect(beforeSendSpan).toHaveBeenCalledTimes(2); + expect(beforeSendSpan).toHaveBeenCalledTimes(3); const capturedEvent = TestClient.instance!.event!; for (const [idx, span] of capturedEvent.spans!.entries()) { const originalSpan = transaction.spans![idx]; expect(span).toEqual({ ...originalSpan, data: { version: 'bravo' } }); } + expect(capturedEvent.contexts?.trace?.data).toEqual({ version: 'bravo' }); }); test('calls `beforeSend` and discards the event', () => { @@ -1163,15 +1220,15 @@ describe('Client', () => { expect(loggerWarnSpy).toBeCalledWith('before send for type `transaction` returned `null`, will not send event.'); }); - test('calls `beforeSendSpan` and discards the span', () => { + test('does not discard span and warn when returning null from `beforeSendSpan', () => { const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation(() => undefined); - const beforeSendSpan = jest.fn(() => null); + const beforeSendSpan = jest.fn(() => null as unknown as SpanJSON); const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendSpan }); const client = new TestClient(options); const transaction: Event = { - transaction: '/cats/are/great', + transaction: '/dogs/are/great', type: 'transaction', spans: [ { @@ -1192,14 +1249,14 @@ describe('Client', () => { }; client.captureEvent(transaction); - expect(beforeSendSpan).toHaveBeenCalledTimes(2); + expect(beforeSendSpan).toHaveBeenCalledTimes(3); const capturedEvent = TestClient.instance!.event!; - expect(capturedEvent.spans).toHaveLength(0); - expect(client['_outcomes']).toEqual({ 'before_send:span': 2 }); + expect(capturedEvent.spans).toHaveLength(2); + expect(client['_outcomes']).toEqual({}); expect(consoleWarnSpy).toHaveBeenCalledTimes(1); - expect(consoleWarnSpy).toBeCalledWith( - '[Sentry] Deprecation warning: Returning null from `beforeSendSpan` will be disallowed from SDK version 9.0.0 onwards. The callback will only support mutating spans. To drop certain spans, configure the respective integrations directly.', + expect(consoleWarnSpy).toHaveBeenCalledWith( + '[Sentry] Returning null from `beforeSendSpan` is disallowed. To drop certain spans, configure the respective integrations directly.', ); consoleWarnSpy.mockRestore(); }); diff --git a/packages/core/test/lib/tracing/sentrySpan.test.ts b/packages/core/test/lib/tracing/sentrySpan.test.ts index 52f116df8349..cfc3745f4387 100644 --- a/packages/core/test/lib/tracing/sentrySpan.test.ts +++ b/packages/core/test/lib/tracing/sentrySpan.test.ts @@ -1,3 +1,4 @@ +import type { SpanJSON } from '../../../src'; import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, getCurrentScope, setCurrentClient, timestampInSeconds } from '../../../src'; import { SentrySpan } from '../../../src/tracing/sentrySpan'; import { SPAN_STATUS_ERROR } from '../../../src/tracing/spanstatus'; @@ -176,10 +177,10 @@ describe('SentrySpan', () => { expect(mockSend).toHaveBeenCalled(); }); - test('does not send the span if `beforeSendSpan` drops the span', () => { + test('does not drop the span if `beforeSendSpan` returns null', () => { const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation(() => undefined); - const beforeSendSpan = jest.fn(() => null); + const beforeSendSpan = jest.fn(() => null as unknown as SpanJSON); const client = new TestClient( getDefaultTestClientOptions({ dsn: 'https://username@domain/123', @@ -201,12 +202,11 @@ describe('SentrySpan', () => { }); span.end(); - expect(mockSend).not.toHaveBeenCalled(); - expect(recordDroppedEventSpy).toHaveBeenCalledWith('before_send', 'span'); + expect(mockSend).toHaveBeenCalled(); + expect(recordDroppedEventSpy).not.toHaveBeenCalled(); - expect(consoleWarnSpy).toHaveBeenCalledTimes(1); - expect(consoleWarnSpy).toBeCalledWith( - '[Sentry] Deprecation warning: Returning null from `beforeSendSpan` will be disallowed from SDK version 9.0.0 onwards. The callback will only support mutating spans. To drop certain spans, configure the respective integrations directly.', + expect(consoleWarnSpy).toHaveBeenCalledWith( + '[Sentry] Returning null from `beforeSendSpan` is disallowed. To drop certain spans, configure the respective integrations directly.', ); consoleWarnSpy.mockRestore(); }); diff --git a/packages/core/test/lib/utils/transactionEvent.test.ts b/packages/core/test/lib/utils/transactionEvent.test.ts new file mode 100644 index 000000000000..cd5a3cd750c5 --- /dev/null +++ b/packages/core/test/lib/utils/transactionEvent.test.ts @@ -0,0 +1,170 @@ +import { SEMANTIC_ATTRIBUTE_EXCLUSIVE_TIME, SEMANTIC_ATTRIBUTE_PROFILE_ID } from '../../../src/semanticAttributes'; +import type { SpanJSON, TransactionEvent } from '../../../src/types-hoist'; +import {} from '../../../src/types-hoist'; +import { + convertSpanJsonToTransactionEvent, + convertTransactionEventToSpanJson, +} from '../../../src/utils/transactionEvent'; + +describe('convertTransactionEventToSpanJson', () => { + it('should convert a minimal transaction event to span JSON', () => { + const event: TransactionEvent = { + type: 'transaction', + contexts: { + trace: { + trace_id: 'abc123', + span_id: 'span456', + }, + }, + timestamp: 1234567890, + }; + + expect(convertTransactionEventToSpanJson(event)).toEqual({ + data: {}, + span_id: 'span456', + start_timestamp: 0, + timestamp: 1234567890, + trace_id: 'abc123', + is_segment: true, + }); + }); + + it('should convert a full transaction event to span JSON', () => { + const event: TransactionEvent = { + type: 'transaction', + transaction: 'Test Transaction', + contexts: { + trace: { + trace_id: 'abc123', + parent_span_id: 'parent789', + span_id: 'span456', + status: 'ok', + origin: 'manual', + op: 'http', + data: { + [SEMANTIC_ATTRIBUTE_PROFILE_ID]: 'profile123', + [SEMANTIC_ATTRIBUTE_EXCLUSIVE_TIME]: 123.45, + other: 'value', + }, + }, + }, + start_timestamp: 1234567800, + timestamp: 1234567890, + measurements: { + fp: { value: 123, unit: 'millisecond' }, + }, + }; + + expect(convertTransactionEventToSpanJson(event)).toEqual({ + data: { + [SEMANTIC_ATTRIBUTE_PROFILE_ID]: 'profile123', + [SEMANTIC_ATTRIBUTE_EXCLUSIVE_TIME]: 123.45, + other: 'value', + }, + description: 'Test Transaction', + op: 'http', + parent_span_id: 'parent789', + span_id: 'span456', + start_timestamp: 1234567800, + status: 'ok', + timestamp: 1234567890, + trace_id: 'abc123', + origin: 'manual', + profile_id: 'profile123', + exclusive_time: 123.45, + measurements: { + fp: { value: 123, unit: 'millisecond' }, + }, + is_segment: true, + }); + }); + + it('should handle missing contexts.trace', () => { + const event: TransactionEvent = { + type: 'transaction', + contexts: {}, + }; + + expect(convertTransactionEventToSpanJson(event)).toEqual({ + data: {}, + span_id: '', + start_timestamp: 0, + trace_id: '', + is_segment: true, + }); + }); +}); + +describe('convertSpanJsonToTransactionEvent', () => { + it('should convert a minimal span JSON to transaction event', () => { + const span: SpanJSON = { + data: {}, + parent_span_id: '', + span_id: 'span456', + start_timestamp: 0, + timestamp: 1234567890, + trace_id: 'abc123', + }; + + expect(convertSpanJsonToTransactionEvent(span)).toEqual({ + type: 'transaction', + timestamp: 1234567890, + start_timestamp: 0, + contexts: { + trace: { + trace_id: 'abc123', + span_id: 'span456', + parent_span_id: '', + data: {}, + }, + }, + }); + }); + + it('should convert a full span JSON to transaction event', () => { + const span: SpanJSON = { + data: { + other: 'value', + }, + description: 'Test Transaction', + op: 'http', + parent_span_id: 'parent789', + span_id: 'span456', + start_timestamp: 1234567800, + status: 'ok', + timestamp: 1234567890, + trace_id: 'abc123', + origin: 'manual', + profile_id: 'profile123', + exclusive_time: 123.45, + measurements: { + fp: { value: 123, unit: 'millisecond' }, + }, + }; + + expect(convertSpanJsonToTransactionEvent(span)).toEqual({ + type: 'transaction', + timestamp: 1234567890, + start_timestamp: 1234567800, + transaction: 'Test Transaction', + contexts: { + trace: { + trace_id: 'abc123', + span_id: 'span456', + parent_span_id: 'parent789', + op: 'http', + status: 'ok', + origin: 'manual', + data: { + other: 'value', + [SEMANTIC_ATTRIBUTE_PROFILE_ID]: 'profile123', + [SEMANTIC_ATTRIBUTE_EXCLUSIVE_TIME]: 123.45, + }, + }, + }, + measurements: { + fp: { value: 123, unit: 'millisecond' }, + }, + }); + }); +});