From 60378d458b86dc6e77c06583a0a3807738df8340 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 27 May 2024 12:02:14 +0200 Subject: [PATCH 1/3] fix: Only import `inspector` asynchronously --- packages/node/src/integrations/anr/index.ts | 2 +- packages/node/src/integrations/anr/worker.ts | 2 +- .../local-variables/local-variables-async.ts | 2 +- .../local-variables/local-variables-sync.ts | 215 +++++++++--------- 4 files changed, 109 insertions(+), 112 deletions(-) diff --git a/packages/node/src/integrations/anr/index.ts b/packages/node/src/integrations/anr/index.ts index 671aae2bad49..92a4078aa766 100644 --- a/packages/node/src/integrations/anr/index.ts +++ b/packages/node/src/integrations/anr/index.ts @@ -1,4 +1,3 @@ -import * as inspector from 'node:inspector'; import { Worker } from 'node:worker_threads'; import { defineIntegration, getCurrentScope, getGlobalScope, getIsolationScope, mergeScopeData } from '@sentry/core'; import type { Contexts, Event, EventHint, Integration, IntegrationFn, ScopeData } from '@sentry/types'; @@ -148,6 +147,7 @@ async function _startWorker( }; if (options.captureStackTrace) { + const inspector = await import('node:inspector'); if (!inspector.url()) { inspector.open(0); } diff --git a/packages/node/src/integrations/anr/worker.ts b/packages/node/src/integrations/anr/worker.ts index 6f701919c531..c6b1d654ec45 100644 --- a/packages/node/src/integrations/anr/worker.ts +++ b/packages/node/src/integrations/anr/worker.ts @@ -15,7 +15,7 @@ import { uuid4, watchdogTimer, } from '@sentry/utils'; -import { Session as InspectorSession } from 'inspector'; +import { Session as InspectorSession } from 'node:inspector'; import { makeNodeTransport } from '../../transports'; import { createGetModuleFromFilename } from '../../utils/module'; diff --git a/packages/node/src/integrations/local-variables/local-variables-async.ts b/packages/node/src/integrations/local-variables/local-variables-async.ts index 944ccca758d9..06056aae6a39 100644 --- a/packages/node/src/integrations/local-variables/local-variables-async.ts +++ b/packages/node/src/integrations/local-variables/local-variables-async.ts @@ -75,7 +75,7 @@ export const localVariablesAsyncIntegration = defineIntegration((( async function startInspector(): Promise { // We load inspector dynamically because on some platforms Node is built without inspector support - const inspector = await import('inspector'); + const inspector = await import('node:inspector'); if (!inspector.url()) { inspector.open(0); } diff --git a/packages/node/src/integrations/local-variables/local-variables-sync.ts b/packages/node/src/integrations/local-variables/local-variables-sync.ts index 66e8eca4d32f..3433fc069a41 100644 --- 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,4 @@ -import type { Debugger, InspectorNotification, Runtime } from 'node:inspector'; -import { Session } from 'node:inspector'; +import type { Debugger, InspectorNotification, Runtime, Session } from 'node:inspector'; import { defineIntegration, getClient } from '@sentry/core'; import type { Event, Exception, IntegrationFn, StackParser } from '@sentry/types'; import { LRUMap, logger } from '@sentry/utils'; @@ -75,11 +74,18 @@ export function createCallbackList(complete: Next): CallbackWrapper { * https://nodejs.org/docs/latest-v14.x/api/inspector.html */ class AsyncSession implements DebugSession { - private readonly _session: Session; - /** Throws if inspector API is not available */ - public constructor() { - this._session = new Session(); + private constructor(private readonly _session: Session) { + // + } + + public static async create(orDefault?: DebugSession | undefined): Promise { + if (orDefault) { + return orDefault; + } + + const inspector = await import('node:inspector'); + return new AsyncSession(new inspector.Session()); } /** @inheritdoc */ @@ -194,18 +200,6 @@ class AsyncSession implements DebugSession { } } -/** - * When using Vercel pkg, the inspector module is not available. - * https://github.com/getsentry/sentry-javascript/issues/6769 - */ -function tryNewAsyncSession(): AsyncSession | undefined { - try { - return new AsyncSession(); - } catch (e) { - return undefined; - } -} - const INTEGRATION_NAME = 'LocalVariables'; /** @@ -213,66 +207,12 @@ const INTEGRATION_NAME = 'LocalVariables'; */ const _localVariablesSyncIntegration = (( options: LocalVariablesIntegrationOptions = {}, - session: DebugSession | undefined = tryNewAsyncSession(), + sessionOverride?: DebugSession, ) => { const cachedFrames: LRUMap = new LRUMap(20); let rateLimiter: RateLimitIncrement | undefined; let shouldProcessEvent = false; - function handlePaused( - stackParser: StackParser, - { params: { reason, data, callFrames } }: InspectorNotification, - complete: () => void, - ): void { - if (reason !== 'exception' && reason !== 'promiseRejection') { - complete(); - return; - } - - rateLimiter?.(); - - // data.description contains the original error.stack - const exceptionHash = hashFromStack(stackParser, data?.description); - - if (exceptionHash == undefined) { - complete(); - return; - } - - const { add, next } = createCallbackList(frames => { - cachedFrames.set(exceptionHash, frames); - complete(); - }); - - // Because we're queuing up and making all these calls synchronously, we can potentially overflow the stack - // For this reason we only attempt to get local variables for the first 5 frames - for (let i = 0; i < Math.min(callFrames.length, 5); i++) { - const { scopeChain, functionName, this: obj } = callFrames[i]; - - const localScope = scopeChain.find(scope => scope.type === 'local'); - - // obj.className is undefined in ESM modules - const fn = obj.className === 'global' || !obj.className ? functionName : `${obj.className}.${functionName}`; - - if (localScope?.object.objectId === undefined) { - add(frames => { - frames[i] = { function: fn }; - next(frames); - }); - } else { - const id = localScope.object.objectId; - add(frames => - session?.getLocalVariables(id, vars => { - frames[i] = { function: fn, vars }; - next(frames); - }), - ); - } - } - - next([]); - } - function addLocalVariablesToException(exception: Exception): void { const hash = hashFrames(exception?.stacktrace?.frames); @@ -327,47 +267,104 @@ const _localVariablesSyncIntegration = (( return { name: INTEGRATION_NAME, setupOnce() { - const client = getClient(); - const clientOptions = client?.getOptions(); - - 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_MAJOR < 18; + // eslint-disable-next-line @typescript-eslint/no-floating-promises + AsyncSession.create(sessionOverride).then(session => { + function handlePaused( + stackParser: StackParser, + { params: { reason, data, callFrames } }: InspectorNotification, + complete: () => void, + ): void { + if (reason !== 'exception' && reason !== 'promiseRejection') { + complete(); + return; + } + + rateLimiter?.(); + + // data.description contains the original error.stack + const exceptionHash = hashFromStack(stackParser, data?.description); + + if (exceptionHash == undefined) { + complete(); + return; + } + + const { add, next } = createCallbackList(frames => { + cachedFrames.set(exceptionHash, frames); + complete(); + }); + + // Because we're queuing up and making all these calls synchronously, we can potentially overflow the stack + // For this reason we only attempt to get local variables for the first 5 frames + for (let i = 0; i < Math.min(callFrames.length, 5); i++) { + const { scopeChain, functionName, this: obj } = callFrames[i]; + + const localScope = scopeChain.find(scope => scope.type === 'local'); + + // obj.className is undefined in ESM modules + const fn = obj.className === 'global' || !obj.className ? functionName : `${obj.className}.${functionName}`; + + if (localScope?.object.objectId === undefined) { + add(frames => { + frames[i] = { function: fn }; + next(frames); + }); + } else { + const id = localScope.object.objectId; + add(frames => + session?.getLocalVariables(id, vars => { + frames[i] = { function: fn, vars }; + next(frames); + }), + ); + } + } - if (unsupportedNodeVersion) { - logger.log('The `LocalVariables` integration is only supported on Node >= v18.'); - return; + next([]); } - const captureAll = options.captureAllExceptions !== false; - - session.configureAndConnect( - (ev, complete) => - handlePaused(clientOptions.stackParser, ev as InspectorNotification, complete), - captureAll, - ); - - if (captureAll) { - const max = options.maxExceptionsPerSecond || 50; - - rateLimiter = createRateLimiter( - max, - () => { - logger.log('Local variables rate-limit lifted.'); - session?.setPauseOnExceptions(true); - }, - seconds => { - logger.log( - `Local variables rate-limit exceeded. Disabling capturing of caught exceptions for ${seconds} seconds.`, - ); - session?.setPauseOnExceptions(false); - }, + const client = getClient(); + const clientOptions = client?.getOptions(); + + 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_MAJOR < 18; + + if (unsupportedNodeVersion) { + logger.log('The `LocalVariables` integration is only supported on Node >= v18.'); + return; + } + + const captureAll = options.captureAllExceptions !== false; + + session.configureAndConnect( + (ev, complete) => + handlePaused(clientOptions.stackParser, ev as InspectorNotification, complete), + captureAll, ); - } - shouldProcessEvent = true; - } + if (captureAll) { + const max = options.maxExceptionsPerSecond || 50; + + rateLimiter = createRateLimiter( + max, + () => { + logger.log('Local variables rate-limit lifted.'); + session?.setPauseOnExceptions(true); + }, + seconds => { + logger.log( + `Local variables rate-limit exceeded. Disabling capturing of caught exceptions for ${seconds} seconds.`, + ); + session?.setPauseOnExceptions(false); + }, + ); + } + + shouldProcessEvent = true; + } + }); }, processEvent(event: Event): Event { if (shouldProcessEvent) { From 8d3bae007a2b8fc7800f453cbb1608877c10c897 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 27 May 2024 12:41:37 +0200 Subject: [PATCH 2/3] Don't start if its not enabled --- packages/node/src/integrations/anr/worker.ts | 2 +- .../local-variables/local-variables-sync.ts | 127 +++++++++--------- 2 files changed, 68 insertions(+), 61 deletions(-) diff --git a/packages/node/src/integrations/anr/worker.ts b/packages/node/src/integrations/anr/worker.ts index c6b1d654ec45..67532435a39e 100644 --- a/packages/node/src/integrations/anr/worker.ts +++ b/packages/node/src/integrations/anr/worker.ts @@ -1,3 +1,4 @@ +import { Session as InspectorSession } from 'node:inspector'; import { parentPort, workerData } from 'node:worker_threads'; import { applyScopeDataToEvent, @@ -15,7 +16,6 @@ import { uuid4, watchdogTimer, } from '@sentry/utils'; -import { Session as InspectorSession } from 'node:inspector'; import { makeNodeTransport } from '../../transports'; import { createGetModuleFromFilename } from '../../utils/module'; diff --git a/packages/node/src/integrations/local-variables/local-variables-sync.ts b/packages/node/src/integrations/local-variables/local-variables-sync.ts index 3433fc069a41..9616e534625a 100644 --- a/packages/node/src/integrations/local-variables/local-variables-sync.ts +++ b/packages/node/src/integrations/local-variables/local-variables-sync.ts @@ -267,73 +267,77 @@ const _localVariablesSyncIntegration = (( return { name: INTEGRATION_NAME, setupOnce() { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - AsyncSession.create(sessionOverride).then(session => { - function handlePaused( - stackParser: StackParser, - { params: { reason, data, callFrames } }: InspectorNotification, - complete: () => void, - ): void { - if (reason !== 'exception' && reason !== 'promiseRejection') { - complete(); - return; - } + const client = getClient(); + const clientOptions = client?.getOptions(); - rateLimiter?.(); + if (!clientOptions?.includeLocalVariables) { + return; + } - // data.description contains the original error.stack - const exceptionHash = hashFromStack(stackParser, data?.description); + // Only setup this integration if the Node version is >= v18 + // https://github.com/getsentry/sentry-javascript/issues/7697 + const unsupportedNodeVersion = NODE_MAJOR < 18; - if (exceptionHash == undefined) { - complete(); - return; - } + if (unsupportedNodeVersion) { + logger.log('The `LocalVariables` integration is only supported on Node >= v18.'); + return; + } - const { add, next } = createCallbackList(frames => { - cachedFrames.set(exceptionHash, frames); - complete(); - }); - - // Because we're queuing up and making all these calls synchronously, we can potentially overflow the stack - // For this reason we only attempt to get local variables for the first 5 frames - for (let i = 0; i < Math.min(callFrames.length, 5); i++) { - const { scopeChain, functionName, this: obj } = callFrames[i]; - - const localScope = scopeChain.find(scope => scope.type === 'local'); - - // obj.className is undefined in ESM modules - const fn = obj.className === 'global' || !obj.className ? functionName : `${obj.className}.${functionName}`; - - if (localScope?.object.objectId === undefined) { - add(frames => { - frames[i] = { function: fn }; - next(frames); - }); - } else { - const id = localScope.object.objectId; - add(frames => - session?.getLocalVariables(id, vars => { - frames[i] = { function: fn, vars }; - next(frames); - }), - ); + AsyncSession.create(sessionOverride).then( + session => { + function handlePaused( + stackParser: StackParser, + { params: { reason, data, callFrames } }: InspectorNotification, + complete: () => void, + ): void { + if (reason !== 'exception' && reason !== 'promiseRejection') { + complete(); + return; } - } - next([]); - } + rateLimiter?.(); - const client = getClient(); - const clientOptions = client?.getOptions(); + // data.description contains the original error.stack + const exceptionHash = hashFromStack(stackParser, data?.description); + + if (exceptionHash == undefined) { + complete(); + return; + } - 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_MAJOR < 18; + const { add, next } = createCallbackList(frames => { + cachedFrames.set(exceptionHash, frames); + complete(); + }); - if (unsupportedNodeVersion) { - logger.log('The `LocalVariables` integration is only supported on Node >= v18.'); - return; + // Because we're queuing up and making all these calls synchronously, we can potentially overflow the stack + // For this reason we only attempt to get local variables for the first 5 frames + for (let i = 0; i < Math.min(callFrames.length, 5); i++) { + const { scopeChain, functionName, this: obj } = callFrames[i]; + + const localScope = scopeChain.find(scope => scope.type === 'local'); + + // obj.className is undefined in ESM modules + const fn = + obj.className === 'global' || !obj.className ? functionName : `${obj.className}.${functionName}`; + + if (localScope?.object.objectId === undefined) { + add(frames => { + frames[i] = { function: fn }; + next(frames); + }); + } else { + const id = localScope.object.objectId; + add(frames => + session?.getLocalVariables(id, vars => { + frames[i] = { function: fn, vars }; + next(frames); + }), + ); + } + } + + next([]); } const captureAll = options.captureAllExceptions !== false; @@ -363,8 +367,11 @@ const _localVariablesSyncIntegration = (( } shouldProcessEvent = true; - } - }); + }, + error => { + logger.log('The `LocalVariables` integration failed to start.', error); + }, + ); }, processEvent(event: Event): Event { if (shouldProcessEvent) { From 197d9a5114ae4ec9776e81cf1507024e0206f999 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 27 May 2024 13:20:46 +0200 Subject: [PATCH 3/3] Add test for inspector import --- .../public-api/LocalVariables/deny-inspector.mjs | 13 +++++++++++++ .../suites/public-api/LocalVariables/test.ts | 8 ++++++++ 2 files changed, 21 insertions(+) create mode 100644 dev-packages/node-integration-tests/suites/public-api/LocalVariables/deny-inspector.mjs diff --git a/dev-packages/node-integration-tests/suites/public-api/LocalVariables/deny-inspector.mjs b/dev-packages/node-integration-tests/suites/public-api/LocalVariables/deny-inspector.mjs new file mode 100644 index 000000000000..99323e91f0bc --- /dev/null +++ b/dev-packages/node-integration-tests/suites/public-api/LocalVariables/deny-inspector.mjs @@ -0,0 +1,13 @@ +import * as Sentry from '@sentry/node'; +import Hook from 'import-in-the-middle'; + +Hook((_, name) => { + if (name === 'inspector') { + throw new Error('No inspector!'); + } + if (name === 'node:inspector') { + throw new Error('No inspector!'); + } +}); + +Sentry.init({}); diff --git a/dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts b/dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts index 61b9fc3064a4..0ad4ddad7c5a 100644 --- a/dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts +++ b/dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts @@ -76,6 +76,14 @@ conditionalTest({ min: 18 })('LocalVariables integration', () => { .start(done); }); + test('Should not import inspector when not in use', done => { + createRunner(__dirname, 'deny-inspector.mjs') + .withFlags('--import=@sentry/node/import') + .ensureNoErrorOutput() + .ignore('session') + .start(done); + }); + test('Includes local variables for caught exceptions when enabled', done => { createRunner(__dirname, 'local-variables-caught.js') .ignore('session')