From a386262fe4d3971a617b190265e42452e00f1091 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 3 Jan 2025 14:42:27 +0100 Subject: [PATCH 1/3] fix(core): Restore previously active span when passing a custom scope to `startSpanManual` --- packages/core/src/tracing/trace.ts | 12 ++- packages/core/test/lib/tracing/trace.test.ts | 79 ++++++++++++++++---- 2 files changed, 74 insertions(+), 17 deletions(-) diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index 34fa94bec95d..7b377b291543 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -96,7 +96,9 @@ export function startSpan(options: StartSpanOptions, callback: (span: Span) = /** * Similar to `Sentry.startSpan`. Wraps a function with a transaction/span, but does not finish the span - * after the function is done automatically. You'll have to call `span.end()` manually. + * after the function is done automatically. Use the `finish` function passed to the callback, + * to finish the span manually and sett he previous span as the active span again. If you don't use `finish` + * but `span.end()` instead, the span will be ended but remain as the active span. * * The created span is the active span and will be used as parent by other spans created inside the function * and can be accessed via `Sentry.getActiveSpan()`, as long as the function is executed while the scope is active. @@ -111,7 +113,7 @@ export function startSpanManual(options: StartSpanOptions, callback: (span: S } const spanArguments = parseSentrySpanArguments(options); - const { forceTransaction, parentSpan: customParentSpan } = options; + const { forceTransaction, parentSpan: customParentSpan, scope: customScope } = options; return withScope(options.scope, () => { // If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan` @@ -131,10 +133,16 @@ export function startSpanManual(options: StartSpanOptions, callback: (span: S scope, }); + const previousActiveSpanOnCustomScope = customScope && _getSpanForScope(customScope); _setSpanForScope(scope, activeSpan); function finishAndSetSpan(): void { activeSpan.end(); + if (customScope) { + // If a custom scope is passed, we don't fork the scope. Therefore, we need to reset the + // active span on the scope to the previous active span (or to undefined if there was none) + _setSpanForScope(customScope, previousActiveSpanOnCustomScope); + } } return handleCallbackErrors( diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts index 83b875edb59b..a7927ee01882 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -779,27 +779,76 @@ describe('startSpanManual', () => { expect(getActiveSpan()).toBe(undefined); }); - it('allows to pass a scope', () => { - const initialScope = getCurrentScope(); + describe('allows to pass a scope', () => { + it('with parent span', () => { + const initialScope = getCurrentScope(); - const manualScope = initialScope.clone(); - const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true }); - _setSpanForScope(manualScope, parentSpan); + const manualScope = initialScope.clone(); + const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true }); + _setSpanForScope(manualScope, parentSpan); - startSpanManual({ name: 'GET users/[id]', scope: manualScope }, span => { - expect(getCurrentScope()).not.toBe(initialScope); - expect(getCurrentScope()).toBe(manualScope); - expect(getActiveSpan()).toBe(span); - expect(spanToJSON(span).parent_span_id).toBe('parent-span-id'); + let span1: Span | undefined; - span.end(); + startSpanManual({ name: 'GET users/[id]', scope: manualScope }, span => { + span1 = span; + expect(getCurrentScope()).not.toBe(initialScope); + expect(getCurrentScope()).toBe(manualScope); + expect(getActiveSpan()).toBe(span); + expect(spanToJSON(span).parent_span_id).toBe('parent-span-id'); - // Is still the active span - expect(getActiveSpan()).toBe(span); + span.end(); + + // Is still the active span + expect(getActiveSpan()).toBe(span); + }); + + startSpanManual({ name: 'POST users/[id]', scope: manualScope }, (span, finish) => { + expect(getCurrentScope()).not.toBe(initialScope); + expect(getCurrentScope()).toBe(manualScope); + expect(getActiveSpan()).toBe(span); + expect(spanToJSON(span).parent_span_id).toBe(span1?.spanContext().spanId); + + finish(); + + // using finish() resets the scope correctly + expect(getActiveSpan()).toBe(span1); + }); + + expect(getCurrentScope()).toBe(initialScope); + expect(getActiveSpan()).toBe(undefined); }); - expect(getCurrentScope()).toBe(initialScope); - expect(getActiveSpan()).toBe(undefined); + it('without parent span', () => { + const initialScope = getCurrentScope(); + const manualScope = initialScope.clone(); + + startSpanManual({ name: 'GET users/[id]', scope: manualScope }, (span, finish) => { + expect(getCurrentScope()).not.toBe(initialScope); + expect(getCurrentScope()).toBe(manualScope); + expect(getActiveSpan()).toBe(span); + expect(spanToJSON(span).parent_span_id).toBe(undefined); + + finish(); + + // Is still the active span + expect(getActiveSpan()).toBe(undefined); + }); + + startSpanManual({ name: 'GET users/[id]', scope: manualScope }, (span, finish) => { + expect(getCurrentScope()).not.toBe(initialScope); + expect(getCurrentScope()).toBe(manualScope); + expect(getActiveSpan()).toBe(span); + expect(spanToJSON(span).parent_span_id).toBe(undefined); + + finish(); + + // using finish() resets the scope correctly + expect(getActiveSpan()).toBe(undefined); + }); + + expect(getCurrentScope()).toBe(initialScope); + expect(getActiveSpan()).toBe(undefined); + }); }); it('allows to pass a parentSpan', () => { From d1f3d973541138e15824131b5071ca10418f2af0 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 13 Jan 2025 12:38:58 +0100 Subject: [PATCH 2/3] rework implementation to fork scope instead of restoring previously active span --- packages/core/src/tracing/trace.ts | 24 +++---- packages/core/test/lib/tracing/trace.test.ts | 72 +++++++++++++------- 2 files changed, 57 insertions(+), 39 deletions(-) diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index 7b377b291543..eabdb778fb10 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -96,9 +96,7 @@ export function startSpan(options: StartSpanOptions, callback: (span: Span) = /** * Similar to `Sentry.startSpan`. Wraps a function with a transaction/span, but does not finish the span - * after the function is done automatically. Use the `finish` function passed to the callback, - * to finish the span manually and sett he previous span as the active span again. If you don't use `finish` - * but `span.end()` instead, the span will be ended but remain as the active span. + * after the function is done automatically. Use `span.end()` to end the span. * * The created span is the active span and will be used as parent by other spans created inside the function * and can be accessed via `Sentry.getActiveSpan()`, as long as the function is executed while the scope is active. @@ -115,7 +113,9 @@ export function startSpanManual(options: StartSpanOptions, callback: (span: S const spanArguments = parseSentrySpanArguments(options); const { forceTransaction, parentSpan: customParentSpan, scope: customScope } = options; - return withScope(options.scope, () => { + const customForkedScope = customScope?.clone(); + + return withScope(customForkedScope, () => { // If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan` const wrapper = getActiveSpanWrapper(customParentSpan); @@ -133,20 +133,14 @@ export function startSpanManual(options: StartSpanOptions, callback: (span: S scope, }); - const previousActiveSpanOnCustomScope = customScope && _getSpanForScope(customScope); _setSpanForScope(scope, activeSpan); - function finishAndSetSpan(): void { - activeSpan.end(); - if (customScope) { - // If a custom scope is passed, we don't fork the scope. Therefore, we need to reset the - // active span on the scope to the previous active span (or to undefined if there was none) - _setSpanForScope(customScope, previousActiveSpanOnCustomScope); - } - } - return handleCallbackErrors( - () => callback(activeSpan, finishAndSetSpan), + // We pass the `finish` function to the callback, so the user can finish the span manually + // this is mainly here for historic purposes because previously, we instructed users to call + // `finish` instead of `span.end()` to also clean up the scope. Nowadays, calling `span.end()` + // or `finish` has the same effect and we simply leave it here to avoid breaking user code. + () => callback(activeSpan, () => activeSpan.end()), () => { // Only update the span status if it hasn't been changed yet, and the span is not yet finished const { status } = spanToJSON(activeSpan); diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts index a7927ee01882..bbeee8443f4b 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -779,39 +779,57 @@ describe('startSpanManual', () => { expect(getActiveSpan()).toBe(undefined); }); - describe('allows to pass a scope', () => { + describe('starts a span on the fork of a custom scope if passed', () => { it('with parent span', () => { const initialScope = getCurrentScope(); - const manualScope = initialScope.clone(); - const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true }); - _setSpanForScope(manualScope, parentSpan); + const customScope = initialScope.clone(); + customScope.setTag('dogs', 'great'); - let span1: Span | undefined; + const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true }); + _setSpanForScope(customScope, parentSpan); - startSpanManual({ name: 'GET users/[id]', scope: manualScope }, span => { - span1 = span; + startSpanManual({ name: 'GET users/[id]', scope: customScope }, span => { + // current scope is forked from the customScope expect(getCurrentScope()).not.toBe(initialScope); - expect(getCurrentScope()).toBe(manualScope); - expect(getActiveSpan()).toBe(span); + expect(getCurrentScope()).not.toBe(customScope); expect(spanToJSON(span).parent_span_id).toBe('parent-span-id'); + // span is active span + expect(getActiveSpan()).toBe(span); + span.end(); - // Is still the active span + // span is still the active span (weird but it is what it is) expect(getActiveSpan()).toBe(span); + + getCurrentScope().setTag('cats', 'great'); + customScope.setTag('bears', 'great'); + + expect(getCurrentScope().getScopeData().tags).toEqual({ dogs: 'great', cats: 'great' }); + expect(customScope.getScopeData().tags).toEqual({ dogs: 'great', bears: 'great' }); }); - startSpanManual({ name: 'POST users/[id]', scope: manualScope }, (span, finish) => { + expect(getCurrentScope()).toBe(initialScope); + expect(getActiveSpan()).toBe(undefined); + + startSpanManual({ name: 'POST users/[id]', scope: customScope }, (span, finish) => { + // current scope is forked from the customScope expect(getCurrentScope()).not.toBe(initialScope); - expect(getCurrentScope()).toBe(manualScope); + expect(getCurrentScope()).not.toBe(customScope); + expect(spanToJSON(span).parent_span_id).toBe('parent-span-id'); + + // scope data modification from customScope in previous callback is persisted + expect(getCurrentScope().getScopeData().tags).toEqual({ dogs: 'great', bears: 'great' }); + + // span is active span expect(getActiveSpan()).toBe(span); - expect(spanToJSON(span).parent_span_id).toBe(span1?.spanContext().spanId); + // calling finish() or span.end() has the same effect finish(); // using finish() resets the scope correctly - expect(getActiveSpan()).toBe(span1); + expect(getActiveSpan()).toBe(span); }); expect(getCurrentScope()).toBe(initialScope); @@ -823,27 +841,33 @@ describe('startSpanManual', () => { const manualScope = initialScope.clone(); startSpanManual({ name: 'GET users/[id]', scope: manualScope }, (span, finish) => { + // current scope is forked from the customScope expect(getCurrentScope()).not.toBe(initialScope); - expect(getCurrentScope()).toBe(manualScope); + expect(getCurrentScope()).not.toBe(manualScope); + expect(getCurrentScope()).toEqual(manualScope); + + // span is active span and a root span expect(getActiveSpan()).toBe(span); - expect(spanToJSON(span).parent_span_id).toBe(undefined); + expect(getRootSpan(span)).toBe(span); - finish(); + span.end(); - // Is still the active span - expect(getActiveSpan()).toBe(undefined); + expect(getActiveSpan()).toBe(span); }); - startSpanManual({ name: 'GET users/[id]', scope: manualScope }, (span, finish) => { + startSpanManual({ name: 'POST users/[id]', scope: manualScope }, (span, finish) => { expect(getCurrentScope()).not.toBe(initialScope); - expect(getCurrentScope()).toBe(manualScope); + expect(getCurrentScope()).not.toBe(manualScope); + expect(getCurrentScope()).toEqual(manualScope); + + // second span is active span and its own root span expect(getActiveSpan()).toBe(span); - expect(spanToJSON(span).parent_span_id).toBe(undefined); + expect(getRootSpan(span)).toBe(span); finish(); - // using finish() resets the scope correctly - expect(getActiveSpan()).toBe(undefined); + // calling finish() or span.end() has the same effect + expect(getActiveSpan()).toBe(span); }); expect(getCurrentScope()).toBe(initialScope); From 4b7ad70ad580961d9f6ee95e2ba2587d9915315d Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 15 Jan 2025 15:25:33 +0100 Subject: [PATCH 3/3] lint --- packages/core/test/lib/tracing/trace.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts index bbeee8443f4b..db54e1c08ddf 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -840,7 +840,7 @@ describe('startSpanManual', () => { const initialScope = getCurrentScope(); const manualScope = initialScope.clone(); - startSpanManual({ name: 'GET users/[id]', scope: manualScope }, (span, finish) => { + startSpanManual({ name: 'GET users/[id]', scope: manualScope }, span => { // current scope is forked from the customScope expect(getCurrentScope()).not.toBe(initialScope); expect(getCurrentScope()).not.toBe(manualScope);