From 31935f023d5194392e85c5c554846cf3eefee0b2 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Sun, 20 Jun 2021 22:20:16 +0100 Subject: [PATCH 01/25] node stackwalk for Electron --- packages/node/src/backend.ts | 88 +++--------------------- packages/node/src/eventbuilder.ts | 106 +++++++++++++++++++++++++++++ packages/node/src/index.ts | 1 + packages/node/src/parsers.ts | 41 +++++++---- packages/node/test/parsers.test.ts | 16 ++--- 5 files changed, 150 insertions(+), 102 deletions(-) create mode 100644 packages/node/src/eventbuilder.ts diff --git a/packages/node/src/backend.ts b/packages/node/src/backend.ts index 8220d690ceb4..8a2bdb14df4e 100644 --- a/packages/node/src/backend.ts +++ b/packages/node/src/backend.ts @@ -1,17 +1,9 @@ -import { BaseBackend, getCurrentHub } from '@sentry/core'; -import { Event, EventHint, Mechanism, Severity, Transport, TransportOptions } from '@sentry/types'; -import { - addExceptionMechanism, - addExceptionTypeValue, - Dsn, - extractExceptionKeysForMessage, - isError, - isPlainObject, - normalizeToSize, - SyncPromise, -} from '@sentry/utils'; +import { BaseBackend } from '@sentry/core'; +import { Event, EventHint, Severity, Transport, TransportOptions } from '@sentry/types'; +import { Dsn } from '@sentry/utils'; +import { readFile } from 'fs'; -import { extractStackFromError, parseError, parseStack, prepareFramesForEvent } from './parsers'; +import { eventFromException, eventFromMessage } from './eventbuilder'; import { HTTPSTransport, HTTPTransport } from './transports'; import { NodeOptions } from './types'; @@ -24,79 +16,15 @@ export class NodeBackend extends BaseBackend { * @inheritDoc */ // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types - public eventFromException(exception: any, hint?: EventHint): PromiseLike { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - let ex: any = exception; - const providedMechanism: Mechanism | undefined = - hint && hint.data && (hint.data as { mechanism: Mechanism }).mechanism; - const mechanism: Mechanism = providedMechanism || { - handled: true, - type: 'generic', - }; - - if (!isError(exception)) { - if (isPlainObject(exception)) { - // This will allow us to group events based on top-level keys - // which is much better than creating new group when any key/value change - const message = `Non-Error exception captured with keys: ${extractExceptionKeysForMessage(exception)}`; - - getCurrentHub().configureScope(scope => { - scope.setExtra('__serialized__', normalizeToSize(exception as Record)); - }); - - ex = (hint && hint.syntheticException) || new Error(message); - (ex as Error).message = message; - } else { - // This handles when someone does: `throw "something awesome";` - // We use synthesized Error here so we can extract a (rough) stack trace. - ex = (hint && hint.syntheticException) || new Error(exception as string); - (ex as Error).message = exception; - } - mechanism.synthetic = true; - } - - return new SyncPromise((resolve, reject) => - parseError(ex as Error, this._options) - .then(event => { - addExceptionTypeValue(event, undefined, undefined); - addExceptionMechanism(event, mechanism); - - resolve({ - ...event, - event_id: hint && hint.event_id, - }); - }) - .then(null, reject), - ); + public eventFromException(exception: any, hint?: EventHint | undefined): PromiseLike { + return eventFromException(this._options, exception, readFile, hint); } /** * @inheritDoc */ public eventFromMessage(message: string, level: Severity = Severity.Info, hint?: EventHint): PromiseLike { - const event: Event = { - event_id: hint && hint.event_id, - level, - message, - }; - - return new SyncPromise(resolve => { - if (this._options.attachStacktrace && hint && hint.syntheticException) { - const stack = hint.syntheticException ? extractStackFromError(hint.syntheticException) : []; - void parseStack(stack, this._options) - .then(frames => { - event.stacktrace = { - frames: prepareFramesForEvent(frames), - }; - resolve(event); - }) - .then(null, () => { - resolve(event); - }); - } else { - resolve(event); - } - }); + return eventFromMessage(this._options, message, level, readFile, hint); } /** diff --git a/packages/node/src/eventbuilder.ts b/packages/node/src/eventbuilder.ts new file mode 100644 index 000000000000..7ac5868c9227 --- /dev/null +++ b/packages/node/src/eventbuilder.ts @@ -0,0 +1,106 @@ +import { getCurrentHub } from '@sentry/core'; +import { Event, EventHint, Mechanism, Severity } from '@sentry/types'; +import { + addExceptionMechanism, + addExceptionTypeValue, + extractExceptionKeysForMessage, + isError, + isPlainObject, + normalizeToSize, + SyncPromise, +} from '@sentry/utils'; + +import { extractStackFromError, parseError, parseStack, prepareFramesForEvent, ReadFileFn } from './parsers'; +import { NodeOptions } from './types'; + +/** + * Builds and Event from a Exception + * @hidden + */ +export function eventFromException( + options: NodeOptions, + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types + exception: any, + readFile?: ReadFileFn, + hint?: EventHint, +): PromiseLike { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let ex: any = exception; + const providedMechanism: Mechanism | undefined = + hint && hint.data && (hint.data as { mechanism: Mechanism }).mechanism; + const mechanism: Mechanism = providedMechanism || { + handled: true, + type: 'generic', + }; + + if (!isError(exception)) { + if (isPlainObject(exception)) { + // This will allow us to group events based on top-level keys + // which is much better than creating new group when any key/value change + const message = `Non-Error exception captured with keys: ${extractExceptionKeysForMessage(exception)}`; + + getCurrentHub().configureScope(scope => { + scope.setExtra('__serialized__', normalizeToSize(exception as Record)); + }); + + ex = (hint && hint.syntheticException) || new Error(message); + (ex as Error).message = message; + } else { + // This handles when someone does: `throw "something awesome";` + // We use synthesized Error here so we can extract a (rough) stack trace. + ex = (hint && hint.syntheticException) || new Error(exception as string); + (ex as Error).message = exception; + } + mechanism.synthetic = true; + } + + return new SyncPromise((resolve, reject) => + parseError(ex as Error, readFile, options) + .then(event => { + addExceptionTypeValue(event, undefined, undefined); + addExceptionMechanism(event, mechanism); + + resolve({ + ...event, + event_id: hint && hint.event_id, + }); + }) + .then(null, reject), + ); +} + +/** + * Builds and Event from a Message + * @hidden + */ +export function eventFromMessage( + options: NodeOptions, + message: string, + level: Severity = Severity.Info, + readFile?: ReadFileFn, + hint?: EventHint, +): PromiseLike { + const event: Event = { + event_id: hint && hint.event_id, + level, + message, + }; + + return new SyncPromise(resolve => { + if (options.attachStacktrace && hint && hint.syntheticException) { + const stack = hint.syntheticException ? extractStackFromError(hint.syntheticException) : []; + void parseStack(stack, readFile, options) + .then(frames => { + event.stacktrace = { + frames: prepareFramesForEvent(frames), + }; + resolve(event); + }) + .then(null, () => { + resolve(event); + }); + } else { + resolve(event); + } + }); +} diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index b3bae88dfa8a..98910f4ce2b5 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -42,6 +42,7 @@ export { NodeOptions } from './types'; export { NodeBackend } from './backend'; export { NodeClient } from './client'; export { defaultIntegrations, init, lastEventId, flush, close, getSentryRelease } from './sdk'; +export { eventFromException, eventFromMessage } from './eventbuilder'; export { deepReadDirSync } from './utils'; export { SDK_NAME } from './version'; diff --git a/packages/node/src/parsers.ts b/packages/node/src/parsers.ts index e9dbfb9ebb39..208c5d740095 100644 --- a/packages/node/src/parsers.ts +++ b/packages/node/src/parsers.ts @@ -1,6 +1,5 @@ import { Event, Exception, ExtendedError, StackFrame } from '@sentry/types'; import { addContextToFrame, basename, dirname, SyncPromise } from '@sentry/utils'; -import { readFile } from 'fs'; import { LRUMap } from 'lru_map'; import * as stacktrace from './stacktrace'; @@ -9,6 +8,8 @@ import { NodeOptions } from './types'; const DEFAULT_LINES_OF_CONTEXT: number = 7; const FILE_CONTENT_CACHE = new LRUMap(100); +export type ReadFileFn = (path: string, callback: (err: Error | null, data: Buffer) => void) => void; + /** * Resets the file cache. Exists for testing purposes. * @hidden @@ -68,7 +69,7 @@ function getModule(filename: string, base?: string): string { * * @param filenames Array of filepaths to read content from. */ -function readSourceFiles(filenames: string[]): PromiseLike<{ [key: string]: string | null }> { +function readSourceFiles(filenames: string[], readFile: ReadFileFn): PromiseLike<{ [key: string]: string | null }> { // we're relying on filenames being de-duped already if (filenames.length === 0) { return SyncPromise.resolve({}); @@ -135,7 +136,11 @@ export function extractStackFromError(error: Error): stacktrace.StackFrame[] { /** * @hidden */ -export function parseStack(stack: stacktrace.StackFrame[], options?: NodeOptions): PromiseLike { +export function parseStack( + stack: stacktrace.StackFrame[], + readFile?: ReadFileFn, + options?: NodeOptions, +): PromiseLike { const filesToRead: string[] = []; const linesOfContext = @@ -179,13 +184,16 @@ export function parseStack(stack: stacktrace.StackFrame[], options?: NodeOptions return SyncPromise.resolve(frames); } - try { - return addPrePostContext(filesToRead, frames, linesOfContext); - } catch (_) { - // This happens in electron for example where we are not able to read files from asar. - // So it's fine, we recover be just returning all frames without pre/post context. - return SyncPromise.resolve(frames); + if (readFile) { + try { + return addPrePostContext(filesToRead, frames, linesOfContext, readFile); + } catch (_) { + // This happens in electron for example where we are not able to read files from asar. + // So it's fine, we recover be just returning all frames without pre/post context. + } } + + return SyncPromise.resolve(frames); } /** @@ -198,9 +206,10 @@ function addPrePostContext( filesToRead: string[], frames: StackFrame[], linesOfContext: number, + readFile: ReadFileFn, ): PromiseLike { return new SyncPromise(resolve => - readSourceFiles(filesToRead).then(sourceFiles => { + readSourceFiles(filesToRead, readFile).then(sourceFiles => { const result = frames.map(frame => { if (frame.filename && sourceFiles[frame.filename]) { try { @@ -223,11 +232,15 @@ function addPrePostContext( /** * @hidden */ -export function getExceptionFromError(error: Error, options?: NodeOptions): PromiseLike { +export function getExceptionFromError( + error: Error, + readFile?: ReadFileFn, + options?: NodeOptions, +): PromiseLike { const name = error.name || error.constructor.name; const stack = extractStackFromError(error); return new SyncPromise(resolve => - parseStack(stack, options).then(frames => { + parseStack(stack, readFile, options).then(frames => { const result = { stacktrace: { frames: prepareFramesForEvent(frames), @@ -243,9 +256,9 @@ export function getExceptionFromError(error: Error, options?: NodeOptions): Prom /** * @hidden */ -export function parseError(error: ExtendedError, options?: NodeOptions): PromiseLike { +export function parseError(error: ExtendedError, readFile?: ReadFileFn, options?: NodeOptions): PromiseLike { return new SyncPromise(resolve => - getExceptionFromError(error, options).then((exception: Exception) => { + getExceptionFromError(error, readFile, options).then((exception: Exception) => { resolve({ exception: { values: [exception], diff --git a/packages/node/test/parsers.test.ts b/packages/node/test/parsers.test.ts index 3097b85d33c7..501417e4d0f4 100644 --- a/packages/node/test/parsers.test.ts +++ b/packages/node/test/parsers.test.ts @@ -22,10 +22,10 @@ describe('parsers.ts', () => { test('parseStack with same file', done => { expect.assertions(1); let mockCalls = 0; - void Parsers.parseStack(frames) + void Parsers.parseStack(frames, fs.readFile) .then(_ => { mockCalls = spy.mock.calls.length; - void Parsers.parseStack(frames) + void Parsers.parseStack(frames, fs.readFile) .then(_1 => { // Calls to readFile shouldn't increase if there isn't a new error expect(spy).toHaveBeenCalledTimes(mockCalls); @@ -54,7 +54,7 @@ describe('parsers.ts', () => { typeName: 'module.exports../src/index.ts', }, ]; - return Parsers.parseStack(framesWithFilePath).then(_ => { + return Parsers.parseStack(framesWithFilePath, fs.readFile).then(_ => { expect(spy).toHaveBeenCalledTimes(1); }); }); @@ -63,14 +63,14 @@ describe('parsers.ts', () => { expect.assertions(2); let mockCalls = 0; let newErrorCalls = 0; - void Parsers.parseStack(frames) + void Parsers.parseStack(frames, fs.readFile) .then(_ => { mockCalls = spy.mock.calls.length; - void Parsers.parseStack(stacktrace.parse(getError())) + void Parsers.parseStack(stacktrace.parse(getError()), fs.readFile) .then(_1 => { newErrorCalls = spy.mock.calls.length; expect(newErrorCalls).toBeGreaterThan(mockCalls); - void Parsers.parseStack(stacktrace.parse(getError())) + void Parsers.parseStack(stacktrace.parse(getError()), fs.readFile) .then(_2 => { expect(spy).toHaveBeenCalledTimes(newErrorCalls); done(); @@ -120,14 +120,14 @@ describe('parsers.ts', () => { typeName: 'module.exports../src/index.ts', }, ]; - return Parsers.parseStack(framesWithDuplicateFiles).then(_ => { + return Parsers.parseStack(framesWithDuplicateFiles, fs.readFile).then(_ => { expect(spy).toHaveBeenCalledTimes(1); }); }); test('parseStack with no context', async () => { expect.assertions(1); - return Parsers.parseStack(frames, { frameContextLines: 0 }).then(_ => { + return Parsers.parseStack(frames, fs.readFile, { frameContextLines: 0 }).then(_ => { expect(spy).toHaveBeenCalledTimes(0); }); }); From f143c1014ef4211b7c626dc7e0dc5523c9e1682b Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 21 Jun 2021 12:22:56 +0100 Subject: [PATCH 02/25] Re-order params --- packages/node/src/backend.ts | 6 +++--- packages/node/src/eventbuilder.ts | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/node/src/backend.ts b/packages/node/src/backend.ts index 8a2bdb14df4e..fdcf15ed490a 100644 --- a/packages/node/src/backend.ts +++ b/packages/node/src/backend.ts @@ -16,15 +16,15 @@ export class NodeBackend extends BaseBackend { * @inheritDoc */ // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types - public eventFromException(exception: any, hint?: EventHint | undefined): PromiseLike { - return eventFromException(this._options, exception, readFile, hint); + public eventFromException(exception: any, hint?: EventHint): PromiseLike { + return eventFromException(this._options, exception, hint, readFile); } /** * @inheritDoc */ public eventFromMessage(message: string, level: Severity = Severity.Info, hint?: EventHint): PromiseLike { - return eventFromMessage(this._options, message, level, readFile, hint); + return eventFromMessage(this._options, message, level, hint, readFile); } /** diff --git a/packages/node/src/eventbuilder.ts b/packages/node/src/eventbuilder.ts index 7ac5868c9227..320e29acd326 100644 --- a/packages/node/src/eventbuilder.ts +++ b/packages/node/src/eventbuilder.ts @@ -21,8 +21,8 @@ export function eventFromException( options: NodeOptions, // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types exception: any, - readFile?: ReadFileFn, hint?: EventHint, + readFile?: ReadFileFn, ): PromiseLike { // eslint-disable-next-line @typescript-eslint/no-explicit-any let ex: any = exception; @@ -77,8 +77,8 @@ export function eventFromMessage( options: NodeOptions, message: string, level: Severity = Severity.Info, - readFile?: ReadFileFn, hint?: EventHint, + readFile?: ReadFileFn, ): PromiseLike { const event: Event = { event_id: hint && hint.event_id, From 8b30a9a107c517365720aa58fc35a204b634a591 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 21 Jun 2021 13:07:27 +0100 Subject: [PATCH 03/25] Move source loading to seperate file --- packages/node/src/backend.ts | 6 +- packages/node/src/eventbuilder.ts | 10 +-- packages/node/src/parsers.ts | 124 +++-------------------------- packages/node/src/sources.ts | 105 ++++++++++++++++++++++++ packages/node/test/parsers.test.ts | 19 ++--- 5 files changed, 136 insertions(+), 128 deletions(-) create mode 100644 packages/node/src/sources.ts diff --git a/packages/node/src/backend.ts b/packages/node/src/backend.ts index fdcf15ed490a..e8fc14a1d092 100644 --- a/packages/node/src/backend.ts +++ b/packages/node/src/backend.ts @@ -1,9 +1,9 @@ import { BaseBackend } from '@sentry/core'; import { Event, EventHint, Severity, Transport, TransportOptions } from '@sentry/types'; import { Dsn } from '@sentry/utils'; -import { readFile } from 'fs'; import { eventFromException, eventFromMessage } from './eventbuilder'; +import { readFilesAddPrePostContext } from './sources'; import { HTTPSTransport, HTTPTransport } from './transports'; import { NodeOptions } from './types'; @@ -17,14 +17,14 @@ export class NodeBackend extends BaseBackend { */ // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types public eventFromException(exception: any, hint?: EventHint): PromiseLike { - return eventFromException(this._options, exception, hint, readFile); + return eventFromException(this._options, exception, hint, readFilesAddPrePostContext); } /** * @inheritDoc */ public eventFromMessage(message: string, level: Severity = Severity.Info, hint?: EventHint): PromiseLike { - return eventFromMessage(this._options, message, level, hint, readFile); + return eventFromMessage(this._options, message, level, hint, readFilesAddPrePostContext); } /** diff --git a/packages/node/src/eventbuilder.ts b/packages/node/src/eventbuilder.ts index 320e29acd326..61069cea605a 100644 --- a/packages/node/src/eventbuilder.ts +++ b/packages/node/src/eventbuilder.ts @@ -10,7 +10,7 @@ import { SyncPromise, } from '@sentry/utils'; -import { extractStackFromError, parseError, parseStack, prepareFramesForEvent, ReadFileFn } from './parsers'; +import { extractStackFromError, parseError, parseStack, prepareFramesForEvent, ReadFilesFn } from './parsers'; import { NodeOptions } from './types'; /** @@ -22,7 +22,7 @@ export function eventFromException( // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types exception: any, hint?: EventHint, - readFile?: ReadFileFn, + readFiles?: ReadFilesFn, ): PromiseLike { // eslint-disable-next-line @typescript-eslint/no-explicit-any let ex: any = exception; @@ -55,7 +55,7 @@ export function eventFromException( } return new SyncPromise((resolve, reject) => - parseError(ex as Error, readFile, options) + parseError(ex as Error, readFiles, options) .then(event => { addExceptionTypeValue(event, undefined, undefined); addExceptionMechanism(event, mechanism); @@ -78,7 +78,7 @@ export function eventFromMessage( message: string, level: Severity = Severity.Info, hint?: EventHint, - readFile?: ReadFileFn, + readFiles?: ReadFilesFn, ): PromiseLike { const event: Event = { event_id: hint && hint.event_id, @@ -89,7 +89,7 @@ export function eventFromMessage( return new SyncPromise(resolve => { if (options.attachStacktrace && hint && hint.syntheticException) { const stack = hint.syntheticException ? extractStackFromError(hint.syntheticException) : []; - void parseStack(stack, readFile, options) + void parseStack(stack, readFiles, options) .then(frames => { event.stacktrace = { frames: prepareFramesForEvent(frames), diff --git a/packages/node/src/parsers.ts b/packages/node/src/parsers.ts index 208c5d740095..b24df476c5c3 100644 --- a/packages/node/src/parsers.ts +++ b/packages/node/src/parsers.ts @@ -1,22 +1,16 @@ import { Event, Exception, ExtendedError, StackFrame } from '@sentry/types'; -import { addContextToFrame, basename, dirname, SyncPromise } from '@sentry/utils'; -import { LRUMap } from 'lru_map'; +import { basename, dirname, SyncPromise } from '@sentry/utils'; import * as stacktrace from './stacktrace'; import { NodeOptions } from './types'; const DEFAULT_LINES_OF_CONTEXT: number = 7; -const FILE_CONTENT_CACHE = new LRUMap(100); -export type ReadFileFn = (path: string, callback: (err: Error | null, data: Buffer) => void) => void; - -/** - * Resets the file cache. Exists for testing purposes. - * @hidden - */ -export function resetFileContentCache(): void { - FILE_CONTENT_CACHE.clear(); -} +export type ReadFilesFn = ( + filesToRead: string[], + frames: StackFrame[], + linesOfContext: number, +) => PromiseLike; /** JSDoc */ function getFunction(frame: stacktrace.StackFrame): string { @@ -63,65 +57,6 @@ function getModule(filename: string, base?: string): string { return file; } -/** - * This function reads file contents and caches them in a global LRU cache. - * Returns a Promise filepath => content array for all files that we were able to read. - * - * @param filenames Array of filepaths to read content from. - */ -function readSourceFiles(filenames: string[], readFile: ReadFileFn): PromiseLike<{ [key: string]: string | null }> { - // we're relying on filenames being de-duped already - if (filenames.length === 0) { - return SyncPromise.resolve({}); - } - - return new SyncPromise<{ - [key: string]: string | null; - }>(resolve => { - const sourceFiles: { - [key: string]: string | null; - } = {}; - - let count = 0; - // eslint-disable-next-line @typescript-eslint/prefer-for-of - for (let i = 0; i < filenames.length; i++) { - const filename = filenames[i]; - - const cache = FILE_CONTENT_CACHE.get(filename); - // We have a cache hit - if (cache !== undefined) { - // If it's not null (which means we found a file and have a content) - // we set the content and return it later. - if (cache !== null) { - sourceFiles[filename] = cache; - } - // eslint-disable-next-line no-plusplus - count++; - // In any case we want to skip here then since we have a content already or we couldn't - // read the file and don't want to try again. - if (count === filenames.length) { - resolve(sourceFiles); - } - continue; - } - - readFile(filename, (err: Error | null, data: Buffer) => { - const content = err ? null : data.toString(); - sourceFiles[filename] = content; - - // We always want to set the cache, even to null which means there was an error reading the file. - // We do not want to try to read the file again. - FILE_CONTENT_CACHE.set(filename, content); - // eslint-disable-next-line no-plusplus - count++; - if (count === filenames.length) { - resolve(sourceFiles); - } - }); - } - }); -} - /** * @hidden */ @@ -138,7 +73,7 @@ export function extractStackFromError(error: Error): stacktrace.StackFrame[] { */ export function parseStack( stack: stacktrace.StackFrame[], - readFile?: ReadFileFn, + readFiles?: ReadFilesFn, options?: NodeOptions, ): PromiseLike { const filesToRead: string[] = []; @@ -184,9 +119,9 @@ export function parseStack( return SyncPromise.resolve(frames); } - if (readFile) { + if (readFiles) { try { - return addPrePostContext(filesToRead, frames, linesOfContext, readFile); + return readFiles(filesToRead, frames, linesOfContext); } catch (_) { // This happens in electron for example where we are not able to read files from asar. // So it's fine, we recover be just returning all frames without pre/post context. @@ -196,51 +131,18 @@ export function parseStack( return SyncPromise.resolve(frames); } -/** - * This function tries to read the source files + adding pre and post context (source code) - * to a frame. - * @param filesToRead string[] of filepaths - * @param frames StackFrame[] containg all frames - */ -function addPrePostContext( - filesToRead: string[], - frames: StackFrame[], - linesOfContext: number, - readFile: ReadFileFn, -): PromiseLike { - return new SyncPromise(resolve => - readSourceFiles(filesToRead, readFile).then(sourceFiles => { - const result = frames.map(frame => { - if (frame.filename && sourceFiles[frame.filename]) { - try { - const lines = (sourceFiles[frame.filename] as string).split('\n'); - - addContextToFrame(lines, frame, linesOfContext); - } catch (e) { - // anomaly, being defensive in case - // unlikely to ever happen in practice but can definitely happen in theory - } - } - return frame; - }); - - resolve(result); - }), - ); -} - /** * @hidden */ export function getExceptionFromError( error: Error, - readFile?: ReadFileFn, + readFiles?: ReadFilesFn, options?: NodeOptions, ): PromiseLike { const name = error.name || error.constructor.name; const stack = extractStackFromError(error); return new SyncPromise(resolve => - parseStack(stack, readFile, options).then(frames => { + parseStack(stack, readFiles, options).then(frames => { const result = { stacktrace: { frames: prepareFramesForEvent(frames), @@ -256,9 +158,9 @@ export function getExceptionFromError( /** * @hidden */ -export function parseError(error: ExtendedError, readFile?: ReadFileFn, options?: NodeOptions): PromiseLike { +export function parseError(error: ExtendedError, readFiles?: ReadFilesFn, options?: NodeOptions): PromiseLike { return new SyncPromise(resolve => - getExceptionFromError(error, readFile, options).then((exception: Exception) => { + getExceptionFromError(error, readFiles, options).then((exception: Exception) => { resolve({ exception: { values: [exception], diff --git a/packages/node/src/sources.ts b/packages/node/src/sources.ts new file mode 100644 index 000000000000..42fc97eee72f --- /dev/null +++ b/packages/node/src/sources.ts @@ -0,0 +1,105 @@ +import { StackFrame } from '@sentry/types'; +import { addContextToFrame, SyncPromise } from '@sentry/utils'; +import { readFile } from 'fs'; +import { LRUMap } from 'lru_map'; + +const FILE_CONTENT_CACHE = new LRUMap(100); + +/** + * Resets the file cache. Exists for testing purposes. + * @hidden + */ +export function resetFileContentCache(): void { + FILE_CONTENT_CACHE.clear(); +} + +/** + * This function tries to read the source files + adding pre and post context (source code) + * to a frame. + * @param filesToRead string[] of filepaths + * @param frames StackFrame[] containg all frames + */ +export function readFilesAddPrePostContext( + filesToRead: string[], + frames: StackFrame[], + linesOfContext: number, +): PromiseLike { + return new SyncPromise(resolve => + readSourceFiles(filesToRead).then(sourceFiles => { + const result = frames.map(frame => { + if (frame.filename && sourceFiles[frame.filename]) { + try { + const lines = (sourceFiles[frame.filename] as string).split('\n'); + + addContextToFrame(lines, frame, linesOfContext); + } catch (e) { + // anomaly, being defensive in case + // unlikely to ever happen in practice but can definitely happen in theory + } + } + return frame; + }); + + resolve(result); + }), + ); +} + +/** + * This function reads file contents and caches them in a global LRU cache. + * Returns a Promise filepath => content array for all files that we were able to read. + * + * @param filenames Array of filepaths to read content from. + */ +function readSourceFiles(filenames: string[]): PromiseLike<{ [key: string]: string | null }> { + // we're relying on filenames being de-duped already + if (filenames.length === 0) { + return SyncPromise.resolve({}); + } + + return new SyncPromise<{ + [key: string]: string | null; + }>(resolve => { + const sourceFiles: { + [key: string]: string | null; + } = {}; + + let count = 0; + // eslint-disable-next-line @typescript-eslint/prefer-for-of + for (let i = 0; i < filenames.length; i++) { + const filename = filenames[i]; + + const cache = FILE_CONTENT_CACHE.get(filename); + // We have a cache hit + if (cache !== undefined) { + // If it's not null (which means we found a file and have a content) + // we set the content and return it later. + if (cache !== null) { + sourceFiles[filename] = cache; + } + // eslint-disable-next-line no-plusplus + count++; + // In any case we want to skip here then since we have a content already or we couldn't + // read the file and don't want to try again. + if (count === filenames.length) { + resolve(sourceFiles); + } + continue; + } + + readFile(filename, (err: Error | null, data: Buffer) => { + const content = err ? null : data.toString(); + sourceFiles[filename] = content; + + // We always want to set the cache, even to null which means there was an error reading the file. + // We do not want to try to read the file again. + FILE_CONTENT_CACHE.set(filename, content); + // eslint-disable-next-line no-plusplus + count++; + if (count === filenames.length) { + resolve(sourceFiles); + } + }); + } + }); +} diff --git a/packages/node/test/parsers.test.ts b/packages/node/test/parsers.test.ts index 501417e4d0f4..3a6859d20a31 100644 --- a/packages/node/test/parsers.test.ts +++ b/packages/node/test/parsers.test.ts @@ -1,6 +1,7 @@ import * as fs from 'fs'; import * as Parsers from '../src/parsers'; +import * as Sources from '../src/sources'; import * as stacktrace from '../src/stacktrace'; import { getError } from './helper/error'; @@ -11,7 +12,7 @@ describe('parsers.ts', () => { beforeEach(() => { spy = jest.spyOn(fs, 'readFile'); frames = stacktrace.parse(new Error('test')); - Parsers.resetFileContentCache(); + Sources.resetFileContentCache(); }); afterEach(() => { @@ -22,10 +23,10 @@ describe('parsers.ts', () => { test('parseStack with same file', done => { expect.assertions(1); let mockCalls = 0; - void Parsers.parseStack(frames, fs.readFile) + void Parsers.parseStack(frames, Sources.readFilesAddPrePostContext) .then(_ => { mockCalls = spy.mock.calls.length; - void Parsers.parseStack(frames, fs.readFile) + void Parsers.parseStack(frames, Sources.readFilesAddPrePostContext) .then(_1 => { // Calls to readFile shouldn't increase if there isn't a new error expect(spy).toHaveBeenCalledTimes(mockCalls); @@ -54,7 +55,7 @@ describe('parsers.ts', () => { typeName: 'module.exports../src/index.ts', }, ]; - return Parsers.parseStack(framesWithFilePath, fs.readFile).then(_ => { + return Parsers.parseStack(framesWithFilePath, Sources.readFilesAddPrePostContext).then(_ => { expect(spy).toHaveBeenCalledTimes(1); }); }); @@ -63,14 +64,14 @@ describe('parsers.ts', () => { expect.assertions(2); let mockCalls = 0; let newErrorCalls = 0; - void Parsers.parseStack(frames, fs.readFile) + void Parsers.parseStack(frames, Sources.readFilesAddPrePostContext) .then(_ => { mockCalls = spy.mock.calls.length; - void Parsers.parseStack(stacktrace.parse(getError()), fs.readFile) + void Parsers.parseStack(stacktrace.parse(getError()), Sources.readFilesAddPrePostContext) .then(_1 => { newErrorCalls = spy.mock.calls.length; expect(newErrorCalls).toBeGreaterThan(mockCalls); - void Parsers.parseStack(stacktrace.parse(getError()), fs.readFile) + void Parsers.parseStack(stacktrace.parse(getError()), Sources.readFilesAddPrePostContext) .then(_2 => { expect(spy).toHaveBeenCalledTimes(newErrorCalls); done(); @@ -120,14 +121,14 @@ describe('parsers.ts', () => { typeName: 'module.exports../src/index.ts', }, ]; - return Parsers.parseStack(framesWithDuplicateFiles, fs.readFile).then(_ => { + return Parsers.parseStack(framesWithDuplicateFiles, Sources.readFilesAddPrePostContext).then(_ => { expect(spy).toHaveBeenCalledTimes(1); }); }); test('parseStack with no context', async () => { expect.assertions(1); - return Parsers.parseStack(frames, fs.readFile, { frameContextLines: 0 }).then(_ => { + return Parsers.parseStack(frames, Sources.readFilesAddPrePostContext, { frameContextLines: 0 }).then(_ => { expect(spy).toHaveBeenCalledTimes(0); }); }); From f1e3f3106196450664503555d9833c2020bffc9e Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 22 Jun 2021 13:01:07 +0100 Subject: [PATCH 04/25] First try --- packages/node/package.json | 2 +- packages/node/src/backend.ts | 16 +++- packages/node/src/eventbuilder.ts | 57 +++++------- .../node/src/integrations/linkederrors.ts | 11 +-- packages/node/src/parsers.ts | 89 +++++-------------- packages/node/src/sources.ts | 32 ++++++- packages/node/test/parsers.test.ts | 24 +++-- yarn.lock | 5 ++ 8 files changed, 111 insertions(+), 125 deletions(-) diff --git a/packages/node/package.json b/packages/node/package.json index 331e25052b9c..011114f52a00 100644 --- a/packages/node/package.json +++ b/packages/node/package.json @@ -24,7 +24,7 @@ "cookie": "^0.4.1", "https-proxy-agent": "^5.0.0", "lru_map": "^0.3.3", - "tslib": "^1.9.3" + "tslib": "^2.3.0" }, "devDependencies": { "@sentry-internal/eslint-config-sdk": "6.7.1", diff --git a/packages/node/src/backend.ts b/packages/node/src/backend.ts index e8fc14a1d092..84f513aba5a8 100644 --- a/packages/node/src/backend.ts +++ b/packages/node/src/backend.ts @@ -3,7 +3,7 @@ import { Event, EventHint, Severity, Transport, TransportOptions } from '@sentry import { Dsn } from '@sentry/utils'; import { eventFromException, eventFromMessage } from './eventbuilder'; -import { readFilesAddPrePostContext } from './sources'; +import { addSourcesToFrames } from './sources'; import { HTTPSTransport, HTTPTransport } from './transports'; import { NodeOptions } from './types'; @@ -17,14 +17,24 @@ export class NodeBackend extends BaseBackend { */ // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types public eventFromException(exception: any, hint?: EventHint): PromiseLike { - return eventFromException(this._options, exception, hint, readFilesAddPrePostContext); + const event = eventFromException(this._options, exception, hint); + + return addSourcesToFrames(event.stacktrace?.frames || [], this._options).then(frames => { + event.stacktrace = { frames }; + return event; + }); } /** * @inheritDoc */ public eventFromMessage(message: string, level: Severity = Severity.Info, hint?: EventHint): PromiseLike { - return eventFromMessage(this._options, message, level, hint, readFilesAddPrePostContext); + const event = eventFromMessage(this._options, message, level, hint); + + return addSourcesToFrames(event.stacktrace?.frames || [], this._options).then(frames => { + event.stacktrace = { frames }; + return event; + }); } /** diff --git a/packages/node/src/eventbuilder.ts b/packages/node/src/eventbuilder.ts index 61069cea605a..f306de569867 100644 --- a/packages/node/src/eventbuilder.ts +++ b/packages/node/src/eventbuilder.ts @@ -7,10 +7,9 @@ import { isError, isPlainObject, normalizeToSize, - SyncPromise, } from '@sentry/utils'; -import { extractStackFromError, parseError, parseStack, prepareFramesForEvent, ReadFilesFn } from './parsers'; +import { extractStackFromError, parseError, parseStack, prepareFramesForEvent } from './parsers'; import { NodeOptions } from './types'; /** @@ -18,12 +17,11 @@ import { NodeOptions } from './types'; * @hidden */ export function eventFromException( - options: NodeOptions, + _options: NodeOptions, // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types exception: any, hint?: EventHint, - readFiles?: ReadFilesFn, -): PromiseLike { +): Event { // eslint-disable-next-line @typescript-eslint/no-explicit-any let ex: any = exception; const providedMechanism: Mechanism | undefined = @@ -54,19 +52,14 @@ export function eventFromException( mechanism.synthetic = true; } - return new SyncPromise((resolve, reject) => - parseError(ex as Error, readFiles, options) - .then(event => { - addExceptionTypeValue(event, undefined, undefined); - addExceptionMechanism(event, mechanism); + const event = parseError(ex as Error); + addExceptionTypeValue(event, undefined, undefined); + addExceptionMechanism(event, mechanism); - resolve({ - ...event, - event_id: hint && hint.event_id, - }); - }) - .then(null, reject), - ); + return { + ...event, + event_id: hint && hint.event_id, + }; } /** @@ -78,29 +71,21 @@ export function eventFromMessage( message: string, level: Severity = Severity.Info, hint?: EventHint, - readFiles?: ReadFilesFn, -): PromiseLike { +): Event { const event: Event = { event_id: hint && hint.event_id, level, message, }; - return new SyncPromise(resolve => { - if (options.attachStacktrace && hint && hint.syntheticException) { - const stack = hint.syntheticException ? extractStackFromError(hint.syntheticException) : []; - void parseStack(stack, readFiles, options) - .then(frames => { - event.stacktrace = { - frames: prepareFramesForEvent(frames), - }; - resolve(event); - }) - .then(null, () => { - resolve(event); - }); - } else { - resolve(event); - } - }); + if (options.attachStacktrace && hint && hint.syntheticException) { + const stack = hint.syntheticException ? extractStackFromError(hint.syntheticException) : []; + const frames = parseStack(stack); + + event.stacktrace = { + frames: prepareFramesForEvent(frames), + }; + } + + return event; } diff --git a/packages/node/src/integrations/linkederrors.ts b/packages/node/src/integrations/linkederrors.ts index 6428ee7f0a46..548c14421408 100644 --- a/packages/node/src/integrations/linkederrors.ts +++ b/packages/node/src/integrations/linkederrors.ts @@ -81,14 +81,9 @@ export class LinkedErrors implements Integration { return SyncPromise.resolve(stack); } return new SyncPromise((resolve, reject) => { - void getExceptionFromError(error[key]) - .then((exception: Exception) => { - void this._walkErrorTree(error[key], key, [exception, ...stack]) - .then(resolve) - .then(null, () => { - reject(); - }); - }) + const exception = getExceptionFromError(error[key]); + void this._walkErrorTree(error[key], key, [exception, ...stack]) + .then(resolve) .then(null, () => { reject(); }); diff --git a/packages/node/src/parsers.ts b/packages/node/src/parsers.ts index b24df476c5c3..2b9b2cc6f3ba 100644 --- a/packages/node/src/parsers.ts +++ b/packages/node/src/parsers.ts @@ -1,16 +1,7 @@ import { Event, Exception, ExtendedError, StackFrame } from '@sentry/types'; -import { basename, dirname, SyncPromise } from '@sentry/utils'; +import { basename, dirname } from '@sentry/utils'; import * as stacktrace from './stacktrace'; -import { NodeOptions } from './types'; - -const DEFAULT_LINES_OF_CONTEXT: number = 7; - -export type ReadFilesFn = ( - filesToRead: string[], - frames: StackFrame[], - linesOfContext: number, -) => PromiseLike; /** JSDoc */ function getFunction(frame: stacktrace.StackFrame): string { @@ -71,17 +62,8 @@ export function extractStackFromError(error: Error): stacktrace.StackFrame[] { /** * @hidden */ -export function parseStack( - stack: stacktrace.StackFrame[], - readFiles?: ReadFilesFn, - options?: NodeOptions, -): PromiseLike { - const filesToRead: string[] = []; - - const linesOfContext = - options && options.frameContextLines !== undefined ? options.frameContextLines : DEFAULT_LINES_OF_CONTEXT; - - const frames: StackFrame[] = stack.map(frame => { +export function parseStack(stack: stacktrace.StackFrame[]): StackFrame[] { + return stack.map(frame => { const parsedFrame: StackFrame = { colno: frame.columnNumber, filename: frame.fileName?.startsWith('file://') ? frame.fileName.substr(7) : frame.fileName || '', @@ -105,69 +87,40 @@ export function parseStack( // Extract a module name based on the filename if (parsedFrame.filename) { parsedFrame.module = getModule(parsedFrame.filename); - - if (!isInternal && linesOfContext > 0 && filesToRead.indexOf(parsedFrame.filename) === -1) { - filesToRead.push(parsedFrame.filename); - } } return parsedFrame; }); - - // We do an early return if we do not want to fetch context liens - if (linesOfContext <= 0) { - return SyncPromise.resolve(frames); - } - - if (readFiles) { - try { - return readFiles(filesToRead, frames, linesOfContext); - } catch (_) { - // This happens in electron for example where we are not able to read files from asar. - // So it's fine, we recover be just returning all frames without pre/post context. - } - } - - return SyncPromise.resolve(frames); } /** * @hidden */ -export function getExceptionFromError( - error: Error, - readFiles?: ReadFilesFn, - options?: NodeOptions, -): PromiseLike { +export function getExceptionFromError(error: Error): Exception { const name = error.name || error.constructor.name; const stack = extractStackFromError(error); - return new SyncPromise(resolve => - parseStack(stack, readFiles, options).then(frames => { - const result = { - stacktrace: { - frames: prepareFramesForEvent(frames), - }, - type: name, - value: error.message, - }; - resolve(result); - }), - ); + const frames = parseStack(stack); + + return { + stacktrace: { + frames: prepareFramesForEvent(frames), + }, + type: name, + value: error.message, + }; } /** * @hidden */ -export function parseError(error: ExtendedError, readFiles?: ReadFilesFn, options?: NodeOptions): PromiseLike { - return new SyncPromise(resolve => - getExceptionFromError(error, readFiles, options).then((exception: Exception) => { - resolve({ - exception: { - values: [exception], - }, - }); - }), - ); +export function parseError(error: ExtendedError): Event { + const exception = getExceptionFromError(error); + + return { + exception: { + values: [exception], + }, + }; } /** diff --git a/packages/node/src/sources.ts b/packages/node/src/sources.ts index 42fc97eee72f..f20f72a59233 100644 --- a/packages/node/src/sources.ts +++ b/packages/node/src/sources.ts @@ -3,6 +3,9 @@ import { addContextToFrame, SyncPromise } from '@sentry/utils'; import { readFile } from 'fs'; import { LRUMap } from 'lru_map'; +import { NodeOptions } from './types'; + +const DEFAULT_LINES_OF_CONTEXT: number = 7; const FILE_CONTENT_CACHE = new LRUMap(100); /** @@ -13,13 +16,40 @@ export function resetFileContentCache(): void { FILE_CONTENT_CACHE.clear(); } +/** */ +export function addSourcesToFrames(frames: StackFrame[], options?: NodeOptions): PromiseLike { + const linesOfContext = + options && options.frameContextLines !== undefined ? options.frameContextLines : DEFAULT_LINES_OF_CONTEXT; + + const filesToRead: string[] = []; + + for (const frame of frames) { + if (frame.filename && frame.in_app && filesToRead.indexOf(frame.filename) === -1) { + filesToRead.push(frame.filename); + } + } + + if (linesOfContext <= 0) { + return SyncPromise.resolve(frames); + } + + try { + return readFilesAddPrePostContext(filesToRead, frames, linesOfContext); + } catch (_) { + // This happens in electron for example where we are not able to read files from asar. + // So it's fine, we recover be just returning all frames without pre/post context. + } + + return SyncPromise.resolve(frames); +} + /** * This function tries to read the source files + adding pre and post context (source code) * to a frame. * @param filesToRead string[] of filepaths * @param frames StackFrame[] containg all frames */ -export function readFilesAddPrePostContext( +function readFilesAddPrePostContext( filesToRead: string[], frames: StackFrame[], linesOfContext: number, diff --git a/packages/node/test/parsers.test.ts b/packages/node/test/parsers.test.ts index 3a6859d20a31..e9419059fa9d 100644 --- a/packages/node/test/parsers.test.ts +++ b/packages/node/test/parsers.test.ts @@ -23,10 +23,12 @@ describe('parsers.ts', () => { test('parseStack with same file', done => { expect.assertions(1); let mockCalls = 0; - void Parsers.parseStack(frames, Sources.readFilesAddPrePostContext) + const parsedFrames = Parsers.parseStack(frames); + void Sources.addSourcesToFrames(parsedFrames) .then(_ => { mockCalls = spy.mock.calls.length; - void Parsers.parseStack(frames, Sources.readFilesAddPrePostContext) + const parsedFrames = Parsers.parseStack(frames); + void Sources.addSourcesToFrames(parsedFrames) .then(_1 => { // Calls to readFile shouldn't increase if there isn't a new error expect(spy).toHaveBeenCalledTimes(mockCalls); @@ -55,7 +57,8 @@ describe('parsers.ts', () => { typeName: 'module.exports../src/index.ts', }, ]; - return Parsers.parseStack(framesWithFilePath, Sources.readFilesAddPrePostContext).then(_ => { + const parsedFrames = Parsers.parseStack(framesWithFilePath); + return Sources.addSourcesToFrames(parsedFrames).then(_ => { expect(spy).toHaveBeenCalledTimes(1); }); }); @@ -64,14 +67,17 @@ describe('parsers.ts', () => { expect.assertions(2); let mockCalls = 0; let newErrorCalls = 0; - void Parsers.parseStack(frames, Sources.readFilesAddPrePostContext) + const parsedFrames = Parsers.parseStack(frames); + void Sources.addSourcesToFrames(parsedFrames) .then(_ => { mockCalls = spy.mock.calls.length; - void Parsers.parseStack(stacktrace.parse(getError()), Sources.readFilesAddPrePostContext) + const parsedFrames = Parsers.parseStack(stacktrace.parse(getError())); + void Sources.addSourcesToFrames(parsedFrames) .then(_1 => { newErrorCalls = spy.mock.calls.length; expect(newErrorCalls).toBeGreaterThan(mockCalls); - void Parsers.parseStack(stacktrace.parse(getError()), Sources.readFilesAddPrePostContext) + const parsedFrames = Parsers.parseStack(stacktrace.parse(getError())); + void Sources.addSourcesToFrames(parsedFrames) .then(_2 => { expect(spy).toHaveBeenCalledTimes(newErrorCalls); done(); @@ -121,14 +127,16 @@ describe('parsers.ts', () => { typeName: 'module.exports../src/index.ts', }, ]; - return Parsers.parseStack(framesWithDuplicateFiles, Sources.readFilesAddPrePostContext).then(_ => { + const parsedFrames = Parsers.parseStack(framesWithDuplicateFiles); + return Sources.addSourcesToFrames(parsedFrames).then(_ => { expect(spy).toHaveBeenCalledTimes(1); }); }); test('parseStack with no context', async () => { expect.assertions(1); - return Parsers.parseStack(frames, Sources.readFilesAddPrePostContext, { frameContextLines: 0 }).then(_ => { + const parsedFrames = Parsers.parseStack(frames); + return Sources.addSourcesToFrames(parsedFrames, { frameContextLines: 0 }).then(_ => { expect(spy).toHaveBeenCalledTimes(0); }); }); diff --git a/yarn.lock b/yarn.lock index 724aa12da587..70f3ca5e1f02 100644 --- a/yarn.lock +++ b/yarn.lock @@ -19766,6 +19766,11 @@ tslib@^2.0.0, tslib@^2.0.3: resolved "https://registry.yarnpkg.com/tslib/-/tslib-2.1.0.tgz#da60860f1c2ecaa5703ab7d39bc05b6bf988b97a" integrity sha512-hcVC3wYEziELGGmEEXue7D75zbwIIVUMWAVbHItGPx0ziyXxrOMQx4rQEVEV45Ut/1IotuEvwqPopzIOkDMf0A== +tslib@^2.3.0: + version "2.3.0" + resolved "https://registry.yarnpkg.com/tslib/-/tslib-2.3.0.tgz#803b8cdab3e12ba581a4ca41c8839bbb0dacb09e" + integrity sha512-N82ooyxVNm6h1riLCoyS9e3fuJ3AMG2zIZs2Gd1ATcSFjSA23Q0fzjjZeh0jbJvWVDZ0cJT8yaNNaaXHzueNjg== + tslint-config-prettier@^1.18.0: version "1.18.0" resolved "https://registry.yarnpkg.com/tslint-config-prettier/-/tslint-config-prettier-1.18.0.tgz#75f140bde947d35d8f0d238e0ebf809d64592c37" From 6a03a2cefdde923d313f1536dac3aef488180bc2 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 22 Jun 2021 13:15:03 +0100 Subject: [PATCH 05/25] Improve logic --- packages/node/src/sources.ts | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/packages/node/src/sources.ts b/packages/node/src/sources.ts index f20f72a59233..00e917d8d6bd 100644 --- a/packages/node/src/sources.ts +++ b/packages/node/src/sources.ts @@ -21,18 +21,23 @@ export function addSourcesToFrames(frames: StackFrame[], options?: NodeOptions): const linesOfContext = options && options.frameContextLines !== undefined ? options.frameContextLines : DEFAULT_LINES_OF_CONTEXT; + if (linesOfContext <= 0) { + return SyncPromise.resolve(frames); + } + const filesToRead: string[] = []; for (const frame of frames) { - if (frame.filename && frame.in_app && filesToRead.indexOf(frame.filename) === -1) { + if ( + frame.filename && + filesToRead.indexOf(frame.filename) === -1 && + // We want to include sources for files that are in_app and in node_modules + (frame.in_app || frame.filename.indexOf('node_modules/') !== -1) + ) { filesToRead.push(frame.filename); } } - if (linesOfContext <= 0) { - return SyncPromise.resolve(frames); - } - try { return readFilesAddPrePostContext(filesToRead, frames, linesOfContext); } catch (_) { From 7c164dd88da13e4c60d5566747bf4ade4a34ca58 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 22 Jun 2021 13:26:54 +0100 Subject: [PATCH 06/25] Revert tslib change --- packages/node/package.json | 2 +- yarn.lock | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/packages/node/package.json b/packages/node/package.json index 011114f52a00..331e25052b9c 100644 --- a/packages/node/package.json +++ b/packages/node/package.json @@ -24,7 +24,7 @@ "cookie": "^0.4.1", "https-proxy-agent": "^5.0.0", "lru_map": "^0.3.3", - "tslib": "^2.3.0" + "tslib": "^1.9.3" }, "devDependencies": { "@sentry-internal/eslint-config-sdk": "6.7.1", diff --git a/yarn.lock b/yarn.lock index 70f3ca5e1f02..724aa12da587 100644 --- a/yarn.lock +++ b/yarn.lock @@ -19766,11 +19766,6 @@ tslib@^2.0.0, tslib@^2.0.3: resolved "https://registry.yarnpkg.com/tslib/-/tslib-2.1.0.tgz#da60860f1c2ecaa5703ab7d39bc05b6bf988b97a" integrity sha512-hcVC3wYEziELGGmEEXue7D75zbwIIVUMWAVbHItGPx0ziyXxrOMQx4rQEVEV45Ut/1IotuEvwqPopzIOkDMf0A== -tslib@^2.3.0: - version "2.3.0" - resolved "https://registry.yarnpkg.com/tslib/-/tslib-2.3.0.tgz#803b8cdab3e12ba581a4ca41c8839bbb0dacb09e" - integrity sha512-N82ooyxVNm6h1riLCoyS9e3fuJ3AMG2zIZs2Gd1ATcSFjSA23Q0fzjjZeh0jbJvWVDZ0cJT8yaNNaaXHzueNjg== - tslint-config-prettier@^1.18.0: version "1.18.0" resolved "https://registry.yarnpkg.com/tslint-config-prettier/-/tslint-config-prettier-1.18.0.tgz#75f140bde947d35d8f0d238e0ebf809d64592c37" From f362589e4fa784bd0ad9bb6d91989efef2747d78 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 22 Jun 2021 13:40:16 +0100 Subject: [PATCH 07/25] jsdoc --- packages/node/src/sources.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node/src/sources.ts b/packages/node/src/sources.ts index 00e917d8d6bd..ee13dd6f5153 100644 --- a/packages/node/src/sources.ts +++ b/packages/node/src/sources.ts @@ -16,7 +16,7 @@ export function resetFileContentCache(): void { FILE_CONTENT_CACHE.clear(); } -/** */ +/** Attempts to load sources and add context to the frames */ export function addSourcesToFrames(frames: StackFrame[], options?: NodeOptions): PromiseLike { const linesOfContext = options && options.frameContextLines !== undefined ? options.frameContextLines : DEFAULT_LINES_OF_CONTEXT; From 3324578ca122cc1bc2878d43f80c20e0dc98611c Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 20 Jul 2021 22:45:13 +0100 Subject: [PATCH 08/25] With integration --- packages/node/src/backend.ts | 15 +- .../node/src/integrations/contextlines.ts | 119 ++++++++++++ packages/node/src/integrations/index.ts | 1 + packages/node/src/sdk.ts | 4 +- packages/node/src/sources.ts | 140 -------------- packages/node/src/types.ts | 3 - packages/node/test/index.test.ts | 7 +- packages/node/test/parsers.test.ts | 171 ++++++++---------- 8 files changed, 207 insertions(+), 253 deletions(-) create mode 100644 packages/node/src/integrations/contextlines.ts delete mode 100644 packages/node/src/sources.ts diff --git a/packages/node/src/backend.ts b/packages/node/src/backend.ts index 84f513aba5a8..d4a2dc540ebd 100644 --- a/packages/node/src/backend.ts +++ b/packages/node/src/backend.ts @@ -3,7 +3,6 @@ import { Event, EventHint, Severity, Transport, TransportOptions } from '@sentry import { Dsn } from '@sentry/utils'; import { eventFromException, eventFromMessage } from './eventbuilder'; -import { addSourcesToFrames } from './sources'; import { HTTPSTransport, HTTPTransport } from './transports'; import { NodeOptions } from './types'; @@ -17,24 +16,14 @@ export class NodeBackend extends BaseBackend { */ // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types public eventFromException(exception: any, hint?: EventHint): PromiseLike { - const event = eventFromException(this._options, exception, hint); - - return addSourcesToFrames(event.stacktrace?.frames || [], this._options).then(frames => { - event.stacktrace = { frames }; - return event; - }); + return Promise.resolve(eventFromException(this._options, exception, hint)); } /** * @inheritDoc */ public eventFromMessage(message: string, level: Severity = Severity.Info, hint?: EventHint): PromiseLike { - const event = eventFromMessage(this._options, message, level, hint); - - return addSourcesToFrames(event.stacktrace?.frames || [], this._options).then(frames => { - event.stacktrace = { frames }; - return event; - }); + return Promise.resolve(eventFromMessage(this._options, message, level, hint)); } /** diff --git a/packages/node/src/integrations/contextlines.ts b/packages/node/src/integrations/contextlines.ts new file mode 100644 index 000000000000..ef3a08c54b36 --- /dev/null +++ b/packages/node/src/integrations/contextlines.ts @@ -0,0 +1,119 @@ +import { Event, EventProcessor, Integration, StackFrame } from '@sentry/types'; +import { addContextToFrame } from '@sentry/utils'; +import { readFileSync } from 'fs'; +import { LRUMap } from 'lru_map'; + +const FILE_CONTENT_CACHE = new LRUMap(100); + +/** + * Resets the file cache. Exists for testing purposes. + * @hidden + */ +export function resetFileContentCache(): void { + FILE_CONTENT_CACHE.clear(); +} + +type ContextLinesOptions = { + 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; + + private _linesOfContext: number; + + public constructor(options: ContextLinesOptions = {}) { + this._linesOfContext = options.frameContextLines ?? 7; + } + + /** + * @inheritDoc + */ + public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void): void { + addGlobalEventProcessor(event => this.process(event)); + } + + /** Processes an event and adds context lines */ + public process(event: Event): Event { + console.log(this); + const frames = event.exception?.values?.[0].stacktrace?.frames; + + if (frames && this._linesOfContext > 0) { + const filenames: string[] = []; + + for (const frame of frames) { + if (frame.filename && !filenames.includes(frame.filename)) { + filenames.push(frame.filename); + } + } + + const sourceFiles = 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, this._linesOfContext); + } catch (e) { + // anomaly, being defensive in case + // unlikely to ever happen in practice but can definitely happen in theory + } + } + } + + return event; + } + + return event; + } +} + +/** + * This function reads file contents and caches them in a global LRU cache. + * + * @param filenames Array of filepaths to read content from. + */ +function readSourceFiles(filenames: string[]): Record { + // we're relying on filenames being de-duped already + if (!filenames.length) { + return {}; + } + + const sourceFiles: Record = {}; + + for (const filename of filenames) { + const cache = FILE_CONTENT_CACHE.get(filename); + // We have a cache hit + if (cache !== undefined) { + // If stored value is null, it means that we already tried, but couldn't read the content of the file. Skip. + if (cache === null) { + continue; + } + + // Otherwise content is there, so reuse cached value. + sourceFiles[filename] = cache; + continue; + } + + let content: string | null; + try { + content = readFileSync(filename, 'utf8'); + } catch (_e) { + content = null; + } + + FILE_CONTENT_CACHE.set(filename, content); + sourceFiles[filename] = content; + } + + return sourceFiles; +} diff --git a/packages/node/src/integrations/index.ts b/packages/node/src/integrations/index.ts index 772c0e8f3f86..8017ba458e49 100644 --- a/packages/node/src/integrations/index.ts +++ b/packages/node/src/integrations/index.ts @@ -4,3 +4,4 @@ export { OnUncaughtException } from './onuncaughtexception'; export { OnUnhandledRejection } from './onunhandledrejection'; export { LinkedErrors } from './linkederrors'; export { Modules } from './modules'; +export { ContextLines } from './contextlines'; diff --git a/packages/node/src/sdk.ts b/packages/node/src/sdk.ts index 1cc9ad9de825..fb851f555246 100644 --- a/packages/node/src/sdk.ts +++ b/packages/node/src/sdk.ts @@ -5,7 +5,7 @@ import { getGlobalObject } from '@sentry/utils'; import * as domain from 'domain'; import { NodeClient } from './client'; -import { Console, Http, LinkedErrors, OnUncaughtException, OnUnhandledRejection } from './integrations'; +import { Console, ContextLines, Http, LinkedErrors, OnUncaughtException, OnUnhandledRejection } from './integrations'; import { NodeOptions } from './types'; export const defaultIntegrations = [ @@ -20,6 +20,8 @@ export const defaultIntegrations = [ new OnUnhandledRejection(), // Misc new LinkedErrors(), + // Context Lines + new ContextLines(), ]; /** diff --git a/packages/node/src/sources.ts b/packages/node/src/sources.ts deleted file mode 100644 index ee13dd6f5153..000000000000 --- a/packages/node/src/sources.ts +++ /dev/null @@ -1,140 +0,0 @@ -import { StackFrame } from '@sentry/types'; -import { addContextToFrame, SyncPromise } from '@sentry/utils'; -import { readFile } from 'fs'; -import { LRUMap } from 'lru_map'; - -import { NodeOptions } from './types'; - -const DEFAULT_LINES_OF_CONTEXT: number = 7; -const FILE_CONTENT_CACHE = new LRUMap(100); - -/** - * Resets the file cache. Exists for testing purposes. - * @hidden - */ -export function resetFileContentCache(): void { - FILE_CONTENT_CACHE.clear(); -} - -/** Attempts to load sources and add context to the frames */ -export function addSourcesToFrames(frames: StackFrame[], options?: NodeOptions): PromiseLike { - const linesOfContext = - options && options.frameContextLines !== undefined ? options.frameContextLines : DEFAULT_LINES_OF_CONTEXT; - - if (linesOfContext <= 0) { - return SyncPromise.resolve(frames); - } - - const filesToRead: string[] = []; - - for (const frame of frames) { - if ( - frame.filename && - filesToRead.indexOf(frame.filename) === -1 && - // We want to include sources for files that are in_app and in node_modules - (frame.in_app || frame.filename.indexOf('node_modules/') !== -1) - ) { - filesToRead.push(frame.filename); - } - } - - try { - return readFilesAddPrePostContext(filesToRead, frames, linesOfContext); - } catch (_) { - // This happens in electron for example where we are not able to read files from asar. - // So it's fine, we recover be just returning all frames without pre/post context. - } - - return SyncPromise.resolve(frames); -} - -/** - * This function tries to read the source files + adding pre and post context (source code) - * to a frame. - * @param filesToRead string[] of filepaths - * @param frames StackFrame[] containg all frames - */ -function readFilesAddPrePostContext( - filesToRead: string[], - frames: StackFrame[], - linesOfContext: number, -): PromiseLike { - return new SyncPromise(resolve => - readSourceFiles(filesToRead).then(sourceFiles => { - const result = frames.map(frame => { - if (frame.filename && sourceFiles[frame.filename]) { - try { - const lines = (sourceFiles[frame.filename] as string).split('\n'); - - addContextToFrame(lines, frame, linesOfContext); - } catch (e) { - // anomaly, being defensive in case - // unlikely to ever happen in practice but can definitely happen in theory - } - } - return frame; - }); - - resolve(result); - }), - ); -} - -/** - * This function reads file contents and caches them in a global LRU cache. - * Returns a Promise filepath => content array for all files that we were able to read. - * - * @param filenames Array of filepaths to read content from. - */ -function readSourceFiles(filenames: string[]): PromiseLike<{ [key: string]: string | null }> { - // we're relying on filenames being de-duped already - if (filenames.length === 0) { - return SyncPromise.resolve({}); - } - - return new SyncPromise<{ - [key: string]: string | null; - }>(resolve => { - const sourceFiles: { - [key: string]: string | null; - } = {}; - - let count = 0; - // eslint-disable-next-line @typescript-eslint/prefer-for-of - for (let i = 0; i < filenames.length; i++) { - const filename = filenames[i]; - - const cache = FILE_CONTENT_CACHE.get(filename); - // We have a cache hit - if (cache !== undefined) { - // If it's not null (which means we found a file and have a content) - // we set the content and return it later. - if (cache !== null) { - sourceFiles[filename] = cache; - } - // eslint-disable-next-line no-plusplus - count++; - // In any case we want to skip here then since we have a content already or we couldn't - // read the file and don't want to try again. - if (count === filenames.length) { - resolve(sourceFiles); - } - continue; - } - - readFile(filename, (err: Error | null, data: Buffer) => { - const content = err ? null : data.toString(); - sourceFiles[filename] = content; - - // We always want to set the cache, even to null which means there was an error reading the file. - // We do not want to try to read the file again. - FILE_CONTENT_CACHE.set(filename, content); - // eslint-disable-next-line no-plusplus - count++; - if (count === filenames.length) { - resolve(sourceFiles); - } - }); - } - }); -} diff --git a/packages/node/src/types.ts b/packages/node/src/types.ts index d9fec7f4c13e..055006a47e9e 100644 --- a/packages/node/src/types.ts +++ b/packages/node/src/types.ts @@ -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; } diff --git a/packages/node/test/index.test.ts b/packages/node/test/index.test.ts index 341c3351bb9d..76d9fb5896e4 100644 --- a/packages/node/test/index.test.ts +++ b/packages/node/test/index.test.ts @@ -167,7 +167,7 @@ describe('SentryNode', () => { } }); - test('capture an exception no pre/post context', done => { + test('capture an exception with pre/post context', done => { expect.assertions(10); getCurrentHub().bindClient( new NodeClient({ @@ -177,8 +177,8 @@ describe('SentryNode', () => { 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(event.exception!.values![0].stacktrace!.frames![2].pre_context).toBeUndefined(); - expect(event.exception!.values![0].stacktrace!.frames![2].post_context).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].type).toBe('Error'); expect(event.exception!.values![0].value).toBe('test'); expect(event.exception!.values![0].stacktrace).toBeTruthy(); @@ -186,7 +186,6 @@ describe('SentryNode', () => { return null; }, dsn, - frameContextLines: 0, }), ); configureScope((scope: Scope) => { diff --git a/packages/node/test/parsers.test.ts b/packages/node/test/parsers.test.ts index e9419059fa9d..d205a82cc3b3 100644 --- a/packages/node/test/parsers.test.ts +++ b/packages/node/test/parsers.test.ts @@ -1,18 +1,25 @@ +import { StackFrame } from '@sentry/types'; import * as fs from 'fs'; +import { ContextLines, resetFileContentCache } from '../src/integrations/contextlines'; import * as Parsers from '../src/parsers'; -import * as Sources from '../src/sources'; import * as stacktrace from '../src/stacktrace'; import { getError } from './helper/error'; describe('parsers.ts', () => { let frames: stacktrace.StackFrame[]; let spy: jest.SpyInstance; + let contextLines: ContextLines; + + function addContext(frames: StackFrame[]) { + contextLines.process({ exception: { values: [{ stacktrace: { frames } }] } }); + } beforeEach(() => { - spy = jest.spyOn(fs, 'readFile'); + spy = jest.spyOn(fs, 'readFileSync'); frames = stacktrace.parse(new Error('test')); - Sources.resetFileContentCache(); + contextLines = new ContextLines(); + resetFileContentCache(); }); afterEach(() => { @@ -20,30 +27,22 @@ describe('parsers.ts', () => { }); describe('lru file cache', () => { - test('parseStack with same file', done => { + test('parseStack with same file', () => { expect.assertions(1); + let mockCalls = 0; - const parsedFrames = Parsers.parseStack(frames); - void Sources.addSourcesToFrames(parsedFrames) - .then(_ => { - mockCalls = spy.mock.calls.length; - const parsedFrames = Parsers.parseStack(frames); - void Sources.addSourcesToFrames(parsedFrames) - .then(_1 => { - // Calls to readFile shouldn't increase if there isn't a new error - expect(spy).toHaveBeenCalledTimes(mockCalls); - done(); - }) - .then(null, () => { - // no-empty - }); - }) - .then(null, () => { - // no-empty - }); + let parsedFrames = Parsers.parseStack(frames); + addContext(parsedFrames); + + mockCalls = spy.mock.calls.length; + parsedFrames = Parsers.parseStack(frames); + 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', async () => { + test('parseStack with ESM module names', () => { expect.assertions(1); const framesWithFilePath: stacktrace.StackFrame[] = [ @@ -58,85 +57,73 @@ describe('parsers.ts', () => { }, ]; const parsedFrames = Parsers.parseStack(framesWithFilePath); - return Sources.addSourcesToFrames(parsedFrames).then(_ => { - expect(spy).toHaveBeenCalledTimes(1); - }); + addContext(parsedFrames); + expect(spy).toHaveBeenCalledTimes(1); }); - test('parseStack with adding different file', done => { + test('parseStack with adding different file', () => { expect.assertions(2); let mockCalls = 0; let newErrorCalls = 0; - const parsedFrames = Parsers.parseStack(frames); - void Sources.addSourcesToFrames(parsedFrames) - .then(_ => { - mockCalls = spy.mock.calls.length; - const parsedFrames = Parsers.parseStack(stacktrace.parse(getError())); - void Sources.addSourcesToFrames(parsedFrames) - .then(_1 => { - newErrorCalls = spy.mock.calls.length; - expect(newErrorCalls).toBeGreaterThan(mockCalls); - const parsedFrames = Parsers.parseStack(stacktrace.parse(getError())); - void Sources.addSourcesToFrames(parsedFrames) - .then(_2 => { - expect(spy).toHaveBeenCalledTimes(newErrorCalls); - done(); - }) - .then(null, () => { - // no-empty - }); - }) - .then(null, () => { - // no-empty - }); - }) - .then(null, () => { - // no-empty - }); + let parsedFrames = Parsers.parseStack(frames); + addContext(parsedFrames); + + mockCalls = spy.mock.calls.length; + parsedFrames = Parsers.parseStack(stacktrace.parse(getError())); + addContext(parsedFrames); + + newErrorCalls = spy.mock.calls.length; + expect(newErrorCalls).toBeGreaterThan(mockCalls); + + parsedFrames = Parsers.parseStack(stacktrace.parse(getError())); + addContext(parsedFrames); + + expect(spy).toHaveBeenCalledTimes(newErrorCalls); }); - }); - test('parseStack with duplicate files', async () => { - expect.assertions(1); - const framesWithDuplicateFiles: stacktrace.StackFrame[] = [ - { - columnNumber: 1, - fileName: '/var/task/index.js', - functionName: 'module.exports../src/index.ts.fxn1', - lineNumber: 1, - methodName: 'fxn1', - native: false, - typeName: 'module.exports../src/index.ts', - }, - { - columnNumber: 2, - fileName: '/var/task/index.js', - functionName: 'module.exports../src/index.ts.fxn2', - lineNumber: 2, - methodName: 'fxn2', - native: false, - typeName: 'module.exports../src/index.ts', - }, - { - columnNumber: 3, - fileName: '/var/task/index.js', - functionName: 'module.exports../src/index.ts.fxn3', - lineNumber: 3, - methodName: 'fxn3', - native: false, - typeName: 'module.exports../src/index.ts', - }, - ]; - const parsedFrames = Parsers.parseStack(framesWithDuplicateFiles); - return Sources.addSourcesToFrames(parsedFrames).then(_ => { + test('parseStack with duplicate files', () => { + expect.assertions(1); + const framesWithDuplicateFiles: stacktrace.StackFrame[] = [ + { + columnNumber: 1, + fileName: '/var/task/index.js', + functionName: 'module.exports../src/index.ts.fxn1', + lineNumber: 1, + methodName: 'fxn1', + native: false, + typeName: 'module.exports../src/index.ts', + }, + { + columnNumber: 2, + fileName: '/var/task/index.js', + functionName: 'module.exports../src/index.ts.fxn2', + lineNumber: 2, + methodName: 'fxn2', + native: false, + typeName: 'module.exports../src/index.ts', + }, + { + columnNumber: 3, + fileName: '/var/task/index.js', + functionName: 'module.exports../src/index.ts.fxn3', + lineNumber: 3, + methodName: 'fxn3', + native: false, + typeName: 'module.exports../src/index.ts', + }, + ]; + + const parsedFrames = Parsers.parseStack(framesWithDuplicateFiles); + addContext(parsedFrames); expect(spy).toHaveBeenCalledTimes(1); }); - }); - test('parseStack with no context', async () => { - expect.assertions(1); - const parsedFrames = Parsers.parseStack(frames); - return Sources.addSourcesToFrames(parsedFrames, { frameContextLines: 0 }).then(_ => { + test('parseStack with no context', () => { + contextLines = new ContextLines({ frameContextLines: 0 }); + + expect.assertions(1); + const parsedFrames = Parsers.parseStack(frames); + addContext(parsedFrames); expect(spy).toHaveBeenCalledTimes(0); }); }); From 443ea06d8b26237d30b10838c73b1ab98475f1b2 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 20 Jul 2021 22:52:00 +0100 Subject: [PATCH 09/25] oops --- packages/node/src/integrations/contextlines.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/node/src/integrations/contextlines.ts b/packages/node/src/integrations/contextlines.ts index ef3a08c54b36..f4805f38c28b 100644 --- a/packages/node/src/integrations/contextlines.ts +++ b/packages/node/src/integrations/contextlines.ts @@ -1,4 +1,4 @@ -import { Event, EventProcessor, Integration, StackFrame } from '@sentry/types'; +import { Event, EventProcessor, Integration } from '@sentry/types'; import { addContextToFrame } from '@sentry/utils'; import { readFileSync } from 'fs'; import { LRUMap } from 'lru_map'; @@ -44,7 +44,6 @@ export class ContextLines implements Integration { /** Processes an event and adds context lines */ public process(event: Event): Event { - console.log(this); const frames = event.exception?.values?.[0].stacktrace?.frames; if (frames && this._linesOfContext > 0) { From 84155de8d024a4b5234cf748b7a3c495576883e0 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 10 Aug 2021 22:51:43 +0100 Subject: [PATCH 10/25] Make this a non-breaking change by keeping NodeOptions.frameContextLines --- .../node/src/integrations/contextlines.ts | 21 +++++++++++++------ packages/node/src/types.ts | 3 +++ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/packages/node/src/integrations/contextlines.ts b/packages/node/src/integrations/contextlines.ts index f4805f38c28b..e25433033b84 100644 --- a/packages/node/src/integrations/contextlines.ts +++ b/packages/node/src/integrations/contextlines.ts @@ -1,8 +1,11 @@ +import { getCurrentHub } from '@sentry/core'; import { Event, EventProcessor, Integration } from '@sentry/types'; import { addContextToFrame } from '@sentry/utils'; import { readFileSync } from 'fs'; import { LRUMap } from 'lru_map'; +import { NodeClient } from '../client'; + const FILE_CONTENT_CACHE = new LRUMap(100); /** @@ -29,24 +32,30 @@ export class ContextLines implements Integration { */ public name: string = ContextLines.id; - private _linesOfContext: number; + private _linesOfContext?: number; public constructor(options: ContextLinesOptions = {}) { - this._linesOfContext = options.frameContextLines ?? 7; + this._linesOfContext = options.frameContextLines; } /** * @inheritDoc */ public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void): void { - addGlobalEventProcessor(event => this.process(event)); + const optLinesOfContext = getCurrentHub() + .getClient() + ?.getOptions()?.frameContextLines; + + addGlobalEventProcessor(event => this.process(event, optLinesOfContext)); } /** Processes an event and adds context lines */ - public process(event: Event): Event { + public process(event: Event, optLinesOfContext?: number): Event { + const contextLines = optLinesOfContext || this._linesOfContext || 7; + const frames = event.exception?.values?.[0].stacktrace?.frames; - if (frames && this._linesOfContext > 0) { + if (frames && contextLines > 0) { const filenames: string[] = []; for (const frame of frames) { @@ -61,7 +70,7 @@ export class ContextLines implements Integration { if (frame.filename && sourceFiles[frame.filename]) { try { const lines = (sourceFiles[frame.filename] as string).split('\n'); - addContextToFrame(lines, frame, this._linesOfContext); + addContextToFrame(lines, frame, contextLines); } catch (e) { // anomaly, being defensive in case // unlikely to ever happen in practice but can definitely happen in theory diff --git a/packages/node/src/types.ts b/packages/node/src/types.ts index 055006a47e9e..d9fec7f4c13e 100644 --- a/packages/node/src/types.ts +++ b/packages/node/src/types.ts @@ -20,6 +20,9 @@ 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; } From ac4318e0d31d31e9de407b4d6c41c913ed2c3321 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 1 Dec 2021 21:39:12 +0000 Subject: [PATCH 11/25] Correctly handle zero lines of context --- packages/node/src/integrations/contextlines.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/node/src/integrations/contextlines.ts b/packages/node/src/integrations/contextlines.ts index e25433033b84..e92e47dc584f 100644 --- a/packages/node/src/integrations/contextlines.ts +++ b/packages/node/src/integrations/contextlines.ts @@ -51,7 +51,8 @@ export class ContextLines implements Integration { /** Processes an event and adds context lines */ public process(event: Event, optLinesOfContext?: number): Event { - const contextLines = optLinesOfContext || this._linesOfContext || 7; + const contextLines = + optLinesOfContext != undefined ? optLinesOfContext : this._linesOfContext != undefined ? this._linesOfContext : 7; const frames = event.exception?.values?.[0].stacktrace?.frames; From 86e2d4eff99a01382f49c28f14edaa88d72c49ff Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 1 Dec 2021 22:17:51 +0000 Subject: [PATCH 12/25] Make async --- .../node/src/integrations/contextlines.ts | 47 +++++++++---------- packages/node/src/types.ts | 3 -- packages/node/test/index.test.ts | 6 +-- packages/node/test/parsers.test.ts | 32 ++++++------- 4 files changed, 41 insertions(+), 47 deletions(-) diff --git a/packages/node/src/integrations/contextlines.ts b/packages/node/src/integrations/contextlines.ts index e92e47dc584f..ce6de15ad8e3 100644 --- a/packages/node/src/integrations/contextlines.ts +++ b/packages/node/src/integrations/contextlines.ts @@ -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(100); +function readTextFileAsync(path: string): Promise { + 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 @@ -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 { @@ -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() - ?.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 { + const contextLines = this._options.frameContextLines != undefined ? this._options.frameContextLines : 7; const frames = event.exception?.values?.[0].stacktrace?.frames; @@ -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]) { @@ -91,7 +88,7 @@ export class ContextLines implements Integration { * * @param filenames Array of filepaths to read content from. */ -function readSourceFiles(filenames: string[]): Record { +async function readSourceFiles(filenames: string[]): Promise> { // we're relying on filenames being de-duped already if (!filenames.length) { return {}; @@ -113,11 +110,11 @@ function readSourceFiles(filenames: string[]): Record { 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); diff --git a/packages/node/src/types.ts b/packages/node/src/types.ts index d9fec7f4c13e..055006a47e9e 100644 --- a/packages/node/src/types.ts +++ b/packages/node/src/types.ts @@ -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; } diff --git a/packages/node/test/index.test.ts b/packages/node/test/index.test.ts index c976a51fed7f..3a16c07fd552 100644 --- a/packages/node/test/index.test.ts +++ b/packages/node/test/index.test.ts @@ -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(); diff --git a/packages/node/test/parsers.test.ts b/packages/node/test/parsers.test.ts index d205a82cc3b3..8e8618148b15 100644 --- a/packages/node/test/parsers.test.ts +++ b/packages/node/test/parsers.test.ts @@ -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 { + 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(); @@ -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[] = [ @@ -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[] = [ { @@ -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); }); }); From af9b1d82a6776897ef1be4dd7d844119e2dbe47d Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 8 Feb 2022 21:16:18 +0000 Subject: [PATCH 13/25] No longer a breaking change --- packages/node/src/integrations/contextlines.ts | 10 +++++++++- packages/node/src/types.ts | 3 +++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/node/src/integrations/contextlines.ts b/packages/node/src/integrations/contextlines.ts index ce6de15ad8e3..473f0f74302f 100644 --- a/packages/node/src/integrations/contextlines.ts +++ b/packages/node/src/integrations/contextlines.ts @@ -1,8 +1,11 @@ +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(100); function readTextFileAsync(path: string): Promise { @@ -44,12 +47,17 @@ export class ContextLines implements Integration { * @inheritDoc */ public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void): void { + if (this._options.frameContextLines == undefined) { + const initOptions = getCurrentHub().getClient()?.getOptions(); + this._options.frameContextLines = initOptions?.frameContextLines; + } + addGlobalEventProcessor(event => this.addToEvent(event)); } /** Processes an event and adds context lines */ public async addToEvent(event: Event): Promise { - const contextLines = this._options.frameContextLines != undefined ? this._options.frameContextLines : 7; + const contextLines = this._options.frameContextLines == undefined ? 7 : this._options.frameContextLines; const frames = event.exception?.values?.[0].stacktrace?.frames; diff --git a/packages/node/src/types.ts b/packages/node/src/types.ts index 055006a47e9e..d9fec7f4c13e 100644 --- a/packages/node/src/types.ts +++ b/packages/node/src/types.ts @@ -20,6 +20,9 @@ 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; } From 270e13e758e231a2971d607a11b6fd48c44b99dd Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 8 Feb 2022 23:29:28 +0000 Subject: [PATCH 14/25] Fix linting --- packages/node/src/eventbuilder.ts | 170 +++++------ .../node/src/integrations/contextlines.ts | 266 +++++++++--------- 2 files changed, 218 insertions(+), 218 deletions(-) diff --git a/packages/node/src/eventbuilder.ts b/packages/node/src/eventbuilder.ts index 055029c199f3..28bd6190de1e 100644 --- a/packages/node/src/eventbuilder.ts +++ b/packages/node/src/eventbuilder.ts @@ -1,85 +1,85 @@ -import { getCurrentHub } from '@sentry/hub'; -import { Event, EventHint, Mechanism, Options, Severity } from '@sentry/types'; -import { - addExceptionMechanism, - addExceptionTypeValue, - extractExceptionKeysForMessage, - isError, - isPlainObject, - normalizeToSize, -} from '@sentry/utils'; - -import { extractStackFromError, parseError, parseStack, prepareFramesForEvent } from './parsers'; - -/** - * Builds and Event from a Exception - * @hidden - */ -export function eventFromException(exception: unknown, hint?: EventHint): Event { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - let ex: any = exception; - const providedMechanism: Mechanism | undefined = - hint && hint.data && (hint.data as { mechanism: Mechanism }).mechanism; - const mechanism: Mechanism = providedMechanism || { - handled: true, - type: 'generic', - }; - - if (!isError(exception)) { - if (isPlainObject(exception)) { - // This will allow us to group events based on top-level keys - // which is much better than creating new group when any key/value change - const message = `Non-Error exception captured with keys: ${extractExceptionKeysForMessage(exception)}`; - - getCurrentHub().configureScope(scope => { - scope.setExtra('__serialized__', normalizeToSize(exception as Record)); - }); - - ex = (hint && hint.syntheticException) || new Error(message); - (ex as Error).message = message; - } else { - // This handles when someone does: `throw "something awesome";` - // We use synthesized Error here so we can extract a (rough) stack trace. - ex = (hint && hint.syntheticException) || new Error(exception as string); - (ex as Error).message = exception as string; - } - mechanism.synthetic = true; - } - - const event = parseError(ex as Error); - addExceptionTypeValue(event, undefined, undefined); - addExceptionMechanism(event, mechanism); - - return { - ...event, - event_id: hint && hint.event_id, - }; -} - -/** - * Builds and Event from a Message - * @hidden - */ -export function eventFromMessage( - options: Options, - message: string, - level: Severity = Severity.Info, - hint?: EventHint, -): Event { - const event: Event = { - event_id: hint && hint.event_id, - level, - message, - }; - - if (options.attachStacktrace && hint && hint.syntheticException) { - const stack = hint.syntheticException ? extractStackFromError(hint.syntheticException) : []; - const frames = parseStack(stack); - - event.stacktrace = { - frames: prepareFramesForEvent(frames), - }; - } - - return event; -} +import { getCurrentHub } from '@sentry/hub'; +import { Event, EventHint, Mechanism, Options, Severity } from '@sentry/types'; +import { + addExceptionMechanism, + addExceptionTypeValue, + extractExceptionKeysForMessage, + isError, + isPlainObject, + normalizeToSize, +} from '@sentry/utils'; + +import { extractStackFromError, parseError, parseStack, prepareFramesForEvent } from './parsers'; + +/** + * Builds and Event from a Exception + * @hidden + */ +export function eventFromException(exception: unknown, hint?: EventHint): Event { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let ex: any = exception; + const providedMechanism: Mechanism | undefined = + hint && hint.data && (hint.data as { mechanism: Mechanism }).mechanism; + const mechanism: Mechanism = providedMechanism || { + handled: true, + type: 'generic', + }; + + if (!isError(exception)) { + if (isPlainObject(exception)) { + // This will allow us to group events based on top-level keys + // which is much better than creating new group when any key/value change + const message = `Non-Error exception captured with keys: ${extractExceptionKeysForMessage(exception)}`; + + getCurrentHub().configureScope(scope => { + scope.setExtra('__serialized__', normalizeToSize(exception as Record)); + }); + + ex = (hint && hint.syntheticException) || new Error(message); + (ex as Error).message = message; + } else { + // This handles when someone does: `throw "something awesome";` + // We use synthesized Error here so we can extract a (rough) stack trace. + ex = (hint && hint.syntheticException) || new Error(exception as string); + (ex as Error).message = exception as string; + } + mechanism.synthetic = true; + } + + const event = parseError(ex as Error); + addExceptionTypeValue(event, undefined, undefined); + addExceptionMechanism(event, mechanism); + + return { + ...event, + event_id: hint && hint.event_id, + }; +} + +/** + * Builds and Event from a Message + * @hidden + */ +export function eventFromMessage( + options: Options, + message: string, + level: Severity = Severity.Info, + hint?: EventHint, +): Event { + const event: Event = { + event_id: hint && hint.event_id, + level, + message, + }; + + if (options.attachStacktrace && hint && hint.syntheticException) { + const stack = hint.syntheticException ? extractStackFromError(hint.syntheticException) : []; + const frames = parseStack(stack); + + event.stacktrace = { + frames: prepareFramesForEvent(frames), + }; + } + + return event; +} diff --git a/packages/node/src/integrations/contextlines.ts b/packages/node/src/integrations/contextlines.ts index 473f0f74302f..c9bfbd4f622e 100644 --- a/packages/node/src/integrations/contextlines.ts +++ b/packages/node/src/integrations/contextlines.ts @@ -1,133 +1,133 @@ -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(100); - -function readTextFileAsync(path: string): Promise { - 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 { - 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 { - if (this._options.frameContextLines == undefined) { - const initOptions = getCurrentHub().getClient()?.getOptions(); - this._options.frameContextLines = initOptions?.frameContextLines; - } - - addGlobalEventProcessor(event => this.addToEvent(event)); - } - - /** Processes an event and adds context lines */ - public async addToEvent(event: Event): Promise { - const contextLines = this._options.frameContextLines == undefined ? 7 : this._options.frameContextLines; - - const frames = event.exception?.values?.[0].stacktrace?.frames; - - if (frames && contextLines > 0) { - const filenames: string[] = []; - - for (const frame of frames) { - if (frame.filename && !filenames.includes(frame.filename)) { - filenames.push(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; - } - - 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: string[]): Promise> { - // we're relying on filenames being de-duped already - if (!filenames.length) { - return {}; - } - - const sourceFiles: Record = {}; - - for (const filename of filenames) { - const cache = FILE_CONTENT_CACHE.get(filename); - // We have a cache hit - if (cache !== undefined) { - // If stored value is null, it means that we already tried, but couldn't read the content of the file. Skip. - if (cache === null) { - continue; - } - - // Otherwise content is there, so reuse cached value. - sourceFiles[filename] = cache; - continue; - } - - let content: string | null = null; - try { - content = await readTextFileAsync(filename); - } catch (_) { - // - } - - FILE_CONTENT_CACHE.set(filename, content); - sourceFiles[filename] = content; - } - - return sourceFiles; -} +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(100); + +function readTextFileAsync(path: string): Promise { + 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 { + 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 { + if (this._options.frameContextLines == undefined) { + const initOptions = getCurrentHub().getClient()?.getOptions(); + this._options.frameContextLines = initOptions?.frameContextLines; + } + + addGlobalEventProcessor(event => this.addToEvent(event)); + } + + /** Processes an event and adds context lines */ + public async addToEvent(event: Event): Promise { + const contextLines = this._options.frameContextLines == undefined ? 7 : this._options.frameContextLines; + + const frames = event.exception?.values?.[0].stacktrace?.frames; + + if (frames && contextLines > 0) { + const filenames: string[] = []; + + for (const frame of frames) { + if (frame.filename && !filenames.includes(frame.filename)) { + filenames.push(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; + } + + 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: string[]): Promise> { + // we're relying on filenames being de-duped already + if (!filenames.length) { + return {}; + } + + const sourceFiles: Record = {}; + + for (const filename of filenames) { + const cache = FILE_CONTENT_CACHE.get(filename); + // We have a cache hit + if (cache !== undefined) { + // If stored value is null, it means that we already tried, but couldn't read the content of the file. Skip. + if (cache === null) { + continue; + } + + // Otherwise content is there, so reuse cached value. + sourceFiles[filename] = cache; + continue; + } + + let content: string | null = null; + try { + content = await readTextFileAsync(filename); + } catch (_) { + // + } + + FILE_CONTENT_CACHE.set(filename, content); + sourceFiles[filename] = content; + } + + return sourceFiles; +} From ddb8406e93e0fb56127120f509301a332a70f94e Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 14 Feb 2022 14:08:16 +0000 Subject: [PATCH 15/25] Add docs and `@deprecated` --- packages/node/src/integrations/contextlines.ts | 5 +++++ packages/node/src/types.ts | 14 +++++++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/packages/node/src/integrations/contextlines.ts b/packages/node/src/integrations/contextlines.ts index c9bfbd4f622e..12d2a67dcb90 100644 --- a/packages/node/src/integrations/contextlines.ts +++ b/packages/node/src/integrations/contextlines.ts @@ -26,6 +26,11 @@ export function resetFileContentCache(): void { } interface ContextLinesOptions { + /** + * Sets the number of context lines for each frame when loading a file + * + * Set to 0 to disable loading and inclusion of source files. + * */ frameContextLines?: number; } diff --git a/packages/node/src/types.ts b/packages/node/src/types.ts index d9fec7f4c13e..677e40214e0d 100644 --- a/packages/node/src/types.ts +++ b/packages/node/src/types.ts @@ -20,7 +20,19 @@ export interface NodeOptions extends Options { /** HTTPS proxy certificates path */ caCerts?: string; - /** Sets the number of context lines for each frame when loading a file. */ + /** + * Sets the number of context lines for each frame when loading a file. + * + * @deprecated Context lines configuration has moved to the `ContextLines` integration. + * + * ``` + * init({ + * dsn: '__DSN__', + * integrations: [new ContextLines({ frameContextLines: 10 })] + * }) + * ``` + * + * */ frameContextLines?: number; /** Callback that is executed when a fatal global error occurs. */ From 83f859b1747eae2a4972c0375369e5f148611279 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 14 Feb 2022 14:35:13 +0000 Subject: [PATCH 16/25] Disable warning for internal usage of deprecated field --- packages/node/src/integrations/contextlines.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/node/src/integrations/contextlines.ts b/packages/node/src/integrations/contextlines.ts index 12d2a67dcb90..1cbd7f8bf05c 100644 --- a/packages/node/src/integrations/contextlines.ts +++ b/packages/node/src/integrations/contextlines.ts @@ -54,6 +54,7 @@ export class ContextLines implements Integration { public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void): void { if (this._options.frameContextLines == undefined) { const initOptions = getCurrentHub().getClient()?.getOptions(); + // eslint-disable-next-line deprecation/deprecation this._options.frameContextLines = initOptions?.frameContextLines; } From 8bc20b028f5063db55a830b49eed23becbedefff Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 14 Feb 2022 21:51:11 +0000 Subject: [PATCH 17/25] Revert promise changes --- packages/node/src/backend.ts | 4 +- packages/node/src/eventbuilder.ts | 46 +++++++++++-------- .../node/src/integrations/linkederrors.ts | 11 +++-- packages/node/src/parsers.ts | 42 +++++++++-------- 4 files changed, 60 insertions(+), 43 deletions(-) diff --git a/packages/node/src/backend.ts b/packages/node/src/backend.ts index bb1dac45eb56..707d0bec9324 100644 --- a/packages/node/src/backend.ts +++ b/packages/node/src/backend.ts @@ -16,14 +16,14 @@ export class NodeBackend extends BaseBackend { */ // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types public eventFromException(exception: any, hint?: EventHint): PromiseLike { - return Promise.resolve(eventFromException(exception, hint)); + return eventFromException(exception, hint); } /** * @inheritDoc */ public eventFromMessage(message: string, level: Severity = Severity.Info, hint?: EventHint): PromiseLike { - return Promise.resolve(eventFromMessage(this._options, message, level, hint)); + return eventFromMessage(this._options, message, level, hint); } /** diff --git a/packages/node/src/eventbuilder.ts b/packages/node/src/eventbuilder.ts index 28bd6190de1e..3959e8f6daa2 100644 --- a/packages/node/src/eventbuilder.ts +++ b/packages/node/src/eventbuilder.ts @@ -7,6 +7,7 @@ import { isError, isPlainObject, normalizeToSize, + SyncPromise, } from '@sentry/utils'; import { extractStackFromError, parseError, parseStack, prepareFramesForEvent } from './parsers'; @@ -15,7 +16,7 @@ import { extractStackFromError, parseError, parseStack, prepareFramesForEvent } * Builds and Event from a Exception * @hidden */ -export function eventFromException(exception: unknown, hint?: EventHint): Event { +export function eventFromException(exception: unknown, hint?: EventHint): PromiseLike { // eslint-disable-next-line @typescript-eslint/no-explicit-any let ex: any = exception; const providedMechanism: Mechanism | undefined = @@ -46,14 +47,19 @@ export function eventFromException(exception: unknown, hint?: EventHint): Event mechanism.synthetic = true; } - const event = parseError(ex as Error); - addExceptionTypeValue(event, undefined, undefined); - addExceptionMechanism(event, mechanism); + return new SyncPromise((resolve, reject) => + parseError(ex as Error) + .then(event => { + addExceptionTypeValue(event, undefined, undefined); + addExceptionMechanism(event, mechanism); - return { - ...event, - event_id: hint && hint.event_id, - }; + resolve({ + ...event, + event_id: hint && hint.event_id, + }); + }) + .then(null, reject), + ); } /** @@ -65,21 +71,23 @@ export function eventFromMessage( message: string, level: Severity = Severity.Info, hint?: EventHint, -): Event { +): PromiseLike { const event: Event = { event_id: hint && hint.event_id, level, message, }; - if (options.attachStacktrace && hint && hint.syntheticException) { - const stack = hint.syntheticException ? extractStackFromError(hint.syntheticException) : []; - const frames = parseStack(stack); - - event.stacktrace = { - frames: prepareFramesForEvent(frames), - }; - } - - return event; + return new SyncPromise(resolve => { + if (options.attachStacktrace && hint && hint.syntheticException) { + const stack = hint.syntheticException ? extractStackFromError(hint.syntheticException) : []; + const frames = parseStack(stack); + event.stacktrace = { + frames: prepareFramesForEvent(frames), + }; + resolve(event); + } else { + resolve(event); + } + }); } diff --git a/packages/node/src/integrations/linkederrors.ts b/packages/node/src/integrations/linkederrors.ts index 87e9de3af4d6..0b70aad73360 100644 --- a/packages/node/src/integrations/linkederrors.ts +++ b/packages/node/src/integrations/linkederrors.ts @@ -81,9 +81,14 @@ export class LinkedErrors implements Integration { return resolvedSyncPromise(stack); } return new SyncPromise((resolve, reject) => { - const exception = getExceptionFromError(error[key]); - void this._walkErrorTree(error[key], key, [exception, ...stack]) - .then(resolve) + void getExceptionFromError(error[key]) + .then((exception: Exception) => { + void this._walkErrorTree(error[key], key, [exception, ...stack]) + .then(resolve) + .then(null, () => { + reject(); + }); + }) .then(null, () => { reject(); }); diff --git a/packages/node/src/parsers.ts b/packages/node/src/parsers.ts index 29e027387bf1..835dfa33ec9f 100644 --- a/packages/node/src/parsers.ts +++ b/packages/node/src/parsers.ts @@ -1,5 +1,5 @@ import { Event, Exception, ExtendedError, StackFrame } from '@sentry/types'; -import { basename, dirname } from '@sentry/utils'; +import { basename, dirname, SyncPromise } from '@sentry/utils'; import * as stacktrace from './stacktrace'; @@ -97,31 +97,35 @@ export function parseStack(stack: stacktrace.StackFrame[]): StackFrame[] { /** * @hidden */ -export function getExceptionFromError(error: Error): Exception { +export function getExceptionFromError(error: Error): PromiseLike { const name = error.name || error.constructor.name; const stack = extractStackFromError(error); - const frames = parseStack(stack); - - return { - stacktrace: { - frames: prepareFramesForEvent(frames), - }, - type: name, - value: error.message, - }; + return new SyncPromise(resolve => { + const frames = parseStack(stack); + const result = { + stacktrace: { + frames: prepareFramesForEvent(frames), + }, + type: name, + value: error.message, + }; + resolve(result); + }); } /** * @hidden */ -export function parseError(error: ExtendedError): Event { - const exception = getExceptionFromError(error); - - return { - exception: { - values: [exception], - }, - }; +export function parseError(error: ExtendedError): PromiseLike { + return new SyncPromise(resolve => + getExceptionFromError(error).then((exception: Exception) => { + resolve({ + exception: { + values: [exception], + }, + }); + }), + ); } /** From 2266ac5c229d905ae67e4d87f8821a46ce44bf70 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 15 Feb 2022 12:07:40 +0000 Subject: [PATCH 18/25] Minor improve --- packages/node/src/index.ts | 1 - packages/node/src/integrations/contextlines.ts | 4 +++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index 02dadc20f34f..9f65c3f1a1a8 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -45,7 +45,6 @@ export { NodeOptions } from './types'; export { NodeBackend } from './backend'; export { NodeClient } from './client'; export { defaultIntegrations, init, lastEventId, flush, close, getSentryRelease } from './sdk'; -export { eventFromException, eventFromMessage } from './eventbuilder'; export { deepReadDirSync } from './utils'; export { SDK_NAME } from './version'; diff --git a/packages/node/src/integrations/contextlines.ts b/packages/node/src/integrations/contextlines.ts index 1cbd7f8bf05c..874708e50896 100644 --- a/packages/node/src/integrations/contextlines.ts +++ b/packages/node/src/integrations/contextlines.ts @@ -7,6 +7,7 @@ import { LRUMap } from 'lru_map'; import { NodeClient } from '../client'; const FILE_CONTENT_CACHE = new LRUMap(100); +const DEFAULT_LINES_OF_CONTEXT = 7; function readTextFileAsync(path: string): Promise { return new Promise((resolve, reject) => { @@ -63,7 +64,8 @@ export class ContextLines implements Integration { /** Processes an event and adds context lines */ public async addToEvent(event: Event): Promise { - const contextLines = this._options.frameContextLines == undefined ? 7 : this._options.frameContextLines; + const contextLines = + this._options.frameContextLines !== undefined ? this._options.frameContextLines : DEFAULT_LINES_OF_CONTEXT; const frames = event.exception?.values?.[0].stacktrace?.frames; From 03cb320cc7661fea4d85ad999e4cfb452a68ad40 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 16 Feb 2022 15:51:32 +0000 Subject: [PATCH 19/25] Fix nextjs test --- packages/nextjs/test/index.server.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nextjs/test/index.server.test.ts b/packages/nextjs/test/index.server.test.ts index 62d51a9aa860..9daf1217d96c 100644 --- a/packages/nextjs/test/index.server.test.ts +++ b/packages/nextjs/test/index.server.test.ts @@ -103,7 +103,7 @@ describe('Server init()', () => { transaction.finish(); expect(sendEvent).not.toHaveBeenCalled(); - expect(captureEvent.mock.results[0].value).toBeUndefined(); + expect(captureEvent).toHaveBeenCalled(); expect(logError).toHaveBeenCalledWith(new SentryError('An event processor returned null, will not send event.')); }); From 965d48f6e1558eb46fda4b32d04343454e4958a4 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 16 Feb 2022 16:34:00 +0000 Subject: [PATCH 20/25] revert --- packages/nextjs/test/index.server.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nextjs/test/index.server.test.ts b/packages/nextjs/test/index.server.test.ts index 9daf1217d96c..62d51a9aa860 100644 --- a/packages/nextjs/test/index.server.test.ts +++ b/packages/nextjs/test/index.server.test.ts @@ -103,7 +103,7 @@ describe('Server init()', () => { transaction.finish(); expect(sendEvent).not.toHaveBeenCalled(); - expect(captureEvent).toHaveBeenCalled(); + expect(captureEvent.mock.results[0].value).toBeUndefined(); expect(logError).toHaveBeenCalledWith(new SentryError('An event processor returned null, will not send event.')); }); From e08bcbb7093d1e34c24049fd1948f97c010e8998 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 16 Feb 2022 15:51:32 +0000 Subject: [PATCH 21/25] Fix nextjs test --- packages/nextjs/test/index.server.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/nextjs/test/index.server.test.ts b/packages/nextjs/test/index.server.test.ts index 62d51a9aa860..e4e359a10624 100644 --- a/packages/nextjs/test/index.server.test.ts +++ b/packages/nextjs/test/index.server.test.ts @@ -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'; @@ -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()', () => { @@ -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, @@ -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.')); }); From f278e8bcb92c294df83694296ff6a269a309e951 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 17 Feb 2022 00:03:39 +0000 Subject: [PATCH 22/25] Code review changes! --- .../node/src/integrations/contextlines.ts | 39 ++++++++---------- packages/node/src/sdk.ts | 3 +- packages/node/src/types.ts | 2 +- packages/node/test/parsers.test.ts | 41 ++++++++----------- 4 files changed, 36 insertions(+), 49 deletions(-) diff --git a/packages/node/src/integrations/contextlines.ts b/packages/node/src/integrations/contextlines.ts index 874708e50896..8739dbdf922a 100644 --- a/packages/node/src/integrations/contextlines.ts +++ b/packages/node/src/integrations/contextlines.ts @@ -53,28 +53,32 @@ export class ContextLines implements Integration { * @inheritDoc */ public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void): void { - if (this._options.frameContextLines == undefined) { + // 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()?.getOptions(); // eslint-disable-next-line deprecation/deprecation this._options.frameContextLines = initOptions?.frameContextLines; } - addGlobalEventProcessor(event => this.addToEvent(event)); - } - - /** Processes an event and adds context lines */ - public async addToEvent(event: Event): Promise { 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 { const frames = event.exception?.values?.[0].stacktrace?.frames; if (frames && contextLines > 0) { - const filenames: string[] = []; + const filenames: Set = new Set(); for (const frame of frames) { - if (frame.filename && !filenames.includes(frame.filename)) { - filenames.push(frame.filename); + if (frame.filename) { + filenames.add(frame.filename); } } @@ -91,8 +95,6 @@ export class ContextLines implements Integration { } } } - - return event; } return event; @@ -104,25 +106,20 @@ export class ContextLines implements Integration { * * @param filenames Array of filepaths to read content from. */ -async function readSourceFiles(filenames: string[]): Promise> { - // we're relying on filenames being de-duped already - if (!filenames.length) { - return {}; - } - +async function readSourceFiles(filenames: Set): Promise> { const sourceFiles: Record = {}; for (const filename of filenames) { - const cache = FILE_CONTENT_CACHE.get(filename); + const cachedFile = FILE_CONTENT_CACHE.get(filename); // We have a cache hit - if (cache !== undefined) { + 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 (cache === null) { + if (cachedFile === null) { continue; } // Otherwise content is there, so reuse cached value. - sourceFiles[filename] = cache; + sourceFiles[filename] = cachedFile; continue; } diff --git a/packages/node/src/sdk.ts b/packages/node/src/sdk.ts index 586684982677..dee07c196566 100644 --- a/packages/node/src/sdk.ts +++ b/packages/node/src/sdk.ts @@ -12,6 +12,7 @@ export const defaultIntegrations = [ // Common new CoreIntegrations.InboundFilters(), new CoreIntegrations.FunctionToString(), + new ContextLines(), // Native Wrappers new Console(), new Http(), @@ -20,8 +21,6 @@ export const defaultIntegrations = [ new OnUnhandledRejection(), // Misc new LinkedErrors(), - // Context Lines - new ContextLines(), ]; /** diff --git a/packages/node/src/types.ts b/packages/node/src/types.ts index 677e40214e0d..6c6651bc7b88 100644 --- a/packages/node/src/types.ts +++ b/packages/node/src/types.ts @@ -23,7 +23,7 @@ export interface NodeOptions extends Options { /** * Sets the number of context lines for each frame when loading a file. * - * @deprecated Context lines configuration has moved to the `ContextLines` integration. + * @deprecated Context lines configuration has moved to the `ContextLines` integration, and can be used like this: * * ``` * init({ diff --git a/packages/node/test/parsers.test.ts b/packages/node/test/parsers.test.ts index 8e8618148b15..501645fe9215 100644 --- a/packages/node/test/parsers.test.ts +++ b/packages/node/test/parsers.test.ts @@ -8,15 +8,15 @@ import { getError } from './helper/error'; describe('parsers.ts', () => { let frames: stacktrace.StackFrame[]; - let spy: jest.SpyInstance; + let readFileSpy: jest.SpyInstance; let contextLines: ContextLines; - async function addContext(frames: StackFrame[]): Promise { - await contextLines.addToEvent({ exception: { values: [{ stacktrace: { frames } }] } }); + async function addContext(frames: StackFrame[], lines: number = 7): Promise { + await contextLines.addSourceContext({ exception: { values: [{ stacktrace: { frames } }] } }, lines); } beforeEach(() => { - spy = jest.spyOn(fs, 'readFile'); + readFileSpy = jest.spyOn(fs, 'readFile'); frames = stacktrace.parse(new Error('test')); contextLines = new ContextLines(); resetFileContentCache(); @@ -30,16 +30,16 @@ describe('parsers.ts', () => { test('parseStack with same file', async () => { expect.assertions(1); - let mockCalls = 0; let parsedFrames = Parsers.parseStack(frames); await addContext(parsedFrames); - mockCalls = spy.mock.calls.length; + const numCalls = readFileSpy.mock.calls.length; parsedFrames = Parsers.parseStack(frames); await addContext(parsedFrames); - // Calls to readFile shouldn't increase if there isn't a new error - expect(spy).toHaveBeenCalledTimes(mockCalls); + // Calls to `readFile` shouldn't increase if there isn't a new error to + // parse whose stacktrace contains a file we haven't yet seen + expect(readFileSpy).toHaveBeenCalledTimes(numCalls); }); test('parseStack with ESM module names', async () => { @@ -58,27 +58,20 @@ describe('parsers.ts', () => { ]; const parsedFrames = Parsers.parseStack(framesWithFilePath); await addContext(parsedFrames); - expect(spy).toHaveBeenCalledTimes(1); + expect(readFileSpy).toHaveBeenCalledTimes(1); }); test('parseStack with adding different file', async () => { - expect.assertions(2); - let mockCalls = 0; - let newErrorCalls = 0; + expect.assertions(1); let parsedFrames = Parsers.parseStack(frames); await addContext(parsedFrames); - mockCalls = spy.mock.calls.length; + const numCalls = readFileSpy.mock.calls.length; parsedFrames = Parsers.parseStack(stacktrace.parse(getError())); await addContext(parsedFrames); - newErrorCalls = spy.mock.calls.length; - expect(newErrorCalls).toBeGreaterThan(mockCalls); - - parsedFrames = Parsers.parseStack(stacktrace.parse(getError())); - await addContext(parsedFrames); - - expect(spy).toHaveBeenCalledTimes(newErrorCalls); + const newErrorCalls = readFileSpy.mock.calls.length; + expect(newErrorCalls).toBeGreaterThan(numCalls); }); test('parseStack with duplicate files', async () => { @@ -115,16 +108,14 @@ describe('parsers.ts', () => { const parsedFrames = Parsers.parseStack(framesWithDuplicateFiles); await addContext(parsedFrames); - expect(spy).toHaveBeenCalledTimes(1); + expect(readFileSpy).toHaveBeenCalledTimes(1); }); test('parseStack with no context', async () => { - contextLines = new ContextLines({ frameContextLines: 0 }); - expect.assertions(1); const parsedFrames = Parsers.parseStack(frames); - await addContext(parsedFrames); - expect(spy).toHaveBeenCalledTimes(0); + await addContext(parsedFrames, 0); + expect(readFileSpy).toHaveBeenCalledTimes(0); }); }); }); From ef7d423b55adb06a48000eabc2a7582592ba8354 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 17 Feb 2022 17:57:31 +0000 Subject: [PATCH 23/25] Abhi code review --- packages/node/src/integrations/contextlines.ts | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/packages/node/src/integrations/contextlines.ts b/packages/node/src/integrations/contextlines.ts index 8739dbdf922a..a275f462dc51 100644 --- a/packages/node/src/integrations/contextlines.ts +++ b/packages/node/src/integrations/contextlines.ts @@ -3,6 +3,7 @@ import { Event, EventProcessor, Integration } from '@sentry/types'; import { addContextToFrame } from '@sentry/utils'; import { readFile } from 'fs'; import { LRUMap } from 'lru_map'; +import { promisify } from 'util'; import { NodeClient } from '../client'; @@ -10,12 +11,7 @@ const FILE_CONTENT_CACHE = new LRUMap(100); const DEFAULT_LINES_OF_CONTEXT = 7; function readTextFileAsync(path: string): Promise { - return new Promise((resolve, reject) => { - readFile(path, 'utf8', (err, data) => { - if (err) reject(err); - else resolve(data); - }); - }); + return promisify(readFile)(path, { encoding: 'utf8' }); } /** @@ -28,10 +24,11 @@ export function resetFileContentCache(): void { interface ContextLinesOptions { /** - * Sets the number of context lines for each frame when loading a file + * 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; } From a172765d71ce11011ff016943a6425b682154eb3 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Fri, 18 Feb 2022 16:25:58 +0000 Subject: [PATCH 24/25] Revert promisify --- packages/node/src/integrations/contextlines.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/node/src/integrations/contextlines.ts b/packages/node/src/integrations/contextlines.ts index a275f462dc51..0f71ce8c2d3d 100644 --- a/packages/node/src/integrations/contextlines.ts +++ b/packages/node/src/integrations/contextlines.ts @@ -3,7 +3,6 @@ import { Event, EventProcessor, Integration } from '@sentry/types'; import { addContextToFrame } from '@sentry/utils'; import { readFile } from 'fs'; import { LRUMap } from 'lru_map'; -import { promisify } from 'util'; import { NodeClient } from '../client'; @@ -11,7 +10,12 @@ const FILE_CONTENT_CACHE = new LRUMap(100); const DEFAULT_LINES_OF_CONTEXT = 7; function readTextFileAsync(path: string): Promise { - return promisify(readFile)(path, { encoding: 'utf8' }); + return new Promise((resolve, reject) => { + readFile(path, 'utf8', (err, data) => { + if (err) reject(err); + else resolve(data); + }); + }); } /** From bd85ce9032d5bf75b993b618f0457e912b088d7b Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Fri, 18 Feb 2022 17:57:05 +0000 Subject: [PATCH 25/25] Add TODO comment --- packages/node/src/integrations/contextlines.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/node/src/integrations/contextlines.ts b/packages/node/src/integrations/contextlines.ts index 0f71ce8c2d3d..be77a4cfefbf 100644 --- a/packages/node/src/integrations/contextlines.ts +++ b/packages/node/src/integrations/contextlines.ts @@ -9,6 +9,7 @@ import { NodeClient } from '../client'; const FILE_CONTENT_CACHE = new LRUMap(100); const DEFAULT_LINES_OF_CONTEXT = 7; +// TODO: Replace with promisify when minimum supported node >= v8 function readTextFileAsync(path: string): Promise { return new Promise((resolve, reject) => { readFile(path, 'utf8', (err, data) => {