Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(node): Remove ambiguity and race conditions when matching local variables to exceptions#13501
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
d6d04627ac938f46a057e87d5d75efacd0a306dca7069d6f2ac543cfefbad10efdbd45File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| 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 type { Event, Exception, IntegrationFn, StackFrame, StackParser } from '@sentry/types'; | ||
| import { LRUMap, logger } from '@sentry/utils'; | ||
| import { NODE_MAJOR } from '../../nodeVersion'; | ||
| @@ -12,7 +12,29 @@ import type { | ||
| RateLimitIncrement, | ||
| Variables, | ||
| } from './common'; | ||
| import { createRateLimiter, functionNamesMatch, hashFrames, hashFromStack } from './common'; | ||
| import { createRateLimiter, functionNamesMatch } from './common'; | ||
| /** Creates a unique hash from stack frames */ | ||
| export function hashFrames(frames: StackFrame[] | undefined): string | undefined { | ||
| if (frames === undefined) { | ||
| return; | ||
| } | ||
| // Only hash the 10 most recent frames (ie. the last 10) | ||
| return frames.slice(-10).reduce((acc, frame) => `${acc},${frame.function},${frame.lineno},${frame.colno}`, ''); | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we should filter out system frames here, so basically either
CollaboratorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code is for the sync debugger has simply been copied from | ||
| } | ||
| /** | ||
| * We use the stack parser to create a unique hash from the exception stack trace | ||
| * This is used to lookup vars when the exception passes through the event processor | ||
| */ | ||
| export function hashFromStack(stackParser: StackParser, stack: string | undefined): string | undefined { | ||
| if (stack === undefined) { | ||
| return undefined; | ||
| } | ||
| return hashFrames(stackParser(stack, 1)); | ||
| } | ||
| type OnPauseEvent = InspectorNotification<Debugger.PausedEventDataType>; | ||
| export interface DebugSession { | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,12 @@ | ||
| import type { Debugger, InspectorNotification, Runtime } from 'node:inspector'; | ||
| import { Session } from 'node:inspector/promises'; | ||
| import { parentPort, workerData } from 'node:worker_threads'; | ||
| import type { StackParser } from '@sentry/types'; | ||
| import { createStackParser, nodeStackLineParser } from '@sentry/utils'; | ||
| import { createGetModuleFromFilename } from '../../utils/module'; | ||
| import { workerData } from 'node:worker_threads'; | ||
| import type { LocalVariablesWorkerArgs, PausedExceptionEvent, RateLimitIncrement, Variables } from './common'; | ||
| import { createRateLimiter, hashFromStack } from './common'; | ||
| import { LOCAL_VARIABLES_KEY } from './common'; | ||
| import { createRateLimiter } from './common'; | ||
| const options: LocalVariablesWorkerArgs = workerData; | ||
| const stackParser = createStackParser(nodeStackLineParser(createGetModuleFromFilename(options.basePath))); | ||
| function log(...args: unknown[]): void { | ||
| if (options.debug) { | ||
| // eslint-disable-next-line no-console | ||
| @@ -88,19 +84,15 @@ let rateLimiter: RateLimitIncrement | undefined; | ||
| async function handlePaused( | ||
| session: Session, | ||
| stackParser: StackParser, | ||
| { reason, data, callFrames }: PausedExceptionEvent, | ||
| ): Promise<void> { | ||
| { reason, data: { objectId }, callFrames }: PausedExceptionEvent, | ||
| ): Promise<string | undefined> { | ||
| if (reason !== 'exception' && reason !== 'promiseRejection') { | ||
| return; | ||
| } | ||
| rateLimiter?.(); | ||
| // data.description contains the original error.stack | ||
| const exceptionHash = hashFromStack(stackParser, data?.description); | ||
| if (exceptionHash == undefined) { | ||
| if (objectId == undefined) { | ||
| return; | ||
| } | ||
| @@ -123,7 +115,15 @@ async function handlePaused( | ||
| } | ||
| } | ||
| parentPort?.postMessage({ exceptionHash, frames }); | ||
| // We write the local variables to a property on the error object. These can be read by the integration as the error | ||
| // event pass through the SDK event pipeline | ||
| await session.post('Runtime.callFunctionOn', { | ||
| functionDeclaration: `function() { this.${LOCAL_VARIABLES_KEY} = ${JSON.stringify(frames)}; }`, | ||
Uh oh!There was an error while loading. Please reload this page. Check warningCode scanning / CodeQL Improper code sanitization
Code construction depends on an [improperly sanitized value](1).
| ||
| silent: true, | ||
| objectId, | ||
| }); | ||
| return objectId; | ||
| } | ||
| async function startDebugger(): Promise<void> { | ||
| @@ -141,13 +141,23 @@ async function startDebugger(): Promise<void> { | ||
| session.on('Debugger.paused', (event: InspectorNotification<Debugger.PausedEventDataType>) => { | ||
| isPaused = true; | ||
| handlePaused(session, stackParser, event.params as PausedExceptionEvent).then( | ||
| () => { | ||
| handlePaused(session, event.params as PausedExceptionEvent).then( | ||
| async objectId => { | ||
| // After the pause work is complete, resume execution! | ||
| return isPaused ? session.post('Debugger.resume') : Promise.resolve(); | ||
| if (isPaused) { | ||
| await session.post('Debugger.resume'); | ||
| } | ||
| if (objectId) { | ||
| // The object must be released after the debugger has resumed or we get a memory leak. | ||
| // For node v20, setImmediate is enough here but for v22 a longer delay is required | ||
| setTimeout(async () => { | ||
| await session.post('Runtime.releaseObject', { objectId }); | ||
| }, 1_000); | ||
| } | ||
| }, | ||
| _ => { | ||
| // ignore | ||
| // ignore any errors | ||
| }, | ||
| ); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in both here and the worker, is it worth adding some kind of debug logging for when the sdk is in debug mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can validate it pretty easily by inspecting the event in
beforeSend, so I think we don't explicitly need logging for every event, but @timfish if you think it's a good idea go for it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also the discussion over whether some of our logging here should be behind
debug: trueor regularconsole.*. If we hit rate limiting maybe this should always be logged rather than only when debug is enabled? Users are unlikely to have debug logging enabled in production but it's likely useful to know when you're hitting rate limiting.