Skip to content

Commit 52b10a8

Browse files
committed
rework implementation to fork scope instead of manual cleanup
1 parent 547ec0f commit 52b10a8

File tree

4 files changed

+95
-33
lines changed

4 files changed

+95
-33
lines changed

packages/core/src/tracing/trace.ts

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -46,9 +46,11 @@ export function startSpan<T>(options: StartSpanOptions, callback: (span: Span) =
4646
const spanArguments = parseSentrySpanArguments(options);
4747
const { forceTransaction, parentSpan: customParentSpan, scope: customScope } = options;
4848

49-
const previousActiveSpanOnCustomScope = customScope && _getSpanForScope(customScope);
49+
// We still need to fork a potentially passed scope, as we set the active span on it
50+
// and we need to ensure that it is cleaned up properly once the span ends.
51+
const customForkedScope = customScope?.clone();
5052

51-
return withScope(customScope, () => {
53+
return withScope(customForkedScope, () => {
5254
// If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan`
5355
const wrapper = getActiveSpanWrapper<T>(customParentSpan);
5456

@@ -79,11 +81,6 @@ export function startSpan<T>(options: StartSpanOptions, callback: (span: Span) =
7981
},
8082
() => {
8183
activeSpan.end();
82-
if (customScope) {
83-
// If a custom scope is passed, we don't fork the scope. Therefore, we need to reset the
84-
// active span on the scope to the previous active span (or to undefined if there was none)
85-
_setSpanForScope(customScope, previousActiveSpanOnCustomScope);
86-
}
8784
},
8885
);
8986
});

packages/core/src/types-hoist/startSpanOptions.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,16 @@ export interface StartSpanOptions {
55
/** A manually specified start time for the created `Span` object. */
66
startTime?: SpanTimeInput;
77

8-
/** If defined, start this span off this scope instead off the current scope. */
8+
/**
9+
* If set, start the span on a fork of this scope instead of on the current scope.
10+
* To ensure proper span cleanup, the passed scope is cloned for the duration of the span.
11+
*
12+
* If you want to modify the passed scope inside the callback, be aware that:
13+
* - calling `getCurrentScope()` will return the cloned scope, meaning all modifications
14+
* will be reset once the callback finishes
15+
* - if you want to modify the passed scope and have the changes persist after the callback
16+
* ends, modify the scope directly and don't use `getCurrentScope()`
17+
*/
918
scope?: Scope;
1019

1120
/** The name of the span. */

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

Lines changed: 58 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ import {
2525
import { SentryNonRecordingSpan } from '../../../src/tracing/sentryNonRecordingSpan';
2626
import { startNewTrace } from '../../../src/tracing/trace';
2727
import type { Event, Span, StartSpanOptions } from '../../../src/types-hoist';
28-
import { _setSpanForScope } from '../../../src/utils/spanOnScope';
28+
import { _getSpanForScope, _setSpanForScope } from '../../../src/utils/spanOnScope';
2929
import { getActiveSpan, getRootSpan, getSpanDescendants, spanIsSampled } from '../../../src/utils/spanUtils';
3030
import { TestClient, getDefaultTestClientOptions } from '../../mocks/client';
3131

@@ -251,20 +251,44 @@ describe('startSpan', () => {
251251
expect(getActiveSpan()).toBe(undefined);
252252
});
253253

254-
it('allows to pass a scope', () => {
254+
it('starts the span on the fork of a passed custom scope', () => {
255255
const initialScope = getCurrentScope();
256256

257-
const manualScope = initialScope.clone();
257+
const customScope = initialScope.clone();
258+
customScope.setTag('dogs', 'great');
259+
258260
const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true });
259-
_setSpanForScope(manualScope, parentSpan);
261+
_setSpanForScope(customScope, parentSpan);
260262

261-
startSpan({ name: 'GET users/[id]', scope: manualScope }, span => {
263+
startSpan({ name: 'GET users/[id]', scope: customScope }, span => {
264+
// current scope is forked from the customScope
262265
expect(getCurrentScope()).not.toBe(initialScope);
263-
expect(getCurrentScope()).toBe(manualScope);
266+
expect(getCurrentScope()).not.toBe(customScope);
267+
expect(getCurrentScope().getScopeData().tags).toEqual({ dogs: 'great' });
268+
269+
// active span is set correctly
264270
expect(getActiveSpan()).toBe(span);
271+
272+
// span has the correct parent span
265273
expect(spanToJSON(span).parent_span_id).toBe('parent-span-id');
274+
275+
// scope data modifications
276+
getCurrentScope().setTag('cats', 'great');
277+
customScope.setTag('bears', 'great');
278+
279+
expect(getCurrentScope().getScopeData().tags).toEqual({ dogs: 'great', cats: 'great' });
280+
expect(customScope.getScopeData().tags).toEqual({ dogs: 'great', bears: 'great' });
266281
});
267282

283+
// customScope modifications are persisted
284+
expect(customScope.getScopeData().tags).toEqual({ dogs: 'great', bears: 'great' });
285+
286+
// span is parent span again on customScope
287+
withScope(customScope, () => {
288+
expect(getActiveSpan()).toBe(parentSpan);
289+
});
290+
291+
// but activeSpan and currentScope are reset, since customScope was never active
268292
expect(getCurrentScope()).toBe(initialScope);
269293
expect(getActiveSpan()).toBe(undefined);
270294
});
@@ -273,29 +297,35 @@ describe('startSpan', () => {
273297
it('with parent span', () => {
274298
const initialScope = getCurrentScope();
275299

276-
const manualScope = initialScope.clone();
300+
const customScope = initialScope.clone();
277301
const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true });
278-
_setSpanForScope(manualScope, parentSpan);
302+
_setSpanForScope(customScope, parentSpan);
279303

280-
startSpan({ name: 'span 1', scope: manualScope }, span1 => {
304+
startSpan({ name: 'span 1', scope: customScope }, span1 => {
305+
// current scope is forked from the customScope
281306
expect(getCurrentScope()).not.toBe(initialScope);
282-
expect(getCurrentScope()).toBe(manualScope);
307+
expect(getCurrentScope()).not.toBe(customScope);
308+
283309
expect(getActiveSpan()).toBe(span1);
284310
expect(spanToJSON(span1).parent_span_id).toBe('parent-span-id');
285311
});
286312

287-
withScope(manualScope, () => {
313+
// active span on customScope is reset
314+
withScope(customScope, () => {
288315
expect(getActiveSpan()).toBe(parentSpan);
289316
});
290317

291-
startSpan({ name: 'span 2', scope: manualScope }, span2 => {
318+
startSpan({ name: 'span 2', scope: customScope }, span2 => {
319+
// current scope is forked from the customScope
292320
expect(getCurrentScope()).not.toBe(initialScope);
293-
expect(getCurrentScope()).toBe(manualScope);
321+
expect(getCurrentScope()).not.toBe(customScope);
322+
294323
expect(getActiveSpan()).toBe(span2);
324+
// both, span1 and span2 are children of the parent span
295325
expect(spanToJSON(span2).parent_span_id).toBe('parent-span-id');
296326
});
297327

298-
withScope(manualScope, () => {
328+
withScope(customScope, () => {
299329
expect(getActiveSpan()).toBe(parentSpan);
300330
});
301331

@@ -305,31 +335,35 @@ describe('startSpan', () => {
305335

306336
it('without parent span', () => {
307337
const initialScope = getCurrentScope();
308-
const manualScope = initialScope.clone();
338+
const customScope = initialScope.clone();
309339

310-
const traceId = manualScope.getPropagationContext()?.traceId;
340+
const traceId = customScope.getPropagationContext()?.traceId;
311341

312-
startSpan({ name: 'span 1', scope: manualScope }, span1 => {
342+
startSpan({ name: 'span 1', scope: customScope }, span1 => {
313343
expect(getCurrentScope()).not.toBe(initialScope);
314-
expect(getCurrentScope()).toBe(manualScope);
344+
expect(getCurrentScope()).not.toBe(customScope);
345+
315346
expect(getActiveSpan()).toBe(span1);
316-
expect(spanToJSON(span1).parent_span_id).toBe(undefined);
347+
expect(getRootSpan(getActiveSpan()!)).toBe(span1);
348+
317349
expect(span1.spanContext().traceId).toBe(traceId);
318350
});
319351

320-
withScope(manualScope, () => {
352+
withScope(customScope, () => {
321353
expect(getActiveSpan()).toBe(undefined);
322354
});
323355

324-
startSpan({ name: 'span 2', scope: manualScope }, span2 => {
356+
startSpan({ name: 'span 2', scope: customScope }, span2 => {
325357
expect(getCurrentScope()).not.toBe(initialScope);
326-
expect(getCurrentScope()).toBe(manualScope);
358+
expect(getCurrentScope()).not.toBe(customScope);
359+
327360
expect(getActiveSpan()).toBe(span2);
328-
expect(spanToJSON(span2).parent_span_id).toBe(undefined);
361+
expect(getRootSpan(getActiveSpan()!)).toBe(span2);
362+
329363
expect(span2.spanContext().traceId).toBe(traceId);
330364
});
331365

332-
withScope(manualScope, () => {
366+
withScope(customScope, () => {
333367
expect(getActiveSpan()).toBe(undefined);
334368
});
335369

packages/opentelemetry/test/trace.test.ts

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -290,24 +290,46 @@ describe('trace', () => {
290290
let manualScope: Scope;
291291
let parentSpan: Span;
292292

293+
// "hack" to create a manual scope with a parent span
293294
startSpanManual({ name: 'detached' }, span => {
294295
parentSpan = span;
295296
manualScope = getCurrentScope();
296297
manualScope.setTag('manual', 'tag');
297298
});
298299

300+
expect(manualScope!.getScopeData().tags).toEqual({ manual: 'tag' });
301+
expect(getCurrentScope()).not.toBe(manualScope!);
302+
299303
getCurrentScope().setTag('outer', 'tag');
300304

301305
startSpan({ name: 'GET users/[id]', scope: manualScope! }, span => {
306+
// the current scope in the callback is a fork of the manual scope
302307
expect(getCurrentScope()).not.toBe(initialScope);
308+
expect(getCurrentScope()).not.toBe(manualScope);
309+
expect(getCurrentScope().getScopeData().tags).toEqual({ manual: 'tag' });
303310

304-
expect(getCurrentScope()).toEqual(manualScope);
311+
// getActiveSpan returns the correct span
305312
expect(getActiveSpan()).toBe(span);
306313

314+
// span hierarchy is correct
307315
expect(getSpanParentSpanId(span)).toBe(parentSpan.spanContext().spanId);
316+
317+
// scope data modifications are isolated between original and forked manual scope
318+
getCurrentScope().setTag('inner', 'tag');
319+
manualScope!.setTag('manual-scope-inner', 'tag');
320+
321+
expect(getCurrentScope().getScopeData().tags).toEqual({ manual: 'tag', inner: 'tag' });
322+
expect(manualScope!.getScopeData().tags).toEqual({ manual: 'tag', 'manual-scope-inner': 'tag' });
308323
});
309324

325+
// manualScope modifications remain set outside the callback
326+
expect(manualScope!.getScopeData().tags).toEqual({ manual: 'tag', 'manual-scope-inner': 'tag' });
327+
328+
// current scope is reset back to initial scope
310329
expect(getCurrentScope()).toBe(initialScope);
330+
expect(getCurrentScope().getScopeData().tags).toEqual({ outer: 'tag' });
331+
332+
// although the manual span is still running, it's no longer active due to being outside of the callback
311333
expect(getActiveSpan()).toBe(undefined);
312334
});
313335

0 commit comments

Comments
 (0)