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`. 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 076fa64a1d37..8f90bc2e9ec6 100644 --- a/packages/opentelemetry/src/sampler.ts +++ b/packages/opentelemetry/src/sampler.ts @@ -95,45 +95,62 @@ 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 }), + attributes, + }; + } - if (!sampled) { - if (parentSampled === undefined) { + if ( + !sampled && + // We check for `parentSampled === undefined` because we only want to record client reports for spans that are trace roots (ie. when there was incoming trace) + parentSampled === undefined + ) { DEBUG_BUILD && logger.log('[Tracing] Discarding root span because its trace was not chosen to be sampled.'); this._client.recordDroppedEvent('sample_rate', 'transaction'); } return { - ...wrapSamplingDecision({ decision: SamplingDecision.NOT_RECORD, context, spanAttributes }), + ...wrapSamplingDecision({ + decision: sampled ? SamplingDecision.RECORD_AND_SAMPLED : SamplingDecision.NOT_RECORD, + context, + spanAttributes, + }), attributes, }; + } else { + return { + ...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. */ 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); diff --git a/packages/opentelemetry/test/trace.test.ts b/packages/opentelemetry/test/trace.test.ts index 293036a93964..3eedc0743ea0 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',