From 98eb57faa2baeae26af222471cbe4929538ab7e8 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 17 Jul 2024 09:25:04 +0200 Subject: [PATCH 1/2] fix(node): Pass inferred name & attributes to `tracesSampler` --- .../express/tracing/tracesSampler/server.js | 40 +++++++++++++++++++ .../express/tracing/tracesSampler/test.ts | 24 +++++++++++ packages/opentelemetry/src/sampler.ts | 31 +++++++++++--- .../src/utils/parseSpanDescription.ts | 26 +++++++----- 4 files changed, 106 insertions(+), 15 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/server.js create mode 100644 dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/test.ts diff --git a/dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/server.js b/dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/server.js new file mode 100644 index 000000000000..bfe39fc89ee5 --- /dev/null +++ b/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); diff --git a/dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/test.ts b/dev-packages/node-integration-tests/suites/express/tracing/tracesSampler/test.ts new file mode 100644 index 000000000000..a19299787f91 --- /dev/null +++ b/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'); + }); + }); +}); diff --git a/packages/opentelemetry/src/sampler.ts b/packages/opentelemetry/src/sampler.ts index 0a7d2f9af972..6da0df78ac7d 100644 --- a/packages/opentelemetry/src/sampler.ts +++ b/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,25 @@ 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 = { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: op, + ...inferredAttributes, + ...spanAttributes, + }; + const mutableSamplingDecision = { decision: true }; this._client.emit( 'beforeSampling', { - spanAttributes: spanAttributes, - spanName: spanName, + spanAttributes: mergedAttributes, + spanName: inferredSpanName, parentSampled: parentSampled, parentContext: parentContext, }, @@ -72,10 +91,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, diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts index c9e732683c38..a9d99aa91b8a 100644 --- a/packages/opentelemetry/src/utils/parseSpanDescription.ts +++ b/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 : ''; - +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 : ''; + 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]; From 32e6f042c493e775396011082db5cc8ec78e90d9 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 17 Jul 2024 11:08:00 +0200 Subject: [PATCH 2/2] properly handle op --- packages/opentelemetry/src/sampler.ts | 5 ++++- packages/opentelemetry/src/trace.ts | 23 +++++++---------------- packages/opentelemetry/test/trace.test.ts | 23 +++++++++++++++-------- 3 files changed, 26 insertions(+), 25 deletions(-) diff --git a/packages/opentelemetry/src/sampler.ts b/packages/opentelemetry/src/sampler.ts index 6da0df78ac7d..446c325f3ac7 100644 --- a/packages/opentelemetry/src/sampler.ts +++ b/packages/opentelemetry/src/sampler.ts @@ -70,11 +70,14 @@ export class SentrySampler implements Sampler { } = inferSpanData(spanName, spanAttributes, spanKind); const mergedAttributes = { - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: op, ...inferredAttributes, ...spanAttributes, }; + if (op) { + mergedAttributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] = op; + } + const mutableSamplingDecision = { decision: true }; this._client.emit( 'beforeSampling', diff --git a/packages/opentelemetry/src/trace.ts b/packages/opentelemetry/src/trace.ts index 6f9fe5dad6d1..54195beb61ee 100644 --- a/packages/opentelemetry/src/trace.ts +++ b/packages/opentelemetry/src/trace.ts @@ -45,8 +45,6 @@ export function startSpan(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( 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, }; diff --git a/packages/opentelemetry/test/trace.test.ts b/packages/opentelemetry/test/trace.test.ts index 2332fd1ced05..5d9329650969 100644 --- a/packages/opentelemetry/test/trace.test.ts +++ b/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 }, });