feat(opentelemetry)!: Exclusively pass root spans through sampling pipeline (#14951)

lforst · web-flow · commit 6a6e05bd9544 · 2025-01-10T09:00:49.000+01:00
diff --git a/docs/migration/v8-to-v9.md b/docs/migration/v8-to-v9.md
@@ -88,6 +88,8 @@ In v9, an `undefined` value will be treated the same as if the value is not defi
 
 - The `requestDataIntegration` will no longer automatically set the user from `request.user`. This is an express-specific, undocumented behavior, and also conflicts with our privacy-by-default strategy. Starting in v9, you'll need to manually call `Sentry.setUser()` e.g. in a middleware to set the user on Sentry events.
 
+- The `tracesSampler` hook will no longer be called for _every_ span. Instead, it will only be called for "root spans". Root spans are spans that have no local parent span. Root spans may however have incoming trace data from a different service, for example when using distributed tracing.
+
 ### `@sentry/browser`
 
 - The `captureUserFeedback` method has been removed. Use `captureFeedback` instead and update the `comments` field to `message`.
diff --git a/packages/core/src/semanticAttributes.ts b/packages/core/src/semanticAttributes.ts
@@ -7,6 +7,8 @@ export const SEMANTIC_ATTRIBUTE_SENTRY_SOURCE = 'sentry.source';
 
 /**
  * Use this attribute to represent the sample rate used for a span.
+ *
+ * NOTE: Is only defined on root spans.
  */
 export const SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE = 'sentry.sample_rate';
 
diff --git a/packages/opentelemetry/src/sampler.ts b/packages/opentelemetry/src/sampler.ts
@@ -95,45 +95,62 @@ export class SentrySampler implements Sampler {
       return wrapSamplingDecision({ decision: undefined, context, spanAttributes });
     }
 
-    const [sampled, sampleRate] = sampleSpan(options, {
-      name: inferredSpanName,
-      attributes: mergedAttributes,
-      transactionContext: {
+    const isRootSpan = !parentSpan || parentContext?.isRemote;
+
+    // We only sample based on parameters (like tracesSampleRate or tracesSampler) for root spans (which is done in sampleSpan).
+    // Non-root-spans simply inherit the sampling decision from their parent.
+    if (isRootSpan) {
+      const [sampled, sampleRate] = sampleSpan(options, {
         name: inferredSpanName,
+        attributes: mergedAttributes,
+        transactionContext: {
+          name: inferredSpanName,
+          parentSampled,
+        },
         parentSampled,
-      },
-      parentSampled,
-    });
+      });
 
-    const attributes: Attributes = {
-      [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: sampleRate,
-    };
+      const attributes: Attributes = {
+        [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: sampleRate,
+      };
 
-    const method = `${maybeSpanHttpMethod}`.toUpperCase();
-    if (method === 'OPTIONS' || method === 'HEAD') {
-      DEBUG_BUILD && logger.log(`[Tracing] Not sampling span because HTTP method is '${method}' for ${spanName}`);
+      const method = `${maybeSpanHttpMethod}`.toUpperCase();
+      if (method === 'OPTIONS' || method === 'HEAD') {
+        DEBUG_BUILD && logger.log(`[Tracing] Not sampling span because HTTP method is '${method}' for ${spanName}`);
 
-      return {
-        ...wrapSamplingDecision({ decision: SamplingDecision.NOT_RECORD, context, spanAttributes }),
-        attributes,
-      };
-    }
+        return {
+          ...wrapSamplingDecision({ decision: SamplingDecision.NOT_RECORD, context, spanAttributes }),
+          attributes,
+        };
+      }
 
-    if (!sampled) {
-      if (parentSampled === undefined) {
+      if (
+        !sampled &&
+        // We check for `parentSampled === undefined` because we only want to record client reports for spans that are trace roots (ie. when there was incoming trace)
+        parentSampled === undefined
+      ) {
         DEBUG_BUILD && logger.log('[Tracing] Discarding root span because its trace was not chosen to be sampled.');
         this._client.recordDroppedEvent('sample_rate', 'transaction');
       }
 
       return {
-        ...wrapSamplingDecision({ decision: SamplingDecision.NOT_RECORD, context, spanAttributes }),
+        ...wrapSamplingDecision({
+          decision: sampled ? SamplingDecision.RECORD_AND_SAMPLED : SamplingDecision.NOT_RECORD,
+          context,
+          spanAttributes,
+        }),
         attributes,
       };
+    } else {
+      return {
+        ...wrapSamplingDecision({
+          decision: parentSampled ? SamplingDecision.RECORD_AND_SAMPLED : SamplingDecision.NOT_RECORD,
+          context,
+          spanAttributes,
+        }),
+        attributes: {},
+      };
     }
-    return {
-      ...wrapSamplingDecision({ decision: SamplingDecision.RECORD_AND_SAMPLED, context, spanAttributes }),
-      attributes,
-    };
   }
 
   /** Returns the sampler name or short description with the configuration. */
diff --git a/packages/opentelemetry/test/sampler.test.ts b/packages/opentelemetry/test/sampler.test.ts
@@ -57,7 +57,7 @@ describe('SentrySampler', () => {
     const actual = sampler.shouldSample(ctx, traceId, spanName, spanKind, spanAttributes, links);
     expect(actual).toEqual({
       decision: SamplingDecision.NOT_RECORD,
-      attributes: { 'sentry.sample_rate': 0 },
+      attributes: {},
       traceState: new TraceState().set(SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING, '1'),
     });
     expect(spyOnDroppedEvent).toHaveBeenCalledTimes(0);
diff --git a/packages/opentelemetry/test/trace.test.ts b/packages/opentelemetry/test/trace.test.ts
@@ -1336,12 +1336,27 @@ describe('trace (sampling)', () => {
       });
     });
 
-    expect(tracesSampler).toHaveBeenCalledTimes(3);
-    expect(tracesSampler).toHaveBeenLastCalledWith({
-      parentSampled: false,
+    expect(tracesSampler).toHaveBeenCalledTimes(2);
+    expect(tracesSampler).toHaveBeenCalledWith(
+      expect.objectContaining({
+        parentSampled: undefined,
+        name: 'outer',
+        attributes: {},
+        transactionContext: { name: 'outer', parentSampled: undefined },
+      }),
+    );
+    expect(tracesSampler).toHaveBeenCalledWith(
+      expect.objectContaining({
+        parentSampled: undefined,
+        name: 'outer2',
+        attributes: {},
+        transactionContext: { name: 'outer2', parentSampled: undefined },
+      }),
+    );
+
+    // Only root spans should go through the sampler
+    expect(tracesSampler).not.toHaveBeenLastCalledWith({
       name: 'inner2',
-      attributes: {},
-      transactionContext: { name: 'inner2', parentSampled: false },
     });
   });
 
@@ -1390,13 +1405,22 @@ describe('trace (sampling)', () => {
       });
     });
 
-    expect(tracesSampler).toHaveBeenCalledTimes(3);
-    expect(tracesSampler).toHaveBeenLastCalledWith({
-      parentSampled: false,
-      name: 'inner2',
-      attributes: {},
-      transactionContext: { name: 'inner2', parentSampled: false },
-    });
+    expect(tracesSampler).toHaveBeenCalledTimes(2);
+    expect(tracesSampler).toHaveBeenCalledWith(
+      expect.objectContaining({
+        parentSampled: undefined,
+        name: 'outer2',
+        attributes: {},
+        transactionContext: { name: 'outer2', parentSampled: undefined },
+      }),
+    );
+
+    // Only root spans should be passed to tracesSampler
+    expect(tracesSampler).not.toHaveBeenLastCalledWith(
+      expect.objectContaining({
+        name: 'inner2',
+      }),
+    );
 
     // Now return `0.4`, it should not sample
     tracesSamplerResponse = 0.4;
@@ -1405,7 +1429,7 @@ describe('trace (sampling)', () => {
       expect(outerSpan.isRecording()).toBe(false);
     });
 
-    expect(tracesSampler).toHaveBeenCalledTimes(4);
+    expect(tracesSampler).toHaveBeenCalledTimes(3);
     expect(tracesSampler).toHaveBeenLastCalledWith({
       parentSampled: undefined,
       name: 'outer3',
