From c9552a4601df2fe4ac660cabd2e1ee3e64f798db Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 19 Dec 2023 09:59:19 +0100 Subject: [PATCH] ref(node): Refactor LocalVariables integration to avoid `setupOnce` (#9897) Slowly getting rid of `getCurrentHub()`... --- packages/core/src/baseclient.ts | 4 +- .../node/src/integrations/localvariables.ts | 29 +++++--- .../test/integrations/localvariables.test.ts | 68 ++++++++----------- 3 files changed, 51 insertions(+), 50 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index c5d9fff7df6f..3fbdd13250f8 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -115,14 +115,14 @@ export abstract class BaseClient implements Client { /** Number of calls being processed */ protected _numProcessing: number; + protected _eventProcessors: EventProcessor[]; + /** Holds flushable */ private _outcomes: { [key: string]: number }; // eslint-disable-next-line @typescript-eslint/ban-types private _hooks: Record; - private _eventProcessors: EventProcessor[]; - /** * Initializes this client instance. * diff --git a/packages/node/src/integrations/localvariables.ts b/packages/node/src/integrations/localvariables.ts index a41822331ea5..1ba9907c4806 100644 --- a/packages/node/src/integrations/localvariables.ts +++ b/packages/node/src/integrations/localvariables.ts @@ -2,9 +2,9 @@ import type { Event, EventProcessor, Exception, Hub, Integration, StackFrame, StackParser } from '@sentry/types'; import { LRUMap, logger } from '@sentry/utils'; import type { Debugger, InspectorNotification, Runtime, Session } from 'inspector'; +import type { NodeClient } from '../client'; import { NODE_VERSION } from '../nodeVersion'; -import type { NodeClientOptions } from '../types'; type Variables = Record; type OnPauseEvent = InspectorNotification; @@ -332,6 +332,7 @@ export class LocalVariables implements Integration { private readonly _cachedFrames: LRUMap = new LRUMap(20); private _rateLimiter: RateLimitIncrement | undefined; + private _shouldProcessEvent = false; public constructor( private readonly _options: Options = {}, @@ -341,16 +342,15 @@ export class LocalVariables implements Integration { /** * @inheritDoc */ - public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void { - this._setup(addGlobalEventProcessor, getCurrentHub().getClient()?.getOptions()); + public setupOnce(_addGlobalEventProcessor: (callback: EventProcessor) => void, _getCurrentHub: () => Hub): void { + // noop } - /** Setup in a way that's easier to call from tests */ - private _setup( - addGlobalEventProcessor: (callback: EventProcessor) => void, - clientOptions: NodeClientOptions | undefined, - ): void { - if (this._session && clientOptions?.includeLocalVariables) { + /** @inheritdoc */ + public setup(client: NodeClient): void { + const clientOptions = client.getOptions(); + + if (this._session && clientOptions.includeLocalVariables) { // Only setup this integration if the Node version is >= v18 // https://github.com/getsentry/sentry-javascript/issues/7697 const unsupportedNodeVersion = (NODE_VERSION.major || 0) < 18; @@ -386,10 +386,19 @@ export class LocalVariables implements Integration { ); } - addGlobalEventProcessor(async event => this._addLocalVariables(event)); + this._shouldProcessEvent = true; } } + /** @inheritdoc */ + public processEvent(event: Event): Event { + if (this._shouldProcessEvent) { + return this._addLocalVariables(event); + } + + return event; + } + /** * Handle the pause event */ diff --git a/packages/node/test/integrations/localvariables.test.ts b/packages/node/test/integrations/localvariables.test.ts index 640a59e03ae4..6a09111370a8 100644 --- a/packages/node/test/integrations/localvariables.test.ts +++ b/packages/node/test/integrations/localvariables.test.ts @@ -2,7 +2,7 @@ import type { ClientOptions, EventProcessor } from '@sentry/types'; import type { LRUMap } from '@sentry/utils'; import type { Debugger, InspectorNotification } from 'inspector'; -import { defaultStackParser } from '../../src'; +import { NodeClient, defaultStackParser } from '../../src'; import type { DebugSession, FrameVariables } from '../../src/integrations/localvariables'; import { LocalVariables, createCallbackList, createRateLimiter } from '../../src/integrations/localvariables'; import { NODE_VERSION } from '../../src/nodeVersion'; @@ -52,7 +52,6 @@ class MockDebugSession implements DebugSession { interface LocalVariablesPrivate { _cachedFrames: LRUMap; - _setup(addGlobalEventProcessor: (callback: EventProcessor) => void, clientOptions: ClientOptions): void; } const exceptionEvent = { @@ -154,8 +153,6 @@ const exceptionEvent100Frames = { describeIf((NODE_VERSION.major || 0) >= 18)('LocalVariables', () => { it('Adds local variables to stack frames', async () => { - expect.assertions(7); - const session = new MockDebugSession({ '-6224981551105448869.1.2': { name: 'tim' }, '-6224981551105448869.1.6': { arr: [1, 2, 3] }, @@ -164,13 +161,14 @@ describeIf((NODE_VERSION.major || 0) >= 18)('LocalVariables', () => { const options = getDefaultNodeClientOptions({ stackParser: defaultStackParser, includeLocalVariables: true, + integrations: [localVariables], }); - let eventProcessor: EventProcessor | undefined; + const client = new NodeClient(options); + client.setupIntegrations(true); - (localVariables as unknown as LocalVariablesPrivate)._setup(callback => { - eventProcessor = callback; - }, options); + const eventProcessors = client['_eventProcessors']; + const eventProcessor = eventProcessors.find(processor => processor.id === 'LocalVariables'); expect(eventProcessor).toBeDefined(); @@ -189,7 +187,7 @@ describeIf((NODE_VERSION.major || 0) >= 18)('LocalVariables', () => { { function: 'one', vars: { arr: [1, 2, 3] } }, ]); - const event = await eventProcessor?.( + const event = await eventProcessor!( { event_id: '9cbf882ade9a415986632ac4e16918eb', platform: 'node', @@ -249,22 +247,16 @@ describeIf((NODE_VERSION.major || 0) >= 18)('LocalVariables', () => { }); it('Only considers the first 5 frames', async () => { - expect.assertions(4); - const session = new MockDebugSession({}); const localVariables = new LocalVariables({}, session); const options = getDefaultNodeClientOptions({ stackParser: defaultStackParser, includeLocalVariables: true, + integrations: [localVariables], }); - let eventProcessor: EventProcessor | undefined; - - (localVariables as unknown as LocalVariablesPrivate)._setup(callback => { - eventProcessor = callback; - }, options); - - expect(eventProcessor).toBeDefined(); + const client = new NodeClient(options); + client.setupIntegrations(true); await session.runPause(exceptionEvent100Frames); @@ -280,16 +272,16 @@ describeIf((NODE_VERSION.major || 0) >= 18)('LocalVariables', () => { }); it('Should not lookup variables for non-exception reasons', async () => { - expect.assertions(1); - const session = new MockDebugSession({}, { getLocalVariables: true }); const localVariables = new LocalVariables({}, session); const options = getDefaultNodeClientOptions({ stackParser: defaultStackParser, includeLocalVariables: true, + integrations: [localVariables], }); - (localVariables as unknown as LocalVariablesPrivate)._setup(_ => {}, options); + const client = new NodeClient(options); + client.setupIntegrations(true); const nonExceptionEvent = { method: exceptionEvent.method, @@ -302,43 +294,41 @@ describeIf((NODE_VERSION.major || 0) >= 18)('LocalVariables', () => { }); it('Should not initialize when disabled', async () => { - expect.assertions(1); - const session = new MockDebugSession({}, { configureAndConnect: true }); const localVariables = new LocalVariables({}, session); const options = getDefaultNodeClientOptions({ stackParser: defaultStackParser, + integrations: [localVariables], }); - let eventProcessor: EventProcessor | undefined; + const client = new NodeClient(options); + client.setupIntegrations(true); - (localVariables as unknown as LocalVariablesPrivate)._setup(callback => { - eventProcessor = callback; - }, options); + const eventProcessors = client['_eventProcessors']; + const eventProcessor = eventProcessors.find(processor => processor.id === 'LocalVariables'); - expect(eventProcessor).toBeUndefined(); + expect(eventProcessor).toBeDefined(); + expect(localVariables['_shouldProcessEvent']).toBe(false); }); it('Should not initialize when inspector not loaded', async () => { - expect.assertions(1); - const localVariables = new LocalVariables({}, undefined); const options = getDefaultNodeClientOptions({ stackParser: defaultStackParser, + integrations: [localVariables], }); - let eventProcessor: EventProcessor | undefined; + const client = new NodeClient(options); + client.setupIntegrations(true); - (localVariables as unknown as LocalVariablesPrivate)._setup(callback => { - eventProcessor = callback; - }, options); + const eventProcessors = client['_eventProcessors']; + const eventProcessor = eventProcessors.find(processor => processor.id === 'LocalVariables'); - expect(eventProcessor).toBeUndefined(); + expect(eventProcessor).toBeDefined(); + expect(localVariables['_shouldProcessEvent']).toBe(false); }); it('Should cache identical uncaught exception events', async () => { - expect.assertions(1); - const session = new MockDebugSession({ '-6224981551105448869.1.2': { name: 'tim' }, '-6224981551105448869.1.6': { arr: [1, 2, 3] }, @@ -347,9 +337,11 @@ describeIf((NODE_VERSION.major || 0) >= 18)('LocalVariables', () => { const options = getDefaultNodeClientOptions({ stackParser: defaultStackParser, includeLocalVariables: true, + integrations: [localVariables], }); - (localVariables as unknown as LocalVariablesPrivate)._setup(_ => {}, options); + const client = new NodeClient(options); + client.setupIntegrations(true); await session.runPause(exceptionEvent); await session.runPause(exceptionEvent);