From 0f1ea2f24b7a5f01f2e4640315b82f5467149141 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 14 Mar 2024 14:00:10 +0000 Subject: [PATCH] fix(opentelemetry): Fix span & sampling propagation (#11092) OK, this was a tricky one, but I _think_ it now works as expected. This PR fixes to fundamental issues with sampling & propagation that were uncovered by @Lms24 & myself while trying to use OTEL for remix & sveltekit: 1. `continueTrace` updates the propagation context, but if there is an active parent span (even a remote one) this is ignored. 2. Sampling inheritance did not work as expected, due to the fact that OTEL spans cannot differentiate between `sampled=false` (sampled to be not recorded) and `sampled=undefined` (no sampling decision yet). ## Update to `continueTrace` & trace propagation While my first instinct was to ensure that in the trace methods, if we have remote span we ignore it and look at the propagation context, this has a bunch of problems - because it means we can run out of sync, if this is set from outside, etc. So instead, I now provide a custom `continueTrace` method from `@sentry/opentelemetry` & `@sentry/node` which should be used instead of the core one in meta SDKs. This method will, in addition to updating the propagation context, _also_ create a remote span with the passed in data, and make it the active span in the callback. Then, I updated the otel start span APIs to always use that, if it exists (which was already the behavior we had), PLUS also added behavior that if there is no active span at all (not even a remote one), _then_ we look at the propagation context. ## Update to sampling inheritance Previously, we basically did the following: ```ts const sampled: Boolean | undefined = spanContext.traceFlags === TraceFlags.SAMPLED; // this will always be true or false, never undefined ``` Which means that if we create a remote span from a minimal propagation context: ```ts // This could be a generated propagation context from a scope const propagationContext = { spanId: 'xxx', traceId: 'yyy' }; const spanContext: SpanContext = { sampled: propagationContext.sampled ? TraceFlags.SAMPLED : TraceFlags.NONE, } ``` We would later always get `sampled: false`, and inherit this decision for all downstream spans - instead of treating it as `undefined`, and going through the sampler, as we actually want it to. In order to "solve" this, I added a new trace state `SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING`, which we set if we _know_ this is actually `sampled: false`, and not just unset. Then, based on this we can interpret `sampled` as being `false` or `undefined`, respectively. This is a bit hacky but should work - it means that if we get a sampling decision from outside we'll treat it as `undefined`, which is OK I would say. Our own sampler will set this correctly so we inherit correctly as well, and our propagator does so too. --------- Co-authored-by: Lukas Stracke --- packages/astro/src/index.types.ts | 7 +- packages/core/src/utils/spanUtils.ts | 3 +- packages/node-experimental/src/index.ts | 5 +- .../test/integration/scope.test.ts | 10 +- .../test/integration/transactions.test.ts | 4 + packages/opentelemetry/src/constants.ts | 4 +- packages/opentelemetry/src/index.ts | 2 +- packages/opentelemetry/src/propagator.ts | 125 +++++++-- packages/opentelemetry/src/sampler.ts | 13 +- packages/opentelemetry/src/spanProcessor.ts | 2 +- packages/opentelemetry/src/trace.ts | 80 +++++- .../src/utils/dynamicSamplingContext.ts | 10 +- .../test/integration/scope.test.ts | 39 ++- .../test/integration/transactions.test.ts | 4 + .../opentelemetry/test/propagator.test.ts | 116 +++++++- packages/opentelemetry/test/trace.test.ts | 258 +++++++++++++++++- 16 files changed, 603 insertions(+), 79 deletions(-) diff --git a/packages/astro/src/index.types.ts b/packages/astro/src/index.types.ts index ec0e8b507725..3719a2d5ecea 100644 --- a/packages/astro/src/index.types.ts +++ b/packages/astro/src/index.types.ts @@ -28,11 +28,8 @@ export declare const getActiveSpan: typeof clientSdk.getActiveSpan; // eslint-disable-next-line deprecation/deprecation export declare const getCurrentHub: typeof clientSdk.getCurrentHub; export declare const getClient: typeof clientSdk.getClient; -export declare const startSpan: typeof clientSdk.startSpan; -export declare const startInactiveSpan: typeof clientSdk.startInactiveSpan; -export declare const startSpanManual: typeof clientSdk.startSpanManual; -export declare const withActiveSpan: typeof clientSdk.withActiveSpan; -export declare const getRootSpan: typeof clientSdk.getRootSpan; +export declare const continueTrace: typeof clientSdk.continueTrace; + export declare const Span: clientSdk.Span; export declare const metrics: typeof clientSdk.metrics & typeof serverSdk.metrics; diff --git a/packages/core/src/utils/spanUtils.ts b/packages/core/src/utils/spanUtils.ts index 9e311195b577..094f6674121c 100644 --- a/packages/core/src/utils/spanUtils.ts +++ b/packages/core/src/utils/spanUtils.ts @@ -162,8 +162,7 @@ export function spanIsSampled(span: Span): boolean { // We align our trace flags with the ones OpenTelemetry use // So we also check for sampled the same way they do. const { traceFlags } = span.spanContext(); - // eslint-disable-next-line no-bitwise - return Boolean(traceFlags & TRACE_FLAG_SAMPLED); + return traceFlags === TRACE_FLAG_SAMPLED; } /** Get the status message to use for a JSON representation of a span. */ diff --git a/packages/node-experimental/src/index.ts b/packages/node-experimental/src/index.ts index 68f663e42a60..a8d511702189 100644 --- a/packages/node-experimental/src/index.ts +++ b/packages/node-experimental/src/index.ts @@ -47,6 +47,10 @@ export { extractRequestData, } from '@sentry/utils'; +// These are custom variants that need to be used instead of the core one +// As they have slightly different implementations +export { continueTrace } from '@sentry/opentelemetry'; + export { addBreadcrumb, isInitialized, @@ -78,7 +82,6 @@ export { setCurrentClient, Scope, setMeasurement, - continueTrace, getSpanDescendants, parameterize, getCurrentScope, diff --git a/packages/node-experimental/test/integration/scope.test.ts b/packages/node-experimental/test/integration/scope.test.ts index cc8ca4d6b383..e740ca0584f7 100644 --- a/packages/node-experimental/test/integration/scope.test.ts +++ b/packages/node-experimental/test/integration/scope.test.ts @@ -65,6 +65,8 @@ describe('Integration | Scope', () => { trace: { span_id: spanId, trace_id: traceId, + // local span ID from propagation context + ...(enableTracing ? { parent_span_id: expect.any(String) } : undefined), }, }), }), @@ -110,6 +112,8 @@ describe('Integration | Scope', () => { status: 'ok', trace_id: traceId, origin: 'manual', + // local span ID from propagation context + parent_span_id: expect.any(String), }, }), spans: [], @@ -194,7 +198,8 @@ describe('Integration | Scope', () => { ? { span_id: spanId1, trace_id: traceId1, - parent_span_id: undefined, + // local span ID from propagation context + ...(enableTracing ? { parent_span_id: expect.any(String) } : undefined), } : expect.any(Object), }), @@ -220,7 +225,8 @@ describe('Integration | Scope', () => { ? { span_id: spanId2, trace_id: traceId2, - parent_span_id: undefined, + // local span ID from propagation context + ...(enableTracing ? { parent_span_id: expect.any(String) } : undefined), } : expect.any(Object), }), diff --git a/packages/node-experimental/test/integration/transactions.test.ts b/packages/node-experimental/test/integration/transactions.test.ts index 6695912729d0..ea368a7b2fb8 100644 --- a/packages/node-experimental/test/integration/transactions.test.ts +++ b/packages/node-experimental/test/integration/transactions.test.ts @@ -267,6 +267,8 @@ describe('Integration | Transactions', () => { status: 'ok', trace_id: expect.any(String), origin: 'auto.test', + // local span ID from propagation context + parent_span_id: expect.any(String), }, }), spans: [expect.any(Object), expect.any(Object)], @@ -312,6 +314,8 @@ describe('Integration | Transactions', () => { status: 'ok', trace_id: expect.any(String), origin: 'manual', + // local span ID from propagation context + parent_span_id: expect.any(String), }, }), spans: [expect.any(Object), expect.any(Object)], diff --git a/packages/opentelemetry/src/constants.ts b/packages/opentelemetry/src/constants.ts index 0673b40b6af3..e153d50b0180 100644 --- a/packages/opentelemetry/src/constants.ts +++ b/packages/opentelemetry/src/constants.ts @@ -2,8 +2,10 @@ import { createContextKey } from '@opentelemetry/api'; export const SENTRY_TRACE_HEADER = 'sentry-trace'; export const SENTRY_BAGGAGE_HEADER = 'baggage'; -export const SENTRY_TRACE_STATE_DSC = 'sentry.trace'; + +export const SENTRY_TRACE_STATE_DSC = 'sentry.dsc'; export const SENTRY_TRACE_STATE_PARENT_SPAN_ID = 'sentry.parent_span_id'; +export const SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING = 'sentry.sampled_not_recording'; export const SENTRY_SCOPES_CONTEXT_KEY = createContextKey('sentry_scopes'); diff --git a/packages/opentelemetry/src/index.ts b/packages/opentelemetry/src/index.ts index 2fca830ab721..378216ceab83 100644 --- a/packages/opentelemetry/src/index.ts +++ b/packages/opentelemetry/src/index.ts @@ -21,7 +21,7 @@ export { export { isSentryRequestSpan } from './utils/isSentryRequest'; export { getActiveSpan } from './utils/getActiveSpan'; -export { startSpan, startSpanManual, startInactiveSpan, withActiveSpan } from './trace'; +export { startSpan, startSpanManual, startInactiveSpan, withActiveSpan, continueTrace } from './trace'; // eslint-disable-next-line deprecation/deprecation export { setupGlobalHub } from './custom/hub'; diff --git a/packages/opentelemetry/src/propagator.ts b/packages/opentelemetry/src/propagator.ts index bfdc6ecf970c..39be6c4d764c 100644 --- a/packages/opentelemetry/src/propagator.ts +++ b/packages/opentelemetry/src/propagator.ts @@ -1,6 +1,8 @@ import type { Baggage, Context, SpanContext, TextMapGetter, TextMapSetter } from '@opentelemetry/api'; +import { context } from '@opentelemetry/api'; import { TraceFlags, propagation, trace } from '@opentelemetry/api'; import { TraceState, W3CBaggagePropagator, isTracingSuppressed } from '@opentelemetry/core'; +import type { continueTrace } from '@sentry/core'; import { getClient, getCurrentScope, getDynamicSamplingContextFromClient, getIsolationScope } from '@sentry/core'; import type { DynamicSamplingContext, PropagationContext } from '@sentry/types'; import { @@ -16,18 +18,20 @@ import { SENTRY_TRACE_HEADER, SENTRY_TRACE_STATE_DSC, SENTRY_TRACE_STATE_PARENT_SPAN_ID, + SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING, } from './constants'; import { getScopesFromContext, setScopesOnContext } from './utils/contextData'; import { setIsSetup } from './utils/setupCheck'; /** Get the Sentry propagation context from a span context. */ export function getPropagationContextFromSpanContext(spanContext: SpanContext): PropagationContext { - const { traceId, spanId, traceFlags, traceState } = spanContext; + const { traceId, spanId, traceState } = spanContext; const dscString = traceState ? traceState.get(SENTRY_TRACE_STATE_DSC) : undefined; const dsc = dscString ? baggageHeaderToDynamicSamplingContext(dscString) : undefined; const parentSpanId = traceState ? traceState.get(SENTRY_TRACE_STATE_PARENT_SPAN_ID) : undefined; - const sampled = traceFlags === TraceFlags.SAMPLED; + + const sampled = getSamplingDecision(spanContext); return { traceId, @@ -78,32 +82,18 @@ export class SentryPropagator extends W3CBaggagePropagator { */ public extract(context: Context, carrier: unknown, getter: TextMapGetter): Context { const maybeSentryTraceHeader: string | string[] | undefined = getter.get(carrier, SENTRY_TRACE_HEADER); - const maybeBaggageHeader = getter.get(carrier, SENTRY_BAGGAGE_HEADER); + const baggage = getter.get(carrier, SENTRY_BAGGAGE_HEADER); - const sentryTraceHeader = maybeSentryTraceHeader + const sentryTrace = maybeSentryTraceHeader ? Array.isArray(maybeSentryTraceHeader) ? maybeSentryTraceHeader[0] : maybeSentryTraceHeader : undefined; - const propagationContext = propagationContextFromHeaders(sentryTraceHeader, maybeBaggageHeader); - - // We store the DSC as OTEL trace state on the span context - const traceState = makeTraceState({ - parentSpanId: propagationContext.parentSpanId, - dsc: propagationContext.dsc, - }); - - const spanContext: SpanContext = { - traceId: propagationContext.traceId, - spanId: propagationContext.parentSpanId || '', - isRemote: true, - traceFlags: propagationContext.sampled === true ? TraceFlags.SAMPLED : TraceFlags.NONE, - traceState, - }; + const propagationContext = propagationContextFromHeaders(sentryTrace, baggage); // Add remote parent span context, - const ctxWithSpanContext = trace.setSpanContext(context, spanContext); + const ctxWithSpanContext = getContextWithRemoteActiveSpan(context, { sentryTrace, baggage }); // Also update the scope on the context (to be sure this is picked up everywhere) const scopes = getScopesFromContext(ctxWithSpanContext); @@ -128,8 +118,13 @@ export class SentryPropagator extends W3CBaggagePropagator { export function makeTraceState({ parentSpanId, dsc, -}: { parentSpanId?: string; dsc?: Partial }): TraceState | undefined { - if (!parentSpanId && !dsc) { + sampled, +}: { + parentSpanId?: string; + dsc?: Partial; + sampled?: boolean; +}): TraceState | undefined { + if (!parentSpanId && !dsc && sampled !== false) { return undefined; } @@ -140,7 +135,11 @@ export function makeTraceState({ ? new TraceState().set(SENTRY_TRACE_STATE_PARENT_SPAN_ID, parentSpanId) : new TraceState(); - return dscString ? traceStateBase.set(SENTRY_TRACE_STATE_DSC, dscString) : traceStateBase; + const traceStateWithDsc = dscString ? traceStateBase.set(SENTRY_TRACE_STATE_DSC, dscString) : traceStateBase; + + // We also specifically want to store if this is sampled to be not recording, + // or unsampled (=could be either sampled or not) + return sampled === false ? traceStateWithDsc.set(SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING, '1') : traceStateWithDsc; } function getInjectionData(context: Context): { @@ -161,7 +160,7 @@ function getInjectionData(context: Context): { dynamicSamplingContext, traceId: spanContext.traceId, spanId: spanContext.spanId, - sampled: spanContext.traceFlags === TraceFlags.SAMPLED, + sampled: getSamplingDecision(spanContext), }; } @@ -188,7 +187,7 @@ function getInjectionData(context: Context): { dynamicSamplingContext, traceId: spanContext.traceId, spanId: spanContext.spanId, - sampled: spanContext.traceFlags === TraceFlags.SAMPLED, + sampled: getSamplingDecision(spanContext), }; } @@ -221,3 +220,79 @@ function getDynamicSamplingContext( return undefined; } + +function getContextWithRemoteActiveSpan( + ctx: Context, + { sentryTrace, baggage }: Parameters[0], +): Context { + const propagationContext = propagationContextFromHeaders(sentryTrace, baggage); + + // We store the DSC as OTEL trace state on the span context + const traceState = makeTraceState({ + parentSpanId: propagationContext.parentSpanId, + dsc: propagationContext.dsc, + sampled: propagationContext.sampled, + }); + + const spanContext: SpanContext = { + traceId: propagationContext.traceId, + spanId: propagationContext.parentSpanId || '', + isRemote: true, + traceFlags: propagationContext.sampled ? TraceFlags.SAMPLED : TraceFlags.NONE, + traceState, + }; + + return trace.setSpanContext(ctx, spanContext); +} + +/** + * Takes trace strings and propagates them as a remote active span. + * This should be used in addition to `continueTrace` in OTEL-powered environments. + */ +export function continueTraceAsRemoteSpan( + ctx: Context, + options: Parameters[0], + callback: () => T, +): T { + const ctxWithSpanContext = getContextWithRemoteActiveSpan(ctx, options); + + return context.with(ctxWithSpanContext, callback); +} + +/** + * OpenTelemetry only knows about SAMPLED or NONE decision, + * but for us it is important to differentiate between unset and unsampled. + * + * Both of these are identified as `traceFlags === TracegFlags.NONE`, + * but we additionally look at a special trace state to differentiate between them. + */ +export function getSamplingDecision(spanContext: SpanContext): boolean | undefined { + const { traceFlags, traceState } = spanContext; + + const sampledNotRecording = traceState ? traceState.get(SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING) === '1' : false; + + // If trace flag is `SAMPLED`, we interpret this as sampled + // If it is `NONE`, it could mean either it was sampled to be not recorder, or that it was not sampled at all + // For us this is an important difference, sow e look at the SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING + // to identify which it is + if (traceFlags === TraceFlags.SAMPLED) { + return true; + } + + if (sampledNotRecording) { + return false; + } + + // Fall back to DSC as a last resort, that may also contain `sampled`... + const dscString = traceState ? traceState.get(SENTRY_TRACE_STATE_DSC) : undefined; + const dsc = dscString ? baggageHeaderToDynamicSamplingContext(dscString) : undefined; + + if (dsc?.sampled === 'true') { + return true; + } + if (dsc?.sampled === 'false') { + return false; + } + + return undefined; +} diff --git a/packages/opentelemetry/src/sampler.ts b/packages/opentelemetry/src/sampler.ts index 2b715eedd705..9ae0b60699ca 100644 --- a/packages/opentelemetry/src/sampler.ts +++ b/packages/opentelemetry/src/sampler.ts @@ -1,14 +1,15 @@ -/* eslint-disable no-bitwise */ import type { Attributes, Context, SpanContext } from '@opentelemetry/api'; -import { TraceFlags, isSpanContextValid, trace } from '@opentelemetry/api'; +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 } from '@sentry/core'; import type { Client, ClientOptions, SamplingContext } from '@sentry/types'; import { isNaN, logger } from '@sentry/utils'; +import { SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING } from './constants'; import { DEBUG_BUILD } from './debug-build'; -import { getPropagationContextFromSpanContext } from './propagator'; +import { getPropagationContextFromSpanContext, getSamplingDecision } from './propagator'; import { setIsSetup } from './utils/setupCheck'; /** @@ -38,6 +39,7 @@ export class SentrySampler implements Sampler { } const parentContext = trace.getSpanContext(context); + const traceState = parentContext?.traceState || new TraceState(); let parentSampled: boolean | undefined = undefined; @@ -49,7 +51,7 @@ export class SentrySampler implements Sampler { DEBUG_BUILD && logger.log(`[Tracing] Inheriting remote parent's sampled decision for ${spanName}: ${parentSampled}`); } else { - parentSampled = Boolean(parentContext.traceFlags & TraceFlags.SAMPLED); + parentSampled = getSamplingDecision(parentContext); DEBUG_BUILD && logger.log(`[Tracing] Inheriting parent's sampled decision for ${spanName}: ${parentSampled}`); } } @@ -76,6 +78,7 @@ export class SentrySampler implements Sampler { return { decision: SamplingDecision.NOT_RECORD, attributes, + traceState: traceState.set(SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING, '1'), }; } @@ -93,6 +96,7 @@ export class SentrySampler implements Sampler { return { decision: SamplingDecision.NOT_RECORD, attributes, + traceState: traceState.set(SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING, '1'), }; } @@ -112,6 +116,7 @@ export class SentrySampler implements Sampler { return { decision: SamplingDecision.NOT_RECORD, attributes, + traceState: traceState.set(SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING, '1'), }; } diff --git a/packages/opentelemetry/src/spanProcessor.ts b/packages/opentelemetry/src/spanProcessor.ts index c0a0accf4b3c..5dedcb464d58 100644 --- a/packages/opentelemetry/src/spanProcessor.ts +++ b/packages/opentelemetry/src/spanProcessor.ts @@ -20,7 +20,7 @@ function onSpanStart(span: Span, parentContext: Context): void { let scopes = getScopesFromContext(parentContext); // We need access to the parent span in order to be able to move up the span tree for breadcrumbs - if (parentSpan) { + if (parentSpan && !parentSpan.spanContext().isRemote) { addChildSpanToSpan(parentSpan, span); } diff --git a/packages/opentelemetry/src/trace.ts b/packages/opentelemetry/src/trace.ts index 29491805bd5a..945e60a811f1 100644 --- a/packages/opentelemetry/src/trace.ts +++ b/packages/opentelemetry/src/trace.ts @@ -2,21 +2,22 @@ import type { Context, Span, SpanContext, SpanOptions, Tracer } from '@opentelem import { TraceFlags } from '@opentelemetry/api'; import { context } from '@opentelemetry/api'; import { SpanStatusCode, trace } from '@opentelemetry/api'; -import { TraceState, suppressTracing } from '@opentelemetry/core'; +import { suppressTracing } from '@opentelemetry/core'; import { SDK_VERSION, SEMANTIC_ATTRIBUTE_SENTRY_OP, + continueTrace as baseContinueTrace, getClient, getCurrentScope, + getDynamicSamplingContextFromClient, getRootSpan, handleCallbackErrors, } from '@sentry/core'; import type { Client, Scope } from '@sentry/types'; -import { dynamicSamplingContextToSentryBaggageHeader } from '@sentry/utils'; -import { SENTRY_TRACE_STATE_DSC } from './constants'; +import { continueTraceAsRemoteSpan, getSamplingDecision, makeTraceState } from './propagator'; import type { OpenTelemetryClient, OpenTelemetrySpanContext } from './types'; -import { getContextFromScope } from './utils/contextData'; +import { getContextFromScope, getScopesFromContext } from './utils/contextData'; import { getDynamicSamplingContextFromSpan } from './utils/dynamicSamplingContext'; /** @@ -167,33 +168,65 @@ function ensureTimestampInMilliseconds(timestamp: number): number { function getContext(scope: Scope | undefined, forceTransaction: boolean | undefined): Context { const ctx = getContextForScope(scope); + const actualScope = getScopesFromContext(ctx)?.scope; - if (!forceTransaction) { - return ctx; - } - - // Else we need to "fix" the context to have no parent span const parentSpan = trace.getSpan(ctx); - // If there is no parent span, all good, nothing to do! + // In the case that we have no parent span, we need to "simulate" one to ensure the propagation context is correct if (!parentSpan) { + const client = getClient(); + + if (actualScope && client) { + const propagationContext = actualScope.getPropagationContext(); + const dynamicSamplingContext = + propagationContext.dsc || getDynamicSamplingContextFromClient(propagationContext.traceId, client); + + // We store the DSC as OTEL trace state on the span context + const traceState = makeTraceState({ + parentSpanId: propagationContext.parentSpanId, + dsc: dynamicSamplingContext, + sampled: propagationContext.sampled, + }); + + const spanContext: SpanContext = { + traceId: propagationContext.traceId, + spanId: propagationContext.parentSpanId || propagationContext.spanId, + isRemote: true, + traceFlags: propagationContext.sampled ? TraceFlags.SAMPLED : TraceFlags.NONE, + traceState, + }; + + // Add remote parent span context, + return trace.setSpanContext(ctx, spanContext); + } + + // if we have no scope or client, we just return the context as-is + return ctx; + } + + // If we don't want to force a transaction, and we have a parent span, all good, we just return as-is! + if (!forceTransaction) { return ctx; } + // Else, if we do have a parent span but want to force a transaction, we have to simulate a "root" context + // Else, we need to do two things: // 1. Unset the parent span from the context, so we'll create a new root span // 2. Ensure the propagation context is correct, so we'll continue from the parent span const ctxWithoutSpan = trace.deleteSpan(ctx); - const { spanId, traceId, traceFlags } = parentSpan.spanContext(); - // eslint-disable-next-line no-bitwise - const sampled = Boolean(traceFlags & TraceFlags.SAMPLED); + const { spanId, traceId } = parentSpan.spanContext(); + const sampled = getSamplingDecision(parentSpan.spanContext()); const rootSpan = getRootSpan(parentSpan); const dsc = getDynamicSamplingContextFromSpan(rootSpan); - const dscString = dynamicSamplingContextToSentryBaggageHeader(dsc); - const traceState = dscString ? new TraceState().set(SENTRY_TRACE_STATE_DSC, dscString) : undefined; + const traceState = makeTraceState({ + dsc, + parentSpanId: spanId, + sampled, + }); const spanContext: SpanContext = { traceId, @@ -218,3 +251,20 @@ function getContextForScope(scope?: Scope): Context { return context.active(); } + +/** + * Continue a trace from `sentry-trace` and `baggage` values. + * These values can be obtained from incoming request headers, or in the browser from `` + * and `` HTML tags. + * + * Spans started with `startSpan`, `startSpanManual` and `startInactiveSpan`, within the callback will automatically + * be attached to the incoming trace. + * + * This is a custom version of `continueTrace` that is used in OTEL-powered environments. + * It propagates the trace as a remote span, in addition to setting it on the propagation context. + */ +export function continueTrace(options: Parameters[0], callback: () => T): T { + return baseContinueTrace(options, () => { + return continueTraceAsRemoteSpan(context.active(), options, callback); + }); +} diff --git a/packages/opentelemetry/src/utils/dynamicSamplingContext.ts b/packages/opentelemetry/src/utils/dynamicSamplingContext.ts index cda72c11cac5..8fcedf65c6a4 100644 --- a/packages/opentelemetry/src/utils/dynamicSamplingContext.ts +++ b/packages/opentelemetry/src/utils/dynamicSamplingContext.ts @@ -1,4 +1,3 @@ -import { TraceFlags } from '@opentelemetry/api'; import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, @@ -8,6 +7,7 @@ import { import type { DynamicSamplingContext } from '@sentry/types'; import { baggageHeaderToDynamicSamplingContext } from '@sentry/utils'; import { SENTRY_TRACE_STATE_DSC } from '../constants'; +import { getSamplingDecision } from '../propagator'; import type { AbstractSpan } from '../types'; import { spanHasAttributes, spanHasName } from './spanTypes'; @@ -51,10 +51,10 @@ export function getDynamicSamplingContextFromSpan(span: AbstractSpan): Readonly< dsc.transaction = name; } - // TODO: Once we aligned span types, use spanIsSampled() from core instead - // eslint-disable-next-line no-bitwise - const sampled = Boolean(span.spanContext().traceFlags & TraceFlags.SAMPLED); - dsc.sampled = String(sampled); + const sampled = getSamplingDecision(span.spanContext()); + if (sampled != null) { + dsc.sampled = String(sampled); + } client.emit('createDsc', dsc); diff --git a/packages/opentelemetry/test/integration/scope.test.ts b/packages/opentelemetry/test/integration/scope.test.ts index 36c794b1dbb0..7742fcf06767 100644 --- a/packages/opentelemetry/test/integration/scope.test.ts +++ b/packages/opentelemetry/test/integration/scope.test.ts @@ -72,6 +72,8 @@ describe('Integration | Scope', () => { trace: { span_id: spanId, trace_id: traceId, + // local span ID from propagation context + ...(enableTracing ? { parent_span_id: expect.any(String) } : undefined), }, }, }), @@ -117,6 +119,8 @@ describe('Integration | Scope', () => { status: 'ok', trace_id: traceId, origin: 'manual', + // local span ID from propagation context + parent_span_id: expect.any(String), }, }), spans: [], @@ -211,6 +215,8 @@ describe('Integration | Scope', () => { ? { span_id: spanId1, trace_id: traceId1, + // local span ID from propagation context + ...(enableTracing ? { parent_span_id: expect.any(String) } : undefined), } : expect.any(Object), }), @@ -236,7 +242,8 @@ describe('Integration | Scope', () => { ? { span_id: spanId2, trace_id: traceId2, - parent_span_id: undefined, + // local span ID from propagation context + ...(enableTracing ? { parent_span_id: expect.any(String) } : undefined), } : expect.any(Object), }), @@ -327,16 +334,21 @@ describe('Integration | Scope', () => { await client.flush(); + expect(spanId1).toBeDefined(); + expect(spanId2).toBeDefined(); + expect(traceId1).toBeDefined(); + expect(traceId2).toBeDefined(); + expect(beforeSend).toHaveBeenCalledTimes(2); expect(beforeSend).toHaveBeenCalledWith( expect.objectContaining({ contexts: expect.objectContaining({ - trace: spanId1 - ? { - span_id: spanId1, - trace_id: traceId1, - } - : expect.any(Object), + trace: { + span_id: spanId1, + trace_id: traceId1, + // local span ID from propagation context + ...(enableTracing ? { parent_span_id: expect.any(String) } : undefined), + }, }), tags: { tag1: 'val1', @@ -358,13 +370,12 @@ describe('Integration | Scope', () => { expect(beforeSend).toHaveBeenCalledWith( expect.objectContaining({ contexts: expect.objectContaining({ - trace: spanId2 - ? { - span_id: spanId2, - trace_id: traceId2, - parent_span_id: undefined, - } - : expect.any(Object), + trace: { + span_id: spanId2, + trace_id: traceId2, + // local span ID from propagation context + ...(enableTracing ? { parent_span_id: expect.any(String) } : undefined), + }, }), tags: { tag1: 'val1', diff --git a/packages/opentelemetry/test/integration/transactions.test.ts b/packages/opentelemetry/test/integration/transactions.test.ts index 4f021e77d456..ba382a5081d9 100644 --- a/packages/opentelemetry/test/integration/transactions.test.ts +++ b/packages/opentelemetry/test/integration/transactions.test.ts @@ -280,6 +280,8 @@ describe('Integration | Transactions', () => { status: 'ok', trace_id: expect.any(String), origin: 'auto.test', + // local span ID from propagation context + parent_span_id: expect.any(String), }, }), spans: [expect.any(Object), expect.any(Object)], @@ -325,6 +327,8 @@ describe('Integration | Transactions', () => { status: 'ok', trace_id: expect.any(String), origin: 'manual', + // local span ID from propagation context + parent_span_id: expect.any(String), }, }), spans: [expect.any(Object), expect.any(Object)], diff --git a/packages/opentelemetry/test/propagator.test.ts b/packages/opentelemetry/test/propagator.test.ts index 50629e965713..66d846085cfe 100644 --- a/packages/opentelemetry/test/propagator.test.ts +++ b/packages/opentelemetry/test/propagator.test.ts @@ -11,7 +11,7 @@ import { suppressTracing } from '@opentelemetry/core'; import { addTracingExtensions, withScope } from '@sentry/core'; import { SENTRY_BAGGAGE_HEADER, SENTRY_SCOPES_CONTEXT_KEY, SENTRY_TRACE_HEADER } from '../src/constants'; -import { SentryPropagator, makeTraceState } from '../src/propagator'; +import { SentryPropagator, getSamplingDecision, makeTraceState } from '../src/propagator'; import { getScopesFromContext } from '../src/utils/contextData'; import { cleanupOtel, mockSdkInit } from './helpers/mockSdkInit'; @@ -74,6 +74,25 @@ describe('SentryPropagator', () => { 'sentry-public_key=abc', 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', ], + 'd4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92', + ], + [ + 'uses remote spanContext with trace state & without DSC for unsampled remote span', + { + traceId: 'd4cda95b652f4a1592b449d5929fda1b', + spanId: '6e0c63257de34c92', + traceFlags: TraceFlags.NONE, + isRemote: true, + traceState: makeTraceState({ + sampled: false, + }), + }, + [ + 'sentry-environment=production', + 'sentry-release=1.0.0', + 'sentry-public_key=abc', + 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', + ], 'd4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-0', ], [ @@ -269,6 +288,7 @@ describe('SentryPropagator', () => { 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', ], 'd4cda95b652f4a1592b449d5929fda1b-{{spanId}}-1', + true, ], [ 'continues a remote trace with dsc', @@ -302,6 +322,7 @@ describe('SentryPropagator', () => { 'sentry-replay_id=dsc_replay_id', ], 'd4cda95b652f4a1592b449d5929fda1b-{{spanId}}-1', + true, ], [ 'continues an unsampled remote trace without dsc', @@ -317,7 +338,28 @@ describe('SentryPropagator', () => { 'sentry-public_key=abc', 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', ], + 'd4cda95b652f4a1592b449d5929fda1b-{{spanId}}-1', + undefined, + ], + [ + 'continues an unsampled remote trace with sampled trace state & without dsc', + { + traceId: 'd4cda95b652f4a1592b449d5929fda1b', + spanId: '6e0c63257de34c92', + traceFlags: TraceFlags.NONE, + isRemote: true, + traceState: makeTraceState({ + sampled: false, + }), + }, + [ + 'sentry-environment=production', + 'sentry-release=1.0.0', + 'sentry-public_key=abc', + 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', + ], 'd4cda95b652f4a1592b449d5929fda1b-{{spanId}}-0', + false, ], [ 'continues an unsampled remote trace with dsc', @@ -351,6 +393,40 @@ describe('SentryPropagator', () => { 'sentry-replay_id=dsc_replay_id', ], 'd4cda95b652f4a1592b449d5929fda1b-{{spanId}}-0', + false, + ], + [ + 'continues an unsampled remote trace with dsc & sampled trace state', + { + traceId: 'd4cda95b652f4a1592b449d5929fda1b', + spanId: '6e0c63257de34c92', + traceFlags: TraceFlags.NONE, + isRemote: true, + traceState: makeTraceState({ + sampled: false, + parentSpanId: '6e0c63257de34c92', + dsc: { + transaction: 'sampled-transaction', + trace_id: 'dsc_trace_id', + public_key: 'dsc_public_key', + environment: 'dsc_environment', + release: 'dsc_release', + sample_rate: '0.5', + replay_id: 'dsc_replay_id', + }, + }), + }, + [ + 'sentry-environment=dsc_environment', + 'sentry-release=dsc_release', + 'sentry-public_key=dsc_public_key', + 'sentry-trace_id=dsc_trace_id', + 'sentry-transaction=sampled-transaction', + 'sentry-sample_rate=0.5', + 'sentry-replay_id=dsc_replay_id', + ], + 'd4cda95b652f4a1592b449d5929fda1b-{{spanId}}-0', + false, ], [ 'starts a new trace without existing dsc', @@ -366,8 +442,11 @@ describe('SentryPropagator', () => { 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', ], 'd4cda95b652f4a1592b449d5929fda1b-{{spanId}}-1', + true, ], - ])('%s', (_name, spanContext, baggage, sentryTrace) => { + ])('%s', (_name, spanContext, baggage, sentryTrace, samplingDecision) => { + expect(getSamplingDecision(spanContext)).toBe(samplingDecision); + context.with(trace.setSpanContext(ROOT_CONTEXT, spanContext), () => { trace.getTracer('test').startActiveSpan('test', span => { propagator.inject(context.active(), carrier, defaultTextMapSetter); @@ -500,6 +579,35 @@ describe('SentryPropagator', () => { traceId: 'd4cda95b652f4a1592b449d5929fda1b', traceState: makeTraceState({ parentSpanId: '6e0c63257de34c92' }), }); + expect(getSamplingDecision(trace.getSpanContext(context)!)).toBe(true); + }); + + it('sets data from negative sampled sentry trace header on span context', () => { + const sentryTraceHeader = 'd4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-0'; + carrier[SENTRY_TRACE_HEADER] = sentryTraceHeader; + const context = propagator.extract(ROOT_CONTEXT, carrier, defaultTextMapGetter); + expect(trace.getSpanContext(context)).toEqual({ + isRemote: true, + spanId: '6e0c63257de34c92', + traceFlags: TraceFlags.NONE, + traceId: 'd4cda95b652f4a1592b449d5929fda1b', + traceState: makeTraceState({ parentSpanId: '6e0c63257de34c92', sampled: false }), + }); + expect(getSamplingDecision(trace.getSpanContext(context)!)).toBe(false); + }); + + it('sets data from not sampled sentry trace header on span context', () => { + const sentryTraceHeader = 'd4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92'; + carrier[SENTRY_TRACE_HEADER] = sentryTraceHeader; + const context = propagator.extract(ROOT_CONTEXT, carrier, defaultTextMapGetter); + expect(trace.getSpanContext(context)).toEqual({ + isRemote: true, + spanId: '6e0c63257de34c92', + traceFlags: TraceFlags.NONE, + traceId: 'd4cda95b652f4a1592b449d5929fda1b', + traceState: makeTraceState({ parentSpanId: '6e0c63257de34c92' }), + }); + expect(getSamplingDecision(trace.getSpanContext(context)!)).toBe(undefined); }); it('sets data from sentry trace header on scope', () => { @@ -517,6 +625,7 @@ describe('SentryPropagator', () => { parentSpanId: '6e0c63257de34c92', dsc: {}, }); + expect(getSamplingDecision(trace.getSpanContext(context)!)).toBe(true); }); it('handles undefined sentry trace header', () => { @@ -529,6 +638,7 @@ describe('SentryPropagator', () => { traceFlags: TraceFlags.NONE, traceId: expect.any(String), }); + expect(getSamplingDecision(trace.getSpanContext(context)!)).toBe(undefined); }); it('sets data from baggage header on span context', () => { @@ -554,6 +664,7 @@ describe('SentryPropagator', () => { }, }), }); + expect(getSamplingDecision(trace.getSpanContext(context)!)).toBe(true); }); it('sets data from baggage header on scope', () => { @@ -592,6 +703,7 @@ describe('SentryPropagator', () => { traceFlags: TraceFlags.NONE, traceId: expect.any(String), }); + expect(getSamplingDecision(trace.getSpanContext(context)!)).toBe(undefined); }); it('handles when sentry-trace is an empty array', () => { diff --git a/packages/opentelemetry/test/trace.test.ts b/packages/opentelemetry/test/trace.test.ts index a868d439bd4e..27ae4a285b8a 100644 --- a/packages/opentelemetry/test/trace.test.ts +++ b/packages/opentelemetry/test/trace.test.ts @@ -11,13 +11,18 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, getClient, getCurrentScope, + getDynamicSamplingContextFromClient, getRootSpan, + spanIsSampled, + spanToJSON, withScope, } from '@sentry/core'; import type { Event, Scope } from '@sentry/types'; +import { getSamplingDecision, makeTraceState } from '../src/propagator'; -import { startInactiveSpan, startSpan, startSpanManual } from '../src/trace'; +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 { getSpanKind } from '../src/utils/getSpanKind'; import { spanHasAttributes, spanHasName } from '../src/utils/spanTypes'; @@ -932,6 +937,111 @@ describe('trace', () => { }); }); }); + + describe('propagation', () => { + it('picks up the trace context from the scope, if there is no parent', () => { + withScope(scope => { + const propagationContext = scope.getPropagationContext(); + const span = startInactiveSpan({ name: 'test span' }); + + expect(span).toBeDefined(); + expect(spanToJSON(span).trace_id).toEqual(propagationContext.traceId); + expect(spanToJSON(span).parent_span_id).toEqual(propagationContext.spanId); + expect(getDynamicSamplingContextFromSpan(span)).toEqual( + getDynamicSamplingContextFromClient(propagationContext.traceId, getClient()!), + ); + }); + }); + + it('picks up the trace context from the parent without DSC', () => { + 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, () => { + 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(getDynamicSamplingContextFromSpan(span)).toEqual({ + ...getDynamicSamplingContextFromClient(propagationContext.traceId, getClient()!), + trace_id: '12312012123120121231201212312012', + transaction: 'test span', + sampled: 'true', + sample_rate: '1', + }); + }); + }); + }); + + it('picks up the trace context from the parent with DSC', () => { + withScope(() => { + const ctx = trace.setSpanContext(ROOT_CONTEXT, { + traceId: '12312012123120121231201212312012', + spanId: '1121201211212012', + isRemote: false, + traceFlags: TraceFlags.SAMPLED, + traceState: makeTraceState({ + parentSpanId: '1121201211212011', + dsc: { + release: '1.0', + environment: 'production', + }, + }), + }); + + context.with(ctx, () => { + 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(getDynamicSamplingContextFromSpan(span)).toEqual({ + release: '1.0', + environment: 'production', + }); + }); + }); + }); + + it('picks up the trace context from a remote parent', () => { + withScope(() => { + const ctx = trace.setSpanContext(ROOT_CONTEXT, { + traceId: '12312012123120121231201212312012', + spanId: '1121201211212012', + isRemote: true, + traceFlags: TraceFlags.SAMPLED, + traceState: makeTraceState({ + parentSpanId: '1121201211212011', + dsc: { + release: '1.0', + environment: 'production', + }, + }), + }); + + context.with(ctx, () => { + 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(getDynamicSamplingContextFromSpan(span)).toEqual({ + release: '1.0', + environment: 'production', + }); + }); + }); + }); + }); }); describe('trace (tracing disabled)', () => { @@ -1243,6 +1353,152 @@ describe('trace (sampling)', () => { }); }); +describe('continueTrace', () => { + beforeEach(() => { + mockSdkInit({ enableTracing: true }); + }); + + afterEach(() => { + cleanupOtel(); + }); + + it('works without trace & baggage data', () => { + const scope = continueTrace({ sentryTrace: undefined, baggage: undefined }, () => { + const span = getActiveSpan()!; + expect(span).toBeDefined(); + expect(spanToJSON(span)).toEqual({ + span_id: '', + trace_id: expect.any(String), + }); + expect(getSamplingDecision(span.spanContext())).toBe(undefined); + expect(spanIsSampled(span)).toBe(false); + + return getCurrentScope(); + }); + + expect(scope.getPropagationContext()).toEqual({ + sampled: undefined, + spanId: expect.any(String), + traceId: expect.any(String), + }); + + expect(scope.getScopeData().sdkProcessingMetadata).toEqual({}); + }); + + it('works with trace data', () => { + const scope = continueTrace( + { + sentryTrace: '12312012123120121231201212312012-1121201211212012-0', + baggage: undefined, + }, + () => { + const span = getActiveSpan()!; + expect(span).toBeDefined(); + expect(spanToJSON(span)).toEqual({ + span_id: '1121201211212012', + trace_id: '12312012123120121231201212312012', + }); + expect(getSamplingDecision(span.spanContext())).toBe(false); + expect(spanIsSampled(span)).toBe(false); + + return getCurrentScope(); + }, + ); + + expect(scope.getPropagationContext()).toEqual({ + dsc: {}, // DSC should be an empty object (frozen), because there was an incoming trace + sampled: false, + parentSpanId: '1121201211212012', + spanId: expect.any(String), + traceId: '12312012123120121231201212312012', + }); + + expect(scope.getScopeData().sdkProcessingMetadata).toEqual({}); + }); + + it('works with trace & baggage data', () => { + const scope = continueTrace( + { + sentryTrace: '12312012123120121231201212312012-1121201211212012-1', + baggage: 'sentry-version=1.0,sentry-environment=production', + }, + () => { + const span = getActiveSpan()!; + expect(span).toBeDefined(); + expect(spanToJSON(span)).toEqual({ + span_id: '1121201211212012', + trace_id: '12312012123120121231201212312012', + }); + expect(getSamplingDecision(span.spanContext())).toBe(true); + expect(spanIsSampled(span)).toBe(true); + + return getCurrentScope(); + }, + ); + + expect(scope.getPropagationContext()).toEqual({ + dsc: { + environment: 'production', + version: '1.0', + }, + sampled: true, + parentSpanId: '1121201211212012', + spanId: expect.any(String), + traceId: '12312012123120121231201212312012', + }); + + expect(scope.getScopeData().sdkProcessingMetadata).toEqual({}); + }); + + it('works with trace & 3rd party baggage data', () => { + const scope = continueTrace( + { + sentryTrace: '12312012123120121231201212312012-1121201211212012-1', + baggage: 'sentry-version=1.0,sentry-environment=production,dogs=great,cats=boring', + }, + () => { + const span = getActiveSpan()!; + expect(span).toBeDefined(); + expect(spanToJSON(span)).toEqual({ + span_id: '1121201211212012', + trace_id: '12312012123120121231201212312012', + }); + expect(getSamplingDecision(span.spanContext())).toBe(true); + expect(spanIsSampled(span)).toBe(true); + + return getCurrentScope(); + }, + ); + + expect(scope.getPropagationContext()).toEqual({ + dsc: { + environment: 'production', + version: '1.0', + }, + sampled: true, + parentSpanId: '1121201211212012', + spanId: expect.any(String), + traceId: '12312012123120121231201212312012', + }); + + expect(scope.getScopeData().sdkProcessingMetadata).toEqual({}); + }); + + it('returns response of callback', () => { + const result = continueTrace( + { + sentryTrace: '12312012123120121231201212312012-1121201211212012-0', + baggage: undefined, + }, + () => { + return 'aha'; + }, + ); + + expect(result).toEqual('aha'); + }); +}); + function getSpanName(span: AbstractSpan): string | undefined { return spanHasName(span) ? span.name : undefined; }