-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(node): Skip context lines lookup when source maps are enabled #17405
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import type { IntegrationFn } from '@sentry/core'; | ||
import { defineIntegration } from '@sentry/core'; | ||
import { contextLinesIntegration as originalContextLinesIntegration } from '@sentry/node'; | ||
|
||
const _contextLinesIntegration = ((options?: { frameContextLines?: number }) => { | ||
return originalContextLinesIntegration({ | ||
...options, | ||
// Nest.js always enabled this, without an easy way for us to detect this | ||
// so we just enable it by default | ||
// see: https://github.com/nestjs/nest-cli/blob/f5dbb573df1fe103139026a36b6d0efe65e8e985/actions/start.action.ts#L220 | ||
hasSourceMaps: true, | ||
}); | ||
}) satisfies IntegrationFn; | ||
|
||
/** | ||
* Capture the lines before and after the frame's context. | ||
* | ||
* A Nest-specific variant of the node-core contextLineIntegration. | ||
* This has source maps enabled by default, as Nest.js enables this under the hood. | ||
*/ | ||
export const contextLinesIntegration = defineIntegration(_contextLinesIntegration); |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -22,6 +22,14 @@ interface ContextLinesOptions { | |||||
* Set to 0 to disable loading and inclusion of source files. | ||||||
**/ | ||||||
frameContextLines?: number; | ||||||
|
||||||
/** | ||||||
* If error stacktraces are already sourcemapped. | ||||||
* In this case, we can skip the sourcemap lookup for certain cases. | ||||||
* This is the case e.g. if the node process is run with `node --enable-source-maps`. | ||||||
* If this is undefined, the SDK tries to infer it from the environment. | ||||||
*/ | ||||||
hasSourceMaps?: boolean; | ||||||
} | ||||||
|
||||||
/** | ||||||
|
@@ -62,6 +70,19 @@ function shouldSkipContextLinesForFile(path: string): boolean { | |||||
return false; | ||||||
} | ||||||
|
||||||
/** | ||||||
* Skip frames that we determine to already have been sourcemapped. | ||||||
*/ | ||||||
function shouldSkipContextLinesThatAreAlreadySourceMapped(path: string, frame: StackFrame): boolean { | ||||||
// For non-in-app frames, we skip context lines when we are reasonably certain that the path is already sourcemapped. | ||||||
// For now, we only consider .ts files because they can never appear otherwise in a stackframe, if not already sourcemapped. | ||||||
if (frame.in_app === false && path.endsWith('.ts')) { | ||||||
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. super-l: We could probably match against
Suggested change
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. yeah good point, likely a good idea 👍 |
||||||
return true; | ||||||
} | ||||||
|
||||||
return false; | ||||||
} | ||||||
|
||||||
/** | ||||||
* Determines if we should skip contextlines based off the max lineno and colno values. | ||||||
*/ | ||||||
|
@@ -164,10 +185,11 @@ function getContextLinesFromFile(path: string, ranges: ReadlineRange[], output: | |||||
|
||||||
// We use this inside Promise.all, so we need to resolve the promise even if there is an error | ||||||
// to prevent Promise.all from short circuiting the rest. | ||||||
function onStreamError(e: Error): void { | ||||||
function onStreamError(): void { | ||||||
// Mark file path as failed to read and prevent multiple read attempts. | ||||||
LRU_FILE_CONTENTS_FS_READ_FAILED.set(path, 1); | ||||||
DEBUG_BUILD && debug.error(`Failed to read file: ${path}. Error: ${e}`); | ||||||
DEBUG_BUILD && | ||||||
debug.warn(`ContextLines: Failed to read file: ${path}. Skipping context line extraction for this file.`); | ||||||
lineReaded.close(); | ||||||
lineReaded.removeAllListeners(); | ||||||
destroyStreamAndResolve(); | ||||||
|
@@ -215,7 +237,10 @@ function getContextLinesFromFile(path: string, ranges: ReadlineRange[], output: | |||||
* failing reads from happening. | ||||||
*/ | ||||||
/* eslint-disable complexity */ | ||||||
async function addSourceContext(event: Event, contextLines: number): Promise<Event> { | ||||||
async function addSourceContext( | ||||||
event: Event, | ||||||
{ contextLines, hasSourceMaps }: { contextLines: number; hasSourceMaps: boolean }, | ||||||
): Promise<Event> { | ||||||
// keep a lookup map of which files we've already enqueued to read, | ||||||
// so we don't enqueue the same file multiple times which would cause multiple i/o reads | ||||||
const filesToLines: Record<string, number[]> = {}; | ||||||
|
@@ -242,6 +267,10 @@ async function addSourceContext(event: Event, contextLines: number): Promise<Eve | |||||
continue; | ||||||
} | ||||||
|
||||||
if (hasSourceMaps && shouldSkipContextLinesThatAreAlreadySourceMapped(filename, frame)) { | ||||||
continue; | ||||||
} | ||||||
|
||||||
const filesToLinesOutput = filesToLines[filename]; | ||||||
if (!filesToLinesOutput) filesToLines[filename] = []; | ||||||
// @ts-expect-error this is defined above | ||||||
|
@@ -399,11 +428,12 @@ function makeContextRange(line: number, linecontext: number): [start: number, en | |||||
/** Exported only for tests, as a type-safe variant. */ | ||||||
export const _contextLinesIntegration = ((options: ContextLinesOptions = {}) => { | ||||||
const contextLines = options.frameContextLines !== undefined ? options.frameContextLines : DEFAULT_LINES_OF_CONTEXT; | ||||||
const hasSourceMaps = options.hasSourceMaps ?? inferHasSourceMaps(); | ||||||
|
||||||
return { | ||||||
name: INTEGRATION_NAME, | ||||||
processEvent(event) { | ||||||
return addSourceContext(event, contextLines); | ||||||
return addSourceContext(event, { contextLines, hasSourceMaps }); | ||||||
}, | ||||||
}; | ||||||
}) satisfies IntegrationFn; | ||||||
|
@@ -412,3 +442,10 @@ export const _contextLinesIntegration = ((options: ContextLinesOptions = {}) => | |||||
* Capture the lines before and after the frame's context. | ||||||
*/ | ||||||
export const contextLinesIntegration = defineIntegration(_contextLinesIntegration); | ||||||
|
||||||
function inferHasSourceMaps(): boolean { | ||||||
return ( | ||||||
(process.env.NODE_OPTIONS?.includes('--enable-source-maps') || process.argv.includes('--enable-source-maps')) ?? | ||||||
false | ||||||
); | ||||||
} |
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.
m: I'm not sure if this is really always the case. I could imagine a deployment config where a nest app isn't started via the Nest CLI but directly via
node
.In which case, this probably doesn't work as we'd expect it 🤔 Might make sense to check official NestJS deployment guides to be safe.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.
Looks like the CLI isn't meant to start a Nest app in prod: https://docs.nestjs.com/deployment#node_envproduction
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.
hmmm, good point! 😬
I haven't found a reliably way to figure out if this flag is set in Nest.js 😢 I can't seem to get a hold of these args/env vars in nest, probably due to how this is spawned (I suppose?)... I'll think about it some more. Generally speaking, this should mostly be not too dangerous either way, as the presence of
.ts
file more or less indicates that this is most likely source mapped already anyhow, but it is less safe 🤔