feat!: Remove spanId from propagation context (#14733)

Closes #12385 This also deprecates `getPropagationContextFromSpan` as it is no longer used/needed. We may think about removing this in v9, but IMHO we can also just leave this for v9, it does not hurt too much to have it in there... --------- Co-authored-by: Luca Forstner <[email protected]>
getsentry · Jan 9, 2025 · d5af638 · d5af638
1 parent d4e94fe
commit d5af638
Show file tree

Hide file tree

Showing 38 changed files with 190 additions and 401 deletions.
diff --git a/...ration-tests/suites/public-api/startSpan/parallel-root-spans-with-parentSpanId/subject.js b/...ration-tests/suites/public-api/startSpan/parallel-root-spans-with-parentSpanId/subject.js
@@ -1,6 +1,5 @@
 Sentry.getCurrentScope().setPropagationContext({
   parentSpanId: '1234567890123456',
-  spanId: '123456789012345x',
   traceId: '12345678901234567890123456789012',
 });
 

diff --git a/...rowser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/init.js b/...rowser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/init.js
@@ -0,0 +1,12 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://[email protected]/1337',
+  integrations: integrations => {
+    integrations.push(Sentry.browserTracingIntegration());
+    return integrations.filter(i => i.name !== 'BrowserSession');
+  },
+  tracesSampleRate: 0,
+});
diff --git a/...ser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/subject.js b/...ser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/subject.js
@@ -0,0 +1,2 @@
+Sentry.captureException(new Error('test error'));
+Sentry.captureException(new Error('test error 2'));
diff --git a/...-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/template.html b/...-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/template.html
@@ -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>
diff --git a/...rowser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/test.ts b/...rowser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors-meta/test.ts
@@ -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);
+});
diff --git a/...ges/browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/init.js b/...ges/browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/init.js
@@ -0,0 +1,12 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://[email protected]/1337',
+  integrations: integrations => {
+    integrations.push(Sentry.browserTracingIntegration());
+    return integrations.filter(i => i.name !== 'BrowserSession');
+  },
+  tracesSampleRate: 0,
+});
diff --git a/.../browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/subject.js b/.../browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/subject.js
@@ -0,0 +1,2 @@
+Sentry.captureException(new Error('test error'));
+Sentry.captureException(new Error('test error 2'));
diff --git a/...ges/browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/test.ts b/...ges/browser-integration-tests/suites/tracing/browserTracingIntegration/twp-errors/test.ts
@@ -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);
+});
diff --git a/...ckages/node-integration-tests/suites/public-api/startSpan/parallel-root-spans/scenario.ts b/...ckages/node-integration-tests/suites/public-api/startSpan/parallel-root-spans/scenario.ts
@@ -10,7 +10,6 @@ Sentry.init({
 
 Sentry.getCurrentScope().setPropagationContext({
   parentSpanId: '1234567890123456',
-  spanId: '123456789012345x',
   traceId: '12345678901234567890123456789012',
 });
 

diff --git a/...n-tests/suites/public-api/startSpan/parallel-spans-in-scope-with-parentSpanId/scenario.ts b/...n-tests/suites/public-api/startSpan/parallel-spans-in-scope-with-parentSpanId/scenario.ts
@@ -11,7 +11,6 @@ Sentry.init({
 Sentry.withScope(scope => {
   scope.setPropagationContext({
     parentSpanId: '1234567890123456',
-    spanId: '123456789012345x',
     traceId: '12345678901234567890123456789012',
   });
 

diff --git a/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts b/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts
@@ -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({
@@ -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({
@@ -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);
           expect(traceData.metaTags).toContain(`sentry-trace_id=${trace_id}`);
           expect(traceData.metaTags).not.toContain('sentry-sampled=');
         },

diff --git a/packages/browser/test/tracing/browserTracingIntegration.test.ts b/packages/browser/test/tracing/browserTracingIntegration.test.ts
@@ -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}/),
       });
 
@@ -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: {
@@ -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: {

diff --git a/packages/core/src/currentScopes.ts b/packages/core/src/currentScopes.ts
@@ -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';
 
 /**
@@ -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,
   });
 

diff --git a/packages/core/src/scope.ts b/packages/core/src/scope.ts
@@ -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';
@@ -166,7 +166,6 @@ export class Scope {
     this._sdkProcessingMetadata = {};
     this._propagationContext = {
       traceId: generateTraceId(),
-      spanId: generateSpanId(),
     };
   }
 
@@ -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;
   }
 

diff --git a/packages/core/src/types-hoist/tracing.ts b/packages/core/src/types-hoist/tracing.ts
@@ -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.
    *

diff --git a/packages/core/src/utils-hoist/propagationContext.ts b/packages/core/src/utils-hoist/propagationContext.ts
@@ -9,7 +9,6 @@ import { uuid4 } from './misc';
 export function generatePropagationContext(): PropagationContext {
   return {
     traceId: generateTraceId(),
-    spanId: generateSpanId(),
   };
 }
 

diff --git a/packages/core/src/utils-hoist/tracing.ts b/packages/core/src/utils-hoist/tracing.ts
@@ -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
   };
@@ -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 = '';

diff --git a/packages/core/src/utils/spanUtils.ts b/packages/core/src/utils/spanUtils.ts
@@ -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,
@@ -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,

diff --git a/packages/core/src/utils/traceData.ts b/packages/core/src/utils/traceData.ts
@@ -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);
 }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		Sentry.captureException(new Error('test error'));
		Sentry.captureException(new Error('test error 2'));