From da287e60c48264ad4be0847e656e6232895addba Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 18 Jun 2024 10:22:39 +0200 Subject: [PATCH 1/3] fix(node): Unify`getDynamicSamplingContextFromSpan` 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. --- .../src/tracing/dynamicSamplingContext.ts | 22 +++++-- .../tracing/dynamicSamplingContext.test.ts | 23 ++++++- packages/opentelemetry/src/index.ts | 3 +- packages/opentelemetry/src/propagator.ts | 9 ++- .../src/setupEventContextTrace.ts | 4 +- packages/opentelemetry/src/spanExporter.ts | 4 +- packages/opentelemetry/src/trace.ts | 2 +- .../src/utils/dynamicSamplingContext.ts | 65 ------------------- packages/opentelemetry/test/trace.test.ts | 20 ++---- 9 files changed, 58 insertions(+), 94 deletions(-) delete mode 100644 packages/opentelemetry/src/utils/dynamicSamplingContext.ts diff --git a/packages/core/src/tracing/dynamicSamplingContext.ts b/packages/core/src/tracing/dynamicSamplingContext.ts index 15486950649c..9e6337a7c014 100644 --- 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 { 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/opentelemetry/src/index.ts b/packages/opentelemetry/src/index.ts index 97ed2f2e4764..539d4e29eb75 100644 --- a/packages/opentelemetry/src/index.ts +++ b/packages/opentelemetry/src/index.ts @@ -16,7 +16,8 @@ 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'; diff --git a/packages/opentelemetry/src/propagator.ts b/packages/opentelemetry/src/propagator.ts index 05b108583f98..ac8340bdc66a 100644 --- 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 index fc3441b06792..f2e1454a02df 100644 --- 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 index ebd1536ff9b4..94e12bc019a8 100644 --- 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 index d55a7ba04a9f..b7d60ab117ad 100644 --- 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 deleted file mode 100644 index 3e4f9f67ae84..000000000000 --- a/packages/opentelemetry/src/utils/dynamicSamplingContext.ts +++ /dev/null @@ -1,65 +0,0 @@ -import { - SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, - SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, - getClient, - getDynamicSamplingContextFromClient, -} from '@sentry/core'; -import type { DynamicSamplingContext } from '@sentry/types'; -import { baggageHeaderToDynamicSamplingContext } from '@sentry/utils'; -import { SENTRY_TRACE_STATE_DSC } from '../constants'; -import type { AbstractSpan } from '../types'; -import { getSamplingDecision } from './getSamplingDecision'; -import { parseSpanDescription } from './parseSpanDescription'; -import { spanHasAttributes, spanHasName } from './spanTypes'; - -/** - * Creates a dynamic sampling context from a span (and client and scope) - * - * @param span the span from which a few values like the root span name and sample rate are extracted. - * - * @returns a dynamic sampling context - */ -export function getDynamicSamplingContextFromSpan(span: AbstractSpan): Readonly> { - const client = getClient(); - if (!client) { - return {}; - } - - const traceState = span.spanContext().traceState; - const traceStateDsc = traceState?.get(SENTRY_TRACE_STATE_DSC); - - // If the span has a DSC, we want it to take precedence - const dscOnSpan = traceStateDsc ? baggageHeaderToDynamicSamplingContext(traceStateDsc) : undefined; - - if (dscOnSpan) { - return dscOnSpan; - } - - const dsc = getDynamicSamplingContextFromClient(span.spanContext().traceId, client); - - const attributes = spanHasAttributes(span) ? span.attributes : {}; - - const sampleRate = attributes[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]; - if (sampleRate != null) { - dsc.sample_rate = `${sampleRate}`; - } - - // We don't want to have a transaction name in the DSC if the source is "url" because URLs might contain PII - const source = attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]; - - // If the span has no name, we assume it is non-recording and want to opt out of using any description - const { description } = spanHasName(span) ? parseSpanDescription(span) : { description: '' }; - - if (source !== 'url' && description) { - dsc.transaction = description; - } - - const sampled = getSamplingDecision(span.spanContext()); - if (sampled != null) { - dsc.sampled = String(sampled); - } - - client.emit('createDsc', dsc); - - return dsc; -} diff --git a/packages/opentelemetry/test/trace.test.ts b/packages/opentelemetry/test/trace.test.ts index 289c494c7a75..2da8c7a8b511 100644 --- 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', }); From c7222130ccc40d5f97f1b997a8b85a5f548c9711 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 19 Jun 2024 09:17:55 +0200 Subject: [PATCH 2/3] fix: Ensure DSC is correct --- packages/core/src/baseclient.ts | 4 +-- .../src/tracing/dynamicSamplingContext.ts | 2 +- packages/node/src/sdk/index.ts | 2 ++ packages/opentelemetry/src/index.ts | 2 ++ .../src/utils/setupDscHandler.ts | 27 +++++++++++++++++++ packages/types/src/client.ts | 4 +-- 6 files changed, 36 insertions(+), 5 deletions(-) create mode 100644 packages/opentelemetry/src/utils/setupDscHandler.ts diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 4b3796c295f3..59afda8dc43b 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -414,7 +414,7 @@ export abstract class BaseClient implements Client { 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 implements Client { 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 index 9e6337a7c014..d47dfd7ff317 100644 --- a/packages/core/src/tracing/dynamicSamplingContext.ts +++ b/packages/core/src/tracing/dynamicSamplingContext.ts @@ -105,7 +105,7 @@ export function getDynamicSamplingContextFromSpan(span: Span): Readonly { + // 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/types/src/client.ts b/packages/types/src/client.ts index b4609ec0568c..bb0ec4646211 100644 --- a/packages/types/src/client.ts +++ b/packages/types/src/client.ts @@ -244,7 +244,7 @@ export interface Client { /** * 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 { /** * 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 From 4cdadc4166df361f76adc6bbf886a0464a28c8f8 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 19 Jun 2024 10:46:57 +0200 Subject: [PATCH 3/3] rename method --- packages/node/src/sdk/index.ts | 4 ++-- packages/opentelemetry/src/index.ts | 2 +- ...Handler.ts => enhanceDscWithOpenTelemetryRootSpanName.ts} | 5 +++-- 3 files changed, 6 insertions(+), 5 deletions(-) rename packages/opentelemetry/src/utils/{setupDscHandler.ts => enhanceDscWithOpenTelemetryRootSpanName.ts} (84%) diff --git a/packages/node/src/sdk/index.ts b/packages/node/src/sdk/index.ts index a83e66cf5e97..490826c3953b 100644 --- a/packages/node/src/sdk/index.ts +++ b/packages/node/src/sdk/index.ts @@ -12,9 +12,9 @@ import { startSession, } from '@sentry/core'; import { + enhanceDscWithOpenTelemetryRootSpanName, openTelemetrySetupCheck, setOpenTelemetryContextAsyncContextStrategy, - setupDscHandler, setupEventContextTrace, } from '@sentry/opentelemetry'; import type { Client, Integration, Options } from '@sentry/types'; @@ -176,7 +176,7 @@ function _init( validateOpenTelemetrySetup(); } - setupDscHandler(client); + enhanceDscWithOpenTelemetryRootSpanName(client); setupEventContextTrace(client); } diff --git a/packages/opentelemetry/src/index.ts b/packages/opentelemetry/src/index.ts index 11be8e8a5d8b..5e99f7eb8c33 100644 --- a/packages/opentelemetry/src/index.ts +++ b/packages/opentelemetry/src/index.ts @@ -21,7 +21,7 @@ export { getDynamicSamplingContextFromSpan } from '@sentry/core'; export { isSentryRequestSpan } from './utils/isSentryRequest'; -export { setupDscHandler } from './utils/setupDscHandler'; +export { enhanceDscWithOpenTelemetryRootSpanName } from './utils/enhanceDscWithOpenTelemetryRootSpanName'; export { getActiveSpan } from './utils/getActiveSpan'; export { startSpan, startSpanManual, startInactiveSpan, withActiveSpan, continueTrace } from './trace'; diff --git a/packages/opentelemetry/src/utils/setupDscHandler.ts b/packages/opentelemetry/src/utils/enhanceDscWithOpenTelemetryRootSpanName.ts similarity index 84% rename from packages/opentelemetry/src/utils/setupDscHandler.ts rename to packages/opentelemetry/src/utils/enhanceDscWithOpenTelemetryRootSpanName.ts index 2d14575b1f0d..af086451ac1b 100644 --- a/packages/opentelemetry/src/utils/setupDscHandler.ts +++ b/packages/opentelemetry/src/utils/enhanceDscWithOpenTelemetryRootSpanName.ts @@ -4,9 +4,10 @@ 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. + * Setup a DSC handler on the passed client, + * ensuring that the transaction name is inferred from the span correctly. */ -export function setupDscHandler(client: Client): void { +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