fix(node): LocalVariables integration should use setupOnce (#10307)

timfish · web-flow · commit d471dcf6066d · 2024-01-24T09:09:18.000-05:00
Closes #10278 This integration incorrectly used `setup` which then caused issues when the core code was changed. It should have been using `setupOnce`. Unfortunately, `setupOnce` causes issues with the unit tests since it then relies on `getClient` to get the client instance for the client options. I've tried using jest to mock `getClient` but this doesn't work which is likely due to `installedIntegrations`. Because we already have [extensive integration tests for LocalVariables](https://github.com/getsentry/sentry-javascript/tree/develop/dev-packages/node-integration-tests/suites/public-api/LocalVariables) I have removed the unit tests because they are becoming a maintenance burden and actually test less than the integration tests. After v8, I plan to move Local variables lookup to the worker thread at which point we can revisit how these might be better tested in unit tests.
diff --git 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';
@@ -326,12 +326,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<NodeClient>();
+      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;
diff --git a/packages/node/test/integrations/localvariables.test.ts b/packages/node/test/integrations/localvariables.test.ts
@@ -1,356 +1,12 @@
-import type { Debugger, InspectorNotification } from 'inspector';
-
-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 { 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<Debugger.PausedEventDataType>, callback: () => void) => void;
-
-  constructor(private readonly _vars: Record<string, Record<string, unknown>>, private readonly _throwOn?: ThrowOn) {}
-
-  public configureAndConnect(
-    onPause: (message: InspectorNotification<Debugger.PausedEventDataType>, 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<string, unknown>) => void): void {
-    if (this._throwOn?.getLocalVariables) {
-      throw new Error('getLocalVariables should not be called');
-    }
-
-    callback(this._vars[objectId]);
-  }
-
-  public runPause(message: InspectorNotification<Debugger.PausedEventDataType>): Promise<void> {
-    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);
-    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);
-    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);
-    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);
-    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);
-    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);
-    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[] = [];
