From 18eb9325fa4444ff85a77b2e7a46e2f69bc9b52f Mon Sep 17 00:00:00 2001 From: JonasBa Date: Wed, 8 Mar 2023 10:28:06 -0500 Subject: [PATCH 1/3] ref(contextlines): store split file in cache --- .../node/src/integrations/contextlines.ts | 34 +++++--- packages/node/test/context-lines.benchmark.ts | 84 +++++++++++++++++++ packages/node/test/context-lines.test.ts | 27 ++++++ 3 files changed, 132 insertions(+), 13 deletions(-) create mode 100644 packages/node/test/context-lines.benchmark.ts diff --git a/packages/node/src/integrations/contextlines.ts b/packages/node/src/integrations/contextlines.ts index f3cebc1d4037..2bc6441794e5 100644 --- a/packages/node/src/integrations/contextlines.ts +++ b/packages/node/src/integrations/contextlines.ts @@ -3,7 +3,7 @@ import { addContextToFrame } from '@sentry/utils'; import { readFile } from 'fs'; import { LRUMap } from 'lru_map'; -const FILE_CONTENT_CACHE = new LRUMap(100); +const FILE_CONTENT_CACHE = new LRUMap(100); const DEFAULT_LINES_OF_CONTEXT = 7; // TODO: Replace with promisify when minimum supported node >= v8 @@ -102,8 +102,8 @@ export class ContextLines implements Integration { // and attempt to add source context to frames. if (this._contextLines > 0 && event.exception?.values) { for (const exception of event.exception.values) { - if (exception.stacktrace?.frames) { - this.addSourceContextToFrames(exception.stacktrace.frames); + if (exception.stacktrace && exception.stacktrace.frames) { + await this.addSourceContextToFrames(exception.stacktrace.frames); } } } @@ -116,12 +116,11 @@ export class ContextLines implements Integration { for (const frame of frames) { // Only add context if we have a filename and it hasn't already been added if (frame.filename && frame.context_line === undefined) { - const sourceFile = FILE_CONTENT_CACHE.get(frame.filename); + const sourceFileLines = FILE_CONTENT_CACHE.get(frame.filename); - if (sourceFile) { + if (sourceFileLines) { try { - const lines = sourceFile.split('\n'); - addContextToFrame(lines, frame, this._contextLines); + addContextToFrame(sourceFileLines, frame, this._contextLines); } catch (e) { // anomaly, being defensive in case // unlikely to ever happen in practice but can definitely happen in theory @@ -134,25 +133,34 @@ export class ContextLines implements Integration { /** * Reads file contents and caches them in a global LRU cache. + * If reading fails, mark the file as null in the cache so we don't try again. * * @param filename filepath to read content from. */ -async function _readSourceFile(filename: string): Promise { +async function _readSourceFile(filename: string): Promise { const cachedFile = FILE_CONTENT_CACHE.get(filename); - // We have a cache hit + + // We have already attempted to read this file and failed, do not try again + if (cachedFile === null) { + return null; + } + + // We have a cache hit, return it if (cachedFile !== undefined) { return cachedFile; } - let content: string | null = null; - // Guard from throwing if readFile fails, this enables us to use Promise.all and // not have it short circuiting if one of the promises rejects + since context lines are added // on a best effort basis, we want to throw here anyways. + + // If we made it to here, it means that our file is not cache nor marked as failed, so attempt to read it + let content: string[] | null = null; try { - content = await readTextFileAsync(filename); + const rawFileContents = await readTextFileAsync(filename); + content = rawFileContents.split('\n'); } catch (_) { - // + // if we fail, we will mark the file as null in the cache and short circuit next time we try to read it } FILE_CONTENT_CACHE.set(filename, content); diff --git a/packages/node/test/context-lines.benchmark.ts b/packages/node/test/context-lines.benchmark.ts new file mode 100644 index 000000000000..1c003c98571e --- /dev/null +++ b/packages/node/test/context-lines.benchmark.ts @@ -0,0 +1,84 @@ +import type { StackFrame } from '@sentry/types'; +import * as fs from 'fs'; + +import { parseStackFrames } from '../src/eventbuilder'; +import { ContextLines, resetFileContentCache } from '../src/integrations/contextlines'; +import { defaultStackParser } from '../src/sdk'; +import { getError } from './helper/error'; + +import * as Benchmark from 'benchmark'; + +const lines = new ContextLines({}); + +const source = { + exception: { + values: [ + { + stacktrace: { + frames: [ + { + colno: 1, + filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/test/requestdata.test.ts', + lineno: 1, + function: 'fxn1', + }, + { + colno: 1, + filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/test/requestdata.test.ts', + lineno: 1, + function: 'fxn1', + }, + { + colno: 1, + filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/test/sdk.test.ts', + lineno: 1, + function: 'fxn1', + }, + { + colno: 1, + filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/test/stacktrace.test.ts', + lineno: 1, + function: 'fxn1', + }, + { + colno: 1, + filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/test/utils.test.ts', + lineno: 1, + function: 'fxn1', + }, + { + colno: 1, + filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/test/transports/http.test.ts', + lineno: 1, + function: 'fxn1', + }, + { + colno: 1, + filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/test/transports/https.test.ts', + lineno: 1, + function: 'fxn1', + }, + { + colno: 1, + filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/tsconfig.json', + lineno: 1, + function: 'fxn1', + }, + ], + }, + }, + ], + }, +}; + +const suite = new Benchmark.Suite({ setup: resetFileContentCache, teardown: resetFileContentCache }); + +suite + .add('parallel io', async function () { + await lines.addSourceContext(source); + await lines.addSourceContext(source); + }) + .on('cycle', function (event: any) { + console.log(String(event.target)); + }) + .run({ async: true }); diff --git a/packages/node/test/context-lines.test.ts b/packages/node/test/context-lines.test.ts index de6f6f382cd8..b469796214b1 100644 --- a/packages/node/test/context-lines.test.ts +++ b/packages/node/test/context-lines.test.ts @@ -107,4 +107,31 @@ describe('ContextLines', () => { expect(readFileSpy).toHaveBeenCalledTimes(0); }); }); + test.only('does not attempt to readfile multiple times if it fails', async () => { + expect.assertions(1); + contextLines = new ContextLines({}); + + readFileSpy.mockImplementation(() => { + throw new Error("ENOENT: no such file or directory, open '/does/not/exist.js'"); + }); + + await addContext([ + { + colno: 1, + filename: '/does/not/exist.js', + lineno: 1, + function: 'fxn1', + }, + ]); + await addContext([ + { + colno: 1, + filename: '/does/not/exist.js', + lineno: 1, + function: 'fxn1', + }, + ]); + + expect(readFileSpy).toHaveBeenCalledTimes(1); + }); }); From b961202fd38ee3a3eda639d865a7e9f450502e60 Mon Sep 17 00:00:00 2001 From: JonasBa Date: Wed, 8 Mar 2023 10:42:29 -0500 Subject: [PATCH 2/3] ref(contextlines): remove benchmark --- packages/node/test/context-lines.benchmark.ts | 84 ------------------- 1 file changed, 84 deletions(-) delete mode 100644 packages/node/test/context-lines.benchmark.ts diff --git a/packages/node/test/context-lines.benchmark.ts b/packages/node/test/context-lines.benchmark.ts deleted file mode 100644 index 1c003c98571e..000000000000 --- a/packages/node/test/context-lines.benchmark.ts +++ /dev/null @@ -1,84 +0,0 @@ -import type { StackFrame } from '@sentry/types'; -import * as fs from 'fs'; - -import { parseStackFrames } from '../src/eventbuilder'; -import { ContextLines, resetFileContentCache } from '../src/integrations/contextlines'; -import { defaultStackParser } from '../src/sdk'; -import { getError } from './helper/error'; - -import * as Benchmark from 'benchmark'; - -const lines = new ContextLines({}); - -const source = { - exception: { - values: [ - { - stacktrace: { - frames: [ - { - colno: 1, - filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/test/requestdata.test.ts', - lineno: 1, - function: 'fxn1', - }, - { - colno: 1, - filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/test/requestdata.test.ts', - lineno: 1, - function: 'fxn1', - }, - { - colno: 1, - filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/test/sdk.test.ts', - lineno: 1, - function: 'fxn1', - }, - { - colno: 1, - filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/test/stacktrace.test.ts', - lineno: 1, - function: 'fxn1', - }, - { - colno: 1, - filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/test/utils.test.ts', - lineno: 1, - function: 'fxn1', - }, - { - colno: 1, - filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/test/transports/http.test.ts', - lineno: 1, - function: 'fxn1', - }, - { - colno: 1, - filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/test/transports/https.test.ts', - lineno: 1, - function: 'fxn1', - }, - { - colno: 1, - filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/tsconfig.json', - lineno: 1, - function: 'fxn1', - }, - ], - }, - }, - ], - }, -}; - -const suite = new Benchmark.Suite({ setup: resetFileContentCache, teardown: resetFileContentCache }); - -suite - .add('parallel io', async function () { - await lines.addSourceContext(source); - await lines.addSourceContext(source); - }) - .on('cycle', function (event: any) { - console.log(String(event.target)); - }) - .run({ async: true }); From f79c15ec6bd9a9fc1c793a0127cedf25f8419bf4 Mon Sep 17 00:00:00 2001 From: JonasBa Date: Wed, 8 Mar 2023 10:54:39 -0500 Subject: [PATCH 3/3] ref(contextlines): fix readSourceFile signature --- packages/node/src/integrations/contextlines.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node/src/integrations/contextlines.ts b/packages/node/src/integrations/contextlines.ts index 2bc6441794e5..bad96ae43fb2 100644 --- a/packages/node/src/integrations/contextlines.ts +++ b/packages/node/src/integrations/contextlines.ts @@ -65,7 +65,7 @@ export class ContextLines implements Integration { // keep a lookup map of which files we've already enqueued to read, // so we don't enqueue the same file multiple times which would cause multiple i/o reads const enqueuedReadSourceFileTasks: Record = {}; - const readSourceFileTasks: Promise[] = []; + const readSourceFileTasks: Promise[] = []; if (this._contextLines > 0 && event.exception?.values) { for (const exception of event.exception.values) {