Skip to content

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions packages/nestjs/src/integrations/contextlines.ts
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,
Copy link
Member

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.

Copy link
Member

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

Copy link
Member Author

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 🤔

});
}) 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);
8 changes: 7 additions & 1 deletion packages/nestjs/src/sdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
} from '@sentry/core';
import type { NodeClient, NodeOptions, Span } from '@sentry/node';
import { getDefaultIntegrations as getDefaultNodeIntegrations, init as nodeInit } from '@sentry/node';
import { contextLinesIntegration } from './integrations/contextlines';
import { nestIntegration } from './integrations/nest';

/**
Expand Down Expand Up @@ -34,7 +35,12 @@ export function init(options: NodeOptions | undefined = {}): NodeClient | undefi

/** Get the default integrations for the NestJS SDK. */
export function getDefaultIntegrations(options: NodeOptions): Integration[] | undefined {
return [nestIntegration(), ...getDefaultNodeIntegrations(options)];
return [
nestIntegration(),
...getDefaultNodeIntegrations(options).filter(i => i.name !== 'ContextLines'),
// Custom variant for Nest.js
contextLinesIntegration(),
];
}

function addNestSpanAttributes(span: Span): void {
Expand Down
45 changes: 41 additions & 4 deletions packages/node-core/src/integrations/contextlines.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super-l: We could probably match against /.(c|m)?tsx?/ to cover some more TS file variations. Happy to leave this up to you though

Suggested change
if (frame.in_app === false && path.endsWith('.ts')) {
if (frame.in_app === false && path.endsWith('.ts')) {

Copy link
Member Author

Choose a reason for hiding this comment

The 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.
*/
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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[]> = {};
Expand All @@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -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
);
}
110 changes: 110 additions & 0 deletions packages/node-core/test/integrations/contextlines.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -232,4 +232,114 @@ describe('ContextLines', () => {
expect(readStreamSpy).not.toHaveBeenCalled();
});
});

describe('hasSourceMaps', () => {
let _argv: string[];
let _env: any;

beforeEach(() => {
_argv = process.argv;
_env = process.env;
});

afterEach(() => {
process.argv = _argv;
process.env = _env;
});

test.each([
[undefined, 'file:///var/task/hasSourceMaps/index1a.ts', true],
[true, 'file:///var/task/hasSourceMaps/index1b.ts', true],
[false, 'file:///var/task/hasSourceMaps/index1c.ts', false],
[undefined, 'file:///var/task/hasSourceMaps/index1d.js', true],
[true, 'file:///var/task/hasSourceMaps/index1e.js', true],
[false, 'file:///var/task/hasSourceMaps/index1f.js', true],
])('handles in_app=%s, filename=%s', async (in_app, filename, hasBeenCalled) => {
contextLines = _contextLinesIntegration({ hasSourceMaps: true });
const readStreamSpy = vi.spyOn(fs, 'createReadStream');

const frames: StackFrame[] = [
{
colno: 1,
filename,
lineno: 1,
function: 'fxn1',
in_app,
},
];

await addContext(frames);

expect(readStreamSpy).toHaveBeenCalledTimes(hasBeenCalled ? 1 : 0);
});

test('infer hasSourceMaps from NODE_OPTIONS', async () => {
process.env = {
..._env,
NODE_OPTIONS: '--enable-source-maps --other-option',
};

contextLines = _contextLinesIntegration();
const readStreamSpy = vi.spyOn(fs, 'createReadStream');

const frames: StackFrame[] = [
{
colno: 1,
filename: 'file:///var/task/hasSourceMaps/index-infer1.ts',
lineno: 1,
function: 'fxn1',
in_app: false,
},
];

await addContext(frames);

expect(readStreamSpy).toHaveBeenCalledTimes(0);
});

test('infer hasSourceMaps from process.argv', async () => {
process.argv = [..._argv, '--enable-source-maps', '--other-option'];

contextLines = _contextLinesIntegration();
const readStreamSpy = vi.spyOn(fs, 'createReadStream');

const frames: StackFrame[] = [
{
colno: 1,
filename: 'file:///var/task/hasSourceMaps/index-infer2.ts',
lineno: 1,
function: 'fxn1',
in_app: false,
},
];

await addContext(frames);

expect(readStreamSpy).toHaveBeenCalledTimes(0);
});

test('does not infer hasSourceMaps if hasSourceMaps is set', async () => {
process.env = {
..._env,
NODE_OPTIONS: '--enable-source-maps --other-option',
};

contextLines = _contextLinesIntegration({ hasSourceMaps: false });
const readStreamSpy = vi.spyOn(fs, 'createReadStream');

const frames: StackFrame[] = [
{
colno: 1,
filename: 'file:///var/task/hasSourceMaps/index-infer3.ts',
lineno: 1,
function: 'fxn1',
in_app: false,
},
];

await addContext(frames);

expect(readStreamSpy).toHaveBeenCalledTimes(1);
});
});
});
5 changes: 5 additions & 0 deletions packages/node-core/test/sdk/init.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,11 @@ describe('init()', () => {
});

describe('validateOpenTelemetrySetup', () => {
beforeEach(() => {
// just swallowing these
vi.spyOn(debug, 'log').mockImplementation(() => {});
});

afterEach(() => {
global.__SENTRY__ = {};
cleanupOtel();
Expand Down
Loading