From 277c9a7846a407f2b6807d7a6c64764d3b75c920 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Og=C3=B3rek?= Date: Tue, 11 Dec 2018 19:39:56 +0100 Subject: [PATCH 01/16] fix: Limit concurrent FS calls to prevent memory fragmentation --- packages/raven-node/lib/utils.js | 4 ++-- packages/utils/.gitignore | 1 + packages/utils/src/declarations.d.ts | 1 + packages/utils/src/fs.ts | 27 +++++++++++++++++---------- packages/utils/test/object.test.ts | 2 +- 5 files changed, 22 insertions(+), 13 deletions(-) create mode 100644 packages/utils/src/declarations.d.ts diff --git a/packages/raven-node/lib/utils.js b/packages/raven-node/lib/utils.js index b01d2c454504..21d02d8a931b 100644 --- a/packages/raven-node/lib/utils.js +++ b/packages/raven-node/lib/utils.js @@ -18,7 +18,7 @@ var protocolMap = { var consoleAlerts = new Set(); -var fsLimiter = new Limiter({concurrency: 25); +var fsLimiter = new Limiter({concurrency: 25}); // Default Node.js REPL depth var MAX_SERIALIZE_EXCEPTION_DEPTH = 3; @@ -282,7 +282,7 @@ function readSourceFiles(filenames, cb) { var sourceFiles = {}; var numFilesToRead = filenames.length; return filenames.forEach(function(filename) { - fsLimiter.push(function (done) { + fsLimiter.push(function(done) { fs.readFile(filename, function(readErr, file) { done(); diff --git a/packages/utils/.gitignore b/packages/utils/.gitignore index 1a514e34e2c2..e38f8ebea3a7 100644 --- a/packages/utils/.gitignore +++ b/packages/utils/.gitignore @@ -1,3 +1,4 @@ *.js.map *.d.ts +!src/*.d.ts *.js diff --git a/packages/utils/src/declarations.d.ts b/packages/utils/src/declarations.d.ts new file mode 100644 index 000000000000..843b79454518 --- /dev/null +++ b/packages/utils/src/declarations.d.ts @@ -0,0 +1 @@ +declare module 'async-limiter'; diff --git a/packages/utils/src/fs.ts b/packages/utils/src/fs.ts index 3097dd044697..98b6364546ff 100644 --- a/packages/utils/src/fs.ts +++ b/packages/utils/src/fs.ts @@ -1,25 +1,32 @@ +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 creates the given directory. + * Asynchronously reads given files content. * - * @param path A relative or absolute path to the directory. - * @param mode The permission mode. - * @returns A Promise that resolves when the path has been created. + * @param path A relative or absolute path to the file + * @returns A Promise that resolves when the file has been read. */ export async function readFileAsync(path: string): Promise { // 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((res, reject) => { - readFile(path, 'utf8', (err, data) => { - if (err) { - reject(err); - } else { - res(data); - } + // tslint:disable-next-line:no-unsafe-any + fsLimiter.push((done: () => void) => { + readFile(path, 'utf8', (err, data) => { + done(); + + if (err) { + reject(err); + } else { + res(data); + } + }); }); }); } diff --git a/packages/utils/test/object.test.ts b/packages/utils/test/object.test.ts index 2e9444758dd5..71b5a2d5fe91 100644 --- a/packages/utils/test/object.test.ts +++ b/packages/utils/test/object.test.ts @@ -1,4 +1,4 @@ -import { clone, deserialize, fill, serialize, safeNormalize, urlEncode } from '../src/object'; +import { clone, deserialize, fill, safeNormalize, serialize, urlEncode } from '../src/object'; const MATRIX = [ { name: 'boolean', object: true, serialized: 'true' }, From 8d14c6e8360c73ac0a3426df17f8f4a335247303 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 18 Dec 2018 18:50:36 +0100 Subject: [PATCH 02/16] fix: Move limiter to node, Add frameContextLines option --- packages/node/package.json | 1 + packages/node/src/backend.ts | 7 +++-- packages/node/src/declarations.d.ts | 1 + packages/node/src/parsers.ts | 41 ++++++++++++++++++++-------- packages/node/test/index.test.ts | 34 ++++++++++++++++++++++- packages/utils/src/declarations.d.ts | 1 - packages/utils/src/fs.ts | 7 ++--- yarn.lock | 2 +- 8 files changed, 72 insertions(+), 22 deletions(-) delete mode 100644 packages/utils/src/declarations.d.ts diff --git a/packages/node/package.json b/packages/node/package.json index a02c245b9275..7a9990d46d8c 100644 --- 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 index c9e4ce1bb2f1..76b9ee46a060 100644 --- 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 { } } - 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 { 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 index 8bdafc3a34f7..7fc38ca348ca 100644 --- 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 index a45cf16c9d78..8fab0222668f 100644 --- 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 { +export async function parseStack(stack: stacktrace.StackFrame[], options?: NodeOptions): Promise { 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,7 +134,7 @@ export async function parseStack(stack: stacktrace.StackFrame[]): Promise 0) { filesToRead.push(parsedFrame.filename); } } @@ -134,8 +142,13 @@ export async function parseStack(stack: stacktrace.StackFrame[]): Promise { +async function addPrePostContext( + filesToRead: string[], + frames: StackFrame[], + linesOfContext: number, +): Promise { const sourceFiles = await readSourceFiles(filesToRead); return frames.map(frame => { if (frame.filename && sourceFiles[frame.filename]) { @@ -157,13 +174,13 @@ async function addPrePostContext(filesToRead: string[], frames: StackFrame[]): P 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 { +export async function getExceptionFromError(error: Error, options?: NodeOptions): Promise { 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 { +export async function parseError(error: ExtendedError, options?: NodeOptions): Promise { 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 index 5af3ae473a00..962377380e37 100644 --- 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 deleted file mode 100644 index 843b79454518..000000000000 --- a/packages/utils/src/declarations.d.ts +++ /dev/null @@ -1 +0,0 @@ -declare module 'async-limiter'; diff --git a/packages/utils/src/fs.ts b/packages/utils/src/fs.ts index 98b6364546ff..5a41dfc8cfae 100644 --- a/packages/utils/src/fs.ts +++ b/packages/utils/src/fs.ts @@ -1,18 +1,16 @@ -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 { +export async function readFileAsync(path: string, fsLimiter: any): Promise { // 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((res, reject) => { @@ -20,7 +18,6 @@ export async function readFileAsync(path: string): Promise { fsLimiter.push((done: () => void) => { readFile(path, 'utf8', (err, data) => { done(); - if (err) { reject(err); } else { diff --git a/yarn.lock b/yarn.lock index 1e311ce5eedd..debe10af990d 100644 --- 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" From 95ee9996687d1e600b414d261b00ddbc48142430 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 19 Dec 2018 09:19:41 +0100 Subject: [PATCH 03/16] Update .gitignore --- packages/utils/.gitignore | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/utils/.gitignore b/packages/utils/.gitignore index e38f8ebea3a7..1a514e34e2c2 100644 --- a/packages/utils/.gitignore +++ b/packages/utils/.gitignore @@ -1,4 +1,3 @@ *.js.map *.d.ts -!src/*.d.ts *.js From 547945b10fafe46d932c4530b3a1864089db50fc Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Thu, 20 Dec 2018 17:01:00 +0100 Subject: [PATCH 04/16] feat: Pass string into transport, Use request buffer directly in transport --- packages/browser/src/backend.ts | 3 +- packages/browser/src/transports/base.ts | 4 +- packages/browser/src/transports/beacon.ts | 9 ++-- packages/browser/src/transports/fetch.ts | 7 ++- packages/browser/src/transports/xhr.ts | 7 ++- packages/core/src/basebackend.ts | 11 ---- packages/core/src/baseclient.ts | 42 +++------------- packages/core/src/interfaces.ts | 7 --- packages/core/src/requestbuffer.ts | 5 ++ packages/node/src/backend.ts | 4 +- packages/node/src/transports/base.ts | 61 ++++++++++++----------- packages/node/src/transports/http.ts | 8 +-- packages/node/src/transports/https.ts | 8 +-- packages/types/src/index.ts | 2 +- 14 files changed, 69 insertions(+), 109 deletions(-) diff --git a/packages/browser/src/backend.ts b/packages/browser/src/backend.ts index 3f831ef37270..ea42c1dec43f 100644 --- a/packages/browser/src/backend.ts +++ b/packages/browser/src/backend.ts @@ -2,6 +2,7 @@ import { BaseBackend, Options, SentryError } from '@sentry/core'; import { SentryEvent, SentryEventHint, SentryResponse, Severity, Status } from '@sentry/types'; import { isDOMError, isDOMException, isError, isErrorEvent, isPlainObject } from '@sentry/utils/is'; import { logger } from '@sentry/utils/logger'; +import { serialize } from '@sentry/utils/object'; import { supportsBeacon, supportsFetch } from '@sentry/utils/supports'; import { eventFromPlainObject, eventFromStacktrace, prepareFramesForEvent } from './parsers'; import { computeStackTrace } from './tracekit'; @@ -153,6 +154,6 @@ export class BrowserBackend extends BaseBackend { } } - return this.transport.captureEvent(event); + return this.transport.sendEvent(serialize(event)); } } diff --git a/packages/browser/src/transports/base.ts b/packages/browser/src/transports/base.ts index b63a5f231b40..8831e4546a5f 100644 --- a/packages/browser/src/transports/base.ts +++ b/packages/browser/src/transports/base.ts @@ -1,5 +1,5 @@ import { API, SentryError } from '@sentry/core'; -import { SentryEvent, SentryResponse, Transport, TransportOptions } from '@sentry/types'; +import { SentryResponse, Transport, TransportOptions } from '@sentry/types'; /** Base Transport class implementation */ export abstract class BaseTransport implements Transport { @@ -15,7 +15,7 @@ export abstract class BaseTransport implements Transport { /** * @inheritDoc */ - public async captureEvent(_: SentryEvent): Promise { + public async sendEvent(_: string): Promise { throw new SentryError('Transport Class has to implement `captureEvent` method'); } } diff --git a/packages/browser/src/transports/beacon.ts b/packages/browser/src/transports/beacon.ts index e61e89bdba0e..fd3d124dbd51 100644 --- a/packages/browser/src/transports/beacon.ts +++ b/packages/browser/src/transports/beacon.ts @@ -1,6 +1,5 @@ -import { SentryEvent, SentryResponse, Status } from '@sentry/types'; +import { SentryResponse, Status } from '@sentry/types'; import { getGlobalObject } from '@sentry/utils/misc'; -import { serialize } from '@sentry/utils/object'; import { BaseTransport } from './base'; const global = getGlobalObject() as Window; @@ -10,10 +9,8 @@ export class BeaconTransport extends BaseTransport { /** * @inheritDoc */ - public async captureEvent(event: SentryEvent): Promise { - const data = serialize(event); - - const result = global.navigator.sendBeacon(this.url, data); + public async sendEvent(body: string): Promise { + const result = global.navigator.sendBeacon(this.url, body); return { status: result ? Status.Success : Status.Failed, diff --git a/packages/browser/src/transports/fetch.ts b/packages/browser/src/transports/fetch.ts index ba947bfd1c64..7650e2de3618 100644 --- a/packages/browser/src/transports/fetch.ts +++ b/packages/browser/src/transports/fetch.ts @@ -1,6 +1,5 @@ -import { SentryEvent, SentryResponse, Status } from '@sentry/types'; +import { SentryResponse, Status } from '@sentry/types'; import { getGlobalObject } from '@sentry/utils/misc'; -import { serialize } from '@sentry/utils/object'; import { supportsReferrerPolicy } from '@sentry/utils/supports'; import { BaseTransport } from './base'; @@ -11,9 +10,9 @@ export class FetchTransport extends BaseTransport { /** * @inheritDoc */ - public async captureEvent(event: SentryEvent): Promise { + public async sendEvent(body: string): Promise { const defaultOptions: RequestInit = { - body: serialize(event), + body, method: 'POST', // Despite all stars in the sky saying that Edge supports old draft syntax, aka 'never', 'always', 'origin' and 'default // https://caniuse.com/#feat=referrer-policy diff --git a/packages/browser/src/transports/xhr.ts b/packages/browser/src/transports/xhr.ts index 039633a53998..3821ed2ba70b 100644 --- a/packages/browser/src/transports/xhr.ts +++ b/packages/browser/src/transports/xhr.ts @@ -1,5 +1,4 @@ -import { SentryEvent, SentryResponse, Status } from '@sentry/types'; -import { serialize } from '@sentry/utils/object'; +import { SentryResponse, Status } from '@sentry/types'; import { BaseTransport } from './base'; /** `XHR` based transport */ @@ -7,7 +6,7 @@ export class XHRTransport extends BaseTransport { /** * @inheritDoc */ - public async captureEvent(event: SentryEvent): Promise { + public async sendEvent(body: string): Promise { return new Promise((resolve, reject) => { const request = new XMLHttpRequest(); @@ -26,7 +25,7 @@ export class XHRTransport extends BaseTransport { }; request.open('POST', this.url); - request.send(serialize(event)); + request.send(body); }); } } diff --git a/packages/core/src/basebackend.ts b/packages/core/src/basebackend.ts index bec82c2579f9..1d39b5b3c15e 100644 --- a/packages/core/src/basebackend.ts +++ b/packages/core/src/basebackend.ts @@ -3,7 +3,6 @@ import { Breadcrumb, SentryEvent, SentryEventHint, SentryResponse, Severity, Tra import { logger } from '@sentry/utils/logger'; import { SentryError } from './error'; import { Backend, Options } from './interfaces'; -import { RequestBuffer } from './requestbuffer'; /** A class object that can instanciate Backend objects. */ export interface BackendClass { @@ -28,9 +27,6 @@ export abstract class BaseBackend implements Backend { /** Cached transport used internally. */ protected transport?: Transport; - /** A simple buffer holding all requests. */ - protected readonly buffer: RequestBuffer = new RequestBuffer(); - /** * @inheritDoc */ @@ -65,11 +61,4 @@ export abstract class BaseBackend implements Backend { public storeScope(_: Scope): void { // Noop } - - /** - * @inheritDoc - */ - public getBuffer(): RequestBuffer { - return this.buffer; - } } diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 00fd899bd7f3..3a24a4ac4f5c 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -128,27 +128,12 @@ export abstract class BaseClient implement return (this.installed = true); } - /** - * Internal helper function to buffer promises. - * - * @param promise Any promise, but in this case Promise. - */ - protected async buffer(promise: Promise): Promise { - return this.getBackend() - .getBuffer() - .add(promise); - } - /** * @inheritDoc */ public async captureException(exception: any, hint?: SentryEventHint, scope?: Scope): Promise { - return this.buffer( - (async () => { - const event = await this.getBackend().eventFromException(exception, hint); - return this.captureEvent(event, hint, scope); - })(), - ); + const event = await this.getBackend().eventFromException(exception, hint); + return this.captureEvent(event, hint, scope); } /** @@ -160,25 +145,15 @@ export abstract class BaseClient implement hint?: SentryEventHint, scope?: Scope, ): Promise { - return this.buffer( - (async () => { - const event = await this.getBackend().eventFromMessage(message, level, hint); - return this.captureEvent(event, hint, scope); - })(), - ); + const event = await this.getBackend().eventFromMessage(message, level, hint); + return this.captureEvent(event, hint, scope); } /** * @inheritDoc */ public async captureEvent(event: SentryEvent, hint?: SentryEventHint, scope?: Scope): Promise { - // Adding this here is technically not correct since if you call captureMessage/captureException it's already - // buffered. But since we not really need the count and we only need to know if the buffer is full or not, - // This is fine... - return this.buffer( - (async () => - this.processEvent(event, async finalEvent => this.getBackend().sendEvent(finalEvent), hint, scope))(), - ); + return this.processEvent(event, async finalEvent => this.getBackend().sendEvent(finalEvent), hint, scope); } /** @@ -380,10 +355,9 @@ export abstract class BaseClient implement /** * @inheritDoc */ - public async close(timeout?: number): Promise { - return this.getBackend() - .getBuffer() - .drain(timeout); + public async close(_?: number): Promise { + // TODO + return true; } /** diff --git a/packages/core/src/interfaces.ts b/packages/core/src/interfaces.ts index 6bdf6864a73d..657508fcb6db 100644 --- a/packages/core/src/interfaces.ts +++ b/packages/core/src/interfaces.ts @@ -13,7 +13,6 @@ import { TransportOptions, } from '@sentry/types'; import { Dsn } from './dsn'; -import { RequestBuffer } from './requestbuffer'; /** Console logging verbosity for the SDK. */ export enum LogLevel { @@ -277,10 +276,4 @@ export interface Backend { * @param scope The scope to store. */ storeScope(scope: Scope): void; - - /** - * Returns the internal instance of the request buffer. - * Only used internally. - */ - getBuffer(): RequestBuffer; } diff --git a/packages/core/src/requestbuffer.ts b/packages/core/src/requestbuffer.ts index 18cde1e744e2..5dec902dde03 100644 --- a/packages/core/src/requestbuffer.ts +++ b/packages/core/src/requestbuffer.ts @@ -1,5 +1,7 @@ /** A simple queue that holds promises. */ export class RequestBuffer { + public constructor(protected limit: number = 30) {} + /** Internal set of queued Promises */ private readonly buffer: Array> = []; @@ -10,6 +12,9 @@ export class RequestBuffer { * @returns The original promise. */ public async add(task: Promise): Promise { + if (this.length() >= this.limit) { + return Promise.reject('Discarded'); + } if (this.buffer.indexOf(task) === -1) { this.buffer.push(task); } diff --git a/packages/node/src/backend.ts b/packages/node/src/backend.ts index 76b9ee46a060..2a7306ce6fa9 100644 --- a/packages/node/src/backend.ts +++ b/packages/node/src/backend.ts @@ -1,7 +1,7 @@ import { BaseBackend, Dsn, getCurrentHub, Options, SentryError } from '@sentry/core'; import { SentryEvent, SentryEventHint, SentryResponse, Severity } from '@sentry/types'; import { isError, isPlainObject } from '@sentry/utils/is'; -import { limitObjectDepthToSize, serializeKeysToEventMessage } from '@sentry/utils/object'; +import { limitObjectDepthToSize, serialize, serializeKeysToEventMessage } from '@sentry/utils/object'; import { createHash } from 'crypto'; import { extractStackFromError, parseError, parseStack, prepareFramesForEvent } from './parsers'; import { HTTPSTransport, HTTPTransport } from './transports'; @@ -130,6 +130,6 @@ export class NodeBackend extends BaseBackend { : new HTTPSTransport(transportOptions); } - return this.transport.captureEvent(event); + return this.transport.sendEvent(serialize(event)); } } diff --git a/packages/node/src/transports/base.ts b/packages/node/src/transports/base.ts index f8ac6a8f845e..e63eaf20a743 100644 --- a/packages/node/src/transports/base.ts +++ b/packages/node/src/transports/base.ts @@ -1,6 +1,5 @@ -import { API, SentryError } from '@sentry/core'; -import { SentryEvent, SentryResponse, Status, Transport, TransportOptions } from '@sentry/types'; -import { serialize } from '@sentry/utils/object'; +import { API, RequestBuffer, SentryError } from '@sentry/core'; +import { SentryResponse, Status, Transport, TransportOptions } from '@sentry/types'; import * as fs from 'fs'; import * as http from 'http'; import * as https from 'https'; @@ -26,6 +25,9 @@ export abstract class BaseTransport implements Transport { /** The Agent used for corresponding transport */ public client?: http.Agent | https.Agent; + /** A simple buffer holding all requests. */ + protected readonly buffer: RequestBuffer = new RequestBuffer(30); + /** Create instance and set this.dsn */ public constructor(public options: TransportOptions) { this.api = new API(options.dsn); @@ -59,40 +61,41 @@ export abstract class BaseTransport implements Transport { } /** JSDoc */ - protected async sendWithModule(httpModule: HTTPRequest, event: SentryEvent): Promise { - const requestOptions = this.getRequestOptions(); - return new Promise((resolve, reject) => { - const req = httpModule.request(requestOptions, (res: http.IncomingMessage) => { - res.setEncoding('utf8'); - if (res.statusCode && res.statusCode >= 200 && res.statusCode < 300) { - resolve({ - status: Status.fromHttpCode(res.statusCode), - }); - } else { - if (res.headers && res.headers['x-sentry-error']) { - const reason = res.headers['x-sentry-error']; - reject(new SentryError(`HTTP Error (${res.statusCode}): ${reason}`)); + protected async sendWithModule(httpModule: HTTPRequest, body: string): Promise { + return this.buffer.add( + new Promise((resolve, reject) => { + const req = httpModule.request(this.getRequestOptions(), (res: http.IncomingMessage) => { + res.setEncoding('utf8'); + if (res.statusCode && res.statusCode >= 200 && res.statusCode < 300) { + resolve({ + status: Status.fromHttpCode(res.statusCode), + }); } else { - reject(new SentryError(`HTTP Error (${res.statusCode})`)); + if (res.headers && res.headers['x-sentry-error']) { + const reason = res.headers['x-sentry-error']; + reject(new SentryError(`HTTP Error (${res.statusCode}): ${reason}`)); + } else { + reject(new SentryError(`HTTP Error (${res.statusCode})`)); + } } - } - // force the socket to drain - res.on('data', () => { - // Drain - }); - res.on('end', () => { - // Drain + // force the socket to drain + res.on('data', () => { + // Drain + }); + res.on('end', () => { + // Drain + }); }); - }); - req.on('error', reject); - req.end(serialize(event)); - }); + req.on('error', reject); + req.end(body); + }), + ); } /** * @inheritDoc */ - public async captureEvent(_: SentryEvent): Promise { + public async sendEvent(_: string): Promise { throw new SentryError('Transport Class has to implement `captureEvent` method'); } } diff --git a/packages/node/src/transports/http.ts b/packages/node/src/transports/http.ts index 0882cbea6bcc..4be6ce3ca8f8 100644 --- a/packages/node/src/transports/http.ts +++ b/packages/node/src/transports/http.ts @@ -1,5 +1,5 @@ import { SentryError } from '@sentry/core'; -import { SentryEvent, SentryResponse, TransportOptions } from '@sentry/types'; +import { SentryResponse, TransportOptions } from '@sentry/types'; import * as http from 'http'; import * as HttpsProxyAgent from 'https-proxy-agent'; import { BaseTransport } from './base'; @@ -14,16 +14,16 @@ export class HTTPTransport extends BaseTransport { this.client = proxy ? // tslint:disable-next-line:no-unsafe-any (new HttpsProxyAgent(proxy) as http.Agent) - : new http.Agent({ keepAlive: true, maxSockets: 100 }); + : new http.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 }); } /** * @inheritDoc */ - public async captureEvent(event: SentryEvent): Promise { + public async sendEvent(body: string): Promise { if (!this.module) { throw new SentryError('No module available in HTTPTransport'); } - return this.sendWithModule(this.module, event); + return this.sendWithModule(this.module, body); } } diff --git a/packages/node/src/transports/https.ts b/packages/node/src/transports/https.ts index 0035f90f7354..7e0999f46683 100644 --- a/packages/node/src/transports/https.ts +++ b/packages/node/src/transports/https.ts @@ -1,5 +1,5 @@ import { SentryError } from '@sentry/core'; -import { SentryEvent, SentryResponse, TransportOptions } from '@sentry/types'; +import { SentryResponse, TransportOptions } from '@sentry/types'; import * as https from 'https'; import * as HttpsProxyAgent from 'https-proxy-agent'; import { BaseTransport } from './base'; @@ -14,17 +14,17 @@ export class HTTPSTransport extends BaseTransport { this.client = proxy ? // tslint:disable-next-line:no-unsafe-any (new HttpsProxyAgent(proxy) as https.Agent) - : new https.Agent({ keepAlive: true, maxSockets: 100 }); + : new https.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 }); } /** * @inheritDoc */ - public async captureEvent(event: SentryEvent): Promise { + public async sendEvent(body: string): Promise { if (!this.module) { throw new SentryError('No module available in HTTPSTransport'); } - return this.sendWithModule(this.module, event); + return this.sendWithModule(this.module, body); } } diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 5c64ce04e5ff..86e11c20153c 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -240,7 +240,7 @@ export interface TransportOptions { /** JSDoc */ export interface Transport { - captureEvent(event: SentryEvent): Promise; + sendEvent(body: string): Promise; } /** JSDoc */ From d60be011b2ec8d85af7beca192cae8e1124e7b46 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Fri, 21 Dec 2018 11:30:38 +0100 Subject: [PATCH 05/16] feat: Use buffer in transport --- packages/browser/src/backend.ts | 55 ++++++++----------- packages/browser/src/transports/base.ts | 15 ++++- packages/browser/src/transports/beacon.ts | 8 ++- packages/browser/src/transports/fetch.ts | 10 ++-- packages/browser/src/transports/xhr.ts | 34 ++++++------ packages/browser/test/backend.test.ts | 14 +---- packages/browser/test/index.test.ts | 31 ++--------- .../browser/test/mocks/simpletransport.ts | 12 ++++ packages/browser/test/transports/base.test.ts | 6 +- .../browser/test/transports/beacon.test.ts | 6 +- .../browser/test/transports/fetch.test.ts | 6 +- packages/browser/test/transports/xhr.test.ts | 6 +- packages/core/src/basebackend.ts | 26 +++++++-- packages/core/src/baseclient.ts | 18 +++++- packages/core/src/index.ts | 1 + packages/core/src/interfaces.ts | 8 +++ packages/core/src/transports/noop.ts | 21 +++++++ packages/node/src/backend.ts | 39 ++++++++++++- packages/node/src/transports/base.ts | 19 ++++++- packages/node/src/transports/https.ts | 1 - packages/types/src/index.ts | 13 ++++- 21 files changed, 228 insertions(+), 121 deletions(-) create mode 100644 packages/browser/test/mocks/simpletransport.ts create mode 100644 packages/core/src/transports/noop.ts diff --git a/packages/browser/src/backend.ts b/packages/browser/src/backend.ts index ea42c1dec43f..4e173d3c6da8 100644 --- a/packages/browser/src/backend.ts +++ b/packages/browser/src/backend.ts @@ -1,8 +1,6 @@ -import { BaseBackend, Options, SentryError } from '@sentry/core'; -import { SentryEvent, SentryEventHint, SentryResponse, Severity, Status } from '@sentry/types'; +import { BaseBackend, NoopTransport, Options, SentryError } from '@sentry/core'; +import { SentryEvent, SentryEventHint, Severity, Transport } from '@sentry/types'; import { isDOMError, isDOMException, isError, isErrorEvent, isPlainObject } from '@sentry/utils/is'; -import { logger } from '@sentry/utils/logger'; -import { serialize } from '@sentry/utils/object'; import { supportsBeacon, supportsFetch } from '@sentry/utils/supports'; import { eventFromPlainObject, eventFromStacktrace, prepareFramesForEvent } from './parsers'; import { computeStackTrace } from './tracekit'; @@ -47,6 +45,26 @@ export class BrowserBackend extends BaseBackend { return true; } + /** + * @inheritdoc + */ + protected setupTransport(): Transport { + if (!this.options.dsn) { + return new NoopTransport(); + } + + const transportOptions = this.options.transportOptions ? this.options.transportOptions : { dsn: this.options.dsn }; + + if (this.options.transport) { + return new this.options.transport(transportOptions); + } else if (supportsBeacon()) { + return new BeaconTransport(transportOptions); + } else if (supportsFetch()) { + return new FetchTransport(transportOptions); + } + return new XHRTransport(transportOptions); + } + /** * @inheritDoc */ @@ -127,33 +145,4 @@ export class BrowserBackend extends BaseBackend { return event; } - - /** - * @inheritDoc - */ - public async sendEvent(event: SentryEvent): Promise { - if (!this.options.dsn) { - logger.warn(`Event has been skipped because no Dsn is configured.`); - // We do nothing in case there is no DSN - return { status: Status.Skipped, reason: `Event has been skipped because no Dsn is configured.` }; - } - - if (!this.transport) { - const transportOptions = this.options.transportOptions - ? this.options.transportOptions - : { dsn: this.options.dsn }; - - if (this.options.transport) { - this.transport = new this.options.transport({ dsn: this.options.dsn }); - } else if (supportsBeacon()) { - this.transport = new BeaconTransport(transportOptions); - } else if (supportsFetch()) { - this.transport = new FetchTransport(transportOptions); - } else { - this.transport = new XHRTransport(transportOptions); - } - } - - return this.transport.sendEvent(serialize(event)); - } } diff --git a/packages/browser/src/transports/base.ts b/packages/browser/src/transports/base.ts index 8831e4546a5f..79e2f5e781f4 100644 --- a/packages/browser/src/transports/base.ts +++ b/packages/browser/src/transports/base.ts @@ -1,4 +1,4 @@ -import { API, SentryError } from '@sentry/core'; +import { API, RequestBuffer, SentryError } from '@sentry/core'; import { SentryResponse, Transport, TransportOptions } from '@sentry/types'; /** Base Transport class implementation */ @@ -8,6 +8,9 @@ export abstract class BaseTransport implements Transport { */ public url: string; + /** A simple buffer holding all requests. */ + protected readonly buffer: RequestBuffer = new RequestBuffer(30); + public constructor(public options: TransportOptions) { this.url = new API(this.options.dsn).getStoreEndpointWithUrlEncodedAuth(); } @@ -16,6 +19,14 @@ export abstract class BaseTransport implements Transport { * @inheritDoc */ public async sendEvent(_: string): Promise { - throw new SentryError('Transport Class has to implement `captureEvent` method'); + throw new SentryError('Transport Class has to implement `sendEvent` method'); + } + + /** + * @inheritDoc + */ + public close(timeout?: number): Promise { + console.log('calling close'); + return this.buffer.drain(timeout); } } diff --git a/packages/browser/src/transports/beacon.ts b/packages/browser/src/transports/beacon.ts index fd3d124dbd51..4bd547d7a683 100644 --- a/packages/browser/src/transports/beacon.ts +++ b/packages/browser/src/transports/beacon.ts @@ -12,8 +12,10 @@ export class BeaconTransport extends BaseTransport { public async sendEvent(body: string): Promise { const result = global.navigator.sendBeacon(this.url, body); - return { - status: result ? Status.Success : Status.Failed, - }; + return this.buffer.add( + Promise.resolve({ + status: result ? Status.Success : Status.Failed, + }), + ); } } diff --git a/packages/browser/src/transports/fetch.ts b/packages/browser/src/transports/fetch.ts index 7650e2de3618..c07087c1d794 100644 --- a/packages/browser/src/transports/fetch.ts +++ b/packages/browser/src/transports/fetch.ts @@ -21,10 +21,10 @@ export class FetchTransport extends BaseTransport { referrerPolicy: (supportsReferrerPolicy() ? 'origin' : '') as ReferrerPolicy, }; - const response = await global.fetch(this.url, defaultOptions); - - return { - status: Status.fromHttpCode(response.status), - }; + return this.buffer.add( + global.fetch(this.url, defaultOptions).then(response => ({ + status: Status.fromHttpCode(response.status), + })), + ); } } diff --git a/packages/browser/src/transports/xhr.ts b/packages/browser/src/transports/xhr.ts index 3821ed2ba70b..a6e5af9a48c9 100644 --- a/packages/browser/src/transports/xhr.ts +++ b/packages/browser/src/transports/xhr.ts @@ -7,25 +7,27 @@ export class XHRTransport extends BaseTransport { * @inheritDoc */ public async sendEvent(body: string): Promise { - return new Promise((resolve, reject) => { - const request = new XMLHttpRequest(); + return this.buffer.add( + new Promise((resolve, reject) => { + const request = new XMLHttpRequest(); - request.onreadystatechange = () => { - if (request.readyState !== 4) { - return; - } + request.onreadystatechange = () => { + if (request.readyState !== 4) { + return; + } - if (request.status === 200) { - resolve({ - status: Status.fromHttpCode(request.status), - }); - } + if (request.status === 200) { + resolve({ + status: Status.fromHttpCode(request.status), + }); + } - reject(request); - }; + reject(request); + }; - request.open('POST', this.url); - request.send(body); - }); + request.open('POST', this.url); + request.send(body); + }), + ); } } diff --git a/packages/browser/test/backend.test.ts b/packages/browser/test/backend.test.ts index d48b3179b7e5..413cd91fe2e6 100644 --- a/packages/browser/test/backend.test.ts +++ b/packages/browser/test/backend.test.ts @@ -1,15 +1,7 @@ import { expect } from 'chai'; -import { SentryEvent, SentryResponse, Status } from '../src'; +import { Status } from '../src'; import { BrowserBackend } from '../src/backend'; -import { BaseTransport } from '../src/transports'; - -class SimpleTransport extends BaseTransport { - public async captureEvent(_: SentryEvent): Promise { - return { - status: Status.fromHttpCode(200), - }; - } -} +import { SimpleTransport } from './mocks/simpletransport'; const dsn = 'https://123@sentry.io/42'; const testEvent = { @@ -34,7 +26,7 @@ describe('BrowserBackend', () => { } }); - it('should call captureEvent() on provided transport', async () => { + it('should call sendEvent() on provided transport', async () => { backend = new BrowserBackend({ dsn, transport: SimpleTransport }); const status = await backend.sendEvent(testEvent); expect(status.status).equal(Status.Success); diff --git a/packages/browser/test/index.test.ts b/packages/browser/test/index.test.ts index 729afaadec59..a50ac003cd19 100644 --- a/packages/browser/test/index.test.ts +++ b/packages/browser/test/index.test.ts @@ -1,8 +1,7 @@ import { expect } from 'chai'; -import { SinonSpy, spy, stub } from 'sinon'; +import { SinonSpy, spy } from 'sinon'; import { addBreadcrumb, - BrowserBackend, BrowserClient, captureEvent, captureException, @@ -13,20 +12,21 @@ import { Integrations, Scope, SentryEvent, - Status, } from '../src'; +import { SimpleTransport } from './mocks/simpletransport'; const dsn = 'https://53039209a22b4ec1bcc296a3c9fdecd6@sentry.io/4291'; declare var global: any; describe('SentryBrowser', () => { - const beforeSend: SinonSpy = spy(); + const beforeSend: SinonSpy = spy((event: SentryEvent) => event); before(() => { init({ beforeSend, dsn, + transport: SimpleTransport, }); }); @@ -69,37 +69,18 @@ describe('SentryBrowser', () => { }); describe('breadcrumbs', () => { - let s: sinon.SinonStub; - - beforeEach(() => { - s = stub(BrowserBackend.prototype, 'sendEvent').returns(Promise.resolve({ status: Status.Success })); - }); - - afterEach(() => { - s.restore(); - }); - - it('should record breadcrumbs', async () => { + it.only('should record breadcrumbs', async () => { addBreadcrumb({ message: 'test1' }); addBreadcrumb({ message: 'test2' }); captureMessage('event'); await (getCurrentHub().getClient() as BrowserClient).close(2000); + console.log(beforeSend.calledOnce); expect(beforeSend.args[0][0].breadcrumbs).to.have.lengthOf(2); }); }); describe('capture', () => { - let s: sinon.SinonStub; - - beforeEach(() => { - s = stub(BrowserBackend.prototype, 'sendEvent').returns(Promise.resolve({ status: Status.Success })); - }); - - afterEach(() => { - s.restore(); - }); - it('should capture an exception', async () => { try { throw new Error('test'); diff --git a/packages/browser/test/mocks/simpletransport.ts b/packages/browser/test/mocks/simpletransport.ts new file mode 100644 index 000000000000..2e57687e37be --- /dev/null +++ b/packages/browser/test/mocks/simpletransport.ts @@ -0,0 +1,12 @@ +import { SentryResponse, Status } from '../../src'; +import { BaseTransport } from '../../src/transports'; + +export class SimpleTransport extends BaseTransport { + public async sendEvent(_: string): Promise { + return this.buffer.add( + Promise.resolve({ + status: Status.fromHttpCode(200), + }), + ); + } +} diff --git a/packages/browser/test/transports/base.test.ts b/packages/browser/test/transports/base.test.ts index b1ca8656a8f3..907eb6190c88 100644 --- a/packages/browser/test/transports/base.test.ts +++ b/packages/browser/test/transports/base.test.ts @@ -6,13 +6,13 @@ const testDsn = 'https://123@sentry.io/42'; class SimpleTransport extends BaseTransport {} describe('BaseTransport', () => { - it('doesnt provide captureEvent() implementation', async () => { + it('doesnt provide sendEvent() implementation', async () => { const transport = new SimpleTransport({ dsn: testDsn }); try { - await transport.captureEvent({}); + await transport.sendEvent(''); } catch (e) { - expect(e.message).equal('Transport Class has to implement `captureEvent` method'); + expect(e.message).equal('Transport Class has to implement `sendEvent` method'); } }); diff --git a/packages/browser/test/transports/beacon.test.ts b/packages/browser/test/transports/beacon.test.ts index 3dc0beea7fb0..9d6bce60be74 100644 --- a/packages/browser/test/transports/beacon.test.ts +++ b/packages/browser/test/transports/beacon.test.ts @@ -29,11 +29,11 @@ describe('BeaconTransport', () => { expect(transport.url).equal(transportUrl); }); - describe('captureEvent()', async () => { + describe('sendEvent()', async () => { it('sends a request to Sentry servers', async () => { sendBeacon.returns(true); - return transport.captureEvent(payload).then(res => { + return transport.sendEvent(JSON.stringify(payload)).then(res => { expect(res.status).equal(Status.Success); expect(sendBeacon.calledOnce).equal(true); expect(sendBeacon.calledWith(transportUrl, JSON.stringify(payload))).equal(true); @@ -43,7 +43,7 @@ describe('BeaconTransport', () => { it('rejects with failed status', async () => { sendBeacon.returns(false); - return transport.captureEvent(payload).catch(res => { + return transport.sendEvent(JSON.stringify(payload)).catch(res => { expect(res.status).equal(Status.Failed); expect(sendBeacon.calledOnce).equal(true); expect(sendBeacon.calledWith(transportUrl, JSON.stringify(payload))).equal(true); diff --git a/packages/browser/test/transports/fetch.test.ts b/packages/browser/test/transports/fetch.test.ts index 92ded06ea5e9..b7be26bb3e46 100644 --- a/packages/browser/test/transports/fetch.test.ts +++ b/packages/browser/test/transports/fetch.test.ts @@ -29,13 +29,13 @@ describe('FetchTransport', () => { expect(transport.url).equal(transportUrl); }); - describe('captureEvent()', async () => { + describe('sendEvent()', async () => { it('sends a request to Sentry servers', async () => { const response = { status: 200 }; fetch.returns(Promise.resolve(response)); - return transport.captureEvent(payload).then(res => { + return transport.sendEvent(JSON.stringify(payload)).then(res => { expect(res.status).equal(Status.Success); expect(fetch.calledOnce).equal(true); expect( @@ -53,7 +53,7 @@ describe('FetchTransport', () => { fetch.returns(Promise.reject(response)); - return transport.captureEvent(payload).catch(res => { + return transport.sendEvent(JSON.stringify(payload)).catch(res => { expect(res.status).equal(403); expect(fetch.calledOnce).equal(true); expect( diff --git a/packages/browser/test/transports/xhr.test.ts b/packages/browser/test/transports/xhr.test.ts index 9082dbede2f3..ef586c1778cd 100644 --- a/packages/browser/test/transports/xhr.test.ts +++ b/packages/browser/test/transports/xhr.test.ts @@ -30,11 +30,11 @@ describe('XHRTransport', () => { expect(transport.url).equal(transportUrl); }); - describe('captureEvent()', async () => { + describe('sendEvent()', async () => { it('sends a request to Sentry servers', async () => { server.respondWith('POST', transportUrl, [200, {}, '']); - return transport.captureEvent(payload).then(res => { + return transport.sendEvent(JSON.stringify(payload)).then(res => { expect(res.status).equal(Status.Success); const request = server.requests[0]; expect(server.requests.length).equal(1); @@ -46,7 +46,7 @@ describe('XHRTransport', () => { it('rejects with non-200 status code', done => { server.respondWith('POST', transportUrl, [403, {}, '']); - transport.captureEvent(payload).catch(res => { + transport.sendEvent(JSON.stringify(payload)).catch(res => { expect(res.status).equal(403); const request = server.requests[0]; diff --git a/packages/core/src/basebackend.ts b/packages/core/src/basebackend.ts index 1d39b5b3c15e..a0773bf7857f 100644 --- a/packages/core/src/basebackend.ts +++ b/packages/core/src/basebackend.ts @@ -1,6 +1,7 @@ import { Scope } from '@sentry/hub'; import { Breadcrumb, SentryEvent, SentryEventHint, SentryResponse, Severity, Transport } from '@sentry/types'; import { logger } from '@sentry/utils/logger'; +import { serialize } from '@sentry/utils/object'; import { SentryError } from './error'; import { Backend, Options } from './interfaces'; @@ -16,16 +17,24 @@ export abstract class BaseBackend implements Backend { /** Options passed to the SDK. */ protected readonly options: O; + /** Cached transport used internally. */ + protected transport: Transport; + /** Creates a new browser backend instance. */ public constructor(options: O) { this.options = options; if (!this.options.dsn) { logger.warn('No DSN provided, backend will not do anything.'); } + this.transport = this.setupTransport(); } - /** Cached transport used internally. */ - protected transport?: Transport; + /** + * Sets up the transport so it can be used later to send requests. + */ + protected setupTransport(): Transport { + throw new SentryError('Backend has to implement `setupTransport` method'); + } /** * @inheritDoc @@ -44,8 +53,10 @@ export abstract class BaseBackend implements Backend { /** * @inheritDoc */ - public async sendEvent(_event: SentryEvent): Promise { - throw new SentryError('Backend has to implement `sendEvent` method'); + public async sendEvent(event: SentryEvent): Promise { + const response = await this.transport.sendEvent(serialize(event)); + logger.log(`Request finished with: ${response}`); + return response; } /** @@ -61,4 +72,11 @@ export abstract class BaseBackend implements Backend { public storeScope(_: Scope): void { // Noop } + + /** + * @inheritDoc + */ + public getTransport(): Transport { + return this.transport; + } } diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 3a24a4ac4f5c..db6411b2eb55 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -313,6 +313,7 @@ export abstract class BaseClient implement try { const isInternalException = hint && hint.data && (hint.data as { [key: string]: any }).__sentry__ === true; if (!isInternalException && beforeSend) { + console.log('would call before send'); finalEvent = await beforeSend(prepared, hint); if ((typeof finalEvent as any) === 'undefined') { logger.error('`beforeSend` method has to return `null` or a valid event'); @@ -355,9 +356,20 @@ export abstract class BaseClient implement /** * @inheritDoc */ - public async close(_?: number): Promise { - // TODO - return true; + public async close(timeout?: number): Promise { + return this.getBackend() + .getTransport() + .close(timeout); + + return new Promise(resolve => { + setTimeout(() => { + resolve( + this.getBackend() + .getTransport() + .close(timeout), + ); + }, 100); + }); } /** diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 06592c02b159..b6237bad8d18 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -15,6 +15,7 @@ export { SentryError } from './error'; export { RequestBuffer } from './requestbuffer'; export { Backend, Client, LogLevel, Options } from './interfaces'; export { initAndBind, ClientClass } from './sdk'; +export { NoopTransport } from './transports/noop'; import * as Integrations from './integrations'; diff --git a/packages/core/src/interfaces.ts b/packages/core/src/interfaces.ts index 657508fcb6db..5bcf9f4811ae 100644 --- a/packages/core/src/interfaces.ts +++ b/packages/core/src/interfaces.ts @@ -276,4 +276,12 @@ export interface Backend { * @param scope The scope to store. */ storeScope(scope: Scope): void; + + /** + * Returns the transport that is used by the backend. + * Please note that the transport gets lazy initialized so it will only be there once the first event has been sent. + * + * @returns The transport. + */ + getTransport(): Transport; } diff --git a/packages/core/src/transports/noop.ts b/packages/core/src/transports/noop.ts new file mode 100644 index 000000000000..2e66d25e66a0 --- /dev/null +++ b/packages/core/src/transports/noop.ts @@ -0,0 +1,21 @@ +import { SentryResponse, Status, Transport } from '@sentry/types'; + +/** Noop transport */ +export class NoopTransport implements Transport { + /** + * @inheritDoc + */ + public async sendEvent(_: string): Promise { + return Promise.resolve({ + reason: `NoopTransport: Event has been skipped because no Dsn is configured.`, + status: Status.Skipped, + }); + } + + /** + * @inheritDoc + */ + public close(_?: number): Promise { + return Promise.resolve(true); + } +} diff --git a/packages/node/src/backend.ts b/packages/node/src/backend.ts index 2a7306ce6fa9..972c9a62472a 100644 --- a/packages/node/src/backend.ts +++ b/packages/node/src/backend.ts @@ -1,6 +1,7 @@ -import { BaseBackend, Dsn, getCurrentHub, Options, SentryError } from '@sentry/core'; -import { SentryEvent, SentryEventHint, SentryResponse, Severity } from '@sentry/types'; +import { BaseBackend, Dsn, getCurrentHub, NoopTransport, Options } from '@sentry/core'; +import { SentryEvent, SentryEventHint, SentryResponse, Severity, Status, Transport } from '@sentry/types'; import { isError, isPlainObject } from '@sentry/utils/is'; +import { logger } from '@sentry/utils/logger'; import { limitObjectDepthToSize, serialize, serializeKeysToEventMessage } from '@sentry/utils/object'; import { createHash } from 'crypto'; import { extractStackFromError, parseError, parseStack, prepareFramesForEvent } from './parsers'; @@ -37,6 +38,36 @@ export interface NodeOptions extends Options { /** The Sentry Node SDK Backend. */ export class NodeBackend extends BaseBackend { + /** + * @inheritdoc + */ + protected setupTransport(): Transport { + let dsn: Dsn | undefined; + + if (!this.options.dsn) { + return new NoopTransport(); + } + + dsn = new Dsn(this.options.dsn); + + const transportOptions = this.options.transportOptions ? this.options.transportOptions : { dsn }; + const clientOptions = ['httpProxy', 'httpsProxy', 'caCerts']; + + for (const option of clientOptions) { + if (this.options[option]) { + transportOptions[option] = transportOptions[option] || this.options[option]; + } + } + + if (this.options.transport) { + return new this.options.transport(transportOptions); + } else if (dsn.protocol === 'http') { + return new HTTPTransport(transportOptions); + } else { + return new HTTPSTransport(transportOptions); + } + } + /** * @inheritDoc */ @@ -108,7 +139,9 @@ export class NodeBackend extends BaseBackend { let dsn: Dsn; if (!this.options.dsn) { - throw new SentryError('Cannot sendEvent without a valid DSN'); + logger.warn(`Event has been skipped because no Dsn is configured.`); + // We do nothing in case there is no DSN + return { status: Status.Skipped, reason: `Event has been skipped because no Dsn is configured.` }; } else { dsn = new Dsn(this.options.dsn); } diff --git a/packages/node/src/transports/base.ts b/packages/node/src/transports/base.ts index e63eaf20a743..b8ed5b909829 100644 --- a/packages/node/src/transports/base.ts +++ b/packages/node/src/transports/base.ts @@ -17,7 +17,7 @@ export interface HTTPRequest { /** Base Transport class implementation */ export abstract class BaseTransport implements Transport { /** API object */ - protected api: API; + protected api?: API; /** The Agent used for corresponding transport */ public module?: HTTPRequest; @@ -35,6 +35,10 @@ export abstract class BaseTransport implements Transport { /** Returns a build request option object used by request */ protected getRequestOptions(): http.RequestOptions | https.RequestOptions { + if (!this.api) { + return {}; + } + const headers = { ...this.api.getRequestHeaders(SDK_NAME, SDK_VERSION), ...this.options.headers, @@ -64,6 +68,10 @@ export abstract class BaseTransport implements Transport { protected async sendWithModule(httpModule: HTTPRequest, body: string): Promise { return this.buffer.add( new Promise((resolve, reject) => { + if (!this.api) { + resolve({ status: Status.Skipped, reason: `Event has been skipped because no Dsn is configured.` }); + return; + } const req = httpModule.request(this.getRequestOptions(), (res: http.IncomingMessage) => { res.setEncoding('utf8'); if (res.statusCode && res.statusCode >= 200 && res.statusCode < 300) { @@ -96,6 +104,13 @@ export abstract class BaseTransport implements Transport { * @inheritDoc */ public async sendEvent(_: string): Promise { - throw new SentryError('Transport Class has to implement `captureEvent` method'); + throw new SentryError('Transport Class has to implement `sendEvent` method'); + } + + /** + * @inheritDoc + */ + public close(timeout?: number): Promise { + return this.buffer.drain(timeout); } } diff --git a/packages/node/src/transports/https.ts b/packages/node/src/transports/https.ts index 7e0999f46683..35ca3353bde8 100644 --- a/packages/node/src/transports/https.ts +++ b/packages/node/src/transports/https.ts @@ -24,7 +24,6 @@ export class HTTPSTransport extends BaseTransport { if (!this.module) { throw new SentryError('No module available in HTTPSTransport'); } - return this.sendWithModule(this.module, body); } } diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 86e11c20153c..1e94ee3d4a23 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -238,9 +238,20 @@ export interface TransportOptions { caCerts?: string; } -/** JSDoc */ +/** Transport used sending data to Sentry */ export interface Transport { + /** + * Sends the body to the Store endpoint in Sentry. + * + * @param body String body that should be sent to Sentry. + */ sendEvent(body: string): Promise; + /** + * Call this function to wait until all pending requests have been sent. + * + * @param timeout Number time in ms to wait until the buffer is drained. + */ + close(timeout?: number): Promise; } /** JSDoc */ From 53e634163a02f5055d71edf68d3c13b10132dc5a Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Fri, 21 Dec 2018 13:33:29 +0100 Subject: [PATCH 06/16] feat: Add all changes for transport and buffers --- packages/browser/src/backend.ts | 5 +- packages/browser/src/transports/base.ts | 5 +- packages/browser/test/index.test.ts | 3 +- packages/browser/test/integration/init.js | 2 +- packages/core/src/basebackend.ts | 7 +- packages/core/src/baseclient.ts | 71 ++++++++++++------- packages/core/src/index.ts | 2 +- .../{requestbuffer.ts => promisebuffer.ts} | 17 +++-- packages/core/src/transports/noop.ts | 2 +- ...stbuffer.test.ts => promisebuffer.test.ts} | 33 +++++---- packages/node/src/backend.ts | 48 ++----------- packages/node/src/transports/base.ts | 16 ++--- .../node/test/helper/settimeouttransport.ts | 20 +++--- .../manual/express-scope-separation/start.js | 2 +- packages/node/test/transports/http.test.ts | 40 +++++++---- packages/node/test/transports/https.test.ts | 48 ++++++++----- 16 files changed, 168 insertions(+), 153 deletions(-) rename packages/core/src/{requestbuffer.ts => promisebuffer.ts} (80%) rename packages/core/test/lib/{requestbuffer.test.ts => promisebuffer.test.ts} (68%) diff --git a/packages/browser/src/backend.ts b/packages/browser/src/backend.ts index 4e173d3c6da8..94241273ecfc 100644 --- a/packages/browser/src/backend.ts +++ b/packages/browser/src/backend.ts @@ -1,4 +1,4 @@ -import { BaseBackend, NoopTransport, Options, SentryError } from '@sentry/core'; +import { BaseBackend, Options, SentryError } from '@sentry/core'; import { SentryEvent, SentryEventHint, Severity, Transport } from '@sentry/types'; import { isDOMError, isDOMException, isError, isErrorEvent, isPlainObject } from '@sentry/utils/is'; import { supportsBeacon, supportsFetch } from '@sentry/utils/supports'; @@ -50,7 +50,8 @@ export class BrowserBackend extends BaseBackend { */ protected setupTransport(): Transport { if (!this.options.dsn) { - return new NoopTransport(); + // We return the noop transport here in case there is no Dsn. + return super.setupTransport(); } const transportOptions = this.options.transportOptions ? this.options.transportOptions : { dsn: this.options.dsn }; diff --git a/packages/browser/src/transports/base.ts b/packages/browser/src/transports/base.ts index 79e2f5e781f4..d1d9a93d1458 100644 --- a/packages/browser/src/transports/base.ts +++ b/packages/browser/src/transports/base.ts @@ -1,4 +1,4 @@ -import { API, RequestBuffer, SentryError } from '@sentry/core'; +import { API, PromiseBuffer, SentryError } from '@sentry/core'; import { SentryResponse, Transport, TransportOptions } from '@sentry/types'; /** Base Transport class implementation */ @@ -9,7 +9,7 @@ export abstract class BaseTransport implements Transport { public url: string; /** A simple buffer holding all requests. */ - protected readonly buffer: RequestBuffer = new RequestBuffer(30); + protected readonly buffer: PromiseBuffer = new PromiseBuffer(30); public constructor(public options: TransportOptions) { this.url = new API(this.options.dsn).getStoreEndpointWithUrlEncodedAuth(); @@ -26,7 +26,6 @@ export abstract class BaseTransport implements Transport { * @inheritDoc */ public close(timeout?: number): Promise { - console.log('calling close'); return this.buffer.drain(timeout); } } diff --git a/packages/browser/test/index.test.ts b/packages/browser/test/index.test.ts index a50ac003cd19..096083173227 100644 --- a/packages/browser/test/index.test.ts +++ b/packages/browser/test/index.test.ts @@ -69,13 +69,12 @@ describe('SentryBrowser', () => { }); describe('breadcrumbs', () => { - it.only('should record breadcrumbs', async () => { + it('should record breadcrumbs', async () => { addBreadcrumb({ message: 'test1' }); addBreadcrumb({ message: 'test2' }); captureMessage('event'); await (getCurrentHub().getClient() as BrowserClient).close(2000); - console.log(beforeSend.calledOnce); expect(beforeSend.args[0][0].breadcrumbs).to.have.lengthOf(2); }); }); diff --git a/packages/browser/test/integration/init.js b/packages/browser/test/integration/init.js index 9408b4530691..c5ee26de9532 100644 --- a/packages/browser/test/integration/init.js +++ b/packages/browser/test/integration/init.js @@ -22,7 +22,7 @@ window.sentryBreadcrumbs = []; // stub transport so we don't actually transmit any data function DummyTransport() {} -DummyTransport.prototype.captureEvent = function(event) { +DummyTransport.prototype.sendEvent = function(event) { // console.log(JSON.stringify(event, null, 2)); sentryData.push(event); done(sentryData); diff --git a/packages/core/src/basebackend.ts b/packages/core/src/basebackend.ts index a0773bf7857f..d0af2caa2426 100644 --- a/packages/core/src/basebackend.ts +++ b/packages/core/src/basebackend.ts @@ -4,6 +4,7 @@ import { logger } from '@sentry/utils/logger'; import { serialize } from '@sentry/utils/object'; import { SentryError } from './error'; import { Backend, Options } from './interfaces'; +import { NoopTransport } from './transports/noop'; /** A class object that can instanciate Backend objects. */ export interface BackendClass { @@ -33,7 +34,7 @@ export abstract class BaseBackend implements Backend { * Sets up the transport so it can be used later to send requests. */ protected setupTransport(): Transport { - throw new SentryError('Backend has to implement `setupTransport` method'); + return new NoopTransport(); } /** @@ -54,9 +55,7 @@ export abstract class BaseBackend implements Backend { * @inheritDoc */ public async sendEvent(event: SentryEvent): Promise { - const response = await this.transport.sendEvent(serialize(event)); - logger.log(`Request finished with: ${response}`); - return response; + return this.transport.sendEvent(serialize(event)); } /** diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index db6411b2eb55..08ec438c0987 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -18,6 +18,7 @@ import { BackendClass } from './basebackend'; import { Dsn } from './dsn'; import { IntegrationIndex, setupIntegrations } from './integration'; import { Backend, Client, Options } from './interfaces'; +import { PromiseBuffer } from './promisebuffer'; /** * Default maximum number of breadcrumbs added to an event. Can be overwritten @@ -94,6 +95,9 @@ export abstract class BaseClient implement /** Array of used integrations. */ private readonly integrations: IntegrationIndex; + /** A simple buffer holding all requests. */ + protected readonly buffer: PromiseBuffer = new PromiseBuffer(); + /** * Initializes this client instance. * @@ -132,8 +136,12 @@ export abstract class BaseClient implement * @inheritDoc */ public async captureException(exception: any, hint?: SentryEventHint, scope?: Scope): Promise { - const event = await this.getBackend().eventFromException(exception, hint); - return this.captureEvent(event, hint, scope); + return this.buffer.add( + (async () => { + const event = await this.getBackend().eventFromException(exception, hint); + return this.captureEvent(event, hint, scope); + })(), + ); } /** @@ -145,15 +153,25 @@ export abstract class BaseClient implement hint?: SentryEventHint, scope?: Scope, ): Promise { - const event = await this.getBackend().eventFromMessage(message, level, hint); - return this.captureEvent(event, hint, scope); + return this.buffer.add( + (async () => { + const event = await this.getBackend().eventFromMessage(message, level, hint); + return this.captureEvent(event, hint, scope); + })(), + ); } /** * @inheritDoc */ public async captureEvent(event: SentryEvent, hint?: SentryEventHint, scope?: Scope): Promise { - return this.processEvent(event, async finalEvent => this.getBackend().sendEvent(finalEvent), hint, scope); + // Adding this here is technically not correct since if you call captureMessage/captureException it's already + // buffered. But since we not really need the count and we only need to know if the buffer is full or not, + // This is fine... + return this.buffer.add( + (async () => + this.processEvent(event, async finalEvent => this.getBackend().sendEvent(finalEvent), hint, scope))(), + ); } /** @@ -313,7 +331,6 @@ export abstract class BaseClient implement try { const isInternalException = hint && hint.data && (hint.data as { [key: string]: any }).__sentry__ === true; if (!isInternalException && beforeSend) { - console.log('would call before send'); finalEvent = await beforeSend(prepared, hint); if ((typeof finalEvent as any) === 'undefined') { logger.error('`beforeSend` method has to return `null` or a valid event'); @@ -342,34 +359,36 @@ export abstract class BaseClient implement }; } - const response = await send(finalEvent); - response.event = finalEvent; + try { + const response = await send(finalEvent); + response.event = finalEvent; - if (response.status === Status.RateLimit) { - // TODO: Handle rate limits and maintain a queue. For now, we require SDK - // implementors to override this method and handle it themselves. + if (response.status === Status.RateLimit) { + // TODO: Handle rate limits and maintain a queue. For now, we require SDK + // implementors to override this method and handle it themselves. + } + return response; + } catch (error) { + // We have a catch here since the transport can reject the request internally. + // If we do not catch this here, we will run into an endless loop. + logger.error(`${error}`); + return { + reason: `${error}`, + status: Status.Failed, + }; } - - return response; } /** * @inheritDoc */ public async close(timeout?: number): Promise { - return this.getBackend() - .getTransport() - .close(timeout); - - return new Promise(resolve => { - setTimeout(() => { - resolve( - this.getBackend() - .getTransport() - .close(timeout), - ); - }, 100); - }); + return (await Promise.all([ + this.getBackend() + .getTransport() + .close(timeout), + this.buffer.drain(timeout), + ])).reduce((prev, current) => prev && current); } /** diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index b6237bad8d18..aa425970895f 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -12,7 +12,7 @@ export { BaseClient } from './baseclient'; export { BackendClass, BaseBackend } from './basebackend'; export { Dsn } from './dsn'; export { SentryError } from './error'; -export { RequestBuffer } from './requestbuffer'; +export { PromiseBuffer } from './promisebuffer'; export { Backend, Client, LogLevel, Options } from './interfaces'; export { initAndBind, ClientClass } from './sdk'; export { NoopTransport } from './transports/noop'; diff --git a/packages/core/src/requestbuffer.ts b/packages/core/src/promisebuffer.ts similarity index 80% rename from packages/core/src/requestbuffer.ts rename to packages/core/src/promisebuffer.ts index 5dec902dde03..6952399880ad 100644 --- a/packages/core/src/requestbuffer.ts +++ b/packages/core/src/promisebuffer.ts @@ -1,10 +1,19 @@ +import { SentryError } from './error'; + /** A simple queue that holds promises. */ -export class RequestBuffer { - public constructor(protected limit: number = 30) {} +export class PromiseBuffer { + public constructor(protected limit?: number) {} /** Internal set of queued Promises */ private readonly buffer: Array> = []; + /** + * Says if the buffer is ready to take more requests + */ + public isReady(): boolean { + return this.limit === undefined || this.length() < this.limit; + } + /** * Add a promise to the queue. * @@ -12,8 +21,8 @@ export class RequestBuffer { * @returns The original promise. */ public async add(task: Promise): Promise { - if (this.length() >= this.limit) { - return Promise.reject('Discarded'); + if (!this.isReady()) { + return Promise.reject(new SentryError('Not adding promises due to internal limit.')); } if (this.buffer.indexOf(task) === -1) { this.buffer.push(task); diff --git a/packages/core/src/transports/noop.ts b/packages/core/src/transports/noop.ts index 2e66d25e66a0..65ff574a65f8 100644 --- a/packages/core/src/transports/noop.ts +++ b/packages/core/src/transports/noop.ts @@ -15,7 +15,7 @@ export class NoopTransport implements Transport { /** * @inheritDoc */ - public close(_?: number): Promise { + public async close(_?: number): Promise { return Promise.resolve(true); } } diff --git a/packages/core/test/lib/requestbuffer.test.ts b/packages/core/test/lib/promisebuffer.test.ts similarity index 68% rename from packages/core/test/lib/requestbuffer.test.ts rename to packages/core/test/lib/promisebuffer.test.ts index a41aa91dedcb..daf620f3b398 100644 --- a/packages/core/test/lib/requestbuffer.test.ts +++ b/packages/core/test/lib/promisebuffer.test.ts @@ -1,22 +1,31 @@ -import { RequestBuffer } from '../../src/requestbuffer'; +import { PromiseBuffer } from '../../src/promisebuffer'; // tslint:disable:no-floating-promises -describe('RequestBuffer', () => { +describe('PromiseBuffer', () => { beforeEach(() => { jest.useFakeTimers(); }); - test('add()', () => { - const q = new RequestBuffer(); - const p = new Promise(resolve => setTimeout(resolve, 1)); - q.add(p); - expect(q.length()).toBe(1); + describe('add()', () => { + test('no limit', () => { + const q = new PromiseBuffer(); + const p = new Promise(resolve => setTimeout(resolve, 1)); + q.add(p); + expect(q.length()).toBe(1); + }); + test('with limit', () => { + const q = new PromiseBuffer(1); + const p = new Promise(resolve => setTimeout(resolve, 1)); + expect(q.add(p)).toEqual(p); + expect(q.add(new Promise(resolve => setTimeout(resolve, 1)))).rejects.toThrowError(); + expect(q.length()).toBe(1); + }); }); test('resolved promises should not show up in buffer length', async () => { expect.assertions(2); - const q = new RequestBuffer(); + const q = new PromiseBuffer(); const p = new Promise(resolve => setTimeout(resolve, 1)); q.add(p).then(() => { expect(q.length()).toBe(0); @@ -27,7 +36,7 @@ describe('RequestBuffer', () => { test('receive promise result outside and from buffer', async () => { expect.assertions(4); - const q = new RequestBuffer(); + const q = new PromiseBuffer(); const p = new Promise(resolve => setTimeout(() => { resolve('test'); @@ -46,7 +55,7 @@ describe('RequestBuffer', () => { test('drain()', async () => { expect.assertions(3); - const q = new RequestBuffer(); + const q = new PromiseBuffer(); for (let i = 0; i < 5; i++) { const p = new Promise(resolve => setTimeout(resolve, 1)); q.add(p); @@ -61,7 +70,7 @@ describe('RequestBuffer', () => { test('drain() with timeout', async () => { expect.assertions(2); - const q = new RequestBuffer(); + const q = new PromiseBuffer(); for (let i = 0; i < 5; i++) { const p = new Promise(resolve => setTimeout(resolve, 100)); q.add(p); @@ -75,7 +84,7 @@ describe('RequestBuffer', () => { test('drain() on empty buffer', async () => { expect.assertions(3); - const q = new RequestBuffer(); + const q = new PromiseBuffer(); expect(q.length()).toBe(0); q.drain().then(result => { expect(result).toBeTruthy(); diff --git a/packages/node/src/backend.ts b/packages/node/src/backend.ts index 972c9a62472a..335d8db481de 100644 --- a/packages/node/src/backend.ts +++ b/packages/node/src/backend.ts @@ -1,8 +1,7 @@ -import { BaseBackend, Dsn, getCurrentHub, NoopTransport, Options } from '@sentry/core'; -import { SentryEvent, SentryEventHint, SentryResponse, Severity, Status, Transport } from '@sentry/types'; +import { BaseBackend, Dsn, getCurrentHub, Options } from '@sentry/core'; +import { SentryEvent, SentryEventHint, Severity, Transport } from '@sentry/types'; import { isError, isPlainObject } from '@sentry/utils/is'; -import { logger } from '@sentry/utils/logger'; -import { limitObjectDepthToSize, serialize, serializeKeysToEventMessage } from '@sentry/utils/object'; +import { limitObjectDepthToSize, serializeKeysToEventMessage } from '@sentry/utils/object'; import { createHash } from 'crypto'; import { extractStackFromError, parseError, parseStack, prepareFramesForEvent } from './parsers'; import { HTTPSTransport, HTTPTransport } from './transports'; @@ -42,13 +41,12 @@ export class NodeBackend extends BaseBackend { * @inheritdoc */ protected setupTransport(): Transport { - let dsn: Dsn | undefined; - if (!this.options.dsn) { - return new NoopTransport(); + // We return the noop transport here in case there is no Dsn. + return super.setupTransport(); } - dsn = new Dsn(this.options.dsn); + const dsn = new Dsn(this.options.dsn); const transportOptions = this.options.transportOptions ? this.options.transportOptions : { dsn }; const clientOptions = ['httpProxy', 'httpsProxy', 'caCerts']; @@ -131,38 +129,4 @@ export class NodeBackend extends BaseBackend { return event; } - - /** - * @inheritDoc - */ - public async sendEvent(event: SentryEvent): Promise { - let dsn: Dsn; - - if (!this.options.dsn) { - logger.warn(`Event has been skipped because no Dsn is configured.`); - // We do nothing in case there is no DSN - return { status: Status.Skipped, reason: `Event has been skipped because no Dsn is configured.` }; - } else { - dsn = new Dsn(this.options.dsn); - } - - if (!this.transport) { - const transportOptions = this.options.transportOptions ? this.options.transportOptions : { dsn }; - const clientOptions = ['httpProxy', 'httpsProxy', 'caCerts']; - - for (const option of clientOptions) { - if (this.options[option]) { - transportOptions[option] = transportOptions[option] || this.options[option]; - } - } - - this.transport = this.options.transport - ? new this.options.transport({ dsn }) - : dsn.protocol === 'http' - ? new HTTPTransport(transportOptions) - : new HTTPSTransport(transportOptions); - } - - return this.transport.sendEvent(serialize(event)); - } } diff --git a/packages/node/src/transports/base.ts b/packages/node/src/transports/base.ts index b8ed5b909829..b4ac1d9c9e39 100644 --- a/packages/node/src/transports/base.ts +++ b/packages/node/src/transports/base.ts @@ -1,4 +1,4 @@ -import { API, RequestBuffer, SentryError } from '@sentry/core'; +import { API, PromiseBuffer, SentryError } from '@sentry/core'; import { SentryResponse, Status, Transport, TransportOptions } from '@sentry/types'; import * as fs from 'fs'; import * as http from 'http'; @@ -17,7 +17,7 @@ export interface HTTPRequest { /** Base Transport class implementation */ export abstract class BaseTransport implements Transport { /** API object */ - protected api?: API; + protected api: API; /** The Agent used for corresponding transport */ public module?: HTTPRequest; @@ -26,7 +26,7 @@ export abstract class BaseTransport implements Transport { public client?: http.Agent | https.Agent; /** A simple buffer holding all requests. */ - protected readonly buffer: RequestBuffer = new RequestBuffer(30); + protected readonly buffer: PromiseBuffer = new PromiseBuffer(30); /** Create instance and set this.dsn */ public constructor(public options: TransportOptions) { @@ -35,10 +35,6 @@ export abstract class BaseTransport implements Transport { /** Returns a build request option object used by request */ protected getRequestOptions(): http.RequestOptions | https.RequestOptions { - if (!this.api) { - return {}; - } - const headers = { ...this.api.getRequestHeaders(SDK_NAME, SDK_VERSION), ...this.options.headers, @@ -68,10 +64,6 @@ export abstract class BaseTransport implements Transport { protected async sendWithModule(httpModule: HTTPRequest, body: string): Promise { return this.buffer.add( new Promise((resolve, reject) => { - if (!this.api) { - resolve({ status: Status.Skipped, reason: `Event has been skipped because no Dsn is configured.` }); - return; - } const req = httpModule.request(this.getRequestOptions(), (res: http.IncomingMessage) => { res.setEncoding('utf8'); if (res.statusCode && res.statusCode >= 200 && res.statusCode < 300) { @@ -104,7 +96,7 @@ export abstract class BaseTransport implements Transport { * @inheritDoc */ public async sendEvent(_: string): Promise { - throw new SentryError('Transport Class has to implement `sendEvent` method'); + throw new SentryError('Transport Class has to implement `sendEvent` method.'); } /** diff --git a/packages/node/test/helper/settimeouttransport.ts b/packages/node/test/helper/settimeouttransport.ts index c9f261e6dc52..313504b2b083 100644 --- a/packages/node/test/helper/settimeouttransport.ts +++ b/packages/node/test/helper/settimeouttransport.ts @@ -1,14 +1,16 @@ -import { SentryEvent, SentryResponse, Status } from '../../src'; +import { SentryResponse, Status } from '../../src'; import { BaseTransport } from '../../src/transports'; export class SetTimeoutTransport extends BaseTransport { - public async captureEvent(_: SentryEvent): Promise { - return new Promise(resolve => { - setTimeout(() => { - resolve({ - status: Status.fromHttpCode(200), - }); - }, 1); - }); + public async sendEvent(_: string): Promise { + return this.buffer.add( + new Promise(resolve => { + setTimeout(() => { + resolve({ + status: Status.fromHttpCode(200), + }); + }, 1); + }), + ); } } diff --git a/packages/node/test/manual/express-scope-separation/start.js b/packages/node/test/manual/express-scope-separation/start.js index 2d0446b4d0c6..0df06260958c 100644 --- a/packages/node/test/manual/express-scope-separation/start.js +++ b/packages/node/test/manual/express-scope-separation/start.js @@ -13,7 +13,7 @@ function assertTags(actual, expected) { let remaining = 3; class DummyTransport { - captureEvent(event) { + sendEvent(event) { --remaining; if (!remaining) { diff --git a/packages/node/test/transports/http.test.ts b/packages/node/test/transports/http.test.ts index 812bff202354..a38952233968 100644 --- a/packages/node/test/transports/http.test.ts +++ b/packages/node/test/transports/http.test.ts @@ -44,9 +44,11 @@ describe('HTTPTransport', () => { test('send 200', async () => { const transport = createTransport({ dsn }); - await transport.captureEvent({ - message: 'test', - }); + await transport.sendEvent( + JSON.stringify({ + message: 'test', + }), + ); const requestOptions = (transport.module!.request as jest.Mock).mock.calls[0][0]; assertBasicOptions(requestOptions); @@ -58,9 +60,11 @@ describe('HTTPTransport', () => { const transport = createTransport({ dsn }); try { - await transport.captureEvent({ - message: 'test', - }); + await transport.sendEvent( + JSON.stringify({ + message: 'test', + }), + ); } catch (e) { const requestOptions = (transport.module!.request as jest.Mock).mock.calls[0][0]; assertBasicOptions(requestOptions); @@ -76,9 +80,11 @@ describe('HTTPTransport', () => { const transport = createTransport({ dsn }); try { - await transport.captureEvent({ - message: 'test', - }); + await transport.sendEvent( + JSON.stringify({ + message: 'test', + }), + ); } catch (e) { const requestOptions = (transport.module!.request as jest.Mock).mock.calls[0][0]; assertBasicOptions(requestOptions); @@ -94,9 +100,11 @@ describe('HTTPTransport', () => { a: 'b', }, }); - await transport.captureEvent({ - message: 'test', - }); + await transport.sendEvent( + JSON.stringify({ + message: 'test', + }), + ); const requestOptions = (transport.module!.request as jest.Mock).mock.calls[0][0]; assertBasicOptions(requestOptions); @@ -109,9 +117,11 @@ describe('HTTPTransport', () => { dsn, httpProxy: 'http://example.com:8080', }); - await transport.captureEvent({ - message: 'test', - }); + await transport.sendEvent( + JSON.stringify({ + message: 'test', + }), + ); const requestOptions = (transport.module!.request as jest.Mock).mock.calls[0][0]; assertBasicOptions(requestOptions); diff --git a/packages/node/test/transports/https.test.ts b/packages/node/test/transports/https.test.ts index a7cac397961b..23ec9063ac55 100644 --- a/packages/node/test/transports/https.test.ts +++ b/packages/node/test/transports/https.test.ts @@ -50,9 +50,11 @@ describe('HTTPSTransport', () => { test('send 200', async () => { const transport = createTransport({ dsn }); - await transport.captureEvent({ - message: 'test', - }); + await transport.sendEvent( + JSON.stringify({ + message: 'test', + }), + ); const requestOptions = (transport.module!.request as jest.Mock).mock.calls[0][0]; assertBasicOptions(requestOptions); @@ -64,9 +66,11 @@ describe('HTTPSTransport', () => { const transport = createTransport({ dsn }); try { - await transport.captureEvent({ - message: 'test', - }); + await transport.sendEvent( + JSON.stringify({ + message: 'test', + }), + ); } catch (e) { const requestOptions = (transport.module!.request as jest.Mock).mock.calls[0][0]; assertBasicOptions(requestOptions); @@ -82,9 +86,11 @@ describe('HTTPSTransport', () => { const transport = createTransport({ dsn }); try { - await transport.captureEvent({ - message: 'test', - }); + await transport.sendEvent( + JSON.stringify({ + message: 'test', + }), + ); } catch (e) { const requestOptions = (transport.module!.request as jest.Mock).mock.calls[0][0]; assertBasicOptions(requestOptions); @@ -100,9 +106,11 @@ describe('HTTPSTransport', () => { a: 'b', }, }); - await transport.captureEvent({ - message: 'test', - }); + await transport.sendEvent( + JSON.stringify({ + message: 'test', + }), + ); const requestOptions = (transport.module!.request as jest.Mock).mock.calls[0][0]; assertBasicOptions(requestOptions); @@ -115,9 +123,11 @@ describe('HTTPSTransport', () => { dsn, httpsProxy: 'https://example.com:8080', }); - await transport.captureEvent({ - message: 'test', - }); + await transport.sendEvent( + JSON.stringify({ + message: 'test', + }), + ); const requestOptions = (transport.module!.request as jest.Mock).mock.calls[0][0]; assertBasicOptions(requestOptions); @@ -134,9 +144,11 @@ describe('HTTPSTransport', () => { caCerts: './some/path.pem', dsn, }); - await transport.captureEvent({ - message: 'test', - }); + await transport.sendEvent( + JSON.stringify({ + message: 'test', + }), + ); const requestOptions = (transport.module!.request as jest.Mock).mock.calls[0][0]; assertBasicOptions(requestOptions); expect(requestOptions.ca).toEqual('mockedCert'); From 8ba05642756d1bb9bf999ae502b93f0a9e969494 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Fri, 21 Dec 2018 13:35:24 +0100 Subject: [PATCH 07/16] fix: Linter --- packages/browser/src/transports/base.ts | 2 +- packages/node/src/transports/base.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/browser/src/transports/base.ts b/packages/browser/src/transports/base.ts index d1d9a93d1458..14c6a42de6cc 100644 --- a/packages/browser/src/transports/base.ts +++ b/packages/browser/src/transports/base.ts @@ -25,7 +25,7 @@ export abstract class BaseTransport implements Transport { /** * @inheritDoc */ - public close(timeout?: number): Promise { + public async close(timeout?: number): Promise { return this.buffer.drain(timeout); } } diff --git a/packages/node/src/transports/base.ts b/packages/node/src/transports/base.ts index b4ac1d9c9e39..c1b828757922 100644 --- a/packages/node/src/transports/base.ts +++ b/packages/node/src/transports/base.ts @@ -102,7 +102,7 @@ export abstract class BaseTransport implements Transport { /** * @inheritDoc */ - public close(timeout?: number): Promise { + public async close(timeout?: number): Promise { return this.buffer.drain(timeout); } } From 27e31bcde79870fcece2e3924f4a8709cd54dd3b Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Fri, 21 Dec 2018 13:37:08 +0100 Subject: [PATCH 08/16] fix: Wording --- packages/core/src/promisebuffer.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/promisebuffer.ts b/packages/core/src/promisebuffer.ts index 6952399880ad..38a7088b4669 100644 --- a/packages/core/src/promisebuffer.ts +++ b/packages/core/src/promisebuffer.ts @@ -22,7 +22,7 @@ export class PromiseBuffer { */ public async add(task: Promise): Promise { if (!this.isReady()) { - return Promise.reject(new SentryError('Not adding promises due to internal limit.')); + return Promise.reject(new SentryError('Not adding Promise due to buffer limit reached.')); } if (this.buffer.indexOf(task) === -1) { this.buffer.push(task); From a0e3339bb6267f4641cf818eaf29f397e6f4072d Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 2 Jan 2019 15:55:38 +0100 Subject: [PATCH 09/16] fix: Don't break the API --- packages/core/src/basebackend.ts | 5 +++++ packages/types/src/index.ts | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/packages/core/src/basebackend.ts b/packages/core/src/basebackend.ts index d0af2caa2426..c2e2a20eb279 100644 --- a/packages/core/src/basebackend.ts +++ b/packages/core/src/basebackend.ts @@ -55,6 +55,11 @@ export abstract class BaseBackend implements Backend { * @inheritDoc */ public async sendEvent(event: SentryEvent): Promise { + // TODO: Remove with v5 + if (this.transport.captureEvent) { + return this.transport.captureEvent(event); + } + // -------------------- return this.transport.sendEvent(serialize(event)); } diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 1e94ee3d4a23..8f7d84095dd9 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -246,12 +246,17 @@ export interface Transport { * @param body String body that should be sent to Sentry. */ sendEvent(body: string): Promise; + /** * Call this function to wait until all pending requests have been sent. * * @param timeout Number time in ms to wait until the buffer is drained. */ close(timeout?: number): Promise; + + // TODO: Remove with v5 + /** @deprecated Implement sendEvent instead */ + captureEvent?(event: SentryEvent): Promise; } /** JSDoc */ From 2b5d7563aa9c4a1b259402cc390c6d294c6539a2 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 8 Jan 2019 11:52:21 +0100 Subject: [PATCH 10/16] feat: Add simple lru cache for readFiles --- packages/node/package.json | 8 ++++-- packages/node/src/parsers.ts | 45 ++++++++++++++++++++---------- packages/node/test/helper/error.ts | 3 ++ packages/node/test/parsers.test.ts | 45 ++++++++++++++++++++++++++++++ yarn.lock | 12 ++++++-- 5 files changed, 94 insertions(+), 19 deletions(-) create mode 100644 packages/node/test/helper/error.ts create mode 100644 packages/node/test/parsers.test.ts diff --git a/packages/node/package.json b/packages/node/package.json index 7a9990d46d8c..2a2b097eaee7 100644 --- a/packages/node/package.json +++ b/packages/node/package.json @@ -20,15 +20,17 @@ "@sentry/types": "4.4.2", "@sentry/utils": "4.4.2", "@types/stack-trace": "0.0.29", - "async-limiter": "^1.0.0", + "async-limiter": "1.0.0", "cookie": "0.3.1", - "https-proxy-agent": "^2.2.1", + "https-proxy-agent": "2.2.1", + "lru_map": "0.3.3", "lsmod": "1.0.0", "stack-trace": "0.0.10", "tslib": "^1.9.3" }, "devDependencies": { "@types/cookie": "0.3.1", + "@types/lru-cache": "^4.1.1", "@types/node": "^10.12.10", "express": "^4.16.4", "jest": "^22.4.3", @@ -52,7 +54,7 @@ "fix:tslint": "tslint --fix -t stylish -p .", "test": "run-s test:jest test:express", "test:jest": "jest", - "test:watch": "jest --watch --notify", + "test:watch": "jest --watch", "test:express": "node test/manual/express-scope-separation/start.js", "version": "node ../../scripts/versionbump.js src/version.ts" }, diff --git a/packages/node/src/parsers.ts b/packages/node/src/parsers.ts index 8fab0222668f..52d257bde9c3 100644 --- a/packages/node/src/parsers.ts +++ b/packages/node/src/parsers.ts @@ -3,12 +3,18 @@ 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 { LRUMap } from 'lru_map'; import * as stacktrace from 'stack-trace'; import { NodeOptions } from './backend'; // tslint:disable-next-line:no-unsafe-any const fsLimiter = new Limiter({ concurrency: 25 }); const DEFAULT_LINES_OF_CONTEXT: number = 7; +const FILE_CONTENT_CACHE = new LRUMap(100); + +export function resetFileContentCache(): void { + FILE_CONTENT_CACHE.clear(); +} /** * Just an Error object with arbitrary attributes attached to it. @@ -75,20 +81,31 @@ async function readSourceFiles( [key: string]: string; } = {}; - await Promise.all( - filenames.map(async filename => { - let content; - try { - content = await readFileAsync(filename, fsLimiter); - } catch (_) { - // unsure what to add here as the file is unreadable - content = null; - } - if (typeof content === 'string') { - sourceFiles[filename] = content; - } - }), - ); + // tslint:disable-next-line:prefer-for-of + for (let i = 0; i < filenames.length; i++) { + const filename = filenames[i]; + // We have a cache hit + const cache = FILE_CONTENT_CACHE.get(filename); + if (cache) { + sourceFiles[filename] = cache; + continue; + } + + let content; + try { + content = await readFileAsync(filename, fsLimiter); + } catch (_) { + // unsure what to add here as the file is unreadable + content = null; + } + + if (typeof content === 'string') { + sourceFiles[filename] = content; + FILE_CONTENT_CACHE.set(filename, content); + } + } + + // ); return sourceFiles; } diff --git a/packages/node/test/helper/error.ts b/packages/node/test/helper/error.ts new file mode 100644 index 000000000000..0228338f036b --- /dev/null +++ b/packages/node/test/helper/error.ts @@ -0,0 +1,3 @@ +export function getError(): Error { + return new Error('mock error'); +} diff --git a/packages/node/test/parsers.test.ts b/packages/node/test/parsers.test.ts new file mode 100644 index 000000000000..1337413cc53f --- /dev/null +++ b/packages/node/test/parsers.test.ts @@ -0,0 +1,45 @@ +// import { StackFrame } from '@sentry/types'; +import * as fs from 'fs'; +import * as stacktrace from 'stack-trace'; +import { Parsers } from '../src'; +import { getError } from './helper/error'; + +describe('parsers.ts', () => { + let frames: stacktrace.StackFrame[]; + let spy: jest.SpyInstance; + + beforeEach(() => { + spy = jest.spyOn(fs, 'readFile'); + frames = stacktrace.parse(new Error('test')); + Parsers.resetFileContentCache(); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + describe('lru file cache', () => { + test('parseStack with same file', async () => { + await Parsers.parseStack(frames); + const mockCalls = spy.mock.calls.length; + await Parsers.parseStack(frames); + // Calls to readFile shouldn't increase if there isn't a new error + expect(spy).toHaveBeenCalledTimes(mockCalls); + }); + + test('parseStack with adding different file', async () => { + await Parsers.parseStack(frames); + const mockCalls = spy.mock.calls.length; + await Parsers.parseStack(stacktrace.parse(getError())); + const newErrorCalls = spy.mock.calls.length; + expect(newErrorCalls).toBeGreaterThan(mockCalls); + await Parsers.parseStack(stacktrace.parse(getError())); + expect(spy).toHaveBeenCalledTimes(newErrorCalls); + }); + }); + + test('parseStack with no context', async () => { + await Parsers.parseStack(frames, { frameContextLines: 0 }); + expect(spy).toHaveBeenCalledTimes(0); + }); +}); diff --git a/yarn.lock b/yarn.lock index debe10af990d..2c2a3eabd768 100644 --- a/yarn.lock +++ b/yarn.lock @@ -714,6 +714,10 @@ version "4.14.118" resolved "https://registry.yarnpkg.com/@types/lodash/-/lodash-4.14.118.tgz#247bab39bfcc6d910d4927c6e06cbc70ec376f27" +"@types/lru-cache@^4.1.1": + version "4.1.1" + resolved "https://registry.yarnpkg.com/@types/lru-cache/-/lru-cache-4.1.1.tgz#b2d87a5e3df8d4b18ca426c5105cd701c2306d40" + "@types/marked@^0.4.0": version "0.4.2" resolved "https://registry.yarnpkg.com/@types/marked/-/marked-0.4.2.tgz#64a89e53ea37f61cc0f3ee1732c555c2dbf6452f" @@ -1299,7 +1303,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, async-limiter@~1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/async-limiter/-/async-limiter-1.0.0.tgz#78faed8c3d074ab81f22b4e985d79e8738f720f8" @@ -4595,7 +4599,7 @@ https-browserify@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/https-browserify/-/https-browserify-1.0.0.tgz#ec06c10e0a34c0f2faf199f7fd7fc78fffd03c73" -https-proxy-agent@^2.2.0, https-proxy-agent@^2.2.1: +https-proxy-agent@2.2.1, https-proxy-agent@^2.2.0, https-proxy-agent@^2.2.1: version "2.2.1" resolved "https://registry.yarnpkg.com/https-proxy-agent/-/https-proxy-agent-2.2.1.tgz#51552970fa04d723e04c56d04178c3f92592bbc0" dependencies: @@ -6357,6 +6361,10 @@ lru-cache@^4.0.0, lru-cache@^4.0.1, lru-cache@^4.1.2, lru-cache@^4.1.3: pseudomap "^1.0.2" yallist "^3.0.2" +lru_map@0.3.3: + version "0.3.3" + resolved "https://registry.yarnpkg.com/lru_map/-/lru_map-0.3.3.tgz#b5c8351b9464cbd750335a79650a0ec0e56118dd" + lsmod@1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/lsmod/-/lsmod-1.0.0.tgz#9a00f76dca36eb23fa05350afe1b585d4299e64b" From 04fc2e84b07f1c47b0b40a92ebe3ab10d031d878 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 8 Jan 2019 15:26:05 +0100 Subject: [PATCH 11/16] meta: Code Review --- packages/node/package.json | 1 - packages/node/src/backend.ts | 4 ++-- packages/node/src/parsers.ts | 13 +++++-------- packages/utils/src/fs.ts | 17 +++++++---------- yarn.lock | 2 +- 5 files changed, 15 insertions(+), 22 deletions(-) diff --git a/packages/node/package.json b/packages/node/package.json index 2a2b097eaee7..3fdc83dd6f3d 100644 --- a/packages/node/package.json +++ b/packages/node/package.json @@ -20,7 +20,6 @@ "@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", "lru_map": "0.3.3", diff --git a/packages/node/src/backend.ts b/packages/node/src/backend.ts index 335d8db481de..5423686e1aa5 100644 --- a/packages/node/src/backend.ts +++ b/packages/node/src/backend.ts @@ -48,11 +48,11 @@ export class NodeBackend extends BaseBackend { const dsn = new Dsn(this.options.dsn); - const transportOptions = this.options.transportOptions ? this.options.transportOptions : { dsn }; + const transportOptions = this.options.transportOptions || { dsn }; const clientOptions = ['httpProxy', 'httpsProxy', 'caCerts']; for (const option of clientOptions) { - if (this.options[option]) { + if (this.options[option] || transportOptions[option]) { transportOptions[option] = transportOptions[option] || this.options[option]; } } diff --git a/packages/node/src/parsers.ts b/packages/node/src/parsers.ts index 52d257bde9c3..90838bc15840 100644 --- a/packages/node/src/parsers.ts +++ b/packages/node/src/parsers.ts @@ -2,13 +2,11 @@ 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 { LRUMap } from 'lru_map'; import * as stacktrace from 'stack-trace'; import { NodeOptions } from './backend'; // tslint:disable-next-line:no-unsafe-any -const fsLimiter = new Limiter({ concurrency: 25 }); const DEFAULT_LINES_OF_CONTEXT: number = 7; const FILE_CONTENT_CACHE = new LRUMap(100); @@ -93,16 +91,15 @@ async function readSourceFiles( let content; try { - content = await readFileAsync(filename, fsLimiter); + content = await readFileAsync(filename); + if (typeof content === 'string') { + sourceFiles[filename] = content; + FILE_CONTENT_CACHE.set(filename, content); + } } catch (_) { // unsure what to add here as the file is unreadable content = null; } - - if (typeof content === 'string') { - sourceFiles[filename] = content; - FILE_CONTENT_CACHE.set(filename, content); - } } // ); diff --git a/packages/utils/src/fs.ts b/packages/utils/src/fs.ts index 5a41dfc8cfae..9373225925af 100644 --- a/packages/utils/src/fs.ts +++ b/packages/utils/src/fs.ts @@ -10,20 +10,17 @@ const _0777 = parseInt('0777', 8); * @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, fsLimiter: any): Promise { +export async function readFileAsync(path: string): Promise { // 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((res, reject) => { // tslint:disable-next-line:no-unsafe-any - fsLimiter.push((done: () => void) => { - readFile(path, 'utf8', (err, data) => { - done(); - if (err) { - reject(err); - } else { - res(data); - } - }); + readFile(path, 'utf8', (err, data) => { + if (err) { + reject(err); + } else { + res(data); + } }); }); } diff --git a/yarn.lock b/yarn.lock index 2c2a3eabd768..f6104f9f44c1 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1303,7 +1303,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" From f3c3882d2c43f913dd25feb71955d8b952c909e2 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 8 Jan 2019 15:29:49 +0100 Subject: [PATCH 12/16] meta: Linter errros --- packages/core/src/basebackend.ts | 2 ++ packages/node/src/parsers.ts | 3 +++ 2 files changed, 5 insertions(+) diff --git a/packages/core/src/basebackend.ts b/packages/core/src/basebackend.ts index c2e2a20eb279..b42439622175 100644 --- a/packages/core/src/basebackend.ts +++ b/packages/core/src/basebackend.ts @@ -56,7 +56,9 @@ export abstract class BaseBackend implements Backend { */ public async sendEvent(event: SentryEvent): Promise { // TODO: Remove with v5 + // tslint:disable-next-line if (this.transport.captureEvent) { + // tslint:disable-next-line return this.transport.captureEvent(event); } // -------------------- diff --git a/packages/node/src/parsers.ts b/packages/node/src/parsers.ts index 90838bc15840..5aa92686fa2d 100644 --- a/packages/node/src/parsers.ts +++ b/packages/node/src/parsers.ts @@ -10,6 +10,9 @@ import { NodeOptions } from './backend'; const DEFAULT_LINES_OF_CONTEXT: number = 7; const FILE_CONTENT_CACHE = new LRUMap(100); +/** + * Resets the file cache. Exists for testing purposes. + */ export function resetFileContentCache(): void { FILE_CONTENT_CACHE.clear(); } From a353ef162e40ee1720373591be14de6f69d7f8ce Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Tue, 8 Jan 2019 15:35:51 +0100 Subject: [PATCH 13/16] fix: Changelog + docs --- CHANGELOG.md | 9 +++++++++ packages/utils/src/fs.ts | 2 -- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0dba3e747715..2b97ee66dfb2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,15 @@ ## Unreleased +- [core] feat: Deprecate `captureEvent`, prefer `sendEvent` for transports. `sendEvent` now takes a string (body) + instead of `Event` object. +- [core] feat: Use correct buffer for requests in transports +- [node] feat: Add file cache for providing pre/post context in frames +- [node] feat: New option `frameContextLines`, if set to `0` we do not provide source code pre/post context, default is + `7` lines pre/post +- [core]: ref: Change way how transports are initialized +- [core]: ref: Rename `RequestBuffer` to `PromiseBuffer`, also introduce limit + ## 4.4.2 - [node] Port memory-leak tests from raven-node diff --git a/packages/utils/src/fs.ts b/packages/utils/src/fs.ts index 9373225925af..1a8ee7776633 100644 --- a/packages/utils/src/fs.ts +++ b/packages/utils/src/fs.ts @@ -7,14 +7,12 @@ 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 { // 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((res, reject) => { - // tslint:disable-next-line:no-unsafe-any readFile(path, 'utf8', (err, data) => { if (err) { reject(err); From 0b2331124daa80c8e0808074f52981b2301474d8 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 9 Jan 2019 08:31:20 +0100 Subject: [PATCH 14/16] fix: Browser integration tests --- packages/browser/test/integration/init.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/browser/test/integration/init.js b/packages/browser/test/integration/init.js index c5ee26de9532..0a4843c93d03 100644 --- a/packages/browser/test/integration/init.js +++ b/packages/browser/test/integration/init.js @@ -24,7 +24,7 @@ window.sentryBreadcrumbs = []; function DummyTransport() {} DummyTransport.prototype.sendEvent = function(event) { // console.log(JSON.stringify(event, null, 2)); - sentryData.push(event); + sentryData.push(JSON.parse(event)); done(sentryData); return Promise.resolve({ status: 'success', From 68e5592ab6ef8bf78bd5c056c10c39cf6d61b79c Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 9 Jan 2019 08:42:31 +0100 Subject: [PATCH 15/16] fix: Also cache if the file wasn't readable --- packages/node/src/parsers.ts | 32 ++++++++++++++++++++++---------- packages/utils/src/fs.ts | 4 ++-- 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/packages/node/src/parsers.ts b/packages/node/src/parsers.ts index 5aa92686fa2d..49d2955ddf60 100644 --- a/packages/node/src/parsers.ts +++ b/packages/node/src/parsers.ts @@ -8,7 +8,7 @@ import { NodeOptions } from './backend'; // tslint:disable-next-line:no-unsafe-any const DEFAULT_LINES_OF_CONTEXT: number = 7; -const FILE_CONTENT_CACHE = new LRUMap(100); +const FILE_CONTENT_CACHE = new LRUMap(100); /** * Resets the file cache. Exists for testing purposes. @@ -67,7 +67,12 @@ function getModule(filename: string, base?: string): string { return file; } -/** JSDoc */ +/** + * 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. + */ async function readSourceFiles( filenames: string[], ): Promise<{ @@ -85,27 +90,34 @@ async function readSourceFiles( // tslint:disable-next-line:prefer-for-of for (let i = 0; i < filenames.length; i++) { const filename = filenames[i]; - // We have a cache hit + const cache = FILE_CONTENT_CACHE.get(filename); - if (cache) { - sourceFiles[filename] = cache; + // 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; + } + // 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. continue; } - let content; + let content = null; try { content = await readFileAsync(filename); if (typeof content === 'string') { sourceFiles[filename] = content; - FILE_CONTENT_CACHE.set(filename, content); } } catch (_) { // unsure what to add here as the file is unreadable - content = null; } - } - // ); + // 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); + } return sourceFiles; } diff --git a/packages/utils/src/fs.ts b/packages/utils/src/fs.ts index 1a8ee7776633..5380e9d399fb 100644 --- a/packages/utils/src/fs.ts +++ b/packages/utils/src/fs.ts @@ -9,10 +9,10 @@ const _0777 = parseInt('0777', 8); * @param path A relative or absolute path to the file * @returns A Promise that resolves when the file has been read. */ -export async function readFileAsync(path: string): Promise { +export async function readFileAsync(path: string): Promise { // 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((res, reject) => { + return new Promise((res, reject) => { readFile(path, 'utf8', (err, data) => { if (err) { reject(err); From 38053e40e86cb50c374223ac90f719ed546ef605 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Wed, 9 Jan 2019 09:24:58 +0100 Subject: [PATCH 16/16] fix: Linter --- packages/node/src/parsers.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/node/src/parsers.ts b/packages/node/src/parsers.ts index 49d2955ddf60..9dd135bef2d2 100644 --- a/packages/node/src/parsers.ts +++ b/packages/node/src/parsers.ts @@ -107,9 +107,7 @@ async function readSourceFiles( let content = null; try { content = await readFileAsync(filename); - if (typeof content === 'string') { - sourceFiles[filename] = content; - } + sourceFiles[filename] = content; } catch (_) { // unsure what to add here as the file is unreadable }