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

Author: mydea

dev-packages/node-integration-tests/suites/aws-serverless/aws-integration/s3/test.ts

@@ -26,6 +26,6 @@ describe('awsIntegration', () => {
   });
 
   test('should auto-instrument aws-sdk v2 package.', done => {
-    createRunner(__dirname, 'scenario.js').expect({ transaction: EXPECTED_TRANSCATION }).start(done);
+    createRunner(__dirname, 'scenario.js').ignore('event').expect({ transaction: EXPECTED_TRANSCATION }).start(done);
   });
 });

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

@@ -15,6 +15,18 @@ Sentry.withScope(scope => {
     traceId: '12345678901234567890123456789012',
   });
 
-  Sentry.startSpan({ name: 'test_span_1' }, () => undefined);
-  Sentry.startSpan({ name: 'test_span_2' }, () => undefined);
+  const spanIdTraceId = Sentry.startSpan(
+    {
+      name: 'test_span_1',
+    },
+    span1 => span1.spanContext().traceId,
+  );
+
+  Sentry.startSpan(
+    {
+      name: 'test_span_2',
+      attributes: { spanIdTraceId },
+    },
+    () => undefined,
+  );
 });

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

@@ -6,28 +6,19 @@ afterAll(() => {
 
 test('should send manually started parallel root spans outside of root context with parentSpanId', done => {
   createRunner(__dirname, 'scenario.ts')
+    .expect({ transaction: { transaction: 'test_span_1' } })
     .expect({
-      transaction: {
-        transaction: 'test_span_1',
-        contexts: {
-          trace: {
-            span_id: expect.any(String),
-            parent_span_id: '1234567890123456',
-            trace_id: '12345678901234567890123456789012',
-          },
-        },
-      },
-    })
-    .expect({
-      transaction: {
-        transaction: 'test_span_2',
-        contexts: {
-          trace: {
-            span_id: expect.any(String),
-            parent_span_id: '1234567890123456',
-            trace_id: '12345678901234567890123456789012',
-          },
-        },
+      transaction: transaction => {
+        expect(transaction).toBeDefined();
+        const traceId = transaction.contexts?.trace?.trace_id;
+        expect(traceId).toBeDefined();
+        expect(transaction.contexts?.trace?.parent_span_id).toBeUndefined();
+
+        const trace1Id = transaction.contexts?.trace?.data?.spanIdTraceId;
+        expect(trace1Id).toBeDefined();
+
+        // Different trace ID as the first span
+        expect(trace1Id).not.toBe(traceId);
       },
     })
     .start(done);

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

@@ -19,8 +19,8 @@ test('should send manually started parallel root spans outside of root context',
         const trace1Id = transaction.contexts?.trace?.data?.spanIdTraceId;
         expect(trace1Id).toBeDefined();
 
-        // Same trace ID as the first span
-        expect(trace1Id).toBe(traceId);
+        // Different trace ID as the first span
+        expect(trace1Id).not.toBe(traceId);
       },
     })
     .start(done);

packages/opentelemetry/src/constants.ts

@@ -4,7 +4,6 @@ export const SENTRY_TRACE_HEADER = 'sentry-trace';
 export const SENTRY_BAGGAGE_HEADER = 'baggage';
 
 export const SENTRY_TRACE_STATE_DSC = 'sentry.dsc';
-export const SENTRY_TRACE_STATE_PARENT_SPAN_ID = 'sentry.parent_span_id';
 export const SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING = 'sentry.sampled_not_recording';
 export const SENTRY_TRACE_STATE_URL = 'sentry.url';
 

packages/opentelemetry/src/propagator.ts

@@ -26,14 +26,14 @@ import {
   SENTRY_BAGGAGE_HEADER,
   SENTRY_TRACE_HEADER,
   SENTRY_TRACE_STATE_DSC,
-  SENTRY_TRACE_STATE_PARENT_SPAN_ID,
   SENTRY_TRACE_STATE_URL,
 } from './constants';
 import { DEBUG_BUILD } from './debug-build';
 import { getScopesFromContext, setScopesOnContext } from './utils/contextData';
 import { getSamplingDecision } from './utils/getSamplingDecision';
 import { makeTraceState } from './utils/makeTraceState';
 import { setIsSetup } from './utils/setupCheck';
+import { spanHasParentId } from './utils/spanTypes';
 
 /** Get the Sentry propagation context from a span context. */
 export function getPropagationContextFromSpan(span: Span): PropagationContext {
@@ -45,8 +45,7 @@ export function getPropagationContextFromSpan(span: Span): PropagationContext {
   const dscString = traceState ? traceState.get(SENTRY_TRACE_STATE_DSC) : undefined;
   const traceStateDsc = dscString ? baggageHeaderToDynamicSamplingContext(dscString) : undefined;
 
-  const parentSpanId = traceState ? traceState.get(SENTRY_TRACE_STATE_PARENT_SPAN_ID) || undefined : undefined;
-
+  const parentSpanId = spanHasParentId(span) ? span.parentSpanId : undefined;
   const sampled = getSamplingDecision(spanContext);
 
   // No trace state? --> Take DSC from root span
@@ -244,7 +243,15 @@ function getContextWithRemoteActiveSpan(
 ): Context {
   const propagationContext = propagationContextFromHeaders(sentryTrace, baggage);
 
-  const spanContext = generateSpanContextForPropagationContext(propagationContext);
+  const { traceId, parentSpanId, sampled, dsc } = propagationContext;
+
+  // We only want to set the virtual span if we are continuing a concrete trace
+  // Otherwise, we ignore the propagation context
+  if (!parentSpanId) {
+    return ctx;
+  }
+
+  const spanContext = generateSpanContextForPropagationContext({ traceId, parentSpanId, sampled, dsc });
   return trace.setSpanContext(ctx, spanContext);
 }
 
@@ -299,20 +306,28 @@ function getCurrentURL(span: Span): string | undefined {
   return undefined;
 }
 
-// TODO: Adjust this behavior to avoid invalid spans
-function generateSpanContextForPropagationContext(propagationContext: PropagationContext): SpanContext {
+function generateSpanContextForPropagationContext({
+  parentSpanId,
+  traceId,
+  sampled,
+  dsc,
+}: {
+  parentSpanId: string;
+  traceId: string;
+  sampled: boolean | undefined;
+  dsc?: Partial<DynamicSamplingContext>;
+}): SpanContext {
   // We store the DSC as OTEL trace state on the span context
   const traceState = makeTraceState({
-    parentSpanId: propagationContext.parentSpanId,
-    dsc: propagationContext.dsc,
-    sampled: propagationContext.sampled,
+    dsc,
+    sampled,
   });
 
   const spanContext: SpanContext = {
-    traceId: propagationContext.traceId,
-    spanId: propagationContext.parentSpanId || '',
+    traceId,
+    spanId: parentSpanId,
     isRemote: true,
-    traceFlags: propagationContext.sampled ? TraceFlags.SAMPLED : TraceFlags.NONE,
+    traceFlags: sampled ? TraceFlags.SAMPLED : TraceFlags.NONE,
     traceState,
   };
 

packages/opentelemetry/src/setupEventContextTrace.ts

@@ -1,9 +1,6 @@
-import { getDynamicSamplingContextFromSpan, getRootSpan } from '@sentry/core';
-import { dropUndefinedKeys } from '@sentry/core';
+import { getDynamicSamplingContextFromSpan, getRootSpan, spanToTraceContext } from '@sentry/core';
 import type { Client } from '@sentry/types';
-import { SENTRY_TRACE_STATE_PARENT_SPAN_ID } from './constants';
 import { getActiveSpan } from './utils/getActiveSpan';
-import { spanHasParentId } from './utils/spanTypes';
 
 /** Ensure the `trace` context is set on all events. */
 export function setupEventContextTrace(client: Client): void {
@@ -15,25 +12,9 @@ export function setupEventContextTrace(client: Client): void {
       return;
     }
 
-    const spanContext = span.spanContext();
-
-    // If we have a parent span id from trace state, use that ('' means no parent should be used)
-    // Else, pick the one from the span
-    const parentSpanIdFromTraceState = spanContext.traceState?.get(SENTRY_TRACE_STATE_PARENT_SPAN_ID);
-    const parent_span_id =
-      typeof parentSpanIdFromTraceState === 'string'
-        ? parentSpanIdFromTraceState || undefined
-        : spanHasParentId(span)
-          ? span.parentSpanId
-          : undefined;
-
     // If event has already set `trace` context, use that one.
     event.contexts = {
-      trace: dropUndefinedKeys({
-        trace_id: spanContext.traceId,
-        span_id: spanContext.spanId,
-        parent_span_id,
-      }),
+      trace: spanToTraceContext(span),
       ...event.contexts,
     };
 

packages/opentelemetry/src/spanExporter.ts

@@ -19,7 +19,6 @@ import {
 } from '@sentry/core';
 import { dropUndefinedKeys, logger } from '@sentry/core';
 import type { SpanJSON, SpanOrigin, TraceContext, TransactionEvent, TransactionSource } from '@sentry/types';
-import { SENTRY_TRACE_STATE_PARENT_SPAN_ID } from './constants';
 
 import { DEBUG_BUILD } from './debug-build';
 import { SEMANTIC_ATTRIBUTE_SENTRY_PARENT_IS_REMOTE } from './semanticAttributes';
@@ -242,15 +241,12 @@ function createTransactionForOtelSpan(span: ReadableSpan): TransactionEvent {
 
   const { traceId: trace_id, spanId: span_id } = span.spanContext();
 
-  const parentSpanIdFromTraceState = span.spanContext().traceState?.get(SENTRY_TRACE_STATE_PARENT_SPAN_ID);
-
   // If parentSpanIdFromTraceState is defined at all, we want it to take precedence
   // In that case, an empty string should be interpreted as "no parent span id",
   // even if `span.parentSpanId` is set
   // this is the case when we are starting a new trace, where we have a virtual span based on the propagationContext
   // We only want to continue the traceId in this case, but ignore the parent span
-  const parent_span_id =
-    typeof parentSpanIdFromTraceState === 'string' ? parentSpanIdFromTraceState || undefined : span.parentSpanId;
+  const parent_span_id = span.parentSpanId;
 
   const status = mapStatus(span);
 

packages/opentelemetry/src/trace.ts

@@ -1,5 +1,5 @@
 import type { Context, Span, SpanContext, SpanOptions, Tracer } from '@opentelemetry/api';
-import { INVALID_SPANID, SpanStatusCode, TraceFlags, context, trace } from '@opentelemetry/api';
+import { SpanStatusCode, TraceFlags, context, trace } from '@opentelemetry/api';
 import { suppressTracing } from '@opentelemetry/core';
 import {
   SDK_VERSION,
@@ -19,7 +19,7 @@ import type { Client, DynamicSamplingContext, Scope, Span as SentrySpan, TraceCo
 import { continueTraceAsRemoteSpan } from './propagator';
 
 import type { OpenTelemetryClient, OpenTelemetrySpanContext } from './types';
-import { getContextFromScope, getScopesFromContext } from './utils/contextData';
+import { getContextFromScope } from './utils/contextData';
 import { getSamplingDecision } from './utils/getSamplingDecision';
 import { makeTraceState } from './utils/makeTraceState';
 
@@ -179,40 +179,11 @@ function ensureTimestampInMilliseconds(timestamp: number): number {
 
 function getContext(scope: Scope | undefined, forceTransaction: boolean | undefined): Context {
   const ctx = getContextForScope(scope);
-  // Note: If the context is the ROOT_CONTEXT, no scope is attached
-  // Thus we will not use the propagation context in this case, which is desired
-  const actualScope = getScopesFromContext(ctx)?.scope;
   const parentSpan = trace.getSpan(ctx);
 
-  // In the case that we have no parent span, we need to "simulate" one to ensure the propagation context is correct
+  // In the case that we have no parent span, we start a new trace
+  // Note that if we continue a trace, we'll always have a remote parent span here anyhow
   if (!parentSpan) {
-    const client = getClient();
-
-    if (actualScope && client) {
-      const propagationContext = actualScope.getPropagationContext();
-
-      // We store the DSC as OTEL trace state on the span context
-      const traceState = makeTraceState({
-        parentSpanId: propagationContext.parentSpanId,
-        // Not defined yet, we want to pick this up on-demand only
-        dsc: undefined,
-        sampled: propagationContext.sampled,
-      });
-
-      const spanOptions: SpanContext = {
-        traceId: propagationContext.traceId,
-        // eslint-disable-next-line deprecation/deprecation
-        spanId: propagationContext.parentSpanId || propagationContext.spanId,
-        isRemote: true,
-        traceFlags: propagationContext.sampled ? TraceFlags.SAMPLED : TraceFlags.NONE,
-        traceState,
-      };
-
-      // Add remote parent span context,
-      return trace.setSpanContext(ctx, spanOptions);
-    }
-
-    // if we have no scope or client, we just return the context as-is
     return ctx;
   }
 
@@ -238,7 +209,6 @@ function getContext(scope: Scope | undefined, forceTransaction: boolean | undefi
 
   const traceState = makeTraceState({
     dsc,
-    parentSpanId: spanId !== INVALID_SPANID ? spanId : undefined,
     sampled,
   });
 

packages/opentelemetry/src/utils/generateSpanContextForPropagationContext.ts

@@ -12,7 +12,6 @@ import { makeTraceState } from './makeTraceState';
 export function generateSpanContextForPropagationContext(propagationContext: PropagationContext): SpanContext {
   // We store the DSC as OTEL trace state on the span context
   const traceState = makeTraceState({
-    parentSpanId: propagationContext.parentSpanId,
     dsc: propagationContext.dsc,
     sampled: propagationContext.sampled,
   });

packages/opentelemetry/src/utils/makeTraceState.ts

@@ -1,32 +1,22 @@
 import { TraceState } from '@opentelemetry/core';
 import { dynamicSamplingContextToSentryBaggageHeader } from '@sentry/core';
 import type { DynamicSamplingContext } from '@sentry/types';
-import {
-  SENTRY_TRACE_STATE_DSC,
-  SENTRY_TRACE_STATE_PARENT_SPAN_ID,
-  SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING,
-} from '../constants';
+import { SENTRY_TRACE_STATE_DSC, SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING } from '../constants';
 
 /**
  * Generate a TraceState for the given data.
  */
 export function makeTraceState({
-  parentSpanId,
   dsc,
   sampled,
 }: {
-  parentSpanId?: string;
   dsc?: Partial<DynamicSamplingContext>;
   sampled?: boolean;
 }): TraceState {
   // We store the DSC as OTEL trace state on the span context
   const dscString = dsc ? dynamicSamplingContextToSentryBaggageHeader(dsc) : undefined;
 
-  // We _always_ set the parent span ID, even if it is empty
-  // If we'd set this to 'undefined' we could not know if the trace state was set, but there was no parentSpanId,
-  // vs the trace state was not set at all (in which case we want to do fallback handling)
-  // If `''`, it should be considered "no parent"
-  const traceStateBase = new TraceState().set(SENTRY_TRACE_STATE_PARENT_SPAN_ID, parentSpanId || '');
+  const traceStateBase = new TraceState();
 
   const traceStateWithDsc = dscString ? traceStateBase.set(SENTRY_TRACE_STATE_DSC, dscString) : traceStateBase;
 

packages/opentelemetry/test/integration/transactions.test.ts

@@ -327,7 +327,6 @@ describe('Integration | Transactions', () => {
     const parentSpanId = '6e0c63257de34c92';
 
     const traceState = makeTraceState({
-      parentSpanId,
       dsc: undefined,
       sampled: true,
     });