Skip to content

ref(node): Refactor node source fetching into integration #3729

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 45 commits into from
Feb 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
45 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
ef7d423
Abhi code review
timfish Feb 17, 2022
a172765
Revert promisify
timfish Feb 18, 2022
bd85ce9
Add TODO comment
timfish Feb 18, 2022
d7952ea
Merge remote-tracking branch 'upstream/master' into feat/separate-sou…
timfish Feb 23, 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
8 changes: 4 additions & 4 deletions packages/nextjs/test/index.server.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { BaseClient } from '@sentry/core';
import { RewriteFrames } from '@sentry/integrations';
import * as SentryNode from '@sentry/node';
import { getCurrentHub, NodeClient } from '@sentry/node';
Expand All @@ -17,7 +16,6 @@ const global = getGlobalObject();
(global as typeof global & { __rewriteFramesDistDir__: string }).__rewriteFramesDistDir__ = '.next';

const nodeInit = jest.spyOn(SentryNode, 'init');
const captureEvent = jest.spyOn(BaseClient.prototype, 'captureEvent');
const logError = jest.spyOn(logger, 'error');

describe('Server init()', () => {
Expand Down Expand Up @@ -91,7 +89,7 @@ describe('Server init()', () => {
expect(currentScope._tags.vercel).toBeUndefined();
});

it('adds 404 transaction filter', () => {
it('adds 404 transaction filter', async () => {
init({
dsn: 'https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012',
tracesSampleRate: 1.0,
Expand All @@ -102,8 +100,10 @@ describe('Server init()', () => {
const transaction = hub.startTransaction({ name: '/404' });
transaction.finish();

// We need to flush because the event processor pipeline is async whereas transaction.finish() is sync.
await SentryNode.flush();

expect(sendEvent).not.toHaveBeenCalled();
expect(captureEvent.mock.results[0].value).toBeUndefined();
expect(logError).toHaveBeenCalledWith(new SentryError('An event processor returned null, will not send event.'));
});

Expand Down
2 changes: 1 addition & 1 deletion packages/node/src/backend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export class NodeBackend extends BaseBackend<NodeOptions> {
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types
public eventFromException(exception: any, hint?: EventHint): PromiseLike<Event> {
return eventFromException(this._options, exception, hint);
return eventFromException(exception, hint);
}

/**
Expand Down
19 changes: 7 additions & 12 deletions packages/node/src/eventbuilder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { extractStackFromError, parseError, parseStack, prepareFramesForEvent }
* Builds and Event from a Exception
* @hidden
*/
export function eventFromException(options: Options, exception: unknown, hint?: EventHint): PromiseLike<Event> {
export function eventFromException(exception: unknown, hint?: EventHint): PromiseLike<Event> {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
let ex: any = exception;
const providedMechanism: Mechanism | undefined =
Expand Down Expand Up @@ -48,7 +48,7 @@ export function eventFromException(options: Options, exception: unknown, hint?:
}

return new SyncPromise<Event>((resolve, reject) =>
parseError(ex as Error, options)
parseError(ex as Error)
.then(event => {
addExceptionTypeValue(event, undefined, undefined);
addExceptionMechanism(event, mechanism);
Expand Down Expand Up @@ -81,16 +81,11 @@ export function eventFromMessage(
return new SyncPromise<Event>(resolve => {
if (options.attachStacktrace && hint && hint.syntheticException) {
const stack = hint.syntheticException ? extractStackFromError(hint.syntheticException) : [];
void parseStack(stack, options)
.then(frames => {
event.stacktrace = {
frames: prepareFramesForEvent(frames),
};
resolve(event);
})
.then(null, () => {
resolve(event);
});
const frames = parseStack(stack);
event.stacktrace = {
frames: prepareFramesForEvent(frames),
};
resolve(event);
} else {
resolve(event);
}
Expand Down
140 changes: 140 additions & 0 deletions packages/node/src/integrations/contextlines.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
import { getCurrentHub } from '@sentry/core';
import { Event, EventProcessor, Integration } from '@sentry/types';
import { addContextToFrame } from '@sentry/utils';
import { readFile } from 'fs';
import { LRUMap } from 'lru_map';

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

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

// TODO: Replace with promisify when minimum supported node >= v8
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
*/
export function resetFileContentCache(): void {
FILE_CONTENT_CACHE.clear();
}

interface ContextLinesOptions {
/**
* Sets the number of context lines for each frame when loading a file.
* Defaults to 7.
*
* Set to 0 to disable loading and inclusion of source files.
**/
frameContextLines?: number;
}

/** Add node modules / packages to the event */
export class ContextLines implements Integration {
/**
* @inheritDoc
*/
public static id: string = 'ContextLines';

/**
* @inheritDoc
*/
public name: string = ContextLines.id;

public constructor(private readonly _options: ContextLinesOptions = {}) {}

/**
* @inheritDoc
*/
public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void): void {
// This is only here to copy frameContextLines from init options if it hasn't
// been set via this integrations constructor.
//
// TODO: Remove on next major!
if (this._options.frameContextLines === undefined) {
const initOptions = getCurrentHub().getClient<NodeClient>()?.getOptions();
// eslint-disable-next-line deprecation/deprecation
this._options.frameContextLines = initOptions?.frameContextLines;
}

const contextLines =
this._options.frameContextLines !== undefined ? this._options.frameContextLines : DEFAULT_LINES_OF_CONTEXT;

addGlobalEventProcessor(event => this.addSourceContext(event, contextLines));
}

/** Processes an event and adds context lines */
public async addSourceContext(event: Event, contextLines: number): Promise<Event> {
const frames = event.exception?.values?.[0].stacktrace?.frames;

if (frames && contextLines > 0) {
const filenames: Set<string> = new Set();

for (const frame of frames) {
if (frame.filename) {
filenames.add(frame.filename);
}
}

const sourceFiles = await readSourceFiles(filenames);

for (const frame of frames) {
if (frame.filename && sourceFiles[frame.filename]) {
try {
const lines = (sourceFiles[frame.filename] as string).split('\n');
addContextToFrame(lines, frame, contextLines);
} catch (e) {
// anomaly, being defensive in case
// unlikely to ever happen in practice but can definitely happen in theory
}
}
}
}

return event;
}
}

/**
* This function reads file contents and caches them in a global LRU cache.
*
* @param filenames Array of filepaths to read content from.
*/
async function readSourceFiles(filenames: Set<string>): Promise<Record<string, string | null>> {
const sourceFiles: Record<string, string | null> = {};

for (const filename of filenames) {
const cachedFile = FILE_CONTENT_CACHE.get(filename);
// We have a cache hit
if (cachedFile !== undefined) {
// If stored value is null, it means that we already tried, but couldn't read the content of the file. Skip.
if (cachedFile === null) {
continue;
}

// Otherwise content is there, so reuse cached value.
sourceFiles[filename] = cachedFile;
continue;
}

let content: string | null = null;
try {
content = await readTextFileAsync(filename);
} catch (_) {
//
}

FILE_CONTENT_CACHE.set(filename, content);
sourceFiles[filename] = content;
}

return sourceFiles;
}
1 change: 1 addition & 0 deletions packages/node/src/integrations/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@ export { OnUncaughtException } from './onuncaughtexception';
export { OnUnhandledRejection } from './onunhandledrejection';
export { LinkedErrors } from './linkederrors';
export { Modules } from './modules';
export { ContextLines } from './contextlines';
Loading