fix(node): UnifygetDynamicSamplingContextFromSpan (#12522)

mydea · web-flow · commit 9724ec15e570 · 2024-06-19T09:22:15.000Z
We used to have a different implementation, that worked slightly different. This had two problems: 1. We did not look up the root span in the OTEL implementation, but relied on already passing in the root span (in core, we convert it to root span) 2. We did not use frozen DSC properly Now, the core implementation just works for both OTEL and core spans. While at this, it also aligns the behavior of looking at the `source` of a span for DSC. Now, only if the source is `url` we'll _not_ set the transaction on the DSC - previously, if no source was set at all, we'd also skip this. But conceptually "no source" is similar to "custom", which we _do_ allow, so now this is a bit simpler. Fixes #12508
diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts
@@ -414,7 +414,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   public on(hook: 'beforeAddBreadcrumb', callback: (breadcrumb: Breadcrumb, hint?: BreadcrumbHint) => void): void;
 
   /** @inheritdoc */
-  public on(hook: 'createDsc', callback: (dsc: DynamicSamplingContext) => void): void;
+  public on(hook: 'createDsc', callback: (dsc: DynamicSamplingContext, rootSpan?: Span) => void): void;
 
   /** @inheritdoc */
   public on(
@@ -499,7 +499,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   public emit(hook: 'beforeAddBreadcrumb', breadcrumb: Breadcrumb, hint?: BreadcrumbHint): void;
 
   /** @inheritdoc */
-  public emit(hook: 'createDsc', dsc: DynamicSamplingContext): void;
+  public emit(hook: 'createDsc', dsc: DynamicSamplingContext, rootSpan?: Span): void;
 
   /** @inheritdoc */
   public emit(hook: 'beforeSendFeedback', feedback: FeedbackEvent, options?: { includeReplay: boolean }): void;
diff --git a/packages/core/src/tracing/dynamicSamplingContext.ts b/packages/core/src/tracing/dynamicSamplingContext.ts
@@ -1,6 +1,7 @@
 import type { Client, DynamicSamplingContext, Span } from '@sentry/types';
 import {
   addNonEnumerableProperty,
+  baggageHeaderToDynamicSamplingContext,
   dropUndefinedKeys,
   dynamicSamplingContextToSentryBaggageHeader,
 } from '@sentry/utils';
@@ -66,15 +67,25 @@ export function getDynamicSamplingContextFromSpan(span: Span): Readonly<Partial<
   const dsc = getDynamicSamplingContextFromClient(spanToJSON(span).trace_id || '', client);
 
   const rootSpan = getRootSpan(span);
-  if (!rootSpan) {
-    return dsc;
-  }
 
+  // For core implementation, we freeze the DSC onto the span as a non-enumerable property
   const frozenDsc = (rootSpan as SpanWithMaybeDsc)[FROZEN_DSC_FIELD];
   if (frozenDsc) {
     return frozenDsc;
   }
 
+  // For OpenTelemetry, we freeze the DSC on the trace state
+  const traceState = rootSpan.spanContext().traceState;
+  const traceStateDsc = traceState && traceState.get('sentry.dsc');
+
+  // If the span has a DSC, we want it to take precedence
+  const dscOnTraceState = traceStateDsc && baggageHeaderToDynamicSamplingContext(traceStateDsc);
+
+  if (dscOnTraceState) {
+    return dscOnTraceState;
+  }
+
+  // Else, we generate it from the span
   const jsonSpan = spanToJSON(rootSpan);
   const attributes = jsonSpan.data || {};
   const maybeSampleRate = attributes[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE];
@@ -87,13 +98,14 @@ export function getDynamicSamplingContextFromSpan(span: Span): Readonly<Partial<
   const source = attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE];
 
   // after JSON conversion, txn.name becomes jsonSpan.description
-  if (source && source !== 'url') {
-    dsc.transaction = jsonSpan.description;
+  const name = jsonSpan.description;
+  if (source !== 'url' && name) {
+    dsc.transaction = name;
   }
 
   dsc.sampled = String(spanIsSampled(rootSpan));
 
-  client.emit('createDsc', dsc);
+  client.emit('createDsc', dsc, rootSpan);
 
   return dsc;
 }
diff --git a/packages/core/test/lib/tracing/dynamicSamplingContext.test.ts b/packages/core/test/lib/tracing/dynamicSamplingContext.test.ts
@@ -1,4 +1,4 @@
-import type { TransactionSource } from '@sentry/types';
+import type { Span, SpanContextData, TransactionSource } from '@sentry/types';
 import {
   SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE,
   SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
@@ -33,6 +33,27 @@ describe('getDynamicSamplingContextFromSpan', () => {
     expect(dynamicSamplingContext).toStrictEqual({ environment: 'myEnv' });
   });
 
+  test('uses frozen DSC from traceState', () => {
+    const rootSpan = {
+      spanContext() {
+        return {
+          traceId: '1234',
+          spanId: '12345',
+          traceFlags: 0,
+          traceState: {
+            get() {
+              return 'sentry-environment=myEnv2';
+            },
+          } as unknown as SpanContextData['traceState'],
+        };
+      },
+    } as Span;
+
+    const dynamicSamplingContext = getDynamicSamplingContextFromSpan(rootSpan);
+
+    expect(dynamicSamplingContext).toStrictEqual({ environment: 'myEnv2' });
+  });
+
   test('returns a new DSC, if no DSC was provided during rootSpan creation (via attributes)', () => {
     const rootSpan = startInactiveSpan({ name: 'tx' });
 
diff --git a/packages/node/src/sdk/index.ts b/packages/node/src/sdk/index.ts
@@ -12,6 +12,7 @@ import {
   startSession,
 } from '@sentry/core';
 import {
+  enhanceDscWithOpenTelemetryRootSpanName,
   openTelemetrySetupCheck,
   setOpenTelemetryContextAsyncContextStrategy,
   setupEventContextTrace,
@@ -175,6 +176,7 @@ function _init(
     validateOpenTelemetrySetup();
   }
 
+  enhanceDscWithOpenTelemetryRootSpanName(client);
   setupEventContextTrace(client);
 }
 
diff --git a/packages/opentelemetry/src/index.ts b/packages/opentelemetry/src/index.ts
@@ -16,10 +16,13 @@ export {
   spanHasStatus,
 } from './utils/spanTypes';
 
-export { getDynamicSamplingContextFromSpan } from './utils/dynamicSamplingContext';
+// Re-export this for backwards compatibility (this used to be a different implementation)
+export { getDynamicSamplingContextFromSpan } from '@sentry/core';
 
 export { isSentryRequestSpan } from './utils/isSentryRequest';
 
+export { enhanceDscWithOpenTelemetryRootSpanName } from './utils/enhanceDscWithOpenTelemetryRootSpanName';
+
 export { getActiveSpan } from './utils/getActiveSpan';
 export { startSpan, startSpanManual, startInactiveSpan, withActiveSpan, continueTrace } from './trace';
 
diff --git a/packages/opentelemetry/src/propagator.ts b/packages/opentelemetry/src/propagator.ts
@@ -8,7 +8,13 @@ import type { continueTrace } from '@sentry/core';
 import { hasTracingEnabled } from '@sentry/core';
 import { getRootSpan } from '@sentry/core';
 import { spanToJSON } from '@sentry/core';
-import { getClient, getCurrentScope, getDynamicSamplingContextFromClient, getIsolationScope } from '@sentry/core';
+import {
+  getClient,
+  getCurrentScope,
+  getDynamicSamplingContextFromClient,
+  getDynamicSamplingContextFromSpan,
+  getIsolationScope,
+} from '@sentry/core';
 import type { DynamicSamplingContext, Options, PropagationContext } from '@sentry/types';
 import {
   LRUMap,
@@ -32,7 +38,6 @@ import {
 } from './constants';
 import { DEBUG_BUILD } from './debug-build';
 import { getScopesFromContext, setScopesOnContext } from './utils/contextData';
-import { getDynamicSamplingContextFromSpan } from './utils/dynamicSamplingContext';
 import { getSamplingDecision } from './utils/getSamplingDecision';
 import { setIsSetup } from './utils/setupCheck';
 
diff --git a/packages/opentelemetry/src/setupEventContextTrace.ts b/packages/opentelemetry/src/setupEventContextTrace.ts
@@ -1,8 +1,6 @@
+import { getDynamicSamplingContextFromSpan, getRootSpan } from '@sentry/core';
 import type { Client } from '@sentry/types';
 import { dropUndefinedKeys } from '@sentry/utils';
-import { getDynamicSamplingContextFromSpan } from './utils/dynamicSamplingContext';
-
-import { getRootSpan } from '@sentry/core';
 import { SENTRY_TRACE_STATE_PARENT_SPAN_ID } from './constants';
 import { getActiveSpan } from './utils/getActiveSpan';
 import { spanHasParentId } from './utils/spanTypes';
diff --git a/packages/opentelemetry/src/spanExporter.ts b/packages/opentelemetry/src/spanExporter.ts
@@ -5,6 +5,7 @@ import { SEMATTRS_HTTP_STATUS_CODE } from '@opentelemetry/semantic-conventions';
 import {
   captureEvent,
   getCapturedScopesOnSpan,
+  getDynamicSamplingContextFromSpan,
   getMetricSummaryJsonForSpan,
   timedEventsToMeasurements,
 } from '@sentry/core';
@@ -22,7 +23,6 @@ import { SENTRY_TRACE_STATE_PARENT_SPAN_ID } from './constants';
 import { DEBUG_BUILD } from './debug-build';
 import { SEMANTIC_ATTRIBUTE_SENTRY_PARENT_IS_REMOTE } from './semanticAttributes';
 import { convertOtelTimeToSeconds } from './utils/convertOtelTimeToSeconds';
-import { getDynamicSamplingContextFromSpan } from './utils/dynamicSamplingContext';
 import { getRequestSpanData } from './utils/getRequestSpanData';
 import type { SpanNode } from './utils/groupSpansWithParents';
 import { getLocalParentId } from './utils/groupSpansWithParents';
@@ -242,7 +242,7 @@ function createTransactionForOtelSpan(span: ReadableSpan): TransactionEvent {
         capturedSpanScope: capturedSpanScopes.scope,
         capturedSpanIsolationScope: capturedSpanScopes.isolationScope,
         sampleRate,
-        dynamicSamplingContext: getDynamicSamplingContextFromSpan(span),
+        dynamicSamplingContext: getDynamicSamplingContextFromSpan(span as unknown as Span),
       }),
     },
     ...(source && {
diff --git a/packages/opentelemetry/src/trace.ts b/packages/opentelemetry/src/trace.ts
@@ -7,6 +7,7 @@ import {
   continueTrace as baseContinueTrace,
   getClient,
   getCurrentScope,
+  getDynamicSamplingContextFromSpan,
   getRootSpan,
   handleCallbackErrors,
   spanToJSON,
@@ -16,7 +17,6 @@ import { continueTraceAsRemoteSpan, makeTraceState } from './propagator';
 
 import type { OpenTelemetryClient, OpenTelemetrySpanContext } from './types';
 import { getContextFromScope, getScopesFromContext } from './utils/contextData';
-import { getDynamicSamplingContextFromSpan } from './utils/dynamicSamplingContext';
 import { getSamplingDecision } from './utils/getSamplingDecision';
 
 /**
diff --git a/packages/opentelemetry/src/utils/dynamicSamplingContext.ts b/packages/opentelemetry/src/utils/dynamicSamplingContext.ts
diff --git a/packages/opentelemetry/src/utils/enhanceDscWithOpenTelemetryRootSpanName.ts b/packages/opentelemetry/src/utils/enhanceDscWithOpenTelemetryRootSpanName.ts
@@ -0,0 +1,28 @@
+import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, spanToJSON } from '@sentry/core';
+import type { Client } from '@sentry/types';
+import { parseSpanDescription } from './parseSpanDescription';
+import { spanHasName } from './spanTypes';
+
+/**
+ * Setup a DSC handler on the passed client,
+ * ensuring that the transaction name is inferred from the span correctly.
+ */
+export function enhanceDscWithOpenTelemetryRootSpanName(client: Client): void {
+  client.on('createDsc', (dsc, rootSpan) => {
+    // We want to overwrite the transaction on the DSC that is created by default in core
+    // The reason for this is that we want to infer the span name, not use the initial one
+    // Otherwise, we'll get names like "GET" instead of e.g. "GET /foo"
+    // `parseSpanDescription` takes the attributes of the span into account for the name
+    // This mutates the passed-in DSC
+    if (rootSpan) {
+      const jsonSpan = spanToJSON(rootSpan);
+      const attributes = jsonSpan.data || {};
+      const source = attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE];
+
+      const { description } = spanHasName(rootSpan) ? parseSpanDescription(rootSpan) : { description: undefined };
+      if (source !== 'url' && description) {
+        dsc.transaction = description;
+      }
+    }
+  });
+}
diff --git a/packages/opentelemetry/test/trace.test.ts b/packages/opentelemetry/test/trace.test.ts
@@ -12,6 +12,7 @@ import {
   getClient,
   getCurrentScope,
   getDynamicSamplingContextFromClient,
+  getDynamicSamplingContextFromSpan,
   getRootSpan,
   spanIsSampled,
   spanToJSON,
@@ -24,7 +25,6 @@ import { makeTraceState } from '../src/propagator';
 import { SEMATTRS_HTTP_METHOD } from '@opentelemetry/semantic-conventions';
 import { continueTrace, startInactiveSpan, startSpan, startSpanManual } from '../src/trace';
 import type { AbstractSpan } from '../src/types';
-import { getDynamicSamplingContextFromSpan } from '../src/utils/dynamicSamplingContext';
 import { getActiveSpan } from '../src/utils/getActiveSpan';
 import { getSamplingDecision } from '../src/utils/getSamplingDecision';
 import { getSpanKind } from '../src/utils/getSpanKind';
@@ -983,24 +983,16 @@ describe('trace', () => {
       withScope(scope => {
         const propagationContext = scope.getPropagationContext();
 
-        const ctx = trace.setSpanContext(ROOT_CONTEXT, {
-          traceId: '12312012123120121231201212312012',
-          spanId: '1121201211212012',
-          isRemote: false,
-          traceFlags: TraceFlags.SAMPLED,
-          traceState: undefined,
-        });
-
-        context.with(ctx, () => {
+        startSpan({ name: 'parent span' }, parentSpan => {
           const span = startInactiveSpan({ name: 'test span' });
 
           expect(span).toBeDefined();
-          expect(spanToJSON(span).trace_id).toEqual('12312012123120121231201212312012');
-          expect(spanToJSON(span).parent_span_id).toEqual('1121201211212012');
+          expect(spanToJSON(span).trace_id).toEqual(parentSpan.spanContext().traceId);
+          expect(spanToJSON(span).parent_span_id).toEqual(parentSpan.spanContext().spanId);
           expect(getDynamicSamplingContextFromSpan(span)).toEqual({
             ...getDynamicSamplingContextFromClient(propagationContext.traceId, getClient()!),
-            trace_id: '12312012123120121231201212312012',
-            transaction: 'test span',
+            trace_id: parentSpan.spanContext().traceId,
+            transaction: 'parent span',
             sampled: 'true',
             sample_rate: '1',
           });
diff --git a/packages/types/src/client.ts b/packages/types/src/client.ts
@@ -244,7 +244,7 @@ export interface Client<O extends ClientOptions = ClientOptions> {
   /**
    * Register a callback when a DSC (Dynamic Sampling Context) is created.
    */
-  on(hook: 'createDsc', callback: (dsc: DynamicSamplingContext) => void): void;
+  on(hook: 'createDsc', callback: (dsc: DynamicSamplingContext, rootSpan?: Span) => void): void;
 
   /**
    * Register a callback when a Feedback event has been prepared.
@@ -338,7 +338,7 @@ export interface Client<O extends ClientOptions = ClientOptions> {
   /**
    * Fire a hook for when a DSC (Dynamic Sampling Context) is created. Expects the DSC as second argument.
    */
-  emit(hook: 'createDsc', dsc: DynamicSamplingContext): void;
+  emit(hook: 'createDsc', dsc: DynamicSamplingContext, rootSpan?: Span): void;
 
   /**
    * Fire a hook event for after preparing a feedback event. Events to be given

Original file line number	Diff line number	Diff line change
`@@ -12,6 +12,7 @@ import {`
`12`	`12`	`startSession,`
`13`	`13`	`} from '@sentry/core';`
`14`	`14`	`import {`
	`15`	`+ enhanceDscWithOpenTelemetryRootSpanName,`
`15`	`16`	`openTelemetrySetupCheck,`
`16`	`17`	`setOpenTelemetryContextAsyncContextStrategy,`
`17`	`18`	`setupEventContextTrace,`
`@@ -175,6 +176,7 @@ function _init(`
`175`	`176`	`validateOpenTelemetrySetup();`
`176`	`177`	`}`
`177`	`178`
	`179`	`+ enhanceDscWithOpenTelemetryRootSpanName(client);`
`178`	`180`	`setupEventContextTrace(client);`
`179`	`181`	`}`
`180`	`182`