From 375a0a6908712e74bc6cdcc7af83e928d554d648 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 29 Feb 2024 07:42:16 -0800 Subject: [PATCH] fix(processing): Only mark aggregate errors as exception groups (#10850) When Sentry started supporting the idea of exception groups, two changes happened. In the SDK, we adapted our logic for handling linked errors to also handle `AggregateError`s. And in our ingest pipeline, we began looking for an `is_exception_group` flag on the last entry in `event.exception.values; when we found it, we'd then ignore that entry when grouping and titling events, under the assumption that it was just a container and therefore wasn't meaningful. When it came to instances of `AggregateError`, this worked great. For linked errors, however, this caused us to focus on the `cause` error rather than the error which was actually caught, with the result that it both threw off grouping and made for some very unhelpful titling of issues. (See the screenshot below, in which the first three errors are, respectively, an `UndefinedResponseBodyError`, a `RequestError`, and an `InternalServerError`, though you'd be hard pressed to figure that out without opening them up.) This fixes those problems by restricting the use of the `is_exception_group` flag to `AggregateError`s. Note: In order to update the tests to work with this change, I had add in consideration of the error `name` property and the corresponding event `type` property, to match what we do in real life. To keep things readable, there's a new mock `AggregateError` class, which I adapted all the tests to use. --- packages/utils/src/aggregate-errors.ts | 3 +- packages/utils/test/aggregate-errors.test.ts | 51 ++++++++++++++------ 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/packages/utils/src/aggregate-errors.ts b/packages/utils/src/aggregate-errors.ts index 4547203b4fd2..864956ad4716 100644 --- a/packages/utils/src/aggregate-errors.ts +++ b/packages/utils/src/aggregate-errors.ts @@ -57,6 +57,7 @@ function aggregateExceptionsFromError( let newExceptions = [...prevExceptions]; + // Recursively call this function in order to walk down a chain of errors if (isInstanceOf(error[key], Error)) { applyExceptionGroupFieldsForParentException(exception, exceptionId); const newException = exceptionFromErrorImplementation(parser, error[key]); @@ -106,7 +107,7 @@ function applyExceptionGroupFieldsForParentException(exception: Exception, excep exception.mechanism = { ...exception.mechanism, - is_exception_group: true, + ...(exception.type === 'AggregateError' && { is_exception_group: true }), exception_id: exceptionId, }; } diff --git a/packages/utils/test/aggregate-errors.test.ts b/packages/utils/test/aggregate-errors.test.ts index 66b0f3fcfdb1..8d5fb3be6ded 100644 --- a/packages/utils/test/aggregate-errors.test.ts +++ b/packages/utils/test/aggregate-errors.test.ts @@ -4,8 +4,17 @@ import { applyAggregateErrorsToEvent, createStackParser } from '../src/index'; const stackParser = createStackParser([0, line => ({ filename: line })]); const exceptionFromError = (_stackParser: StackParser, ex: Error): Exception => { - return { value: ex.message, mechanism: { type: 'instrument', handled: true } }; + return { value: ex.message, type: ex.name, mechanism: { type: 'instrument', handled: true } }; }; +class FakeAggregateError extends Error { + public errors: Error[]; + + constructor(errors: Error[], message: string) { + super(message); + this.errors = errors; + this.name = 'AggregateError'; + } +} describe('applyAggregateErrorsToEvent()', () => { test('should not do anything if event does not contain an exception', () => { @@ -57,6 +66,7 @@ describe('applyAggregateErrorsToEvent()', () => { exception: { values: [ { + type: 'Error', value: 'Nested Error 2', mechanism: { exception_id: 2, @@ -67,22 +77,22 @@ describe('applyAggregateErrorsToEvent()', () => { }, }, { + type: 'Error', value: 'Nested Error 1', mechanism: { exception_id: 1, handled: true, parent_id: 0, - is_exception_group: true, source: 'cause', type: 'chained', }, }, { + type: 'Error', value: 'Root Error', mechanism: { exception_id: 0, handled: true, - is_exception_group: true, type: 'instrument', }, }, @@ -123,19 +133,21 @@ describe('applyAggregateErrorsToEvent()', () => { // Last exception in list should be the root exception expect(event.exception?.values?.[event.exception?.values.length - 1]).toStrictEqual({ + type: 'Error', value: 'Root Error', mechanism: { exception_id: 0, handled: true, - is_exception_group: true, type: 'instrument', }, }); }); test('should keep the original mechanism type for the root exception', () => { - const fakeAggregateError: ExtendedError = new Error('Root Error'); - fakeAggregateError.errors = [new Error('Nested Error 1'), new Error('Nested Error 2')]; + const fakeAggregateError = new FakeAggregateError( + [new Error('Nested Error 1'), new Error('Nested Error 2')], + 'Root Error', + ); const event: Event = { exception: { values: [exceptionFromError(stackParser, fakeAggregateError)] } }; const eventHint: EventHint = { originalException: fakeAggregateError }; @@ -147,10 +159,12 @@ describe('applyAggregateErrorsToEvent()', () => { test('should recursively walk mixed errors (Aggregate errors and based on `key`)', () => { const chainedError: ExtendedError = new Error('Nested Error 3'); chainedError.cause = new Error('Nested Error 4'); - const fakeAggregateError2: ExtendedError = new Error('AggregateError2'); - fakeAggregateError2.errors = [new Error('Nested Error 2'), chainedError]; - const fakeAggregateError1: ExtendedError = new Error('AggregateError1'); - fakeAggregateError1.errors = [new Error('Nested Error 1'), fakeAggregateError2]; + + const fakeAggregateError2 = new FakeAggregateError([new Error('Nested Error 2'), chainedError], 'AggregateError2'); + const fakeAggregateError1 = new FakeAggregateError( + [new Error('Nested Error 1'), fakeAggregateError2], + 'AggregateError1', + ); const event: Event = { exception: { values: [exceptionFromError(stackParser, fakeAggregateError1)] } }; const eventHint: EventHint = { originalException: fakeAggregateError1 }; @@ -167,17 +181,18 @@ describe('applyAggregateErrorsToEvent()', () => { source: 'cause', type: 'chained', }, + type: 'Error', value: 'Nested Error 4', }, { mechanism: { exception_id: 4, handled: true, - is_exception_group: true, parent_id: 2, source: 'errors[1]', type: 'chained', }, + type: 'Error', value: 'Nested Error 3', }, { @@ -188,6 +203,7 @@ describe('applyAggregateErrorsToEvent()', () => { source: 'errors[0]', type: 'chained', }, + type: 'Error', value: 'Nested Error 2', }, { @@ -199,6 +215,7 @@ describe('applyAggregateErrorsToEvent()', () => { source: 'errors[1]', type: 'chained', }, + type: 'AggregateError', value: 'AggregateError2', }, { @@ -209,6 +226,7 @@ describe('applyAggregateErrorsToEvent()', () => { source: 'errors[0]', type: 'chained', }, + type: 'Error', value: 'Nested Error 1', }, { @@ -218,6 +236,7 @@ describe('applyAggregateErrorsToEvent()', () => { is_exception_group: true, type: 'instrument', }, + type: 'AggregateError', value: 'AggregateError1', }, ], @@ -239,6 +258,7 @@ describe('applyAggregateErrorsToEvent()', () => { exception: { values: [ { + type: 'Error', value: 'Nested Error 2', mechanism: { exception_id: 2, @@ -249,22 +269,22 @@ describe('applyAggregateErrorsToEvent()', () => { }, }, { + type: 'Error', value: 'Nested Error 1', mechanism: { exception_id: 1, handled: true, parent_id: 0, - is_exception_group: true, source: 'cause', type: 'chained', }, }, { + type: 'Error', value: 'Root Error', mechanism: { exception_id: 0, handled: true, - is_exception_group: true, type: 'instrument', }, }, @@ -287,6 +307,7 @@ describe('applyAggregateErrorsToEvent()', () => { exception: { values: [ { + type: 'Error', value: 'Nested Error 2 ...', mechanism: { exception_id: 2, @@ -297,22 +318,22 @@ describe('applyAggregateErrorsToEvent()', () => { }, }, { + type: 'Error', value: 'Nested Error 1 ...', mechanism: { exception_id: 1, handled: true, parent_id: 0, - is_exception_group: true, source: 'cause', type: 'chained', }, }, { + type: 'Error', value: 'Root Error with...', mechanism: { exception_id: 0, handled: true, - is_exception_group: true, type: 'instrument', }, },