Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 24, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
395f5b6
fix(node): LocalVariables integration should use setupOnce
timfish Jan 24, 2024
68a1df5
remove todo
timfish Jan 24, 2024
740d280
Fix tests
timfish Jan 24, 2024
ff0e264
Remove unit tests
timfish Jan 24, 2024
4429f5d
Slim down unit tests
timfish Jan 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions packages/node/src/integrations/local-variables/local-variables-sync.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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;
Expand Down
346 changes: 1 addition & 345 deletions packages/node/test/integrations/localvariables.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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[] = [];
Expand Down