diff --git a/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts b/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts index f5697c937660..4ec29414868c 100644 --- a/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts +++ b/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts @@ -86,9 +86,7 @@ test('Should not propagate baggage and ignore original 3rd party baggage entries }); }); -// TODO(v8): Fix this test -// eslint-disable-next-line jest/no-disabled-tests -test.skip('Should populate and propagate sentry baggage if sentry-trace header does not exist', async () => { +test('Should populate and propagate sentry baggage if sentry-trace header does not exist', async () => { const runner = createRunner(__dirname, '..', 'server.ts').start(); const response = await runner.makeRequest('get', '/test/express'); @@ -105,9 +103,7 @@ test.skip('Should populate and propagate sentry baggage if sentry-trace header d }); }); -// TODO(v8): Fix this test -// eslint-disable-next-line jest/no-disabled-tests -test.skip('Should populate Sentry and ignore 3rd party content if sentry-trace header does not exist', async () => { +test('Should populate Sentry and ignore 3rd party content if sentry-trace header does not exist', async () => { const runner = createRunner(__dirname, '..', 'server.ts').start(); const response = await runner.makeRequest('get', '/test/express', { diff --git a/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/server.ts b/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/server.ts index 5d787fd87d92..a4a9bf108a95 100644 --- a/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/server.ts +++ b/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/server.ts @@ -26,9 +26,8 @@ Sentry.setUser({ id: 'user123' }); app.use(cors()); app.get('/test/express', (_req, res) => { - // eslint-disable-next-line deprecation/deprecation - const transaction = Sentry.getCurrentScope().getTransaction(); - const traceId = transaction?.spanContext().traceId; + const span = Sentry.getActiveSpan(); + const traceId = span?.spanContext().traceId; const headers = http.get('http://somewhere.not.sentry/').getHeaders(); if (traceId) { headers['baggage'] = (headers['baggage'] as string).replace(traceId, '__SENTRY_TRACE_ID__'); diff --git a/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/test.ts b/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/test.ts index fcc2f9da939d..5a052a454b56 100644 --- a/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/test.ts +++ b/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/test.ts @@ -5,21 +5,28 @@ afterAll(() => { cleanupChildProcesses(); }); -// TODO(v8): Fix this test -// eslint-disable-next-line jest/no-disabled-tests -test.skip('should attach a baggage header to an outgoing request.', async () => { +test('should attach a baggage header to an outgoing request.', async () => { const runner = createRunner(__dirname, 'server.ts').start(); const response = await runner.makeRequest('get', '/test/express'); expect(response).toBeDefined(); + + const baggage = response?.test_data.baggage?.split(',').sort(); + + expect(baggage).toEqual([ + 'sentry-environment=prod', + 'sentry-public_key=public', + 'sentry-release=1.0', + 'sentry-sample_rate=1', + 'sentry-sampled=true', + 'sentry-trace_id=__SENTRY_TRACE_ID__', + 'sentry-transaction=GET%20%2Ftest%2Fexpress', + ]); + expect(response).toMatchObject({ test_data: { host: 'somewhere.not.sentry', - baggage: - 'sentry-environment=prod,sentry-release=1.0,sentry-public_key=public' + - ',sentry-trace_id=__SENTRY_TRACE_ID__,sentry-sample_rate=1,sentry-transaction=GET%20%2Ftest%2Fexpress' + - ',sentry-sampled=true', }, }); }); diff --git a/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts b/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts index f4c071e58611..9af5d4456c89 100644 --- a/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts +++ b/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts @@ -5,9 +5,7 @@ afterAll(() => { cleanupChildProcesses(); }); -// TODO(v8): Fix this test -// eslint-disable-next-line jest/no-disabled-tests -test.skip('should ignore sentry-values in `baggage` header of a third party vendor and overwrite them with incoming DSC', async () => { +test('should ignore sentry-values in `baggage` header of a third party vendor and overwrite them with incoming DSC', async () => { const runner = createRunner(__dirname, 'server.ts').start(); const response = await runner.makeRequest('get', '/test/express', { @@ -16,34 +14,52 @@ test.skip('should ignore sentry-values in `baggage` header of a third party vend }); expect(response).toBeDefined(); + + const baggage = response?.test_data.baggage?.split(',').sort(); + expect(response).toMatchObject({ test_data: { host: 'somewhere.not.sentry', - baggage: [ - 'other=vendor,foo=bar,third=party,sentry-release=9.9.9,sentry-environment=staging,sentry-sample_rate=0.54,last=item', - 'sentry-release=2.1.0,sentry-environment=myEnv', - ], }, }); + + expect(baggage).toEqual([ + 'foo=bar', + 'last=item', + 'other=vendor', + 'sentry-environment=myEnv', + 'sentry-release=2.1.0', + 'sentry-sample_rate=0.54', + 'third=party', + ]); }); -// TODO(v8): Fix this test -// eslint-disable-next-line jest/no-disabled-tests -test.skip('should ignore sentry-values in `baggage` header of a third party vendor and overwrite them with new DSC', async () => { +test('should ignore sentry-values in `baggage` header of a third party vendor and overwrite them with new DSC', async () => { const runner = createRunner(__dirname, 'server.ts').start(); const response = await runner.makeRequest('get', '/test/express'); expect(response).toBeDefined(); + + const baggage = response?.test_data.baggage?.split(',').sort(); + expect(response).toMatchObject({ test_data: { host: 'somewhere.not.sentry', - baggage: [ - 'other=vendor,foo=bar,third=party,sentry-release=9.9.9,sentry-environment=staging,sentry-sample_rate=0.54,last=item', - expect.stringMatching( - /sentry-environment=prod,sentry-release=1\.0,sentry-public_key=public,sentry-trace_id=[0-9a-f]{32},sentry-sample_rate=1,sentry-transaction=GET%20%2Ftest%2Fexpress/, - ), - ], }, }); + + expect(baggage).toEqual([ + 'foo=bar', + 'last=item', + 'other=vendor', + 'sentry-environment=prod', + 'sentry-public_key=public', + 'sentry-release=1.0', + 'sentry-sample_rate=1', + 'sentry-sampled=true', + expect.stringMatching(/sentry-trace_id=[0-9a-f]{32}/), + 'sentry-transaction=GET%20%2Ftest%2Fexpress', + 'third=party', + ]); }); diff --git a/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/test.ts b/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/test.ts index bc84e5967362..dd3c0f8cddd7 100644 --- a/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/test.ts +++ b/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/test.ts @@ -5,9 +5,7 @@ afterAll(() => { cleanupChildProcesses(); }); -// TODO(v8): Fix this test -// eslint-disable-next-line jest/no-disabled-tests -test.skip('should merge `baggage` header of a third party vendor with the Sentry DSC baggage items', async () => { +test('should merge `baggage` header of a third party vendor with the Sentry DSC baggage items', async () => { const runner = createRunner(__dirname, 'server.ts').start(); const response = await runner.makeRequest('get', '/test/express', { @@ -19,7 +17,7 @@ test.skip('should merge `baggage` header of a third party vendor with the Sentry expect(response).toMatchObject({ test_data: { host: 'somewhere.not.sentry', - baggage: ['other=vendor,foo=bar,third=party', 'sentry-release=2.0.0,sentry-environment=myEnv'], + baggage: 'other=vendor,foo=bar,third=party,sentry-release=2.0.0,sentry-environment=myEnv', }, }); }); diff --git a/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/server.ts b/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/server.ts index 133d5ee257b2..ed8f7487a9c3 100644 --- a/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/server.ts +++ b/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/server.ts @@ -1,5 +1,4 @@ import { loggingTransport } from '@sentry-internal/node-integration-tests'; -import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/core'; import * as Sentry from '@sentry/node'; export type TestAPIResponse = { test_data: { host: string; 'sentry-trace': string; baggage: string } }; @@ -31,10 +30,6 @@ Sentry.setUser({ id: 'user123' }); app.use(cors()); app.get('/test/express', (_req, res) => { - // eslint-disable-next-line deprecation/deprecation - const transaction = Sentry.getCurrentScope().getTransaction(); - transaction?.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); - const headers = http.get('http://somewhere.not.sentry/').getHeaders(); // Responding with the headers outgoing request headers back to the assertions. res.send({ test_data: headers }); diff --git a/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/test.ts b/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/test.ts index 8be934aee9cf..1001d0839aea 100644 --- a/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/test.ts +++ b/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/test.ts @@ -5,9 +5,7 @@ afterAll(() => { cleanupChildProcesses(); }); -// TODO(v8): Fix this test -// eslint-disable-next-line jest/no-disabled-tests -test.skip('Includes transaction in baggage if the transaction name is parameterized', async () => { +test('Includes transaction in baggage if the transaction name is parameterized', async () => { const runner = createRunner(__dirname, 'server.ts').start(); const response = await runner.makeRequest('get', '/test/express'); diff --git a/dev-packages/node-integration-tests/suites/tracing/spans/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/spans/scenario.ts index 8af365587b06..5220701b1b4d 100644 --- a/dev-packages/node-integration-tests/suites/tracing/spans/scenario.ts +++ b/dev-packages/node-integration-tests/suites/tracing/spans/scenario.ts @@ -12,9 +12,21 @@ import * as http from 'http'; // eslint-disable-next-line @typescript-eslint/no-floating-promises Sentry.startSpan({ name: 'test_transaction' }, async () => { - http.get(`${process.env.SERVER_URL}/api/v0`); - http.get(`${process.env.SERVER_URL}/api/v1`); - - // Give it a tick to resolve... - await new Promise(resolve => setTimeout(resolve, 100)); + await makeHttpRequest(`${process.env.SERVER_URL}/api/v0`); + await makeHttpRequest(`${process.env.SERVER_URL}/api/v1`); }); + +function makeHttpRequest(url: string): Promise { + return new Promise(resolve => { + http + .request(url, httpRes => { + httpRes.on('data', () => { + // we don't care about data + }); + httpRes.on('end', () => { + resolve(); + }); + }) + .end(); + }); +} diff --git a/packages/core/test/lib/utils/spanUtils.test.ts b/packages/core/test/lib/utils/spanUtils.test.ts index b802c7bfb69f..cb36e701cf33 100644 --- a/packages/core/test/lib/utils/spanUtils.test.ts +++ b/packages/core/test/lib/utils/spanUtils.test.ts @@ -234,7 +234,7 @@ describe('getRootSpan', () => { expect(getRootSpan(root)).toBe(root); }); - it('returns the root span of a child span xxx', () => { + it('returns the root span of a child span', () => { startSpan({ name: 'outer' }, root => { startSpan({ name: 'inner' }, inner => { expect(getRootSpan(inner)).toBe(root); diff --git a/packages/opentelemetry/src/propagator.ts b/packages/opentelemetry/src/propagator.ts index b8de206d84f1..d96f19c16b7c 100644 --- a/packages/opentelemetry/src/propagator.ts +++ b/packages/opentelemetry/src/propagator.ts @@ -1,9 +1,10 @@ -import type { Baggage, Context, SpanContext, TextMapGetter, TextMapSetter } from '@opentelemetry/api'; +import type { Baggage, Context, Span, 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 { SemanticAttributes } from '@opentelemetry/semantic-conventions'; import type { continueTrace } from '@sentry/core'; +import { getRootSpan } from '@sentry/core'; import { spanToJSON } from '@sentry/core'; import { getClient, getCurrentScope, getDynamicSamplingContextFromClient, getIsolationScope } from '@sentry/core'; import type { DynamicSamplingContext, Options, PropagationContext } from '@sentry/types'; @@ -14,6 +15,7 @@ import { dynamicSamplingContextToSentryBaggageHeader, generateSentryTraceHeader, logger, + parseBaggageHeader, propagationContextFromHeaders, stringMatchesSomePattern, } from '@sentry/utils'; @@ -27,18 +29,27 @@ 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'; /** Get the Sentry propagation context from a span context. */ -export function getPropagationContextFromSpanContext(spanContext: SpanContext): PropagationContext { +export function getPropagationContextFromSpan(span: Span): PropagationContext { + const spanContext = span.spanContext(); const { traceId, spanId, traceState } = spanContext; + // When we have a dsc trace state, it means this came from the incoming trace + // Then this takes presedence over the root span const dscString = traceState ? traceState.get(SENTRY_TRACE_STATE_DSC) : undefined; - const dsc = dscString ? baggageHeaderToDynamicSamplingContext(dscString) : undefined; + const traceStateDsc = dscString ? baggageHeaderToDynamicSamplingContext(dscString) : undefined; + const parentSpanId = traceState ? traceState.get(SENTRY_TRACE_STATE_PARENT_SPAN_ID) : undefined; const sampled = getSamplingDecision(spanContext); + // No trace state? --> Take DSC from root span + const dsc = traceStateDsc || getDynamicSamplingContextFromSpan(getRootSpan(span)); + return { traceId, spanId, @@ -85,10 +96,21 @@ export class SentryPropagator extends W3CBaggagePropagator { return; } + const existingBaggageHeader = getExistingBaggage(carrier); let baggage = propagation.getBaggage(context) || propagation.createBaggage({}); const { dynamicSamplingContext, traceId, spanId, sampled } = getInjectionData(context); + if (existingBaggageHeader) { + const baggageEntries = parseBaggageHeader(existingBaggageHeader); + + if (baggageEntries) { + Object.entries(baggageEntries).forEach(([key, value]) => { + baggage = baggage.setEntry(key, { value }); + }); + } + } + if (dynamicSamplingContext) { baggage = Object.entries(dynamicSamplingContext).reduce((b, [dscKey, dscValue]) => { if (dscValue) { @@ -196,7 +218,8 @@ function getInjectionData(context: Context): { // If we have a local span, we can just pick everything from it if (span && !spanIsRemote) { const spanContext = span.spanContext(); - const propagationContext = getPropagationContextFromSpanContext(spanContext); + + const propagationContext = getPropagationContextFromSpan(span); const dynamicSamplingContext = getDynamicSamplingContext(propagationContext, spanContext.traceId); return { dynamicSamplingContext, @@ -220,9 +243,9 @@ function getInjectionData(context: Context): { } // Else, we look at the remote span context - const spanContext = trace.getSpanContext(context); - if (spanContext) { - const propagationContext = getPropagationContextFromSpanContext(spanContext); + if (span) { + const spanContext = span.spanContext(); + const propagationContext = getPropagationContextFromSpan(span); const dynamicSamplingContext = getDynamicSamplingContext(propagationContext, spanContext.traceId); return { @@ -301,40 +324,12 @@ export function continueTraceAsRemoteSpan( 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; +/** Try to get the existing baggage header so we can merge this in. */ +function getExistingBaggage(carrier: unknown): string | undefined { + try { + const baggage = (carrier as Record)[SENTRY_BAGGAGE_HEADER]; + return Array.isArray(baggage) ? baggage.join(',') : baggage; + } catch { + return undefined; } - - return undefined; } diff --git a/packages/opentelemetry/src/sampler.ts b/packages/opentelemetry/src/sampler.ts index 3d70a7c35621..22ad9b181773 100644 --- a/packages/opentelemetry/src/sampler.ts +++ b/packages/opentelemetry/src/sampler.ts @@ -1,4 +1,4 @@ -import type { Attributes, Context, SpanContext } from '@opentelemetry/api'; +import type { Attributes, Context, Span } from '@opentelemetry/api'; import { isSpanContextValid, trace } from '@opentelemetry/api'; import { TraceState } from '@opentelemetry/core'; import type { Sampler, SamplingResult } from '@opentelemetry/sdk-trace-base'; @@ -9,7 +9,8 @@ import { logger } from '@sentry/utils'; import { SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING } from './constants'; import { DEBUG_BUILD } from './debug-build'; -import { getPropagationContextFromSpanContext, getSamplingDecision } from './propagator'; +import { getPropagationContextFromSpan } from './propagator'; +import { getSamplingDecision } from './utils/getSamplingDecision'; import { setIsSetup } from './utils/setupCheck'; /** @@ -38,16 +39,17 @@ export class SentrySampler implements Sampler { return { decision: SamplingDecision.NOT_RECORD }; } - const parentContext = trace.getSpanContext(context); + const parentSpan = trace.getSpan(context); + const parentContext = parentSpan?.spanContext(); const traceState = parentContext?.traceState || new TraceState(); let parentSampled: boolean | undefined = undefined; // Only inherit sample rate if `traceId` is the same // Note for testing: `isSpanContextValid()` checks the format of the traceId/spanId, so we need to pass valid ones - if (parentContext && isSpanContextValid(parentContext) && parentContext.traceId === traceId) { + if (parentSpan && parentContext && isSpanContextValid(parentContext) && parentContext.traceId === traceId) { if (parentContext.isRemote) { - parentSampled = getParentRemoteSampled(parentContext); + parentSampled = getParentRemoteSampled(parentSpan); DEBUG_BUILD && logger.log(`[Tracing] Inheriting remote parent's sampled decision for ${spanName}: ${parentSampled}`); } else { @@ -90,9 +92,9 @@ export class SentrySampler implements Sampler { } } -function getParentRemoteSampled(spanContext: SpanContext): boolean | undefined { - const traceId = spanContext.traceId; - const traceparentData = getPropagationContextFromSpanContext(spanContext); +function getParentRemoteSampled(parentSpan: Span): boolean | undefined { + const traceId = parentSpan.spanContext().traceId; + const traceparentData = getPropagationContextFromSpan(parentSpan); // Only inherit sampled if `traceId` is the same return traceparentData && traceId === traceparentData.traceId ? traceparentData.sampled : undefined; diff --git a/packages/opentelemetry/src/trace.ts b/packages/opentelemetry/src/trace.ts index 31bdb46eec67..02d8a1216692 100644 --- a/packages/opentelemetry/src/trace.ts +++ b/packages/opentelemetry/src/trace.ts @@ -9,17 +9,17 @@ import { continueTrace as baseContinueTrace, getClient, getCurrentScope, - getDynamicSamplingContextFromClient, getRootSpan, handleCallbackErrors, spanToJSON, } from '@sentry/core'; import type { Client, Scope } from '@sentry/types'; -import { continueTraceAsRemoteSpan, getSamplingDecision, makeTraceState } from './propagator'; +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'; /** * Wraps a function with a transaction/span and finishes the span after the function is done. @@ -186,13 +186,12 @@ function getContext(scope: Scope | undefined, forceTransaction: boolean | undefi 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, + // Not defined yet, we want to pick this up on-demand only + dsc: undefined, sampled: propagationContext.sampled, }); @@ -227,6 +226,8 @@ function getContext(scope: Scope | undefined, forceTransaction: boolean | undefi const { spanId, traceId } = parentSpan.spanContext(); const sampled = getSamplingDecision(parentSpan.spanContext()); + // In this case, when we are forcing a transaction, we want to treat this like continuing an incoming trace + // so we set the traceState according to the root span const rootSpan = getRootSpan(parentSpan); const dsc = getDynamicSamplingContextFromSpan(rootSpan); diff --git a/packages/opentelemetry/src/utils/dynamicSamplingContext.ts b/packages/opentelemetry/src/utils/dynamicSamplingContext.ts index 8fcedf65c6a4..3e4f9f67ae84 100644 --- a/packages/opentelemetry/src/utils/dynamicSamplingContext.ts +++ b/packages/opentelemetry/src/utils/dynamicSamplingContext.ts @@ -7,8 +7,9 @@ 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 { getSamplingDecision } from './getSamplingDecision'; +import { parseSpanDescription } from './parseSpanDescription'; import { spanHasAttributes, spanHasName } from './spanTypes'; /** @@ -45,10 +46,12 @@ export function getDynamicSamplingContextFromSpan(span: AbstractSpan): Readonly< // 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]; - const name = spanHasName(span) ? span.name : ''; - if (source !== 'url' && name) { - dsc.transaction = name; + // 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()); diff --git a/packages/opentelemetry/src/utils/getSamplingDecision.ts b/packages/opentelemetry/src/utils/getSamplingDecision.ts new file mode 100644 index 000000000000..05e2aba26525 --- /dev/null +++ b/packages/opentelemetry/src/utils/getSamplingDecision.ts @@ -0,0 +1,42 @@ +import type { SpanContext } from '@opentelemetry/api'; +import { TraceFlags } from '@opentelemetry/api'; +import { baggageHeaderToDynamicSamplingContext } from '@sentry/utils'; +import { SENTRY_TRACE_STATE_DSC, SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING } from '../constants'; + +/** + * 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/test/propagator.test.ts b/packages/opentelemetry/test/propagator.test.ts index 66d846085cfe..244b0aa93d2b 100644 --- a/packages/opentelemetry/test/propagator.test.ts +++ b/packages/opentelemetry/test/propagator.test.ts @@ -11,8 +11,9 @@ 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, getSamplingDecision, makeTraceState } from '../src/propagator'; +import { SentryPropagator, makeTraceState } from '../src/propagator'; import { getScopesFromContext } from '../src/utils/contextData'; +import { getSamplingDecision } from '../src/utils/getSamplingDecision'; import { cleanupOtel, mockSdkInit } from './helpers/mockSdkInit'; beforeAll(() => { @@ -56,6 +57,7 @@ describe('SentryPropagator', () => { 'sentry-environment=production', 'sentry-release=1.0.0', 'sentry-public_key=abc', + 'sentry-sampled=true', 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', ], 'd4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-1', @@ -91,6 +93,7 @@ describe('SentryPropagator', () => { 'sentry-environment=production', 'sentry-release=1.0.0', 'sentry-public_key=abc', + 'sentry-sampled=false', 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', ], 'd4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-0', @@ -285,7 +288,10 @@ describe('SentryPropagator', () => { 'sentry-environment=production', 'sentry-release=1.0.0', 'sentry-public_key=abc', + 'sentry-sample_rate=1', + 'sentry-sampled=true', 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', + 'sentry-transaction=test', ], 'd4cda95b652f4a1592b449d5929fda1b-{{spanId}}-1', true, @@ -336,7 +342,10 @@ describe('SentryPropagator', () => { 'sentry-environment=production', 'sentry-release=1.0.0', 'sentry-public_key=abc', + 'sentry-sample_rate=1', + 'sentry-sampled=true', 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', + 'sentry-transaction=test', ], 'd4cda95b652f4a1592b449d5929fda1b-{{spanId}}-1', undefined, @@ -357,6 +366,7 @@ describe('SentryPropagator', () => { 'sentry-release=1.0.0', 'sentry-public_key=abc', 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', + 'sentry-sampled=false', ], 'd4cda95b652f4a1592b449d5929fda1b-{{spanId}}-0', false, @@ -439,6 +449,7 @@ describe('SentryPropagator', () => { 'sentry-environment=production', 'sentry-release=1.0.0', 'sentry-public_key=abc', + 'sentry-sampled=true', 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', ], 'd4cda95b652f4a1592b449d5929fda1b-{{spanId}}-1', @@ -482,7 +493,10 @@ describe('SentryPropagator', () => { 'sentry-environment=production', 'sentry-release=1.0.0', 'sentry-public_key=abc', + 'sentry-sample_rate=1', + 'sentry-sampled=true', 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', + 'sentry-transaction=test', ].sort(), ); expect(carrier[SENTRY_TRACE_HEADER]).toBe( @@ -493,6 +507,7 @@ describe('SentryPropagator', () => { }, ); + const carrier2: Record = {}; context.with( trace.setSpanContext(ROOT_CONTEXT, { traceId: 'd4cda95b652f4a1592b449d5929fda1b', @@ -509,9 +524,9 @@ describe('SentryPropagator', () => { sampled: true, }); - propagator.inject(context.active(), carrier, defaultTextMapSetter); + propagator.inject(context.active(), carrier2, defaultTextMapSetter); - expect(baggageToArray(carrier[SENTRY_BAGGAGE_HEADER])).toEqual( + expect(baggageToArray(carrier2[SENTRY_BAGGAGE_HEADER])).toEqual( [ 'sentry-environment=production', 'sentry-release=1.0.0', @@ -519,7 +534,7 @@ describe('SentryPropagator', () => { 'sentry-trace_id=TRACE_ID', ].sort(), ); - expect(carrier[SENTRY_TRACE_HEADER]).toBe('TRACE_ID-SPAN_ID-1'); + expect(carrier2[SENTRY_TRACE_HEADER]).toBe('TRACE_ID-SPAN_ID-1'); }); }, ); @@ -542,6 +557,89 @@ describe('SentryPropagator', () => { 'sentry-public_key=abc', 'sentry-environment=production', 'sentry-release=1.0.0', + 'sentry-sampled=true', + ].sort(), + ); + }); + + it('should include existing baggage header', () => { + const spanContext = { + traceId: 'd4cda95b652f4a1592b449d5929fda1b', + spanId: '6e0c63257de34c92', + traceFlags: TraceFlags.SAMPLED, + }; + + const carrier = { + other: 'header', + baggage: 'foo=bar,other=yes', + }; + const context = trace.setSpanContext(ROOT_CONTEXT, spanContext); + const baggage = propagation.createBaggage(); + propagator.inject(propagation.setBaggage(context, baggage), carrier, defaultTextMapSetter); + expect(baggageToArray(carrier[SENTRY_BAGGAGE_HEADER])).toEqual( + [ + 'foo=bar', + 'other=yes', + 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', + 'sentry-public_key=abc', + 'sentry-environment=production', + 'sentry-release=1.0.0', + 'sentry-sampled=true', + ].sort(), + ); + }); + + it('should include existing baggage array header', () => { + const spanContext = { + traceId: 'd4cda95b652f4a1592b449d5929fda1b', + spanId: '6e0c63257de34c92', + traceFlags: TraceFlags.SAMPLED, + }; + + const carrier = { + other: 'header', + baggage: ['foo=bar,other=yes', 'other2=no'], + }; + const context = trace.setSpanContext(ROOT_CONTEXT, spanContext); + const baggage = propagation.createBaggage(); + propagator.inject(propagation.setBaggage(context, baggage), carrier, defaultTextMapSetter); + expect(baggageToArray(carrier[SENTRY_BAGGAGE_HEADER])).toEqual( + [ + 'foo=bar', + 'other=yes', + 'other2=no', + 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', + 'sentry-public_key=abc', + 'sentry-environment=production', + 'sentry-release=1.0.0', + 'sentry-sampled=true', + ].sort(), + ); + }); + + it('should overwrite existing sentry baggage header', () => { + const spanContext = { + traceId: 'd4cda95b652f4a1592b449d5929fda1b', + spanId: '6e0c63257de34c92', + traceFlags: TraceFlags.SAMPLED, + }; + + const carrier = { + baggage: 'foo=bar,other=yes,sentry-release=9.9.9,sentry-other=yes', + }; + const context = trace.setSpanContext(ROOT_CONTEXT, spanContext); + const baggage = propagation.createBaggage(); + propagator.inject(propagation.setBaggage(context, baggage), carrier, defaultTextMapSetter); + expect(baggageToArray(carrier[SENTRY_BAGGAGE_HEADER])).toEqual( + [ + 'foo=bar', + 'other=yes', + 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', + 'sentry-public_key=abc', + 'sentry-environment=production', + 'sentry-other=yes', + 'sentry-release=1.0.0', + 'sentry-sampled=true', ].sort(), ); }); diff --git a/packages/opentelemetry/test/trace.test.ts b/packages/opentelemetry/test/trace.test.ts index 27ae4a285b8a..f02bd4b5673e 100644 --- a/packages/opentelemetry/test/trace.test.ts +++ b/packages/opentelemetry/test/trace.test.ts @@ -18,12 +18,13 @@ import { withScope, } from '@sentry/core'; import type { Event, Scope } from '@sentry/types'; -import { getSamplingDecision, makeTraceState } from '../src/propagator'; +import { makeTraceState } from '../src/propagator'; 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'; import { spanHasAttributes, spanHasName } from '../src/utils/spanTypes'; import { cleanupOtel, mockSdkInit } from './helpers/mockSdkInit'; @@ -947,9 +948,13 @@ describe('trace', () => { 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()!), - ); + + expect(getDynamicSamplingContextFromSpan(span)).toEqual({ + ...getDynamicSamplingContextFromClient(propagationContext.traceId, getClient()!), + sample_rate: '1', + sampled: 'true', + transaction: 'test span', + }); }); }); diff --git a/packages/utils/src/baggage.ts b/packages/utils/src/baggage.ts index 25210d02bbc4..b0e506b8938a 100644 --- a/packages/utils/src/baggage.ts +++ b/packages/utils/src/baggage.ts @@ -28,31 +28,10 @@ export function baggageHeaderToDynamicSamplingContext( // Very liberal definition of what any incoming header might look like baggageHeader: string | string[] | number | null | undefined | boolean, ): Partial | undefined { - if (!isString(baggageHeader) && !Array.isArray(baggageHeader)) { - return undefined; - } - - // Intermediary object to store baggage key value pairs of incoming baggage headers on. - // It is later used to read Sentry-DSC-values from. - let baggageObject: Readonly> = {}; + const baggageObject = parseBaggageHeader(baggageHeader); - if (Array.isArray(baggageHeader)) { - // Combine all baggage headers into one object containing the baggage values so we can later read the Sentry-DSC-values from it - baggageObject = baggageHeader.reduce>((acc, curr) => { - const currBaggageObject = baggageHeaderToObject(curr); - for (const key of Object.keys(currBaggageObject)) { - acc[key] = currBaggageObject[key]; - } - return acc; - }, {}); - } else { - // Return undefined if baggage header is an empty string (technically an empty baggage header is not spec conform but - // this is how we choose to handle it) - if (!baggageHeader) { - return undefined; - } - - baggageObject = baggageHeaderToObject(baggageHeader); + if (!baggageObject) { + return undefined; } // Read all "sentry-" prefixed values out of the baggage object and put it onto a dynamic sampling context object. @@ -104,6 +83,30 @@ export function dynamicSamplingContextToSentryBaggageHeader( return objectToBaggageHeader(sentryPrefixedDSC); } +/** + * Take a baggage header and parse it into an object. + */ +export function parseBaggageHeader( + baggageHeader: string | string[] | number | null | undefined | boolean, +): Record | undefined { + if (!baggageHeader || (!isString(baggageHeader) && !Array.isArray(baggageHeader))) { + return undefined; + } + + if (Array.isArray(baggageHeader)) { + // Combine all baggage headers into one object containing the baggage values so we can later read the Sentry-DSC-values from it + return baggageHeader.reduce>((acc, curr) => { + const currBaggageObject = baggageHeaderToObject(curr); + for (const key of Object.keys(currBaggageObject)) { + acc[key] = currBaggageObject[key]; + } + return acc; + }, {}); + } + + return baggageHeaderToObject(baggageHeader); +} + /** * Will parse a baggage header, which is a simple key-value map, into a flat object. *