From 5107916b51d758a5e9e8c3f22efbcc09c45c1373 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 9 Jan 2025 09:48:08 +0000 Subject: [PATCH 1/4] feat(opentelemetry): Only pass root spans through sampling pipeline --- packages/core/src/semanticAttributes.ts | 2 + packages/opentelemetry/src/sampler.ts | 59 +++++++++++++++---------- 2 files changed, 38 insertions(+), 23 deletions(-) diff --git a/packages/core/src/semanticAttributes.ts b/packages/core/src/semanticAttributes.ts index b799f5321a0e..a0b1921ec8f3 100644 --- a/packages/core/src/semanticAttributes.ts +++ b/packages/core/src/semanticAttributes.ts @@ -7,6 +7,8 @@ export const SEMANTIC_ATTRIBUTE_SENTRY_SOURCE = 'sentry.source'; /** * Use this attribute to represent the sample rate used for a span. + * + * NOTE: Is only defined on root spans. */ export const SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE = 'sentry.sample_rate'; diff --git a/packages/opentelemetry/src/sampler.ts b/packages/opentelemetry/src/sampler.ts index a561bb863561..f047464a573e 100644 --- a/packages/opentelemetry/src/sampler.ts +++ b/packages/opentelemetry/src/sampler.ts @@ -96,40 +96,53 @@ export class SentrySampler implements Sampler { return wrapSamplingDecision({ decision: undefined, context, spanAttributes }); } - const [sampled, sampleRate] = sampleSpan(options, { - name: inferredSpanName, - attributes: mergedAttributes, - transactionContext: { + const isRootSpan = !parentSpan || parentContext?.isRemote; + + // We only sample based on parameters (like tracesSampleRate or tracesSampler) for root spans (which is done in sampleSpan). + // Non-root-spans simply inherit the sampling decision from their parent. + if (isRootSpan) { + const [sampled, sampleRate] = sampleSpan(options, { name: inferredSpanName, + attributes: mergedAttributes, + transactionContext: { + name: inferredSpanName, + parentSampled, + }, parentSampled, - }, - parentSampled, - }); + }); - const attributes: Attributes = { - [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: sampleRate, - }; + const attributes: Attributes = { + [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: sampleRate, + }; + + const method = `${maybeSpanHttpMethod}`.toUpperCase(); + if (method === 'OPTIONS' || method === 'HEAD') { + DEBUG_BUILD && logger.log(`[Tracing] Not sampling span because HTTP method is '${method}' for ${spanName}`); - const method = `${maybeSpanHttpMethod}`.toUpperCase(); - if (method === 'OPTIONS' || method === 'HEAD') { - DEBUG_BUILD && logger.log(`[Tracing] Not sampling span because HTTP method is '${method}' for ${spanName}`); + return { + ...wrapSamplingDecision({ decision: SamplingDecision.NOT_RECORD, context, spanAttributes }), + attributes, + }; + } return { - ...wrapSamplingDecision({ decision: SamplingDecision.NOT_RECORD, context, spanAttributes }), + ...wrapSamplingDecision({ + decision: sampled ? SamplingDecision.RECORD_AND_SAMPLED : SamplingDecision.NOT_RECORD, + context, + spanAttributes, + }), attributes, }; - } - - if (!sampled) { + } else { return { - ...wrapSamplingDecision({ decision: SamplingDecision.NOT_RECORD, context, spanAttributes }), - attributes, + ...wrapSamplingDecision({ + decision: parentSampled ? SamplingDecision.RECORD_AND_SAMPLED : SamplingDecision.NOT_RECORD, + context, + spanAttributes, + }), + attributes: {}, }; } - return { - ...wrapSamplingDecision({ decision: SamplingDecision.RECORD_AND_SAMPLED, context, spanAttributes }), - attributes, - }; } /** Returns the sampler name or short description with the configuration. */ From 8c6ea7127fcc39a16a7b9b0d6a3bdce6dde1b16f Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 9 Jan 2025 11:06:32 +0000 Subject: [PATCH 2/4] Update tests --- packages/opentelemetry/test/trace.test.ts | 50 +++++++++++++++++------ 1 file changed, 37 insertions(+), 13 deletions(-) diff --git a/packages/opentelemetry/test/trace.test.ts b/packages/opentelemetry/test/trace.test.ts index 6852b8b40988..b886e3a40555 100644 --- a/packages/opentelemetry/test/trace.test.ts +++ b/packages/opentelemetry/test/trace.test.ts @@ -1336,12 +1336,27 @@ describe('trace (sampling)', () => { }); }); - expect(tracesSampler).toHaveBeenCalledTimes(3); - expect(tracesSampler).toHaveBeenLastCalledWith({ - parentSampled: false, + expect(tracesSampler).toHaveBeenCalledTimes(2); + expect(tracesSampler).toHaveBeenCalledWith( + expect.objectContaining({ + parentSampled: undefined, + name: 'outer', + attributes: {}, + transactionContext: { name: 'outer', parentSampled: undefined }, + }), + ); + expect(tracesSampler).toHaveBeenCalledWith( + expect.objectContaining({ + parentSampled: undefined, + name: 'outer2', + attributes: {}, + transactionContext: { name: 'outer2', parentSampled: undefined }, + }), + ); + + // Only root spans should go through the sampler + expect(tracesSampler).not.toHaveBeenLastCalledWith({ name: 'inner2', - attributes: {}, - transactionContext: { name: 'inner2', parentSampled: false }, }); }); @@ -1390,13 +1405,22 @@ describe('trace (sampling)', () => { }); }); - expect(tracesSampler).toHaveBeenCalledTimes(3); - expect(tracesSampler).toHaveBeenLastCalledWith({ - parentSampled: false, - name: 'inner2', - attributes: {}, - transactionContext: { name: 'inner2', parentSampled: false }, - }); + expect(tracesSampler).toHaveBeenCalledTimes(2); + expect(tracesSampler).toHaveBeenCalledWith( + expect.objectContaining({ + parentSampled: undefined, + name: 'outer2', + attributes: {}, + transactionContext: { name: 'outer2', parentSampled: undefined }, + }), + ); + + // Only root spans should be passed to tracesSampler + expect(tracesSampler).not.toHaveBeenLastCalledWith( + expect.objectContaining({ + name: 'inner2', + }), + ); // Now return `0.4`, it should not sample tracesSamplerResponse = 0.4; @@ -1405,7 +1429,7 @@ describe('trace (sampling)', () => { expect(outerSpan.isRecording()).toBe(false); }); - expect(tracesSampler).toHaveBeenCalledTimes(4); + expect(tracesSampler).toHaveBeenCalledTimes(3); expect(tracesSampler).toHaveBeenLastCalledWith({ parentSampled: undefined, name: 'outer3', From 71cd67d9d44f7e5ec77f7a7a422709d1fd0bb789 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 9 Jan 2025 11:47:04 +0000 Subject: [PATCH 3/4] Fix test --- packages/opentelemetry/test/sampler.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry/test/sampler.test.ts b/packages/opentelemetry/test/sampler.test.ts index ece4cfd8b087..9f6545bc14a3 100644 --- a/packages/opentelemetry/test/sampler.test.ts +++ b/packages/opentelemetry/test/sampler.test.ts @@ -57,7 +57,7 @@ describe('SentrySampler', () => { const actual = sampler.shouldSample(ctx, traceId, spanName, spanKind, spanAttributes, links); expect(actual).toEqual({ decision: SamplingDecision.NOT_RECORD, - attributes: { 'sentry.sample_rate': 0 }, + attributes: {}, traceState: new TraceState().set(SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING, '1'), }); expect(spyOnDroppedEvent).toHaveBeenCalledTimes(0); From f7c24760e27fdaf0f46522f321ed3931a83585b0 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 9 Jan 2025 11:47:10 +0000 Subject: [PATCH 4/4] Add migration docs --- docs/migration/v8-to-v9.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/migration/v8-to-v9.md b/docs/migration/v8-to-v9.md index 84e0526d102d..b81e9e98ef4a 100644 --- a/docs/migration/v8-to-v9.md +++ b/docs/migration/v8-to-v9.md @@ -88,6 +88,8 @@ In v9, an `undefined` value will be treated the same as if the value is not defi - The `requestDataIntegration` will no longer automatically set the user from `request.user`. This is an express-specific, undocumented behavior, and also conflicts with our privacy-by-default strategy. Starting in v9, you'll need to manually call `Sentry.setUser()` e.g. in a middleware to set the user on Sentry events. +- The `tracesSampler` hook will no longer be called for _every_ span. Instead, it will only be called for "root spans". Root spans are spans that have no local parent span. Root spans may however have incoming trace data from a different service, for example when using distributed tracing. + ### `@sentry/browser` - The `captureUserFeedback` method has been removed. Use `captureFeedback` instead and update the `comments` field to `message`.