Skip to content

feat!: Remove spanId from propagation context #14733

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

Merged
merged 5 commits into from
Jan 9, 2025
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
@@ -1,6 +1,5 @@
Sentry.getCurrentScope().setPropagationContext({
parentSpanId: '1234567890123456',
spanId: '123456789012345x',
traceId: '12345678901234567890123456789012',
});

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
dsn: 'https://public@dsn.ingest.sentry.io/1337',
integrations: integrations => {
integrations.push(Sentry.browserTracingIntegration());
return integrations.filter(i => i.name !== 'BrowserSession');
},
tracesSampleRate: 0,
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Sentry.captureException(new Error('test error'));
Sentry.captureException(new Error('test error 2'));
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8" />
<meta name="sentry-trace" content="12312012123120121231201212312012-1121201211212012" />
<meta
name="baggage"
content="sentry-release=2.1.12,sentry-public_key=public,sentry-trace_id=123"
/>
</head>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { expect } from '@playwright/test';
import type { Event } from '@sentry/browser';
import { sentryTest } from '../../../../utils/fixtures';
import { getMultipleSentryEnvelopeRequests, shouldSkipTracingTest } from '../../../../utils/helpers';

sentryTest('errors in TwP mode have same trace ID & span IDs', async ({ getLocalTestUrl, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}

const traceId = '12312012123120121231201212312012';
const spanId = '1121201211212012';

const url = await getLocalTestUrl({ testDir: __dirname });
const [event1, event2] = await getMultipleSentryEnvelopeRequests<Event>(page, 2, { url });

// Ensure these are the actual errors we care about
expect(event1.exception?.values?.[0].value).toContain('test error');
expect(event2.exception?.values?.[0].value).toContain('test error');

const contexts1 = event1.contexts;
const { trace_id: traceId1, span_id: spanId1 } = contexts1?.trace || {};
expect(traceId1).toEqual(traceId);

// Span ID is a virtual span, not the propagated one
expect(spanId1).not.toEqual(spanId);
expect(spanId1).toMatch(/^[a-f0-9]{16}$/);

const contexts2 = event2.contexts;
const { trace_id: traceId2, span_id: spanId2 } = contexts2?.trace || {};
expect(traceId2).toEqual(traceId);
expect(spanId2).toMatch(/^[a-f0-9]{16}$/);

expect(spanId2).toEqual(spanId1);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
dsn: 'https://public@dsn.ingest.sentry.io/1337',
integrations: integrations => {
integrations.push(Sentry.browserTracingIntegration());
return integrations.filter(i => i.name !== 'BrowserSession');
},
tracesSampleRate: 0,
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Sentry.captureException(new Error('test error'));
Sentry.captureException(new Error('test error 2'));
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { expect } from '@playwright/test';
import type { Event } from '@sentry/browser';
import { sentryTest } from '../../../../utils/fixtures';
import { getMultipleSentryEnvelopeRequests, shouldSkipTracingTest } from '../../../../utils/helpers';

sentryTest('errors in TwP mode have same trace ID & span IDs', async ({ getLocalTestUrl, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}

const url = await getLocalTestUrl({ testDir: __dirname });
const [event1, event2] = await getMultipleSentryEnvelopeRequests<Event>(page, 2, { url });

// Ensure these are the actual errors we care about
expect(event1.exception?.values?.[0].value).toContain('test error');
expect(event2.exception?.values?.[0].value).toContain('test error');

const contexts1 = event1.contexts;
const { trace_id: traceId1, span_id: spanId1 } = contexts1?.trace || {};
expect(traceId1).toMatch(/^[a-f0-9]{32}$/);
expect(spanId1).toMatch(/^[a-f0-9]{16}$/);

const contexts2 = event2.contexts;
const { trace_id: traceId2, span_id: spanId2 } = contexts2?.trace || {};
expect(traceId2).toMatch(/^[a-f0-9]{32}$/);
expect(spanId2).toMatch(/^[a-f0-9]{16}$/);

expect(traceId2).toEqual(traceId1);
expect(spanId2).toEqual(spanId1);
});
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ Sentry.init({

Sentry.getCurrentScope().setPropagationContext({
parentSpanId: '1234567890123456',
spanId: '123456789012345x',
traceId: '12345678901234567890123456789012',
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ Sentry.init({
Sentry.withScope(scope => {
scope.setPropagationContext({
parentSpanId: '1234567890123456',
spanId: '123456789012345x',
traceId: '12345678901234567890123456789012',
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ describe('errors in TwP mode have same trace in trace context and getTraceData()
cleanupChildProcesses();
});

// In a request handler, the spanId is consistent inside of the request
test('in incoming request', done => {
createRunner(__dirname, 'server.js')
.expect({
Expand All @@ -30,6 +31,7 @@ describe('errors in TwP mode have same trace in trace context and getTraceData()
.makeRequest('get', '/test');
});

// Outside of a request handler, the spanId is random
test('outside of a request handler', done => {
createRunner(__dirname, 'no-server.js')
.expect({
Expand All @@ -41,11 +43,18 @@ describe('errors in TwP mode have same trace in trace context and getTraceData()

const traceData = contexts?.traceData || {};

expect(traceData['sentry-trace']).toEqual(`${trace_id}-${span_id}`);
expect(traceData['sentry-trace']).toMatch(/^[a-f0-9]{32}-[a-f0-9]{16}$/);
expect(traceData['sentry-trace']).toContain(`${trace_id}-`);
// span_id is a random span ID
expect(traceData['sentry-trace']).not.toContain(span_id);

expect(traceData.baggage).toContain(`sentry-trace_id=${trace_id}`);
expect(traceData.baggage).not.toContain('sentry-sampled=');

expect(traceData.metaTags).toContain(`<meta name="sentry-trace" content="${trace_id}-${span_id}"/>`);
expect(traceData.metaTags).toMatch(/<meta name="sentry-trace" content="[a-f0-9]{32}-[a-f0-9]{16}"\/>/);
expect(traceData.metaTags).toContain(`<meta name="sentry-trace" content="${trace_id}-`);
// span_id is a random span ID
expect(traceData.metaTags).not.toContain(span_id);
Comment on lines +46 to +57
Copy link
Member

Choose a reason for hiding this comment

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

hmm so I'm a bit concerned about this. IIUC previously, the spanIds in this test were equal because we stored it on the PC. Now, consecutive calls to APIs like getTraceData() or getTraceMetaTags() like in this test deliver diverging span ids.

This also implies the following scenario for a TwP-configured SDK:

  1. app makes http request and propagates a sentry-trace header with spanId 123
  2. directly afterwards an error is captured and its event.context.trace holds a different spanId 456

I thought about the implications of this and while I'm still worried, I couldn't really come up with a concrete use case where this is actually problematic. I was especially worried if we'd send different spanIds within one error or transaction event but I think we only send this in event.context.trace. The trace envelope header does not contain the spanId at all, so it shouldn't be a problem.

However, we should ensure that the same traceId is still used in both places. Which I hope we have tests for but it's better to double-check.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, so the traceId is tested here (see line 53).

I totally get what you mean, it was the same for me - thinking, hmm, this is different, but then, is it actually a problem?

expect(traceData.metaTags).toContain(`sentry-trace_id=${trace_id}`);
expect(traceData.metaTags).not.toContain('sentry-sampled=');
},
Expand Down
10 changes: 0 additions & 10 deletions packages/browser/test/tracing/browserTracingIntegration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -643,19 +643,15 @@ describe('browserTracingIntegration', () => {
const newCurrentScopePropCtx = getCurrentScope().getPropagationContext();

expect(oldCurrentScopePropCtx).toEqual({
spanId: expect.stringMatching(/[a-f0-9]{16}/),
traceId: expect.stringMatching(/[a-f0-9]{32}/),
});
expect(oldIsolationScopePropCtx).toEqual({
spanId: expect.stringMatching(/[a-f0-9]{16}/),
traceId: expect.stringMatching(/[a-f0-9]{32}/),
});
expect(newCurrentScopePropCtx).toEqual({
spanId: expect.stringMatching(/[a-f0-9]{16}/),
traceId: expect.stringMatching(/[a-f0-9]{32}/),
});
expect(newIsolationScopePropCtx).toEqual({
spanId: expect.stringMatching(/[a-f0-9]{16}/),
traceId: expect.stringMatching(/[a-f0-9]{32}/),
});

Expand All @@ -680,16 +676,13 @@ describe('browserTracingIntegration', () => {

const propCtxBeforeEnd = getCurrentScope().getPropagationContext();
expect(propCtxBeforeEnd).toStrictEqual({
spanId: expect.stringMatching(/[a-f0-9]{16}/),
traceId: expect.stringMatching(/[a-f0-9]{32}/),
});

navigationSpan!.end();

const propCtxAfterEnd = getCurrentScope().getPropagationContext();
expect(propCtxAfterEnd).toStrictEqual({
// eslint-disable-next-line deprecation/deprecation
spanId: propCtxBeforeEnd.spanId,
traceId: propCtxBeforeEnd.traceId,
sampled: true,
dsc: {
Expand Down Expand Up @@ -720,16 +713,13 @@ describe('browserTracingIntegration', () => {

const propCtxBeforeEnd = getCurrentScope().getPropagationContext();
expect(propCtxBeforeEnd).toStrictEqual({
spanId: expect.stringMatching(/[a-f0-9]{16}/),
traceId: expect.stringMatching(/[a-f0-9]{32}/),
});

navigationSpan!.end();

const propCtxAfterEnd = getCurrentScope().getPropagationContext();
expect(propCtxAfterEnd).toStrictEqual({
// eslint-disable-next-line deprecation/deprecation
spanId: propCtxBeforeEnd.spanId,
traceId: propCtxBeforeEnd.traceId,
sampled: false,
dsc: {
Expand Down
7 changes: 3 additions & 4 deletions packages/core/src/currentScopes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { getGlobalSingleton, getMainCarrier } from './carrier';
import type { Client } from './client';
import { Scope } from './scope';
import type { TraceContext } from './types-hoist';
import { generateSpanId } from './utils-hoist';
import { dropUndefinedKeys } from './utils-hoist/object';

/**
Expand Down Expand Up @@ -127,13 +128,11 @@ export function getClient<C extends Client>(): C | undefined {
export function getTraceContextFromScope(scope: Scope): TraceContext {
const propagationContext = scope.getPropagationContext();

// TODO(v9): Use generateSpanId() instead of spanId
// eslint-disable-next-line deprecation/deprecation
const { traceId, spanId, parentSpanId } = propagationContext;
const { traceId, parentSpanId, propagationSpanId } = propagationContext;

const traceContext: TraceContext = dropUndefinedKeys({
trace_id: traceId,
span_id: spanId,
span_id: propagationSpanId || generateSpanId(),
parent_span_id: parentSpanId,
});

Expand Down
13 changes: 3 additions & 10 deletions packages/core/src/scope.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import type {
import { isPlainObject } from './utils-hoist/is';
import { logger } from './utils-hoist/logger';
import { uuid4 } from './utils-hoist/misc';
import { generateSpanId, generateTraceId } from './utils-hoist/propagationContext';
import { generateTraceId } from './utils-hoist/propagationContext';
import { dateTimestampInSeconds } from './utils-hoist/time';
import { merge } from './utils/merge';
import { _getSpanForScope, _setSpanForScope } from './utils/spanOnScope';
Expand Down Expand Up @@ -166,7 +166,6 @@ export class Scope {
this._sdkProcessingMetadata = {};
this._propagationContext = {
traceId: generateTraceId(),
spanId: generateSpanId(),
};
}

Expand Down Expand Up @@ -555,14 +554,8 @@ export class Scope {
/**
* Add propagation context to the scope, used for distributed tracing
*/
public setPropagationContext(
context: Omit<PropagationContext, 'spanId'> & Partial<Pick<PropagationContext, 'spanId'>>,
): this {
this._propagationContext = {
// eslint-disable-next-line deprecation/deprecation
spanId: generateSpanId(),
...context,
};
public setPropagationContext(context: PropagationContext): this {
this._propagationContext = context;
return this;
}

Expand Down
19 changes: 10 additions & 9 deletions packages/core/src/types-hoist/tracing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,28 +15,29 @@ export interface PropagationContext {
* Either represents the incoming `traceId` or the `traceId` generated by the current SDK, if there was no incoming trace.
*/
traceId: string;
/**
* Represents the execution context of the current SDK. This acts as a fallback value to associate events with a
* particular execution context when performance monitoring is disabled.
*
* The ID of a current span (if one exists) should have precedence over this value when propagating trace data.
*
* @deprecated This value will not be used anymore in the future, and should not be set or read anymore.
*/
spanId: string;

/**
* Represents the sampling decision of the incoming trace.
*
* The current SDK should not modify this value!
*/
sampled?: boolean;

/**
* The `parentSpanId` denotes the ID of the incoming client span. If there is no `parentSpanId` on the propagation
* context, it means that the the incoming trace didn't come from a span.
*
* The current SDK should not modify this value!
*/
parentSpanId?: string;

/**
* A span ID that should be used for the `trace` context of various event types, and for propagation of a `parentSpanId` to downstream services, when performance is disabled or when there is no active span.
* This value should be set by the SDK in an informed way when the same span ID should be used for one unit of execution (e.g. a request, usually tied to the isolation scope).
* If this value is undefined on the propagation context, the SDK will generate a random span ID for `trace` contexts and trace propagation.
*/
propagationSpanId?: string;

/**
* An undefined dsc in the propagation context means that the current SDK invocation is the head of trace and still free to modify and set the DSC for outgoing requests.
*
Expand Down
1 change: 0 additions & 1 deletion packages/core/src/utils-hoist/propagationContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import { uuid4 } from './misc';
export function generatePropagationContext(): PropagationContext {
return {
traceId: generateTraceId(),
spanId: generateSpanId(),
};
}

Expand Down
9 changes: 3 additions & 6 deletions packages/core/src/utils-hoist/tracing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,17 +55,14 @@ export function propagationContextFromHeaders(
const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(baggage);

if (!traceparentData || !traceparentData.traceId) {
return { traceId: generateTraceId(), spanId: generateSpanId() };
return { traceId: generateTraceId() };
}

const { traceId, parentSpanId, parentSampled } = traceparentData;

const virtualSpanId = generateSpanId();

return {
traceId,
parentSpanId,
spanId: virtualSpanId,
sampled: parentSampled,
dsc: dynamicSamplingContext || {}, // If we have traceparent data but no DSC it means we are not head of trace and we must freeze it
};
Expand All @@ -75,8 +72,8 @@ export function propagationContextFromHeaders(
* Create sentry-trace header from span context values.
*/
export function generateSentryTraceHeader(
traceId: string = generateTraceId(),
spanId: string = generateSpanId(),
traceId: string | undefined = generateTraceId(),
spanId: string | undefined = generateSpanId(),
sampled?: boolean,
): string {
let sampledString = '';
Expand Down
5 changes: 4 additions & 1 deletion packages/core/src/utils/spanUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
} from '../semanticAttributes';
import type { SentrySpan } from '../tracing/sentrySpan';
import { SPAN_STATUS_OK, SPAN_STATUS_UNSET } from '../tracing/spanstatus';
import { getCapturedScopesOnSpan } from '../tracing/utils';
import type {
Span,
SpanAttributes,
Expand Down Expand Up @@ -61,7 +62,9 @@ export function spanToTraceContext(span: Span): TraceContext {
// 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;
const scope = getCapturedScopesOnSpan(span).scope;

const span_id = isRemote ? scope?.getPropagationContext().propagationSpanId || generateSpanId() : spanId;

return dropUndefinedKeys({
parent_span_id,
Expand Down
6 changes: 2 additions & 4 deletions packages/core/src/utils/traceData.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,6 @@ export function getTraceData(options: { span?: Span } = {}): SerializedTraceData
* Get a sentry-trace header value for the given scope.
*/
function scopeToTraceHeader(scope: Scope): string {
// TODO(v9): Use generateSpanId() instead of spanId
// eslint-disable-next-line deprecation/deprecation
const { traceId, sampled, spanId } = scope.getPropagationContext();
return generateSentryTraceHeader(traceId, spanId, sampled);
const { traceId, sampled, propagationSpanId } = scope.getPropagationContext();
return generateSentryTraceHeader(traceId, propagationSpanId, sampled);
}
Loading
Loading