ref(node): Include source context lines for all exceptions (#4734)

- Ensures source context is added to **all** exceptions! - `readSourceFiles` becomes `readSourceFile` - Since we already have an LRU, nothing is gained by loading files in groups
getsentry · Mar 21, 2022 · 79a109c · 79a109c
1 parent 76acf2f
commit 79a109c
Show file tree

Hide file tree

Showing 3 changed files with 74 additions and 44 deletions.
diff --git a/packages/node/src/integrations/contextlines.ts b/packages/node/src/integrations/contextlines.ts
@@ -1,5 +1,5 @@
 import { getCurrentHub } from '@sentry/core';
-import { Event, EventProcessor, Integration } from '@sentry/types';
+import { Event, EventProcessor, Integration, StackFrame } from '@sentry/types';
 import { addContextToFrame } from '@sentry/utils';
 import { readFile } from 'fs';
 import { LRUMap } from 'lru_map';
@@ -73,23 +73,26 @@ export class ContextLines implements Integration {
 
   /** Processes an event and adds context lines */
   public async addSourceContext(event: Event, contextLines: number): Promise<Event> {
-    const frames = event.exception?.values?.[0].stacktrace?.frames;
-
-    if (frames && contextLines > 0) {
-      const filenames: Set<string> = new Set();
-
-      for (const frame of frames) {
-        if (frame.filename) {
-          filenames.add(frame.filename);
+    if (contextLines > 0 && event.exception?.values) {
+      for (const exception of event.exception.values) {
+        if (exception.stacktrace?.frames) {
+          await this._addSourceContextToFrames(exception.stacktrace.frames, contextLines);
         }
       }
+    }
+
+    return event;
+  }
 
-      const sourceFiles = await readSourceFiles(filenames);
+  /** Adds context lines to frames */
+  public async _addSourceContextToFrames(frames: StackFrame[], contextLines: number): Promise<void> {
+    for (const frame of frames) {
+      if (frame.filename) {
+        const sourceFile = await _readSourceFile(frame.filename);
 
-      for (const frame of frames) {
-        if (frame.filename && sourceFiles[frame.filename]) {
+        if (sourceFile) {
           try {
-            const lines = (sourceFiles[frame.filename] as string).split('\n');
+            const lines = sourceFile.split('\n');
             addContextToFrame(lines, frame, contextLines);
           } catch (e) {
             // anomaly, being defensive in case
@@ -98,43 +101,28 @@ export class ContextLines implements Integration {
         }
       }
     }
-
-    return event;
   }
 }
 
 /**
- * This function reads file contents and caches them in a global LRU cache.
+ * Reads file contents and caches them in a global LRU cache.
  *
- * @param filenames Array of filepaths to read content from.
+ * @param filename filepath to read content from.
  */
-async function readSourceFiles(filenames: Set<string>): Promise<Record<string, string | null>> {
-  const sourceFiles: Record<string, string | null> = {};
-
-  for (const filename of filenames) {
-    const cachedFile = FILE_CONTENT_CACHE.get(filename);
-    // We have a cache hit
-    if (cachedFile !== undefined) {
-      // If stored value is null, it means that we already tried, but couldn't read the content of the file. Skip.
-      if (cachedFile === null) {
-        continue;
-      }
-
-      // Otherwise content is there, so reuse cached value.
-      sourceFiles[filename] = cachedFile;
-      continue;
-    }
-
-    let content: string | null = null;
-    try {
-      content = await readTextFileAsync(filename);
-    } catch (_) {
-      //
-    }
+async function _readSourceFile(filename: string): Promise<string | null> {
+  const cachedFile = FILE_CONTENT_CACHE.get(filename);
+  // We have a cache hit
+  if (cachedFile !== undefined) {
+    return cachedFile;
+  }
 
-    FILE_CONTENT_CACHE.set(filename, content);
-    sourceFiles[filename] = content;
+  let content: string | null = null;
+  try {
+    content = await readTextFileAsync(filename);
+  } catch (_) {
+    //
   }
 
-  return sourceFiles;
+  FILE_CONTENT_CACHE.set(filename, content);
+  return content;
 }
diff --git a/packages/node/src/sdk.ts b/packages/node/src/sdk.ts
@@ -12,7 +12,6 @@ export const defaultIntegrations = [
   // Common
   new CoreIntegrations.InboundFilters(),
   new CoreIntegrations.FunctionToString(),
-  new ContextLines(),
   // Native Wrappers
   new Console(),
   new Http(),
@@ -21,6 +20,8 @@ export const defaultIntegrations = [
   new OnUnhandledRejection(),
   // Misc
   new LinkedErrors(),
+  // ContextLines must come after LinkedErrors so that context is added to linked errors
+  new ContextLines(),
 ];
 
 /**

diff --git a/packages/node/test/index.test.ts b/packages/node/test/index.test.ts
@@ -16,6 +16,7 @@ import {
   Scope,
 } from '../src';
 import { NodeBackend } from '../src/backend';
+import { LinkedErrors } from '../src/integrations';
 
 jest.mock('@sentry/core', () => {
   const original = jest.requireActual('@sentry/core');
@@ -198,6 +199,46 @@ describe('SentryNode', () => {
       }
     });
 
+    test.only('capture a linked exception with pre/post context', done => {
+      expect.assertions(15);
+      getCurrentHub().bindClient(
+        new NodeClient({
+          integrations: i => [new LinkedErrors(), ...i],
+          beforeSend: (event: Event) => {
+            expect(event.exception).not.toBeUndefined();
+            expect(event.exception!.values![1]).not.toBeUndefined();
+            expect(event.exception!.values![1].stacktrace!).not.toBeUndefined();
+            expect(event.exception!.values![1].stacktrace!.frames![1]).not.toBeUndefined();
+            expect(event.exception!.values![1].stacktrace!.frames![1].pre_context).not.toBeUndefined();
+            expect(event.exception!.values![1].stacktrace!.frames![1].post_context).not.toBeUndefined();
+            expect(event.exception!.values![1].type).toBe('Error');
+            expect(event.exception!.values![1].value).toBe('test');
+
+            expect(event.exception!.values![0]).not.toBeUndefined();
+            expect(event.exception!.values![0].stacktrace!).not.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('cause');
+            done();
+            return null;
+          },
+          dsn,
+        }),
+      );
+      try {
+        throw new Error('test');
+      } catch (e) {
+        try {
+          throw new Error('cause');
+        } catch (c) {
+          e.cause = c;
+          captureException(e);
+        }
+      }
+    });
+
     test('capture a message', done => {
       expect.assertions(2);
       getCurrentHub().bindClient(