diff --git a/dev-packages/e2e-tests/test-applications/nestjs-8/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/nestjs-8/tests/errors.test.ts index 2e50c90658ca..e60e94691210 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-8/tests/errors.test.ts +++ b/dev-packages/e2e-tests/test-applications/nestjs-8/tests/errors.test.ts @@ -26,6 +26,7 @@ test('Sends exception to Sentry', async ({ baseURL }) => { expect(errorEvent.contexts?.trace).toEqual({ trace_id: expect.stringMatching(/[a-f0-9]{32}/), span_id: expect.stringMatching(/[a-f0-9]{16}/), + parent_span_id: expect.stringMatching(/[a-f0-9]{16}/), }); }); diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic-with-graphql/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic-with-graphql/tests/errors.test.ts index babc9af99090..62103cfafbd9 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-basic-with-graphql/tests/errors.test.ts +++ b/dev-packages/e2e-tests/test-applications/nestjs-basic-with-graphql/tests/errors.test.ts @@ -26,6 +26,7 @@ test('Sends exception to Sentry', async ({ baseURL }) => { expect(errorEvent.contexts?.trace).toEqual({ trace_id: expect.stringMatching(/[a-f0-9]{32}/), span_id: expect.stringMatching(/[a-f0-9]{16}/), + parent_span_id: expect.stringMatching(/[a-f0-9]{16}/), }); }); @@ -114,5 +115,6 @@ test('Sends graphql exception to Sentry', async ({ baseURL }) => { expect(errorEvent.contexts?.trace).toEqual({ trace_id: expect.stringMatching(/[a-f0-9]{32}/), span_id: expect.stringMatching(/[a-f0-9]{16}/), + parent_span_id: expect.stringMatching(/[a-f0-9]{16}/), }); }); diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/tests/errors.test.ts index 7054f26562cb..748730985cf6 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-basic/tests/errors.test.ts +++ b/dev-packages/e2e-tests/test-applications/nestjs-basic/tests/errors.test.ts @@ -26,6 +26,7 @@ test('Sends exception to Sentry', async ({ baseURL }) => { expect(errorEvent.contexts?.trace).toEqual({ trace_id: expect.stringMatching(/[a-f0-9]{32}/), span_id: expect.stringMatching(/[a-f0-9]{16}/), + parent_span_id: expect.stringMatching(/[a-f0-9]{16}/), }); }); diff --git a/dev-packages/e2e-tests/test-applications/nestjs-graphql/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/nestjs-graphql/tests/errors.test.ts index 0352557e7753..9f9e7cdc0d17 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-graphql/tests/errors.test.ts +++ b/dev-packages/e2e-tests/test-applications/nestjs-graphql/tests/errors.test.ts @@ -45,5 +45,6 @@ test('Sends exception to Sentry', async ({ baseURL }) => { expect(errorEvent.contexts?.trace).toEqual({ trace_id: expect.stringMatching(/[a-f0-9]{32}/), span_id: expect.stringMatching(/[a-f0-9]{16}/), + parent_span_id: expect.stringMatching(/[a-f0-9]{16}/), }); }); diff --git a/dev-packages/e2e-tests/test-applications/nestjs-with-submodules-decorator/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/nestjs-with-submodules-decorator/tests/errors.test.ts index 89fd7a990138..fc7520fbcb7b 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-with-submodules-decorator/tests/errors.test.ts +++ b/dev-packages/e2e-tests/test-applications/nestjs-with-submodules-decorator/tests/errors.test.ts @@ -34,6 +34,7 @@ test('Sends unexpected exception to Sentry if thrown in module with global filte expect(errorEvent.contexts?.trace).toEqual({ trace_id: expect.stringMatching(/[a-f0-9]{32}/), span_id: expect.stringMatching(/[a-f0-9]{16}/), + parent_span_id: expect.stringMatching(/[a-f0-9]{16}/), }); }); @@ -70,6 +71,7 @@ test('Sends unexpected exception to Sentry if thrown in module with local filter expect(errorEvent.contexts?.trace).toEqual({ trace_id: expect.stringMatching(/[a-f0-9]{32}/), span_id: expect.stringMatching(/[a-f0-9]{16}/), + parent_span_id: expect.stringMatching(/[a-f0-9]{16}/), }); }); @@ -108,6 +110,7 @@ test('Sends unexpected exception to Sentry if thrown in module that was register expect(errorEvent.contexts?.trace).toEqual({ trace_id: expect.stringMatching(/[a-f0-9]{32}/), span_id: expect.stringMatching(/[a-f0-9]{16}/), + parent_span_id: expect.stringMatching(/[a-f0-9]{16}/), }); }); diff --git a/dev-packages/e2e-tests/test-applications/nestjs-with-submodules/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/nestjs-with-submodules/tests/errors.test.ts index 5501f058abbd..e29d4677397f 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-with-submodules/tests/errors.test.ts +++ b/dev-packages/e2e-tests/test-applications/nestjs-with-submodules/tests/errors.test.ts @@ -26,6 +26,7 @@ test('Sends unexpected exception to Sentry if thrown in module with global filte expect(errorEvent.contexts?.trace).toEqual({ trace_id: expect.stringMatching(/[a-f0-9]{32}/), span_id: expect.stringMatching(/[a-f0-9]{16}/), + parent_span_id: expect.stringMatching(/[a-f0-9]{16}/), }); }); @@ -54,6 +55,7 @@ test('Sends unexpected exception to Sentry if thrown in module with local filter expect(errorEvent.contexts?.trace).toEqual({ trace_id: expect.stringMatching(/[a-f0-9]{32}/), span_id: expect.stringMatching(/[a-f0-9]{16}/), + parent_span_id: expect.stringMatching(/[a-f0-9]{16}/), }); }); @@ -84,6 +86,7 @@ test('Sends unexpected exception to Sentry if thrown in module that was register expect(errorEvent.contexts?.trace).toEqual({ trace_id: expect.stringMatching(/[a-f0-9]{32}/), span_id: expect.stringMatching(/[a-f0-9]{16}/), + parent_span_id: expect.stringMatching(/[a-f0-9]{16}/), }); }); diff --git a/dev-packages/e2e-tests/test-applications/node-fastify-5/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/node-fastify-5/tests/errors.test.ts index 55a59fc90080..f79eb30e9b4c 100644 --- a/dev-packages/e2e-tests/test-applications/node-fastify-5/tests/errors.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-fastify-5/tests/errors.test.ts @@ -25,5 +25,6 @@ test('Sends correct error event', async ({ baseURL }) => { expect(errorEvent.contexts?.trace).toEqual({ trace_id: expect.stringMatching(/[a-f0-9]{32}/), span_id: expect.stringMatching(/[a-f0-9]{16}/), + parent_span_id: expect.stringMatching(/[a-f0-9]{16}/), }); }); diff --git a/dev-packages/e2e-tests/test-applications/node-fastify/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/node-fastify/tests/errors.test.ts index 4e147b969199..1b63fe0e0c55 100644 --- a/dev-packages/e2e-tests/test-applications/node-fastify/tests/errors.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-fastify/tests/errors.test.ts @@ -25,5 +25,6 @@ test('Sends correct error event', async ({ baseURL }) => { expect(errorEvent.contexts?.trace).toEqual({ trace_id: expect.stringMatching(/[a-f0-9]{32}/), span_id: expect.stringMatching(/[a-f0-9]{16}/), + parent_span_id: expect.stringMatching(/[a-f0-9]{16}/), }); }); diff --git a/dev-packages/e2e-tests/test-applications/node-koa/tests/assert.test.ts b/dev-packages/e2e-tests/test-applications/node-koa/tests/assert.test.ts index 1180303eb708..e274f451b959 100644 --- a/dev-packages/e2e-tests/test-applications/node-koa/tests/assert.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-koa/tests/assert.test.ts @@ -26,6 +26,7 @@ test('Returns 400 from failed assert', async ({ baseURL }) => { expect(errorEvent.contexts?.trace).toEqual({ trace_id: expect.stringMatching(/[a-f0-9]{32}/), span_id: expect.stringMatching(/[a-f0-9]{16}/), + parent_span_id: expect.stringMatching(/[a-f0-9]{16}/), }); }); diff --git a/dev-packages/e2e-tests/test-applications/node-koa/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/node-koa/tests/errors.test.ts index f28bb17372bd..cadf29fbda90 100644 --- a/dev-packages/e2e-tests/test-applications/node-koa/tests/errors.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-koa/tests/errors.test.ts @@ -25,5 +25,6 @@ test('Sends correct error event', async ({ baseURL }) => { expect(errorEvent.contexts?.trace).toEqual({ trace_id: expect.stringMatching(/[a-f0-9]{32}/), span_id: expect.stringMatching(/[a-f0-9]{16}/), + parent_span_id: expect.stringMatching(/[a-f0-9]{16}/), }); }); diff --git a/dev-packages/e2e-tests/test-applications/node-otel-without-tracing/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/node-otel-without-tracing/tests/transactions.test.ts index bb069b7e3e11..678841bdb249 100644 --- a/dev-packages/e2e-tests/test-applications/node-otel-without-tracing/tests/transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-otel-without-tracing/tests/transactions.test.ts @@ -34,17 +34,55 @@ test('Sends an API route transaction to OTLP', async ({ baseURL }) => { const scopeSpans = json.resourceSpans?.[0]?.scopeSpans; expect(scopeSpans).toBeDefined(); - // Http server span & undici client spans are emitted + // Http server span & undici client spans are emitted, + // as well as the spans emitted via `Sentry.startSpan()` // But our default node-fetch spans are not emitted - expect(scopeSpans.length).toEqual(2); + expect(scopeSpans.length).toEqual(3); const httpScopes = scopeSpans?.filter(scopeSpan => scopeSpan.scope.name === '@opentelemetry/instrumentation-http'); const undiciScopes = scopeSpans?.filter( scopeSpan => scopeSpan.scope.name === '@opentelemetry/instrumentation-undici', ); + const startSpanScopes = scopeSpans?.filter(scopeSpan => scopeSpan.scope.name === '@sentry/node'); expect(httpScopes.length).toBe(1); + expect(startSpanScopes.length).toBe(1); + expect(startSpanScopes[0].spans.length).toBe(2); + expect(startSpanScopes[0].spans).toEqual([ + { + traceId: expect.any(String), + spanId: expect.any(String), + parentSpanId: expect.any(String), + name: 'test-span', + kind: 1, + startTimeUnixNano: expect.any(String), + endTimeUnixNano: expect.any(String), + attributes: [], + droppedAttributesCount: 0, + events: [], + droppedEventsCount: 0, + status: { code: 0 }, + links: [], + droppedLinksCount: 0, + }, + { + traceId: expect.any(String), + spanId: expect.any(String), + name: 'test-transaction', + kind: 1, + startTimeUnixNano: expect.any(String), + endTimeUnixNano: expect.any(String), + attributes: [{ key: 'sentry.op', value: { stringValue: 'e2e-test' } }], + droppedAttributesCount: 0, + events: [], + droppedEventsCount: 0, + status: { code: 0 }, + links: [], + droppedLinksCount: 0, + }, + ]); + // Undici spans are emitted correctly expect(undiciScopes.length).toBe(1); expect(undiciScopes[0].spans.length).toBe(1); diff --git a/dev-packages/node-integration-tests/suites/aws-serverless/aws-integration/s3/test.ts b/dev-packages/node-integration-tests/suites/aws-serverless/aws-integration/s3/test.ts index 6ffc6f6303fa..10d3f4bdd005 100644 --- a/dev-packages/node-integration-tests/suites/aws-serverless/aws-integration/s3/test.ts +++ b/dev-packages/node-integration-tests/suites/aws-serverless/aws-integration/s3/test.ts @@ -26,6 +26,6 @@ describe('awsIntegration', () => { }); test('should auto-instrument aws-sdk v2 package.', done => { - createRunner(__dirname, 'scenario.js').expect({ transaction: EXPECTED_TRANSCATION }).start(done); + createRunner(__dirname, 'scenario.js').ignore('event').expect({ transaction: EXPECTED_TRANSCATION }).start(done); }); }); diff --git a/dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-spans-in-scope-with-parentSpanId/scenario.ts b/dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-spans-in-scope-with-parentSpanId/scenario.ts index ec761a7d591d..7c4f702f5df8 100644 --- a/dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-spans-in-scope-with-parentSpanId/scenario.ts +++ b/dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-spans-in-scope-with-parentSpanId/scenario.ts @@ -15,6 +15,18 @@ Sentry.withScope(scope => { traceId: '12345678901234567890123456789012', }); - Sentry.startSpan({ name: 'test_span_1' }, () => undefined); - Sentry.startSpan({ name: 'test_span_2' }, () => undefined); + const spanIdTraceId = Sentry.startSpan( + { + name: 'test_span_1', + }, + span1 => span1.spanContext().traceId, + ); + + Sentry.startSpan( + { + name: 'test_span_2', + attributes: { spanIdTraceId }, + }, + () => undefined, + ); }); diff --git a/dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-spans-in-scope-with-parentSpanId/test.ts b/dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-spans-in-scope-with-parentSpanId/test.ts index b0f79680eea7..ecc45e46b4a0 100644 --- a/dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-spans-in-scope-with-parentSpanId/test.ts +++ b/dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-spans-in-scope-with-parentSpanId/test.ts @@ -6,28 +6,19 @@ afterAll(() => { test('should send manually started parallel root spans outside of root context with parentSpanId', done => { createRunner(__dirname, 'scenario.ts') + .expect({ transaction: { transaction: 'test_span_1' } }) .expect({ - transaction: { - transaction: 'test_span_1', - contexts: { - trace: { - span_id: expect.stringMatching(/[a-f0-9]{16}/), - parent_span_id: '1234567890123456', - trace_id: '12345678901234567890123456789012', - }, - }, - }, - }) - .expect({ - transaction: { - transaction: 'test_span_2', - contexts: { - trace: { - span_id: expect.stringMatching(/[a-f0-9]{16}/), - parent_span_id: '1234567890123456', - trace_id: '12345678901234567890123456789012', - }, - }, + transaction: transaction => { + expect(transaction).toBeDefined(); + const traceId = transaction.contexts?.trace?.trace_id; + expect(traceId).toBeDefined(); + expect(transaction.contexts?.trace?.parent_span_id).toBeUndefined(); + + const trace1Id = transaction.contexts?.trace?.data?.spanIdTraceId; + expect(trace1Id).toBeDefined(); + + // Different trace ID as the first span + expect(trace1Id).not.toBe(traceId); }, }) .start(done); diff --git a/dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-spans-in-scope/test.ts b/dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-spans-in-scope/test.ts index 97ceaa1e382c..58cf67c7c69a 100644 --- a/dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-spans-in-scope/test.ts +++ b/dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-spans-in-scope/test.ts @@ -19,8 +19,8 @@ test('should send manually started parallel root spans outside of root context', const trace1Id = transaction.contexts?.trace?.data?.spanIdTraceId; expect(trace1Id).toBeDefined(); - // Same trace ID as the first span - expect(trace1Id).toBe(traceId); + // Different trace ID as the first span + expect(trace1Id).not.toBe(traceId); }, }) .start(done); diff --git a/docs/assets/node-sdk-trace-propagation.png b/docs/assets/node-sdk-trace-propagation.png new file mode 100644 index 000000000000..f4d5c7bf1b5f Binary files /dev/null and b/docs/assets/node-sdk-trace-propagation.png differ diff --git a/docs/trace-propagation.md b/docs/trace-propagation.md new file mode 100644 index 000000000000..14de136135e1 --- /dev/null +++ b/docs/trace-propagation.md @@ -0,0 +1,14 @@ +# How Trace Propagation Works in the JavaScript SDKs + +Trace propagation describes how and when traceId & spanId are set and send for various types of events. +How this behaves varies a bit from Browser to Node SDKs. + +## Node SDKs (OpenTelemetry based) + +In the Node SDK and related OpenTelemetry-based SDKs, trace propagation works as follows: + +![node-sdk-trace-propagation-scenarios](./assets/node-sdk-trace-propagation.png) + +## Browser/Other SDKs + +TODO diff --git a/packages/core/src/asyncContext/stackStrategy.ts b/packages/core/src/asyncContext/stackStrategy.ts index 0e1a84961f5a..68c72fb8e92d 100644 --- a/packages/core/src/asyncContext/stackStrategy.ts +++ b/packages/core/src/asyncContext/stackStrategy.ts @@ -2,7 +2,6 @@ import { getDefaultCurrentScope, getDefaultIsolationScope } from '../defaultScop import { Scope } from '../scope'; import type { Client, Scope as ScopeInterface } from '../types-hoist'; import { isThenable } from '../utils-hoist/is'; - import { getMainCarrier, getSentryCarrier } from './../carrier'; import type { AsyncContextStrategy } from './types'; diff --git a/packages/core/src/tracing/sentryNonRecordingSpan.ts b/packages/core/src/tracing/sentryNonRecordingSpan.ts index 89997cb0ad35..e7c9a8e9ac41 100644 --- a/packages/core/src/tracing/sentryNonRecordingSpan.ts +++ b/packages/core/src/tracing/sentryNonRecordingSpan.ts @@ -7,7 +7,7 @@ import type { SpanStatus, SpanTimeInput, } from '../types-hoist'; -import { uuid4 } from '../utils-hoist/misc'; +import { generateSpanId, generateTraceId } from '../utils-hoist/propagationContext'; import { TRACE_FLAG_NONE } from '../utils/spanUtils'; /** @@ -18,8 +18,8 @@ export class SentryNonRecordingSpan implements Span { private _spanId: string; public constructor(spanContext: SentrySpanArguments = {}) { - this._traceId = spanContext.traceId || uuid4(); - this._spanId = spanContext.spanId || uuid4().substring(16); + this._traceId = spanContext.traceId || generateTraceId(); + this._spanId = spanContext.spanId || generateSpanId(); } /** @inheritdoc */ diff --git a/packages/core/src/tracing/sentrySpan.ts b/packages/core/src/tracing/sentrySpan.ts index fcc071c39603..126702dfad2b 100644 --- a/packages/core/src/tracing/sentrySpan.ts +++ b/packages/core/src/tracing/sentrySpan.ts @@ -25,8 +25,8 @@ import type { TransactionSource, } from '../types-hoist'; import { logger } from '../utils-hoist/logger'; -import { uuid4 } from '../utils-hoist/misc'; import { dropUndefinedKeys } from '../utils-hoist/object'; +import { generateSpanId, generateTraceId } from '../utils-hoist/propagationContext'; import { timestampInSeconds } from '../utils-hoist/time'; import { TRACE_FLAG_NONE, @@ -75,8 +75,8 @@ export class SentrySpan implements Span { * @hidden */ public constructor(spanContext: SentrySpanArguments = {}) { - this._traceId = spanContext.traceId || uuid4(); - this._spanId = spanContext.spanId || uuid4().substring(16); + this._traceId = spanContext.traceId || generateTraceId(); + this._spanId = spanContext.spanId || generateSpanId(); this._startTime = spanContext.startTimestamp || timestampInSeconds(); this._attributes = {}; diff --git a/packages/core/src/utils-hoist/tracing.ts b/packages/core/src/utils-hoist/tracing.ts index e3f984048ae8..eb649ff34d6c 100644 --- a/packages/core/src/utils-hoist/tracing.ts +++ b/packages/core/src/utils-hoist/tracing.ts @@ -1,7 +1,6 @@ import type { PropagationContext, TraceparentData } from '../types-hoist'; import { baggageHeaderToDynamicSamplingContext } from './baggage'; -import { uuid4 } from './misc'; import { generateSpanId, generateTraceId } from './propagationContext'; // eslint-disable-next-line @sentry-internal/sdk/no-regexp-constructor -- RegExp is used for readability here @@ -76,8 +75,8 @@ export function propagationContextFromHeaders( * Create sentry-trace header from span context values. */ export function generateSentryTraceHeader( - traceId: string = uuid4(), - spanId: string = uuid4().substring(16), + traceId: string = generateTraceId(), + spanId: string = generateSpanId(), sampled?: boolean, ): string { let sampledString = ''; diff --git a/packages/core/src/utils/spanUtils.ts b/packages/core/src/utils/spanUtils.ts index 0926117ae7b0..594a297f9395 100644 --- a/packages/core/src/utils/spanUtils.ts +++ b/packages/core/src/utils/spanUtils.ts @@ -19,6 +19,7 @@ import type { } from '../types-hoist'; import { consoleSandbox } from '../utils-hoist/logger'; import { addNonEnumerableProperty, dropUndefinedKeys } from '../utils-hoist/object'; +import { generateSpanId } from '../utils-hoist/propagationContext'; import { timestampInSeconds } from '../utils-hoist/time'; import { generateSentryTraceHeader } from '../utils-hoist/tracing'; import { _getSpanForScope } from './spanOnScope'; @@ -54,10 +55,18 @@ export function spanToTransactionTraceContext(span: Span): TraceContext { * Convert a span to a trace context, which can be sent as the `trace` context in a non-transaction event. */ export function spanToTraceContext(span: Span): TraceContext { - const { spanId: span_id, traceId: trace_id } = span.spanContext(); - const { parent_span_id } = spanToJSON(span); + const { spanId, traceId: trace_id, isRemote } = span.spanContext(); + + // If the span is remote, we use a random/virtual span as span_id to the trace context, + // and the remote span as parent_span_id + const parent_span_id = isRemote ? spanId : spanToJSON(span).parent_span_id; + const span_id = isRemote ? generateSpanId() : spanId; - return dropUndefinedKeys({ parent_span_id, span_id, trace_id }); + return dropUndefinedKeys({ + parent_span_id, + span_id, + trace_id, + }); } /** diff --git a/packages/core/test/lib/utils/spanUtils.test.ts b/packages/core/test/lib/utils/spanUtils.test.ts index edd594bfac5b..f7187695a025 100644 --- a/packages/core/test/lib/utils/spanUtils.test.ts +++ b/packages/core/test/lib/utils/spanUtils.test.ts @@ -14,9 +14,48 @@ import { } from '../../../src'; import type { Span, SpanAttributes, SpanStatus, SpanTimeInput } from '../../../src/types-hoist'; import type { OpenTelemetrySdkTraceBaseSpan } from '../../../src/utils/spanUtils'; +import { spanToTraceContext } from '../../../src/utils/spanUtils'; import { getRootSpan, spanIsSampled, spanTimeInputToSeconds, spanToJSON } from '../../../src/utils/spanUtils'; import { TestClient, getDefaultTestClientOptions } from '../../mocks/client'; +function createMockedOtelSpan({ + spanId, + traceId, + isRemote, + attributes = {}, + startTime = Date.now(), + name = 'test span', + status = { code: SPAN_STATUS_UNSET }, + endTime = Date.now(), + parentSpanId, +}: { + spanId: string; + traceId: string; + attributes?: SpanAttributes; + startTime?: SpanTimeInput; + isRemote?: boolean; + name?: string; + status?: SpanStatus; + endTime?: SpanTimeInput; + parentSpanId?: string; +}): Span { + return { + spanContext: () => { + return { + spanId, + traceId, + isRemote, + }; + }, + attributes, + startTime, + name, + status, + endTime, + parentSpanId, + } as OpenTelemetrySdkTraceBaseSpan; +} + describe('spanToTraceHeader', () => { test('simple', () => { const span = new SentrySpan(); @@ -28,6 +67,88 @@ describe('spanToTraceHeader', () => { }); }); +describe('spanToTraceContext', () => { + it('works with a minimal span', () => { + const span = new SentrySpan({ spanId: '1234', traceId: 'ABCD' }); + + expect(spanToTraceContext(span)).toEqual({ + span_id: '1234', + trace_id: 'ABCD', + }); + }); + + it('works with a span with parentSpanId', () => { + const span = new SentrySpan({ + spanId: '1234', + traceId: 'ABCD', + parentSpanId: '5678', + }); + + expect(spanToTraceContext(span)).toEqual({ + span_id: '1234', + trace_id: 'ABCD', + parent_span_id: '5678', + }); + }); + + it('works with a local OTEL span', () => { + const span = createMockedOtelSpan({ + spanId: '1234', + traceId: 'ABCD', + isRemote: false, + }); + + expect(spanToTraceContext(span)).toEqual({ + span_id: '1234', + trace_id: 'ABCD', + }); + }); + + it('works with a local OTEL span with parentSpanId', () => { + const span = createMockedOtelSpan({ + spanId: '1234', + traceId: 'ABCD', + isRemote: false, + parentSpanId: 'XYZ', + }); + + expect(spanToTraceContext(span)).toEqual({ + parent_span_id: 'XYZ', + span_id: '1234', + trace_id: 'ABCD', + }); + }); + + it('works with a remote OTEL span', () => { + const span = createMockedOtelSpan({ + spanId: '1234', + traceId: 'ABCD', + isRemote: true, + }); + + expect(spanToTraceContext(span)).toEqual({ + parent_span_id: '1234', + span_id: expect.stringMatching(/^[0-9a-f]{16}$/), + trace_id: 'ABCD', + }); + }); + + it('works with a remote OTEL span with parentSpanId', () => { + const span = createMockedOtelSpan({ + spanId: '1234', + traceId: 'ABCD', + isRemote: true, + parentSpanId: 'XYZ', + }); + + expect(spanToTraceContext(span)).toEqual({ + parent_span_id: '1234', + span_id: expect.stringMatching(/^[0-9a-f]{16}$/), + trace_id: 'ABCD', + }); + }); +}); + describe('spanTimeInputToSeconds', () => { it('works with undefined', () => { const now = timestampInSeconds(); @@ -111,41 +232,6 @@ describe('spanToJSON', () => { }); describe('OpenTelemetry Span', () => { - function createMockedOtelSpan({ - spanId, - traceId, - attributes, - startTime, - name, - status, - endTime, - parentSpanId, - }: { - spanId: string; - traceId: string; - attributes: SpanAttributes; - startTime: SpanTimeInput; - name: string; - status: SpanStatus; - endTime: SpanTimeInput; - parentSpanId?: string; - }): Span { - return { - spanContext: () => { - return { - spanId, - traceId, - }; - }, - attributes, - startTime, - name, - status, - endTime, - parentSpanId, - } as OpenTelemetrySdkTraceBaseSpan; - } - it('works with a simple span', () => { const span = createMockedOtelSpan({ spanId: 'SPAN-1', diff --git a/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts b/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts index 521e45df4e27..4b764aeeed8e 100644 --- a/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts +++ b/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts @@ -6,6 +6,8 @@ import { SPAN_STATUS_OK, Scope, captureException, + generateSpanId, + generateTraceId, getActiveSpan, getCapturedScopesOnSpan, getClient, @@ -14,7 +16,6 @@ import { propagationContextFromHeaders, setCapturedScopesOnSpan, startSpanManual, - uuid4, winterCGHeadersToDict, withIsolationScope, withScope, @@ -88,8 +89,8 @@ export function wrapGenerationFunctionWithSentry a headersDict?.['sentry-trace'] ? propagationContextFromHeaders(headersDict['sentry-trace'], headersDict['baggage']) : { - traceId: requestTraceId || uuid4(), - spanId: uuid4().substring(16), + traceId: requestTraceId || generateTraceId(), + spanId: generateSpanId(), }, ); scope.setPropagationContext(propagationContext); diff --git a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts index ff91823faf43..0d4ffe5f32d1 100644 --- a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts +++ b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts @@ -6,6 +6,8 @@ import { SPAN_STATUS_OK, Scope, captureException, + generateSpanId, + generateTraceId, getActiveSpan, getCapturedScopesOnSpan, getRootSpan, @@ -13,7 +15,6 @@ import { propagationContextFromHeaders, setCapturedScopesOnSpan, startSpanManual, - uuid4, vercelWaitUntil, winterCGHeadersToDict, withIsolationScope, @@ -67,8 +68,8 @@ export function wrapServerComponentWithSentry any> headersDict?.['sentry-trace'] ? propagationContextFromHeaders(headersDict['sentry-trace'], headersDict['baggage']) : { - traceId: requestTraceId || uuid4(), - spanId: uuid4().substring(16), + traceId: requestTraceId || generateTraceId(), + spanId: generateSpanId(), }, ); diff --git a/packages/opentelemetry/src/constants.ts b/packages/opentelemetry/src/constants.ts index 0f056d09a4ea..22e3d01c46a8 100644 --- a/packages/opentelemetry/src/constants.ts +++ b/packages/opentelemetry/src/constants.ts @@ -4,7 +4,6 @@ export const SENTRY_TRACE_HEADER = 'sentry-trace'; export const SENTRY_BAGGAGE_HEADER = 'baggage'; export const SENTRY_TRACE_STATE_DSC = 'sentry.dsc'; -export const SENTRY_TRACE_STATE_PARENT_SPAN_ID = 'sentry.parent_span_id'; export const SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING = 'sentry.sampled_not_recording'; export const SENTRY_TRACE_STATE_URL = 'sentry.url'; diff --git a/packages/opentelemetry/src/propagator.ts b/packages/opentelemetry/src/propagator.ts index 74f5452960df..09ba10a173a4 100644 --- a/packages/opentelemetry/src/propagator.ts +++ b/packages/opentelemetry/src/propagator.ts @@ -8,6 +8,7 @@ import { SENTRY_BAGGAGE_KEY_PREFIX, baggageHeaderToDynamicSamplingContext, generateSentryTraceHeader, + generateSpanId, getClient, getCurrentScope, getDynamicSamplingContextFromScope, @@ -24,7 +25,6 @@ import { SENTRY_BAGGAGE_HEADER, SENTRY_TRACE_HEADER, SENTRY_TRACE_STATE_DSC, - SENTRY_TRACE_STATE_PARENT_SPAN_ID, SENTRY_TRACE_STATE_URL, } from './constants'; import { DEBUG_BUILD } from './debug-build'; @@ -32,6 +32,7 @@ import { getScopesFromContext, setScopesOnContext } from './utils/contextData'; import { getSamplingDecision } from './utils/getSamplingDecision'; import { makeTraceState } from './utils/makeTraceState'; import { setIsSetup } from './utils/setupCheck'; +import { spanHasParentId } from './utils/spanTypes'; /** Get the Sentry propagation context from a span context. */ export function getPropagationContextFromSpan(span: Span): PropagationContext { @@ -43,8 +44,7 @@ export function getPropagationContextFromSpan(span: Span): PropagationContext { const dscString = traceState ? traceState.get(SENTRY_TRACE_STATE_DSC) : undefined; const traceStateDsc = dscString ? baggageHeaderToDynamicSamplingContext(dscString) : undefined; - const parentSpanId = traceState ? traceState.get(SENTRY_TRACE_STATE_PARENT_SPAN_ID) || undefined : undefined; - + const parentSpanId = spanHasParentId(span) ? span.parentSpanId : undefined; const sampled = getSamplingDecision(spanContext); // No trace state? --> Take DSC from root span @@ -141,20 +141,9 @@ export class SentryPropagator extends W3CBaggagePropagator { : maybeSentryTraceHeader : undefined; - const propagationContext = propagationContextFromHeaders(sentryTrace, baggage); - // Add remote parent span context - const ctxWithSpanContext = getContextWithRemoteActiveSpan(context, { sentryTrace, baggage }); - - // Also update the scope on the context (to be sure this is picked up everywhere) - const scopes = getScopesFromContext(ctxWithSpanContext); - const newScopes = { - scope: scopes ? scopes.scope.clone() : getCurrentScope().clone(), - isolationScope: scopes ? scopes.isolationScope : getIsolationScope(), - }; - newScopes.scope.setPropagationContext(propagationContext); - - return setScopesOnContext(ctxWithSpanContext, newScopes); + // If there is no incoming trace, this will return the context as-is + return ensureScopesOnContext(getContextWithRemoteActiveSpan(context, { sentryTrace, baggage })); } /** @@ -205,13 +194,28 @@ export function getInjectionData(context: Context): { sampled: boolean | undefined; } { const span = trace.getSpan(context); - const spanIsRemote = span?.spanContext().isRemote; - // If we have a local span, we can just pick everything from it - if (span && !spanIsRemote) { + // If we have a remote span, the spanId should be considered as the parentSpanId, not spanId itself + // Instead, we use a virtual (generated) spanId for propagation + if (span && span.spanContext().isRemote) { const spanContext = span.spanContext(); + const dynamicSamplingContext = getDynamicSamplingContextFromSpan(span); + return { + dynamicSamplingContext, + traceId: spanContext.traceId, + // Because this is a remote span, we do not want to propagate this directly + // As otherwise things may be attached "directly" to an unrelated span + spanId: generateSpanId(), + sampled: getSamplingDecision(spanContext), + }; + } + + // If we have a local span, we just use this + if (span) { + const spanContext = span.spanContext(); const dynamicSamplingContext = getDynamicSamplingContextFromSpan(span); + return { dynamicSamplingContext, traceId: spanContext.traceId, @@ -221,6 +225,7 @@ export function getInjectionData(context: Context): { } // Else we try to use the propagation context from the scope + // The only scenario where this should happen is when we neither have a span, nor an incoming trace const scope = getScopesFromContext(context)?.scope || getCurrentScope(); const client = getClient(); @@ -242,7 +247,21 @@ function getContextWithRemoteActiveSpan( ): Context { const propagationContext = propagationContextFromHeaders(sentryTrace, baggage); - const spanContext = generateSpanContextForPropagationContext(propagationContext); + const { traceId, parentSpanId, sampled, dsc } = propagationContext; + + // We only want to set the virtual span if we are continuing a concrete trace + // Otherwise, we ignore the incoming trace here, e.g. if we have no trace headers + if (!parentSpanId) { + return ctx; + } + + const spanContext = generateRemoteSpanContext({ + traceId, + spanId: parentSpanId, + sampled, + dsc, + }); + return trace.setSpanContext(ctx, spanContext); } @@ -255,11 +274,24 @@ export function continueTraceAsRemoteSpan( options: Parameters[0], callback: () => T, ): T { - const ctxWithSpanContext = getContextWithRemoteActiveSpan(ctx, options); + const ctxWithSpanContext = ensureScopesOnContext(getContextWithRemoteActiveSpan(ctx, options)); return context.with(ctxWithSpanContext, callback); } +function ensureScopesOnContext(ctx: Context): Context { + // If there are no scopes yet on the context, ensure we have them + const scopes = getScopesFromContext(ctx); + const newScopes = { + // If we have no scope here, this is most likely either the root context or a context manually derived from it + // In this case, we want to fork the current scope, to ensure we do not pollute the root scope + scope: scopes ? scopes.scope : getCurrentScope().clone(), + isolationScope: scopes ? scopes.isolationScope : getIsolationScope(), + }; + + return setScopesOnContext(ctx, newScopes); +} + /** Try to get the existing baggage header so we can merge this in. */ function getExistingBaggage(carrier: unknown): string | undefined { try { @@ -297,20 +329,28 @@ function getCurrentURL(span: Span): string | undefined { return undefined; } -// TODO: Adjust this behavior to avoid invalid spans -function generateSpanContextForPropagationContext(propagationContext: PropagationContext): SpanContext { +function generateRemoteSpanContext({ + spanId, + traceId, + sampled, + dsc, +}: { + spanId: string; + traceId: string; + sampled: boolean | undefined; + dsc?: Partial; +}): SpanContext { // We store the DSC as OTEL trace state on the span context const traceState = makeTraceState({ - parentSpanId: propagationContext.parentSpanId, - dsc: propagationContext.dsc, - sampled: propagationContext.sampled, + dsc, + sampled, }); const spanContext: SpanContext = { - traceId: propagationContext.traceId, - spanId: propagationContext.parentSpanId || '', + traceId, + spanId, isRemote: true, - traceFlags: propagationContext.sampled ? TraceFlags.SAMPLED : TraceFlags.NONE, + traceFlags: sampled ? TraceFlags.SAMPLED : TraceFlags.NONE, traceState, }; diff --git a/packages/opentelemetry/src/setupEventContextTrace.ts b/packages/opentelemetry/src/setupEventContextTrace.ts index 43d4344a8ce7..1bf9bcb961be 100644 --- a/packages/opentelemetry/src/setupEventContextTrace.ts +++ b/packages/opentelemetry/src/setupEventContextTrace.ts @@ -1,8 +1,6 @@ import type { Client } from '@sentry/core'; -import { dropUndefinedKeys, getDynamicSamplingContextFromSpan, getRootSpan } from '@sentry/core'; -import { SENTRY_TRACE_STATE_PARENT_SPAN_ID } from './constants'; +import { getDynamicSamplingContextFromSpan, getRootSpan, spanToTraceContext } from '@sentry/core'; import { getActiveSpan } from './utils/getActiveSpan'; -import { spanHasParentId } from './utils/spanTypes'; /** Ensure the `trace` context is set on all events. */ export function setupEventContextTrace(client: Client): void { @@ -14,25 +12,9 @@ export function setupEventContextTrace(client: Client): void { return; } - const spanContext = span.spanContext(); - - // If we have a parent span id from trace state, use that ('' means no parent should be used) - // Else, pick the one from the span - const parentSpanIdFromTraceState = spanContext.traceState?.get(SENTRY_TRACE_STATE_PARENT_SPAN_ID); - const parent_span_id = - typeof parentSpanIdFromTraceState === 'string' - ? parentSpanIdFromTraceState || undefined - : spanHasParentId(span) - ? span.parentSpanId - : undefined; - // 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, - }), + trace: spanToTraceContext(span), ...event.contexts, }; diff --git a/packages/opentelemetry/src/spanExporter.ts b/packages/opentelemetry/src/spanExporter.ts index 126efed460b6..ad044372e0df 100644 --- a/packages/opentelemetry/src/spanExporter.ts +++ b/packages/opentelemetry/src/spanExporter.ts @@ -18,7 +18,6 @@ import { spanTimeInputToSeconds, timedEventsToMeasurements, } from '@sentry/core'; -import { SENTRY_TRACE_STATE_PARENT_SPAN_ID } from './constants'; import { DEBUG_BUILD } from './debug-build'; import { SEMANTIC_ATTRIBUTE_SENTRY_PARENT_IS_REMOTE } from './semanticAttributes'; import { getRequestSpanData } from './utils/getRequestSpanData'; @@ -239,15 +238,12 @@ function createTransactionForOtelSpan(span: ReadableSpan): TransactionEvent { const { traceId: trace_id, spanId: span_id } = span.spanContext(); - const parentSpanIdFromTraceState = span.spanContext().traceState?.get(SENTRY_TRACE_STATE_PARENT_SPAN_ID); - // If parentSpanIdFromTraceState is defined at all, we want it to take precedence // In that case, an empty string should be interpreted as "no parent span id", // even if `span.parentSpanId` is set // this is the case when we are starting a new trace, where we have a virtual span based on the propagationContext // We only want to continue the traceId in this case, but ignore the parent span - const parent_span_id = - typeof parentSpanIdFromTraceState === 'string' ? parentSpanIdFromTraceState || undefined : span.parentSpanId; + const parent_span_id = span.parentSpanId; const status = mapStatus(span); diff --git a/packages/opentelemetry/src/trace.ts b/packages/opentelemetry/src/trace.ts index 1c1710b94793..e1f082c1d424 100644 --- a/packages/opentelemetry/src/trace.ts +++ b/packages/opentelemetry/src/trace.ts @@ -1,5 +1,5 @@ import type { Context, Span, SpanContext, SpanOptions, Tracer } from '@opentelemetry/api'; -import { INVALID_SPANID, SpanStatusCode, TraceFlags, context, trace } from '@opentelemetry/api'; +import { SpanStatusCode, TraceFlags, context, trace } from '@opentelemetry/api'; import { suppressTracing } from '@opentelemetry/core'; import type { Client, DynamicSamplingContext, Scope, Span as SentrySpan, TraceContext } from '@sentry/core'; import { @@ -18,7 +18,7 @@ import { } from '@sentry/core'; import { continueTraceAsRemoteSpan } from './propagator'; import type { OpenTelemetryClient, OpenTelemetrySpanContext } from './types'; -import { getContextFromScope, getScopesFromContext } from './utils/contextData'; +import { getContextFromScope } from './utils/contextData'; import { getSamplingDecision } from './utils/getSamplingDecision'; import { makeTraceState } from './utils/makeTraceState'; @@ -178,40 +178,11 @@ function ensureTimestampInMilliseconds(timestamp: number): number { function getContext(scope: Scope | undefined, forceTransaction: boolean | undefined): Context { const ctx = getContextForScope(scope); - // Note: If the context is the ROOT_CONTEXT, no scope is attached - // Thus we will not use the propagation context in this case, which is desired - const actualScope = getScopesFromContext(ctx)?.scope; const parentSpan = trace.getSpan(ctx); - // In the case that we have no parent span, we need to "simulate" one to ensure the propagation context is correct + // In the case that we have no parent span, we start a new trace + // Note that if we continue a trace, we'll always have a remote parent span here anyhow if (!parentSpan) { - const client = getClient(); - - if (actualScope && client) { - const propagationContext = actualScope.getPropagationContext(); - - // We store the DSC as OTEL trace state on the span context - const traceState = makeTraceState({ - parentSpanId: propagationContext.parentSpanId, - // Not defined yet, we want to pick this up on-demand only - dsc: undefined, - sampled: propagationContext.sampled, - }); - - const spanOptions: SpanContext = { - traceId: propagationContext.traceId, - // eslint-disable-next-line deprecation/deprecation - spanId: propagationContext.parentSpanId || propagationContext.spanId, - isRemote: true, - traceFlags: propagationContext.sampled ? TraceFlags.SAMPLED : TraceFlags.NONE, - traceState, - }; - - // Add remote parent span context, - return trace.setSpanContext(ctx, spanOptions); - } - - // if we have no scope or client, we just return the context as-is return ctx; } @@ -237,7 +208,6 @@ function getContext(scope: Scope | undefined, forceTransaction: boolean | undefi const traceState = makeTraceState({ dsc, - parentSpanId: spanId !== INVALID_SPANID ? spanId : undefined, sampled, }); diff --git a/packages/opentelemetry/src/utils/enhanceDscWithOpenTelemetryRootSpanName.ts b/packages/opentelemetry/src/utils/enhanceDscWithOpenTelemetryRootSpanName.ts index 307f25364c5e..4aa9ecfdb8b6 100644 --- a/packages/opentelemetry/src/utils/enhanceDscWithOpenTelemetryRootSpanName.ts +++ b/packages/opentelemetry/src/utils/enhanceDscWithOpenTelemetryRootSpanName.ts @@ -1,5 +1,6 @@ -import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, spanToJSON } from '@sentry/core'; +import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, hasTracingEnabled, spanToJSON } from '@sentry/core'; import type { Client } from '@sentry/core'; +import { getSamplingDecision } from './getSamplingDecision'; import { parseSpanDescription } from './parseSpanDescription'; import { spanHasName } from './spanTypes'; @@ -9,20 +10,31 @@ import { spanHasName } from './spanTypes'; */ export function enhanceDscWithOpenTelemetryRootSpanName(client: Client): void { client.on('createDsc', (dsc, rootSpan) => { + if (!rootSpan) { + return; + } + // We want to overwrite the transaction on the DSC that is created by default in core // The reason for this is that we want to infer the span name, not use the initial one // Otherwise, we'll get names like "GET" instead of e.g. "GET /foo" // `parseSpanDescription` takes the attributes of the span into account for the name // This mutates the passed-in DSC - if (rootSpan) { - const jsonSpan = spanToJSON(rootSpan); - const attributes = jsonSpan.data || {}; - const source = attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]; - const { description } = spanHasName(rootSpan) ? parseSpanDescription(rootSpan) : { description: undefined }; - if (source !== 'url' && description) { - dsc.transaction = description; - } + const jsonSpan = spanToJSON(rootSpan); + const attributes = jsonSpan.data || {}; + const source = attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]; + + const { description } = spanHasName(rootSpan) ? parseSpanDescription(rootSpan) : { description: undefined }; + if (source !== 'url' && description) { + dsc.transaction = description; + } + + // Also ensure sampling decision is correctly inferred + // In core, we use `spanIsSampled`, which just looks at the trace flags + // but in OTEL, we use a slightly more complex logic to be able to differntiate between unsampled and deferred sampling + if (hasTracingEnabled()) { + const sampled = getSamplingDecision(rootSpan.spanContext()); + dsc.sampled = sampled == undefined ? undefined : String(sampled); } }); } diff --git a/packages/opentelemetry/src/utils/generateSpanContextForPropagationContext.ts b/packages/opentelemetry/src/utils/generateSpanContextForPropagationContext.ts index 8f5f0b41b2b9..bf0af49b1200 100644 --- a/packages/opentelemetry/src/utils/generateSpanContextForPropagationContext.ts +++ b/packages/opentelemetry/src/utils/generateSpanContextForPropagationContext.ts @@ -12,7 +12,6 @@ import { makeTraceState } from './makeTraceState'; export function generateSpanContextForPropagationContext(propagationContext: PropagationContext): SpanContext { // We store the DSC as OTEL trace state on the span context const traceState = makeTraceState({ - parentSpanId: propagationContext.parentSpanId, dsc: propagationContext.dsc, sampled: propagationContext.sampled, }); diff --git a/packages/opentelemetry/src/utils/makeTraceState.ts b/packages/opentelemetry/src/utils/makeTraceState.ts index 6175ffd982c1..c232c981bb41 100644 --- a/packages/opentelemetry/src/utils/makeTraceState.ts +++ b/packages/opentelemetry/src/utils/makeTraceState.ts @@ -1,32 +1,22 @@ import { TraceState } from '@opentelemetry/core'; import { dynamicSamplingContextToSentryBaggageHeader } from '@sentry/core'; import type { DynamicSamplingContext } from '@sentry/core'; -import { - SENTRY_TRACE_STATE_DSC, - SENTRY_TRACE_STATE_PARENT_SPAN_ID, - SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING, -} from '../constants'; +import { SENTRY_TRACE_STATE_DSC, SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING } from '../constants'; /** * Generate a TraceState for the given data. */ export function makeTraceState({ - parentSpanId, dsc, sampled, }: { - parentSpanId?: string; dsc?: Partial; sampled?: boolean; }): TraceState { // We store the DSC as OTEL trace state on the span context const dscString = dsc ? dynamicSamplingContextToSentryBaggageHeader(dsc) : undefined; - // We _always_ set the parent span ID, even if it is empty - // If we'd set this to 'undefined' we could not know if the trace state was set, but there was no parentSpanId, - // vs the trace state was not set at all (in which case we want to do fallback handling) - // If `''`, it should be considered "no parent" - const traceStateBase = new TraceState().set(SENTRY_TRACE_STATE_PARENT_SPAN_ID, parentSpanId || ''); + const traceStateBase = new TraceState(); const traceStateWithDsc = dscString ? traceStateBase.set(SENTRY_TRACE_STATE_DSC, dscString) : traceStateBase; diff --git a/packages/opentelemetry/test/helpers/initOtel.ts b/packages/opentelemetry/test/helpers/initOtel.ts index 7e59400cc1b1..53bd039762ad 100644 --- a/packages/opentelemetry/test/helpers/initOtel.ts +++ b/packages/opentelemetry/test/helpers/initOtel.ts @@ -16,6 +16,7 @@ import { SentryPropagator } from '../../src/propagator'; import { SentrySampler } from '../../src/sampler'; import { setupEventContextTrace } from '../../src/setupEventContextTrace'; import { SentrySpanProcessor } from '../../src/spanProcessor'; +import { enhanceDscWithOpenTelemetryRootSpanName } from '../../src/utils/enhanceDscWithOpenTelemetryRootSpanName'; import type { TestClientInterface } from './TestClient'; /** @@ -44,6 +45,7 @@ export function initOtel(): void { } setupEventContextTrace(client); + enhanceDscWithOpenTelemetryRootSpanName(client); const provider = setupOtel(client); client.traceProvider = provider; diff --git a/packages/opentelemetry/test/integration/transactions.test.ts b/packages/opentelemetry/test/integration/transactions.test.ts index 49d4a30b6780..bc0179b55e38 100644 --- a/packages/opentelemetry/test/integration/transactions.test.ts +++ b/packages/opentelemetry/test/integration/transactions.test.ts @@ -327,7 +327,6 @@ describe('Integration | Transactions', () => { const parentSpanId = '6e0c63257de34c92'; const traceState = makeTraceState({ - parentSpanId, dsc: undefined, sampled: true, }); diff --git a/packages/opentelemetry/test/propagator.test.ts b/packages/opentelemetry/test/propagator.test.ts index d3b9483674a1..56aed551353e 100644 --- a/packages/opentelemetry/test/propagator.test.ts +++ b/packages/opentelemetry/test/propagator.test.ts @@ -12,7 +12,6 @@ import { getCurrentScope, withScope } from '@sentry/core'; import { SENTRY_BAGGAGE_HEADER, SENTRY_SCOPES_CONTEXT_KEY, SENTRY_TRACE_HEADER } from '../src/constants'; import { SentryPropagator } from '../src/propagator'; -import { getScopesFromContext } from '../src/utils/contextData'; import { getSamplingDecision } from '../src/utils/getSamplingDecision'; import { makeTraceState } from '../src/utils/makeTraceState'; import { cleanupOtel, mockSdkInit } from './helpers/mockSdkInit'; @@ -101,39 +100,6 @@ describe('SentryPropagator', () => { }); }); - it('uses scope propagation context over remote spanContext', () => { - context.with( - trace.setSpanContext(ROOT_CONTEXT, { - traceId: 'd4cda95b652f4a1592b449d5929fda1b', - spanId: '6e0c63257de34c92', - traceFlags: TraceFlags.NONE, - isRemote: true, - }), - () => { - withScope(scope => { - scope.setPropagationContext({ - traceId: 'TRACE_ID', - parentSpanId: 'PARENT_SPAN_ID', - spanId: 'SPAN_ID', - sampled: true, - }); - - propagator.inject(context.active(), carrier, defaultTextMapSetter); - - expect(baggageToArray(carrier[SENTRY_BAGGAGE_HEADER])).toEqual( - [ - 'sentry-environment=production', - 'sentry-release=1.0.0', - 'sentry-public_key=abc', - 'sentry-trace_id=TRACE_ID', - ].sort(), - ); - expect(carrier[SENTRY_TRACE_HEADER]).toBe('TRACE_ID-SPAN_ID-1'); - }); - }, - ); - }); - it('uses propagation data from current scope if no scope & span is found', () => { const scope = getCurrentScope(); const traceId = scope.getPropagationContext().traceId; @@ -181,7 +147,6 @@ describe('SentryPropagator', () => { traceFlags: TraceFlags.SAMPLED, isRemote: true, traceState: makeTraceState({ - parentSpanId: '6e0c63257de34c92', dsc: { transaction: 'sampled-transaction', sampled: 'true', @@ -256,7 +221,6 @@ describe('SentryPropagator', () => { traceFlags: TraceFlags.NONE, isRemote: true, traceState: makeTraceState({ - parentSpanId: '6e0c63257de34c92', dsc: { transaction: 'sampled-transaction', sampled: 'false', @@ -291,7 +255,6 @@ describe('SentryPropagator', () => { isRemote: true, traceState: makeTraceState({ sampled: false, - parentSpanId: '6e0c63257de34c92', dsc: { transaction: 'sampled-transaction', trace_id: 'dsc_trace_id', @@ -383,14 +346,53 @@ describe('SentryPropagator', () => { }); }, ); + }); + + it('uses remote span with deferred sampling decision over propagation context', () => { + const carrier: Record = {}; + context.with( + trace.setSpanContext(ROOT_CONTEXT, { + traceId: 'd4cda95b652f4a1592b449d5929fda1b', + spanId: '6e0c63257de34c92', + traceFlags: TraceFlags.NONE, + isRemote: true, + }), + () => { + withScope(scope => { + scope.setPropagationContext({ + traceId: 'TRACE_ID', + parentSpanId: 'PARENT_SPAN_ID', + spanId: 'SPAN_ID', + sampled: true, + }); + + propagator.inject(context.active(), carrier, defaultTextMapSetter); + + expect(baggageToArray(carrier[SENTRY_BAGGAGE_HEADER])).toEqual( + [ + 'sentry-environment=production', + 'sentry-release=1.0.0', + 'sentry-public_key=abc', + 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', + ].sort(), + ); + // Used spanId is a random ID, not from the remote span + expect(carrier[SENTRY_TRACE_HEADER]).toMatch(/d4cda95b652f4a1592b449d5929fda1b-[a-f0-9]{16}/); + expect(carrier[SENTRY_TRACE_HEADER]).not.toBe('d4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92'); + }); + }, + ); + }); - const carrier2: Record = {}; + it('uses remote span over propagation context', () => { + const carrier: Record = {}; context.with( trace.setSpanContext(ROOT_CONTEXT, { traceId: 'd4cda95b652f4a1592b449d5929fda1b', spanId: '6e0c63257de34c92', traceFlags: TraceFlags.NONE, isRemote: true, + traceState: makeTraceState({ sampled: false }), }), () => { withScope(scope => { @@ -401,17 +403,20 @@ describe('SentryPropagator', () => { sampled: true, }); - propagator.inject(context.active(), carrier2, defaultTextMapSetter); + propagator.inject(context.active(), carrier, defaultTextMapSetter); - expect(baggageToArray(carrier2[SENTRY_BAGGAGE_HEADER])).toEqual( + expect(baggageToArray(carrier[SENTRY_BAGGAGE_HEADER])).toEqual( [ 'sentry-environment=production', 'sentry-release=1.0.0', 'sentry-public_key=abc', - 'sentry-trace_id=TRACE_ID', + 'sentry-sampled=false', + 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', ].sort(), ); - expect(carrier2[SENTRY_TRACE_HEADER]).toBe('TRACE_ID-SPAN_ID-1'); + // Used spanId is a random ID, not from the remote span + expect(carrier[SENTRY_TRACE_HEADER]).toMatch(/d4cda95b652f4a1592b449d5929fda1b-[a-f0-9]{16}-0/); + expect(carrier[SENTRY_TRACE_HEADER]).not.toBe('d4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-0'); }); }, ); @@ -557,7 +562,7 @@ describe('SentryPropagator', () => { spanId: '6e0c63257de34c92', traceFlags: TraceFlags.SAMPLED, traceId: 'd4cda95b652f4a1592b449d5929fda1b', - traceState: makeTraceState({ parentSpanId: '6e0c63257de34c92' }), + traceState: makeTraceState({}), }); expect(getSamplingDecision(trace.getSpanContext(context)!)).toBe(true); }); @@ -571,7 +576,7 @@ describe('SentryPropagator', () => { spanId: '6e0c63257de34c92', traceFlags: TraceFlags.NONE, traceId: 'd4cda95b652f4a1592b449d5929fda1b', - traceState: makeTraceState({ parentSpanId: '6e0c63257de34c92', sampled: false }), + traceState: makeTraceState({ sampled: false }), }); expect(getSamplingDecision(trace.getSpanContext(context)!)).toBe(false); }); @@ -585,41 +590,20 @@ describe('SentryPropagator', () => { spanId: '6e0c63257de34c92', traceFlags: TraceFlags.NONE, traceId: 'd4cda95b652f4a1592b449d5929fda1b', - traceState: makeTraceState({ parentSpanId: '6e0c63257de34c92' }), + traceState: makeTraceState({}), }); expect(getSamplingDecision(trace.getSpanContext(context)!)).toBe(undefined); }); - it('sets data from sentry trace header on scope', () => { - const sentryTraceHeader = 'd4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-1'; - carrier[SENTRY_TRACE_HEADER] = sentryTraceHeader; - const context = propagator.extract(ROOT_CONTEXT, carrier, defaultTextMapGetter); - - const scopes = getScopesFromContext(context); - - expect(scopes).toBeDefined(); - expect(scopes?.scope.getPropagationContext()).toEqual({ - spanId: expect.any(String), - sampled: true, - traceId: 'd4cda95b652f4a1592b449d5929fda1b', - parentSpanId: '6e0c63257de34c92', - dsc: {}, - }); - expect(getSamplingDecision(trace.getSpanContext(context)!)).toBe(true); - }); - it('handles undefined sentry trace header', () => { const sentryTraceHeader = undefined; carrier[SENTRY_TRACE_HEADER] = sentryTraceHeader; const context = propagator.extract(ROOT_CONTEXT, carrier, defaultTextMapGetter); - expect(trace.getSpanContext(context)).toEqual({ - isRemote: true, - spanId: expect.any(String), - traceFlags: TraceFlags.NONE, - traceId: expect.any(String), - traceState: makeTraceState({}), + expect(trace.getSpanContext(context)).toEqual(undefined); + expect(getCurrentScope().getPropagationContext()).toEqual({ + spanId: expect.stringMatching(/[a-f0-9]{16}/), + traceId: expect.stringMatching(/[a-f0-9]{32}/), }); - expect(getSamplingDecision(trace.getSpanContext(context)!)).toBe(undefined); }); it('sets data from baggage header on span context', () => { @@ -635,7 +619,6 @@ describe('SentryPropagator', () => { traceFlags: TraceFlags.SAMPLED, traceId: 'd4cda95b652f4a1592b449d5929fda1b', traceState: makeTraceState({ - parentSpanId: '6e0c63257de34c92', dsc: { environment: 'production', release: '1.0.0', @@ -648,55 +631,29 @@ describe('SentryPropagator', () => { expect(getSamplingDecision(trace.getSpanContext(context)!)).toBe(true); }); - it('sets data from baggage header on scope', () => { - const sentryTraceHeader = 'd4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-1'; - const baggage = - 'sentry-environment=production,sentry-release=1.0.0,sentry-public_key=abc,sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b,sentry-transaction=dsc-transaction'; - carrier[SENTRY_TRACE_HEADER] = sentryTraceHeader; - carrier[SENTRY_BAGGAGE_HEADER] = baggage; - const context = propagator.extract(ROOT_CONTEXT, carrier, defaultTextMapGetter); - - const scopes = getScopesFromContext(context); - - expect(scopes).toBeDefined(); - expect(scopes?.scope.getPropagationContext()).toEqual({ - spanId: expect.any(String), - sampled: true, - traceId: 'd4cda95b652f4a1592b449d5929fda1b', - parentSpanId: '6e0c63257de34c92', - dsc: { - environment: 'production', - release: '1.0.0', - public_key: 'abc', - trace_id: 'd4cda95b652f4a1592b449d5929fda1b', - transaction: 'dsc-transaction', - }, - }); - }); - it('handles empty dsc baggage header', () => { + const sentryTraceHeader = 'd4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-1'; const baggage = ''; + carrier[SENTRY_TRACE_HEADER] = sentryTraceHeader; carrier[SENTRY_BAGGAGE_HEADER] = baggage; const context = propagator.extract(ROOT_CONTEXT, carrier, defaultTextMapGetter); expect(trace.getSpanContext(context)).toEqual({ isRemote: true, - spanId: expect.any(String), - traceFlags: TraceFlags.NONE, - traceId: expect.any(String), + spanId: '6e0c63257de34c92', + traceFlags: TraceFlags.SAMPLED, + traceId: 'd4cda95b652f4a1592b449d5929fda1b', traceState: makeTraceState({}), }); - expect(getSamplingDecision(trace.getSpanContext(context)!)).toBe(undefined); + expect(getSamplingDecision(trace.getSpanContext(context)!)).toBe(true); }); it('handles when sentry-trace is an empty array', () => { carrier[SENTRY_TRACE_HEADER] = []; const context = propagator.extract(ROOT_CONTEXT, carrier, defaultTextMapGetter); - expect(trace.getSpanContext(context)).toEqual({ - isRemote: true, - spanId: expect.any(String), - traceFlags: TraceFlags.NONE, - traceId: expect.any(String), - traceState: makeTraceState({}), + expect(trace.getSpanContext(context)).toEqual(undefined); + expect(getCurrentScope().getPropagationContext()).toEqual({ + spanId: expect.stringMatching(/[a-f0-9]{16}/), + traceId: expect.stringMatching(/[a-f0-9]{32}/), }); }); }); diff --git a/packages/opentelemetry/test/trace.test.ts b/packages/opentelemetry/test/trace.test.ts index 95edc98dfd81..2c22318ec977 100644 --- a/packages/opentelemetry/test/trace.test.ts +++ b/packages/opentelemetry/test/trace.test.ts @@ -329,9 +329,7 @@ describe('trace', () => { it('allows to pass parentSpan=null', () => { startSpan({ name: 'GET users/[id' }, () => { startSpan({ name: 'child', parentSpan: null }, span => { - // Due to the way we propagate the scope in OTEL, - // the parent_span_id is not actually undefined here, but comes from the propagation context - expect(spanToJSON(span).parent_span_id).toBe(getCurrentScope().getPropagationContext().spanId); + expect(spanToJSON(span).parent_span_id).toBe(undefined); }); }); }); @@ -591,10 +589,7 @@ describe('trace', () => { it('allows to pass parentSpan=null', () => { startSpan({ name: 'outer' }, () => { const span = startInactiveSpan({ name: 'test span', parentSpan: null }); - - // Due to the way we propagate the scope in OTEL, - // the parent_span_id is not actually undefined here, but comes from the propagation context - expect(spanToJSON(span).parent_span_id).toBe(getCurrentScope().getPropagationContext().spanId); + expect(spanToJSON(span).parent_span_id).toBe(undefined); span.end(); }); }); @@ -881,9 +876,7 @@ describe('trace', () => { it('allows to pass parentSpan=null', () => { startSpan({ name: 'outer' }, () => { startSpanManual({ name: 'GET users/[id]', parentSpan: null }, span => { - // Due to the way we propagate the scope in OTEL, - // the parent_span_id is not actually undefined here, but comes from the propagation context - expect(spanToJSON(span).parent_span_id).toBe(getCurrentScope().getPropagationContext().spanId); + expect(spanToJSON(span).parent_span_id).toBe(undefined); span.end(); }); }); @@ -1016,17 +1009,21 @@ describe('trace', () => { }); describe('propagation', () => { - it('picks up the trace context from the scope, if there is no parent', () => { + it('starts new trace, if there is no parent', () => { withScope(scope => { const propagationContext = scope.getPropagationContext(); const span = startInactiveSpan({ name: 'test span' }); expect(span).toBeDefined(); - expect(spanToJSON(span).trace_id).toEqual(propagationContext.traceId); - expect(spanToJSON(span).parent_span_id).toEqual(propagationContext.spanId); + const traceId = spanToJSON(span).trace_id; + expect(traceId).toMatch(/[a-f0-9]{32}/); + expect(spanToJSON(span).parent_span_id).toBe(undefined); + expect(spanToJSON(span).trace_id).not.toEqual(propagationContext.traceId); expect(getDynamicSamplingContextFromSpan(span)).toEqual({ - ...getDynamicSamplingContextFromClient(propagationContext.traceId, getClient()!), + trace_id: traceId, + environment: 'production', + public_key: 'username', sample_rate: '1', sampled: 'true', transaction: 'test span', @@ -1034,18 +1031,23 @@ describe('trace', () => { }); }); - it('picks up the trace context from the scope, including parentSpanId, if there is no parent', () => { + // Note: This _should_ never happen, when we have an incoming trace, we should always have a parent span + it('starts new trace, ignoring parentSpanId, if there is no parent', () => { withScope(scope => { const propagationContext = scope.getPropagationContext(); propagationContext.parentSpanId = '1121201211212012'; const span = startInactiveSpan({ name: 'test span' }); expect(span).toBeDefined(); - expect(spanToJSON(span).trace_id).toEqual(propagationContext.traceId); - expect(spanToJSON(span).parent_span_id).toEqual('1121201211212012'); + const traceId = spanToJSON(span).trace_id; + expect(traceId).toMatch(/[a-f0-9]{32}/); + expect(spanToJSON(span).parent_span_id).toBe(undefined); + expect(spanToJSON(span).trace_id).not.toEqual(propagationContext.traceId); expect(getDynamicSamplingContextFromSpan(span)).toEqual({ - ...getDynamicSamplingContextFromClient(propagationContext.traceId, getClient()!), + environment: 'production', + public_key: 'username', + trace_id: traceId, sample_rate: '1', sampled: 'true', transaction: 'test span', @@ -1082,7 +1084,6 @@ describe('trace', () => { isRemote: false, traceFlags: TraceFlags.SAMPLED, traceState: makeTraceState({ - parentSpanId: '1121201211212011', dsc: { release: '1.0', environment: 'production', @@ -1112,7 +1113,6 @@ describe('trace', () => { isRemote: true, traceFlags: TraceFlags.SAMPLED, traceState: makeTraceState({ - parentSpanId: '1121201211212011', dsc: { release: '1.0', environment: 'production', @@ -1540,14 +1540,7 @@ describe('continueTrace', () => { it('works without trace & baggage data', () => { const scope = continueTrace({ sentryTrace: undefined, baggage: undefined }, () => { const span = getActiveSpan()!; - expect(span).toBeDefined(); - expect(spanToJSON(span)).toEqual({ - span_id: '', - trace_id: expect.stringMatching(/[a-f0-9]{32}/), - }); - expect(getSamplingDecision(span.spanContext())).toBe(undefined); - expect(spanIsSampled(span)).toBe(false); - + expect(span).toBeUndefined(); return getCurrentScope(); });