Rework handling of AggregateException

[mattjohnsonpint]

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:

• Nested inner exceptions are not filtered correctly #1916
  • fixed by Exception filters should consider child exceptions of an AggregateException #1924
• Stack traces missing from inner exceptions of aggregate exceptions #1917
  • fixed by Fix missing details when aggregate exception is filtered out #1922
• UnobservedTaskExceptionIntegration could provide a stack trace (but doesn't) #2065
  • fixed by Fix missing stack trace on UnobservedTaskException #2067

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.

[mattjohnsonpint]

Reference: https://github.com/getsentry/rfcs/blob/main/text/0079-exception-groups.md

That RFC was revised after feedback from various other teams within Sentry. Adjusting for the new plan, the changes to the Sentry .NET SDK would be as follows:

• Deprecate KeepAggregateException. Aggregate exceptions will always be kept.
• Remove any current workarounds where we are moving the stack trace or other fields between different exceptions.
• Update the recursive walking of the exception tree such that inner exceptions are followed in the correct order.
• Annotate the mechanism with the new fields
• Wait for server-side updates to Sentry issue grouping, UI, and Relay before releasing.
• Be sure to note in the changelog that these changes affect issue grouping, and require version x.y.z of Sentry (for self-hosted, etc.)