Skip to content

Commit

Permalink
ref(node): Refactor LocalVariables integration to avoid setupOnce (#…
Browse files Browse the repository at this point in the history 
…9897)

Slowly getting rid of `getCurrentHub()`...
  • Loading branch information
@mydea
mydea authored Dec 19, 2023
1 parent 91a6b4e commit c9552a4
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 50 deletions.
4 changes: 2 additions & 2 deletions packages/core/src/baseclient.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,14 +115,14 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
/** 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<string, Function[]>;

private _eventProcessors: EventProcessor[];

/**
* Initializes this client instance.
*
Expand Down
29 changes: 19 additions & 10 deletions packages/node/src/integrations/localvariables.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, unknown>;
type OnPauseEvent = InspectorNotification<Debugger.PausedEventDataType>;
Expand Down Expand Up @@ -332,6 +332,7 @@ export class LocalVariables implements Integration {

private readonly _cachedFrames: LRUMap<string, FrameVariables[]> = new LRUMap(20);
private _rateLimiter: RateLimitIncrement | undefined;
private _shouldProcessEvent = false;

public constructor(
private readonly _options: Options = {},
Expand All @@ -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;
Expand Down Expand Up @@ -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
*/
Expand Down
68 changes: 30 additions & 38 deletions packages/node/test/integrations/localvariables.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -52,7 +52,6 @@ class MockDebugSession implements DebugSession {

interface LocalVariablesPrivate {
_cachedFrames: LRUMap<string, FrameVariables[]>;
_setup(addGlobalEventProcessor: (callback: EventProcessor) => void, clientOptions: ClientOptions): void;
}

const exceptionEvent = {
Expand Down Expand Up @@ -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] },
Expand All @@ -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();

Expand All @@ -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',
Expand Down Expand Up @@ -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);

Expand All @@ -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,
Expand All @@ -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] },
Expand All @@ -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);
Expand Down

0 comments on commit c9552a4

Please sign in to comment.