Skip to content

Commit b0c0004

Browse files
mydeamydea
authored
feat(core): Emit client reports for unsampled root spans on span start (#14936)
With this PR, the `sample_rate`,`transaction` client report is now consistently emitted at the time when the root span is sampled. Previously, in Node (OTEL) we did not emit this at all, while in browser we emit it at span end time. This is inconsistent and not ideal. Emitting it in OTEL at span end time is difficult because `span.end()` is a no-op there and you do not get access to it anywhere. So doing it at span start (=sample time) is easier, and also makes more sense as this is also actually the time when the span is dropped.
1 parent 3e73850 commit b0c0004

File tree

15 files changed

+181
-33
lines changed

15 files changed

+181
-33
lines changed

‎dev-packages/node-integration-tests/suites/tracing/requests/http-unsampled/test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ test('outgoing http requests are correctly instrumented when not sampled', done
2727
.then(([SERVER_URL, closeTestServer]) => {
2828
createRunner(__dirname, 'scenario.ts')
2929
.withEnv({ SERVER_URL })
30+
.ignore('client_report')
3031
.expect({
3132
event: {
3233
exception: {

‎packages/core/src/tracing/idleSpan.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,12 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti
124124
beforeSpanEnd(span);
125125
}
126126

127+
// If the span is non-recording, nothing more to do here...
128+
// This is the case if tracing is enabled but this specific span was not sampled
129+
if (thisArg instanceof SentryNonRecordingSpan) {
130+
return;
131+
}
132+
127133
// Just ensuring that this keeps working, even if we ever have more arguments here
128134
const [definedEndTimestamp, ...rest] = args;
129135
const timestamp = definedEndTimestamp || timestampInSeconds();

‎packages/core/src/tracing/sentrySpan.ts

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -333,17 +333,8 @@ export class SentrySpan implements Span {
333333
}
334334

335335
const { scope: capturedSpanScope, isolationScope: capturedSpanIsolationScope } = getCapturedScopesOnSpan(this);
336-
const scope = capturedSpanScope || getCurrentScope();
337-
const client = scope.getClient() || getClient();
338336

339337
if (this._sampled !== true) {
340-
// At this point if `sampled !== true` we want to discard the transaction.
341-
DEBUG_BUILD && logger.log('[Tracing] Discarding transaction because its trace was not chosen to be sampled.');
342-
343-
if (client) {
344-
client.recordDroppedEvent('sample_rate', 'transaction');
345-
}
346-
347338
return undefined;
348339
}
349340

‎packages/core/src/tracing/trace.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -395,6 +395,12 @@ function _startRootSpan(spanArguments: SentrySpanArguments, scope: Scope, parent
395395
},
396396
sampled,
397397
});
398+
399+
if (!sampled && client) {
400+
DEBUG_BUILD && logger.log('[Tracing] Discarding root span because its trace was not chosen to be sampled.');
401+
client.recordDroppedEvent('sample_rate', 'transaction');
402+
}
403+
398404
if (sampleRate !== undefined) {
399405
rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, sampleRate);
400406
}

‎packages/opentelemetry/src/sampler.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,11 @@ export class SentrySampler implements Sampler {
120120
}
121121

122122
if (!sampled) {
123+
if (parentSampled === undefined) {
124+
DEBUG_BUILD && logger.log('[Tracing] Discarding root span because its trace was not chosen to be sampled.');
125+
this._client.recordDroppedEvent('sample_rate', 'transaction');
126+
}
127+
123128
return {
124129
...wrapSamplingDecision({ decision: SamplingDecision.NOT_RECORD, context, spanAttributes }),
125130
attributes,

‎packages/opentelemetry/test/helpers/TestClient.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ export const TestClient = wrapClientClass(BaseTestClient);
3333
export type TestClientInterface = Client & OpenTelemetryClient;
3434

3535
export function init(options: Partial<Options> = {}): void {
36-
const client = new TestClient(getDefaultTestClientOptions(options));
36+
const client = new TestClient(getDefaultTestClientOptions({ tracesSampleRate: 1, ...options }));
3737

3838
// The client is on the current scope, from where it generally is inherited
3939
getCurrentScope().setClient(client);
@@ -42,7 +42,6 @@ export function init(options: Partial<Options> = {}): void {
4242

4343
export function getDefaultTestClientOptions(options: Partial<Options> = {}): ClientOptions {
4444
return {
45-
enableTracing: true,
4645
integrations: [],
4746
transport: () => createTransport({ recordDroppedEvent: () => undefined }, _ => resolvedSyncPromise({})),
4847
stackParser: () => [],

‎packages/opentelemetry/test/integration/breadcrumbs.test.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ describe('Integration | breadcrumbs', () => {
9898
const beforeSend = jest.fn(() => null);
9999
const beforeBreadcrumb = jest.fn(breadcrumb => breadcrumb);
100100

101-
mockSdkInit({ beforeSend, beforeBreadcrumb, beforeSendTransaction, enableTracing: true });
101+
mockSdkInit({ beforeSend, beforeBreadcrumb, beforeSendTransaction, tracesSampleRate: 1 });
102102

103103
const client = getClient() as TestClientInterface;
104104

@@ -143,7 +143,7 @@ describe('Integration | breadcrumbs', () => {
143143
const beforeSend = jest.fn(() => null);
144144
const beforeBreadcrumb = jest.fn(breadcrumb => breadcrumb);
145145

146-
mockSdkInit({ beforeSend, beforeBreadcrumb, beforeSendTransaction, enableTracing: true });
146+
mockSdkInit({ beforeSend, beforeBreadcrumb, beforeSendTransaction, tracesSampleRate: 1 });
147147

148148
const client = getClient() as TestClientInterface;
149149

@@ -195,7 +195,7 @@ describe('Integration | breadcrumbs', () => {
195195
const beforeSend = jest.fn(() => null);
196196
const beforeBreadcrumb = jest.fn(breadcrumb => breadcrumb);
197197

198-
mockSdkInit({ beforeSend, beforeBreadcrumb, beforeSendTransaction, enableTracing: true });
198+
mockSdkInit({ beforeSend, beforeBreadcrumb, beforeSendTransaction, tracesSampleRate: 1 });
199199

200200
const client = getClient() as TestClientInterface;
201201

@@ -236,7 +236,7 @@ describe('Integration | breadcrumbs', () => {
236236
const beforeSend = jest.fn(() => null);
237237
const beforeBreadcrumb = jest.fn(breadcrumb => breadcrumb);
238238

239-
mockSdkInit({ beforeSend, beforeBreadcrumb, beforeSendTransaction, enableTracing: true });
239+
mockSdkInit({ beforeSend, beforeBreadcrumb, beforeSendTransaction, tracesSampleRate: 1 });
240240

241241
const client = getClient() as TestClientInterface;
242242

@@ -294,7 +294,7 @@ describe('Integration | breadcrumbs', () => {
294294
const beforeSend = jest.fn(() => null);
295295
const beforeBreadcrumb = jest.fn(breadcrumb => breadcrumb);
296296

297-
mockSdkInit({ beforeSend, beforeBreadcrumb, beforeSendTransaction, enableTracing: true });
297+
mockSdkInit({ beforeSend, beforeBreadcrumb, beforeSendTransaction, tracesSampleRate: 1 });
298298

299299
const client = getClient() as TestClientInterface;
300300

‎packages/opentelemetry/test/integration/scope.test.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,11 @@ describe('Integration | Scope', () => {
2626
const beforeSend = jest.fn(() => null);
2727
const beforeSendTransaction = jest.fn(() => null);
2828

29-
mockSdkInit({ enableTracing, beforeSend, beforeSendTransaction });
29+
mockSdkInit({
30+
tracesSampleRate: enableTracing ? 1 : 0,
31+
beforeSend,
32+
beforeSendTransaction,
33+
});
3034

3135
const client = getClient() as TestClientInterface;
3236

‎packages/opentelemetry/test/integration/transactions.test.ts

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ describe('Integration | Transactions', () => {
3737
});
3838

3939
mockSdkInit({
40-
enableTracing: true,
40+
tracesSampleRate: 1,
4141
beforeSendTransaction,
4242
release: '8.0.0',
4343
});
@@ -178,7 +178,7 @@ describe('Integration | Transactions', () => {
178178
it('correctly creates concurrent transaction & spans', async () => {
179179
const beforeSendTransaction = jest.fn(() => null);
180180

181-
mockSdkInit({ enableTracing: true, beforeSendTransaction });
181+
mockSdkInit({ tracesSampleRate: 1, beforeSendTransaction });
182182

183183
const client = getClient() as TestClientInterface;
184184

@@ -339,7 +339,7 @@ describe('Integration | Transactions', () => {
339339
traceState,
340340
};
341341

342-
mockSdkInit({ enableTracing: true, beforeSendTransaction });
342+
mockSdkInit({ tracesSampleRate: 1, beforeSendTransaction });
343343

344344
const client = getClient() as TestClientInterface;
345345

@@ -443,7 +443,7 @@ describe('Integration | Transactions', () => {
443443
const logs: unknown[] = [];
444444
jest.spyOn(logger, 'log').mockImplementation(msg => logs.push(msg));
445445

446-
mockSdkInit({ enableTracing: true, beforeSendTransaction });
446+
mockSdkInit({ tracesSampleRate: 1, beforeSendTransaction });
447447

448448
const provider = getProvider();
449449
const multiSpanProcessor = provider?.activeSpanProcessor as
@@ -516,7 +516,7 @@ describe('Integration | Transactions', () => {
516516
const transactions: Event[] = [];
517517

518518
mockSdkInit({
519-
enableTracing: true,
519+
tracesSampleRate: 1,
520520
beforeSendTransaction: event => {
521521
transactions.push(event);
522522
return null;
@@ -573,7 +573,7 @@ describe('Integration | Transactions', () => {
573573
const transactions: Event[] = [];
574574

575575
mockSdkInit({
576-
enableTracing: true,
576+
tracesSampleRate: 1,
577577
beforeSendTransaction: event => {
578578
transactions.push(event);
579579
return null;
@@ -644,7 +644,7 @@ describe('Integration | Transactions', () => {
644644
};
645645

646646
mockSdkInit({
647-
enableTracing: true,
647+
tracesSampleRate: 1,
648648
beforeSendTransaction,
649649
release: '7.0.0',
650650
});

‎packages/opentelemetry/test/propagator.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ describe('SentryPropagator', () => {
2525
mockSdkInit({
2626
environment: 'production',
2727
release: '1.0.0',
28-
enableTracing: true,
28+
tracesSampleRate: 1,
2929
dsn: 'https://abc@domain/123',
3030
});
3131
});

‎packages/opentelemetry/test/sampler.test.ts

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
import { SpanKind, context, trace } from '@opentelemetry/api';
2+
import { TraceState } from '@opentelemetry/core';
3+
import { SamplingDecision } from '@opentelemetry/sdk-trace-base';
4+
import { ATTR_HTTP_REQUEST_METHOD } from '@opentelemetry/semantic-conventions';
5+
import { generateSpanId, generateTraceId } from '@sentry/core';
6+
import { SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING } from '../src/constants';
7+
import { SentrySampler } from '../src/sampler';
8+
import { TestClient, getDefaultTestClientOptions } from './helpers/TestClient';
9+
import { cleanupOtel } from './helpers/mockSdkInit';
10+
11+
describe('SentrySampler', () => {
12+
afterEach(() => {
13+
cleanupOtel();
14+
});
15+
16+
it('works with tracesSampleRate=0', () => {
17+
const client = new TestClient(getDefaultTestClientOptions({ tracesSampleRate: 0 }));
18+
const spyOnDroppedEvent = jest.spyOn(client, 'recordDroppedEvent');
19+
const sampler = new SentrySampler(client);
20+
21+
const ctx = context.active();
22+
const traceId = generateTraceId();
23+
const spanName = 'test';
24+
const spanKind = SpanKind.INTERNAL;
25+
const spanAttributes = {};
26+
const links = undefined;
27+
28+
const actual = sampler.shouldSample(ctx, traceId, spanName, spanKind, spanAttributes, links);
29+
expect(actual).toEqual({
30+
decision: SamplingDecision.NOT_RECORD,
31+
attributes: { 'sentry.sample_rate': 0 },
32+
traceState: new TraceState().set('sentry.sampled_not_recording', '1'),
33+
});
34+
expect(spyOnDroppedEvent).toHaveBeenCalledTimes(1);
35+
expect(spyOnDroppedEvent).toHaveBeenCalledWith('sample_rate', 'transaction');
36+
37+
spyOnDroppedEvent.mockReset();
38+
});
39+
40+
it('works with tracesSampleRate=0 & for a child span', () => {
41+
const client = new TestClient(getDefaultTestClientOptions({ tracesSampleRate: 0 }));
42+
const spyOnDroppedEvent = jest.spyOn(client, 'recordDroppedEvent');
43+
const sampler = new SentrySampler(client);
44+
45+
const traceId = generateTraceId();
46+
const ctx = trace.setSpanContext(context.active(), {
47+
spanId: generateSpanId(),
48+
traceId,
49+
traceFlags: 0,
50+
traceState: new TraceState().set(SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING, '1'),
51+
});
52+
const spanName = 'test';
53+
const spanKind = SpanKind.INTERNAL;
54+
const spanAttributes = {};
55+
const links = undefined;
56+
57+
const actual = sampler.shouldSample(ctx, traceId, spanName, spanKind, spanAttributes, links);
58+
expect(actual).toEqual({
59+
decision: SamplingDecision.NOT_RECORD,
60+
attributes: { 'sentry.sample_rate': 0 },
61+
traceState: new TraceState().set(SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING, '1'),
62+
});
63+
expect(spyOnDroppedEvent).toHaveBeenCalledTimes(0);
64+
65+
spyOnDroppedEvent.mockReset();
66+
});
67+
68+
it('works with tracesSampleRate=1', () => {
69+
const client = new TestClient(getDefaultTestClientOptions({ tracesSampleRate: 1 }));
70+
const spyOnDroppedEvent = jest.spyOn(client, 'recordDroppedEvent');
71+
const sampler = new SentrySampler(client);
72+
73+
const ctx = context.active();
74+
const traceId = generateTraceId();
75+
const spanName = 'test';
76+
const spanKind = SpanKind.INTERNAL;
77+
const spanAttributes = {};
78+
const links = undefined;
79+
80+
const actual = sampler.shouldSample(ctx, traceId, spanName, spanKind, spanAttributes, links);
81+
expect(actual).toEqual({
82+
decision: SamplingDecision.RECORD_AND_SAMPLED,
83+
attributes: { 'sentry.sample_rate': 1 },
84+
traceState: new TraceState(),
85+
});
86+
expect(spyOnDroppedEvent).toHaveBeenCalledTimes(0);
87+
88+
spyOnDroppedEvent.mockReset();
89+
});
90+
91+
it('works with traceSampleRate=undefined', () => {
92+
const client = new TestClient(getDefaultTestClientOptions({ tracesSampleRate: undefined }));
93+
const spyOnDroppedEvent = jest.spyOn(client, 'recordDroppedEvent');
94+
const sampler = new SentrySampler(client);
95+
96+
const ctx = context.active();
97+
const traceId = generateTraceId();
98+
const spanName = 'test';
99+
const spanKind = SpanKind.INTERNAL;
100+
const spanAttributes = {};
101+
const links = undefined;
102+
103+
const actual = sampler.shouldSample(ctx, traceId, spanName, spanKind, spanAttributes, links);
104+
expect(actual).toEqual({
105+
decision: SamplingDecision.NOT_RECORD,
106+
traceState: new TraceState(),
107+
});
108+
expect(spyOnDroppedEvent).toHaveBeenCalledTimes(0);
109+
110+
spyOnDroppedEvent.mockReset();
111+
});
112+
113+
it('ignores local http client root spans', () => {
114+
const client = new TestClient(getDefaultTestClientOptions({ tracesSampleRate: 0 }));
115+
const spyOnDroppedEvent = jest.spyOn(client, 'recordDroppedEvent');
116+
const sampler = new SentrySampler(client);
117+
118+
const ctx = context.active();
119+
const traceId = generateTraceId();
120+
const spanName = 'test';
121+
const spanKind = SpanKind.CLIENT;
122+
const spanAttributes = {
123+
[ATTR_HTTP_REQUEST_METHOD]: 'GET',
124+
};
125+
const links = undefined;
126+
127+
const actual = sampler.shouldSample(ctx, traceId, spanName, spanKind, spanAttributes, links);
128+
expect(actual).toEqual({
129+
decision: SamplingDecision.NOT_RECORD,
130+
traceState: new TraceState(),
131+
});
132+
expect(spyOnDroppedEvent).toHaveBeenCalledTimes(0);
133+
134+
spyOnDroppedEvent.mockReset();
135+
});
136+
});

‎packages/opentelemetry/test/spanExporter.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { cleanupOtel, mockSdkInit } from './helpers/mockSdkInit';
66
describe('createTransactionForOtelSpan', () => {
77
beforeEach(() => {
88
mockSdkInit({
9-
enableTracing: true,
9+
tracesSampleRate: 1,
1010
});
1111
});
1212

0 commit comments

Comments
 (0)