Skip to content
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.

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

feat(node): Rate limit local variables for caught exceptions and enable captureAllExceptions by default #9102

Merged
merged 9 commits into from
Sep 28, 2023
94 changes: 88 additions & 6 deletions packages/node/src/integrations/localvariables.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable max-lines */
import type { Event, EventProcessor, Exception, Hub, Integration, StackFrame, StackParser } from '@sentry/types';
import { logger } from '@sentry/utils';
import type { Debugger, InspectorNotification, Runtime, Session } from 'inspector';
Expand All @@ -11,6 +12,8 @@ type OnPauseEvent = InspectorNotification<Debugger.PausedEventDataType>;
export interface DebugSession {
/** Configures and connects to the debug session */
configureAndConnect(onPause: (message: OnPauseEvent, complete: () => void) => void, captureAll: boolean): void;
/** Updates which kind of exceptions to capture */
setPauseOnExceptions(captureAll: boolean): void;
/** Gets local variables for an objectId */
getLocalVariables(objectId: string, callback: (vars: Variables) => void): void;
}
Expand All @@ -19,6 +22,52 @@ type Next<T> = (result: T) => void;
type Add<T> = (fn: Next<T>) => void;
type CallbackWrapper<T> = { add: Add<T>; next: Next<T> };

type RateLimitIncrement = () => void;

/**
* Creates a rate limiter
* @param maxPerSecond Maximum number of calls per second
* @param enable Callback to enable capture
* @param disable Callback to disable capture
* @returns A function to call to increment the rate limiter count
*/
export function createRateLimiter(
maxPerSecond: number,
enable: () => void,
disable: (seconds: number) => void,
): RateLimitIncrement {
let count = 0;
let retrySeconds = 5;
let disabledTimeout = 0;

setInterval(() => {
AbhiPrasad marked this conversation as resolved.
Show resolved Hide resolved
if (disabledTimeout === 0) {
if (count > maxPerSecond) {
retrySeconds *= 2;
disable(retrySeconds);

// Cap at one day
if (retrySeconds > 86400) {
retrySeconds = 86400;
}
disabledTimeout = retrySeconds;
}
} else {
disabledTimeout -= 1;

if (disabledTimeout === 0) {
enable();
}
}

count = 0;
}, 1_000).unref();

return () => {
count += 1;
};
}

/** Creates a container for callbacks to be called sequentially */
export function createCallbackList<T>(complete: Next<T>): CallbackWrapper<T> {
// A collection of callbacks to be executed last to first
Expand Down Expand Up @@ -103,6 +152,10 @@ class AsyncSession implements DebugSession {
this._session.post('Debugger.setPauseOnExceptions', { state: captureAll ? 'all' : 'uncaught' });
}

public setPauseOnExceptions(captureAll: boolean): void {
this._session.post('Debugger.setPauseOnExceptions', { state: captureAll ? 'all' : 'uncaught' });
}

/** @inheritdoc */
public getLocalVariables(objectId: string, complete: (vars: Variables) => void): void {
this._getProperties(objectId, props => {
Expand Down Expand Up @@ -245,15 +298,21 @@ export interface FrameVariables {
vars?: Variables;
}

/** There are no options yet. This allows them to be added later without breaking changes */
// eslint-disable-next-line @typescript-eslint/no-empty-interface
interface Options {
/**
* Capture local variables for both handled and unhandled exceptions
* Capture local variables for both caught and uncaught exceptions
*
* - When false, only uncaught exceptions will have local variables
* - When true, both caught and uncaught exceptions will have local variables.
* - When a number, both caught and uncaught exceptions will have local variables until that many exceptions have been
* captured per second.
*
* Default: true (50 exceptions per second)
*
* Default: false - Only captures local variables for uncaught exceptions
* Capturing local variables for all exceptions can be expensive and is rate-limited. Once the rate limit is reached,
* local variables will only be captured for uncaught exceptions until a timeout has been reached.
*/
captureAllExceptions?: boolean;
captureAllExceptions?: boolean | number;
timfish marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand All @@ -265,6 +324,7 @@ export class LocalVariables implements Integration {
public readonly name: string = LocalVariables.id;

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

public constructor(
private readonly _options: Options = {},
Expand Down Expand Up @@ -293,12 +353,32 @@ export class LocalVariables implements Integration {
return;
}

const captureAll = this._options.captureAllExceptions !== false;

this._session.configureAndConnect(
(ev, complete) =>
this._handlePaused(clientOptions.stackParser, ev as InspectorNotification<PausedExceptionEvent>, complete),
!!this._options.captureAllExceptions,
captureAll,
);

if (captureAll) {
const max = typeof this._options.captureAllExceptions === 'number' ? this._options.captureAllExceptions : 50;

this._rateLimiter = createRateLimiter(
max,
() => {
logger.log('Local variables rate limit lifted.');
this._session?.setPauseOnExceptions(true);
},
seconds => {
logger.log(
`Local variables rate limit exceeded. Disabling capturing of caught exceptions for ${seconds} seconds.`,
);
this._session?.setPauseOnExceptions(false);
},
);
}

addGlobalEventProcessor(async event => this._addLocalVariables(event));
}
}
Expand All @@ -316,6 +396,8 @@ export class LocalVariables implements Integration {
return;
}

this._rateLimiter?.();

// data.description contains the original error.stack
const exceptionHash = hashFromStack(stackParser, data?.description);

Expand Down
69 changes: 68 additions & 1 deletion packages/node/test/integrations/localvariables.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@ import type { LRUMap } from 'lru_map';

import { defaultStackParser } from '../../src';
import type { DebugSession, FrameVariables } from '../../src/integrations/localvariables';
import { createCallbackList, LocalVariables } from '../../src/integrations/localvariables';
import { createCallbackList, createRateLimiter, LocalVariables } from '../../src/integrations/localvariables';
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 {
Expand All @@ -31,6 +33,8 @@ class MockDebugSession implements DebugSession {
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');
Expand Down Expand Up @@ -431,4 +435,67 @@ describeIf((NODE_VERSION.major || 0) >= 18)('LocalVariables', () => {
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();
}
});
});
});