From 7c3a0bbd39bcf2e0a8aad2cbe4a3abd9f50301c7 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Mon, 23 Dec 2024 14:49:16 +0100 Subject: [PATCH 01/11] no null return in beforeSendSpan --- packages/core/src/baseclient.ts | 8 ++++---- packages/core/src/envelope.ts | 13 ++++++++----- packages/core/src/types-hoist/options.ts | 2 +- packages/core/src/utils/spanUtils.ts | 2 +- packages/core/test/lib/baseclient.test.ts | 17 ++++++++--------- .../core/test/lib/tracing/sentrySpan.test.ts | 14 +++++++------- 6 files changed, 29 insertions(+), 27 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 80badfe3fa9d..e51f04474c06 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -994,11 +994,11 @@ function processBeforeSend( const processedSpans: SpanJSON[] = []; for (const span of event.spans) { const processedSpan = beforeSendSpan(span); - if (processedSpan) { - processedSpans.push(processedSpan); - } else { + if (!processedSpan) { showSpanDropWarning(); - client.recordDroppedEvent('before_send', 'span'); + processedSpans.push(span); + } else { + processedSpans.push(processedSpan); } } event.spans = processedSpans; diff --git a/packages/core/src/envelope.ts b/packages/core/src/envelope.ts index 999fb0681cf0..0c51ba45305a 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 a16e491c0c7a..f1b773bbcd38 100644 --- a/packages/core/src/types-hoist/options.ts +++ b/packages/core/src/types-hoist/options.ts @@ -300,7 +300,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/test/lib/baseclient.test.ts b/packages/core/test/lib/baseclient.test.ts index 0432235a17a5..b66b49da4fc1 100644 --- a/packages/core/test/lib/baseclient.test.ts +++ b/packages/core/test/lib/baseclient.test.ts @@ -1,3 +1,4 @@ +import type { SpanJSON } from '@sentry/core'; import { SentryError, SyncPromise, dsnToString } from '@sentry/core'; import type { Client, Envelope, ErrorEvent, Event, TransactionEvent } from '../../src/types-hoist'; @@ -1200,15 +1201,14 @@ describe('BaseClient', () => { 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: [ { @@ -1231,12 +1231,11 @@ describe('BaseClient', () => { expect(beforeSendSpan).toHaveBeenCalledTimes(2); 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(); }); From eaa6d886cba92f8b5dda276ee8dc25cc0b2224c8 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Mon, 30 Dec 2024 16:24:48 +0100 Subject: [PATCH 02/11] add convertTransactionEventToSpanJson --- packages/core/src/utils/eventUtils.ts | 33 ++++- .../core/test/lib/utils/eventUtils.test.ts | 116 ++++++++++++++++++ 2 files changed, 148 insertions(+), 1 deletion(-) create mode 100644 packages/core/test/lib/utils/eventUtils.test.ts diff --git a/packages/core/src/utils/eventUtils.ts b/packages/core/src/utils/eventUtils.ts index 3d1fa16eca58..ca8f8d840225 100644 --- a/packages/core/src/utils/eventUtils.ts +++ b/packages/core/src/utils/eventUtils.ts @@ -1,4 +1,7 @@ -import type { Event } from '../types-hoist'; +import { DEBUG_BUILD } from '../debug-build'; +import { SEMANTIC_ATTRIBUTE_EXCLUSIVE_TIME, SEMANTIC_ATTRIBUTE_PROFILE_ID } from '../semanticAttributes'; +import type { Event, SpanJSON } from '../types-hoist'; +import { logger } from '../utils-hoist'; /** * Get a list of possible event messages from a Sentry event. @@ -25,3 +28,31 @@ export function getPossibleEventMessages(event: Event): string[] { return possibleMessages; } + +/** + * Converts a transaction event to a span JSON object. + */ +export function convertTransactionEventToSpanJson(event: Event): SpanJSON | undefined { + if (!(event.type === 'transaction')) { + DEBUG_BUILD && logger.warn('Event is not a transaction, cannot convert to span JSON'); + return; + } + + const { trace_id, parent_span_id, span_id, status, origin, data, op } = event.contexts?.trace ?? {}; + + return { + data: data ?? {}, + description: event.transaction, + op, + parent_span_id: 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, + }; +} diff --git a/packages/core/test/lib/utils/eventUtils.test.ts b/packages/core/test/lib/utils/eventUtils.test.ts new file mode 100644 index 000000000000..c549aaea0f0c --- /dev/null +++ b/packages/core/test/lib/utils/eventUtils.test.ts @@ -0,0 +1,116 @@ +import { convertTransactionEventToSpanJson } from '../../../src/utils/eventUtils'; +import { SEMANTIC_ATTRIBUTE_PROFILE_ID, SEMANTIC_ATTRIBUTE_EXCLUSIVE_TIME } from '../../../src/semanticAttributes'; +import type { Event } from '../../../src/types-hoist'; +import {} from '../../../src/types-hoist'; + +describe('convertTransactionEventToSpanJson', () => { + it('should return undefined for non-transaction events', () => { + const event: Event = { + type: undefined, + }; + + expect(convertTransactionEventToSpanJson(event)).toBeUndefined(); + }); + + it('should convert a minimal transaction event to span JSON', () => { + const event: Event = { + type: 'transaction', + contexts: { + trace: { + trace_id: 'abc123', + span_id: 'span456', + }, + }, + timestamp: 1234567890, + }; + + expect(convertTransactionEventToSpanJson(event)).toEqual({ + data: {}, + description: undefined, + op: undefined, + parent_span_id: '', + span_id: 'span456', + start_timestamp: 0, + status: undefined, + timestamp: 1234567890, + trace_id: 'abc123', + origin: undefined, + profile_id: undefined, + exclusive_time: undefined, + measurements: undefined, + }); + }); + + it('should convert a full transaction event to span JSON', () => { + const event: Event = { + 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' }, + }, + }); + }); + + it('should handle missing contexts.trace', () => { + const event: Event = { + type: 'transaction', + timestamp: 1234567890, + contexts: {}, + }; + + expect(convertTransactionEventToSpanJson(event)).toEqual({ + data: {}, + description: undefined, + op: undefined, + parent_span_id: '', + span_id: '', + start_timestamp: 0, + status: undefined, + timestamp: 1234567890, + trace_id: '', + origin: undefined, + profile_id: undefined, + exclusive_time: undefined, + measurements: undefined, + }); + }); +}); From af0c0c50c496e69ada01ffdded0fd6e0ad955c03 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Mon, 30 Dec 2024 18:12:06 +0100 Subject: [PATCH 03/11] pass root span to beforeSendSpan --- packages/core/src/baseclient.ts | 53 +++++--- packages/core/src/utils/eventUtils.ts | 33 +---- packages/core/src/utils/transactionEvent.ts | 59 +++++++++ packages/core/test/lib/baseclient.test.ts | 16 +-- ...Utils.test.ts => transactionEvent.test.ts} | 113 +++++++++++++----- 5 files changed, 186 insertions(+), 88 deletions(-) create mode 100644 packages/core/src/utils/transactionEvent.ts rename packages/core/test/lib/utils/{eventUtils.test.ts => transactionEvent.test.ts} (52%) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index e51f04474c06..1a086923d897 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -54,6 +54,7 @@ import { getPossibleEventMessages } from './utils/eventUtils'; 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'; @@ -984,41 +985,57 @@ 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) { - showSpanDropWarning(); - processedSpans.push(span); - } else { - processedSpans.push(processedSpan); + 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 = { + ...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/utils/eventUtils.ts b/packages/core/src/utils/eventUtils.ts index ca8f8d840225..3d1fa16eca58 100644 --- a/packages/core/src/utils/eventUtils.ts +++ b/packages/core/src/utils/eventUtils.ts @@ -1,7 +1,4 @@ -import { DEBUG_BUILD } from '../debug-build'; -import { SEMANTIC_ATTRIBUTE_EXCLUSIVE_TIME, SEMANTIC_ATTRIBUTE_PROFILE_ID } from '../semanticAttributes'; -import type { Event, SpanJSON } from '../types-hoist'; -import { logger } from '../utils-hoist'; +import type { Event } from '../types-hoist'; /** * Get a list of possible event messages from a Sentry event. @@ -28,31 +25,3 @@ export function getPossibleEventMessages(event: Event): string[] { return possibleMessages; } - -/** - * Converts a transaction event to a span JSON object. - */ -export function convertTransactionEventToSpanJson(event: Event): SpanJSON | undefined { - if (!(event.type === 'transaction')) { - DEBUG_BUILD && logger.warn('Event is not a transaction, cannot convert to span JSON'); - return; - } - - const { trace_id, parent_span_id, span_id, status, origin, data, op } = event.contexts?.trace ?? {}; - - return { - data: data ?? {}, - description: event.transaction, - op, - parent_span_id: 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, - }; -} diff --git a/packages/core/src/utils/transactionEvent.ts b/packages/core/src/utils/transactionEvent.ts new file mode 100644 index 000000000000..05225bdc8a5b --- /dev/null +++ b/packages/core/src/utils/transactionEvent.ts @@ -0,0 +1,59 @@ +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, + }); +} + +/** + * 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 }), + }, + }, + }, + }; + + if (span.measurements) { + event.measurements = span.measurements; + } + + return dropUndefinedKeys(event); +} diff --git a/packages/core/test/lib/baseclient.test.ts b/packages/core/test/lib/baseclient.test.ts index b66b49da4fc1..6109fd956608 100644 --- a/packages/core/test/lib/baseclient.test.ts +++ b/packages/core/test/lib/baseclient.test.ts @@ -1033,14 +1033,14 @@ describe('BaseClient', () => { }); 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: [ { @@ -1061,9 +1061,10 @@ describe('BaseClient', () => { }; 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', () => { @@ -1123,7 +1124,7 @@ describe('BaseClient', () => { }); test('calls `beforeSendSpan` and uses the modified spans', () => { - expect.assertions(3); + expect.assertions(4); const beforeSendSpan = jest.fn(span => { span.data = { version: 'bravo' }; @@ -1133,7 +1134,7 @@ describe('BaseClient', () => { 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: [ { @@ -1155,12 +1156,13 @@ describe('BaseClient', () => { 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', () => { @@ -1229,7 +1231,7 @@ describe('BaseClient', () => { }; client.captureEvent(transaction); - expect(beforeSendSpan).toHaveBeenCalledTimes(2); + expect(beforeSendSpan).toHaveBeenCalledTimes(3); const capturedEvent = TestClient.instance!.event!; expect(capturedEvent.spans).toHaveLength(2); expect(client['_outcomes']).toEqual({}); diff --git a/packages/core/test/lib/utils/eventUtils.test.ts b/packages/core/test/lib/utils/transactionEvent.test.ts similarity index 52% rename from packages/core/test/lib/utils/eventUtils.test.ts rename to packages/core/test/lib/utils/transactionEvent.test.ts index c549aaea0f0c..3d63d419830b 100644 --- a/packages/core/test/lib/utils/eventUtils.test.ts +++ b/packages/core/test/lib/utils/transactionEvent.test.ts @@ -1,19 +1,14 @@ -import { convertTransactionEventToSpanJson } from '../../../src/utils/eventUtils'; +import { + convertSpanJsonToTransactionEvent, + convertTransactionEventToSpanJson, +} from '../../../src/utils/transactionEvent'; import { SEMANTIC_ATTRIBUTE_PROFILE_ID, SEMANTIC_ATTRIBUTE_EXCLUSIVE_TIME } from '../../../src/semanticAttributes'; -import type { Event } from '../../../src/types-hoist'; +import type { TransactionEvent, SpanJSON } from '../../../src/types-hoist'; import {} from '../../../src/types-hoist'; describe('convertTransactionEventToSpanJson', () => { - it('should return undefined for non-transaction events', () => { - const event: Event = { - type: undefined, - }; - - expect(convertTransactionEventToSpanJson(event)).toBeUndefined(); - }); - it('should convert a minimal transaction event to span JSON', () => { - const event: Event = { + const event: TransactionEvent = { type: 'transaction', contexts: { trace: { @@ -26,23 +21,15 @@ describe('convertTransactionEventToSpanJson', () => { expect(convertTransactionEventToSpanJson(event)).toEqual({ data: {}, - description: undefined, - op: undefined, - parent_span_id: '', span_id: 'span456', start_timestamp: 0, - status: undefined, timestamp: 1234567890, trace_id: 'abc123', - origin: undefined, - profile_id: undefined, - exclusive_time: undefined, - measurements: undefined, }); }); it('should convert a full transaction event to span JSON', () => { - const event: Event = { + const event: TransactionEvent = { type: 'transaction', transaction: 'Test Transaction', contexts: { @@ -91,26 +78,90 @@ describe('convertTransactionEventToSpanJson', () => { }); it('should handle missing contexts.trace', () => { - const event: Event = { + const event: TransactionEvent = { type: 'transaction', - timestamp: 1234567890, contexts: {}, }; expect(convertTransactionEventToSpanJson(event)).toEqual({ data: {}, - description: undefined, - op: undefined, - parent_span_id: '', span_id: '', start_timestamp: 0, - status: undefined, - timestamp: 1234567890, trace_id: '', - origin: undefined, - profile_id: undefined, - exclusive_time: undefined, - measurements: undefined, + }); + }); +}); + +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' }, + }, }); }); }); From 37534eb6ab15059f253d0782598e7878dd85f2da Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Mon, 30 Dec 2024 18:12:20 +0100 Subject: [PATCH 04/11] add migration entry --- docs/migration/v8-to-v9.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/migration/v8-to-v9.md b/docs/migration/v8-to-v9.md index a43a86bd1566..fbb59a251015 100644 --- a/docs/migration/v8-to-v9.md +++ b/docs/migration/v8-to-v9.md @@ -68,6 +68,9 @@ 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. + ### `@sentry/node` - When `skipOpenTelemetrySetup: true` is configured, `httpIntegration({ spans: false })` will be configured by default. This means that you no longer have to specify this yourself in this scenario. With this change, no spans are emitted once `skipOpenTelemetrySetup: true` is configured, without any further configuration being needed. @@ -191,6 +194,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: From bbd3e8cde741e69ee91b8cc0b441f5d31a5b91e7 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Thu, 2 Jan 2025 10:26:31 +0100 Subject: [PATCH 05/11] format md --- docs/migration/v8-to-v9.md | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/migration/v8-to-v9.md b/docs/migration/v8-to-v9.md index 2bd80d4c4253..73b990fa6310 100644 --- a/docs/migration/v8-to-v9.md +++ b/docs/migration/v8-to-v9.md @@ -80,7 +80,6 @@ Sentry.init({ In v9, an `undefined` value will be treated the same as if the value is not defined at all. You'll need to set `tracesSampleRate: 0` if you want to enable tracing without performance. - ### `@sentry/node` - When `skipOpenTelemetrySetup: true` is configured, `httpIntegration({ spans: false })` will be configured by default. This means that you no longer have to specify this yourself in this scenario. With this change, no spans are emitted once `skipOpenTelemetrySetup: true` is configured, without any further configuration being needed. From e5facf351b960f0ba2eadcc52048a73aa386a7ce Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Thu, 2 Jan 2025 10:38:37 +0100 Subject: [PATCH 06/11] biome --- packages/core/test/lib/utils/transactionEvent.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/core/test/lib/utils/transactionEvent.test.ts b/packages/core/test/lib/utils/transactionEvent.test.ts index 3d63d419830b..01fc8758c66d 100644 --- a/packages/core/test/lib/utils/transactionEvent.test.ts +++ b/packages/core/test/lib/utils/transactionEvent.test.ts @@ -1,10 +1,10 @@ +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'; -import { SEMANTIC_ATTRIBUTE_PROFILE_ID, SEMANTIC_ATTRIBUTE_EXCLUSIVE_TIME } from '../../../src/semanticAttributes'; -import type { TransactionEvent, SpanJSON } from '../../../src/types-hoist'; -import {} from '../../../src/types-hoist'; describe('convertTransactionEventToSpanJson', () => { it('should convert a minimal transaction event to span JSON', () => { From d625cdea73748a92d1179354ccb2055c43446f42 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Thu, 2 Jan 2025 14:05:08 +0100 Subject: [PATCH 07/11] apply feedback for checking undefined measurements --- packages/core/src/utils/transactionEvent.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/core/src/utils/transactionEvent.ts b/packages/core/src/utils/transactionEvent.ts index 05225bdc8a5b..3db240d40306 100644 --- a/packages/core/src/utils/transactionEvent.ts +++ b/packages/core/src/utils/transactionEvent.ts @@ -49,11 +49,8 @@ export function convertSpanJsonToTransactionEvent(span: SpanJSON): TransactionEv }, }, }, + measurements: span.measurements, }; - if (span.measurements) { - event.measurements = span.measurements; - } - return dropUndefinedKeys(event); } From 3de5d06cdf42fac03801b969b263ba2e6a84c859 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Thu, 2 Jan 2025 14:40:16 +0100 Subject: [PATCH 08/11] merge processed event --- packages/core/src/baseclient.ts | 6 +- packages/core/test/lib/baseclient.test.ts | 71 +++++++++++++++++++++++ 2 files changed, 73 insertions(+), 4 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 67745cfcf18e..311a0c91791a 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -55,6 +55,7 @@ import { parseSampleRate } from './utils/parseSampleRate'; import { prepareEvent } from './utils/prepareEvent'; import { showSpanDropWarning } from './utils/spanUtils'; import { convertSpanJsonToTransactionEvent, convertTransactionEventToSpanJson } from './utils/transactionEvent'; +import { merge } from './utils/merge'; 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'; @@ -987,10 +988,7 @@ function processBeforeSend( showSpanDropWarning(); } else { // update event with processed root span values - processedEvent = { - ...event, - ...convertSpanJsonToTransactionEvent(processedRootSpanJson), - }; + processedEvent = merge(event, convertSpanJsonToTransactionEvent(processedRootSpanJson)); } // process child spans diff --git a/packages/core/test/lib/baseclient.test.ts b/packages/core/test/lib/baseclient.test.ts index efcde153d310..bc87f61d2d55 100644 --- a/packages/core/test/lib/baseclient.test.ts +++ b/packages/core/test/lib/baseclient.test.ts @@ -1029,6 +1029,77 @@ describe('BaseClient', () => { expect(capturedEvent.transaction).toEqual(transaction.transaction); }); + 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, beforeSendSpan }); + const client = new TestClient(options); + + 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(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 `beforeSend` and uses the modified event', () => { expect.assertions(2); From 2295f648930bd42391b28df3b8bcbac5c0233b75 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Thu, 2 Jan 2025 14:43:01 +0100 Subject: [PATCH 09/11] biome --- packages/core/src/baseclient.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 311a0c91791a..98b349350262 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -51,11 +51,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'; -import { merge } from './utils/merge'; 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'; From de85def0093f85f41e3efbf9f0236516b88f2fda Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Thu, 2 Jan 2025 16:09:43 +0100 Subject: [PATCH 10/11] set is_segmet to true for root spans --- packages/core/src/utils/transactionEvent.ts | 1 + packages/core/test/lib/utils/transactionEvent.test.ts | 3 +++ 2 files changed, 4 insertions(+) diff --git a/packages/core/src/utils/transactionEvent.ts b/packages/core/src/utils/transactionEvent.ts index 3db240d40306..9ec233b4f078 100644 --- a/packages/core/src/utils/transactionEvent.ts +++ b/packages/core/src/utils/transactionEvent.ts @@ -22,6 +22,7 @@ export function convertTransactionEventToSpanJson(event: TransactionEvent): Span 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, }); } diff --git a/packages/core/test/lib/utils/transactionEvent.test.ts b/packages/core/test/lib/utils/transactionEvent.test.ts index 01fc8758c66d..cd5a3cd750c5 100644 --- a/packages/core/test/lib/utils/transactionEvent.test.ts +++ b/packages/core/test/lib/utils/transactionEvent.test.ts @@ -25,6 +25,7 @@ describe('convertTransactionEventToSpanJson', () => { start_timestamp: 0, timestamp: 1234567890, trace_id: 'abc123', + is_segment: true, }); }); @@ -74,6 +75,7 @@ describe('convertTransactionEventToSpanJson', () => { measurements: { fp: { value: 123, unit: 'millisecond' }, }, + is_segment: true, }); }); @@ -88,6 +90,7 @@ describe('convertTransactionEventToSpanJson', () => { span_id: '', start_timestamp: 0, trace_id: '', + is_segment: true, }); }); }); From feae885a61a708e3d8b3fe5911a4492c4ebeba4e Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Wed, 8 Jan 2025 18:42:11 +0100 Subject: [PATCH 11/11] update client tests --- packages/core/test/lib/client.test.ts | 107 ++++++++++++++++++++------ 1 file changed, 82 insertions(+), 25 deletions(-) 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(); });