Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(opentelemetry): Stop looking at propagation context for span creation #14481

Merged
merged 5 commits into from
Dec 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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}/),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not really know why this was not here before, but IMHO it was incorrect? These errors (same for the other nest E2E tests), as you'll usually have the http.server span and then inside it some e.g. route handler span or so, so the active span at the time of the error should usually have a parent_span_id. So I'd say this fixes incorrect (?) behavior 🤔

});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}/),
});
});

Expand Down Expand Up @@ -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}/),
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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}/),
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}/),
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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}/),
});
});

Expand Down Expand Up @@ -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}/),
});
});

Expand Down Expand Up @@ -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}/),
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}/),
});
});

Expand Down Expand Up @@ -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}/),
});
});

Expand Down Expand Up @@ -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}/),
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}/),
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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}/),
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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}/),
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}/),
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was also incorrect before, spans from startSpan() did not end up here, although they should. This was probably because of incorrect propagation in this scenario.


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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated, but saw this flaking every now and then so decided to just ignore events here.

});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
});
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Binary file added docs/assets/node-sdk-trace-propagation.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
14 changes: 14 additions & 0 deletions docs/trace-propagation.md
Original file line number Diff line number Diff line change
@@ -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
1 change: 0 additions & 1 deletion packages/core/src/asyncContext/stackStrategy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/tracing/sentryNonRecordingSpan.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

/**
Expand All @@ -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 */
Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/tracing/sentrySpan.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 = {};
Expand Down
5 changes: 2 additions & 3 deletions packages/core/src/utils-hoist/tracing.ts
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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 = '';
Expand Down
15 changes: 12 additions & 3 deletions packages/core/src/utils/spanUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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,
});
}

/**
Expand Down
Loading
Loading