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.

Sign up for GitHub

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
mydea merged 5 commits into from
Jan 9, 2025
Merged

feat!: Remove spanId from propagation context #14733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e761fd3
feat!: Remove `spanId` from propagation context
mydea Jan 7, 2025
80182b5
fixes...
mydea Jan 7, 2025
dc4e1b4
fix circular dep check
mydea Jan 8, 2025
7cb9c86
add comments & tests
mydea Jan 8, 2025
a57be3a
Update packages/core/src/types-hoist/tracing.ts
mydea Jan 8, 2025
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
1 change: 0 additions & 1 deletion ...ration-tests/suites/public-api/startSpan/parallel-root-spans-with-parentSpanId/subject.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
Sentry.getCurrentScope().setPropagationContext({
parentSpanId: '1234567890123456',
spanId: '123456789012345x',
traceId: '12345678901234567890123456789012',
});

Expand Down
12 changes: 12 additions & 0 deletions ...rowser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/init.js
View file
Open in desktop
Original file line numberDiff line numberDiff 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,
});
2 changes: 2 additions & 0 deletions ...ser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/subject.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
Sentry.captureException(new Error('test error'));
Sentry.captureException(new Error('test error 2'));
11 changes: 11 additions & 0 deletions ...-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/template.html
View file
Open in desktop
Original file line numberDiff line numberDiff 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>
35 changes: 35 additions & 0 deletions ...rowser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/test.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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);
});
12 changes: 12 additions & 0 deletions ...ges/browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/init.js
View file
Open in desktop
Original file line numberDiff line numberDiff 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,
});
2 changes: 2 additions & 0 deletions .../browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/subject.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
Sentry.captureException(new Error('test error'));
Sentry.captureException(new Error('test error 2'));
30 changes: 30 additions & 0 deletions ...ges/browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/test.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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);
});
1 change: 0 additions & 1 deletion ...ckages/node-integration-tests/suites/public-api/startSpan/parallel-root-spans/scenario.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -10,7 +10,6 @@ Sentry.init({

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

Expand Down
1 change: 0 additions & 1 deletion ...n-tests/suites/public-api/startSpan/parallel-spans-in-scope-with-parentSpanId/scenario.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -11,7 +11,6 @@ Sentry.init({
Sentry.withScope(scope => {
scope.setPropagationContext({
parentSpanId: '1234567890123456',
spanId: '123456789012345x',
traceId: '12345678901234567890123456789012',
});

Expand Down
13 changes: 11 additions & 2 deletions dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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

@Lms24Lms24Dec 17, 2024

Copy link
Copy Markdown
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.

@mydeamydeaDec 18, 2024

Copy link
Copy Markdown
MemberAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand Up@@ -166,7 +166,6 @@ export class Scope {
this._sdkProcessingMetadata = {};
this._propagationContext = {
traceId: generateTraceId(),
spanId: generateSpanId(),
};
}

Expand DownExpand 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

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.

 Sign up for GitHub

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

mydea
merged 5 commits into

from

Jan 9, 2025
 Merged


feat!: Remove spanId from propagation context
#14733
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


5 commits


Select commit
Hold shift + click to select a range
e761fd3

feat!: Remove `spanId` from propagation context


mydea Jan 7, 2025
80182b5

fixes...


mydea Jan 7, 2025
dc4e1b4

fix circular dep check


mydea Jan 8, 2025
7cb9c86

add comments & tests


mydea Jan 8, 2025
a57be3a

Update packages/core/src/types-hoist/tracing.ts


mydea Jan 8, 2025

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


1 change: 0 additions & 1 deletion
...ration-tests/suites/public-api/startSpan/parallel-root-spans-with-parentSpanId/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
Sentry.getCurrentScope().setPropagationContext({
 parentSpanId: '1234567890123456',
 spanId: '123456789012345x',
 traceId: '12345678901234567890123456789012',
});

Expand Down

12 changes: 12 additions & 0 deletions
...rowser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/init.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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,
});

2 changes: 2 additions & 0 deletions
...ser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
Sentry.captureException(new Error('test error'));
Sentry.captureException(new Error('test error 2'));

11 changes: 11 additions & 0 deletions
...-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/template.html

View file


Open in desktop

Original file line numberDiff line numberDiff 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>

35 changes: 35 additions & 0 deletions
...rowser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff 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);
});

12 changes: 12 additions & 0 deletions
...ges/browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/init.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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,
});

2 changes: 2 additions & 0 deletions
.../browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
Sentry.captureException(new Error('test error'));
Sentry.captureException(new Error('test error 2'));

30 changes: 30 additions & 0 deletions
...ges/browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff 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);
});

1 change: 0 additions & 1 deletion
...ckages/node-integration-tests/suites/public-api/startSpan/parallel-root-spans/scenario.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -10,7 +10,6 @@ Sentry.init({

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

Expand Down

1 change: 0 additions & 1 deletion
...n-tests/suites/public-api/startSpan/parallel-spans-in-scope-with-parentSpanId/scenario.ts

View file


Open in desktop

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

Expand Down

13 changes: 11 additions & 2 deletions
dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff 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

@Lms24Lms24Dec 17, 2024

Copy link


Copy Markdown

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.
@mydeamydeaDec 18, 2024

Copy link


Copy Markdown

MemberAuthor
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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand Up@@ -166,7 +166,6 @@ export class Scope {
 this._sdkProcessingMetadata = {};
 this._propagationContext = {
 traceId: generateTraceId(),
 spanId: generateSpanId(),
 };
 }

Expand DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

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.

 Sign up for GitHub

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

mydea
merged 5 commits into

from

Jan 9, 2025
 Merged


feat!: Remove spanId from propagation context
#14733
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


5 commits


Select commit
Hold shift + click to select a range
e761fd3

feat!: Remove `spanId` from propagation context


mydea Jan 7, 2025
80182b5

fixes...


mydea Jan 7, 2025
dc4e1b4

fix circular dep check


mydea Jan 8, 2025
7cb9c86

add comments & tests


mydea Jan 8, 2025
a57be3a

Update packages/core/src/types-hoist/tracing.ts


mydea Jan 8, 2025

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


1 change: 0 additions & 1 deletion
...ration-tests/suites/public-api/startSpan/parallel-root-spans-with-parentSpanId/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
Sentry.getCurrentScope().setPropagationContext({
 parentSpanId: '1234567890123456',
 spanId: '123456789012345x',
 traceId: '12345678901234567890123456789012',
});

Expand Down

12 changes: 12 additions & 0 deletions
...rowser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/init.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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,
});

2 changes: 2 additions & 0 deletions
...ser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
Sentry.captureException(new Error('test error'));
Sentry.captureException(new Error('test error 2'));

11 changes: 11 additions & 0 deletions
...-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/template.html

View file


Open in desktop

Original file line numberDiff line numberDiff 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>

35 changes: 35 additions & 0 deletions
...rowser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff 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);
});

12 changes: 12 additions & 0 deletions
...ges/browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/init.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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,
});

2 changes: 2 additions & 0 deletions
.../browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
Sentry.captureException(new Error('test error'));
Sentry.captureException(new Error('test error 2'));

30 changes: 30 additions & 0 deletions
...ges/browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff 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);
});

1 change: 0 additions & 1 deletion
...ckages/node-integration-tests/suites/public-api/startSpan/parallel-root-spans/scenario.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -10,7 +10,6 @@ Sentry.init({

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

Expand Down

1 change: 0 additions & 1 deletion
...n-tests/suites/public-api/startSpan/parallel-spans-in-scope-with-parentSpanId/scenario.ts

View file


Open in desktop

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

Expand Down

13 changes: 11 additions & 2 deletions
dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff 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

@Lms24Lms24Dec 17, 2024

Copy link


Copy Markdown

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.
@mydeamydeaDec 18, 2024

Copy link


Copy Markdown

MemberAuthor
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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand Up@@ -166,7 +166,6 @@ export class Scope {
 this._sdkProcessingMetadata = {};
 this._propagationContext = {
 traceId: generateTraceId(),
 spanId: generateSpanId(),
 };
 }

Expand DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

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.

 Sign up for GitHub

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

mydea
merged 5 commits into

from

Jan 9, 2025
 Merged


feat!: Remove spanId from propagation context
#14733
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


5 commits


Select commit
Hold shift + click to select a range
e761fd3

feat!: Remove `spanId` from propagation context


mydea Jan 7, 2025
80182b5

fixes...


mydea Jan 7, 2025
dc4e1b4

fix circular dep check


mydea Jan 8, 2025
7cb9c86

add comments & tests


mydea Jan 8, 2025
a57be3a

Update packages/core/src/types-hoist/tracing.ts


mydea Jan 8, 2025

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


1 change: 0 additions & 1 deletion
...ration-tests/suites/public-api/startSpan/parallel-root-spans-with-parentSpanId/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
Sentry.getCurrentScope().setPropagationContext({
 parentSpanId: '1234567890123456',
 spanId: '123456789012345x',
 traceId: '12345678901234567890123456789012',
});

Expand Down

12 changes: 12 additions & 0 deletions
...rowser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/init.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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,
});

2 changes: 2 additions & 0 deletions
...ser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
Sentry.captureException(new Error('test error'));
Sentry.captureException(new Error('test error 2'));

11 changes: 11 additions & 0 deletions
...-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/template.html

View file


Open in desktop

Original file line numberDiff line numberDiff 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>

35 changes: 35 additions & 0 deletions
...rowser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff 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);
});

12 changes: 12 additions & 0 deletions
...ges/browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/init.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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,
});

2 changes: 2 additions & 0 deletions
.../browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
Sentry.captureException(new Error('test error'));
Sentry.captureException(new Error('test error 2'));

30 changes: 30 additions & 0 deletions
...ges/browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff 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);
});

1 change: 0 additions & 1 deletion
...ckages/node-integration-tests/suites/public-api/startSpan/parallel-root-spans/scenario.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -10,7 +10,6 @@ Sentry.init({

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

Expand Down

1 change: 0 additions & 1 deletion
...n-tests/suites/public-api/startSpan/parallel-spans-in-scope-with-parentSpanId/scenario.ts

View file


Open in desktop

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

Expand Down

13 changes: 11 additions & 2 deletions
dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff 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

@Lms24Lms24Dec 17, 2024

Copy link


Copy Markdown

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.
@mydeamydeaDec 18, 2024

Copy link


Copy Markdown

MemberAuthor
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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand Up@@ -166,7 +166,6 @@ export class Scope {
 this._sdkProcessingMetadata = {};
 this._propagationContext = {
 traceId: generateTraceId(),
 spanId: generateSpanId(),
 };
 }

Expand DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

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.

 Sign up for GitHub

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

mydea
merged 5 commits into

from

Jan 9, 2025
 Merged


feat!: Remove spanId from propagation context
#14733
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


5 commits


Select commit
Hold shift + click to select a range
e761fd3

feat!: Remove `spanId` from propagation context


mydea Jan 7, 2025
80182b5

fixes...


mydea Jan 7, 2025
dc4e1b4

fix circular dep check


mydea Jan 8, 2025
7cb9c86

add comments & tests


mydea Jan 8, 2025
a57be3a

Update packages/core/src/types-hoist/tracing.ts


mydea Jan 8, 2025

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


1 change: 0 additions & 1 deletion
...ration-tests/suites/public-api/startSpan/parallel-root-spans-with-parentSpanId/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
Sentry.getCurrentScope().setPropagationContext({
 parentSpanId: '1234567890123456',
 spanId: '123456789012345x',
 traceId: '12345678901234567890123456789012',
});

Expand Down

12 changes: 12 additions & 0 deletions
...rowser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/init.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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,
});

2 changes: 2 additions & 0 deletions
...ser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
Sentry.captureException(new Error('test error'));
Sentry.captureException(new Error('test error 2'));

11 changes: 11 additions & 0 deletions
...-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/template.html

View file


Open in desktop

Original file line numberDiff line numberDiff 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>

35 changes: 35 additions & 0 deletions
...rowser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff 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);
});

12 changes: 12 additions & 0 deletions
...ges/browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/init.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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,
});

2 changes: 2 additions & 0 deletions
.../browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
Sentry.captureException(new Error('test error'));
Sentry.captureException(new Error('test error 2'));

30 changes: 30 additions & 0 deletions
...ges/browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff 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);
});

1 change: 0 additions & 1 deletion
...ckages/node-integration-tests/suites/public-api/startSpan/parallel-root-spans/scenario.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -10,7 +10,6 @@ Sentry.init({

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

Expand Down

1 change: 0 additions & 1 deletion
...n-tests/suites/public-api/startSpan/parallel-spans-in-scope-with-parentSpanId/scenario.ts

View file


Open in desktop

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

Expand Down

13 changes: 11 additions & 2 deletions
dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff 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

@Lms24Lms24Dec 17, 2024

Copy link


Copy Markdown

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.
@mydeamydeaDec 18, 2024

Copy link


Copy Markdown

MemberAuthor
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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand Up@@ -166,7 +166,6 @@ export class Scope {
 this._sdkProcessingMetadata = {};
 this._propagationContext = {
 traceId: generateTraceId(),
 spanId: generateSpanId(),
 };
 }

Expand DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

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.

 Sign up for GitHub

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

mydea
merged 5 commits into

from

Jan 9, 2025
 Merged


feat!: Remove spanId from propagation context
#14733
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


5 commits


Select commit
Hold shift + click to select a range
e761fd3

feat!: Remove `spanId` from propagation context


mydea Jan 7, 2025
80182b5

fixes...


mydea Jan 7, 2025
dc4e1b4

fix circular dep check


mydea Jan 8, 2025
7cb9c86

add comments & tests


mydea Jan 8, 2025
a57be3a

Update packages/core/src/types-hoist/tracing.ts


mydea Jan 8, 2025

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


1 change: 0 additions & 1 deletion
...ration-tests/suites/public-api/startSpan/parallel-root-spans-with-parentSpanId/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
Sentry.getCurrentScope().setPropagationContext({
 parentSpanId: '1234567890123456',
 spanId: '123456789012345x',
 traceId: '12345678901234567890123456789012',
});

Expand Down

12 changes: 12 additions & 0 deletions
...rowser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/init.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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,
});

2 changes: 2 additions & 0 deletions
...ser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
Sentry.captureException(new Error('test error'));
Sentry.captureException(new Error('test error 2'));

11 changes: 11 additions & 0 deletions
...-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/template.html

View file


Open in desktop

Original file line numberDiff line numberDiff 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>

35 changes: 35 additions & 0 deletions
...rowser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff 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);
});

12 changes: 12 additions & 0 deletions
...ges/browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/init.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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,
});

2 changes: 2 additions & 0 deletions
.../browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
Sentry.captureException(new Error('test error'));
Sentry.captureException(new Error('test error 2'));

30 changes: 30 additions & 0 deletions
...ges/browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff 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);
});

1 change: 0 additions & 1 deletion
...ckages/node-integration-tests/suites/public-api/startSpan/parallel-root-spans/scenario.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -10,7 +10,6 @@ Sentry.init({

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

Expand Down

1 change: 0 additions & 1 deletion
...n-tests/suites/public-api/startSpan/parallel-spans-in-scope-with-parentSpanId/scenario.ts

View file


Open in desktop

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

Expand Down

13 changes: 11 additions & 2 deletions
dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff 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

@Lms24Lms24Dec 17, 2024

Copy link


Copy Markdown

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.
@mydeamydeaDec 18, 2024

Copy link


Copy Markdown

MemberAuthor
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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand Up@@ -166,7 +166,6 @@ export class Scope {
 this._sdkProcessingMetadata = {};
 this._propagationContext = {
 traceId: generateTraceId(),
 spanId: generateSpanId(),
 };
 }

Expand DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

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.

 Sign up for GitHub

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

mydea
merged 5 commits into

from

Jan 9, 2025
 Merged


feat!: Remove spanId from propagation context
#14733
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


5 commits


Select commit
Hold shift + click to select a range
e761fd3

feat!: Remove `spanId` from propagation context


mydea Jan 7, 2025
80182b5

fixes...


mydea Jan 7, 2025
dc4e1b4

fix circular dep check


mydea Jan 8, 2025
7cb9c86

add comments & tests


mydea Jan 8, 2025
a57be3a

Update packages/core/src/types-hoist/tracing.ts


mydea Jan 8, 2025

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


1 change: 0 additions & 1 deletion
...ration-tests/suites/public-api/startSpan/parallel-root-spans-with-parentSpanId/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
Sentry.getCurrentScope().setPropagationContext({
 parentSpanId: '1234567890123456',
 spanId: '123456789012345x',
 traceId: '12345678901234567890123456789012',
});

Expand Down

12 changes: 12 additions & 0 deletions
...rowser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/init.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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,
});

2 changes: 2 additions & 0 deletions
...ser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
Sentry.captureException(new Error('test error'));
Sentry.captureException(new Error('test error 2'));

11 changes: 11 additions & 0 deletions
...-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/template.html

View file


Open in desktop

Original file line numberDiff line numberDiff 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>

35 changes: 35 additions & 0 deletions
...rowser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff 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);
});

12 changes: 12 additions & 0 deletions
...ges/browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/init.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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,
});

2 changes: 2 additions & 0 deletions
.../browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
Sentry.captureException(new Error('test error'));
Sentry.captureException(new Error('test error 2'));

30 changes: 30 additions & 0 deletions
...ges/browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff 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);
});

1 change: 0 additions & 1 deletion
...ckages/node-integration-tests/suites/public-api/startSpan/parallel-root-spans/scenario.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -10,7 +10,6 @@ Sentry.init({

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

Expand Down

1 change: 0 additions & 1 deletion
...n-tests/suites/public-api/startSpan/parallel-spans-in-scope-with-parentSpanId/scenario.ts

View file


Open in desktop

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

Expand Down

13 changes: 11 additions & 2 deletions
dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff 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

@Lms24Lms24Dec 17, 2024

Copy link


Copy Markdown

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.
@mydeamydeaDec 18, 2024

Copy link


Copy Markdown

MemberAuthor
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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand Up@@ -166,7 +166,6 @@ export class Scope {
 this._sdkProcessingMetadata = {};
 this._propagationContext = {
 traceId: generateTraceId(),
 spanId: generateSpanId(),
 };
 }

Expand DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

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.

 Sign up for GitHub

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

mydea
merged 5 commits into

from

Jan 9, 2025
 Merged


feat!: Remove spanId from propagation context
#14733
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


5 commits


Select commit
Hold shift + click to select a range
e761fd3

feat!: Remove `spanId` from propagation context


mydea Jan 7, 2025
80182b5

fixes...


mydea Jan 7, 2025
dc4e1b4

fix circular dep check


mydea Jan 8, 2025
7cb9c86

add comments & tests


mydea Jan 8, 2025
a57be3a

Update packages/core/src/types-hoist/tracing.ts


mydea Jan 8, 2025

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


1 change: 0 additions & 1 deletion
...ration-tests/suites/public-api/startSpan/parallel-root-spans-with-parentSpanId/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
Sentry.getCurrentScope().setPropagationContext({
 parentSpanId: '1234567890123456',
 spanId: '123456789012345x',
 traceId: '12345678901234567890123456789012',
});

Expand Down

12 changes: 12 additions & 0 deletions
...rowser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/init.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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,
});

2 changes: 2 additions & 0 deletions
...ser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
Sentry.captureException(new Error('test error'));
Sentry.captureException(new Error('test error 2'));

11 changes: 11 additions & 0 deletions
...-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/template.html

View file


Open in desktop

Original file line numberDiff line numberDiff 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>

35 changes: 35 additions & 0 deletions
...rowser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff 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);
});

12 changes: 12 additions & 0 deletions
...ges/browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/init.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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,
});

2 changes: 2 additions & 0 deletions
.../browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
Sentry.captureException(new Error('test error'));
Sentry.captureException(new Error('test error 2'));

30 changes: 30 additions & 0 deletions
...ges/browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff 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);
});

1 change: 0 additions & 1 deletion
...ckages/node-integration-tests/suites/public-api/startSpan/parallel-root-spans/scenario.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -10,7 +10,6 @@ Sentry.init({

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

Expand Down

1 change: 0 additions & 1 deletion
...n-tests/suites/public-api/startSpan/parallel-spans-in-scope-with-parentSpanId/scenario.ts

View file


Open in desktop

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

Expand Down

13 changes: 11 additions & 2 deletions
dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff 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

@Lms24Lms24Dec 17, 2024

Copy link


Copy Markdown

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.
@mydeamydeaDec 18, 2024

Copy link


Copy Markdown

MemberAuthor
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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand Up@@ -166,7 +166,6 @@ export class Scope {
 this._sdkProcessingMetadata = {};
 this._propagationContext = {
 traceId: generateTraceId(),
 spanId: generateSpanId(),
 };
 }

Expand DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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 DownExpand 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

View file


Open in desktop

Original file line numberDiff line numberDiff 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