From 395f5b6b7e0f9860a86efda05248edf9e042f25e Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 24 Jan 2024 10:42:36 +0100 Subject: [PATCH 1/5] fix(node): LocalVariables integration should use setupOnce --- .../local-variables/local-variables-sync.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/node/src/integrations/local-variables/local-variables-sync.ts b/packages/node/src/integrations/local-variables/local-variables-sync.ts index bfe255002975..b55756e1b4e0 100644 --- a/packages/node/src/integrations/local-variables/local-variables-sync.ts +++ b/packages/node/src/integrations/local-variables/local-variables-sync.ts @@ -1,5 +1,5 @@ /* eslint-disable max-lines */ -import { convertIntegrationFnToClass } from '@sentry/core'; +import { convertIntegrationFnToClass, getClient } from '@sentry/core'; import type { Event, Exception, Integration, IntegrationClass, IntegrationFn, StackParser } from '@sentry/types'; import { LRUMap, logger } from '@sentry/utils'; import type { Debugger, InspectorNotification, Runtime, Session } from 'inspector'; @@ -327,11 +327,11 @@ const localVariablesSyncIntegration = (( return { name: INTEGRATION_NAME, // TODO v8: Remove this - setupOnce() {}, // eslint-disable-line @typescript-eslint/no-empty-function - setup(client: NodeClient) { - const clientOptions = client.getOptions(); + setupOnce() { + const client = getClient(); + const clientOptions = client?.getOptions(); - if (session && clientOptions.includeLocalVariables) { + if (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 < 18; From 68a1df55024189e235270d7ec6352cd731334899 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 24 Jan 2024 10:43:52 +0100 Subject: [PATCH 2/5] remove todo --- .../src/integrations/local-variables/local-variables-sync.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/node/src/integrations/local-variables/local-variables-sync.ts b/packages/node/src/integrations/local-variables/local-variables-sync.ts index b55756e1b4e0..93f3b61b10f2 100644 --- a/packages/node/src/integrations/local-variables/local-variables-sync.ts +++ b/packages/node/src/integrations/local-variables/local-variables-sync.ts @@ -326,7 +326,6 @@ const localVariablesSyncIntegration = (( return { name: INTEGRATION_NAME, - // TODO v8: Remove this setupOnce() { const client = getClient(); const clientOptions = client?.getOptions(); From 740d28070517958fba26561cda634fdc6c419043 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 24 Jan 2024 12:49:35 +0100 Subject: [PATCH 3/5] Fix tests --- packages/node/test/integrations/localvariables.test.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/node/test/integrations/localvariables.test.ts b/packages/node/test/integrations/localvariables.test.ts index b0fc6094e6a5..697f7a13c635 100644 --- a/packages/node/test/integrations/localvariables.test.ts +++ b/packages/node/test/integrations/localvariables.test.ts @@ -1,5 +1,6 @@ import type { Debugger, InspectorNotification } from 'inspector'; +import * as SentryCore from '@sentry/core'; import { NodeClient, defaultStackParser } from '../../src'; import { createRateLimiter } from '../../src/integrations/local-variables/common'; import type { FrameVariables } from '../../src/integrations/local-variables/common'; @@ -166,6 +167,7 @@ describeIf(NODE_VERSION.major >= 18)('LocalVariables', () => { }); const client = new NodeClient(options); + jest.spyOn(SentryCore, 'getClient').mockImplementation(() => client); client.addIntegration(localVariables); const eventProcessors = client['_eventProcessors']; @@ -257,6 +259,7 @@ describeIf(NODE_VERSION.major >= 18)('LocalVariables', () => { }); const client = new NodeClient(options); + jest.spyOn(SentryCore, 'getClient').mockImplementation(() => client); client.addIntegration(localVariables); await session.runPause(exceptionEvent100Frames); @@ -282,6 +285,7 @@ describeIf(NODE_VERSION.major >= 18)('LocalVariables', () => { }); const client = new NodeClient(options); + jest.spyOn(SentryCore, 'getClient').mockImplementation(() => client); client.addIntegration(localVariables); const nonExceptionEvent = { @@ -303,6 +307,7 @@ describeIf(NODE_VERSION.major >= 18)('LocalVariables', () => { }); const client = new NodeClient(options); + jest.spyOn(SentryCore, 'getClient').mockImplementation(() => client); client.addIntegration(localVariables); const eventProcessors = client['_eventProcessors']; @@ -319,6 +324,7 @@ describeIf(NODE_VERSION.major >= 18)('LocalVariables', () => { }); const client = new NodeClient(options); + jest.spyOn(SentryCore, 'getClient').mockImplementation(() => client); client.addIntegration(localVariables); const eventProcessors = client['_eventProcessors']; @@ -340,6 +346,7 @@ describeIf(NODE_VERSION.major >= 18)('LocalVariables', () => { }); const client = new NodeClient(options); + jest.spyOn(SentryCore, 'getClient').mockImplementation(() => client); client.addIntegration(localVariables); await session.runPause(exceptionEvent); From ff0e264496e8481cb2495092574e627eccc5d28d Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 24 Jan 2024 13:27:35 +0100 Subject: [PATCH 4/5] Remove unit tests --- .../test/integrations/localvariables.test.ts | 491 ------------------ 1 file changed, 491 deletions(-) delete mode 100644 packages/node/test/integrations/localvariables.test.ts diff --git a/packages/node/test/integrations/localvariables.test.ts b/packages/node/test/integrations/localvariables.test.ts deleted file mode 100644 index 697f7a13c635..000000000000 --- a/packages/node/test/integrations/localvariables.test.ts +++ /dev/null @@ -1,491 +0,0 @@ -import type { Debugger, InspectorNotification } from 'inspector'; - -import * as SentryCore from '@sentry/core'; -import { NodeClient, defaultStackParser } from '../../src'; -import { createRateLimiter } from '../../src/integrations/local-variables/common'; -import type { FrameVariables } from '../../src/integrations/local-variables/common'; -import type { DebugSession } from '../../src/integrations/local-variables/local-variables-sync'; -import { LocalVariablesSync, createCallbackList } from '../../src/integrations/local-variables/local-variables-sync'; -import { NODE_VERSION } from '../../src/nodeVersion'; -import { getDefaultNodeClientOptions } from '../../test/helper/node-client-options'; - -jest.setTimeout(20_000); - -const describeIf = (condition: boolean) => (condition ? describe : describe.skip); - -interface ThrowOn { - configureAndConnect?: boolean; - getLocalVariables?: boolean; -} - -class MockDebugSession implements DebugSession { - private _onPause?: (message: InspectorNotification, callback: () => void) => void; - - constructor(private readonly _vars: Record>, private readonly _throwOn?: ThrowOn) {} - - public configureAndConnect( - onPause: (message: InspectorNotification, callback: () => void) => void, - _captureAll: boolean, - ): void { - if (this._throwOn?.configureAndConnect) { - throw new Error('configureAndConnect should not be called'); - } - - this._onPause = onPause; - } - - public setPauseOnExceptions(_: boolean): void {} - - public getLocalVariables(objectId: string, callback: (vars: Record) => void): void { - if (this._throwOn?.getLocalVariables) { - throw new Error('getLocalVariables should not be called'); - } - - callback(this._vars[objectId]); - } - - public runPause(message: InspectorNotification): Promise { - return new Promise(resolve => { - this._onPause?.(message, resolve); - }); - } -} - -interface LocalVariablesPrivate { - _getCachedFramesCount(): number; - _getFirstCachedFrame(): FrameVariables[] | undefined; -} - -const exceptionEvent = { - method: 'Debugger.paused', - params: { - reason: 'exception', - data: { - description: - 'Error: Some error\n' + - ' at two (/dist/javascript/src/main.js:23:9)\n' + - ' at one (/dist/javascript/src/main.js:19:3)\n' + - ' at Timeout._onTimeout (/dist/javascript/src/main.js:40:5)\n' + - ' at listOnTimeout (node:internal/timers:559:17)\n' + - ' at process.processTimers (node:internal/timers:502:7)', - }, - callFrames: [ - { - callFrameId: '-6224981551105448869.1.0', - functionName: 'two', - location: { scriptId: '134', lineNumber: 22 }, - url: '', - scopeChain: [ - { - type: 'local', - object: { - type: 'object', - className: 'Object', - objectId: '-6224981551105448869.1.2', - }, - name: 'two', - }, - ], - this: { - type: 'object', - className: 'global', - }, - }, - { - callFrameId: '-6224981551105448869.1.1', - functionName: 'one', - location: { scriptId: '134', lineNumber: 18 }, - url: '', - scopeChain: [ - { - type: 'local', - object: { - type: 'object', - className: 'Object', - objectId: '-6224981551105448869.1.6', - }, - name: 'one', - }, - ], - this: { - type: 'object', - className: 'global', - }, - }, - ], - }, -}; - -const exceptionEvent100Frames = { - method: 'Debugger.paused', - params: { - reason: 'exception', - data: { - description: - 'Error: Some error\n' + - ' at two (/dist/javascript/src/main.js:23:9)\n' + - ' at one (/dist/javascript/src/main.js:19:3)\n' + - ' at Timeout._onTimeout (/dist/javascript/src/main.js:40:5)\n' + - ' at listOnTimeout (node:internal/timers:559:17)\n' + - ' at process.processTimers (node:internal/timers:502:7)', - }, - callFrames: new Array(100).fill({ - callFrameId: '-6224981551105448869.1.0', - functionName: 'two', - location: { scriptId: '134', lineNumber: 22 }, - url: '', - scopeChain: [ - { - type: 'local', - object: { - type: 'object', - className: 'Object', - objectId: '-6224981551105448869.1.2', - }, - name: 'two', - }, - ], - this: { - type: 'object', - className: 'global', - }, - }), - }, -}; - -describeIf(NODE_VERSION.major >= 18)('LocalVariables', () => { - it('Adds local variables to stack frames', async () => { - const session = new MockDebugSession({ - '-6224981551105448869.1.2': { name: 'tim' }, - '-6224981551105448869.1.6': { arr: [1, 2, 3] }, - }); - const localVariables = new LocalVariablesSync({}, session); - const options = getDefaultNodeClientOptions({ - stackParser: defaultStackParser, - includeLocalVariables: true, - integrations: [], - }); - - const client = new NodeClient(options); - jest.spyOn(SentryCore, 'getClient').mockImplementation(() => client); - client.addIntegration(localVariables); - - const eventProcessors = client['_eventProcessors']; - const eventProcessor = eventProcessors.find(processor => processor.id === 'LocalVariables'); - - expect(eventProcessor).toBeDefined(); - - await session.runPause(exceptionEvent); - - expect((localVariables as unknown as LocalVariablesPrivate)._getCachedFramesCount()).toBe(1); - - const frames = (localVariables as unknown as LocalVariablesPrivate)._getFirstCachedFrame(); - - expect(frames).toBeDefined(); - - const vars = frames as FrameVariables[]; - - expect(vars).toEqual([ - { function: 'two', vars: { name: 'tim' } }, - { function: 'one', vars: { arr: [1, 2, 3] } }, - ]); - - const event = await eventProcessor!( - { - event_id: '9cbf882ade9a415986632ac4e16918eb', - platform: 'node', - timestamp: 1671113680.306, - level: 'fatal', - exception: { - values: [ - { - type: 'Error', - value: 'Some error', - stacktrace: { - frames: [ - { - function: 'process.processTimers', - lineno: 502, - colno: 7, - in_app: false, - }, - { - function: 'listOnTimeout', - lineno: 559, - colno: 17, - in_app: false, - }, - { - function: 'Timeout._onTimeout', - lineno: 40, - colno: 5, - in_app: true, - }, - { - function: 'one', - lineno: 19, - colno: 3, - in_app: true, - }, - { - function: 'two', - lineno: 23, - colno: 9, - in_app: true, - }, - ], - }, - mechanism: { type: 'generic', handled: true }, - }, - ], - }, - }, - {}, - ); - - expect(event?.exception?.values?.[0].stacktrace?.frames?.[3]?.vars).toEqual({ arr: [1, 2, 3] }); - expect(event?.exception?.values?.[0].stacktrace?.frames?.[4]?.vars).toEqual({ name: 'tim' }); - - expect((localVariables as unknown as LocalVariablesPrivate)._getCachedFramesCount()).toBe(0); - }); - - it('Only considers the first 5 frames', async () => { - const session = new MockDebugSession({}); - const localVariables = new LocalVariablesSync({}, session); - const options = getDefaultNodeClientOptions({ - stackParser: defaultStackParser, - includeLocalVariables: true, - integrations: [], - }); - - const client = new NodeClient(options); - jest.spyOn(SentryCore, 'getClient').mockImplementation(() => client); - client.addIntegration(localVariables); - - await session.runPause(exceptionEvent100Frames); - - expect((localVariables as unknown as LocalVariablesPrivate)._getCachedFramesCount()).toBe(1); - - const frames = (localVariables as unknown as LocalVariablesPrivate)._getFirstCachedFrame(); - - expect(frames).toBeDefined(); - - const vars = frames as FrameVariables[]; - - expect(vars.length).toEqual(5); - }); - - it('Should not lookup variables for non-exception reasons', async () => { - const session = new MockDebugSession({}, { getLocalVariables: true }); - const localVariables = new LocalVariablesSync({}, session); - const options = getDefaultNodeClientOptions({ - stackParser: defaultStackParser, - includeLocalVariables: true, - integrations: [], - }); - - const client = new NodeClient(options); - jest.spyOn(SentryCore, 'getClient').mockImplementation(() => client); - client.addIntegration(localVariables); - - const nonExceptionEvent = { - method: exceptionEvent.method, - params: { ...exceptionEvent.params, reason: 'non-exception-reason' }, - }; - - await session.runPause(nonExceptionEvent); - - expect((localVariables as unknown as LocalVariablesPrivate)._getCachedFramesCount()).toBe(0); - }); - - it('Should not initialize when disabled', async () => { - const session = new MockDebugSession({}, { configureAndConnect: true }); - const localVariables = new LocalVariablesSync({}, session); - const options = getDefaultNodeClientOptions({ - stackParser: defaultStackParser, - integrations: [], - }); - - const client = new NodeClient(options); - jest.spyOn(SentryCore, 'getClient').mockImplementation(() => client); - client.addIntegration(localVariables); - - const eventProcessors = client['_eventProcessors']; - const eventProcessor = eventProcessors.find(processor => processor.id === 'LocalVariables'); - - expect(eventProcessor).toBeDefined(); - }); - - it('Should not initialize when inspector not loaded', async () => { - const localVariables = new LocalVariablesSync({}, undefined); - const options = getDefaultNodeClientOptions({ - stackParser: defaultStackParser, - integrations: [], - }); - - const client = new NodeClient(options); - jest.spyOn(SentryCore, 'getClient').mockImplementation(() => client); - client.addIntegration(localVariables); - - const eventProcessors = client['_eventProcessors']; - const eventProcessor = eventProcessors.find(processor => processor.id === 'LocalVariables'); - - expect(eventProcessor).toBeDefined(); - }); - - it('Should cache identical uncaught exception events', async () => { - const session = new MockDebugSession({ - '-6224981551105448869.1.2': { name: 'tim' }, - '-6224981551105448869.1.6': { arr: [1, 2, 3] }, - }); - const localVariables = new LocalVariablesSync({}, session); - const options = getDefaultNodeClientOptions({ - stackParser: defaultStackParser, - includeLocalVariables: true, - integrations: [], - }); - - const client = new NodeClient(options); - jest.spyOn(SentryCore, 'getClient').mockImplementation(() => client); - client.addIntegration(localVariables); - - await session.runPause(exceptionEvent); - await session.runPause(exceptionEvent); - await session.runPause(exceptionEvent); - await session.runPause(exceptionEvent); - await session.runPause(exceptionEvent); - - expect((localVariables as unknown as LocalVariablesPrivate)._getCachedFramesCount()).toBe(1); - }); - - describe('createCallbackList', () => { - it('Should call callbacks in reverse order', done => { - const log: number[] = []; - - const { add, next } = createCallbackList(n => { - expect(log).toEqual([5, 4, 3, 2, 1]); - expect(n).toBe(15); - done(); - }); - - add(n => { - log.push(1); - next(n + 1); - }); - - add(n => { - log.push(2); - next(n + 1); - }); - - add(n => { - log.push(3); - next(n + 1); - }); - - add(n => { - log.push(4); - next(n + 1); - }); - - add(n => { - log.push(5); - next(n + 11); - }); - - next(0); - }); - - it('only calls complete once even if multiple next', done => { - const { add, next } = createCallbackList(n => { - expect(n).toBe(1); - done(); - }); - - add(n => { - next(n + 1); - // We dont actually do this in our code... - next(n + 1); - }); - - next(0); - }); - - it('calls completed if added closure throws', done => { - const { add, next } = createCallbackList(n => { - expect(n).toBe(10); - done(); - }); - - add(n => { - throw new Error('test'); - next(n + 1); - }); - - next(10); - }); - }); - - describe('rateLimiter', () => { - it('calls disable if exceeded', done => { - const increment = createRateLimiter( - 5, - () => {}, - () => { - done(); - }, - ); - - for (let i = 0; i < 7; i++) { - increment(); - } - }); - - it('does not call disable if not exceeded', done => { - const increment = createRateLimiter( - 5, - () => { - throw new Error('Should not be called'); - }, - () => { - throw new Error('Should not be called'); - }, - ); - - let count = 0; - - const timer = setInterval(() => { - for (let i = 0; i < 4; i++) { - increment(); - } - - count += 1; - - if (count >= 5) { - clearInterval(timer); - done(); - } - }, 1_000); - }); - - it('re-enables after timeout', done => { - let called = false; - - const increment = createRateLimiter( - 5, - () => { - expect(called).toEqual(true); - done(); - }, - () => { - expect(called).toEqual(false); - called = true; - }, - ); - - for (let i = 0; i < 10; i++) { - increment(); - } - }); - }); -}); From 4429f5d11fa9ba454ab434ab55c46b83d7158717 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 24 Jan 2024 13:40:25 +0100 Subject: [PATCH 5/5] Slim down unit tests --- .../test/integrations/localvariables.test.ts | 140 ++++++++++++++++++ 1 file changed, 140 insertions(+) create mode 100644 packages/node/test/integrations/localvariables.test.ts diff --git a/packages/node/test/integrations/localvariables.test.ts b/packages/node/test/integrations/localvariables.test.ts new file mode 100644 index 000000000000..abc1d241f842 --- /dev/null +++ b/packages/node/test/integrations/localvariables.test.ts @@ -0,0 +1,140 @@ +import { createRateLimiter } from '../../src/integrations/local-variables/common'; +import { createCallbackList } from '../../src/integrations/local-variables/local-variables-sync'; +import { NODE_VERSION } from '../../src/nodeVersion'; + +jest.setTimeout(20_000); + +const describeIf = (condition: boolean) => (condition ? describe : describe.skip); + +describeIf(NODE_VERSION.major >= 18)('LocalVariables', () => { + describe('createCallbackList', () => { + it('Should call callbacks in reverse order', done => { + const log: number[] = []; + + const { add, next } = createCallbackList(n => { + expect(log).toEqual([5, 4, 3, 2, 1]); + expect(n).toBe(15); + done(); + }); + + add(n => { + log.push(1); + next(n + 1); + }); + + add(n => { + log.push(2); + next(n + 1); + }); + + add(n => { + log.push(3); + next(n + 1); + }); + + add(n => { + log.push(4); + next(n + 1); + }); + + add(n => { + log.push(5); + next(n + 11); + }); + + next(0); + }); + + it('only calls complete once even if multiple next', done => { + const { add, next } = createCallbackList(n => { + expect(n).toBe(1); + done(); + }); + + add(n => { + next(n + 1); + // We dont actually do this in our code... + next(n + 1); + }); + + next(0); + }); + + it('calls completed if added closure throws', done => { + const { add, next } = createCallbackList(n => { + expect(n).toBe(10); + done(); + }); + + add(n => { + throw new Error('test'); + next(n + 1); + }); + + next(10); + }); + }); + + describe('rateLimiter', () => { + it('calls disable if exceeded', done => { + const increment = createRateLimiter( + 5, + () => {}, + () => { + done(); + }, + ); + + for (let i = 0; i < 7; i++) { + increment(); + } + }); + + it('does not call disable if not exceeded', done => { + const increment = createRateLimiter( + 5, + () => { + throw new Error('Should not be called'); + }, + () => { + throw new Error('Should not be called'); + }, + ); + + let count = 0; + + const timer = setInterval(() => { + for (let i = 0; i < 4; i++) { + increment(); + } + + count += 1; + + if (count >= 5) { + clearInterval(timer); + done(); + } + }, 1_000); + }); + + it('re-enables after timeout', done => { + let called = false; + + const increment = createRateLimiter( + 5, + () => { + expect(called).toEqual(true); + done(); + }, + () => { + expect(called).toEqual(false); + called = true; + }, + ); + + for (let i = 0; i < 10; i++) { + increment(); + } + }); + }); +});