From 303a7a0f0c41888be7a57728348b6c1f20ac054a Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 25 Nov 2024 09:08:01 +0100 Subject: [PATCH] feat(opentelemetry): Ignore propagation context when not continuing trace --- .../startSpan/parallel-root-spans/scenario.ts | 16 +------- .../startSpan/parallel-root-spans/test.ts | 37 +++++++++++-------- .../startSpan/parallel-spans-in-scope/test.ts | 3 +- packages/opentelemetry/src/trace.ts | 21 ++++++----- packages/opentelemetry/test/trace.test.ts | 24 +++++------- 5 files changed, 45 insertions(+), 56 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-root-spans/scenario.ts b/dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-root-spans/scenario.ts index e352fff5c02c..c5e442f03e0a 100644 --- a/dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-root-spans/scenario.ts +++ b/dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-root-spans/scenario.ts @@ -14,17 +14,5 @@ Sentry.getCurrentScope().setPropagationContext({ traceId: '12345678901234567890123456789012', }); -const spanIdTraceId = Sentry.startSpan( - { - name: 'test_span_1', - }, - span1 => span1.spanContext().traceId, -); - -Sentry.startSpan( - { - name: 'test_span_2', - attributes: { spanIdTraceId }, - }, - () => undefined, -); +Sentry.startSpan({ name: 'test_span_1' }, () => undefined); +Sentry.startSpan({ name: 'test_span_2' }, () => undefined); diff --git a/dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-root-spans/test.ts b/dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-root-spans/test.ts index ed7726d72389..41d281e1dc38 100644 --- a/dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-root-spans/test.ts +++ b/dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-root-spans/test.ts @@ -5,24 +5,29 @@ afterAll(() => { }); test('should send manually started parallel root spans in root context', done => { - expect.assertions(7); - createRunner(__dirname, 'scenario.ts') - .expect({ transaction: { transaction: 'test_span_1' } }) .expect({ - transaction: transaction => { - expect(transaction).toBeDefined(); - const traceId = transaction.contexts?.trace?.trace_id; - expect(traceId).toBeDefined(); - - // It ignores propagation context of the root context - expect(traceId).not.toBe('12345678901234567890123456789012'); - expect(transaction.contexts?.trace?.parent_span_id).toBeUndefined(); - - // Different trace ID than the first span - const trace1Id = transaction.contexts?.trace?.data?.spanIdTraceId; - expect(trace1Id).toBeDefined(); - expect(trace1Id).not.toBe(traceId); + transaction: { + transaction: 'test_span_1', + contexts: { + trace: { + span_id: expect.any(String), + parent_span_id: '1234567890123456', + trace_id: '12345678901234567890123456789012', + }, + }, + }, + }) + .expect({ + transaction: { + transaction: 'test_span_2', + contexts: { + trace: { + span_id: expect.any(String), + parent_span_id: '1234567890123456', + trace_id: '12345678901234567890123456789012', + }, + }, }, }) .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..9068a73a91fa 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,7 @@ 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); + expect(trace1Id).not.toBe(traceId); }, }) .start(done); diff --git a/packages/opentelemetry/src/trace.ts b/packages/opentelemetry/src/trace.ts index 1ed0fa6a1322..781013b55d12 100644 --- a/packages/opentelemetry/src/trace.ts +++ b/packages/opentelemetry/src/trace.ts @@ -176,18 +176,18 @@ 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 if (!parentSpan) { - const client = getClient(); - - if (actualScope && client) { - const propagationContext = actualScope.getPropagationContext(); - + // If we cannot pick a scope from the context (e.g. if it is the root context), we just take the current scope + const actualScope = getScopesFromContext(ctx)?.scope || getCurrentScope(); + const propagationContext = actualScope.getPropagationContext(); + + // If we are continuing an incoming trace, we use it + // This is signified by the presence of `parentSpanId` - + // if this is not set, then we are not continuing an incoming trace + if (propagationContext.parentSpanId) { // We store the DSC as OTEL trace state on the span context const traceState = makeTraceState({ parentSpanId: propagationContext.parentSpanId, @@ -198,7 +198,7 @@ function getContext(scope: Scope | undefined, forceTransaction: boolean | undefi const spanOptions: SpanContext = { traceId: propagationContext.traceId, - spanId: propagationContext.parentSpanId || propagationContext.spanId, + spanId: propagationContext.parentSpanId, isRemote: true, traceFlags: propagationContext.sampled ? TraceFlags.SAMPLED : TraceFlags.NONE, traceState, @@ -208,7 +208,8 @@ function getContext(scope: Scope | undefined, forceTransaction: boolean | undefi return trace.setSpanContext(ctx, spanOptions); } - // if we have no scope or client, we just return the context as-is + // if we have no scope, or the propagationContext is not continuing an incoming trace, + // we just let OTEL start a new trace return ctx; } diff --git a/packages/opentelemetry/test/trace.test.ts b/packages/opentelemetry/test/trace.test.ts index 0a709c9fb5da..f7ef6a08ff5a 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,20 +1009,23 @@ describe('trace', () => { }); describe('propagation', () => { - it('picks up the trace context from the scope, if there is no parent', () => { + it('ignores the trace context from the scope, if there is no parent & no propagationContext.parentSpanId', () => { 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); + expect(spanToJSON(span).trace_id).toMatch(/^[0-9a-f]{32}$/); + expect(spanToJSON(span).trace_id).not.toEqual(propagationContext.traceId); + expect(spanToJSON(span).parent_span_id).toEqual(undefined); expect(getDynamicSamplingContextFromSpan(span)).toEqual({ - ...getDynamicSamplingContextFromClient(propagationContext.traceId, getClient()!), + environment: 'production', + public_key: 'username', sample_rate: '1', sampled: 'true', transaction: 'test span', + trace_id: spanToJSON(span).trace_id, }); }); });