Skip to content

Commit 99d1114

Browse files
author
Luca Forstner
authored
fix: Make startSpan, startSpanManual and startInactiveSpan pick up the scopes at time of creation instead of termination (getsentry#10492)
1 parent 028f4d5 commit 99d1114

File tree

5 files changed

+242
-16
lines changed

5 files changed

+242
-16
lines changed

‎packages/core/src/baseclient.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -222,8 +222,11 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
222222

223223
let eventId: string | undefined = hint && hint.event_id;
224224

225+
const sdkProcessingMetadata = event.sdkProcessingMetadata || {};
226+
const capturedSpanScope: Scope | undefined = sdkProcessingMetadata.capturedSpanScope;
227+
225228
this._process(
226-
this._captureEvent(event, hint, scope).then(result => {
229+
this._captureEvent(event, hint, capturedSpanScope || scope).then(result => {
227230
eventId = result;
228231
}),
229232
);
@@ -753,7 +756,10 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
753756

754757
const dataCategory: DataCategory = eventType === 'replay_event' ? 'replay' : eventType;
755758

756-
return this._prepareEvent(event, hint, scope)
759+
const sdkProcessingMetadata = event.sdkProcessingMetadata || {};
760+
const capturedSpanIsolationScope: Scope | undefined = sdkProcessingMetadata.capturedSpanIsolationScope;
761+
762+
return this._prepareEvent(event, hint, scope, capturedSpanIsolationScope)
757763
.then(prepared => {
758764
if (prepared === null) {
759765
this.recordDroppedEvent('event_processor', dataCategory, event);

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

Lines changed: 48 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
1-
import type { Span, SpanTimeInput, StartSpanOptions, TransactionContext } from '@sentry/types';
1+
import type { Scope, Span, SpanTimeInput, StartSpanOptions, TransactionContext } from '@sentry/types';
22

3-
import { dropUndefinedKeys, logger, tracingContextFromHeaders } from '@sentry/utils';
3+
import { addNonEnumerableProperty, dropUndefinedKeys, logger, tracingContextFromHeaders } from '@sentry/utils';
44

55
import { DEBUG_BUILD } from '../debug-build';
66
import { getCurrentScope, withScope } from '../exports';
@@ -189,20 +189,22 @@ export function startInactiveSpan(context: StartSpanOptions): Span | undefined {
189189
return undefined;
190190
}
191191

192+
const isolationScope = getIsolationScope();
193+
const scope = getCurrentScope();
194+
195+
let span: Span | undefined;
196+
192197
if (parentSpan) {
193198
// eslint-disable-next-line deprecation/deprecation
194-
return parentSpan.startChild(ctx);
199+
span = parentSpan.startChild(ctx);
195200
} else {
196-
const isolationScope = getIsolationScope();
197-
const scope = getCurrentScope();
198-
199201
const { traceId, dsc, parentSpanId, sampled } = {
200202
...isolationScope.getPropagationContext(),
201203
...scope.getPropagationContext(),
202204
};
203205

204206
// eslint-disable-next-line deprecation/deprecation
205-
return hub.startTransaction({
207+
span = hub.startTransaction({
206208
traceId,
207209
parentSpanId,
208210
parentSampled: sampled,
@@ -214,6 +216,10 @@ export function startInactiveSpan(context: StartSpanOptions): Span | undefined {
214216
},
215217
});
216218
}
219+
220+
setCapturedScopesOnSpan(span, scope, isolationScope);
221+
222+
return span;
217223
}
218224

219225
/**
@@ -335,20 +341,21 @@ function createChildSpanOrTransaction(
335341
return undefined;
336342
}
337343

344+
const isolationScope = getIsolationScope();
345+
const scope = getCurrentScope();
346+
347+
let span: Span | undefined;
338348
if (parentSpan) {
339349
// eslint-disable-next-line deprecation/deprecation
340-
return parentSpan.startChild(ctx);
350+
span = parentSpan.startChild(ctx);
341351
} else {
342-
const isolationScope = getIsolationScope();
343-
const scope = getCurrentScope();
344-
345352
const { traceId, dsc, parentSpanId, sampled } = {
346353
...isolationScope.getPropagationContext(),
347354
...scope.getPropagationContext(),
348355
};
349356

350357
// eslint-disable-next-line deprecation/deprecation
351-
return hub.startTransaction({
358+
span = hub.startTransaction({
352359
traceId,
353360
parentSpanId,
354361
parentSampled: sampled,
@@ -360,6 +367,10 @@ function createChildSpanOrTransaction(
360367
},
361368
});
362369
}
370+
371+
setCapturedScopesOnSpan(span, scope, isolationScope);
372+
373+
return span;
363374
}
364375

365376
/**
@@ -379,3 +390,28 @@ function normalizeContext(context: StartSpanOptions): TransactionContext {
379390

380391
return context;
381392
}
393+
394+
const SCOPE_ON_START_SPAN_FIELD = '_sentryScope';
395+
const ISOLATION_SCOPE_ON_START_SPAN_FIELD = '_sentryIsolationScope';
396+
397+
type SpanWithScopes = Span & {
398+
[SCOPE_ON_START_SPAN_FIELD]?: Scope;
399+
[ISOLATION_SCOPE_ON_START_SPAN_FIELD]?: Scope;
400+
};
401+
402+
function setCapturedScopesOnSpan(span: Span | undefined, scope: Scope, isolationScope: Scope): void {
403+
if (span) {
404+
addNonEnumerableProperty(span, ISOLATION_SCOPE_ON_START_SPAN_FIELD, isolationScope);
405+
addNonEnumerableProperty(span, SCOPE_ON_START_SPAN_FIELD, scope);
406+
}
407+
}
408+
409+
/**
410+
* Grabs the scope and isolation scope off a span that were active when the span was started.
411+
*/
412+
export function getCapturedScopesOnSpan(span: Span): { scope?: Scope; isolationScope?: Scope } {
413+
return {
414+
scope: (span as SpanWithScopes)[SCOPE_ON_START_SPAN_FIELD],
415+
isolationScope: (span as SpanWithScopes)[ISOLATION_SCOPE_ON_START_SPAN_FIELD],
416+
};
417+
}

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE
1919
import { spanTimeInputToSeconds, spanToJSON, spanToTraceContext } from '../utils/spanUtils';
2020
import { getDynamicSamplingContextFromSpan } from './dynamicSamplingContext';
2121
import { Span as SpanClass, SpanRecorder } from './span';
22+
import { getCapturedScopesOnSpan } from './trace';
2223

2324
/** JSDoc */
2425
export class Transaction extends SpanClass implements TransactionInterface {
@@ -303,6 +304,8 @@ export class Transaction extends SpanClass implements TransactionInterface {
303304
});
304305
}
305306

307+
const { scope: capturedSpanScope, isolationScope: capturedSpanIsolationScope } = getCapturedScopesOnSpan(this);
308+
306309
// eslint-disable-next-line deprecation/deprecation
307310
const { metadata } = this;
308311
// eslint-disable-next-line deprecation/deprecation
@@ -324,6 +327,8 @@ export class Transaction extends SpanClass implements TransactionInterface {
324327
type: 'transaction',
325328
sdkProcessingMetadata: {
326329
...metadata,
330+
capturedSpanScope,
331+
capturedSpanIsolationScope,
327332
dynamicSamplingContext: getDynamicSamplingContextFromSpan(this),
328333
},
329334
...(source && {

‎packages/core/test/lib/tracing/trace.test.ts

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import type { Span as SpanType } from '@sentry/types';
12
import {
23
Hub,
34
SEMANTIC_ATTRIBUTE_SENTRY_OP,
@@ -387,9 +388,50 @@ describe('startSpan', () => {
387388
transactionContext: expect.objectContaining({ name: 'outer', parentSampled: undefined }),
388389
});
389390
});
391+
392+
it('includes the scope at the time the span was started when finished', async () => {
393+
const transactionEventPromise = new Promise(resolve => {
394+
setCurrentClient(
395+
new TestClient(
396+
getDefaultTestClientOptions({
397+
dsn: 'https://username@domain/123',
398+
tracesSampleRate: 1,
399+
beforeSendTransaction(event) {
400+
resolve(event);
401+
return event;
402+
},
403+
}),
404+
),
405+
);
406+
});
407+
408+
withScope(scope1 => {
409+
scope1.setTag('scope', 1);
410+
startSpanManual({ name: 'my-span' }, span => {
411+
withScope(scope2 => {
412+
scope2.setTag('scope', 2);
413+
span?.end();
414+
});
415+
});
416+
});
417+
418+
expect(await transactionEventPromise).toMatchObject({
419+
tags: {
420+
scope: 1,
421+
},
422+
});
423+
});
390424
});
391425

392426
describe('startSpanManual', () => {
427+
beforeEach(() => {
428+
const options = getDefaultTestClientOptions({ tracesSampleRate: 1 });
429+
client = new TestClient(options);
430+
hub = new Hub(client);
431+
// eslint-disable-next-line deprecation/deprecation
432+
makeMain(hub);
433+
});
434+
393435
it('creates & finishes span', async () => {
394436
startSpanManual({ name: 'GET users/[id]' }, (span, finish) => {
395437
expect(span).toBeDefined();
@@ -492,6 +534,14 @@ describe('startSpanManual', () => {
492534
});
493535

494536
describe('startInactiveSpan', () => {
537+
beforeEach(() => {
538+
const options = getDefaultTestClientOptions({ tracesSampleRate: 1 });
539+
client = new TestClient(options);
540+
hub = new Hub(client);
541+
// eslint-disable-next-line deprecation/deprecation
542+
makeMain(hub);
543+
});
544+
495545
it('creates & finishes span', async () => {
496546
const span = startInactiveSpan({ name: 'GET users/[id]' });
497547

@@ -571,6 +621,41 @@ describe('startInactiveSpan', () => {
571621
expect(span).toBeDefined();
572622
});
573623
});
624+
625+
it('includes the scope at the time the span was started when finished', async () => {
626+
const transactionEventPromise = new Promise(resolve => {
627+
setCurrentClient(
628+
new TestClient(
629+
getDefaultTestClientOptions({
630+
dsn: 'https://username@domain/123',
631+
tracesSampleRate: 1,
632+
beforeSendTransaction(event) {
633+
resolve(event);
634+
return event;
635+
},
636+
}),
637+
),
638+
);
639+
});
640+
641+
let span: SpanType | undefined;
642+
643+
withScope(scope => {
644+
scope.setTag('scope', 1);
645+
span = startInactiveSpan({ name: 'my-span' });
646+
});
647+
648+
withScope(scope => {
649+
scope.setTag('scope', 2);
650+
span?.end();
651+
});
652+
653+
expect(await transactionEventPromise).toMatchObject({
654+
tags: {
655+
scope: 1,
656+
},
657+
});
658+
});
574659
});
575660

576661
describe('continueTrace', () => {

‎packages/node/test/performance.test.ts

Lines changed: 96 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,13 @@
1-
import { setAsyncContextStrategy, setCurrentClient, startSpan, startSpanManual } from '@sentry/core';
2-
import type { TransactionEvent } from '@sentry/types';
1+
import {
2+
setAsyncContextStrategy,
3+
setCurrentClient,
4+
startInactiveSpan,
5+
startSpan,
6+
startSpanManual,
7+
withIsolationScope,
8+
withScope,
9+
} from '@sentry/core';
10+
import type { Span, TransactionEvent } from '@sentry/types';
311
import { NodeClient, defaultStackParser } from '../src';
412
import { setNodeAsyncContextStrategy } from '../src/async';
513
import { getDefaultNodeClientOptions } from './helper/node-client-options';
@@ -147,4 +155,90 @@ describe('startSpanManual()', () => {
147155

148156
expect(transactionEvent.spans).toContainEqual(expect.objectContaining({ description: 'second' }));
149157
});
158+
159+
it('should use the scopes at time of creation instead of the scopes at time of termination', async () => {
160+
const transactionEventPromise = new Promise<TransactionEvent>(resolve => {
161+
setCurrentClient(
162+
new NodeClient(
163+
getDefaultNodeClientOptions({
164+
stackParser: defaultStackParser,
165+
tracesSampleRate: 1,
166+
beforeSendTransaction: event => {
167+
resolve(event);
168+
return null;
169+
},
170+
dsn,
171+
}),
172+
),
173+
);
174+
});
175+
176+
withIsolationScope(isolationScope1 => {
177+
isolationScope1.setTag('isolationScope', 1);
178+
withScope(scope1 => {
179+
scope1.setTag('scope', 1);
180+
startSpanManual({ name: 'my-span' }, span => {
181+
withIsolationScope(isolationScope2 => {
182+
isolationScope2.setTag('isolationScope', 2);
183+
withScope(scope2 => {
184+
scope2.setTag('scope', 2);
185+
span?.end();
186+
});
187+
});
188+
});
189+
});
190+
});
191+
192+
expect(await transactionEventPromise).toMatchObject({
193+
tags: {
194+
scope: 1,
195+
isolationScope: 1,
196+
},
197+
});
198+
});
199+
});
200+
201+
describe('startInactiveSpan()', () => {
202+
it('should use the scopes at time of creation instead of the scopes at time of termination', async () => {
203+
const transactionEventPromise = new Promise<TransactionEvent>(resolve => {
204+
setCurrentClient(
205+
new NodeClient(
206+
getDefaultNodeClientOptions({
207+
stackParser: defaultStackParser,
208+
tracesSampleRate: 1,
209+
beforeSendTransaction: event => {
210+
resolve(event);
211+
return null;
212+
},
213+
dsn,
214+
}),
215+
),
216+
);
217+
});
218+
219+
let span: Span | undefined;
220+
221+
withIsolationScope(isolationScope => {
222+
isolationScope.setTag('isolationScope', 1);
223+
withScope(scope => {
224+
scope.setTag('scope', 1);
225+
span = startInactiveSpan({ name: 'my-span' });
226+
});
227+
});
228+
229+
withIsolationScope(isolationScope => {
230+
isolationScope.setTag('isolationScope', 2);
231+
withScope(scope => {
232+
scope.setTag('scope', 2);
233+
span?.end();
234+
});
235+
});
236+
237+
expect(await transactionEventPromise).toMatchObject({
238+
tags: {
239+
scope: 1,
240+
isolationScope: 1,
241+
},
242+
});
243+
});
150244
});

0 commit comments

Comments
 (0)