fix: Move limiter to node, Add frameContextLines option

getsentry · Dec 18, 2018 · 8d14c6e · 8d14c6e
1 parent 277c9a7
commit 8d14c6e
Show file tree

Hide file tree

Showing 8 changed files with 72 additions and 22 deletions.
diff --git a/packages/node/package.json b/packages/node/package.json
@@ -20,6 +20,7 @@
     "@sentry/types": "4.4.2",
     "@sentry/utils": "4.4.2",
     "@types/stack-trace": "0.0.29",
+    "async-limiter": "^1.0.0",
     "cookie": "0.3.1",
     "https-proxy-agent": "^2.2.1",
     "lsmod": "1.0.0",

diff --git a/packages/node/src/backend.ts b/packages/node/src/backend.ts
@@ -30,6 +30,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;
 }
 
 /** The Sentry Node SDK Backend. */
@@ -65,7 +68,7 @@ export class NodeBackend extends BaseBackend<NodeOptions> {
       }
     }
 
-    const event: SentryEvent = await parseError(ex as Error);
+    const event: SentryEvent = await parseError(ex as Error, this.options);
 
     return {
       ...event,
@@ -89,7 +92,7 @@ export class NodeBackend extends BaseBackend<NodeOptions> {
 
     if (this.options.attachStacktrace && hint && hint.syntheticException) {
       const stack = hint.syntheticException ? await extractStackFromError(hint.syntheticException) : [];
-      const frames = await parseStack(stack);
+      const frames = await parseStack(stack, this.options);
       event.stacktrace = {
         frames: prepareFramesForEvent(frames),
       };

diff --git a/packages/node/src/declarations.d.ts b/packages/node/src/declarations.d.ts
@@ -1,2 +1,3 @@
 declare module 'lsmod';
 declare module 'https-proxy-agent';
+declare module 'async-limiter';
diff --git a/packages/node/src/parsers.ts b/packages/node/src/parsers.ts
@@ -2,9 +2,13 @@ import { SentryEvent, SentryException, StackFrame } from '@sentry/types';
 import { readFileAsync } from '@sentry/utils/fs';
 import { basename, dirname } from '@sentry/utils/path';
 import { snipLine } from '@sentry/utils/string';
+import * as Limiter from 'async-limiter';
 import * as stacktrace from 'stack-trace';
+import { NodeOptions } from './backend';
 
-const LINES_OF_CONTEXT: number = 7;
+// tslint:disable-next-line:no-unsafe-any
+const fsLimiter = new Limiter({ concurrency: 25 });
+const DEFAULT_LINES_OF_CONTEXT: number = 7;
 
 /**
  * Just an Error object with arbitrary attributes attached to it.
@@ -75,7 +79,7 @@ async function readSourceFiles(
     filenames.map(async filename => {
       let content;
       try {
-        content = await readFileAsync(filename);
+        content = await readFileAsync(filename, fsLimiter);
       } catch (_) {
         // unsure what to add here as the file is unreadable
         content = null;
@@ -99,8 +103,12 @@ export async function extractStackFromError(error: Error): Promise<stacktrace.St
 }
 
 /** JSDoc */
-export async function parseStack(stack: stacktrace.StackFrame[]): Promise<StackFrame[]> {
+export async function parseStack(stack: stacktrace.StackFrame[], options?: NodeOptions): Promise<StackFrame[]> {
   const filesToRead: string[] = [];
+
+  const linesOfContext =
+    options && options.frameContextLines !== undefined ? options.frameContextLines : DEFAULT_LINES_OF_CONTEXT;
+
   const frames: StackFrame[] = stack.map(frame => {
     const parsedFrame: StackFrame = {
       colno: frame.getColumnNumber(),
@@ -126,16 +134,21 @@ export async function parseStack(stack: stacktrace.StackFrame[]): Promise<StackF
     if (parsedFrame.filename) {
       parsedFrame.module = getModule(parsedFrame.filename);
 
-      if (!isInternal) {
+      if (!isInternal && linesOfContext > 0) {
         filesToRead.push(parsedFrame.filename);
       }
     }
 
     return parsedFrame;
   });
 
+  // We do an early return if we do not want to fetch context liens
+  if (linesOfContext <= 0) {
+    return frames;
+  }
+
   try {
-    return await addPrePostContext(filesToRead, frames);
+    return await 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.
@@ -149,21 +162,25 @@ export async function parseStack(stack: stacktrace.StackFrame[]): Promise<StackF
  * @param filesToRead string[] of filepaths
  * @param frames StackFrame[] containg all frames
  */
-async function addPrePostContext(filesToRead: string[], frames: StackFrame[]): Promise<StackFrame[]> {
+async function addPrePostContext(
+  filesToRead: string[],
+  frames: StackFrame[],
+  linesOfContext: number,
+): Promise<StackFrame[]> {
   const sourceFiles = await readSourceFiles(filesToRead);
   return frames.map(frame => {
     if (frame.filename && sourceFiles[frame.filename]) {
       try {
         const lines = sourceFiles[frame.filename].split('\n');
 
         frame.pre_context = lines
-          .slice(Math.max(0, (frame.lineno || 0) - (LINES_OF_CONTEXT + 1)), (frame.lineno || 0) - 1)
+          .slice(Math.max(0, (frame.lineno || 0) - (linesOfContext + 1)), (frame.lineno || 0) - 1)
           .map((line: string) => snipLine(line, 0));
 
         frame.context_line = snipLine(lines[(frame.lineno || 0) - 1], frame.colno || 0);
 
         frame.post_context = lines
-          .slice(frame.lineno || 0, (frame.lineno || 0) + LINES_OF_CONTEXT)
+          .slice(frame.lineno || 0, (frame.lineno || 0) + linesOfContext)
           .map((line: string) => snipLine(line, 0));
       } catch (e) {
         // anomaly, being defensive in case
@@ -175,10 +192,10 @@ async function addPrePostContext(filesToRead: string[], frames: StackFrame[]): P
 }
 
 /** JSDoc */
-export async function getExceptionFromError(error: Error): Promise<SentryException> {
+export async function getExceptionFromError(error: Error, options?: NodeOptions): Promise<SentryException> {
   const name = error.name || error.constructor.name;
   const stack = await extractStackFromError(error);
-  const frames = await parseStack(stack);
+  const frames = await parseStack(stack, options);
 
   return {
     stacktrace: {
@@ -190,9 +207,9 @@ export async function getExceptionFromError(error: Error): Promise<SentryExcepti
 }
 
 /** JSDoc */
-export async function parseError(error: ExtendedError): Promise<SentryEvent> {
+export async function parseError(error: ExtendedError, options?: NodeOptions): Promise<SentryEvent> {
   const name = error.name || error.constructor.name;
-  const exception = await getExceptionFromError(error);
+  const exception = await getExceptionFromError(error, options);
   return {
     exception: {
       values: [exception],

diff --git a/packages/node/test/index.test.ts b/packages/node/test/index.test.ts
@@ -151,12 +151,14 @@ describe('SentryNode', () => {
     });
 
     test('capture an exception', done => {
-      expect.assertions(6);
+      expect.assertions(8);
       getCurrentHub().bindClient(
         new NodeClient({
           beforeSend: (event: SentryEvent) => {
             expect(event.tags).toEqual({ test: '1' });
             expect(event.exception).not.toBeUndefined();
+            expect(event.exception!.values![0].stacktrace!.frames![2].pre_context).not.toBeUndefined();
+            expect(event.exception!.values![0].stacktrace!.frames![2].post_context).not.toBeUndefined();
             expect(event.exception!.values![0]).not.toBeUndefined();
             expect(event.exception!.values![0].type).toBe('Error');
             expect(event.exception!.values![0].value).toBe('test');
@@ -177,6 +179,36 @@ describe('SentryNode', () => {
       }
     });
 
+    test('capture an exception no pre/post context', done => {
+      expect.assertions(8);
+      getCurrentHub().bindClient(
+        new NodeClient({
+          beforeSend: (event: SentryEvent) => {
+            expect(event.tags).toEqual({ test: '1' });
+            expect(event.exception).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]).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();
+            done();
+            return event;
+          },
+          dsn,
+          frameContextLines: 0,
+        }),
+      );
+      configureScope((scope: Scope) => {
+        scope.setTag('test', '1');
+      });
+      try {
+        throw new Error('test');
+      } catch (e) {
+        captureException(e);
+      }
+    });
+
     test('capture a message', done => {
       expect.assertions(2);
       getCurrentHub().bindClient(

diff --git a/packages/utils/src/declarations.d.ts b/packages/utils/src/declarations.d.ts
diff --git a/packages/utils/src/fs.ts b/packages/utils/src/fs.ts
@@ -1,26 +1,23 @@
-import * as Limiter from 'async-limiter';
 import { mkdir, mkdirSync, readFile, statSync } from 'fs';
 import { dirname, resolve } from 'path';
 
-// tslint:disable-next-line:no-unsafe-any
-const fsLimiter = new Limiter({ concurrency: 25 });
 const _0777 = parseInt('0777', 8);
 
 /**
  * Asynchronously reads given files content.
  *
  * @param path A relative or absolute path to the file
+ * @param fsLimiter A limiter instance that prevents excessive file access
  * @returns A Promise that resolves when the file has been read.
  */
-export async function readFileAsync(path: string): Promise<Error | string> {
+export async function readFileAsync(path: string, fsLimiter: any): Promise<Error | string> {
   // We cannot use util.promisify here because that was only introduced in Node
   // 8 and we need to support older Node versions.
   return new Promise<Error | string>((res, reject) => {
     // tslint:disable-next-line:no-unsafe-any
     fsLimiter.push((done: () => void) => {
       readFile(path, 'utf8', (err, data) => {
         done();
-
         if (err) {
           reject(err);
         } else {

diff --git a/yarn.lock b/yarn.lock
@@ -1299,7 +1299,7 @@ async-each@^1.0.0:
   version "1.0.1"
   resolved "https://registry.yarnpkg.com/async-each/-/async-each-1.0.1.tgz#19d386a1d9edc6e7c1c85d388aedbcc56d33602d"
 
-async-limiter@~1.0.0:
+async-limiter@^1.0.0, async-limiter@~1.0.0:
   version "1.0.0"
   resolved "https://registry.yarnpkg.com/async-limiter/-/async-limiter-1.0.0.tgz#78faed8c3d074ab81f22b4e985d79e8738f720f8"