From 99b040667a00be3b4260c673562acbc96fec9070 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 12 Dec 2023 14:16:24 +0100 Subject: [PATCH 1/2] ref(core): Replace `Scope.clone()` with non-static `scope.clone()` --- packages/core/src/hub.ts | 4 +- packages/core/src/scope.ts | 42 +++++++++++--------- packages/core/src/utils/prepareEvent.ts | 15 +++++-- packages/opentelemetry/src/contextManager.ts | 3 +- packages/opentelemetry/src/custom/client.ts | 12 ++++-- packages/opentelemetry/src/custom/hub.ts | 13 ------ packages/opentelemetry/src/custom/scope.ts | 40 +++++++++++-------- packages/opentelemetry/src/spanExporter.ts | 4 +- 8 files changed, 73 insertions(+), 60 deletions(-) diff --git a/packages/core/src/hub.ts b/packages/core/src/hub.ts index 8306d033cd75..13d5fd059e93 100644 --- a/packages/core/src/hub.ts +++ b/packages/core/src/hub.ts @@ -142,7 +142,7 @@ export class Hub implements HubInterface { */ public pushScope(): Scope { // We want to clone the content of prev scope - const scope = Scope.clone(this.getScope()); + const scope = this.getScope().clone(); this.getStack().push({ client: this.getClient(), scope, @@ -578,7 +578,7 @@ export function ensureHubOnCarrier(carrier: Carrier, parent: Hub = getGlobalHub( // If there's no hub on current domain, or it's an old API, assign a new one if (!hasHubOnCarrier(carrier) || getHubFromCarrier(carrier).isOlderThan(API_VERSION)) { const globalHubTopStack = parent.getStackTop(); - setHubOnCarrier(carrier, new Hub(globalHubTopStack.client, Scope.clone(globalHubTopStack.scope))); + setHubOnCarrier(carrier, new Hub(globalHubTopStack.client, globalHubTopStack.scope.clone())); } } diff --git a/packages/core/src/scope.ts b/packages/core/src/scope.ts index 266087dd6090..ae6fe70e3185 100644 --- a/packages/core/src/scope.ts +++ b/packages/core/src/scope.ts @@ -110,27 +110,33 @@ export class Scope implements ScopeInterface { /** * Inherit values from the parent scope. - * @param scope to clone. + * @deprecated Use `scope.clone()` and `new Scope()` instead. */ public static clone(scope?: Scope): Scope { + return scope ? scope.clone() : new Scope(); + } + + /** + * Clone this scope instance. + */ + public clone(): Scope { const newScope = new Scope(); - if (scope) { - newScope._breadcrumbs = [...scope._breadcrumbs]; - newScope._tags = { ...scope._tags }; - newScope._extra = { ...scope._extra }; - newScope._contexts = { ...scope._contexts }; - newScope._user = scope._user; - newScope._level = scope._level; - newScope._span = scope._span; - newScope._session = scope._session; - newScope._transactionName = scope._transactionName; - newScope._fingerprint = scope._fingerprint; - newScope._eventProcessors = [...scope._eventProcessors]; - newScope._requestSession = scope._requestSession; - newScope._attachments = [...scope._attachments]; - newScope._sdkProcessingMetadata = { ...scope._sdkProcessingMetadata }; - newScope._propagationContext = { ...scope._propagationContext }; - } + newScope._breadcrumbs = [...this._breadcrumbs]; + newScope._tags = { ...this._tags }; + newScope._extra = { ...this._extra }; + 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; + newScope._eventProcessors = [...this._eventProcessors]; + newScope._requestSession = this._requestSession; + newScope._attachments = [...this._attachments]; + newScope._sdkProcessingMetadata = { ...this._sdkProcessingMetadata }; + newScope._propagationContext = { ...this._propagationContext }; + return newScope; } diff --git a/packages/core/src/utils/prepareEvent.ts b/packages/core/src/utils/prepareEvent.ts index 6e25350202b4..46b26070653f 100644 --- a/packages/core/src/utils/prepareEvent.ts +++ b/packages/core/src/utils/prepareEvent.ts @@ -74,10 +74,7 @@ export function prepareEvent( // If we have scope given to us, use it as the base for further modifications. // This allows us to prevent unnecessary copying of data if `captureContext` is not provided. - let finalScope = scope; - if (hint.captureContext) { - finalScope = Scope.clone(finalScope).update(hint.captureContext); - } + const finalScope = getFinalScope(scope, hint.captureContext); if (hint.mechanism) { addExceptionMechanism(prepared, hint.mechanism); @@ -349,6 +346,16 @@ function normalizeEvent(event: Event | null, depth: number, maxBreadth: number): return normalized; } +function getFinalScope(scope: Scope | undefined, captureContext: CaptureContext | undefined): Scope | undefined { + if (!captureContext) { + return scope; + } + + const finalScope = scope ? scope.clone() : new Scope(); + finalScope.update(captureContext); + return finalScope; +} + /** * Parse either an `EventHint` directly, or convert a `CaptureContext` to an `EventHint`. * This is used to allow to update method signatures that used to accept a `CaptureContext` but should now accept an `EventHint`. diff --git a/packages/opentelemetry/src/contextManager.ts b/packages/opentelemetry/src/contextManager.ts index ca9305dfea9b..3b3a6a280928 100644 --- a/packages/opentelemetry/src/contextManager.ts +++ b/packages/opentelemetry/src/contextManager.ts @@ -1,7 +1,8 @@ import type { Context, ContextManager } from '@opentelemetry/api'; import type { Carrier, Hub } from '@sentry/core'; +import { ensureHubOnCarrier } from '@sentry/core'; -import { ensureHubOnCarrier, getCurrentHub, getHubFromCarrier } from './custom/hub'; +import { getCurrentHub, getHubFromCarrier } from './custom/hub'; import { setHubOnContext } from './utils/contextData'; function createNewHub(parent: Hub | undefined): Hub { diff --git a/packages/opentelemetry/src/custom/client.ts b/packages/opentelemetry/src/custom/client.ts index 36648b63c22f..44dd0cf80f4f 100644 --- a/packages/opentelemetry/src/custom/client.ts +++ b/packages/opentelemetry/src/custom/client.ts @@ -3,7 +3,7 @@ import { trace } from '@opentelemetry/api'; import type { BasicTracerProvider } from '@opentelemetry/sdk-trace-base'; import type { BaseClient, Scope } from '@sentry/core'; import { SDK_VERSION } from '@sentry/core'; -import type { Client, Event, EventHint } from '@sentry/types'; +import type { CaptureContext, Client, Event, EventHint } from '@sentry/types'; import type { OpenTelemetryClient as OpenTelemetryClientInterface } from '../types'; import { OpenTelemetryScope } from './scope'; @@ -65,14 +65,14 @@ export function wrapClientClass< /** * Extends the base `_prepareEvent` so that we can properly handle `captureContext`. - * This uses `Scope.clone()`, which we need to replace with `NodeExperimentalScope.clone()` for this client. + * This uses `Scope.clone()`, which we need to replace with `OpenTelemetryScope.clone()` for this client. */ protected _prepareEvent(event: Event, hint: EventHint, scope?: Scope): PromiseLike { let actualScope = scope; // Remove `captureContext` hint and instead clone already here if (hint && hint.captureContext) { - actualScope = OpenTelemetryScope.clone(scope); + actualScope = getFinalScope(scope, hint.captureContext); delete hint.captureContext; } @@ -83,3 +83,9 @@ export function wrapClientClass< return OpenTelemetryClient as unknown as WrappedClassConstructor; } /* eslint-enable @typescript-eslint/no-explicit-any */ + +function getFinalScope(scope: Scope | undefined, captureContext: CaptureContext): Scope | undefined { + const finalScope = scope ? scope.clone() : new OpenTelemetryScope(); + finalScope.update(captureContext); + return finalScope; +} diff --git a/packages/opentelemetry/src/custom/hub.ts b/packages/opentelemetry/src/custom/hub.ts index 3ecea85b0a6f..871e615b7553 100644 --- a/packages/opentelemetry/src/custom/hub.ts +++ b/packages/opentelemetry/src/custom/hub.ts @@ -13,19 +13,6 @@ export class OpenTelemetryHub extends Hub { public constructor(client?: Client, scope: Scope = new OpenTelemetryScope()) { super(client, scope); } - - /** - * @inheritDoc - */ - public pushScope(): Scope { - // We want to clone the content of prev scope - const scope = OpenTelemetryScope.clone(this.getScope()); - this.getStack().push({ - client: this.getClient(), - scope, - }); - return scope; - } } /** Custom getClient method that uses the custom hub. */ diff --git a/packages/opentelemetry/src/custom/scope.ts b/packages/opentelemetry/src/custom/scope.ts index b65aa437b14f..e206ba8d8096 100644 --- a/packages/opentelemetry/src/custom/scope.ts +++ b/packages/opentelemetry/src/custom/scope.ts @@ -23,24 +23,30 @@ export class OpenTelemetryScope extends Scope { * @inheritDoc */ public static clone(scope?: Scope): Scope { + return scope ? scope.clone() : new OpenTelemetryScope(); + } + + /** + * Clone this scope instance. + */ + public clone(): OpenTelemetryScope { const newScope = new OpenTelemetryScope(); - if (scope) { - newScope._breadcrumbs = [...scope['_breadcrumbs']]; - newScope._tags = { ...scope['_tags'] }; - newScope._extra = { ...scope['_extra'] }; - newScope._contexts = { ...scope['_contexts'] }; - newScope._user = scope['_user']; - newScope._level = scope['_level']; - newScope._span = scope['_span']; - newScope._session = scope['_session']; - newScope._transactionName = scope['_transactionName']; - newScope._fingerprint = scope['_fingerprint']; - newScope._eventProcessors = [...scope['_eventProcessors']]; - newScope._requestSession = scope['_requestSession']; - newScope._attachments = [...scope['_attachments']]; - newScope._sdkProcessingMetadata = { ...scope['_sdkProcessingMetadata'] }; - newScope._propagationContext = { ...scope['_propagationContext'] }; - } + newScope._breadcrumbs = [...this['_breadcrumbs']]; + newScope._tags = { ...this['_tags'] }; + newScope._extra = { ...this['_extra'] }; + 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']; + newScope._eventProcessors = [...this['_eventProcessors']]; + newScope._requestSession = this['_requestSession']; + newScope._attachments = [...this['_attachments']]; + newScope._sdkProcessingMetadata = { ...this['_sdkProcessingMetadata'] }; + newScope._propagationContext = { ...this['_propagationContext'] }; + return newScope; } diff --git a/packages/opentelemetry/src/spanExporter.ts b/packages/opentelemetry/src/spanExporter.ts index d3c3b5b3da90..95ad13997fb9 100644 --- a/packages/opentelemetry/src/spanExporter.ts +++ b/packages/opentelemetry/src/spanExporter.ts @@ -112,8 +112,8 @@ function maybeSend(spans: ReadableSpan[]): ReadableSpan[] { // Now finish the transaction, which will send it together with all the spans // We make sure to use the current span as the activeSpan for this transaction - const scope = getSpanScope(span); - const forkedScope = OpenTelemetryScope.clone(scope as OpenTelemetryScope | undefined) as OpenTelemetryScope; + const scope = getSpanScope(span) as OpenTelemetryScope | undefined; + const forkedScope = scope ? scope.clone() : new OpenTelemetryScope(); forkedScope.activeSpan = span as unknown as Span; transaction.finishWithScope(convertOtelTimeToSeconds(span.endTime), forkedScope); From 7b534230751b1236b7c0a2f33f612a963931d2a1 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 12 Dec 2023 15:10:36 +0100 Subject: [PATCH 2/2] linting fixes --- .../opentelemetry-node/test/spanprocessor.test.ts | 12 ++---------- packages/opentelemetry/src/custom/hub.ts | 5 +---- packages/opentelemetry/test/custom/scope.test.ts | 2 ++ 3 files changed, 5 insertions(+), 14 deletions(-) diff --git a/packages/opentelemetry-node/test/spanprocessor.test.ts b/packages/opentelemetry-node/test/spanprocessor.test.ts index 5361055755ff..9de394d2232d 100644 --- a/packages/opentelemetry-node/test/spanprocessor.test.ts +++ b/packages/opentelemetry-node/test/spanprocessor.test.ts @@ -5,15 +5,7 @@ import type { Span as OtelSpan } from '@opentelemetry/sdk-trace-base'; import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node'; import { SemanticAttributes, SemanticResourceAttributes } from '@opentelemetry/semantic-conventions'; import type { SpanStatusType } from '@sentry/core'; -import { - Hub, - Scope, - Span as SentrySpan, - Transaction, - addTracingExtensions, - createTransport, - makeMain, -} from '@sentry/core'; +import { Hub, Span as SentrySpan, Transaction, addTracingExtensions, createTransport, makeMain } from '@sentry/core'; import { NodeClient } from '@sentry/node'; import { resolvedSyncPromise } from '@sentry/utils'; @@ -973,7 +965,7 @@ describe('SentrySpanProcessor', () => { hub = new Hub(client); makeMain(hub); - const newHub = new Hub(client, Scope.clone(hub.getScope())); + const newHub = new Hub(client, hub.getScope().clone()); newHub.configureScope(scope => { scope.setTag('foo', 'bar'); }); diff --git a/packages/opentelemetry/src/custom/hub.ts b/packages/opentelemetry/src/custom/hub.ts index 871e615b7553..ed37ae75b09f 100644 --- a/packages/opentelemetry/src/custom/hub.ts +++ b/packages/opentelemetry/src/custom/hub.ts @@ -97,10 +97,7 @@ export function ensureHubOnCarrier(carrier: Carrier, parent: Hub = getGlobalHub( // If there's no hub on current domain, or it's an old API, assign a new one if (!hasHubOnCarrier(carrier) || getHubFromCarrier(carrier).isOlderThan(API_VERSION)) { const globalHubTopStack = parent.getStackTop(); - setHubOnCarrier( - carrier, - new OpenTelemetryHub(globalHubTopStack.client, OpenTelemetryScope.clone(globalHubTopStack.scope)), - ); + setHubOnCarrier(carrier, new OpenTelemetryHub(globalHubTopStack.client, globalHubTopStack.scope.clone())); } } diff --git a/packages/opentelemetry/test/custom/scope.test.ts b/packages/opentelemetry/test/custom/scope.test.ts index 6c96fab2f3c5..41827fcd772d 100644 --- a/packages/opentelemetry/test/custom/scope.test.ts +++ b/packages/opentelemetry/test/custom/scope.test.ts @@ -30,6 +30,7 @@ describe('NodeExperimentalScope', () => { scope['_attachments'] = [{ data: '123', filename: 'test.txt' }]; scope['_sdkProcessingMetadata'] = { sdk: 'bar' }; + // eslint-disable-next-line deprecation/deprecation const scope2 = OpenTelemetryScope.clone(scope); expect(scope2).toBeInstanceOf(OpenTelemetryScope); @@ -68,6 +69,7 @@ describe('NodeExperimentalScope', () => { }); it('clone() works without existing scope', () => { + // eslint-disable-next-line deprecation/deprecation const scope = OpenTelemetryScope.clone(undefined); expect(scope).toBeInstanceOf(OpenTelemetryScope);