Skip to content

fix(node): Fix sample rand propagation for negative sampling decisions #15045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 20, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const Sentry = require('@sentry/node');
Sentry.init({
dsn: 'https://[email protected]/1337',
transport: loggingTransport,
tracesSampleRate: 1,
tracesSampleRate: 0.00000001, // It's important that this is not 1, so that we also check logic for NonRecordingSpans, which is usually the edge-case
});

// express must be required after Sentry is initialized
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ test('envelope header for error event during active unsampled span is correct',
environment: 'production',
release: '1.0',
sampled: 'false',
sample_rand: expect.any(String),
},
},
})
Expand Down
9 changes: 8 additions & 1 deletion packages/core/src/tracing/dynamicSamplingContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,14 @@ export function getDynamicSamplingContextFromSpan(span: Span): Readonly<Partial<
// So we end up with an active span that is not sampled (neither positively nor negatively)
if (hasTracingEnabled()) {
dsc.sampled = String(spanIsSampled(rootSpan));
dsc.sample_rand = getCapturedScopesOnSpan(rootSpan).scope?.getPropagationContext().sampleRand.toString();
dsc.sample_rand =
// In OTEL we store the sample rand on the trace state because we cannot access scopes for NonRecordingSpans
// The Sentry OTEL SpanSampler takes care of writing the sample rand on the root span
traceState?.get('sentry.sample_rand') ??
// On all other platforms we can actually get the scopes from a root span (we use this as a fallback)
getCapturedScopesOnSpan(rootSpan)
.scope?.getPropagationContext()
.sampleRand.toString();
}

client.emit('createDsc', dsc, rootSpan);
Expand Down
1 change: 1 addition & 0 deletions packages/opentelemetry/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ export const SENTRY_BAGGAGE_HEADER = 'baggage';
export const SENTRY_TRACE_STATE_DSC = 'sentry.dsc';
export const SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING = 'sentry.sampled_not_recording';
export const SENTRY_TRACE_STATE_URL = 'sentry.url';
export const SENTRY_TRACE_STATE_SAMPLE_RAND = 'sentry.sample_rand';

export const SENTRY_SCOPES_CONTEXT_KEY = createContextKey('sentry_scopes');

Expand Down
26 changes: 20 additions & 6 deletions packages/opentelemetry/src/sampler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@ import {
logger,
sampleSpan,
} from '@sentry/core';
import { SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING, SENTRY_TRACE_STATE_URL } from './constants';
import {
SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING,
SENTRY_TRACE_STATE_SAMPLE_RAND,
SENTRY_TRACE_STATE_URL,
} from './constants';
import { DEBUG_BUILD } from './debug-build';
import { getScopesFromContext } from './utils/contextData';
import { getSamplingDecision } from './utils/getSamplingDecision';
Expand Down Expand Up @@ -99,11 +103,10 @@ export class SentrySampler implements Sampler {

const isRootSpan = !parentSpan || parentContext?.isRemote;

const { isolationScope, scope } = getScopesFromContext(context) ?? {};

// 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 { isolationScope, scope } = getScopesFromContext(context) ?? {};
const sampleRand = scope?.getPropagationContext().sampleRand ?? Math.random();
const [sampled, sampleRate] = sampleSpan(
options,
Expand All @@ -126,7 +129,7 @@ export class SentrySampler implements Sampler {
DEBUG_BUILD && logger.log(`[Tracing] Not sampling span because HTTP method is '${method}' for ${spanName}`);

return {
...wrapSamplingDecision({ decision: SamplingDecision.NOT_RECORD, context, spanAttributes }),
...wrapSamplingDecision({ decision: SamplingDecision.NOT_RECORD, context, spanAttributes, sampleRand }),
attributes,
};
}
Expand All @@ -145,6 +148,7 @@ export class SentrySampler implements Sampler {
decision: sampled ? SamplingDecision.RECORD_AND_SAMPLED : SamplingDecision.NOT_RECORD,
context,
spanAttributes,
sampleRand,
}),
attributes,
};
Expand Down Expand Up @@ -196,8 +200,18 @@ export function wrapSamplingDecision({
decision,
context,
spanAttributes,
}: { decision: SamplingDecision | undefined; context: Context; spanAttributes: SpanAttributes }): SamplingResult {
const traceState = getBaseTraceState(context, spanAttributes);
sampleRand,
}: {
decision: SamplingDecision | undefined;
context: Context;
spanAttributes: SpanAttributes;
sampleRand?: number;
}): SamplingResult {
let traceState = getBaseTraceState(context, spanAttributes);

if (sampleRand !== undefined) {
traceState = traceState.set(SENTRY_TRACE_STATE_SAMPLE_RAND, `${sampleRand}`);
}

// If the decision is undefined, we treat it as NOT_RECORDING, but we don't propagate this decision to downstream SDKs
// Which is done by not setting `SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING` traceState
Expand Down
15 changes: 9 additions & 6 deletions packages/opentelemetry/test/sampler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,14 @@ describe('SentrySampler', () => {
const links = undefined;

const actual = sampler.shouldSample(ctx, traceId, spanName, spanKind, spanAttributes, links);
expect(actual).toEqual({
decision: SamplingDecision.NOT_RECORD,
attributes: { 'sentry.sample_rate': 0 },
traceState: new TraceState().set('sentry.sampled_not_recording', '1'),
});
expect(actual).toEqual(
expect.objectContaining({
decision: SamplingDecision.NOT_RECORD,
attributes: { 'sentry.sample_rate': 0 },
}),
);
expect(actual.traceState?.get('sentry.sampled_not_recording')).toBe('1');
expect(actual.traceState?.get('sentry.sample_rand')).toEqual(expect.any(String));
expect(spyOnDroppedEvent).toHaveBeenCalledTimes(1);
expect(spyOnDroppedEvent).toHaveBeenCalledWith('sample_rate', 'transaction');

Expand Down Expand Up @@ -81,7 +84,7 @@ describe('SentrySampler', () => {
expect(actual).toEqual({
decision: SamplingDecision.RECORD_AND_SAMPLED,
attributes: { 'sentry.sample_rate': 1 },
traceState: new TraceState(),
traceState: expect.any(TraceState),
});
expect(spyOnDroppedEvent).toHaveBeenCalledTimes(0);

Expand Down
Loading