From 78314dcf685dd3139de09974ff63e24190ca33d1 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Sun, 24 Sep 2023 22:52:03 +0100 Subject: [PATCH 1/7] feat(node): Add rate limiting to uncaught exception capturing --- .../node/src/integrations/localvariables.ts | 76 ++++++++++++++++-- .../test/integrations/localvariables.test.ts | 77 ++++++++++++++++++- 2 files changed, 146 insertions(+), 7 deletions(-) diff --git a/packages/node/src/integrations/localvariables.ts b/packages/node/src/integrations/localvariables.ts index f98011d94832..262b28b54c2e 100644 --- a/packages/node/src/integrations/localvariables.ts +++ b/packages/node/src/integrations/localvariables.ts @@ -11,6 +11,8 @@ type OnPauseEvent = InspectorNotification; 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; } @@ -19,6 +21,43 @@ type Next = (result: T) => void; type Add = (fn: Next) => void; type CallbackWrapper = { add: Add; next: Next }; +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: () => void): RateLimitIncrement { + let count = 0; + let retrySeconds = 2; + let disabledTimeout = 0; + + setInterval(() => { + if (disabledTimeout === 0) { + if (count > maxPerSecond) { + disable(); + retrySeconds *= 2; + 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(complete: Next): CallbackWrapper { // A collection of callbacks to be executed last to first @@ -103,6 +142,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 => { @@ -245,15 +288,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 * - * Default: false - Only captures local variables for 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) + * + * 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; } /** @@ -265,6 +314,7 @@ export class LocalVariables implements Integration { public readonly name: string = LocalVariables.id; private readonly _cachedFrames: LRUMap = new LRUMap(20); + private _rateLimiter: RateLimitIncrement | undefined; public constructor( private readonly _options: Options = {}, @@ -293,12 +343,24 @@ export class LocalVariables implements Integration { return; } + const captureAll = this._options.captureAllExceptions !== false; + this._session.configureAndConnect( (ev, complete) => this._handlePaused(clientOptions.stackParser, ev as InspectorNotification, complete), - !!this._options.captureAllExceptions, + captureAll, ); + if (captureAll) { + const max = typeof this._options.captureAllExceptions === 'number' ? this._options.captureAllExceptions : 50; + + this._rateLimiter = createRateLimiter( + max, + () => this._session?.setPauseOnExceptions(true), + () => this._session?.setPauseOnExceptions(false), + ); + } + addGlobalEventProcessor(async event => this._addLocalVariables(event)); } } @@ -316,6 +378,8 @@ export class LocalVariables implements Integration { return; } + this._rateLimiter?.(); + // data.description contains the original error.stack const exceptionHash = hashFromStack(stackParser, data?.description); diff --git a/packages/node/test/integrations/localvariables.test.ts b/packages/node/test/integrations/localvariables.test.ts index 15f62f305231..169ffa2a4175 100644 --- a/packages/node/test/integrations/localvariables.test.ts +++ b/packages/node/test/integrations/localvariables.test.ts @@ -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, LocalVariables, createRateLimiter } from '../../src/integrations/localvariables'; import { NODE_VERSION } from '../../src/nodeVersion'; import { getDefaultNodeClientOptions } from '../../test/helper/node-client-options'; +jest.setTimeout(10_000); + const describeIf = (condition: boolean) => (condition ? describe : describe.skip); interface ThrowOn { @@ -31,6 +33,8 @@ class MockDebugSession implements DebugSession { 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'); @@ -431,4 +435,75 @@ describeIf((NODE_VERSION.major || 0) >= 18)('LocalVariables', () => { next(10); }); }); + + describe('rateLimiter', () => { + it('calls disable if exceeded', done => { + const increment = createRateLimiter( + 5, + () => {}, + () => { + done(); + }, + ); + + increment(); + increment(); + increment(); + increment(); + increment(); + 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(() => { + // only 4 per second + increment(); + increment(); + increment(); + 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; + }, + ); + + increment(); + increment(); + increment(); + increment(); + increment(); + increment(); + }); + }); }); From 9a4a07ccbeb21a5ff75c9a3fe4187677c8e8d090 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 25 Sep 2023 11:29:41 +0100 Subject: [PATCH 2/7] cap at 1 day --- packages/node/src/integrations/localvariables.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/node/src/integrations/localvariables.ts b/packages/node/src/integrations/localvariables.ts index 262b28b54c2e..e4c34fcd1950 100644 --- a/packages/node/src/integrations/localvariables.ts +++ b/packages/node/src/integrations/localvariables.ts @@ -32,7 +32,7 @@ type RateLimitIncrement = () => void; */ export function createRateLimiter(maxPerSecond: number, enable: () => void, disable: () => void): RateLimitIncrement { let count = 0; - let retrySeconds = 2; + let retrySeconds = 5; let disabledTimeout = 0; setInterval(() => { @@ -40,6 +40,11 @@ export function createRateLimiter(maxPerSecond: number, enable: () => void, disa if (count > maxPerSecond) { disable(); retrySeconds *= 2; + + // Cap at one day + if (retrySeconds > 86400) { + retrySeconds = 86400; + } disabledTimeout = retrySeconds; } } else { From f59eab197d46ab444b711616b478620bd2fc4156 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 25 Sep 2023 11:51:57 +0100 Subject: [PATCH 3/7] Fix linting --- packages/node/test/integrations/localvariables.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node/test/integrations/localvariables.test.ts b/packages/node/test/integrations/localvariables.test.ts index 169ffa2a4175..4771caa2bd14 100644 --- a/packages/node/test/integrations/localvariables.test.ts +++ b/packages/node/test/integrations/localvariables.test.ts @@ -4,7 +4,7 @@ import type { LRUMap } from 'lru_map'; import { defaultStackParser } from '../../src'; import type { DebugSession, FrameVariables } from '../../src/integrations/localvariables'; -import { createCallbackList, LocalVariables, createRateLimiter } 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'; From b7a9e1c4516a8fbe251f932ac2f2437164bdc4fe Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 26 Sep 2023 23:55:34 +0200 Subject: [PATCH 4/7] Attempt to fix test --- .../test/integrations/localvariables.test.ts | 26 +++++++------------ 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/packages/node/test/integrations/localvariables.test.ts b/packages/node/test/integrations/localvariables.test.ts index 4771caa2bd14..8520d28a8ef8 100644 --- a/packages/node/test/integrations/localvariables.test.ts +++ b/packages/node/test/integrations/localvariables.test.ts @@ -446,12 +446,9 @@ describeIf((NODE_VERSION.major || 0) >= 18)('LocalVariables', () => { }, ); - increment(); - increment(); - increment(); - increment(); - increment(); - increment(); + for (let i = 0; i < 7; i++) { + increment(); + } }); it('does not call disable if not exceeded', done => { @@ -468,11 +465,9 @@ describeIf((NODE_VERSION.major || 0) >= 18)('LocalVariables', () => { let count = 0; const timer = setInterval(() => { - // only 4 per second - increment(); - increment(); - increment(); - increment(); + for (let i = 0; i < 4; i++) { + increment(); + } count += 1; @@ -498,12 +493,9 @@ describeIf((NODE_VERSION.major || 0) >= 18)('LocalVariables', () => { }, ); - increment(); - increment(); - increment(); - increment(); - increment(); - increment(); + for (let i = 0; i < 10; i++) { + increment(); + } }); }); }); From 1028932bba4c62c6efc6fc40e492f7a12ce47908 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 27 Sep 2023 00:00:26 +0200 Subject: [PATCH 5/7] Add logging for rate limit exceed/reset --- .../node/src/integrations/localvariables.ts | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/packages/node/src/integrations/localvariables.ts b/packages/node/src/integrations/localvariables.ts index e4c34fcd1950..24a19fa87216 100644 --- a/packages/node/src/integrations/localvariables.ts +++ b/packages/node/src/integrations/localvariables.ts @@ -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'; @@ -30,7 +31,11 @@ type RateLimitIncrement = () => void; * @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: () => void): RateLimitIncrement { +export function createRateLimiter( + maxPerSecond: number, + enable: () => void, + disable: (seconds: number) => void, +): RateLimitIncrement { let count = 0; let retrySeconds = 5; let disabledTimeout = 0; @@ -38,8 +43,8 @@ export function createRateLimiter(maxPerSecond: number, enable: () => void, disa setInterval(() => { if (disabledTimeout === 0) { if (count > maxPerSecond) { - disable(); retrySeconds *= 2; + disable(retrySeconds); // Cap at one day if (retrySeconds > 86400) { @@ -361,8 +366,16 @@ export class LocalVariables implements Integration { this._rateLimiter = createRateLimiter( max, - () => this._session?.setPauseOnExceptions(true), - () => this._session?.setPauseOnExceptions(false), + () => { + 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); + }, ); } From 6cff0e857a79033361e3dea7da9b0ac450a7192a Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 27 Sep 2023 00:19:57 +0200 Subject: [PATCH 6/7] extend timeout --- packages/node/test/integrations/localvariables.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node/test/integrations/localvariables.test.ts b/packages/node/test/integrations/localvariables.test.ts index 8520d28a8ef8..55e179c879e6 100644 --- a/packages/node/test/integrations/localvariables.test.ts +++ b/packages/node/test/integrations/localvariables.test.ts @@ -8,7 +8,7 @@ import { createCallbackList, createRateLimiter, LocalVariables } from '../../src import { NODE_VERSION } from '../../src/nodeVersion'; import { getDefaultNodeClientOptions } from '../../test/helper/node-client-options'; -jest.setTimeout(10_000); +jest.setTimeout(20_000); const describeIf = (condition: boolean) => (condition ? describe : describe.skip); From 3682078c00f22c4011ff67ee2a5097cf255ced88 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 28 Sep 2023 13:17:43 +0200 Subject: [PATCH 7/7] Changes from PR review --- .../node/src/integrations/localvariables.ts | 26 ++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/packages/node/src/integrations/localvariables.ts b/packages/node/src/integrations/localvariables.ts index 24a19fa87216..39f250c23778 100644 --- a/packages/node/src/integrations/localvariables.ts +++ b/packages/node/src/integrations/localvariables.ts @@ -304,19 +304,27 @@ interface Options { * * - 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) + * Defaults to `true`. * - * 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. + * Capturing local variables for all exceptions can be expensive since the debugger pauses for every throw to collect + * local variables. + * + * To reduce the likelihood of this feature impacting app performance or throughput, this feature 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; + /** + * Maximum number of exceptions to capture local variables for per second before rate limiting is triggered. */ - captureAllExceptions?: boolean | number; + maxExceptionsPerSecond?: number; } /** * Adds local variables to exception frames + * + * Default: 50 */ export class LocalVariables implements Integration { public static id: string = 'LocalVariables'; @@ -362,17 +370,17 @@ export class LocalVariables implements Integration { ); if (captureAll) { - const max = typeof this._options.captureAllExceptions === 'number' ? this._options.captureAllExceptions : 50; + const max = this._options.maxExceptionsPerSecond || 50; this._rateLimiter = createRateLimiter( max, () => { - logger.log('Local variables rate limit lifted.'); + 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.`, + `Local variables rate-limit exceeded. Disabling capturing of caught exceptions for ${seconds} seconds.`, ); this._session?.setPauseOnExceptions(false); },