From 1d8c81f468de2519c5dcb6b6e9931e04be25c462 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 5 Jul 2023 19:02:09 +0200 Subject: [PATCH] ref: Extract error tree walking logic for `LinkedErrors` integrations (#8455) --- .../browser/src/integrations/linkederrors.ts | 65 +++----- .../unit/integrations/linkederrors.test.ts | 121 -------------- .../node/src/integrations/linkederrors.ts | 56 ++----- .../test/integrations/linkederrors.test.ts | 151 ------------------ packages/utils/src/aggregate-errors.ts | 50 ++++++ packages/utils/src/index.ts | 1 + packages/utils/test/aggregate-errors.test.ts | 97 +++++++++++ 7 files changed, 185 insertions(+), 356 deletions(-) delete mode 100644 packages/browser/test/unit/integrations/linkederrors.test.ts delete mode 100644 packages/node/test/integrations/linkederrors.test.ts create mode 100644 packages/utils/src/aggregate-errors.ts create mode 100644 packages/utils/test/aggregate-errors.test.ts diff --git a/packages/browser/src/integrations/linkederrors.ts b/packages/browser/src/integrations/linkederrors.ts index bb53d0210f96..08581fecb9b3 100644 --- a/packages/browser/src/integrations/linkederrors.ts +++ b/packages/browser/src/integrations/linkederrors.ts @@ -1,8 +1,6 @@ -import { addGlobalEventProcessor, getCurrentHub } from '@sentry/core'; -import type { Event, EventHint, Exception, ExtendedError, Integration, StackParser } from '@sentry/types'; -import { isInstanceOf } from '@sentry/utils'; +import type { Event, EventHint, EventProcessor, Hub, Integration } from '@sentry/types'; +import { applyAggregateErrorsToEvent } from '@sentry/utils'; -import type { BrowserClient } from '../client'; import { exceptionFromError } from '../eventbuilder'; const DEFAULT_KEY = 'cause'; @@ -46,49 +44,26 @@ export class LinkedErrors implements Integration { /** * @inheritDoc */ - public setupOnce(): void { - const client = getCurrentHub().getClient(); - if (!client) { - return; - } + public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void { addGlobalEventProcessor((event: Event, hint?: EventHint) => { - const self = getCurrentHub().getIntegration(LinkedErrors); - return self ? _handler(client.getOptions().stackParser, self._key, self._limit, event, hint) : event; - }); - } -} + const hub = getCurrentHub(); + const client = hub.getClient(); + const self = hub.getIntegration(LinkedErrors); -/** - * @inheritDoc - */ -export function _handler( - parser: StackParser, - key: string, - limit: number, - event: Event, - hint?: EventHint, -): Event | null { - if (!event.exception || !event.exception.values || !hint || !isInstanceOf(hint.originalException, Error)) { - return event; - } - const linkedErrors = _walkErrorTree(parser, limit, hint.originalException as ExtendedError, key); - event.exception.values = [...linkedErrors, ...event.exception.values]; - return event; -} + if (!client || !self) { + return event; + } + + applyAggregateErrorsToEvent( + exceptionFromError, + client.getOptions().stackParser, + self._key, + self._limit, + event, + hint, + ); -/** - * JSDOC - */ -export function _walkErrorTree( - parser: StackParser, - limit: number, - error: ExtendedError, - key: string, - stack: Exception[] = [], -): Exception[] { - if (!isInstanceOf(error[key], Error) || stack.length + 1 >= limit) { - return stack; + return event; + }); } - const exception = exceptionFromError(parser, error[key]); - return _walkErrorTree(parser, limit, error[key], key, [exception, ...stack]); } diff --git a/packages/browser/test/unit/integrations/linkederrors.test.ts b/packages/browser/test/unit/integrations/linkederrors.test.ts deleted file mode 100644 index 40738ca2af09..000000000000 --- a/packages/browser/test/unit/integrations/linkederrors.test.ts +++ /dev/null @@ -1,121 +0,0 @@ -import type { Event as SentryEvent, Exception, ExtendedError } from '@sentry/types'; - -import { BrowserClient } from '../../../src/client'; -import * as LinkedErrorsModule from '../../../src/integrations/linkederrors'; -import { defaultStackParser as parser } from '../../../src/stack-parsers'; -import { getDefaultBrowserClientOptions } from '../helper/browser-client-options'; - -type EventWithException = SentryEvent & { - exception: { - values: Exception[]; - }; -}; - -describe('LinkedErrors', () => { - describe('handler', () => { - it('should bail out if event does not contain exception', () => { - const event = { - message: 'foo', - }; - const result = LinkedErrorsModule._handler(parser, 'cause', 5, event); - expect(result).toEqual(event); - }); - - it('should bail out if event contains exception, but no hint', () => { - const event = { - exception: { - values: [], - }, - message: 'foo', - }; - const result = LinkedErrorsModule._handler(parser, 'cause', 5, event); - expect(result).toEqual(event); - }); - - it('should recursively walk error to find linked exceptions and assign them to the event', async () => { - const three: ExtendedError = new SyntaxError('three'); - - const two: ExtendedError = new TypeError('two'); - two.cause = three; - - const one: ExtendedError = new Error('one'); - one.cause = two; - - const originalException = one; - const options = getDefaultBrowserClientOptions({ stackParser: parser }); - const client = new BrowserClient(options); - return client.eventFromException(originalException).then(event => { - const result = LinkedErrorsModule._handler(parser, 'cause', 5, event, { - originalException, - }) as EventWithException; - - // It shouldn't include root exception, as it's already processed in the event by the main error handler - expect(result.exception.values.length).toBe(3); - expect(result.exception.values[0].type).toBe('SyntaxError'); - expect(result.exception.values[0].value).toBe('three'); - expect(result.exception.values[0].stacktrace).toHaveProperty('frames'); - expect(result.exception.values[1].type).toBe('TypeError'); - expect(result.exception.values[1].value).toBe('two'); - expect(result.exception.values[1].stacktrace).toHaveProperty('frames'); - expect(result.exception.values[2].type).toBe('Error'); - expect(result.exception.values[2].value).toBe('one'); - expect(result.exception.values[2].stacktrace).toHaveProperty('frames'); - }); - }); - - it('should allow to change walk key', async () => { - const three: ExtendedError = new SyntaxError('three'); - - const two: ExtendedError = new TypeError('two'); - two.reason = three; - - const one: ExtendedError = new Error('one'); - one.reason = two; - - const originalException = one; - const options = getDefaultBrowserClientOptions({ stackParser: parser }); - const client = new BrowserClient(options); - return client.eventFromException(originalException).then(event => { - const result = LinkedErrorsModule._handler(parser, 'reason', 5, event, { - originalException, - }) as EventWithException; - - expect(result.exception.values.length).toBe(3); - expect(result.exception.values[0].type).toBe('SyntaxError'); - expect(result.exception.values[0].value).toBe('three'); - expect(result.exception.values[0].stacktrace).toHaveProperty('frames'); - expect(result.exception.values[1].type).toBe('TypeError'); - expect(result.exception.values[1].value).toBe('two'); - expect(result.exception.values[1].stacktrace).toHaveProperty('frames'); - expect(result.exception.values[2].type).toBe('Error'); - expect(result.exception.values[2].value).toBe('one'); - expect(result.exception.values[2].stacktrace).toHaveProperty('frames'); - }); - }); - - it('should allow to change stack size limit', async () => { - const one: ExtendedError = new Error('one'); - const two: ExtendedError = new TypeError('two'); - const three: ExtendedError = new SyntaxError('three'); - one.cause = two; - two.cause = three; - - const options = getDefaultBrowserClientOptions({ stackParser: parser }); - const client = new BrowserClient(options); - const originalException = one; - return client.eventFromException(originalException).then(event => { - const result = LinkedErrorsModule._handler(parser, 'cause', 2, event, { - originalException, - }) as EventWithException; - - expect(result.exception.values.length).toBe(2); - expect(result.exception.values[0].type).toBe('TypeError'); - expect(result.exception.values[0].value).toBe('two'); - expect(result.exception.values[0].stacktrace).toHaveProperty('frames'); - expect(result.exception.values[1].type).toBe('Error'); - expect(result.exception.values[1].value).toBe('one'); - expect(result.exception.values[1].stacktrace).toHaveProperty('frames'); - }); - }); - }); -}); diff --git a/packages/node/src/integrations/linkederrors.ts b/packages/node/src/integrations/linkederrors.ts index 1283d2ebc1e1..d6a130ce5d54 100644 --- a/packages/node/src/integrations/linkederrors.ts +++ b/packages/node/src/integrations/linkederrors.ts @@ -1,6 +1,5 @@ -import { addGlobalEventProcessor, getCurrentHub } from '@sentry/core'; -import type { Event, EventHint, Exception, ExtendedError, Integration, StackParser } from '@sentry/types'; -import { isInstanceOf } from '@sentry/utils'; +import type { Event, EventHint, EventProcessor, Hub, Integration } from '@sentry/types'; +import { applyAggregateErrorsToEvent } from '@sentry/utils'; import { exceptionFromError } from '../eventbuilder'; import { ContextLines } from './contextlines'; @@ -41,15 +40,25 @@ export class LinkedErrors implements Integration { /** * @inheritDoc */ - public setupOnce(): void { - addGlobalEventProcessor(async (event: Event, hint: EventHint) => { + public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void { + addGlobalEventProcessor(async (event: Event, hint?: EventHint) => { const hub = getCurrentHub(); - const self = hub.getIntegration(LinkedErrors); const client = hub.getClient(); - if (client && self && self._handler && typeof self._handler === 'function') { - self._handler(client.getOptions().stackParser, event, hint); + const self = hub.getIntegration(LinkedErrors); + + if (!client || !self) { + return event; } + applyAggregateErrorsToEvent( + exceptionFromError, + client.getOptions().stackParser, + self._key, + self._limit, + event, + hint, + ); + // If the ContextLines integration is enabled, we add source code context to linked errors // because we can't guarantee the order that integrations are run. const contextLines = getCurrentHub().getIntegration(ContextLines); @@ -60,35 +69,4 @@ export class LinkedErrors implements Integration { return event; }); } - - /** - * @inheritDoc - */ - private _handler(stackParser: StackParser, event: Event, hint: EventHint): Event { - if (!event.exception || !event.exception.values || !hint || !isInstanceOf(hint.originalException, Error)) { - return event; - } - - const linkedErrors = this._walkErrorTree(stackParser, hint.originalException as ExtendedError, this._key); - event.exception.values = [...linkedErrors, ...event.exception.values]; - return event; - } - - /** - * @inheritDoc - */ - private _walkErrorTree( - stackParser: StackParser, - error: ExtendedError, - key: string, - stack: Exception[] = [], - ): Exception[] { - if (!isInstanceOf(error[key], Error) || stack.length + 1 >= this._limit) { - return stack; - } - - const exception = exceptionFromError(stackParser, error[key]); - - return this._walkErrorTree(stackParser, error[key], key, [exception, ...stack]); - } } diff --git a/packages/node/test/integrations/linkederrors.test.ts b/packages/node/test/integrations/linkederrors.test.ts deleted file mode 100644 index f59cf068e3de..000000000000 --- a/packages/node/test/integrations/linkederrors.test.ts +++ /dev/null @@ -1,151 +0,0 @@ -import type { ExtendedError } from '@sentry/types'; - -import type { Event } from '../../src'; -import { NodeClient } from '../../src'; -import { LinkedErrors } from '../../src/integrations/linkederrors'; -import { defaultStackParser as stackParser } from '../../src/sdk'; -import { getDefaultNodeClientOptions } from '../helper/node-client-options'; - -let linkedErrors: any; - -describe('LinkedErrors', () => { - beforeEach(() => { - linkedErrors = new LinkedErrors(); - }); - - describe('handler', () => { - it('should bail out if event doesnt contain exception', async () => { - expect.assertions(2); - const spy = jest.spyOn(linkedErrors, '_walkErrorTree'); - const event = { - message: 'foo', - }; - const result = linkedErrors._handler(stackParser, event, {}); - expect(spy.mock.calls.length).toEqual(0); - expect(result).toEqual(event); - }); - - it('should bail out if event contains exception, but no hint', async () => { - expect.assertions(2); - const spy = jest.spyOn(linkedErrors, '_walkErrorTree'); - const one = new Error('originalException'); - const options = getDefaultNodeClientOptions({ stackParser }); - const client = new NodeClient(options); - let event: Event | undefined; - return client - .eventFromException(one) - .then(eventFromException => { - event = eventFromException; - return linkedErrors._handler(stackParser, eventFromException, {}); - }) - .then(result => { - expect(spy.mock.calls.length).toEqual(0); - expect(result).toEqual(event); - }); - }); - - it('should call walkErrorTree if event contains exception and hint with originalException', async () => { - expect.assertions(1); - const spy = jest.spyOn(linkedErrors, '_walkErrorTree').mockImplementation(() => []); - const one = new Error('originalException'); - const options = getDefaultNodeClientOptions({ stackParser }); - const client = new NodeClient(options); - return client.eventFromException(one).then(event => { - linkedErrors._handler(stackParser, event, { - originalException: one, - }); - - expect(spy.mock.calls.length).toEqual(1); - }); - }); - - it('should recursively walk error to find linked exceptions and assign them to the event', async () => { - expect.assertions(10); - const one: ExtendedError = new Error('one'); - const two: ExtendedError = new TypeError('two'); - const three: ExtendedError = new SyntaxError('three'); - one.cause = two; - two.cause = three; - - const options = getDefaultNodeClientOptions({ stackParser }); - const client = new NodeClient(options); - return client.eventFromException(one).then(event => { - const result = linkedErrors._handler(stackParser, event, { - originalException: one, - }); - - expect(result.exception.values.length).toEqual(3); - expect(result.exception.values[0].type).toEqual('SyntaxError'); - expect(result.exception.values[0].value).toEqual('three'); - expect(result.exception.values[0].stacktrace).toHaveProperty('frames'); - expect(result.exception.values[1].type).toEqual('TypeError'); - expect(result.exception.values[1].value).toEqual('two'); - expect(result.exception.values[1].stacktrace).toHaveProperty('frames'); - expect(result.exception.values[2].type).toEqual('Error'); - expect(result.exception.values[2].value).toEqual('one'); - expect(result.exception.values[2].stacktrace).toHaveProperty('frames'); - }); - }); - - it('should allow to change walk key', async () => { - expect.assertions(10); - linkedErrors = new LinkedErrors({ - key: 'reason', - }); - - const one: ExtendedError = new Error('one'); - const two: ExtendedError = new TypeError('two'); - const three: ExtendedError = new SyntaxError('three'); - one.reason = two; - two.reason = three; - - const options = getDefaultNodeClientOptions({ stackParser }); - const client = new NodeClient(options); - return client.eventFromException(one).then(event => { - const result = linkedErrors._handler(stackParser, event, { - originalException: one, - }); - - expect(result.exception.values.length).toEqual(3); - expect(result.exception.values[0].type).toEqual('SyntaxError'); - expect(result.exception.values[0].value).toEqual('three'); - expect(result.exception.values[0].stacktrace).toHaveProperty('frames'); - expect(result.exception.values[1].type).toEqual('TypeError'); - expect(result.exception.values[1].value).toEqual('two'); - expect(result.exception.values[1].stacktrace).toHaveProperty('frames'); - expect(result.exception.values[2].type).toEqual('Error'); - expect(result.exception.values[2].value).toEqual('one'); - expect(result.exception.values[2].stacktrace).toHaveProperty('frames'); - }); - }); - - it('should allow to change stack size limit', async () => { - expect.assertions(7); - linkedErrors = new LinkedErrors({ - limit: 2, - }); - - const one: ExtendedError = new Error('one'); - const two: ExtendedError = new TypeError('two'); - const three: ExtendedError = new SyntaxError('three'); - one.cause = two; - two.cause = three; - - const options = getDefaultNodeClientOptions({ stackParser }); - const client = new NodeClient(options); - return client.eventFromException(one).then(event => { - const result = linkedErrors._handler(stackParser, event, { - originalException: one, - }); - - expect(result.exception.values.length).toEqual(2); - expect(result.exception.values[0].type).toEqual('TypeError'); - expect(result.exception.values[0].value).toEqual('two'); - expect(result.exception.values[0].stacktrace).toHaveProperty('frames'); - expect(result.exception.values[1].type).toEqual('Error'); - expect(result.exception.values[1].value).toEqual('one'); - expect(result.exception.values[1].stacktrace).toHaveProperty('frames'); - }); - }); - }); -}); diff --git a/packages/utils/src/aggregate-errors.ts b/packages/utils/src/aggregate-errors.ts new file mode 100644 index 000000000000..bc61ba49de41 --- /dev/null +++ b/packages/utils/src/aggregate-errors.ts @@ -0,0 +1,50 @@ +import type { Event, EventHint, Exception, ExtendedError, StackParser } from '@sentry/types'; + +import { isInstanceOf } from './is'; + +/** + * Creates exceptions inside `event.exception.values` for errors that are nested on properties based on the `key` parameter. + */ +export function applyAggregateErrorsToEvent( + exceptionFromErrorImplementation: (stackParser: StackParser, ex: Error) => Exception, + parser: StackParser, + key: string, + limit: number, + event: Event, + hint?: EventHint, +): Event | null { + if (!event.exception || !event.exception.values || !hint || !isInstanceOf(hint.originalException, Error)) { + return event; + } + + const linkedErrors = aggregateExceptionsFromError( + exceptionFromErrorImplementation, + parser, + limit, + hint.originalException as ExtendedError, + key, + ); + + event.exception.values = [...linkedErrors, ...event.exception.values]; + + return event; +} + +function aggregateExceptionsFromError( + exceptionFromErrorImplementation: (stackParser: StackParser, ex: Error) => Exception, + parser: StackParser, + limit: number, + error: ExtendedError, + key: string, + stack: Exception[] = [], +): Exception[] { + if (!isInstanceOf(error[key], Error) || stack.length >= limit) { + return stack; + } + + const exception = exceptionFromErrorImplementation(parser, error[key]); + return aggregateExceptionsFromError(exceptionFromErrorImplementation, parser, limit, error[key], key, [ + exception, + ...stack, + ]); +} diff --git a/packages/utils/src/index.ts b/packages/utils/src/index.ts index 6b9426c22149..0464dbec25da 100644 --- a/packages/utils/src/index.ts +++ b/packages/utils/src/index.ts @@ -1,3 +1,4 @@ +export * from './aggregate-errors'; export * from './browser'; export * from './dsn'; export * from './error'; diff --git a/packages/utils/test/aggregate-errors.test.ts b/packages/utils/test/aggregate-errors.test.ts new file mode 100644 index 000000000000..2f84c3629982 --- /dev/null +++ b/packages/utils/test/aggregate-errors.test.ts @@ -0,0 +1,97 @@ +import type { Event, EventHint, Exception, ExtendedError, StackParser } from '@sentry/types'; + +import { applyAggregateErrorsToEvent, createStackParser } from '../src/index'; + +const stackParser = createStackParser([0, line => ({ filename: line })]); +const exceptionFromError = (_stackParser: StackParser, ex: Error): Exception => { + return { value: ex.message }; +}; + +describe('applyAggregateErrorsToEvent()', () => { + test('should not do anything if event does not contain an exception', () => { + const event: Event = { exception: undefined }; + const eventHint: EventHint = { originalException: new Error() }; + applyAggregateErrorsToEvent(exceptionFromError, stackParser, 'cause', 100, event, eventHint); + + // no changes + expect(event).toStrictEqual({ exception: undefined }); + }); + + test('should not do anything if event does not contain exception values', () => { + const event: Event = { exception: { values: undefined } }; + const eventHint: EventHint = { originalException: new Error() }; + applyAggregateErrorsToEvent(exceptionFromError, stackParser, 'cause', 100, event, eventHint); + + // no changes + expect(event).toStrictEqual({ exception: { values: undefined } }); + }); + + test('should not do anything if event does not contain an event hint', () => { + const event: Event = { exception: { values: [] } }; + applyAggregateErrorsToEvent(exceptionFromError, stackParser, 'cause', 100, event, undefined); + + // no changes + expect(event).toStrictEqual({ exception: { values: [] } }); + }); + + test('should not do anything if the event hint does not contain an original exception', () => { + const event: Event = { exception: { values: [] } }; + const eventHint: EventHint = { originalException: undefined }; + applyAggregateErrorsToEvent(exceptionFromError, stackParser, 'cause', 100, event, eventHint); + + // no changes + expect(event).toStrictEqual({ exception: { values: [] } }); + }); + + test('should recursively walk the original exception based on the `key` option and add them as exceptions to the event', () => { + const key = 'cause'; + const originalException: ExtendedError = new Error('Root Error'); + const event: Event = { exception: { values: [exceptionFromError(stackParser, originalException)] } }; + originalException[key] = new Error('Nested Error 1'); + originalException[key][key] = new Error('Nested Error 2'); + const eventHint: EventHint = { originalException }; + applyAggregateErrorsToEvent(exceptionFromError, stackParser, key, 100, event, eventHint); + expect(event).toStrictEqual({ + exception: { + values: [ + { + value: 'Nested Error 2', + }, + { + value: 'Nested Error 1', + }, + { + value: 'Root Error', + }, + ], + }, + }); + }); + + test('should allow to limit number of attached errors', () => { + const key = 'cause'; + const originalException: ExtendedError = new Error('Root Error'); + const event: Event = { exception: { values: [exceptionFromError(stackParser, originalException)] } }; + + let err = originalException; + for (let i = 0; i < 10; i++) { + const newErr = new Error('Nested Error!'); + err[key] = newErr; + err = newErr; + } + + const eventHint: EventHint = { originalException }; + applyAggregateErrorsToEvent(exceptionFromError, stackParser, key, 5, event, eventHint); + + // 6 -> one for original exception + 5 linked + expect(event.exception?.values).toHaveLength(5 + 1); + + // Last exception in list should be the root exception + expect(event.exception?.values?.[event.exception?.values.length - 1]).toStrictEqual({ + value: 'Root Error', + }); + }); + + test.todo('should recursively walk AggregateErrors and add them as exceptions to the event'); + test.todo('should recursively walk mixed errors (Aggregate errors and based on `key`)'); +});