fix(node): Pass inferred name & attributes to tracesSampler (#12945)

Previously, we passed the span name directly to the `tracesSampler`
as-is. However, this is not ideal because this does not match the name
we actually send to sentry later, making this confusing. The reason is
that we infer the name from the attributes for some types of spans, but
we do that at export-time.

This PR adjust this so that we send the inferred name & attributes to
the `tracesSampler`. This works reasonably enough. However, there is a
big caveat: This still only has access to the initial attributes that a
span has at creation time. This means that we'll never have e.g. the
`http.route` correctly there, because all `http.server` spans originate
from the http instrumentation where they do not have a route yet, and
then more specific instrumentation (e.g. express or fastify) add the
`http.route` to that span later. So the name will never be parametrized,
for example, for `http.server` spans, which is not ideal (as it will
still not match the final span exactly) but should still be better than
what we had so far (where the name would always just be e.g. `GET`).

Fixes #12944

Author: mydea

dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/server.js

@@ -0,0 +1,40 @@
+const { loggingTransport } = require('@sentry-internal/node-integration-tests');
+const Sentry = require('@sentry/node');
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  release: '1.0',
+  transport: loggingTransport,
+  tracesSampler: samplingContext => {
+    // The name we get here is inferred at span creation time
+    // At this point, we sadly do not have a http.route attribute yet,
+    // so we infer the name from the unparametrized route instead
+    return (
+      samplingContext.name === 'GET /test/123' &&
+      samplingContext.attributes['sentry.op'] === 'http.server' &&
+      samplingContext.attributes['http.method'] === 'GET'
+    );
+  },
+  debug: true,
+});
+
+// express must be required after Sentry is initialized
+const express = require('express');
+const cors = require('cors');
+const { startExpressServerAndSendPortToRunner } = require('@sentry-internal/node-integration-tests');
+
+const app = express();
+
+app.use(cors());
+
+app.get('/test/:id', (_req, res) => {
+  res.send('Success');
+});
+
+app.get('/test2', (_req, res) => {
+  res.send('Success');
+});
+
+Sentry.setupExpressErrorHandler(app);
+
+startExpressServerAndSendPortToRunner(app);

dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/test.ts

@@ -0,0 +1,24 @@
+import { cleanupChildProcesses, createRunner } from '../../../../utils/runner';
+
+describe('express tracesSampler', () => {
+  afterAll(() => {
+    cleanupChildProcesses();
+  });
+
+  describe('CJS', () => {
+    test('correctly samples & passes data to tracesSampler', done => {
+      const runner = createRunner(__dirname, 'server.js')
+        .expect({
+          transaction: {
+            transaction: 'GET /test/:id',
+          },
+        })
+        .start(done);
+
+      // This is not sampled
+      runner.makeRequest('get', '/test2?q=1');
+      // This is sampled
+      runner.makeRequest('get', '/test/123?q=1');
+    });
+  });
+});

packages/opentelemetry/src/sampler.ts

@@ -4,7 +4,12 @@ import { isSpanContextValid, trace } from '@opentelemetry/api';
 import { TraceState } from '@opentelemetry/core';
 import type { Sampler, SamplingResult } from '@opentelemetry/sdk-trace-base';
 import { SamplingDecision } from '@opentelemetry/sdk-trace-base';
-import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, hasTracingEnabled, sampleSpan } from '@sentry/core';
+import {
+  SEMANTIC_ATTRIBUTE_SENTRY_OP,
+  SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE,
+  hasTracingEnabled,
+  sampleSpan,
+} from '@sentry/core';
 import type { Client, SpanAttributes } from '@sentry/types';
 import { logger } from '@sentry/utils';
 import { SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING, SENTRY_TRACE_STATE_URL } from './constants';
@@ -13,6 +18,7 @@ import { SEMATTRS_HTTP_METHOD, SEMATTRS_HTTP_URL } from '@opentelemetry/semantic
 import { DEBUG_BUILD } from './debug-build';
 import { getPropagationContextFromSpan } from './propagator';
 import { getSamplingDecision } from './utils/getSamplingDecision';
+import { inferSpanData } from './utils/parseSpanDescription';
 import { setIsSetup } from './utils/setupCheck';
 
 /**
@@ -56,12 +62,28 @@ export class SentrySampler implements Sampler {
 
     const parentSampled = parentSpan ? getParentSampled(parentSpan, traceId, spanName) : undefined;
 
+    // We want to pass the inferred name & attributes to the sampler method
+    const {
+      description: inferredSpanName,
+      data: inferredAttributes,
+      op,
+    } = inferSpanData(spanName, spanAttributes, spanKind);
+
+    const mergedAttributes = {
+      ...inferredAttributes,
+      ...spanAttributes,
+    };
+
+    if (op) {
+      mergedAttributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] = op;
+    }
+
     const mutableSamplingDecision = { decision: true };
     this._client.emit(
       'beforeSampling',
       {
-        spanAttributes: spanAttributes,
-        spanName: spanName,
+        spanAttributes: mergedAttributes,
+        spanName: inferredSpanName,
         parentSampled: parentSampled,
         parentContext: parentContext,
       },
@@ -72,10 +94,10 @@ export class SentrySampler implements Sampler {
     }
 
     const [sampled, sampleRate] = sampleSpan(options, {
-      name: spanName,
-      attributes: spanAttributes,
+      name: inferredSpanName,
+      attributes: mergedAttributes,
       transactionContext: {
-        name: spanName,
+        name: inferredSpanName,
         parentSampled,
       },
       parentSampled,

packages/opentelemetry/src/trace.ts

@@ -45,8 +45,6 @@ export function startSpan<T>(options: OpenTelemetrySpanContext, callback: (span:
     const spanOptions = getSpanOptions(options);
 
     return tracer.startActiveSpan(name, spanOptions, ctx, span => {
-      _applySentryAttributesToSpan(span, options);
-
       return handleCallbackErrors(
         () => callback(span),
         () => {
@@ -90,8 +88,6 @@ export function startSpanManual<T>(
     const spanOptions = getSpanOptions(options);
 
     return tracer.startActiveSpan(name, spanOptions, ctx, span => {
-      _applySentryAttributesToSpan(span, options);
-
       return handleCallbackErrors(
         () => callback(span, () => span.end()),
         () => {
@@ -131,8 +127,6 @@ export function startInactiveSpan(options: OpenTelemetrySpanContext): Span {
 
     const span = tracer.startSpan(name, spanOptions, ctx);
 
-    _applySentryAttributesToSpan(span, options);
-
     return span;
   });
 }
@@ -156,22 +150,19 @@ function getTracer(): Tracer {
   return (client && client.tracer) || trace.getTracer('@sentry/opentelemetry', SDK_VERSION);
 }
 
-function _applySentryAttributesToSpan(span: Span, options: OpenTelemetrySpanContext): void {
-  const { op } = options;
-
-  if (op) {
-    span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, op);
-  }
-}
-
 function getSpanOptions(options: OpenTelemetrySpanContext): SpanOptions {
-  const { startTime, attributes, kind } = options;
+  const { startTime, attributes, kind, op } = options;
 
   // OTEL expects timestamps in ms, not seconds
   const fixedStartTime = typeof startTime === 'number' ? ensureTimestampInMilliseconds(startTime) : startTime;
 
   return {
-    attributes,
+    attributes: op
+      ? {
+          [SEMANTIC_ATTRIBUTE_SENTRY_OP]: op,
+          ...attributes,
+        }
+      : attributes,
     kind,
     startTime: fixedStartTime,
   };

packages/opentelemetry/src/utils/parseSpanDescription.ts

@@ -11,7 +11,7 @@ import {
   SEMATTRS_MESSAGING_SYSTEM,
   SEMATTRS_RPC_SERVICE,
 } from '@opentelemetry/semantic-conventions';
-import type { TransactionSource } from '@sentry/types';
+import type { SpanAttributes, TransactionSource } from '@sentry/types';
 import { getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from '@sentry/utils';
 
 import { SEMANTIC_ATTRIBUTE_SENTRY_OP } from '@sentry/core';
@@ -27,14 +27,9 @@ interface SpanDescription {
 }
 
 /**
- * Extract better op/description from an otel span.
- *
- * Based on https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/7422ce2a06337f68a59b552b8c5a2ac125d6bae5/exporter/sentryexporter/sentry_exporter.go#L306
+ * Infer the op & description for a set of name, attributes and kind of a span.
  */
-export function parseSpanDescription(span: AbstractSpan): SpanDescription {
-  const attributes = spanHasAttributes(span) ? span.attributes : {};
-  const name = spanHasName(span) ? span.name : '<unknown>';
-
+export function inferSpanData(name: string, attributes: SpanAttributes, kind: SpanKind): SpanDescription {
   // This attribute is intentionally exported as a SEMATTR constant because it should stay intimite API
   if (attributes['sentry.skip_span_data_inference']) {
     return {
@@ -54,7 +49,7 @@ export function parseSpanDescription(span: AbstractSpan): SpanDescription {
   // conventions export an attribute key for it.
   const httpMethod = attributes['http.request.method'] || attributes[SEMATTRS_HTTP_METHOD];
   if (httpMethod) {
-    return descriptionForHttpMethod({ attributes, name, kind: getSpanKind(span) }, httpMethod);
+    return descriptionForHttpMethod({ attributes, name, kind }, httpMethod);
   }
 
   const dbSystem = attributes[SEMATTRS_DB_SYSTEM];
@@ -97,6 +92,19 @@ export function parseSpanDescription(span: AbstractSpan): SpanDescription {
   return { op: undefined, description: name, source: 'custom' };
 }
 
+/**
+ * Extract better op/description from an otel span.
+ *
+ * Based on https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/7422ce2a06337f68a59b552b8c5a2ac125d6bae5/exporter/sentryexporter/sentry_exporter.go#L306
+ */
+export function parseSpanDescription(span: AbstractSpan): SpanDescription {
+  const attributes = spanHasAttributes(span) ? span.attributes : {};
+  const name = spanHasName(span) ? span.name : '<unknown>';
+  const kind = getSpanKind(span);
+
+  return inferSpanData(name, attributes, kind);
+}
+
 function descriptionForDbSystem({ attributes, name }: { attributes: Attributes; name: string }): SpanDescription {
   // Use DB statement (Ex "SELECT * FROM table") if possible as description.
   const statement = attributes[SEMATTRS_DB_STATEMENT];

packages/opentelemetry/test/trace.test.ts

@@ -1320,9 +1320,7 @@ describe('trace (sampling)', () => {
     expect(tracesSampler).toHaveBeenLastCalledWith({
       parentSampled: undefined,
       name: 'outer',
-      attributes: {
-        [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
-      },
+      attributes: {},
       transactionContext: { name: 'outer', parentSampled: undefined },
     });
 
@@ -1357,16 +1355,25 @@ describe('trace (sampling)', () => {
 
     mockSdkInit({ tracesSampler });
 
-    startSpan({ name: 'outer' }, outerSpan => {
-      expect(outerSpan).toBeDefined();
-    });
+    startSpan(
+      {
+        name: 'outer',
+        op: 'test.op',
+        attributes: { attr1: 'yes', attr2: 1 },
+      },
+      outerSpan => {
+        expect(outerSpan).toBeDefined();
+      },
+    );
 
-    expect(tracesSampler).toBeCalledTimes(1);
+    expect(tracesSampler).toHaveBeenCalledTimes(1);
     expect(tracesSampler).toHaveBeenLastCalledWith({
       parentSampled: undefined,
       name: 'outer',
       attributes: {
-        [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
+        attr1: 'yes',
+        attr2: 1,
+        'sentry.op': 'test.op',
       },
       transactionContext: { name: 'outer', parentSampled: undefined },
     });