From b5b6c16c0b764ef962363dc4a965558dbe5956b4 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 3 Jan 2025 14:31:54 +0100 Subject: [PATCH 1/4] fix(core): Restore previously active span when passing a custom scope to `startSpan` --- packages/core/src/tracing/trace.ts | 24 +++++-- packages/core/test/lib/tracing/trace.test.ts | 69 ++++++++++++++++++++ 2 files changed, 89 insertions(+), 4 deletions(-) diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index 09d27ff43e22..821bdee08423 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -16,7 +16,14 @@ import { propagationContextFromHeaders } from '../utils-hoist/tracing'; import { handleCallbackErrors } from '../utils/handleCallbackErrors'; import { hasTracingEnabled } from '../utils/hasTracingEnabled'; import { _getSpanForScope, _setSpanForScope } from '../utils/spanOnScope'; -import { addChildSpanToSpan, getRootSpan, spanIsSampled, spanTimeInputToSeconds, spanToJSON } from '../utils/spanUtils'; +import { + addChildSpanToSpan, + getActiveSpan, + getRootSpan, + spanIsSampled, + spanTimeInputToSeconds, + spanToJSON, +} from '../utils/spanUtils'; import { freezeDscOnSpan, getDynamicSamplingContextFromSpan } from './dynamicSamplingContext'; import { logSpanStart } from './logSpans'; import { sampleSpan } from './sampling'; @@ -44,9 +51,11 @@ export function startSpan(options: StartSpanOptions, callback: (span: Span) = } const spanArguments = parseSentrySpanArguments(options); - const { forceTransaction, parentSpan: customParentSpan } = options; + const { forceTransaction, parentSpan: customParentSpan, scope: customScope } = options; - return withScope(options.scope, () => { + const previousActiveSpanOnCustomScope = customScope && _getSpanForScope(customScope); + + return withScope(customScope, () => { // If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan` const wrapper = getActiveSpanWrapper(customParentSpan); @@ -75,7 +84,14 @@ export function startSpan(options: StartSpanOptions, callback: (span: Span) = activeSpan.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); } }, - () => activeSpan.end(), + () => { + 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); + } + }, ); }); }); diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts index e0d7bb3b555f..fd7c585f0bc5 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -269,6 +269,75 @@ describe('startSpan', () => { expect(getActiveSpan()).toBe(undefined); }); + describe('handles multiple spans in sequence with a custom 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); + + startSpan({ name: 'span 1', scope: manualScope }, span1 => { + expect(getCurrentScope()).not.toBe(initialScope); + expect(getCurrentScope()).toBe(manualScope); + expect(getActiveSpan()).toBe(span1); + expect(spanToJSON(span1).parent_span_id).toBe('parent-span-id'); + }); + + withScope(manualScope, () => { + expect(getActiveSpan()).toBe(parentSpan); + }); + + startSpan({ name: 'span 2', scope: manualScope }, span2 => { + expect(getCurrentScope()).not.toBe(initialScope); + expect(getCurrentScope()).toBe(manualScope); + expect(getActiveSpan()).toBe(span2); + expect(spanToJSON(span2).parent_span_id).toBe('parent-span-id'); + }); + + withScope(manualScope, () => { + expect(getActiveSpan()).toBe(parentSpan); + }); + + expect(getCurrentScope()).toBe(initialScope); + expect(getActiveSpan()).toBe(undefined); + }); + + it('without parent span', () => { + const initialScope = getCurrentScope(); + const manualScope = initialScope.clone(); + + const traceId = manualScope.getPropagationContext()?.traceId; + + startSpan({ name: 'span 1', scope: manualScope }, span1 => { + expect(getCurrentScope()).not.toBe(initialScope); + expect(getCurrentScope()).toBe(manualScope); + expect(getActiveSpan()).toBe(span1); + expect(spanToJSON(span1).parent_span_id).toBe(undefined); + expect(span1.spanContext().traceId).toBe(traceId); + }); + + withScope(manualScope, () => { + expect(getActiveSpan()).toBe(undefined); + }); + + startSpan({ name: 'span 2', scope: manualScope }, span2 => { + expect(getCurrentScope()).not.toBe(initialScope); + expect(getCurrentScope()).toBe(manualScope); + expect(getActiveSpan()).toBe(span2); + expect(spanToJSON(span2).parent_span_id).toBe(undefined); + expect(span2.spanContext().traceId).toBe(traceId); + }); + + withScope(manualScope, () => { + expect(getActiveSpan()).toBe(undefined); + }); + + expect(getCurrentScope()).toBe(initialScope); + expect(getActiveSpan()).toBe(undefined); + }); + }); + it('allows to pass a parentSpan', () => { const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true, name: 'parent-span' }); From 547ec0f01426c8950ce93008deee8156280998c6 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 3 Jan 2025 14:37:10 +0100 Subject: [PATCH 2/4] formatting --- packages/core/src/tracing/trace.ts | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index 821bdee08423..461258f429ab 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -16,14 +16,7 @@ import { propagationContextFromHeaders } from '../utils-hoist/tracing'; import { handleCallbackErrors } from '../utils/handleCallbackErrors'; import { hasTracingEnabled } from '../utils/hasTracingEnabled'; import { _getSpanForScope, _setSpanForScope } from '../utils/spanOnScope'; -import { - addChildSpanToSpan, - getActiveSpan, - getRootSpan, - spanIsSampled, - spanTimeInputToSeconds, - spanToJSON, -} from '../utils/spanUtils'; +import { addChildSpanToSpan, getRootSpan, spanIsSampled, spanTimeInputToSeconds, spanToJSON } from '../utils/spanUtils'; import { freezeDscOnSpan, getDynamicSamplingContextFromSpan } from './dynamicSamplingContext'; import { logSpanStart } from './logSpans'; import { sampleSpan } from './sampling'; From 52b10a8d6cbdc5094677fb1ced605dcce37e057f Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 10 Jan 2025 11:57:42 +0100 Subject: [PATCH 3/4] rework implementation to fork scope instead of manual cleanup --- packages/core/src/tracing/trace.ts | 11 +-- .../core/src/types-hoist/startSpanOptions.ts | 11 ++- packages/core/test/lib/tracing/trace.test.ts | 82 +++++++++++++------ packages/opentelemetry/test/trace.test.ts | 24 +++++- 4 files changed, 95 insertions(+), 33 deletions(-) diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index 461258f429ab..954bd944f4fa 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -46,9 +46,11 @@ export function startSpan(options: StartSpanOptions, callback: (span: Span) = const spanArguments = parseSentrySpanArguments(options); const { forceTransaction, parentSpan: customParentSpan, scope: customScope } = options; - const previousActiveSpanOnCustomScope = customScope && _getSpanForScope(customScope); + // We still need to fork a potentially passed scope, as we set the active span on it + // and we need to ensure that it is cleaned up properly once the span ends. + const customForkedScope = customScope?.clone(); - return withScope(customScope, () => { + return withScope(customForkedScope, () => { // If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan` const wrapper = getActiveSpanWrapper(customParentSpan); @@ -79,11 +81,6 @@ export function startSpan(options: StartSpanOptions, callback: (span: Span) = }, () => { 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); - } }, ); }); diff --git a/packages/core/src/types-hoist/startSpanOptions.ts b/packages/core/src/types-hoist/startSpanOptions.ts index 5d17cec579dd..4a9332856d09 100644 --- a/packages/core/src/types-hoist/startSpanOptions.ts +++ b/packages/core/src/types-hoist/startSpanOptions.ts @@ -5,7 +5,16 @@ export interface StartSpanOptions { /** A manually specified start time for the created `Span` object. */ startTime?: SpanTimeInput; - /** If defined, start this span off this scope instead off the current scope. */ + /** + * If set, start the span on a fork of this scope instead of on the current scope. + * To ensure proper span cleanup, the passed scope is cloned for the duration of the span. + * + * If you want to modify the passed scope inside the callback, be aware that: + * - calling `getCurrentScope()` will return the cloned scope, meaning all modifications + * will be reset once the callback finishes + * - if you want to modify the passed scope and have the changes persist after the callback + * ends, modify the scope directly and don't use `getCurrentScope()` + */ scope?: Scope; /** The name of the span. */ diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts index fd7c585f0bc5..eccfae5d1be0 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -25,7 +25,7 @@ import { import { SentryNonRecordingSpan } from '../../../src/tracing/sentryNonRecordingSpan'; import { startNewTrace } from '../../../src/tracing/trace'; import type { Event, Span, StartSpanOptions } from '../../../src/types-hoist'; -import { _setSpanForScope } from '../../../src/utils/spanOnScope'; +import { _getSpanForScope, _setSpanForScope } from '../../../src/utils/spanOnScope'; import { getActiveSpan, getRootSpan, getSpanDescendants, spanIsSampled } from '../../../src/utils/spanUtils'; import { TestClient, getDefaultTestClientOptions } from '../../mocks/client'; @@ -251,20 +251,44 @@ describe('startSpan', () => { expect(getActiveSpan()).toBe(undefined); }); - it('allows to pass a scope', () => { + it('starts the span on the fork of a passed custom scope', () => { const initialScope = getCurrentScope(); - const manualScope = initialScope.clone(); + const customScope = initialScope.clone(); + customScope.setTag('dogs', 'great'); + const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true }); - _setSpanForScope(manualScope, parentSpan); + _setSpanForScope(customScope, parentSpan); - startSpan({ name: 'GET users/[id]', scope: manualScope }, span => { + startSpan({ name: 'GET users/[id]', scope: customScope }, span => { + // current scope is forked from the customScope expect(getCurrentScope()).not.toBe(initialScope); - expect(getCurrentScope()).toBe(manualScope); + expect(getCurrentScope()).not.toBe(customScope); + expect(getCurrentScope().getScopeData().tags).toEqual({ dogs: 'great' }); + + // active span is set correctly expect(getActiveSpan()).toBe(span); + + // span has the correct parent span expect(spanToJSON(span).parent_span_id).toBe('parent-span-id'); + + // scope data modifications + 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' }); }); + // customScope modifications are persisted + expect(customScope.getScopeData().tags).toEqual({ dogs: 'great', bears: 'great' }); + + // span is parent span again on customScope + withScope(customScope, () => { + expect(getActiveSpan()).toBe(parentSpan); + }); + + // but activeSpan and currentScope are reset, since customScope was never active expect(getCurrentScope()).toBe(initialScope); expect(getActiveSpan()).toBe(undefined); }); @@ -273,29 +297,35 @@ describe('startSpan', () => { it('with parent span', () => { const initialScope = getCurrentScope(); - const manualScope = initialScope.clone(); + const customScope = initialScope.clone(); const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true }); - _setSpanForScope(manualScope, parentSpan); + _setSpanForScope(customScope, parentSpan); - startSpan({ name: 'span 1', scope: manualScope }, span1 => { + startSpan({ name: 'span 1', scope: customScope }, span1 => { + // current scope is forked from the customScope expect(getCurrentScope()).not.toBe(initialScope); - expect(getCurrentScope()).toBe(manualScope); + expect(getCurrentScope()).not.toBe(customScope); + expect(getActiveSpan()).toBe(span1); expect(spanToJSON(span1).parent_span_id).toBe('parent-span-id'); }); - withScope(manualScope, () => { + // active span on customScope is reset + withScope(customScope, () => { expect(getActiveSpan()).toBe(parentSpan); }); - startSpan({ name: 'span 2', scope: manualScope }, span2 => { + startSpan({ name: 'span 2', scope: customScope }, span2 => { + // current scope is forked from the customScope expect(getCurrentScope()).not.toBe(initialScope); - expect(getCurrentScope()).toBe(manualScope); + expect(getCurrentScope()).not.toBe(customScope); + expect(getActiveSpan()).toBe(span2); + // both, span1 and span2 are children of the parent span expect(spanToJSON(span2).parent_span_id).toBe('parent-span-id'); }); - withScope(manualScope, () => { + withScope(customScope, () => { expect(getActiveSpan()).toBe(parentSpan); }); @@ -305,31 +335,35 @@ describe('startSpan', () => { it('without parent span', () => { const initialScope = getCurrentScope(); - const manualScope = initialScope.clone(); + const customScope = initialScope.clone(); - const traceId = manualScope.getPropagationContext()?.traceId; + const traceId = customScope.getPropagationContext()?.traceId; - startSpan({ name: 'span 1', scope: manualScope }, span1 => { + startSpan({ name: 'span 1', scope: customScope }, span1 => { expect(getCurrentScope()).not.toBe(initialScope); - expect(getCurrentScope()).toBe(manualScope); + expect(getCurrentScope()).not.toBe(customScope); + expect(getActiveSpan()).toBe(span1); - expect(spanToJSON(span1).parent_span_id).toBe(undefined); + expect(getRootSpan(getActiveSpan()!)).toBe(span1); + expect(span1.spanContext().traceId).toBe(traceId); }); - withScope(manualScope, () => { + withScope(customScope, () => { expect(getActiveSpan()).toBe(undefined); }); - startSpan({ name: 'span 2', scope: manualScope }, span2 => { + startSpan({ name: 'span 2', scope: customScope }, span2 => { expect(getCurrentScope()).not.toBe(initialScope); - expect(getCurrentScope()).toBe(manualScope); + expect(getCurrentScope()).not.toBe(customScope); + expect(getActiveSpan()).toBe(span2); - expect(spanToJSON(span2).parent_span_id).toBe(undefined); + expect(getRootSpan(getActiveSpan()!)).toBe(span2); + expect(span2.spanContext().traceId).toBe(traceId); }); - withScope(manualScope, () => { + withScope(customScope, () => { expect(getActiveSpan()).toBe(undefined); }); diff --git a/packages/opentelemetry/test/trace.test.ts b/packages/opentelemetry/test/trace.test.ts index 3eedc0743ea0..1347f79ce64d 100644 --- a/packages/opentelemetry/test/trace.test.ts +++ b/packages/opentelemetry/test/trace.test.ts @@ -290,24 +290,46 @@ describe('trace', () => { let manualScope: Scope; let parentSpan: Span; + // "hack" to create a manual scope with a parent span startSpanManual({ name: 'detached' }, span => { parentSpan = span; manualScope = getCurrentScope(); manualScope.setTag('manual', 'tag'); }); + expect(manualScope!.getScopeData().tags).toEqual({ manual: 'tag' }); + expect(getCurrentScope()).not.toBe(manualScope!); + getCurrentScope().setTag('outer', 'tag'); startSpan({ name: 'GET users/[id]', scope: manualScope! }, span => { + // the current scope in the callback is a fork of the manual scope expect(getCurrentScope()).not.toBe(initialScope); + expect(getCurrentScope()).not.toBe(manualScope); + expect(getCurrentScope().getScopeData().tags).toEqual({ manual: 'tag' }); - expect(getCurrentScope()).toEqual(manualScope); + // getActiveSpan returns the correct span expect(getActiveSpan()).toBe(span); + // span hierarchy is correct expect(getSpanParentSpanId(span)).toBe(parentSpan.spanContext().spanId); + + // scope data modifications are isolated between original and forked manual scope + getCurrentScope().setTag('inner', 'tag'); + manualScope!.setTag('manual-scope-inner', 'tag'); + + expect(getCurrentScope().getScopeData().tags).toEqual({ manual: 'tag', inner: 'tag' }); + expect(manualScope!.getScopeData().tags).toEqual({ manual: 'tag', 'manual-scope-inner': 'tag' }); }); + // manualScope modifications remain set outside the callback + expect(manualScope!.getScopeData().tags).toEqual({ manual: 'tag', 'manual-scope-inner': 'tag' }); + + // current scope is reset back to initial scope expect(getCurrentScope()).toBe(initialScope); + expect(getCurrentScope().getScopeData().tags).toEqual({ outer: 'tag' }); + + // although the manual span is still running, it's no longer active due to being outside of the callback expect(getActiveSpan()).toBe(undefined); }); From 162e1054662c8b671189c2b34243c2221ca81612 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 10 Jan 2025 12:11:32 +0100 Subject: [PATCH 4/4] migration note, formatting --- docs/migration/v8-to-v9.md | 10 ++++++++++ packages/core/src/types-hoist/startSpanOptions.ts | 11 ++++++----- packages/core/test/lib/tracing/trace.test.ts | 2 +- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/docs/migration/v8-to-v9.md b/docs/migration/v8-to-v9.md index 1f100c21d730..8c30b3dfaa34 100644 --- a/docs/migration/v8-to-v9.md +++ b/docs/migration/v8-to-v9.md @@ -82,6 +82,16 @@ In v9, an `undefined` value will be treated the same as if the value is not defi - The `getCurrentHub().getIntegration(IntegrationClass)` method will always return `null` in v9. This has already stopped working mostly in v8, because we stopped exposing integration classes. In v9, the fallback behavior has been removed. Note that this does not change the type signature and is thus not technically breaking, but still worth pointing out. +- The `startSpan` behavior was slightly changed if you pass a custom `scope` to the span start options: While in v8, the passed scope was set active directly on the passed scope, in v9, the scope is cloned. This behavior change does not apply to `@sentry/node` where the scope was already cloned. This change was made to ensure that the span only remains active within the callback and to align behavior between `@sentry/node` and all other SDKs. As a result of the change, your span hierarchy should be more accurate. However, be aware that modifying the scope (e.g. set tags) within the `startSpan` callback behaves a bit differently now. + +```js +startSpan({ name: 'example', scope: customScope }, () => { + getCurrentScope().setTag('tag-a', 'a'); // this tag will only remain within the callback + // set the tag directly on customScope in addition, if you want to to persist the tag outside of the callback + customScope.setTag('tag-a', 'a'); +}); +``` + ### `@sentry/node` - When `skipOpenTelemetrySetup: true` is configured, `httpIntegration({ spans: false })` will be configured by default. This means that you no longer have to specify this yourself in this scenario. With this change, no spans are emitted once `skipOpenTelemetrySetup: true` is configured, without any further configuration being needed. diff --git a/packages/core/src/types-hoist/startSpanOptions.ts b/packages/core/src/types-hoist/startSpanOptions.ts index 4a9332856d09..6e5fa007bde8 100644 --- a/packages/core/src/types-hoist/startSpanOptions.ts +++ b/packages/core/src/types-hoist/startSpanOptions.ts @@ -9,11 +9,12 @@ export interface StartSpanOptions { * If set, start the span on a fork of this scope instead of on the current scope. * To ensure proper span cleanup, the passed scope is cloned for the duration of the span. * - * If you want to modify the passed scope inside the callback, be aware that: - * - calling `getCurrentScope()` will return the cloned scope, meaning all modifications - * will be reset once the callback finishes - * - if you want to modify the passed scope and have the changes persist after the callback - * ends, modify the scope directly and don't use `getCurrentScope()` + * If you want to modify the passed scope inside the callback, calling `getCurrentScope()` + * will return the cloned scope, meaning all scope modifications will be reset once the + * callback finishes + * + * If you want to modify the passed scope and have the changes persist after the callback ends, + * modify the scope directly instead of using `getCurrentScope()` */ scope?: Scope; diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts index eccfae5d1be0..6698b90fed81 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -25,7 +25,7 @@ import { import { SentryNonRecordingSpan } from '../../../src/tracing/sentryNonRecordingSpan'; import { startNewTrace } from '../../../src/tracing/trace'; import type { Event, Span, StartSpanOptions } from '../../../src/types-hoist'; -import { _getSpanForScope, _setSpanForScope } from '../../../src/utils/spanOnScope'; +import { _setSpanForScope } from '../../../src/utils/spanOnScope'; import { getActiveSpan, getRootSpan, getSpanDescendants, spanIsSampled } from '../../../src/utils/spanUtils'; import { TestClient, getDefaultTestClientOptions } from '../../mocks/client';