Skip to content

Rework handling of AggregateException #2240

Closed
@mattjohnsonpint

Description

Problem Statement

In #1672, we implemented logic for dealing with AggregateException by flattening the tree of inner exceptions and their children into a single list of exceptions. At the same time, we added an option KeepAggregateException that controls whether the AggregateException objects would remain in that list or be stripped out. The default is false, meaning to strip them out.

Since then, we also ran in to several bugs caused by this approach:

Despite those fixes, we still have several problems with these workarounds not fitting the Sentry design - which is that multiple exceptions represent a single chain of events.

Consider this example:

  • AggregateException
    • A
      • 1
    • B
      • 2

Currently, we'd flatten that in to one big list and remove the aggregate (by default). Sentry reverses the ordering, and presently we sort the inner exceptions in ascending order, so the list sent would be [1,A,2,B]. An issue would be titled with the details of B, showing 2 as the cause of B (correct), then A as the cause of 2 (incorrect), then 1 as the cause of A (correct).

Besides the incorrect ordering, this also has other more serious consequences:

  • An issue will not get created for A. If it happened to be a different exception type than B, it could go unnoticed.
  • The issue grouping rules will take the whole chain of exceptions into account. Thus if there's another error raised independently for B, it wouldn't be grouped with the instance of B that was part of the aggregate.
  • Similarly, the quantity of exceptions can affect the grouping. Say one has code that performs parallel tasks, but might have a different number of tasks each time. [A,A] would be grouped separately from [A,A,A] or [A,A,A,A,A,A] (etc.) - each issue having an identical title and cause. This also messes up any alerting rules, and can resurrect issues that were intentionally ignored.
  • Within the SDK itself, we have issues with processing aggregate exceptions:
    • Exception filters will only filter out an `AggregateException if all of the inner exceptions were supposed to be filtered.
    • Exception processors, event processors, and BeforeSend all present interfaces that require special handling of AggregateException in each implementation.
    • When we strip away an aggregate exception, if there was no stack trace on the inner exception then we copy the one from the aggregate. This is usually appropriate - so that one can have some sense of where to start debugging. However, the stack trace now no longer applies to the exception being viewed - so it's a bit disingenuous.

Ultimately, the applied workaround is not very robust. It works best when there's only one inner exception in the aggregate, but has problems when there are multiple.

Without the workarounds, the above example would be sent as [1,A,AggregateException]. All issues with an aggregate would be titled the same, and we'd lose any information about B. So this is also not a good plan.

Solution Brainstorm

The latest version of Python (3.11) had a similar feature added called ExceptionGroup. There's also AggregateError in JavaScript, and several others. Together these have increased the demand for a properly designed solution.

We're tracking the new design in getsentry/sentry#37716. When ready, we should undo the previous .NET workarounds and implement the new unified design. Please add feedback about the new design over there.

We'll leave this issue open to track the work in .NET, and to comment on any other issues specifically related to AggregateException or the workarounds we've currently implemented.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Labels

FeatureNew feature or request

Type

No type

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions