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/tracing/trace.ts b/packages/core/src/tracing/trace.ts index 09d27ff43e22..954bd944f4fa 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -44,9 +44,13 @@ 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, () => { + // 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(customForkedScope, () => { // If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan` const wrapper = getActiveSpanWrapper(customParentSpan); @@ -75,7 +79,9 @@ export function startSpan(options: StartSpanOptions, callback: (span: Span) = activeSpan.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); } }, - () => activeSpan.end(), + () => { + activeSpan.end(); + }, ); }); }); diff --git a/packages/core/src/types-hoist/startSpanOptions.ts b/packages/core/src/types-hoist/startSpanOptions.ts index 5d17cec579dd..6e5fa007bde8 100644 --- a/packages/core/src/types-hoist/startSpanOptions.ts +++ b/packages/core/src/types-hoist/startSpanOptions.ts @@ -5,7 +5,17 @@ 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, 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; /** 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 e0d7bb3b555f..6698b90fed81 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -251,24 +251,127 @@ 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); }); + describe('handles multiple spans in sequence with a custom scope', () => { + it('with parent span', () => { + const initialScope = getCurrentScope(); + + const customScope = initialScope.clone(); + const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true }); + _setSpanForScope(customScope, parentSpan); + + startSpan({ name: 'span 1', scope: customScope }, span1 => { + // current scope is forked from the customScope + expect(getCurrentScope()).not.toBe(initialScope); + expect(getCurrentScope()).not.toBe(customScope); + + expect(getActiveSpan()).toBe(span1); + expect(spanToJSON(span1).parent_span_id).toBe('parent-span-id'); + }); + + // active span on customScope is reset + withScope(customScope, () => { + expect(getActiveSpan()).toBe(parentSpan); + }); + + startSpan({ name: 'span 2', scope: customScope }, span2 => { + // current scope is forked from the customScope + expect(getCurrentScope()).not.toBe(initialScope); + 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(customScope, () => { + expect(getActiveSpan()).toBe(parentSpan); + }); + + expect(getCurrentScope()).toBe(initialScope); + expect(getActiveSpan()).toBe(undefined); + }); + + it('without parent span', () => { + const initialScope = getCurrentScope(); + const customScope = initialScope.clone(); + + const traceId = customScope.getPropagationContext()?.traceId; + + startSpan({ name: 'span 1', scope: customScope }, span1 => { + expect(getCurrentScope()).not.toBe(initialScope); + expect(getCurrentScope()).not.toBe(customScope); + + expect(getActiveSpan()).toBe(span1); + expect(getRootSpan(getActiveSpan()!)).toBe(span1); + + expect(span1.spanContext().traceId).toBe(traceId); + }); + + withScope(customScope, () => { + expect(getActiveSpan()).toBe(undefined); + }); + + startSpan({ name: 'span 2', scope: customScope }, span2 => { + expect(getCurrentScope()).not.toBe(initialScope); + expect(getCurrentScope()).not.toBe(customScope); + + expect(getActiveSpan()).toBe(span2); + expect(getRootSpan(getActiveSpan()!)).toBe(span2); + + expect(span2.spanContext().traceId).toBe(traceId); + }); + + withScope(customScope, () => { + 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' }); 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); });