From eb5303e7a31104b2c0d9e90404dabc59dedfade9 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 8 Jan 2025 11:30:10 +0100 Subject: [PATCH 1/6] ref(core): Ensure non-sampled spans are NonRecordingSpans --- packages/core/src/tracing/idleSpan.ts | 9 +++++++++ packages/core/src/tracing/sentrySpan.ts | 26 +++++++++++++++--------- packages/core/src/tracing/trace.ts | 27 ++++++++++++++++--------- 3 files changed, 43 insertions(+), 19 deletions(-) diff --git a/packages/core/src/tracing/idleSpan.ts b/packages/core/src/tracing/idleSpan.ts index 14bce971b5b0..5781d12b6ee1 100644 --- a/packages/core/src/tracing/idleSpan.ts +++ b/packages/core/src/tracing/idleSpan.ts @@ -15,6 +15,7 @@ import { spanToJSON, } from '../utils/spanUtils'; import { SentryNonRecordingSpan } from './sentryNonRecordingSpan'; +import { recordDroppedRootSpan } from './sentrySpan'; import { SPAN_STATUS_ERROR } from './spanstatus'; import { startInactiveSpan } from './trace'; @@ -124,6 +125,14 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti beforeSpanEnd(span); } + // If the span is non-recording, nothing more to do here... + // This is the case if tracing is enabled but this specific span was not sampled + // We make sure to record this as dropped in this case + if (thisArg instanceof SentryNonRecordingSpan) { + recordDroppedRootSpan(thisArg); + return; + } + // Just ensuring that this keeps working, even if we ever have more arguments here const [definedEndTimestamp, ...rest] = args; const timestamp = definedEndTimestamp || timestampInSeconds(); diff --git a/packages/core/src/tracing/sentrySpan.ts b/packages/core/src/tracing/sentrySpan.ts index 309f46ff874c..6567bde55ebd 100644 --- a/packages/core/src/tracing/sentrySpan.ts +++ b/packages/core/src/tracing/sentrySpan.ts @@ -34,6 +34,7 @@ import { getRootSpan, getSpanDescendants, getStatusMessage, + spanIsSampled, spanTimeInputToSeconds, spanToJSON, spanToTransactionTraceContext, @@ -333,17 +334,9 @@ export class SentrySpan implements Span { } const { scope: capturedSpanScope, isolationScope: capturedSpanIsolationScope } = getCapturedScopesOnSpan(this); - const scope = capturedSpanScope || getCurrentScope(); - const client = scope.getClient() || getClient(); if (this._sampled !== true) { - // At this point if `sampled !== true` we want to discard the transaction. - DEBUG_BUILD && logger.log('[Tracing] Discarding transaction because its trace was not chosen to be sampled.'); - - if (client) { - client.recordDroppedEvent('sample_rate', 'transaction'); - } - + recordDroppedRootSpan(this); return undefined; } @@ -442,3 +435,18 @@ function sendSpanEnvelope(envelope: SpanEnvelope): void { // eslint-disable-next-line @typescript-eslint/no-floating-promises client.sendEnvelope(envelope); } + +/** Record a dropped root span. */ +export function recordDroppedRootSpan(span: Span): void { + const { scope: capturedSpanScope } = getCapturedScopesOnSpan(span); + const scope = capturedSpanScope || getCurrentScope(); + const client = scope.getClient() || getClient(); + + if (!spanIsSampled(span)) { + DEBUG_BUILD && logger.log('[Tracing] Discarding root span because its trace was not chosen to be sampled.'); + + if (client) { + client.recordDroppedEvent('sample_rate', 'transaction'); + } + } +} diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index f17867a4e32d..d5a93e901fa7 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -370,9 +370,13 @@ function getAcs(): AsyncContextStrategy { return getAsyncContextStrategy(carrier); } -function _startRootSpan(spanArguments: SentrySpanArguments, scope: Scope, parentSampled?: boolean): SentrySpan { +function _startRootSpan( + spanArguments: SentrySpanArguments, + scope: Scope, + parentSampled?: boolean, +): SentrySpan | SentryNonRecordingSpan { const client = getClient(); - const options: Partial = (client && client.getOptions()) || {}; + const options: Partial = client?.getOptions() || {}; const { name = '', attributes } = spanArguments; const [sampled, sampleRate] = scope.getScopeData().sdkProcessingMetadata[SUPPRESS_TRACING_KEY] @@ -387,14 +391,17 @@ function _startRootSpan(spanArguments: SentrySpanArguments, scope: Scope, parent }, }); - const rootSpan = new SentrySpan({ - ...spanArguments, - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', - ...spanArguments.attributes, - }, - sampled, - }); + const rootSpan = sampled + ? new SentrySpan({ + ...spanArguments, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', + ...spanArguments.attributes, + }, + sampled, + }) + : new SentryNonRecordingSpan({ traceId: spanArguments.traceId }); + if (sampleRate !== undefined) { rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, sampleRate); } From d482c6bc0745f653f6b697a8f1eae225e4831461 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 8 Jan 2025 12:05:46 +0100 Subject: [PATCH 2/6] emit dropped transactions when sampling --- packages/core/src/tracing/idleSpan.ts | 3 - packages/core/src/tracing/sentrySpan.ts | 17 --- packages/core/src/tracing/trace.ts | 5 + packages/opentelemetry/src/sampler.ts | 5 + .../opentelemetry/test/helpers/TestClient.ts | 3 +- .../test/integration/breadcrumbs.test.ts | 10 +- .../test/integration/scope.test.ts | 6 +- .../test/integration/transactions.test.ts | 14 +-- .../opentelemetry/test/propagator.test.ts | 2 +- packages/opentelemetry/test/sampler.test.ts | 106 ++++++++++++++++++ .../opentelemetry/test/spanExporter.test.ts | 2 +- packages/opentelemetry/test/trace.test.ts | 10 +- .../test/utils/getActiveSpan.test.ts | 2 +- .../test/utils/setupEventContextTrace.test.ts | 2 +- 14 files changed, 143 insertions(+), 44 deletions(-) create mode 100644 packages/opentelemetry/test/sampler.test.ts diff --git a/packages/core/src/tracing/idleSpan.ts b/packages/core/src/tracing/idleSpan.ts index 5781d12b6ee1..c37ef7b388a1 100644 --- a/packages/core/src/tracing/idleSpan.ts +++ b/packages/core/src/tracing/idleSpan.ts @@ -15,7 +15,6 @@ import { spanToJSON, } from '../utils/spanUtils'; import { SentryNonRecordingSpan } from './sentryNonRecordingSpan'; -import { recordDroppedRootSpan } from './sentrySpan'; import { SPAN_STATUS_ERROR } from './spanstatus'; import { startInactiveSpan } from './trace'; @@ -127,9 +126,7 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti // If the span is non-recording, nothing more to do here... // This is the case if tracing is enabled but this specific span was not sampled - // We make sure to record this as dropped in this case if (thisArg instanceof SentryNonRecordingSpan) { - recordDroppedRootSpan(thisArg); return; } diff --git a/packages/core/src/tracing/sentrySpan.ts b/packages/core/src/tracing/sentrySpan.ts index 6567bde55ebd..acc14d37bc31 100644 --- a/packages/core/src/tracing/sentrySpan.ts +++ b/packages/core/src/tracing/sentrySpan.ts @@ -34,7 +34,6 @@ import { getRootSpan, getSpanDescendants, getStatusMessage, - spanIsSampled, spanTimeInputToSeconds, spanToJSON, spanToTransactionTraceContext, @@ -336,7 +335,6 @@ export class SentrySpan implements Span { const { scope: capturedSpanScope, isolationScope: capturedSpanIsolationScope } = getCapturedScopesOnSpan(this); if (this._sampled !== true) { - recordDroppedRootSpan(this); return undefined; } @@ -435,18 +433,3 @@ function sendSpanEnvelope(envelope: SpanEnvelope): void { // eslint-disable-next-line @typescript-eslint/no-floating-promises client.sendEnvelope(envelope); } - -/** Record a dropped root span. */ -export function recordDroppedRootSpan(span: Span): void { - const { scope: capturedSpanScope } = getCapturedScopesOnSpan(span); - const scope = capturedSpanScope || getCurrentScope(); - const client = scope.getClient() || getClient(); - - if (!spanIsSampled(span)) { - DEBUG_BUILD && logger.log('[Tracing] Discarding root span because its trace was not chosen to be sampled.'); - - if (client) { - client.recordDroppedEvent('sample_rate', 'transaction'); - } - } -} diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index d5a93e901fa7..69aec358ed86 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -391,6 +391,11 @@ function _startRootSpan( }, }); + if (!sampled && client) { + DEBUG_BUILD && logger.log('[Tracing] Discarding root span because its trace was not chosen to be sampled.'); + client.recordDroppedEvent('sample_rate', 'transaction'); + } + const rootSpan = sampled ? new SentrySpan({ ...spanArguments, diff --git a/packages/opentelemetry/src/sampler.ts b/packages/opentelemetry/src/sampler.ts index a561bb863561..fcd258dd9416 100644 --- a/packages/opentelemetry/src/sampler.ts +++ b/packages/opentelemetry/src/sampler.ts @@ -121,6 +121,11 @@ export class SentrySampler implements Sampler { } if (!sampled) { + if (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 }), attributes, diff --git a/packages/opentelemetry/test/helpers/TestClient.ts b/packages/opentelemetry/test/helpers/TestClient.ts index a60ff8eee831..5089dd160768 100644 --- a/packages/opentelemetry/test/helpers/TestClient.ts +++ b/packages/opentelemetry/test/helpers/TestClient.ts @@ -33,7 +33,7 @@ export const TestClient = wrapClientClass(BaseTestClient); export type TestClientInterface = Client & OpenTelemetryClient; export function init(options: Partial = {}): void { - const client = new TestClient(getDefaultTestClientOptions(options)); + const client = new TestClient(getDefaultTestClientOptions({ tracesSampleRate: 1, ...options })); // The client is on the current scope, from where it generally is inherited getCurrentScope().setClient(client); @@ -42,7 +42,6 @@ export function init(options: Partial = {}): void { export function getDefaultTestClientOptions(options: Partial = {}): ClientOptions { return { - enableTracing: true, integrations: [], transport: () => createTransport({ recordDroppedEvent: () => undefined }, _ => resolvedSyncPromise({})), stackParser: () => [], diff --git a/packages/opentelemetry/test/integration/breadcrumbs.test.ts b/packages/opentelemetry/test/integration/breadcrumbs.test.ts index 9da0f4d6e240..aef5149b8920 100644 --- a/packages/opentelemetry/test/integration/breadcrumbs.test.ts +++ b/packages/opentelemetry/test/integration/breadcrumbs.test.ts @@ -98,7 +98,7 @@ describe('Integration | breadcrumbs', () => { const beforeSend = jest.fn(() => null); const beforeBreadcrumb = jest.fn(breadcrumb => breadcrumb); - mockSdkInit({ beforeSend, beforeBreadcrumb, beforeSendTransaction, enableTracing: true }); + mockSdkInit({ beforeSend, beforeBreadcrumb, beforeSendTransaction, tracesSampleRate: 1 }); const client = getClient() as TestClientInterface; @@ -143,7 +143,7 @@ describe('Integration | breadcrumbs', () => { const beforeSend = jest.fn(() => null); const beforeBreadcrumb = jest.fn(breadcrumb => breadcrumb); - mockSdkInit({ beforeSend, beforeBreadcrumb, beforeSendTransaction, enableTracing: true }); + mockSdkInit({ beforeSend, beforeBreadcrumb, beforeSendTransaction, tracesSampleRate: 1 }); const client = getClient() as TestClientInterface; @@ -195,7 +195,7 @@ describe('Integration | breadcrumbs', () => { const beforeSend = jest.fn(() => null); const beforeBreadcrumb = jest.fn(breadcrumb => breadcrumb); - mockSdkInit({ beforeSend, beforeBreadcrumb, beforeSendTransaction, enableTracing: true }); + mockSdkInit({ beforeSend, beforeBreadcrumb, beforeSendTransaction, tracesSampleRate: 1 }); const client = getClient() as TestClientInterface; @@ -236,7 +236,7 @@ describe('Integration | breadcrumbs', () => { const beforeSend = jest.fn(() => null); const beforeBreadcrumb = jest.fn(breadcrumb => breadcrumb); - mockSdkInit({ beforeSend, beforeBreadcrumb, beforeSendTransaction, enableTracing: true }); + mockSdkInit({ beforeSend, beforeBreadcrumb, beforeSendTransaction, tracesSampleRate: 1 }); const client = getClient() as TestClientInterface; @@ -294,7 +294,7 @@ describe('Integration | breadcrumbs', () => { const beforeSend = jest.fn(() => null); const beforeBreadcrumb = jest.fn(breadcrumb => breadcrumb); - mockSdkInit({ beforeSend, beforeBreadcrumb, beforeSendTransaction, enableTracing: true }); + mockSdkInit({ beforeSend, beforeBreadcrumb, beforeSendTransaction, tracesSampleRate: 1 }); const client = getClient() as TestClientInterface; diff --git a/packages/opentelemetry/test/integration/scope.test.ts b/packages/opentelemetry/test/integration/scope.test.ts index c2e3dcc86701..b7577aa36044 100644 --- a/packages/opentelemetry/test/integration/scope.test.ts +++ b/packages/opentelemetry/test/integration/scope.test.ts @@ -26,7 +26,11 @@ describe('Integration | Scope', () => { const beforeSend = jest.fn(() => null); const beforeSendTransaction = jest.fn(() => null); - mockSdkInit({ enableTracing, beforeSend, beforeSendTransaction }); + mockSdkInit({ + tracesSampleRate: enableTracing ? 1 : 0, + beforeSend, + beforeSendTransaction, + }); const client = getClient() as TestClientInterface; diff --git a/packages/opentelemetry/test/integration/transactions.test.ts b/packages/opentelemetry/test/integration/transactions.test.ts index fe24f49a9bf1..3e299574d51b 100644 --- a/packages/opentelemetry/test/integration/transactions.test.ts +++ b/packages/opentelemetry/test/integration/transactions.test.ts @@ -37,7 +37,7 @@ describe('Integration | Transactions', () => { }); mockSdkInit({ - enableTracing: true, + tracesSampleRate: 1, beforeSendTransaction, release: '8.0.0', }); @@ -178,7 +178,7 @@ describe('Integration | Transactions', () => { it('correctly creates concurrent transaction & spans', async () => { const beforeSendTransaction = jest.fn(() => null); - mockSdkInit({ enableTracing: true, beforeSendTransaction }); + mockSdkInit({ tracesSampleRate: 1, beforeSendTransaction }); const client = getClient() as TestClientInterface; @@ -339,7 +339,7 @@ describe('Integration | Transactions', () => { traceState, }; - mockSdkInit({ enableTracing: true, beforeSendTransaction }); + mockSdkInit({ tracesSampleRate: 1, beforeSendTransaction }); const client = getClient() as TestClientInterface; @@ -443,7 +443,7 @@ describe('Integration | Transactions', () => { const logs: unknown[] = []; jest.spyOn(logger, 'log').mockImplementation(msg => logs.push(msg)); - mockSdkInit({ enableTracing: true, beforeSendTransaction }); + mockSdkInit({ tracesSampleRate: 1, beforeSendTransaction }); const provider = getProvider(); const multiSpanProcessor = provider?.activeSpanProcessor as @@ -516,7 +516,7 @@ describe('Integration | Transactions', () => { const transactions: Event[] = []; mockSdkInit({ - enableTracing: true, + tracesSampleRate: 1, beforeSendTransaction: event => { transactions.push(event); return null; @@ -573,7 +573,7 @@ describe('Integration | Transactions', () => { const transactions: Event[] = []; mockSdkInit({ - enableTracing: true, + tracesSampleRate: 1, beforeSendTransaction: event => { transactions.push(event); return null; @@ -644,7 +644,7 @@ describe('Integration | Transactions', () => { }; mockSdkInit({ - enableTracing: true, + tracesSampleRate: 1, beforeSendTransaction, release: '7.0.0', }); diff --git a/packages/opentelemetry/test/propagator.test.ts b/packages/opentelemetry/test/propagator.test.ts index 56aed551353e..26b7ef328fb2 100644 --- a/packages/opentelemetry/test/propagator.test.ts +++ b/packages/opentelemetry/test/propagator.test.ts @@ -25,7 +25,7 @@ describe('SentryPropagator', () => { mockSdkInit({ environment: 'production', release: '1.0.0', - enableTracing: true, + tracesSampleRate: 1, dsn: 'https://abc@domain/123', }); }); diff --git a/packages/opentelemetry/test/sampler.test.ts b/packages/opentelemetry/test/sampler.test.ts new file mode 100644 index 000000000000..a20d37d88855 --- /dev/null +++ b/packages/opentelemetry/test/sampler.test.ts @@ -0,0 +1,106 @@ +import { SpanKind, context } from '@opentelemetry/api'; +import { TraceState } from '@opentelemetry/core'; +import { SamplingDecision } from '@opentelemetry/sdk-trace-base'; +import { ATTR_HTTP_REQUEST_METHOD } from '@opentelemetry/semantic-conventions'; +import { SentrySampler } from '../src/sampler'; +import { TestClient, getDefaultTestClientOptions } from './helpers/TestClient'; +import { cleanupOtel } from './helpers/mockSdkInit'; + +describe('SentrySampler', () => { + afterEach(() => { + cleanupOtel(); + }); + + it('works with tracesSampleRate=0', () => { + const client = new TestClient(getDefaultTestClientOptions({ tracesSampleRate: 0 })); + const spyOnDroppedEvent = jest.spyOn(client, 'recordDroppedEvent'); + const sampler = new SentrySampler(client); + + const ctx = context.active(); + const traceId = '1234567890123456'; + const spanName = 'test'; + const spanKind = SpanKind.INTERNAL; + const spanAttributes = {}; + 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(spyOnDroppedEvent).toHaveBeenCalledTimes(1); + expect(spyOnDroppedEvent).toHaveBeenCalledWith('sample_rate', 'transaction'); + + spyOnDroppedEvent.mockReset(); + }); + + it('works with tracesSampleRate=1', () => { + const client = new TestClient(getDefaultTestClientOptions({ tracesSampleRate: 1 })); + const spyOnDroppedEvent = jest.spyOn(client, 'recordDroppedEvent'); + const sampler = new SentrySampler(client); + + const ctx = context.active(); + const traceId = '1234567890123456'; + const spanName = 'test'; + const spanKind = SpanKind.INTERNAL; + const spanAttributes = {}; + const links = undefined; + + const actual = sampler.shouldSample(ctx, traceId, spanName, spanKind, spanAttributes, links); + expect(actual).toEqual({ + decision: SamplingDecision.RECORD_AND_SAMPLED, + attributes: { 'sentry.sample_rate': 1 }, + traceState: new TraceState(), + }); + expect(spyOnDroppedEvent).toHaveBeenCalledTimes(0); + + spyOnDroppedEvent.mockReset(); + }); + + it('works with traceSampleRate=undefined', () => { + const client = new TestClient(getDefaultTestClientOptions({ tracesSampleRate: undefined })); + const spyOnDroppedEvent = jest.spyOn(client, 'recordDroppedEvent'); + const sampler = new SentrySampler(client); + + const ctx = context.active(); + const traceId = '1234567890123456'; + const spanName = 'test'; + const spanKind = SpanKind.INTERNAL; + const spanAttributes = {}; + const links = undefined; + + const actual = sampler.shouldSample(ctx, traceId, spanName, spanKind, spanAttributes, links); + expect(actual).toEqual({ + decision: SamplingDecision.NOT_RECORD, + traceState: new TraceState(), + }); + expect(spyOnDroppedEvent).toHaveBeenCalledTimes(0); + + spyOnDroppedEvent.mockReset(); + }); + + it('ignores local http client root spans', () => { + const client = new TestClient(getDefaultTestClientOptions({ tracesSampleRate: 0 })); + const spyOnDroppedEvent = jest.spyOn(client, 'recordDroppedEvent'); + const sampler = new SentrySampler(client); + + const ctx = context.active(); + const traceId = '1234567890123456'; + const spanName = 'test'; + const spanKind = SpanKind.CLIENT; + const spanAttributes = { + [ATTR_HTTP_REQUEST_METHOD]: 'GET', + }; + const links = undefined; + + const actual = sampler.shouldSample(ctx, traceId, spanName, spanKind, spanAttributes, links); + expect(actual).toEqual({ + decision: SamplingDecision.NOT_RECORD, + traceState: new TraceState(), + }); + expect(spyOnDroppedEvent).toHaveBeenCalledTimes(0); + + spyOnDroppedEvent.mockReset(); + }); +}); diff --git a/packages/opentelemetry/test/spanExporter.test.ts b/packages/opentelemetry/test/spanExporter.test.ts index 737171da8908..48ab8da060de 100644 --- a/packages/opentelemetry/test/spanExporter.test.ts +++ b/packages/opentelemetry/test/spanExporter.test.ts @@ -6,7 +6,7 @@ import { cleanupOtel, mockSdkInit } from './helpers/mockSdkInit'; describe('createTransactionForOtelSpan', () => { beforeEach(() => { mockSdkInit({ - enableTracing: true, + tracesSampleRate: 1, }); }); diff --git a/packages/opentelemetry/test/trace.test.ts b/packages/opentelemetry/test/trace.test.ts index 6852b8b40988..1544735f3d16 100644 --- a/packages/opentelemetry/test/trace.test.ts +++ b/packages/opentelemetry/test/trace.test.ts @@ -34,7 +34,7 @@ import { cleanupOtel, mockSdkInit } from './helpers/mockSdkInit'; describe('trace', () => { beforeEach(() => { - mockSdkInit({ enableTracing: true }); + mockSdkInit({ tracesSampleRate: 1 }); }); afterEach(() => { @@ -1138,7 +1138,7 @@ describe('trace', () => { describe('trace (tracing disabled)', () => { beforeEach(() => { - mockSdkInit({ enableTracing: false }); + mockSdkInit({ tracesSampleRate: 0 }); }); afterEach(() => { @@ -1475,7 +1475,7 @@ describe('trace (sampling)', () => { describe('HTTP methods (sampling)', () => { beforeEach(() => { - mockSdkInit({ enableTracing: true }); + mockSdkInit({ tracesSampleRate: 1 }); }); afterEach(() => { @@ -1530,7 +1530,7 @@ describe('HTTP methods (sampling)', () => { describe('continueTrace', () => { beforeEach(() => { - mockSdkInit({ enableTracing: true }); + mockSdkInit({ tracesSampleRate: 1 }); }); afterEach(() => { @@ -1659,7 +1659,7 @@ describe('continueTrace', () => { describe('suppressTracing', () => { beforeEach(() => { - mockSdkInit({ enableTracing: true }); + mockSdkInit({ tracesSampleRate: 1 }); }); afterEach(() => { diff --git a/packages/opentelemetry/test/utils/getActiveSpan.test.ts b/packages/opentelemetry/test/utils/getActiveSpan.test.ts index 9921f82f2982..b4e0efd32cd0 100644 --- a/packages/opentelemetry/test/utils/getActiveSpan.test.ts +++ b/packages/opentelemetry/test/utils/getActiveSpan.test.ts @@ -96,7 +96,7 @@ describe('getRootSpan', () => { let provider: BasicTracerProvider | undefined; beforeEach(() => { - const client = new TestClient(getDefaultTestClientOptions()); + const client = new TestClient(getDefaultTestClientOptions({ tracesSampleRate: 1 })); provider = setupOtel(client); }); diff --git a/packages/opentelemetry/test/utils/setupEventContextTrace.test.ts b/packages/opentelemetry/test/utils/setupEventContextTrace.test.ts index 95c68e5416f1..34c41ad0ef7f 100644 --- a/packages/opentelemetry/test/utils/setupEventContextTrace.test.ts +++ b/packages/opentelemetry/test/utils/setupEventContextTrace.test.ts @@ -18,7 +18,7 @@ describe('setupEventContextTrace', () => { client = new TestClient( getDefaultTestClientOptions({ sampleRate: 1, - enableTracing: true, + tracesSampleRate: 1, beforeSend, debug: true, dsn: PUBLIC_DSN, From 1354cb6a23b9b2e2e0d90fa24b8652488264a8d0 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 8 Jan 2025 12:08:26 +0100 Subject: [PATCH 3/6] Revert "ref(core): Ensure non-sampled spans are NonRecordingSpans" This reverts commit 66d2e776ebdef236e4d87990faf89b29041b1fae. --- packages/core/src/tracing/sentrySpan.ts | 2 ++ packages/core/src/tracing/trace.ts | 28 ++++++++++--------------- 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/packages/core/src/tracing/sentrySpan.ts b/packages/core/src/tracing/sentrySpan.ts index acc14d37bc31..f05ded75c670 100644 --- a/packages/core/src/tracing/sentrySpan.ts +++ b/packages/core/src/tracing/sentrySpan.ts @@ -333,6 +333,8 @@ export class SentrySpan implements Span { } const { scope: capturedSpanScope, isolationScope: capturedSpanIsolationScope } = getCapturedScopesOnSpan(this); + const scope = capturedSpanScope || getCurrentScope(); + const client = scope.getClient() || getClient(); if (this._sampled !== true) { return undefined; diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index 69aec358ed86..455eb470be23 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -370,13 +370,9 @@ function getAcs(): AsyncContextStrategy { return getAsyncContextStrategy(carrier); } -function _startRootSpan( - spanArguments: SentrySpanArguments, - scope: Scope, - parentSampled?: boolean, -): SentrySpan | SentryNonRecordingSpan { +function _startRootSpan(spanArguments: SentrySpanArguments, scope: Scope, parentSampled?: boolean): SentrySpan { const client = getClient(); - const options: Partial = client?.getOptions() || {}; + const options: Partial = (client && client.getOptions()) || {}; const { name = '', attributes } = spanArguments; const [sampled, sampleRate] = scope.getScopeData().sdkProcessingMetadata[SUPPRESS_TRACING_KEY] @@ -391,22 +387,20 @@ function _startRootSpan( }, }); + const rootSpan = new SentrySpan({ + ...spanArguments, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', + ...spanArguments.attributes, + }, + sampled, + }); + if (!sampled && client) { DEBUG_BUILD && logger.log('[Tracing] Discarding root span because its trace was not chosen to be sampled.'); client.recordDroppedEvent('sample_rate', 'transaction'); } - const rootSpan = sampled - ? new SentrySpan({ - ...spanArguments, - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', - ...spanArguments.attributes, - }, - sampled, - }) - : new SentryNonRecordingSpan({ traceId: spanArguments.traceId }); - if (sampleRate !== undefined) { rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, sampleRate); } From 2d740a1dbdbec066aba47579c1e55a9e10389c80 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 8 Jan 2025 12:19:08 +0100 Subject: [PATCH 4/6] fixes --- packages/opentelemetry/test/sampler.test.ts | 40 ++++++++++++++++++--- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/packages/opentelemetry/test/sampler.test.ts b/packages/opentelemetry/test/sampler.test.ts index a20d37d88855..ece4cfd8b087 100644 --- a/packages/opentelemetry/test/sampler.test.ts +++ b/packages/opentelemetry/test/sampler.test.ts @@ -1,7 +1,9 @@ -import { SpanKind, context } from '@opentelemetry/api'; +import { SpanKind, context, trace } from '@opentelemetry/api'; import { TraceState } from '@opentelemetry/core'; import { SamplingDecision } from '@opentelemetry/sdk-trace-base'; import { ATTR_HTTP_REQUEST_METHOD } from '@opentelemetry/semantic-conventions'; +import { generateSpanId, generateTraceId } from '@sentry/core'; +import { SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING } from '../src/constants'; import { SentrySampler } from '../src/sampler'; import { TestClient, getDefaultTestClientOptions } from './helpers/TestClient'; import { cleanupOtel } from './helpers/mockSdkInit'; @@ -17,7 +19,7 @@ describe('SentrySampler', () => { const sampler = new SentrySampler(client); const ctx = context.active(); - const traceId = '1234567890123456'; + const traceId = generateTraceId(); const spanName = 'test'; const spanKind = SpanKind.INTERNAL; const spanAttributes = {}; @@ -35,13 +37,41 @@ describe('SentrySampler', () => { spyOnDroppedEvent.mockReset(); }); + it('works with tracesSampleRate=0 & for a child span', () => { + const client = new TestClient(getDefaultTestClientOptions({ tracesSampleRate: 0 })); + const spyOnDroppedEvent = jest.spyOn(client, 'recordDroppedEvent'); + const sampler = new SentrySampler(client); + + const traceId = generateTraceId(); + const ctx = trace.setSpanContext(context.active(), { + spanId: generateSpanId(), + traceId, + traceFlags: 0, + traceState: new TraceState().set(SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING, '1'), + }); + const spanName = 'test'; + const spanKind = SpanKind.INTERNAL; + const spanAttributes = {}; + 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_TRACE_STATE_SAMPLED_NOT_RECORDING, '1'), + }); + expect(spyOnDroppedEvent).toHaveBeenCalledTimes(0); + + spyOnDroppedEvent.mockReset(); + }); + it('works with tracesSampleRate=1', () => { const client = new TestClient(getDefaultTestClientOptions({ tracesSampleRate: 1 })); const spyOnDroppedEvent = jest.spyOn(client, 'recordDroppedEvent'); const sampler = new SentrySampler(client); const ctx = context.active(); - const traceId = '1234567890123456'; + const traceId = generateTraceId(); const spanName = 'test'; const spanKind = SpanKind.INTERNAL; const spanAttributes = {}; @@ -64,7 +94,7 @@ describe('SentrySampler', () => { const sampler = new SentrySampler(client); const ctx = context.active(); - const traceId = '1234567890123456'; + const traceId = generateTraceId(); const spanName = 'test'; const spanKind = SpanKind.INTERNAL; const spanAttributes = {}; @@ -86,7 +116,7 @@ describe('SentrySampler', () => { const sampler = new SentrySampler(client); const ctx = context.active(); - const traceId = '1234567890123456'; + const traceId = generateTraceId(); const spanName = 'test'; const spanKind = SpanKind.CLIENT; const spanAttributes = { From b1ad8217dee8f99dd6a95d18734bc7f6019f2bbd Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 8 Jan 2025 12:21:06 +0100 Subject: [PATCH 5/6] cleanup --- packages/core/src/tracing/sentrySpan.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/core/src/tracing/sentrySpan.ts b/packages/core/src/tracing/sentrySpan.ts index f05ded75c670..acc14d37bc31 100644 --- a/packages/core/src/tracing/sentrySpan.ts +++ b/packages/core/src/tracing/sentrySpan.ts @@ -333,8 +333,6 @@ export class SentrySpan implements Span { } const { scope: capturedSpanScope, isolationScope: capturedSpanIsolationScope } = getCapturedScopesOnSpan(this); - const scope = capturedSpanScope || getCurrentScope(); - const client = scope.getClient() || getClient(); if (this._sampled !== true) { return undefined; From 71a05a7cd96b30e850287b2e97b06bf3d521eb85 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 8 Jan 2025 15:02:34 +0100 Subject: [PATCH 6/6] fix test --- .../suites/tracing/requests/http-unsampled/test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-unsampled/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-unsampled/test.ts index 3d2e0e421863..0574693d9961 100644 --- a/dev-packages/node-integration-tests/suites/tracing/requests/http-unsampled/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/requests/http-unsampled/test.ts @@ -27,6 +27,7 @@ test('outgoing http requests are correctly instrumented when not sampled', done .then(([SERVER_URL, closeTestServer]) => { createRunner(__dirname, 'scenario.ts') .withEnv({ SERVER_URL }) + .ignore('client_report') .expect({ event: { exception: {