From e69226eed134b44a39f81b5f39739a3ad7e4ca82 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 10 Dec 2024 15:01:44 +0100 Subject: [PATCH 1/5] fix: Disable ANR and Local Variables if debugger is enabled via CLI args --- dev-packages/node-integration-tests/suites/anr/test.ts | 5 +++++ packages/node/src/integrations/anr/index.ts | 6 ++++++ .../integrations/local-variables/local-variables-async.ts | 5 +++++ .../integrations/local-variables/local-variables-sync.ts | 5 +++++ packages/node/src/utils/debug.ts | 6 ++++++ 5 files changed, 27 insertions(+) create mode 100644 packages/node/src/utils/debug.ts diff --git a/dev-packages/node-integration-tests/suites/anr/test.ts b/dev-packages/node-integration-tests/suites/anr/test.ts index fd15df4bd0b8..ea3ccc4ac80d 100644 --- a/dev-packages/node-integration-tests/suites/anr/test.ts +++ b/dev-packages/node-integration-tests/suites/anr/test.ts @@ -99,6 +99,11 @@ conditionalTest({ min: 16 })('should report ANR when event loop blocked', () => }); test('With --inspect', done => { + const ANR_EVENT_NO_STACKTRACE = { ...ANR_EVENT }; + if (ANR_EVENT_NO_STACKTRACE?.exception?.values?.[0]?.stacktrace) { + (ANR_EVENT_NO_STACKTRACE.exception.values[0].stacktrace as any) = {}; + } + createRunner(__dirname, 'basic.mjs') .withMockSentryServer() .withFlags('--inspect') diff --git a/packages/node/src/integrations/anr/index.ts b/packages/node/src/integrations/anr/index.ts index b93fcfd66612..699df292e1c2 100644 --- a/packages/node/src/integrations/anr/index.ts +++ b/packages/node/src/integrations/anr/index.ts @@ -15,6 +15,7 @@ import { import { NODE_VERSION } from '../../nodeVersion'; import type { NodeClient } from '../../sdk/client'; import type { AnrIntegrationOptions, WorkerStartData } from './common'; +import { isDebuggerEnabled } from '../../utils/debug'; const { isPromise } = types; @@ -73,6 +74,11 @@ const _anrIntegration = ((options: Partial = {}) => { let worker: Promise<() => void> | undefined; let client: NodeClient | undefined; + if (isDebuggerEnabled() && options.captureStackTrace) { + logger.warn('ANR captureStackTrace has been disabled because the debugger is enabled'); + options.captureStackTrace = false; + } + // Hookup the scope fetch function to the global object so that it can be called from the worker thread via the // debugger when it pauses const gbl = globalWithScopeFetchFn(); 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 89d92e46bd59..f64f3e624473 100644 --- a/packages/node/src/integrations/local-variables/local-variables-async.ts +++ b/packages/node/src/integrations/local-variables/local-variables-async.ts @@ -4,6 +4,7 @@ import { defineIntegration, logger } from '@sentry/core'; import type { NodeClient } from '../../sdk/client'; import type { FrameVariables, LocalVariablesIntegrationOptions, LocalVariablesWorkerArgs } from './common'; import { LOCAL_VARIABLES_KEY, functionNamesMatch } from './common'; +import { isDebuggerEnabled } from '../../utils/debug'; // This string is a placeholder that gets overwritten with the worker code. export const base64WorkerScript = '###LocalVariablesWorkerScript###'; @@ -108,6 +109,10 @@ export const localVariablesAsyncIntegration = defineIntegration((( return; } + if (isDebuggerEnabled()) { + logger.warn('Local variables capture has been disabled because the debugger is enabled'); + } + const options: LocalVariablesWorkerArgs = { ...integrationOptions, debug: logger.isEnabled(), 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 4de0fe8aa478..7316f6c943f3 100644 --- a/packages/node/src/integrations/local-variables/local-variables-sync.ts +++ b/packages/node/src/integrations/local-variables/local-variables-sync.ts @@ -11,6 +11,7 @@ import type { Variables, } from './common'; import { createRateLimiter, functionNamesMatch } from './common'; +import { isDebuggerEnabled } from '../../utils/debug'; /** Creates a unique hash from stack frames */ export function hashFrames(frames: StackFrame[] | undefined): string | undefined { @@ -306,6 +307,10 @@ const _localVariablesSyncIntegration = (( return; } + if (isDebuggerEnabled()) { + logger.warn('Local variables capture has been disabled because the debugger is enabled'); + } + AsyncSession.create(sessionOverride).then( session => { function handlePaused( diff --git a/packages/node/src/utils/debug.ts b/packages/node/src/utils/debug.ts new file mode 100644 index 000000000000..5b380f2809fc --- /dev/null +++ b/packages/node/src/utils/debug.ts @@ -0,0 +1,6 @@ +/** + * Has the debugger been enabled via the command line? + */ +export function isDebuggerEnabled(): boolean { + return process.execArgv.some(arg => arg.startsWith('--inspect')); +} From 33e642998206bd5e4c1fea839af980fedcaa135e Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 10 Dec 2024 15:05:54 +0100 Subject: [PATCH 2/5] Better test --- .../node-integration-tests/suites/anr/test.ts | 42 +++++++++++++++---- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/anr/test.ts b/dev-packages/node-integration-tests/suites/anr/test.ts index ea3ccc4ac80d..185e1084575f 100644 --- a/dev-packages/node-integration-tests/suites/anr/test.ts +++ b/dev-packages/node-integration-tests/suites/anr/test.ts @@ -52,6 +52,39 @@ const ANR_EVENT = { }, }; +const ANR_EVENT_WITHOUT_STACKTRACE = { + // Ensure we have context + contexts: { + trace: { + span_id: expect.stringMatching(/[a-f0-9]{16}/), + trace_id: expect.stringMatching(/[a-f0-9]{32}/), + }, + device: { + arch: expect.any(String), + }, + app: { + app_start_time: expect.any(String), + }, + os: { + name: expect.any(String), + }, + culture: { + timezone: expect.any(String), + }, + }, + // and an exception that is our ANR + exception: { + values: [ + { + type: 'ApplicationNotResponding', + value: 'Application Not Responding for at least 100 ms', + mechanism: { type: 'ANR' }, + stacktrace: {}, + }, + ], + }, +}; + const ANR_EVENT_WITH_SCOPE = { ...ANR_EVENT, user: { @@ -98,16 +131,11 @@ conditionalTest({ min: 16 })('should report ANR when event loop blocked', () => createRunner(__dirname, 'indefinite.mjs').withMockSentryServer().expect({ event: ANR_EVENT }).start(done); }); - test('With --inspect', done => { - const ANR_EVENT_NO_STACKTRACE = { ...ANR_EVENT }; - if (ANR_EVENT_NO_STACKTRACE?.exception?.values?.[0]?.stacktrace) { - (ANR_EVENT_NO_STACKTRACE.exception.values[0].stacktrace as any) = {}; - } - + test("With --inspect the debugger isn't used", done => { createRunner(__dirname, 'basic.mjs') .withMockSentryServer() .withFlags('--inspect') - .expect({ event: ANR_EVENT_WITH_SCOPE }) + .expect({ event: ANR_EVENT_WITHOUT_STACKTRACE }) .start(done); }); From c19c885336a328906027ad1ea8ea95095e755868 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 10 Dec 2024 15:15:51 +0100 Subject: [PATCH 3/5] Lint --- packages/node/src/integrations/anr/index.ts | 2 +- .../src/integrations/local-variables/local-variables-async.ts | 2 +- .../src/integrations/local-variables/local-variables-sync.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/node/src/integrations/anr/index.ts b/packages/node/src/integrations/anr/index.ts index 699df292e1c2..c07e4ade0eb2 100644 --- a/packages/node/src/integrations/anr/index.ts +++ b/packages/node/src/integrations/anr/index.ts @@ -14,8 +14,8 @@ import { } from '@sentry/core'; import { NODE_VERSION } from '../../nodeVersion'; import type { NodeClient } from '../../sdk/client'; -import type { AnrIntegrationOptions, WorkerStartData } from './common'; import { isDebuggerEnabled } from '../../utils/debug'; +import type { AnrIntegrationOptions, WorkerStartData } from './common'; const { isPromise } = types; 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 f64f3e624473..fcbbbc736999 100644 --- a/packages/node/src/integrations/local-variables/local-variables-async.ts +++ b/packages/node/src/integrations/local-variables/local-variables-async.ts @@ -2,9 +2,9 @@ import { Worker } from 'node:worker_threads'; import type { Event, EventHint, Exception, IntegrationFn } from '@sentry/core'; import { defineIntegration, logger } from '@sentry/core'; import type { NodeClient } from '../../sdk/client'; +import { isDebuggerEnabled } from '../../utils/debug'; import type { FrameVariables, LocalVariablesIntegrationOptions, LocalVariablesWorkerArgs } from './common'; import { LOCAL_VARIABLES_KEY, functionNamesMatch } from './common'; -import { isDebuggerEnabled } from '../../utils/debug'; // This string is a placeholder that gets overwritten with the worker code. export const base64WorkerScript = '###LocalVariablesWorkerScript###'; 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 7316f6c943f3..bb7ced35367f 100644 --- a/packages/node/src/integrations/local-variables/local-variables-sync.ts +++ b/packages/node/src/integrations/local-variables/local-variables-sync.ts @@ -3,6 +3,7 @@ import type { Event, Exception, IntegrationFn, StackFrame, StackParser } from '@ import { LRUMap, defineIntegration, getClient, logger } from '@sentry/core'; import { NODE_MAJOR } from '../../nodeVersion'; import type { NodeClient } from '../../sdk/client'; +import { isDebuggerEnabled } from '../../utils/debug'; import type { FrameVariables, LocalVariablesIntegrationOptions, @@ -11,7 +12,6 @@ import type { Variables, } from './common'; import { createRateLimiter, functionNamesMatch } from './common'; -import { isDebuggerEnabled } from '../../utils/debug'; /** Creates a unique hash from stack frames */ export function hashFrames(frames: StackFrame[] | undefined): string | undefined { From cc18077ed0a0ca9bd826218106bb2887cafdf13d Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 10 Dec 2024 15:48:33 +0100 Subject: [PATCH 4/5] Fix test! --- dev-packages/node-integration-tests/suites/anr/test.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/anr/test.ts b/dev-packages/node-integration-tests/suites/anr/test.ts index 185e1084575f..b1750b308d28 100644 --- a/dev-packages/node-integration-tests/suites/anr/test.ts +++ b/dev-packages/node-integration-tests/suites/anr/test.ts @@ -55,10 +55,6 @@ const ANR_EVENT = { const ANR_EVENT_WITHOUT_STACKTRACE = { // Ensure we have context contexts: { - trace: { - span_id: expect.stringMatching(/[a-f0-9]{16}/), - trace_id: expect.stringMatching(/[a-f0-9]{32}/), - }, device: { arch: expect.any(String), }, From 034be0053f6edb29b3028fe4bf343ab4ce3d5619 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 12 Dec 2024 10:05:21 +0100 Subject: [PATCH 5/5] Use `inspector.url()` --- packages/node/src/integrations/anr/index.ts | 12 ++++++------ .../local-variables/local-variables-async.ts | 7 ++++--- .../local-variables/local-variables-sync.ts | 7 ++++--- packages/node/src/utils/debug.ts | 18 +++++++++++++++--- 4 files changed, 29 insertions(+), 15 deletions(-) diff --git a/packages/node/src/integrations/anr/index.ts b/packages/node/src/integrations/anr/index.ts index c07e4ade0eb2..f0cef4d60831 100644 --- a/packages/node/src/integrations/anr/index.ts +++ b/packages/node/src/integrations/anr/index.ts @@ -74,11 +74,6 @@ const _anrIntegration = ((options: Partial = {}) => { let worker: Promise<() => void> | undefined; let client: NodeClient | undefined; - if (isDebuggerEnabled() && options.captureStackTrace) { - logger.warn('ANR captureStackTrace has been disabled because the debugger is enabled'); - options.captureStackTrace = false; - } - // Hookup the scope fetch function to the global object so that it can be called from the worker thread via the // debugger when it pauses const gbl = globalWithScopeFetchFn(); @@ -104,9 +99,14 @@ const _anrIntegration = ((options: Partial = {}) => { }); } }, - setup(initClient: NodeClient) { + async setup(initClient: NodeClient) { client = initClient; + if (options.captureStackTrace && (await isDebuggerEnabled())) { + logger.warn('ANR captureStackTrace has been disabled because the debugger was already enabled'); + options.captureStackTrace = false; + } + // setImmediate is used to ensure that all other integrations have had their setup called first. // This allows us to call into all integrations to fetch the full context setImmediate(() => this.startWorker()); 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 fcbbbc736999..e1e0ebadf755 100644 --- a/packages/node/src/integrations/local-variables/local-variables-async.ts +++ b/packages/node/src/integrations/local-variables/local-variables-async.ts @@ -102,15 +102,16 @@ export const localVariablesAsyncIntegration = defineIntegration((( return { name: 'LocalVariablesAsync', - setup(client: NodeClient) { + async setup(client: NodeClient) { const clientOptions = client.getOptions(); if (!clientOptions.includeLocalVariables) { return; } - if (isDebuggerEnabled()) { - logger.warn('Local variables capture has been disabled because the debugger is enabled'); + if (await isDebuggerEnabled()) { + logger.warn('Local variables capture has been disabled because the debugger was already enabled'); + return; } const options: LocalVariablesWorkerArgs = { 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 bb7ced35367f..3416dbf47347 100644 --- a/packages/node/src/integrations/local-variables/local-variables-sync.ts +++ b/packages/node/src/integrations/local-variables/local-variables-sync.ts @@ -290,7 +290,7 @@ const _localVariablesSyncIntegration = (( return { name: INTEGRATION_NAME, - setupOnce() { + async setupOnce() { const client = getClient(); const clientOptions = client?.getOptions(); @@ -307,8 +307,9 @@ const _localVariablesSyncIntegration = (( return; } - if (isDebuggerEnabled()) { - logger.warn('Local variables capture has been disabled because the debugger is enabled'); + if (await isDebuggerEnabled()) { + logger.warn('Local variables capture has been disabled because the debugger was already enabled'); + return; } AsyncSession.create(sessionOverride).then( diff --git a/packages/node/src/utils/debug.ts b/packages/node/src/utils/debug.ts index 5b380f2809fc..71df5e761230 100644 --- a/packages/node/src/utils/debug.ts +++ b/packages/node/src/utils/debug.ts @@ -1,6 +1,18 @@ +let cachedDebuggerEnabled: boolean | undefined; + /** - * Has the debugger been enabled via the command line? + * Was the debugger enabled when this function was first called? */ -export function isDebuggerEnabled(): boolean { - return process.execArgv.some(arg => arg.startsWith('--inspect')); +export async function isDebuggerEnabled(): Promise { + if (cachedDebuggerEnabled === undefined) { + try { + // Node can be built without inspector support + const inspector = await import('node:inspector'); + cachedDebuggerEnabled = !!inspector.url(); + } catch (_) { + cachedDebuggerEnabled = false; + } + } + + return cachedDebuggerEnabled; }