From 38de9ccd92e1711969dd2e2d2245af28cfef6c65 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 12 Mar 2024 13:03:48 +0000 Subject: [PATCH 1/6] ref(core): Remove `scope.setSpan()` and `scope.getSpan()` methods Instead, we have an internal utility for this now. --- .../backgroundtab-custom/subject.js | 3 +- .../components/span-context.tsx | 3 +- packages/core/src/fetch.ts | 5 +-- packages/core/src/scope.ts | 37 ++++-------------- packages/core/src/server-runtime-client.ts | 5 +-- packages/core/src/tracing/idleSpan.ts | 7 ++-- packages/core/src/tracing/trace.ts | 39 ++++++++----------- packages/core/src/utils/spanUtils.ts | 31 ++++++++++++++- packages/core/test/lib/scope.test.ts | 17 -------- packages/core/test/lib/tracing/trace.test.ts | 19 ++++----- packages/node/src/_setSpanForScope.ts | 24 ++++++++++++ packages/node/src/handlers.ts | 7 ++-- packages/node/src/integrations/hapi/index.ts | 4 +- packages/node/test/integrations/http.test.ts | 11 ++---- packages/opentelemetry/src/trace.ts | 17 ++++---- packages/types/src/scope.ts | 13 ------- 16 files changed, 111 insertions(+), 131 deletions(-) create mode 100644 packages/node/src/_setSpanForScope.ts diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/backgroundtab-custom/subject.js b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/backgroundtab-custom/subject.js index e40426fdbe26..1d6cf5f0627b 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/backgroundtab-custom/subject.js +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/backgroundtab-custom/subject.js @@ -6,9 +6,8 @@ document.getElementById('go-background').addEventListener('click', () => { }); document.getElementById('start-span').addEventListener('click', () => { - const span = Sentry.startInactiveSpan({ name: 'test-span' }); + const span = Sentry.startBrowserTracingNavigationSpan({ name: 'test-span' }); window.span = span; - Sentry.getCurrentScope().setSpan(span); }); window.getSpanJson = () => { diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/components/span-context.tsx b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/components/span-context.tsx index 3ecd84019471..81a6f404d0f4 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/components/span-context.tsx +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/components/span-context.tsx @@ -1,6 +1,6 @@ 'use client'; -import { getCurrentScope, startInactiveSpan } from '@sentry/nextjs'; +import { startInactiveSpan } from '@sentry/nextjs'; import { Span } from '@sentry/types'; import { PropsWithChildren, createContext, useState } from 'react'; @@ -29,7 +29,6 @@ export function SpanContextProvider({ children }: PropsWithChildren) { spanActive: false, start: (spanName: string) => { const span = startInactiveSpan({ name: spanName }); - getCurrentScope().setSpan(span); setSpan(span); }, } diff --git a/packages/core/src/fetch.ts b/packages/core/src/fetch.ts index 5255e7fa206d..8002d9280297 100644 --- a/packages/core/src/fetch.ts +++ b/packages/core/src/fetch.ts @@ -127,11 +127,8 @@ export function addTracingHeadersToFetchRequest( } | PolymorphicRequestHeaders; }, - requestSpan?: Span, + span?: Span, ): PolymorphicRequestHeaders | undefined { - // eslint-disable-next-line deprecation/deprecation - const span = requestSpan || scope.getSpan(); - const isolationScope = getIsolationScope(); const { traceId, spanId, sampled, dsc } = { diff --git a/packages/core/src/scope.ts b/packages/core/src/scope.ts index 90961cc48bd0..c42a3821d40c 100644 --- a/packages/core/src/scope.ts +++ b/packages/core/src/scope.ts @@ -19,7 +19,6 @@ import type { ScopeData, Session, SeverityLevel, - Span, Transaction, User, } from '@sentry/types'; @@ -27,6 +26,7 @@ import { dateTimestampInSeconds, isPlainObject, logger, uuid4 } from '@sentry/ut import { updateSession } from './session'; import type { SentrySpan } from './tracing/sentrySpan'; +import { _getSpanForScope, _setSpanForScope } from './utils/spanUtils'; /** * Default value for maximum number of breadcrumbs added to an event. @@ -87,9 +87,6 @@ export class Scope implements ScopeInterface { */ protected _transactionName?: string; - /** Span */ - protected _span?: Span; - /** Session */ protected _session?: Session; @@ -134,7 +131,6 @@ export class Scope implements ScopeInterface { newScope._contexts = { ...this._contexts }; newScope._user = this._user; newScope._level = this._level; - newScope._span = this._span; newScope._session = this._session; newScope._transactionName = this._transactionName; newScope._fingerprint = this._fingerprint; @@ -145,6 +141,8 @@ export class Scope implements ScopeInterface { newScope._propagationContext = { ...this._propagationContext }; newScope._client = this._client; + _setSpanForScope(newScope, _getSpanForScope(this)); + return newScope; } @@ -304,25 +302,6 @@ export class Scope implements ScopeInterface { return this; } - /** - * Sets the Span on the scope. - * @param span Span - * @deprecated Instead of setting a span on a scope, use `startSpan()`/`startSpanManual()` instead. - */ - public setSpan(span?: Span): this { - this._span = span; - this._notifyScopeListeners(); - return this; - } - - /** - * Returns the `Span` if there is one. - * @deprecated Use `getActiveSpan()` instead. - */ - public getSpan(): Span | undefined { - return this._span; - } - /** * Returns the `Transaction` attached to the scope (if there is one). * @deprecated You should not rely on the transaction, but just use `startSpan()` APIs instead. @@ -330,7 +309,7 @@ export class Scope implements ScopeInterface { public getTransaction(): Transaction | undefined { // Often, this span (if it exists at all) will be a transaction, but it's not guaranteed to be. Regardless, it will // have a pointer to the currently-active transaction. - const span = this._span; + const span = _getSpanForScope(this); // Cannot replace with getRootSpan because getRootSpan returns a span, not a transaction // Also, this method will be removed anyway. @@ -432,11 +411,12 @@ export class Scope implements ScopeInterface { this._transactionName = undefined; this._fingerprint = undefined; this._requestSession = undefined; - this._span = undefined; this._session = undefined; - this._notifyScopeListeners(); + _setSpanForScope(this, undefined); this._attachments = []; this._propagationContext = generatePropagationContext(); + + this._notifyScopeListeners(); return this; } @@ -522,7 +502,6 @@ export class Scope implements ScopeInterface { _propagationContext, _sdkProcessingMetadata, _transactionName, - _span, } = this; return { @@ -538,7 +517,7 @@ export class Scope implements ScopeInterface { propagationContext: _propagationContext, sdkProcessingMetadata: _sdkProcessingMetadata, transactionName: _transactionName, - span: _span, + span: _getSpanForScope(this), }; } diff --git a/packages/core/src/server-runtime-client.ts b/packages/core/src/server-runtime-client.ts index de88e6f3706e..1c2ffba399e4 100644 --- a/packages/core/src/server-runtime-client.ts +++ b/packages/core/src/server-runtime-client.ts @@ -24,7 +24,7 @@ import { getDynamicSamplingContextFromClient, getDynamicSamplingContextFromSpan, } from './tracing'; -import { getRootSpan, spanToTraceContext } from './utils/spanUtils'; +import { _getSpanForScope, getRootSpan, spanToTraceContext } from './utils/spanUtils'; export interface ServerRuntimeClientOptions extends ClientOptions { platform?: string; @@ -252,8 +252,7 @@ export class ServerRuntimeClient< return [undefined, undefined]; } - // eslint-disable-next-line deprecation/deprecation - const span = scope.getSpan(); + const span = _getSpanForScope(scope); if (span) { const rootSpan = getRootSpan(span); const samplingContext = getDynamicSamplingContextFromSpan(rootSpan); diff --git a/packages/core/src/tracing/idleSpan.ts b/packages/core/src/tracing/idleSpan.ts index 36d1474b39f2..ce96d4bb32bb 100644 --- a/packages/core/src/tracing/idleSpan.ts +++ b/packages/core/src/tracing/idleSpan.ts @@ -6,6 +6,7 @@ import { DEBUG_BUILD } from '../debug-build'; import { SEMANTIC_ATTRIBUTE_SENTRY_IDLE_SPAN_FINISH_REASON } from '../semanticAttributes'; import { hasTracingEnabled } from '../utils/hasTracingEnabled'; import { + _setSpanForScope, getActiveSpan, getSpanDescendants, removeChildSpanFromSpan, @@ -232,8 +233,7 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti beforeSpanEnd(span); } - // eslint-disable-next-line deprecation/deprecation - scope.setSpan(previousActiveSpan); + _setSpanForScope(scope, previousActiveSpan); const spanJSON = spanToJSON(span); @@ -347,8 +347,7 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti function _startIdleSpan(options: StartSpanOptions): Span { const span = startInactiveSpan(options); - // eslint-disable-next-line deprecation/deprecation - getCurrentScope().setSpan(span); + _setSpanForScope(getCurrentScope(), span); DEBUG_BUILD && logger.log(`Setting idle span on scope. Span ID: ${span.spanContext().spanId}`); diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index 78369ce091fc..63fc845fca0e 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -10,6 +10,8 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE } from '../semanticAttributes'; import { handleCallbackErrors } from '../utils/handleCallbackErrors'; import { hasTracingEnabled } from '../utils/hasTracingEnabled'; import { + _getSpanForScope, + _setSpanForScope, addChildSpanToSpan, getActiveSpan, spanIsSampled, @@ -27,13 +29,12 @@ import { setCapturedScopesOnSpan } from './utils'; /** * Wraps a function with a transaction/span and finishes the span after the function is done. * 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.getSpan()`, as long as the function is executed while the scope is active. + * and can be accessed via `Sentry.getActiveSpan()`, as long as the function is executed while the scope is active. * * If you want to create a span that is not set as active, use {@link startInactiveSpan}. * - * Note that if you have not enabled tracing extensions via `addTracingExtensions` - * or you didn't set `tracesSampleRate`, this function will not generate spans - * and the `span` returned from the callback will be undefined. + * You'll always get a span passed to the callback, + * it may just be a non-recording span if the span is not sampled or if tracing is disabled. */ export function startSpan(context: StartSpanOptions, callback: (span: Span) => T): T { const acs = getAcs(); @@ -44,8 +45,7 @@ export function startSpan(context: StartSpanOptions, callback: (span: Span) = const spanContext = normalizeContext(context); return withScope(context.scope, scope => { - // eslint-disable-next-line deprecation/deprecation - const parentSpan = scope.getSpan() as SentrySpan | undefined; + const parentSpan = _getSpanForScope(scope) as SentrySpan | undefined; const shouldSkipSpan = context.onlyIfParent && !parentSpan; const activeSpan = shouldSkipSpan @@ -57,8 +57,7 @@ export function startSpan(context: StartSpanOptions, callback: (span: Span) = scope, }); - // eslint-disable-next-line deprecation/deprecation - scope.setSpan(activeSpan); + _setSpanForScope(scope, activeSpan); return handleCallbackErrors( () => callback(activeSpan), @@ -81,9 +80,8 @@ export function startSpan(context: StartSpanOptions, callback: (span: 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. * - * Note that if you have not enabled tracing extensions via `addTracingExtensions` - * or you didn't set `tracesSampleRate`, this function will not generate spans - * and the `span` returned from the callback will be undefined. + * You'll always get a span passed to the callback, + * it may just be a non-recording span if the span is not sampled or if tracing is disabled. */ export function startSpanManual(context: StartSpanOptions, callback: (span: Span, finish: () => void) => T): T { const acs = getAcs(); @@ -94,8 +92,7 @@ export function startSpanManual(context: StartSpanOptions, callback: (span: S const spanContext = normalizeContext(context); return withScope(context.scope, scope => { - // eslint-disable-next-line deprecation/deprecation - const parentSpan = scope.getSpan() as SentrySpan | undefined; + const parentSpan = _getSpanForScope(scope) as SentrySpan | undefined; const shouldSkipSpan = context.onlyIfParent && !parentSpan; const activeSpan = shouldSkipSpan @@ -107,8 +104,7 @@ export function startSpanManual(context: StartSpanOptions, callback: (span: S scope, }); - // eslint-disable-next-line deprecation/deprecation - scope.setSpan(activeSpan); + _setSpanForScope(scope, activeSpan); function finishAndSetSpan(): void { activeSpan.end(); @@ -129,13 +125,12 @@ export function startSpanManual(context: StartSpanOptions, callback: (span: S /** * Creates a span. This span is not set as active, so will not get automatic instrumentation spans - * as children or be able to be accessed via `Sentry.getSpan()`. + * as children or be able to be accessed via `Sentry.getActiveSpan()`. * * If you want to create a span that is set as active, use {@link startSpan}. * - * Note that if you have not enabled tracing extensions via `addTracingExtensions` - * or you didn't set `tracesSampleRate` or `tracesSampler`, this function will not generate spans - * and the `span` returned from the callback will be undefined. + * This function will always return a span, + * it may just be a non-recording span if the span is not sampled or if tracing is disabled. */ export function startInactiveSpan(context: StartSpanOptions): Span { const acs = getAcs(); @@ -145,8 +140,7 @@ export function startInactiveSpan(context: StartSpanOptions): Span { const spanContext = normalizeContext(context); const parentSpan = context.scope - ? // eslint-disable-next-line deprecation/deprecation - (context.scope.getSpan() as SentrySpan | undefined) + ? (_getSpanForScope(context.scope) as SentrySpan | undefined) : (getActiveSpan() as SentrySpan | undefined); const shouldSkipSpan = context.onlyIfParent && !parentSpan; @@ -206,8 +200,7 @@ export function withActiveSpan(span: Span | null, callback: (scope: Scope) => } return withScope(scope => { - // eslint-disable-next-line deprecation/deprecation - scope.setSpan(span || undefined); + _setSpanForScope(scope, span || undefined); return callback(scope); }); } diff --git a/packages/core/src/utils/spanUtils.ts b/packages/core/src/utils/spanUtils.ts index 8478778d7dd5..14ca08da69f6 100644 --- a/packages/core/src/utils/spanUtils.ts +++ b/packages/core/src/utils/spanUtils.ts @@ -1,6 +1,7 @@ import type { MeasurementUnit, Primitive, + Scope, Span, SpanAttributes, SpanJSON, @@ -241,6 +242,33 @@ export function getRootSpan(span: SpanWithPotentialChildren): Span { return span[ROOT_SPAN_FIELD] || span; } +const SCOPE_SPAN_FIELD = '_sentrySpan'; + +type ScopeWithMaybeSpan = Scope & { + [SCOPE_SPAN_FIELD]?: Span; +}; + +/** + * Set the active span for a given scope. + * NOTE: This should NOT be used directly, but is only used internally by the trace methods. + */ +export function _setSpanForScope(scope: Scope, span: Span | undefined): void { + if (span) { + addNonEnumerableProperty(scope as ScopeWithMaybeSpan, SCOPE_SPAN_FIELD, span); + } else { + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete + delete (scope as ScopeWithMaybeSpan)[SCOPE_SPAN_FIELD]; + } +} + +/** + * Get the active span for a given scope. + * NOTE: This should NOT be used directly, but is only used internally by the trace methods. + */ +export function _getSpanForScope(scope: ScopeWithMaybeSpan): Span | undefined { + return scope[SCOPE_SPAN_FIELD]; +} + /** * Returns the currently active span. */ @@ -251,8 +279,7 @@ export function getActiveSpan(): Span | undefined { return acs.getActiveSpan(); } - // eslint-disable-next-line deprecation/deprecation - return getCurrentScope().getSpan(); + return _getSpanForScope(getCurrentScope()); } /** diff --git a/packages/core/test/lib/scope.test.ts b/packages/core/test/lib/scope.test.ts index 978ebde52c9f..267053e6a9b0 100644 --- a/packages/core/test/lib/scope.test.ts +++ b/packages/core/test/lib/scope.test.ts @@ -203,23 +203,6 @@ describe('Scope', () => { expect(scope['_user']).toEqual({}); }); - test('setSpan', () => { - const scope = new Scope(); - const span = { fake: 'span' } as any; - // eslint-disable-next-line deprecation/deprecation - scope.setSpan(span); - expect(scope['_span']).toEqual(span); - }); - - test('setSpan with no value unsets it', () => { - const scope = new Scope(); - // eslint-disable-next-line deprecation/deprecation - scope.setSpan({ fake: 'span' } as any); - // eslint-disable-next-line deprecation/deprecation - scope.setSpan(); - expect(scope['_span']).toEqual(undefined); - }); - test('setProcessingMetadata', () => { const scope = new Scope(); scope.setSDKProcessingMetadata({ dogs: 'are great!' }); diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts index 4460be6a02d1..d9ab7c3d765d 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -23,7 +23,7 @@ import { withActiveSpan, } from '../../../src/tracing'; import { SentryNonRecordingSpan } from '../../../src/tracing/sentryNonRecordingSpan'; -import { getActiveSpan, getRootSpan, getSpanDescendants } from '../../../src/utils/spanUtils'; +import { _setSpanForScope, getActiveSpan, getRootSpan, getSpanDescendants } from '../../../src/utils/spanUtils'; import { TestClient, getDefaultTestClientOptions } from '../../mocks/client'; beforeAll(() => { @@ -259,8 +259,7 @@ describe('startSpan', () => { const manualScope = initialScope.clone(); const parentSpan = new SentrySpan({ spanId: 'parent-span-id' }); - // eslint-disable-next-line deprecation/deprecation - manualScope.setSpan(parentSpan); + _setSpanForScope(manualScope, parentSpan); startSpan({ name: 'GET users/[id]', scope: manualScope }, span => { expect(getCurrentScope()).not.toBe(initialScope); @@ -576,8 +575,7 @@ describe('startSpanManual', () => { const manualScope = initialScope.clone(); const parentSpan = new SentrySpan({ spanId: 'parent-span-id' }); - // eslint-disable-next-line deprecation/deprecation - manualScope.setSpan(parentSpan); + _setSpanForScope(manualScope, parentSpan); startSpanManual({ name: 'GET users/[id]', scope: manualScope }, (span, finish) => { expect(getCurrentScope()).not.toBe(initialScope); @@ -842,8 +840,7 @@ describe('startInactiveSpan', () => { const manualScope = initialScope.clone(); const parentSpan = new SentrySpan({ spanId: 'parent-span-id' }); - // eslint-disable-next-line deprecation/deprecation - manualScope.setSpan(parentSpan); + _setSpanForScope(manualScope, parentSpan); const span = startInactiveSpan({ name: 'GET users/[id]', scope: manualScope }); @@ -1209,11 +1206,11 @@ describe('getActiveSpan', () => { it('works with an active span on the scope', () => { const activeSpan = new SentrySpan({ spanId: 'aha' }); - // eslint-disable-next-line deprecation/deprecation - getCurrentScope().setSpan(activeSpan); - const span = getActiveSpan(); - expect(span).toBe(activeSpan); + withActiveSpan(activeSpan, () => { + const span = getActiveSpan(); + expect(span).toBe(activeSpan); + }); }); it('uses implementation from ACS, if it exists', () => { diff --git a/packages/node/src/_setSpanForScope.ts b/packages/node/src/_setSpanForScope.ts new file mode 100644 index 000000000000..e0abd9691b09 --- /dev/null +++ b/packages/node/src/_setSpanForScope.ts @@ -0,0 +1,24 @@ +import type { Scope, Span } from '@sentry/types'; +import { addNonEnumerableProperty } from '@sentry/utils'; + +// This is inlined here from packages/core/src/utils/spanUtils.ts to avoid exporting this from there +// ------------------------ + +const SCOPE_SPAN_FIELD = '_sentrySpan'; + +type ScopeWithMaybeSpan = Scope & { + [SCOPE_SPAN_FIELD]?: Span; +}; + +/** + * Set the active span for a given scope. + * NOTE: This should NOT be used directly, but is only used internally by the trace methods. + */ +export function _setSpanForScope(scope: Scope, span: Span | undefined): void { + if (span) { + addNonEnumerableProperty(scope as ScopeWithMaybeSpan, SCOPE_SPAN_FIELD, span); + } else { + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete + delete (scope as ScopeWithMaybeSpan)[SCOPE_SPAN_FIELD]; + } +} diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index 1eca85b55ed8..b501a79a9231 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -29,6 +29,7 @@ import { normalize, } from '@sentry/utils'; +import { _setSpanForScope } from './_setSpanForScope'; import type { NodeClient } from './client'; import { DEBUG_BUILD } from './debug-build'; import { isAutoSessionTrackingEnabled } from './sdk'; @@ -78,8 +79,7 @@ export function tracingHandler(): ( }); // We put the transaction on the scope so users can attach children to it - // eslint-disable-next-line deprecation/deprecation - scope.setSpan(transaction); + _setSpanForScope(getCurrentScope(), transaction); // We also set __sentry_transaction on the response so people can grab the transaction there to add // spans to it later. @@ -232,8 +232,7 @@ export function errorHandler(options?: { // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access const transaction = (res as any).__sentry_transaction as Span; if (transaction && !getActiveSpan()) { - // eslint-disable-next-line deprecation/deprecation - _scope.setSpan(transaction); + _setSpanForScope(_scope, transaction); } const client = getClient(); diff --git a/packages/node/src/integrations/hapi/index.ts b/packages/node/src/integrations/hapi/index.ts index 58e329479f6a..894eca0748fe 100644 --- a/packages/node/src/integrations/hapi/index.ts +++ b/packages/node/src/integrations/hapi/index.ts @@ -16,6 +16,7 @@ import { import type { IntegrationFn } from '@sentry/types'; import { dynamicSamplingContextToSentryBaggageHeader, fill } from '@sentry/utils'; +import { _setSpanForScope } from '../../_setSpanForScope'; import type { Boom, RequestEvent, ResponseObject, Server } from './types'; @@ -84,8 +85,7 @@ export const hapiTracingPlugin = { }, ); - // eslint-disable-next-line deprecation/deprecation - getCurrentScope().setSpan(transaction); + _setSpanForScope(getCurrentScope(), transaction); return h.continue; }); diff --git a/packages/node/test/integrations/http.test.ts b/packages/node/test/integrations/http.test.ts index e4ce93934ed4..df206904b48f 100644 --- a/packages/node/test/integrations/http.test.ts +++ b/packages/node/test/integrations/http.test.ts @@ -11,6 +11,7 @@ import * as nock from 'nock'; import { HttpsProxyAgent } from '../../src/proxy'; import type { Breadcrumb } from '../../src'; +import { _setSpanForScope } from '../../src/_setSpanForScope'; import { NodeClient } from '../../src/client'; import { Http as HttpIntegration, @@ -32,8 +33,7 @@ describe('tracing', () => { }); afterEach(() => { - // eslint-disable-next-line deprecation/deprecation - getCurrentScope().setSpan(undefined); + _setSpanForScope(getCurrentScope(), undefined); }); function createTransactionOnScope( @@ -54,9 +54,7 @@ describe('tracing', () => { }); expect(transaction).toBeInstanceOf(Transaction); - // eslint-disable-next-line deprecation/deprecation - getCurrentScope().setSpan(transaction); - + _setSpanForScope(getCurrentScope(), transaction); return transaction; } @@ -353,8 +351,7 @@ describe('tracing', () => { function createTransactionAndPutOnScope() { addTracingExtensions(); const transaction = startInactiveSpan({ name: 'dogpark' }); - // eslint-disable-next-line deprecation/deprecation - getCurrentScope().setSpan(transaction); + _setSpanForScope(getCurrentScope(), transaction); return transaction; } diff --git a/packages/opentelemetry/src/trace.ts b/packages/opentelemetry/src/trace.ts index d47c8db5ec2b..31bdb46eec67 100644 --- a/packages/opentelemetry/src/trace.ts +++ b/packages/opentelemetry/src/trace.ts @@ -24,11 +24,12 @@ import { getDynamicSamplingContextFromSpan } from './utils/dynamicSamplingContex /** * Wraps a function with a transaction/span and finishes the span after the function is done. * 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.getSpan()`, as long as the function is executed while the scope is active. + * and can be accessed via `Sentry.getActiveSpan()`, as long as the function is executed while the scope is active. * * If you want to create a span that is not set as active, use {@link startInactiveSpan}. * - * Note that you'll always get a span passed to the callback, it may just be a NonRecordingSpan if the span is not sampled. + * You'll always get a span passed to the callback, + * it may just be a non-recording span if the span is not sampled or if tracing is disabled. */ export function startSpan(options: OpenTelemetrySpanContext, callback: (span: Span) => T): T { const tracer = getTracer(); @@ -64,7 +65,8 @@ export function startSpan(options: OpenTelemetrySpanContext, callback: (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. * - * Note that you'll always get a span passed to the callback, it may just be a NonRecordingSpan if the span is not sampled. + * You'll always get a span passed to the callback, + * it may just be a non-recording span if the span is not sampled or if tracing is disabled. */ export function startSpanManual( options: OpenTelemetrySpanContext, @@ -102,13 +104,12 @@ export const startActiveSpan = startSpan; /** * Creates a span. This span is not set as active, so will not get automatic instrumentation spans - * as children or be able to be accessed via `Sentry.getSpan()`. + * as children or be able to be accessed via `Sentry.getActiveSpan()`. * * If you want to create a span that is set as active, use {@link startSpan}. * - * Note that if you have not enabled tracing extensions via `addTracingExtensions` - * or you didn't set `tracesSampleRate` or `tracesSampler`, this function will not generate spans - * and the `span` returned from the callback will be undefined. + * This function will always return a span, + * it may just be a non-recording span if the span is not sampled or if tracing is disabled. */ export function startInactiveSpan(options: OpenTelemetrySpanContext): Span { const tracer = getTracer(); @@ -133,7 +134,7 @@ export function startInactiveSpan(options: OpenTelemetrySpanContext): Span { * passed `null` to start an entirely new span tree. * * @param span Spans started in the context of the provided callback will be children of this span. If `null` is passed, - * spans started within the callback will not be attached to a parent span. + * spans started within the callback will be root spans. * @param callback Execution context in which the provided span will be active. Is passed the newly forked scope. * @returns the value returned from the provided callback function. */ diff --git a/packages/types/src/scope.ts b/packages/types/src/scope.ts index 7883ee0eff91..641a77dd080c 100644 --- a/packages/types/src/scope.ts +++ b/packages/types/src/scope.ts @@ -145,19 +145,6 @@ export interface Scope { */ setContext(name: string, context: Context | null): this; - /** - * Sets the Span on the scope. - * @param span Span - * @deprecated Instead of setting a span on a scope, use `startSpan()`/`startSpanManual()` instead. - */ - setSpan(span?: Span): this; - - /** - * Returns the `Span` if there is one. - * @deprecated Use `getActiveSpan()` instead. - */ - getSpan(): Span | undefined; - /** * Returns the `Transaction` attached to the scope (if there is one). * @deprecated You should not rely on the transaction, but just use `startSpan()` APIs instead. From 1b1f320ec43b2003685c472ab56d8d87f952cff0 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 25 Mar 2024 12:03:51 +0100 Subject: [PATCH 2/6] fix circular dep check --- packages/core/src/scope.ts | 2 +- packages/core/src/server-runtime-client.ts | 3 +- packages/core/src/tracing/idleSpan.ts | 2 +- packages/core/src/tracing/trace.ts | 3 +- packages/core/src/utils/spanOnScope.ts | 29 ++++++++++++++++++++ packages/core/src/utils/spanUtils.ts | 29 +------------------- packages/core/test/lib/tracing/trace.test.ts | 3 +- 7 files changed, 37 insertions(+), 34 deletions(-) create mode 100644 packages/core/src/utils/spanOnScope.ts diff --git a/packages/core/src/scope.ts b/packages/core/src/scope.ts index c42a3821d40c..a86bbfafcc8f 100644 --- a/packages/core/src/scope.ts +++ b/packages/core/src/scope.ts @@ -26,7 +26,7 @@ import { dateTimestampInSeconds, isPlainObject, logger, uuid4 } from '@sentry/ut import { updateSession } from './session'; import type { SentrySpan } from './tracing/sentrySpan'; -import { _getSpanForScope, _setSpanForScope } from './utils/spanUtils'; +import { _getSpanForScope, _setSpanForScope } from './utils/spanOnScope'; /** * Default value for maximum number of breadcrumbs added to an event. diff --git a/packages/core/src/server-runtime-client.ts b/packages/core/src/server-runtime-client.ts index 1c2ffba399e4..9cd05374ddd3 100644 --- a/packages/core/src/server-runtime-client.ts +++ b/packages/core/src/server-runtime-client.ts @@ -24,7 +24,8 @@ import { getDynamicSamplingContextFromClient, getDynamicSamplingContextFromSpan, } from './tracing'; -import { _getSpanForScope, getRootSpan, spanToTraceContext } from './utils/spanUtils'; +import { _getSpanForScope } from './utils/spanOnScope'; +import { getRootSpan, spanToTraceContext } from './utils/spanUtils'; export interface ServerRuntimeClientOptions extends ClientOptions { platform?: string; diff --git a/packages/core/src/tracing/idleSpan.ts b/packages/core/src/tracing/idleSpan.ts index ce96d4bb32bb..8dc61426f6dd 100644 --- a/packages/core/src/tracing/idleSpan.ts +++ b/packages/core/src/tracing/idleSpan.ts @@ -5,8 +5,8 @@ import { getClient, getCurrentScope } from '../currentScopes'; import { DEBUG_BUILD } from '../debug-build'; import { SEMANTIC_ATTRIBUTE_SENTRY_IDLE_SPAN_FINISH_REASON } from '../semanticAttributes'; import { hasTracingEnabled } from '../utils/hasTracingEnabled'; +import { _setSpanForScope } from '../utils/spanOnScope'; import { - _setSpanForScope, getActiveSpan, getSpanDescendants, removeChildSpanFromSpan, diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index 63fc845fca0e..e4102b610cc1 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -9,9 +9,8 @@ import { getAsyncContextStrategy, getCurrentHub } from '../hub'; import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE } from '../semanticAttributes'; import { handleCallbackErrors } from '../utils/handleCallbackErrors'; import { hasTracingEnabled } from '../utils/hasTracingEnabled'; +import { _getSpanForScope, _setSpanForScope } from '../utils/spanOnScope'; import { - _getSpanForScope, - _setSpanForScope, addChildSpanToSpan, getActiveSpan, spanIsSampled, diff --git a/packages/core/src/utils/spanOnScope.ts b/packages/core/src/utils/spanOnScope.ts new file mode 100644 index 000000000000..f6403b57b1b4 --- /dev/null +++ b/packages/core/src/utils/spanOnScope.ts @@ -0,0 +1,29 @@ +import type { Scope, Span } from '@sentry/types'; +import { addNonEnumerableProperty } from '@sentry/utils'; + +const SCOPE_SPAN_FIELD = '_sentrySpan'; + +type ScopeWithMaybeSpan = Scope & { + [SCOPE_SPAN_FIELD]?: Span; +}; + +/** + * Set the active span for a given scope. + * NOTE: This should NOT be used directly, but is only used internally by the trace methods. + */ +export function _setSpanForScope(scope: Scope, span: Span | undefined): void { + if (span) { + addNonEnumerableProperty(scope as ScopeWithMaybeSpan, SCOPE_SPAN_FIELD, span); + } else { + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete + delete (scope as ScopeWithMaybeSpan)[SCOPE_SPAN_FIELD]; + } +} + +/** + * Get the active span for a given scope. + * NOTE: This should NOT be used directly, but is only used internally by the trace methods. + */ +export function _getSpanForScope(scope: ScopeWithMaybeSpan): Span | undefined { + return scope[SCOPE_SPAN_FIELD]; +} diff --git a/packages/core/src/utils/spanUtils.ts b/packages/core/src/utils/spanUtils.ts index 14ca08da69f6..3d06aa0d3204 100644 --- a/packages/core/src/utils/spanUtils.ts +++ b/packages/core/src/utils/spanUtils.ts @@ -1,7 +1,6 @@ import type { MeasurementUnit, Primitive, - Scope, Span, SpanAttributes, SpanJSON, @@ -24,6 +23,7 @@ import type { MetricType } from '../metrics/types'; import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '../semanticAttributes'; import type { SentrySpan } from '../tracing/sentrySpan'; import { SPAN_STATUS_OK, SPAN_STATUS_UNSET } from '../tracing/spanstatus'; +import { _getSpanForScope } from './spanOnScope'; // These are aligned with OpenTelemetry trace flags export const TRACE_FLAG_NONE = 0x0; @@ -242,33 +242,6 @@ export function getRootSpan(span: SpanWithPotentialChildren): Span { return span[ROOT_SPAN_FIELD] || span; } -const SCOPE_SPAN_FIELD = '_sentrySpan'; - -type ScopeWithMaybeSpan = Scope & { - [SCOPE_SPAN_FIELD]?: Span; -}; - -/** - * Set the active span for a given scope. - * NOTE: This should NOT be used directly, but is only used internally by the trace methods. - */ -export function _setSpanForScope(scope: Scope, span: Span | undefined): void { - if (span) { - addNonEnumerableProperty(scope as ScopeWithMaybeSpan, SCOPE_SPAN_FIELD, span); - } else { - // eslint-disable-next-line @typescript-eslint/no-dynamic-delete - delete (scope as ScopeWithMaybeSpan)[SCOPE_SPAN_FIELD]; - } -} - -/** - * Get the active span for a given scope. - * NOTE: This should NOT be used directly, but is only used internally by the trace methods. - */ -export function _getSpanForScope(scope: ScopeWithMaybeSpan): Span | undefined { - return scope[SCOPE_SPAN_FIELD]; -} - /** * Returns the currently active span. */ diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts index d9ab7c3d765d..58fc5623f587 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -23,7 +23,8 @@ import { withActiveSpan, } from '../../../src/tracing'; import { SentryNonRecordingSpan } from '../../../src/tracing/sentryNonRecordingSpan'; -import { _setSpanForScope, getActiveSpan, getRootSpan, getSpanDescendants } from '../../../src/utils/spanUtils'; +import { _setSpanForScope } from '../../../src/utils/spanOnScope'; +import { getActiveSpan, getRootSpan, getSpanDescendants } from '../../../src/utils/spanUtils'; import { TestClient, getDefaultTestClientOptions } from '../../mocks/client'; beforeAll(() => { From fe862961904b16ba5cf43df49479d1fdca8d577d Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 25 Mar 2024 12:47:36 +0100 Subject: [PATCH 3/6] fix test --- .../backgroundtab-custom/init.js | 8 +++++++- .../backgroundtab-custom/subject.js | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/backgroundtab-custom/init.js b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/backgroundtab-custom/init.js index 147a4aa26bfb..635210f2252c 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/backgroundtab-custom/init.js +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/backgroundtab-custom/init.js @@ -4,6 +4,12 @@ window.Sentry = Sentry; Sentry.init({ dsn: 'https://public@dsn.ingest.sentry.io/1337', - integrations: [Sentry.browserTracingIntegration({ idleTimeout: 9000, instrumentPageLoad: false })], + integrations: [ + Sentry.browserTracingIntegration({ + idleTimeout: 9000, + instrumentPageLoad: false, + instrumentNavigation: false, + }), + ], tracesSampleRate: 1, }); diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/backgroundtab-custom/subject.js b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/backgroundtab-custom/subject.js index 1d6cf5f0627b..0ccb8f15d8bf 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/backgroundtab-custom/subject.js +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/backgroundtab-custom/subject.js @@ -6,7 +6,7 @@ document.getElementById('go-background').addEventListener('click', () => { }); document.getElementById('start-span').addEventListener('click', () => { - const span = Sentry.startBrowserTracingNavigationSpan({ name: 'test-span' }); + const span = Sentry.startBrowserTracingNavigationSpan(Sentry.getClient(), { name: 'test-span' }); window.span = span; }); From 6d4b745c359d5a270293c3b51eab82ff23c6cec2 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 26 Mar 2024 16:12:50 +0100 Subject: [PATCH 4/6] fix bundle --- dev-packages/rollup-utils/plugins/bundlePlugins.mjs | 1 + 1 file changed, 1 insertion(+) diff --git a/dev-packages/rollup-utils/plugins/bundlePlugins.mjs b/dev-packages/rollup-utils/plugins/bundlePlugins.mjs index 80dbc4422ea4..07460dcf93f6 100644 --- a/dev-packages/rollup-utils/plugins/bundlePlugins.mjs +++ b/dev-packages/rollup-utils/plugins/bundlePlugins.mjs @@ -127,6 +127,7 @@ export function makeTerserPlugin() { // These are used to keep span relationships '_sentryRootSpan', '_sentryChildSpans', + '_sentrySpan', ], }, }, From 9078ff92fc54fb7d0958c78c12a0089d8faa4d02 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 26 Mar 2024 16:56:49 +0100 Subject: [PATCH 5/6] fix exports --- .../backgroundtab-custom/test.ts | 2 +- .../rollup-utils/plugins/bundlePlugins.mjs | 4 +++- .../src/index.bundle.tracing.replay.feedback.ts | 15 +++++++++++++-- .../browser/src/index.bundle.tracing.replay.ts | 8 +++++++- packages/browser/src/index.bundle.tracing.ts | 8 +++++++- 5 files changed, 31 insertions(+), 6 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/backgroundtab-custom/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/backgroundtab-custom/test.ts index fad37e85d6b4..1e56b405f579 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/backgroundtab-custom/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/backgroundtab-custom/test.ts @@ -10,7 +10,7 @@ sentryTest('should finish a custom transaction when the page goes background', a } const url = await getLocalTestPath({ testDir: __dirname }); - page.goto(url); + await page.goto(url); await page.locator('#start-span').click(); const spanJsonBefore: SpanJSON = await page.evaluate('window.getSpanJson()'); diff --git a/dev-packages/rollup-utils/plugins/bundlePlugins.mjs b/dev-packages/rollup-utils/plugins/bundlePlugins.mjs index 07460dcf93f6..ae9814dbab5b 100644 --- a/dev-packages/rollup-utils/plugins/bundlePlugins.mjs +++ b/dev-packages/rollup-utils/plugins/bundlePlugins.mjs @@ -124,10 +124,12 @@ export function makeTerserPlugin() { // For v7 backwards-compatibility we need to access txn._frozenDynamicSamplingContext // TODO (v8): Remove this reserved word '_frozenDynamicSamplingContext', - // These are used to keep span relationships + // These are used to keep span & scope relationships '_sentryRootSpan', '_sentryChildSpans', '_sentrySpan', + '_sentryScope', + '_sentryIsolationScope' ], }, }, diff --git a/packages/browser/src/index.bundle.tracing.replay.feedback.ts b/packages/browser/src/index.bundle.tracing.replay.feedback.ts index 48064ca051bd..41db6a7cb5f7 100644 --- a/packages/browser/src/index.bundle.tracing.replay.feedback.ts +++ b/packages/browser/src/index.bundle.tracing.replay.feedback.ts @@ -1,6 +1,10 @@ import { feedbackIntegration } from '@sentry-internal/feedback'; import { replayIntegration } from '@sentry-internal/replay'; -import { browserTracingIntegration } from '@sentry-internal/tracing'; +import { + browserTracingIntegration, + startBrowserTracingNavigationSpan, + startBrowserTracingPageLoadSpan, +} from '@sentry-internal/tracing'; import { addTracingExtensions } from '@sentry/core'; // We are patching the global object with our hub extension methods @@ -16,6 +20,13 @@ export { getSpanDescendants, } from '@sentry/core'; -export { feedbackIntegration, replayIntegration, browserTracingIntegration, addTracingExtensions }; +export { + feedbackIntegration, + replayIntegration, + browserTracingIntegration, + addTracingExtensions, + startBrowserTracingNavigationSpan, + startBrowserTracingPageLoadSpan, +}; export * from './index.bundle.base'; diff --git a/packages/browser/src/index.bundle.tracing.replay.ts b/packages/browser/src/index.bundle.tracing.replay.ts index 52d082dd3438..7373fc8e5040 100644 --- a/packages/browser/src/index.bundle.tracing.replay.ts +++ b/packages/browser/src/index.bundle.tracing.replay.ts @@ -1,6 +1,10 @@ import { feedbackIntegrationShim } from '@sentry-internal/integration-shims'; import { replayIntegration } from '@sentry-internal/replay'; -import { browserTracingIntegration } from '@sentry-internal/tracing'; +import { + browserTracingIntegration, + startBrowserTracingNavigationSpan, + startBrowserTracingPageLoadSpan, +} from '@sentry-internal/tracing'; import { addTracingExtensions } from '@sentry/core'; // We are patching the global object with our hub extension methods @@ -21,6 +25,8 @@ export { feedbackIntegrationShim as feedbackIntegration, browserTracingIntegration, addTracingExtensions, + startBrowserTracingNavigationSpan, + startBrowserTracingPageLoadSpan, }; export * from './index.bundle.base'; diff --git a/packages/browser/src/index.bundle.tracing.ts b/packages/browser/src/index.bundle.tracing.ts index 23c424fd7337..fcbc43974a91 100644 --- a/packages/browser/src/index.bundle.tracing.ts +++ b/packages/browser/src/index.bundle.tracing.ts @@ -1,6 +1,10 @@ // This is exported so the loader does not fail when switching off Replay import { feedbackIntegrationShim, replayIntegrationShim } from '@sentry-internal/integration-shims'; -import { browserTracingIntegration } from '@sentry-internal/tracing'; +import { + browserTracingIntegration, + startBrowserTracingNavigationSpan, + startBrowserTracingPageLoadSpan, +} from '@sentry-internal/tracing'; import { addTracingExtensions } from '@sentry/core'; // We are patching the global object with our hub extension methods @@ -21,6 +25,8 @@ export { replayIntegrationShim as replayIntegration, browserTracingIntegration, addTracingExtensions, + startBrowserTracingPageLoadSpan, + startBrowserTracingNavigationSpan, }; export * from './index.bundle.base'; From b29f89a38c9e2a12534fe457402836db430f2823 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 26 Mar 2024 17:12:01 +0100 Subject: [PATCH 6/6] fix linting --- dev-packages/rollup-utils/plugins/bundlePlugins.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-packages/rollup-utils/plugins/bundlePlugins.mjs b/dev-packages/rollup-utils/plugins/bundlePlugins.mjs index ae9814dbab5b..38c7811f3d1a 100644 --- a/dev-packages/rollup-utils/plugins/bundlePlugins.mjs +++ b/dev-packages/rollup-utils/plugins/bundlePlugins.mjs @@ -129,7 +129,7 @@ export function makeTerserPlugin() { '_sentryChildSpans', '_sentrySpan', '_sentryScope', - '_sentryIsolationScope' + '_sentryIsolationScope', ], }, },