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.')); }); diff --git a/packages/node/src/backend.ts b/packages/node/src/backend.ts index 0064b68a7d84..707d0bec9324 100644 --- a/packages/node/src/backend.ts +++ b/packages/node/src/backend.ts @@ -16,7 +16,7 @@ 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); + return eventFromException(exception, hint); } /** diff --git a/packages/node/src/eventbuilder.ts b/packages/node/src/eventbuilder.ts index 4b351898214e..3959e8f6daa2 100644 --- a/packages/node/src/eventbuilder.ts +++ b/packages/node/src/eventbuilder.ts @@ -16,7 +16,7 @@ import { extractStackFromError, parseError, parseStack, prepareFramesForEvent } * Builds and Event from a Exception * @hidden */ -export function eventFromException(options: Options, exception: unknown, hint?: EventHint): PromiseLike { +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 = @@ -48,7 +48,7 @@ export function eventFromException(options: Options, exception: unknown, hint?: } return new SyncPromise((resolve, reject) => - parseError(ex as Error, options) + parseError(ex as Error) .then(event => { addExceptionTypeValue(event, undefined, undefined); addExceptionMechanism(event, mechanism); @@ -81,16 +81,11 @@ export function eventFromMessage( return new SyncPromise(resolve => { if (options.attachStacktrace && hint && hint.syntheticException) { const stack = hint.syntheticException ? extractStackFromError(hint.syntheticException) : []; - void parseStack(stack, options) - .then(frames => { - event.stacktrace = { - frames: prepareFramesForEvent(frames), - }; - resolve(event); - }) - .then(null, () => { - resolve(event); - }); + const frames = parseStack(stack); + event.stacktrace = { + frames: prepareFramesForEvent(frames), + }; + resolve(event); } else { resolve(event); } diff --git a/packages/node/src/integrations/contextlines.ts b/packages/node/src/integrations/contextlines.ts new file mode 100644 index 000000000000..be77a4cfefbf --- /dev/null +++ b/packages/node/src/integrations/contextlines.ts @@ -0,0 +1,140 @@ +import { getCurrentHub } from '@sentry/core'; +import { Event, EventProcessor, Integration } from '@sentry/types'; +import { addContextToFrame } from '@sentry/utils'; +import { readFile } from 'fs'; +import { LRUMap } from 'lru_map'; + +import { NodeClient } from '../client'; + +const FILE_CONTENT_CACHE = new LRUMap(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) => { + if (err) reject(err); + else resolve(data); + }); + }); +} + +/** + * Resets the file cache. Exists for testing purposes. + * @hidden + */ +export function resetFileContentCache(): void { + FILE_CONTENT_CACHE.clear(); +} + +interface ContextLinesOptions { + /** + * Sets the number of context lines for each frame when loading a file. + * Defaults to 7. + * + * Set to 0 to disable loading and inclusion of source files. + **/ + frameContextLines?: number; +} + +/** Add node modules / packages to the event */ +export class ContextLines implements Integration { + /** + * @inheritDoc + */ + public static id: string = 'ContextLines'; + + /** + * @inheritDoc + */ + public name: string = ContextLines.id; + + public constructor(private readonly _options: ContextLinesOptions = {}) {} + + /** + * @inheritDoc + */ + public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void): void { + // This is only here to copy frameContextLines from init options if it hasn't + // been set via this integrations constructor. + // + // TODO: Remove on next major! + if (this._options.frameContextLines === undefined) { + const initOptions = getCurrentHub().getClient()?.getOptions(); + // eslint-disable-next-line deprecation/deprecation + this._options.frameContextLines = initOptions?.frameContextLines; + } + + const contextLines = + this._options.frameContextLines !== undefined ? this._options.frameContextLines : DEFAULT_LINES_OF_CONTEXT; + + addGlobalEventProcessor(event => this.addSourceContext(event, contextLines)); + } + + /** Processes an event and adds context lines */ + public async addSourceContext(event: Event, contextLines: number): Promise { + const frames = event.exception?.values?.[0].stacktrace?.frames; + + if (frames && contextLines > 0) { + const filenames: Set = new Set(); + + for (const frame of frames) { + if (frame.filename) { + filenames.add(frame.filename); + } + } + + const sourceFiles = await readSourceFiles(filenames); + + for (const frame of frames) { + if (frame.filename && sourceFiles[frame.filename]) { + try { + const lines = (sourceFiles[frame.filename] as string).split('\n'); + addContextToFrame(lines, frame, contextLines); + } catch (e) { + // anomaly, being defensive in case + // unlikely to ever happen in practice but can definitely happen in theory + } + } + } + } + + return event; + } +} + +/** + * This function reads file contents and caches them in a global LRU cache. + * + * @param filenames Array of filepaths to read content from. + */ +async function readSourceFiles(filenames: Set): Promise> { + const sourceFiles: Record = {}; + + for (const filename of filenames) { + const cachedFile = FILE_CONTENT_CACHE.get(filename); + // We have a cache hit + if (cachedFile !== undefined) { + // If stored value is null, it means that we already tried, but couldn't read the content of the file. Skip. + if (cachedFile === null) { + continue; + } + + // Otherwise content is there, so reuse cached value. + sourceFiles[filename] = cachedFile; + continue; + } + + let content: string | null = null; + try { + content = await readTextFileAsync(filename); + } catch (_) { + // + } + + FILE_CONTENT_CACHE.set(filename, content); + sourceFiles[filename] = content; + } + + return sourceFiles; +} 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/parsers.ts b/packages/node/src/parsers.ts index 76d5d57fa2ac..835dfa33ec9f 100644 --- a/packages/node/src/parsers.ts +++ b/packages/node/src/parsers.ts @@ -1,21 +1,7 @@ import { Event, Exception, ExtendedError, StackFrame } from '@sentry/types'; -import { addContextToFrame, basename, dirname, resolvedSyncPromise, SyncPromise } from '@sentry/utils'; -import { readFile } from 'fs'; -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); - -/** - * Resets the file cache. Exists for testing purposes. - * @hidden - */ -export function resetFileContentCache(): void { - FILE_CONTENT_CACHE.clear(); -} /** JSDoc */ function getFunction(frame: stacktrace.StackFrame): string { @@ -63,65 +49,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[]): PromiseLike<{ [key: string]: string | null }> { - // we're relying on filenames being de-duped already - if (filenames.length === 0) { - return resolvedSyncPromise({}); - } - - 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 */ @@ -136,13 +63,8 @@ export function extractStackFromError(error: Error): stacktrace.StackFrame[] { /** * @hidden */ -export function parseStack(stack: stacktrace.StackFrame[], 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 || '', @@ -166,87 +88,37 @@ export function parseStack(stack: stacktrace.StackFrame[], options?: NodeOptions // 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 resolvedSyncPromise(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 resolvedSyncPromise(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, -): 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); - }), - ); } /** * @hidden */ -export function getExceptionFromError(error: Error, options?: NodeOptions): PromiseLike { +export function getExceptionFromError(error: Error): PromiseLike { const name = error.name || error.constructor.name; const stack = extractStackFromError(error); - return new SyncPromise(resolve => - parseStack(stack, options).then(frames => { - const result = { - stacktrace: { - frames: prepareFramesForEvent(frames), - }, - type: name, - value: error.message, - }; - resolve(result); - }), - ); + 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, options?: NodeOptions): PromiseLike { +export function parseError(error: ExtendedError): PromiseLike { return new SyncPromise(resolve => - getExceptionFromError(error, options).then((exception: Exception) => { + getExceptionFromError(error).then((exception: Exception) => { resolve({ exception: { values: [exception], diff --git a/packages/node/src/sdk.ts b/packages/node/src/sdk.ts index 52d137277fd8..dee07c196566 100644 --- a/packages/node/src/sdk.ts +++ b/packages/node/src/sdk.ts @@ -5,13 +5,14 @@ import { getGlobalObject, logger } 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 = [ // Common new CoreIntegrations.InboundFilters(), new CoreIntegrations.FunctionToString(), + new ContextLines(), // Native Wrappers new Console(), new Http(), diff --git a/packages/node/src/types.ts b/packages/node/src/types.ts index d9fec7f4c13e..6c6651bc7b88 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, and can be used like this: + * + * ``` + * init({ + * dsn: '__DSN__', + * integrations: [new ContextLines({ frameContextLines: 10 })] + * }) + * ``` + * + * */ frameContextLines?: number; /** Callback that is executed when a fatal global error occurs. */ diff --git a/packages/node/test/index.test.ts b/packages/node/test/index.test.ts index 126bc617ccb7..3a16c07fd552 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({ @@ -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(event.exception!.values![0].stacktrace!.frames![2].pre_context).toBeUndefined(); - expect(event.exception!.values![0].stacktrace!.frames![2].post_context).toBeUndefined(); + 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(); @@ -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 3097b85d33c7..501645fe9215 100644 --- a/packages/node/test/parsers.test.ts +++ b/packages/node/test/parsers.test.ts @@ -1,17 +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 stacktrace from '../src/stacktrace'; 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[], 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')); - Parsers.resetFileContentCache(); + contextLines = new ContextLines(); + resetFileContentCache(); }); afterEach(() => { @@ -19,25 +27,19 @@ describe('parsers.ts', () => { }); describe('lru file cache', () => { - test('parseStack with same file', done => { + test('parseStack with same file', async () => { expect.assertions(1); - let mockCalls = 0; - void Parsers.parseStack(frames) - .then(_ => { - mockCalls = spy.mock.calls.length; - void Parsers.parseStack(frames) - .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); + await addContext(parsedFrames); + + 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 to + // parse whose stacktrace contains a file we haven't yet seen + expect(readFileSpy).toHaveBeenCalledTimes(numCalls); }); test('parseStack with ESM module names', async () => { @@ -54,81 +56,66 @@ describe('parsers.ts', () => { typeName: 'module.exports../src/index.ts', }, ]; - return Parsers.parseStack(framesWithFilePath).then(_ => { - expect(spy).toHaveBeenCalledTimes(1); - }); + const parsedFrames = Parsers.parseStack(framesWithFilePath); + await addContext(parsedFrames); + expect(readFileSpy).toHaveBeenCalledTimes(1); }); - test('parseStack with adding different file', done => { - expect.assertions(2); - let mockCalls = 0; - let newErrorCalls = 0; - void Parsers.parseStack(frames) - .then(_ => { - mockCalls = spy.mock.calls.length; - void Parsers.parseStack(stacktrace.parse(getError())) - .then(_1 => { - newErrorCalls = spy.mock.calls.length; - expect(newErrorCalls).toBeGreaterThan(mockCalls); - void Parsers.parseStack(stacktrace.parse(getError())) - .then(_2 => { - expect(spy).toHaveBeenCalledTimes(newErrorCalls); - done(); - }) - .then(null, () => { - // no-empty - }); - }) - .then(null, () => { - // no-empty - }); - }) - .then(null, () => { - // no-empty - }); + test('parseStack with adding different file', async () => { + expect.assertions(1); + let parsedFrames = Parsers.parseStack(frames); + await addContext(parsedFrames); + + const numCalls = readFileSpy.mock.calls.length; + parsedFrames = Parsers.parseStack(stacktrace.parse(getError())); + await addContext(parsedFrames); + + const newErrorCalls = readFileSpy.mock.calls.length; + expect(newErrorCalls).toBeGreaterThan(numCalls); }); - }); - 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', - }, - ]; - return Parsers.parseStack(framesWithDuplicateFiles).then(_ => { - expect(spy).toHaveBeenCalledTimes(1); + 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); + await addContext(parsedFrames); + expect(readFileSpy).toHaveBeenCalledTimes(1); }); - }); - test('parseStack with no context', async () => { - expect.assertions(1); - return Parsers.parseStack(frames, { frameContextLines: 0 }).then(_ => { - expect(spy).toHaveBeenCalledTimes(0); + test('parseStack with no context', async () => { + expect.assertions(1); + const parsedFrames = Parsers.parseStack(frames); + await addContext(parsedFrames, 0); + expect(readFileSpy).toHaveBeenCalledTimes(0); }); }); });