Skip to content

ref(node): Refactor node stack parsing to use common parser #4612

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

Merged
merged 65 commits into from
Feb 24, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
65 commits
Select commit Hold shift + click to select a range
31935f0
node stackwalk for Electron
timfish Jun 20, 2021
781312e
Merge branch 'master' into feat/node-stackwalk-electron
timfish Jun 21, 2021
f143c10
Re-order params
timfish Jun 21, 2021
c890809
Merge branch 'master' into feat/node-stackwalk-electron
timfish Jun 21, 2021
8b30a9a
Move source loading to seperate file
timfish Jun 21, 2021
f1e3f31
First try
timfish Jun 22, 2021
6a03a2c
Improve logic
timfish Jun 22, 2021
7c164dd
Revert tslib change
timfish Jun 22, 2021
f362589
jsdoc
timfish Jun 22, 2021
da256ee
Merge branch 'master' into feat/separate-source-reading
timfish Jun 22, 2021
04db3fb
Merge branch 'master' into feat/separate-source-reading
timfish Jul 16, 2021
3324578
With integration
timfish Jul 20, 2021
443ea06
oops
timfish Jul 20, 2021
dcd287d
Merge branch 'master' into feat/separate-source-reading
timfish Aug 10, 2021
84155de
Make this a non-breaking change by keeping NodeOptions.frameContextLines
timfish Aug 10, 2021
285a643
Merge branch 'master' into feat/separate-source-reading
timfish Aug 24, 2021
6a8a48b
Merge remote-tracking branch 'upstream/master' into feat/separate-sou…
timfish Nov 3, 2021
c4f48ad
Merge remote-tracking branch 'upstream/master' into feat/separate-sou…
timfish Dec 1, 2021
7f5793a
Merge branch 'master' into feat/separate-source-reading
timfish Dec 1, 2021
ac4318e
Correctly handle zero lines of context
timfish Dec 1, 2021
82573f4
Merge remote-tracking branch 'origin/feat/separate-source-reading' in…
timfish Dec 1, 2021
86e2d4e
Make async
timfish Dec 1, 2021
2ff7efd
Merge branch 'master' into feat/separate-source-reading
timfish Dec 2, 2021
012eff3
Merge remote-tracking branch 'upstream/master' into feat/separate-sou…
timfish Jan 11, 2022
647999d
Merge remote-tracking branch 'upstream/master' into feat/separate-sou…
timfish Jan 21, 2022
2fe44fc
Merge remote-tracking branch 'upstream/master' into feat/separate-sou…
timfish Feb 8, 2022
af9b1d8
No longer a breaking change
timfish Feb 8, 2022
270e13e
Fix linting
timfish Feb 8, 2022
76d8998
Merge branch 'master' into feat/separate-source-reading
timfish Feb 13, 2022
ddb8406
Add docs and `@deprecated`
timfish Feb 14, 2022
83f859b
Disable warning for internal usage of deprecated field
timfish Feb 14, 2022
8bc20b0
Revert promise changes
timfish Feb 14, 2022
18c5780
Merge remote-tracking branch 'upstream/master' into feat/separate-sou…
timfish Feb 14, 2022
2266ac5
Minor improve
timfish Feb 15, 2022
bb37b6c
Merge remote-tracking branch 'upstream/master' into feat/separate-sou…
timfish Feb 16, 2022
03cb320
Fix nextjs test
timfish Feb 16, 2022
965d48f
revert
timfish Feb 16, 2022
e08bcbb
Fix nextjs test
timfish Feb 16, 2022
7cd79af
Merge branch 'feat/separate-source-reading' of https://github.com/tim…
timfish Feb 16, 2022
f278e8b
Code review changes!
timfish Feb 17, 2022
a7d61df
Merge remote-tracking branch 'upstream/master' into feat/separate-sou…
timfish Feb 17, 2022
069244f
Use common parser
timfish Feb 17, 2022
ef7d423
Abhi code review
timfish Feb 17, 2022
a172765
Revert promisify
timfish Feb 18, 2022
bd85ce9
Add TODO comment
timfish Feb 18, 2022
224090c
Merge branch 'feat/separate-source-reading' into node/refactor-stack-…
timfish Feb 21, 2022
e060c3c
no zero
timfish Feb 21, 2022
ba2400d
Only set stacktrace if frames are returned
timfish Feb 21, 2022
ab19826
Merge remote-tracking branch 'upstream/master' into node/refactor-sta…
timfish Feb 23, 2022
ae696b9
Merge remote-tracking branch 'upstream/master' into node/refactor-sta…
timfish Feb 23, 2022
e0ca922
Couple of minor improvements
timfish Feb 23, 2022
0ea2155
Doh
timfish Feb 23, 2022
2bc3bc9
More minor improvements
timfish Feb 23, 2022
6b57641
Allow the parser to mostly work when require is not available
timfish Feb 23, 2022
b54d7cf
chore: add issue forms (#4617)
vladanpaunovic Feb 23, 2022
b7966ce
Don't do the funky require thing
timfish Feb 23, 2022
81f7352
Merge remote-tracking branch 'upstream/master' into node/refactor-sta…
timfish Feb 23, 2022
73cbc09
webpack hates optional chaining?
timfish Feb 24, 2022
93e0293
Include comment about optional chaining
timfish Feb 24, 2022
386b80b
Remove unnecessary try catch
timfish Feb 24, 2022
e867d7c
Merge remote-tracking branch 'upstream/master' into node/refactor-sta…
timfish Feb 24, 2022
19dadbb
Fix bad merge
timfish Feb 24, 2022
c69db78
Rename stack parser
timfish Feb 24, 2022
2dc38e6
Re-add forked comments
timfish Feb 24, 2022
1379939
Fix the lineno
timfish Feb 24, 2022
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
Prev Previous commit
Next Next commit
Make async
  • Loading branch information
timfish committed Dec 2, 2021
commit 86e2d4eff99a01382f49c28f14edaa88d72c49ff
47 changes: 22 additions & 25 deletions packages/node/src/integrations/contextlines.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,19 @@
import { getCurrentHub } from '@sentry/core';
import { Event, EventProcessor, Integration } from '@sentry/types';
import { addContextToFrame } from '@sentry/utils';
import { readFileSync } from 'fs';
import { readFile } from 'fs';
import { LRUMap } from 'lru_map';

import { NodeClient } from '../client';

const FILE_CONTENT_CACHE = new LRUMap<string, string | null>(100);

function readTextFileAsync(path: string): Promise<string> {
return new Promise((resolve, reject) => {
readFile(path, 'utf8', (err, data) => {
if (err) reject(err);
else resolve(data);
});
});
}

/**
* Resets the file cache. Exists for testing purposes.
* @hidden
Expand All @@ -16,9 +22,9 @@ export function resetFileContentCache(): void {
FILE_CONTENT_CACHE.clear();
}

type ContextLinesOptions = {
interface ContextLinesOptions {
frameContextLines?: number;
};
}

/** Add node modules / packages to the event */
export class ContextLines implements Integration {
Expand All @@ -32,27 +38,18 @@ export class ContextLines implements Integration {
*/
public name: string = ContextLines.id;

private _linesOfContext?: number;

public constructor(options: ContextLinesOptions = {}) {
this._linesOfContext = options.frameContextLines;
}
public constructor(private readonly _options: ContextLinesOptions = {}) {}

/**
* @inheritDoc
*/
public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void): void {
const optLinesOfContext = getCurrentHub()
.getClient<NodeClient>()
?.getOptions()?.frameContextLines;

addGlobalEventProcessor(event => this.process(event, optLinesOfContext));
addGlobalEventProcessor(event => this.addToEvent(event));
}

/** Processes an event and adds context lines */
public process(event: Event, optLinesOfContext?: number): Event {
const contextLines =
optLinesOfContext != undefined ? optLinesOfContext : this._linesOfContext != undefined ? this._linesOfContext : 7;
public async addToEvent(event: Event): Promise<Event> {
const contextLines = this._options.frameContextLines != undefined ? this._options.frameContextLines : 7;

const frames = event.exception?.values?.[0].stacktrace?.frames;

Expand All @@ -65,7 +62,7 @@ export class ContextLines implements Integration {
}
}

const sourceFiles = readSourceFiles(filenames);
const sourceFiles = await readSourceFiles(filenames);

for (const frame of frames) {
if (frame.filename && sourceFiles[frame.filename]) {
Expand All @@ -91,7 +88,7 @@ export class ContextLines implements Integration {
*
* @param filenames Array of filepaths to read content from.
*/
function readSourceFiles(filenames: string[]): Record<string, string | null> {
async function readSourceFiles(filenames: string[]): Promise<Record<string, string | null>> {
// we're relying on filenames being de-duped already
if (!filenames.length) {
return {};
Expand All @@ -113,11 +110,11 @@ function readSourceFiles(filenames: string[]): Record<string, string | null> {
continue;
}

let content: string | null;
let content: string | null = null;
try {
content = readFileSync(filename, 'utf8');
} catch (_e) {
content = null;
content = await readTextFileAsync(filename);
} catch (_) {
//
}

FILE_CONTENT_CACHE.set(filename, content);
Expand Down
3 changes: 0 additions & 3 deletions packages/node/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,6 @@ export interface NodeOptions extends Options {
/** HTTPS proxy certificates path */
caCerts?: string;

/** Sets the number of context lines for each frame when loading a file. */
frameContextLines?: number;

/** Callback that is executed when a fatal global error occurs. */
onFatalError?(error: Error): void;
}
6 changes: 3 additions & 3 deletions packages/node/test/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,9 @@ describe('SentryNode', () => {
expect(event.exception).not.toBeUndefined();
expect(event.exception!.values![0]).not.toBeUndefined();
expect(event.exception!.values![0].stacktrace!).not.toBeUndefined();
expect(event.exception!.values![0].stacktrace!.frames![2]).not.toBeUndefined();
expect(Array.isArray(event.exception!.values![0].stacktrace!.frames![2].pre_context)).toBe(true);
expect(Array.isArray(event.exception!.values![0].stacktrace!.frames![2].post_context)).toBe(true);
expect(event.exception!.values![0].stacktrace!.frames![1]).not.toBeUndefined();
expect(event.exception!.values![0].stacktrace!.frames![1].pre_context).not.toBeUndefined();
expect(event.exception!.values![0].stacktrace!.frames![1].post_context).not.toBeUndefined();
expect(event.exception!.values![0].type).toBe('Error');
expect(event.exception!.values![0].value).toBe('test');
expect(event.exception!.values![0].stacktrace).toBeTruthy();
Expand Down
32 changes: 16 additions & 16 deletions packages/node/test/parsers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ describe('parsers.ts', () => {
let spy: jest.SpyInstance;
let contextLines: ContextLines;

function addContext(frames: StackFrame[]) {
contextLines.process({ exception: { values: [{ stacktrace: { frames } }] } });
async function addContext(frames: StackFrame[]): Promise<void> {
await contextLines.addToEvent({ exception: { values: [{ stacktrace: { frames } }] } });
}

beforeEach(() => {
spy = jest.spyOn(fs, 'readFileSync');
spy = jest.spyOn(fs, 'readFile');
frames = stacktrace.parse(new Error('test'));
contextLines = new ContextLines();
resetFileContentCache();
Expand All @@ -27,22 +27,22 @@ describe('parsers.ts', () => {
});

describe('lru file cache', () => {
test('parseStack with same file', () => {
test('parseStack with same file', async () => {
expect.assertions(1);

let mockCalls = 0;
let parsedFrames = Parsers.parseStack(frames);
addContext(parsedFrames);
await addContext(parsedFrames);

mockCalls = spy.mock.calls.length;
parsedFrames = Parsers.parseStack(frames);
addContext(parsedFrames);
await addContext(parsedFrames);

// Calls to readFile shouldn't increase if there isn't a new error
expect(spy).toHaveBeenCalledTimes(mockCalls);
});

test('parseStack with ESM module names', () => {
test('parseStack with ESM module names', async () => {
expect.assertions(1);

const framesWithFilePath: stacktrace.StackFrame[] = [
Expand All @@ -57,31 +57,31 @@ describe('parsers.ts', () => {
},
];
const parsedFrames = Parsers.parseStack(framesWithFilePath);
addContext(parsedFrames);
await addContext(parsedFrames);
expect(spy).toHaveBeenCalledTimes(1);
});

test('parseStack with adding different file', () => {
test('parseStack with adding different file', async () => {
expect.assertions(2);
let mockCalls = 0;
let newErrorCalls = 0;
let parsedFrames = Parsers.parseStack(frames);
addContext(parsedFrames);
await addContext(parsedFrames);

mockCalls = spy.mock.calls.length;
parsedFrames = Parsers.parseStack(stacktrace.parse(getError()));
addContext(parsedFrames);
await addContext(parsedFrames);

newErrorCalls = spy.mock.calls.length;
expect(newErrorCalls).toBeGreaterThan(mockCalls);

parsedFrames = Parsers.parseStack(stacktrace.parse(getError()));
addContext(parsedFrames);
await addContext(parsedFrames);

expect(spy).toHaveBeenCalledTimes(newErrorCalls);
});

test('parseStack with duplicate files', () => {
test('parseStack with duplicate files', async () => {
expect.assertions(1);
const framesWithDuplicateFiles: stacktrace.StackFrame[] = [
{
Expand Down Expand Up @@ -114,16 +114,16 @@ describe('parsers.ts', () => {
];

const parsedFrames = Parsers.parseStack(framesWithDuplicateFiles);
addContext(parsedFrames);
await addContext(parsedFrames);
expect(spy).toHaveBeenCalledTimes(1);
});

test('parseStack with no context', () => {
test('parseStack with no context', async () => {
contextLines = new ContextLines({ frameContextLines: 0 });

expect.assertions(1);
const parsedFrames = Parsers.parseStack(frames);
addContext(parsedFrames);
await addContext(parsedFrames);
expect(spy).toHaveBeenCalledTimes(0);
});
});
Expand Down