From 28d7c17b89a8db8462897b393ac37d5e9d41b6c5 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 16 Apr 2024 15:35:35 +0200 Subject: [PATCH 1/2] fix(node): Ensure DSC is correctly set in envelope headers --- .../error-active-span-unsampled/scenario.ts | 15 +++++ .../error-active-span-unsampled/test.ts | 18 ++++++ .../error-active-span/scenario.ts | 15 +++++ .../envelope-header/error-active-span/test.ts | 20 +++++++ .../tracing/envelope-header/error/scenario.ts | 13 ++++ .../tracing/envelope-header/error/test.ts | 17 ++++++ .../transaction-route/scenario.ts | 26 ++++++++ .../envelope-header/transaction-route/test.ts | 20 +++++++ .../transaction-url/scenario.ts | 26 ++++++++ .../envelope-header/transaction-url/test.ts | 19 ++++++ .../envelope-header/transaction/scenario.ts | 15 +++++ .../envelope-header/transaction/test.ts | 20 +++++++ .../node-integration-tests/utils/runner.ts | 59 ++++++++++++++++++- .../src/setupEventContextTrace.ts | 23 +++----- 14 files changed, 288 insertions(+), 18 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span-unsampled/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span-unsampled/test.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span/test.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/envelope-header/error/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/envelope-header/error/test.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-route/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-route/test.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-url/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-url/test.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction/test.ts diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span-unsampled/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span-unsampled/scenario.ts new file mode 100644 index 000000000000..d88751f77ff5 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span-unsampled/scenario.ts @@ -0,0 +1,15 @@ +import { loggingTransport } from '@sentry-internal/node-integration-tests'; +import * as Sentry from '@sentry/node'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracePropagationTargets: [/\/v0/, 'v1'], + tracesSampleRate: 0, + integrations: [], + transport: loggingTransport, +}); + +Sentry.startSpan({ name: 'test span' }, () => { + Sentry.captureException(new Error('foo')); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span-unsampled/test.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span-unsampled/test.ts new file mode 100644 index 000000000000..b4d971654ea0 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span-unsampled/test.ts @@ -0,0 +1,18 @@ +import { createRunner } from '../../../../utils/runner'; + +test('envelope header for error event during active unsampled span is correct', done => { + createRunner(__dirname, 'scenario.ts') + .ignore('session', 'sessions', 'transaction') + .expectHeader({ + event: { + trace: { + trace_id: expect.any(String), + public_key: 'public', + environment: 'production', + release: '1.0', + sampled: 'false', + }, + }, + }) + .start(done); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span/scenario.ts new file mode 100644 index 000000000000..082db9d94b82 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span/scenario.ts @@ -0,0 +1,15 @@ +import { loggingTransport } from '@sentry-internal/node-integration-tests'; +import * as Sentry from '@sentry/node'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracePropagationTargets: [/\/v0/, 'v1'], + tracesSampleRate: 1, + integrations: [], + transport: loggingTransport, +}); + +Sentry.startSpan({ name: 'test span' }, () => { + Sentry.captureException(new Error('foo')); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span/test.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span/test.ts new file mode 100644 index 000000000000..0e425ac58d55 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span/test.ts @@ -0,0 +1,20 @@ +import { createRunner } from '../../../../utils/runner'; + +test('envelope header for error event during active span is correct', done => { + createRunner(__dirname, 'scenario.ts') + .ignore('session', 'sessions', 'transaction') + .expectHeader({ + event: { + trace: { + trace_id: expect.any(String), + public_key: 'public', + environment: 'production', + release: '1.0', + sample_rate: '1', + sampled: 'true', + transaction: 'test span', + }, + }, + }) + .start(done); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/error/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/error/scenario.ts new file mode 100644 index 000000000000..16eba4ecfd4c --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/envelope-header/error/scenario.ts @@ -0,0 +1,13 @@ +import { loggingTransport } from '@sentry-internal/node-integration-tests'; +import * as Sentry from '@sentry/node'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracePropagationTargets: [/\/v0/, 'v1'], + tracesSampleRate: 0, + integrations: [], + transport: loggingTransport, +}); + +Sentry.captureException(new Error('foo')); diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/error/test.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/error/test.ts new file mode 100644 index 000000000000..e45e18baa29a --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/envelope-header/error/test.ts @@ -0,0 +1,17 @@ +import { createRunner } from '../../../../utils/runner'; + +test('envelope header for error events is correct', done => { + createRunner(__dirname, 'scenario.ts') + .ignore('session', 'sessions') + .expectHeader({ + event: { + trace: { + trace_id: expect.any(String), + environment: 'production', + public_key: 'public', + release: '1.0', + }, + }, + }) + .start(done); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-route/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-route/scenario.ts new file mode 100644 index 000000000000..21bc821787fe --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-route/scenario.ts @@ -0,0 +1,26 @@ +import { loggingTransport } from '@sentry-internal/node-integration-tests'; +import * as Sentry from '@sentry/node'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracePropagationTargets: [/\/v0/, 'v1'], + tracesSampleRate: 1, + integrations: [], + transport: loggingTransport, +}); + +Sentry.startSpan( + { + name: 'GET /route', + attributes: { + 'http.method': 'GET', + 'http.route': '/route', + [Sentry.SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.server', + [Sentry.SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + }, + }, + () => { + // noop + }, +); diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-route/test.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-route/test.ts new file mode 100644 index 000000000000..b64ef4ff55ed --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-route/test.ts @@ -0,0 +1,20 @@ +import { createRunner } from '../../../../utils/runner'; + +test('envelope header for transaction event of route correct', done => { + createRunner(__dirname, 'scenario.ts') + .ignore('session', 'sessions') + .expectHeader({ + transaction: { + trace: { + trace_id: expect.any(String), + public_key: 'public', + transaction: 'GET /route', + environment: 'production', + release: '1.0', + sample_rate: '1', + sampled: 'true', + }, + }, + }) + .start(done); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-url/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-url/scenario.ts new file mode 100644 index 000000000000..a4a5f9290216 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-url/scenario.ts @@ -0,0 +1,26 @@ +import { loggingTransport } from '@sentry-internal/node-integration-tests'; +import * as Sentry from '@sentry/node'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracePropagationTargets: [/\/v0/, 'v1'], + tracesSampleRate: 1, + integrations: [], + transport: loggingTransport, +}); + +Sentry.startSpan( + { + name: 'GET /route/1', + attributes: { + 'http.method': 'GET', + 'http.route': '/route', + [Sentry.SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.server', + [Sentry.SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', + }, + }, + () => { + // noop + }, +); diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-url/test.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-url/test.ts new file mode 100644 index 000000000000..80f187165614 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-url/test.ts @@ -0,0 +1,19 @@ +import { createRunner } from '../../../../utils/runner'; + +test('envelope header for transaction event with source=url correct', done => { + createRunner(__dirname, 'scenario.ts') + .ignore('session', 'sessions') + .expectHeader({ + transaction: { + trace: { + trace_id: expect.any(String), + public_key: 'public', + environment: 'production', + release: '1.0', + sample_rate: '1', + sampled: 'true', + }, + }, + }) + .start(done); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction/scenario.ts new file mode 100644 index 000000000000..7fe727534bc9 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction/scenario.ts @@ -0,0 +1,15 @@ +import { loggingTransport } from '@sentry-internal/node-integration-tests'; +import * as Sentry from '@sentry/node'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracePropagationTargets: [/\/v0/, 'v1'], + tracesSampleRate: 1, + integrations: [], + transport: loggingTransport, +}); + +Sentry.startSpan({ name: 'test span' }, () => { + // noop +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction/test.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction/test.ts new file mode 100644 index 000000000000..10ef348dfaf0 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction/test.ts @@ -0,0 +1,20 @@ +import { createRunner } from '../../../../utils/runner'; + +test('envelope header for transaction event is correct', done => { + createRunner(__dirname, 'scenario.ts') + .ignore('session', 'sessions') + .expectHeader({ + transaction: { + trace: { + trace_id: expect.any(String), + public_key: 'public', + environment: 'production', + release: '1.0', + sample_rate: '1', + sampled: 'true', + transaction: 'test span', + }, + }, + }) + .start(done); +}); diff --git a/dev-packages/node-integration-tests/utils/runner.ts b/dev-packages/node-integration-tests/utils/runner.ts index eb96059d41c5..98d2e6283687 100644 --- a/dev-packages/node-integration-tests/utils/runner.ts +++ b/dev-packages/node-integration-tests/utils/runner.ts @@ -1,6 +1,15 @@ +/* eslint-disable max-lines */ import { spawn, spawnSync } from 'child_process'; import { join } from 'path'; -import type { Envelope, EnvelopeItemType, Event, SerializedSession, SessionAggregates } from '@sentry/types'; +import { SDK_VERSION } from '@sentry/node'; +import type { + Envelope, + EnvelopeItemType, + Event, + EventEnvelope, + SerializedSession, + SessionAggregates, +} from '@sentry/types'; import axios from 'axios'; import { createBasicSentryServer } from './server'; @@ -29,6 +38,18 @@ export function assertSentryTransaction(actual: Event, expected: Partial) }); } +export function assertEnvelopeHeader(actual: Envelope[0], expected: Partial): void { + expect(actual).toEqual({ + event_id: expect.any(String), + sent_at: expect.any(String), + sdk: { + name: 'sentry.javascript.node', + version: SDK_VERSION, + }, + ...expected, + }); +} + const CLEANUP_STEPS = new Set(); export function cleanupChildProcesses(): void { @@ -118,12 +139,19 @@ type Expected = sessions: Partial | ((event: SessionAggregates) => void); }; +type ExpectedEnvelopeHeader = + | { event: Partial } + | { transaction: Partial } + | { session: Partial } + | { sessions: Partial }; + /** Creates a test runner */ // eslint-disable-next-line @typescript-eslint/explicit-function-return-type export function createRunner(...paths: string[]) { const testPath = join(...paths); const expectedEnvelopes: Expected[] = []; + let expectedEnvelopeHeaders: ExpectedEnvelopeHeader[] | undefined = undefined; const flags: string[] = []; const ignored: EnvelopeItemType[] = []; let withEnv: Record = {}; @@ -141,6 +169,14 @@ export function createRunner(...paths: string[]) { expectedEnvelopes.push(expected); return this; }, + expectHeader: function (expected: ExpectedEnvelopeHeader) { + if (!expectedEnvelopeHeaders) { + expectedEnvelopeHeaders = []; + } + + expectedEnvelopeHeaders.push(expected); + return this; + }, expectError: function () { expectError = true; return this; @@ -170,7 +206,7 @@ export function createRunner(...paths: string[]) { return this; }, start: function (done?: (e?: unknown) => void) { - const expectedEnvelopeCount = expectedEnvelopes.length; + const expectedEnvelopeCount = Math.max(expectedEnvelopes.length, (expectedEnvelopeHeaders || []).length); let envelopeCount = 0; let scenarioServerPort: number | undefined; @@ -198,6 +234,25 @@ export function createRunner(...paths: string[]) { continue; } + if (expectedEnvelopeHeaders) { + const header = envelope[0]; + const expected = expectedEnvelopeHeaders.shift()?.[envelopeItemType as keyof ExpectedEnvelopeHeader]; + + try { + if (!expected) { + throw new Error(`No more expected envelope items but we received ${JSON.stringify(header)}`); + } + + assertEnvelopeHeader(header, expected); + + expectCallbackCalled(); + } catch (e) { + complete(e as Error); + } + + return; + } + const expected = expectedEnvelopes.shift(); // Catch any error or failed assertions and pass them to done to end the test quickly diff --git a/packages/opentelemetry/src/setupEventContextTrace.ts b/packages/opentelemetry/src/setupEventContextTrace.ts index 1aa1edbbe12c..c302f1c741cd 100644 --- a/packages/opentelemetry/src/setupEventContextTrace.ts +++ b/packages/opentelemetry/src/setupEventContextTrace.ts @@ -1,30 +1,23 @@ import { getRootSpan } from '@sentry/core'; import type { Client } from '@sentry/types'; -import { dropUndefinedKeys } from '@sentry/utils'; +import { getDynamicSamplingContextFromSpan } from './utils/dynamicSamplingContext'; import { getActiveSpan } from './utils/getActiveSpan'; -import { spanHasName, spanHasParentId } from './utils/spanTypes'; +import { spanHasName } from './utils/spanTypes'; /** Ensure the `trace` context is set on all events. */ export function setupEventContextTrace(client: Client): void { - client.addEventProcessor(event => { + client.on('preprocessEvent', event => { const span = getActiveSpan(); // For transaction events, this is handled separately // Because the active span may not be the span that is actually the transaction event if (!span || event.type === 'transaction') { - return event; + return; } - const spanContext = span.spanContext(); - - // If event has already set `trace` context, use that one. - event.contexts = { - trace: dropUndefinedKeys({ - trace_id: spanContext.traceId, - span_id: spanContext.spanId, - parent_span_id: spanHasParentId(span) ? span.parentSpanId : undefined, - }), - ...event.contexts, + event.sdkProcessingMetadata = { + dynamicSamplingContext: getDynamicSamplingContextFromSpan(span), + ...event.sdkProcessingMetadata, }; const rootSpan = getRootSpan(span); @@ -32,7 +25,5 @@ export function setupEventContextTrace(client: Client): void { if (transactionName && !event.transaction) { event.transaction = transactionName; } - - return event; }); } From e66a4f5cbdd840ff44a3b35a90bd8910543ebe55 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 16 Apr 2024 16:19:05 +0200 Subject: [PATCH 2/2] fix event trace context --- .../opentelemetry/src/setupEventContextTrace.ts | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/opentelemetry/src/setupEventContextTrace.ts b/packages/opentelemetry/src/setupEventContextTrace.ts index c302f1c741cd..c22aa46c57a4 100644 --- a/packages/opentelemetry/src/setupEventContextTrace.ts +++ b/packages/opentelemetry/src/setupEventContextTrace.ts @@ -1,9 +1,10 @@ import { getRootSpan } from '@sentry/core'; import type { Client } from '@sentry/types'; +import { dropUndefinedKeys } from '@sentry/utils'; import { getDynamicSamplingContextFromSpan } from './utils/dynamicSamplingContext'; import { getActiveSpan } from './utils/getActiveSpan'; -import { spanHasName } from './utils/spanTypes'; +import { spanHasName, spanHasParentId } from './utils/spanTypes'; /** Ensure the `trace` context is set on all events. */ export function setupEventContextTrace(client: Client): void { @@ -15,6 +16,18 @@ export function setupEventContextTrace(client: Client): void { return; } + const spanContext = span.spanContext(); + + // If event has already set `trace` context, use that one. + event.contexts = { + trace: dropUndefinedKeys({ + trace_id: spanContext.traceId, + span_id: spanContext.spanId, + parent_span_id: spanHasParentId(span) ? span.parentSpanId : undefined, + }), + ...event.contexts, + }; + event.sdkProcessingMetadata = { dynamicSamplingContext: getDynamicSamplingContextFromSpan(span), ...event.sdkProcessingMetadata,