Skip to content

Commit c8e81d5

Browse files
mydeamydea
authored
feat(opentelemetry): Stop looking at propagation context for span creation (#14481)
This PR changes the behavior of the OTEL-based Node SDK to ignore the propagation context when starting spans. Previously, when you called `startSpan` and there was no incoming trace, we would ensure that the new span has the trace ID + span ID from the propagation context. This has a few problems: 1. Multiple parallel root spans will continue the same virtual trace, instead of having separate traces. 2. This is really invalid in OTEL, as we have to provide a span ID and cannot really tell it to use a specific trace ID out of the box. Because of this, we had to add a bunch of special handling to ensure we can differentiate real and fake parent span IDs properly. This PR fixes this by simply not looking at this anymore. For TWP and error marking the propagation context is still used as before, only for new spans is there a difference. I also added docs explaining how trace propagation in node works now: ![node-sdk-trace-propagation-3](https://github.com/user-attachments/assets/73cc5972-2d2a-438c-9c32-2551fc52568e)
1 parent 97abe0a commit c8e81d5

File tree

37 files changed

+433
-329
lines changed

37 files changed

+433
-329
lines changed

Diff for: ‎dev-packages/e2e-tests/test-applications/nestjs-8/tests/errors.test.ts

+1
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ test('Sends exception to Sentry', async ({ baseURL }) => {
2626
expect(errorEvent.contexts?.trace).toEqual({
2727
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
2828
span_id: expect.stringMatching(/[a-f0-9]{16}/),
29+
parent_span_id: expect.stringMatching(/[a-f0-9]{16}/),
2930
});
3031
});
3132

Diff for: ‎dev-packages/e2e-tests/test-applications/nestjs-basic-with-graphql/tests/errors.test.ts

+2
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ test('Sends exception to Sentry', async ({ baseURL }) => {
2626
expect(errorEvent.contexts?.trace).toEqual({
2727
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
2828
span_id: expect.stringMatching(/[a-f0-9]{16}/),
29+
parent_span_id: expect.stringMatching(/[a-f0-9]{16}/),
2930
});
3031
});
3132

@@ -114,5 +115,6 @@ test('Sends graphql exception to Sentry', async ({ baseURL }) => {
114115
expect(errorEvent.contexts?.trace).toEqual({
115116
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
116117
span_id: expect.stringMatching(/[a-f0-9]{16}/),
118+
parent_span_id: expect.stringMatching(/[a-f0-9]{16}/),
117119
});
118120
});

Diff for: ‎dev-packages/e2e-tests/test-applications/nestjs-basic/tests/errors.test.ts

+1
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ test('Sends exception to Sentry', async ({ baseURL }) => {
2626
expect(errorEvent.contexts?.trace).toEqual({
2727
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
2828
span_id: expect.stringMatching(/[a-f0-9]{16}/),
29+
parent_span_id: expect.stringMatching(/[a-f0-9]{16}/),
2930
});
3031
});
3132

Diff for: ‎dev-packages/e2e-tests/test-applications/nestjs-graphql/tests/errors.test.ts

+1
Original file line numberDiff line numberDiff line change
@@ -45,5 +45,6 @@ test('Sends exception to Sentry', async ({ baseURL }) => {
4545
expect(errorEvent.contexts?.trace).toEqual({
4646
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
4747
span_id: expect.stringMatching(/[a-f0-9]{16}/),
48+
parent_span_id: expect.stringMatching(/[a-f0-9]{16}/),
4849
});
4950
});

Diff for: ‎dev-packages/e2e-tests/test-applications/nestjs-with-submodules-decorator/tests/errors.test.ts

+3
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ test('Sends unexpected exception to Sentry if thrown in module with global filte
3434
expect(errorEvent.contexts?.trace).toEqual({
3535
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
3636
span_id: expect.stringMatching(/[a-f0-9]{16}/),
37+
parent_span_id: expect.stringMatching(/[a-f0-9]{16}/),
3738
});
3839
});
3940

@@ -70,6 +71,7 @@ test('Sends unexpected exception to Sentry if thrown in module with local filter
7071
expect(errorEvent.contexts?.trace).toEqual({
7172
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
7273
span_id: expect.stringMatching(/[a-f0-9]{16}/),
74+
parent_span_id: expect.stringMatching(/[a-f0-9]{16}/),
7375
});
7476
});
7577

@@ -108,6 +110,7 @@ test('Sends unexpected exception to Sentry if thrown in module that was register
108110
expect(errorEvent.contexts?.trace).toEqual({
109111
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
110112
span_id: expect.stringMatching(/[a-f0-9]{16}/),
113+
parent_span_id: expect.stringMatching(/[a-f0-9]{16}/),
111114
});
112115
});
113116

Diff for: ‎dev-packages/e2e-tests/test-applications/nestjs-with-submodules/tests/errors.test.ts

+3
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ test('Sends unexpected exception to Sentry if thrown in module with global filte
2626
expect(errorEvent.contexts?.trace).toEqual({
2727
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
2828
span_id: expect.stringMatching(/[a-f0-9]{16}/),
29+
parent_span_id: expect.stringMatching(/[a-f0-9]{16}/),
2930
});
3031
});
3132

@@ -54,6 +55,7 @@ test('Sends unexpected exception to Sentry if thrown in module with local filter
5455
expect(errorEvent.contexts?.trace).toEqual({
5556
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
5657
span_id: expect.stringMatching(/[a-f0-9]{16}/),
58+
parent_span_id: expect.stringMatching(/[a-f0-9]{16}/),
5759
});
5860
});
5961

@@ -84,6 +86,7 @@ test('Sends unexpected exception to Sentry if thrown in module that was register
8486
expect(errorEvent.contexts?.trace).toEqual({
8587
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
8688
span_id: expect.stringMatching(/[a-f0-9]{16}/),
89+
parent_span_id: expect.stringMatching(/[a-f0-9]{16}/),
8790
});
8891
});
8992

Diff for: ‎dev-packages/e2e-tests/test-applications/node-fastify-5/tests/errors.test.ts

+1
Original file line numberDiff line numberDiff line change
@@ -25,5 +25,6 @@ test('Sends correct error event', async ({ baseURL }) => {
2525
expect(errorEvent.contexts?.trace).toEqual({
2626
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
2727
span_id: expect.stringMatching(/[a-f0-9]{16}/),
28+
parent_span_id: expect.stringMatching(/[a-f0-9]{16}/),
2829
});
2930
});

Diff for: ‎dev-packages/e2e-tests/test-applications/node-fastify/tests/errors.test.ts

+1
Original file line numberDiff line numberDiff line change
@@ -25,5 +25,6 @@ test('Sends correct error event', async ({ baseURL }) => {
2525
expect(errorEvent.contexts?.trace).toEqual({
2626
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
2727
span_id: expect.stringMatching(/[a-f0-9]{16}/),
28+
parent_span_id: expect.stringMatching(/[a-f0-9]{16}/),
2829
});
2930
});

Diff for: ‎dev-packages/e2e-tests/test-applications/node-koa/tests/assert.test.ts

+1
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ test('Returns 400 from failed assert', async ({ baseURL }) => {
2626
expect(errorEvent.contexts?.trace).toEqual({
2727
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
2828
span_id: expect.stringMatching(/[a-f0-9]{16}/),
29+
parent_span_id: expect.stringMatching(/[a-f0-9]{16}/),
2930
});
3031
});
3132

Diff for: ‎dev-packages/e2e-tests/test-applications/node-koa/tests/errors.test.ts

+1
Original file line numberDiff line numberDiff line change
@@ -25,5 +25,6 @@ test('Sends correct error event', async ({ baseURL }) => {
2525
expect(errorEvent.contexts?.trace).toEqual({
2626
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
2727
span_id: expect.stringMatching(/[a-f0-9]{16}/),
28+
parent_span_id: expect.stringMatching(/[a-f0-9]{16}/),
2829
});
2930
});

Diff for: ‎dev-packages/e2e-tests/test-applications/node-otel-without-tracing/tests/transactions.test.ts

+40-2
Original file line numberDiff line numberDiff line change
@@ -34,17 +34,55 @@ test('Sends an API route transaction to OTLP', async ({ baseURL }) => {
3434
const scopeSpans = json.resourceSpans?.[0]?.scopeSpans;
3535
expect(scopeSpans).toBeDefined();
3636

37-
// Http server span & undici client spans are emitted
37+
// Http server span & undici client spans are emitted,
38+
// as well as the spans emitted via `Sentry.startSpan()`
3839
// But our default node-fetch spans are not emitted
39-
expect(scopeSpans.length).toEqual(2);
40+
expect(scopeSpans.length).toEqual(3);
4041

4142
const httpScopes = scopeSpans?.filter(scopeSpan => scopeSpan.scope.name === '@opentelemetry/instrumentation-http');
4243
const undiciScopes = scopeSpans?.filter(
4344
scopeSpan => scopeSpan.scope.name === '@opentelemetry/instrumentation-undici',
4445
);
46+
const startSpanScopes = scopeSpans?.filter(scopeSpan => scopeSpan.scope.name === '@sentry/node');
4547

4648
expect(httpScopes.length).toBe(1);
4749

50+
expect(startSpanScopes.length).toBe(1);
51+
expect(startSpanScopes[0].spans.length).toBe(2);
52+
expect(startSpanScopes[0].spans).toEqual([
53+
{
54+
traceId: expect.any(String),
55+
spanId: expect.any(String),
56+
parentSpanId: expect.any(String),
57+
name: 'test-span',
58+
kind: 1,
59+
startTimeUnixNano: expect.any(String),
60+
endTimeUnixNano: expect.any(String),
61+
attributes: [],
62+
droppedAttributesCount: 0,
63+
events: [],
64+
droppedEventsCount: 0,
65+
status: { code: 0 },
66+
links: [],
67+
droppedLinksCount: 0,
68+
},
69+
{
70+
traceId: expect.any(String),
71+
spanId: expect.any(String),
72+
name: 'test-transaction',
73+
kind: 1,
74+
startTimeUnixNano: expect.any(String),
75+
endTimeUnixNano: expect.any(String),
76+
attributes: [{ key: 'sentry.op', value: { stringValue: 'e2e-test' } }],
77+
droppedAttributesCount: 0,
78+
events: [],
79+
droppedEventsCount: 0,
80+
status: { code: 0 },
81+
links: [],
82+
droppedLinksCount: 0,
83+
},
84+
]);
85+
4886
// Undici spans are emitted correctly
4987
expect(undiciScopes.length).toBe(1);
5088
expect(undiciScopes[0].spans.length).toBe(1);

Diff for: ‎dev-packages/node-integration-tests/suites/aws-serverless/aws-integration/s3/test.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,6 @@ describe('awsIntegration', () => {
2626
});
2727

2828
test('should auto-instrument aws-sdk v2 package.', done => {
29-
createRunner(__dirname, 'scenario.js').expect({ transaction: EXPECTED_TRANSCATION }).start(done);
29+
createRunner(__dirname, 'scenario.js').ignore('event').expect({ transaction: EXPECTED_TRANSCATION }).start(done);
3030
});
3131
});

Diff for: ‎dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-spans-in-scope-with-parentSpanId/scenario.ts

+14-2
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,18 @@ Sentry.withScope(scope => {
1515
traceId: '12345678901234567890123456789012',
1616
});
1717

18-
Sentry.startSpan({ name: 'test_span_1' }, () => undefined);
19-
Sentry.startSpan({ name: 'test_span_2' }, () => undefined);
18+
const spanIdTraceId = Sentry.startSpan(
19+
{
20+
name: 'test_span_1',
21+
},
22+
span1 => span1.spanContext().traceId,
23+
);
24+
25+
Sentry.startSpan(
26+
{
27+
name: 'test_span_2',
28+
attributes: { spanIdTraceId },
29+
},
30+
() => undefined,
31+
);
2032
});

Diff for: ‎dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-spans-in-scope-with-parentSpanId/test.ts

+12-21
Original file line numberDiff line numberDiff line change
@@ -6,28 +6,19 @@ afterAll(() => {
66

77
test('should send manually started parallel root spans outside of root context with parentSpanId', done => {
88
createRunner(__dirname, 'scenario.ts')
9+
.expect({ transaction: { transaction: 'test_span_1' } })
910
.expect({
10-
transaction: {
11-
transaction: 'test_span_1',
12-
contexts: {
13-
trace: {
14-
span_id: expect.stringMatching(/[a-f0-9]{16}/),
15-
parent_span_id: '1234567890123456',
16-
trace_id: '12345678901234567890123456789012',
17-
},
18-
},
19-
},
20-
})
21-
.expect({
22-
transaction: {
23-
transaction: 'test_span_2',
24-
contexts: {
25-
trace: {
26-
span_id: expect.stringMatching(/[a-f0-9]{16}/),
27-
parent_span_id: '1234567890123456',
28-
trace_id: '12345678901234567890123456789012',
29-
},
30-
},
11+
transaction: transaction => {
12+
expect(transaction).toBeDefined();
13+
const traceId = transaction.contexts?.trace?.trace_id;
14+
expect(traceId).toBeDefined();
15+
expect(transaction.contexts?.trace?.parent_span_id).toBeUndefined();
16+
17+
const trace1Id = transaction.contexts?.trace?.data?.spanIdTraceId;
18+
expect(trace1Id).toBeDefined();
19+
20+
// Different trace ID as the first span
21+
expect(trace1Id).not.toBe(traceId);
3122
},
3223
})
3324
.start(done);

Diff for: ‎dev-packages/node-integration-tests/suites/public-api/startSpan/parallel-spans-in-scope/test.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,8 @@ test('should send manually started parallel root spans outside of root context',
1919
const trace1Id = transaction.contexts?.trace?.data?.spanIdTraceId;
2020
expect(trace1Id).toBeDefined();
2121

22-
// Same trace ID as the first span
23-
expect(trace1Id).toBe(traceId);
22+
// Different trace ID as the first span
23+
expect(trace1Id).not.toBe(traceId);
2424
},
2525
})
2626
.start(done);

Diff for: ‎docs/assets/node-sdk-trace-propagation.png

1.27 MB
Loading

Diff for: ‎docs/trace-propagation.md

+14
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
# How Trace Propagation Works in the JavaScript SDKs
2+
3+
Trace propagation describes how and when traceId & spanId are set and send for various types of events.
4+
How this behaves varies a bit from Browser to Node SDKs.
5+
6+
## Node SDKs (OpenTelemetry based)
7+
8+
In the Node SDK and related OpenTelemetry-based SDKs, trace propagation works as follows:
9+
10+
![node-sdk-trace-propagation-scenarios](./assets/node-sdk-trace-propagation.png)
11+
12+
## Browser/Other SDKs
13+
14+
TODO

Diff for: ‎packages/core/src/asyncContext/stackStrategy.ts

-1
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ import { getDefaultCurrentScope, getDefaultIsolationScope } from '../defaultScop
22
import { Scope } from '../scope';
33
import type { Client, Scope as ScopeInterface } from '../types-hoist';
44
import { isThenable } from '../utils-hoist/is';
5-
65
import { getMainCarrier, getSentryCarrier } from './../carrier';
76
import type { AsyncContextStrategy } from './types';
87

Diff for: ‎packages/core/src/tracing/sentryNonRecordingSpan.ts

+3-3
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import type {
77
SpanStatus,
88
SpanTimeInput,
99
} from '../types-hoist';
10-
import { uuid4 } from '../utils-hoist/misc';
10+
import { generateSpanId, generateTraceId } from '../utils-hoist/propagationContext';
1111
import { TRACE_FLAG_NONE } from '../utils/spanUtils';
1212

1313
/**
@@ -18,8 +18,8 @@ export class SentryNonRecordingSpan implements Span {
1818
private _spanId: string;
1919

2020
public constructor(spanContext: SentrySpanArguments = {}) {
21-
this._traceId = spanContext.traceId || uuid4();
22-
this._spanId = spanContext.spanId || uuid4().substring(16);
21+
this._traceId = spanContext.traceId || generateTraceId();
22+
this._spanId = spanContext.spanId || generateSpanId();
2323
}
2424

2525
/** @inheritdoc */

Diff for: ‎packages/core/src/tracing/sentrySpan.ts

+3-3
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,8 @@ import type {
2525
TransactionSource,
2626
} from '../types-hoist';
2727
import { logger } from '../utils-hoist/logger';
28-
import { uuid4 } from '../utils-hoist/misc';
2928
import { dropUndefinedKeys } from '../utils-hoist/object';
29+
import { generateSpanId, generateTraceId } from '../utils-hoist/propagationContext';
3030
import { timestampInSeconds } from '../utils-hoist/time';
3131
import {
3232
TRACE_FLAG_NONE,
@@ -75,8 +75,8 @@ export class SentrySpan implements Span {
7575
* @hidden
7676
*/
7777
public constructor(spanContext: SentrySpanArguments = {}) {
78-
this._traceId = spanContext.traceId || uuid4();
79-
this._spanId = spanContext.spanId || uuid4().substring(16);
78+
this._traceId = spanContext.traceId || generateTraceId();
79+
this._spanId = spanContext.spanId || generateSpanId();
8080
this._startTime = spanContext.startTimestamp || timestampInSeconds();
8181

8282
this._attributes = {};

Diff for: ‎packages/core/src/utils-hoist/tracing.ts

+2-3
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
import type { PropagationContext, TraceparentData } from '../types-hoist';
22

33
import { baggageHeaderToDynamicSamplingContext } from './baggage';
4-
import { uuid4 } from './misc';
54
import { generateSpanId, generateTraceId } from './propagationContext';
65

76
// eslint-disable-next-line @sentry-internal/sdk/no-regexp-constructor -- RegExp is used for readability here
@@ -76,8 +75,8 @@ export function propagationContextFromHeaders(
7675
* Create sentry-trace header from span context values.
7776
*/
7877
export function generateSentryTraceHeader(
79-
traceId: string = uuid4(),
80-
spanId: string = uuid4().substring(16),
78+
traceId: string = generateTraceId(),
79+
spanId: string = generateSpanId(),
8180
sampled?: boolean,
8281
): string {
8382
let sampledString = '';

Diff for: ‎packages/core/src/utils/spanUtils.ts

+12-3
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import type {
1919
} from '../types-hoist';
2020
import { consoleSandbox } from '../utils-hoist/logger';
2121
import { addNonEnumerableProperty, dropUndefinedKeys } from '../utils-hoist/object';
22+
import { generateSpanId } from '../utils-hoist/propagationContext';
2223
import { timestampInSeconds } from '../utils-hoist/time';
2324
import { generateSentryTraceHeader } from '../utils-hoist/tracing';
2425
import { _getSpanForScope } from './spanOnScope';
@@ -54,10 +55,18 @@ export function spanToTransactionTraceContext(span: Span): TraceContext {
5455
* Convert a span to a trace context, which can be sent as the `trace` context in a non-transaction event.
5556
*/
5657
export function spanToTraceContext(span: Span): TraceContext {
57-
const { spanId: span_id, traceId: trace_id } = span.spanContext();
58-
const { parent_span_id } = spanToJSON(span);
58+
const { spanId, traceId: trace_id, isRemote } = span.spanContext();
59+
60+
// If the span is remote, we use a random/virtual span as span_id to the trace context,
61+
// and the remote span as parent_span_id
62+
const parent_span_id = isRemote ? spanId : spanToJSON(span).parent_span_id;
63+
const span_id = isRemote ? generateSpanId() : spanId;
5964

60-
return dropUndefinedKeys({ parent_span_id, span_id, trace_id });
65+
return dropUndefinedKeys({
66+
parent_span_id,
67+
span_id,
68+
trace_id,
69+
});
6170
}
6271

6372
/**

0 commit comments

Comments
 (0)