Rework handling of AggregateException to align with Sentry Exceptio…

…n Groups (#2287)

CHANGELOG.md

@@ -1,5 +1,15 @@
 # Changelog
 
+## Unreleased
+
+### Features
+
+- .NET SDK changes for exception groups ([#2287](https://github.com/getsentry/sentry-dotnet/pull/2287))
+  - This changes how `AggregateException` is handled.  Instead of filtering them out client-side, the SDK marks them as an "exception group",
+    and adds includes data that represents the hierarchical structure of inner exceptions. Sentry now recognizes this server-side,
+    improving the accuracy of the issue detail page.
+  - Accordingly, the `KeepAggregateException` option is now obsolete and does nothing.  Please remove any usages of `KeepAggregateException`.
+
 ## 3.32.0
 
 ### Features

src/Sentry/Internal/Extensions/MiscExtensions.cs

@@ -82,4 +82,19 @@ public static void CancelAfterSafe(this CancellationTokenSource cts, TimeSpan ti
         TKey key,
         TValue value) =>
         collection.Add(new KeyValuePair<TKey, TValue>(key, value));
+
+    internal static string GetRawMessage(this AggregateException exception)
+    {
+        var message = exception.Message;
+        if (exception.InnerException is { } inner)
+        {
+            var i = message.IndexOf($" ({inner.Message})", StringComparison.Ordinal);
+            if (i > 0)
+            {
+                return message[..i];
+            }
+        }
+
+        return message;
+    }
 }

src/Sentry/Internal/MainExceptionProcessor.cs

@@ -30,6 +30,68 @@ public void Process(Exception exception, SentryEvent sentryEvent)
         sentryEvent.SentryExceptions = sentryExceptions;
     }
 
+    // Sentry exceptions are sorted oldest to newest.
+    // See https://develop.sentry.dev/sdk/event-payloads/exception
+    internal IReadOnlyList<SentryException> CreateSentryExceptions(Exception exception)
+    {
+        var exceptions = WalkExceptions(exception).Reverse().ToList();
+
+        // In the case of only one exception, ExceptionId and ParentId are useless.
+        if (exceptions.Count == 1 && exceptions[0].Mechanism is { } mechanism)
+        {
+            mechanism.ExceptionId = null;
+            mechanism.ParentId = null;
+            if (mechanism.IsDefaultOrEmpty())
+            {
+                // No need to convey an empty mechanism.
+                exceptions[0].Mechanism = null;
+            }
+        }
+
+        return exceptions;
+    }
+
+    private class Counter
+    {
+        private int _value;
+
+        public int GetNextValue() => _value++;
+    }
+
+    private IEnumerable<SentryException> WalkExceptions(Exception exception) =>
+        WalkExceptions(exception, new Counter(), null, null);
+
+    private IEnumerable<SentryException> WalkExceptions(Exception exception, Counter counter, int? parentId, string? source)
+    {
+        var ex = exception;
+        while (ex is not null)
+        {
+            var id = counter.GetNextValue();
+
+            yield return BuildSentryException(ex, id, parentId, source);
+
+            if (ex is AggregateException aex)
+            {
+                for (var i = 0; i < aex.InnerExceptions.Count; i++)
+                {
+                    ex = aex.InnerExceptions[i];
+                    source = $"{nameof(AggregateException.InnerExceptions)}[{i}]";
+                    var sentryExceptions = WalkExceptions(ex, counter, id, source);
+                    foreach (var sentryException in sentryExceptions)
+                    {
+                        yield return sentryException;
+                    }
+                }
+
+                break;
+            }
+
+            ex = ex.InnerException;
+            parentId = id;
+            source = nameof(AggregateException.InnerException);
+        }
+    }
+
     private static void MoveExceptionDataToEvent(SentryEvent sentryEvent, IEnumerable<SentryException> sentryExceptions)
     {
         var keysToRemove = new List<string>();
@@ -77,41 +139,17 @@ private static void MoveExceptionDataToEvent(SentryEvent sentryEvent, IEnumerabl
         }
     }
 
-    internal List<SentryException> CreateSentryExceptions(Exception exception)
-    {
-        var exceptions = exception
-            .EnumerateChainedExceptions(_options)
-            .Select(BuildSentryException)
-            .ToList();
-
-        // If we've filtered out the aggregate exception, we'll need to copy over details from it.
-        if (exception is AggregateException && !_options.KeepAggregateException)
-        {
-            var original = BuildSentryException(exception);
-
-            // Exceptions are sent from oldest to newest, so the details belong on the LAST exception.
-            var last = exceptions.Last();
-            last.Mechanism = original.Mechanism;
-
-            // In some cases the stack trace is already positioned on the inner exception.
-            // Only copy it over when it is missing.
-            last.Stacktrace ??= original.Stacktrace;
-        }
-
-        return exceptions;
-    }
-
-    private SentryException BuildSentryException(Exception exception)
+    private SentryException BuildSentryException(Exception exception, int id, int? parentId, string? source)
     {
         var sentryEx = new SentryException
         {
             Type = exception.GetType().FullName,
             Module = exception.GetType().Assembly.FullName,
-            Value = exception.Message,
+            Value = exception is AggregateException agg ? agg.GetRawMessage() : exception.Message,
             ThreadId = Environment.CurrentManagedThreadId
         };
 
-        var mechanism = GetMechanism(exception);
+        var mechanism = GetMechanism(exception, id, parentId, source);
         if (!mechanism.IsDefaultOrEmpty())
         {
             sentryEx.Mechanism = mechanism;
@@ -121,7 +159,7 @@ private SentryException BuildSentryException(Exception exception)
         return sentryEx;
     }
 
-    private static Mechanism GetMechanism(Exception exception)
+    private static Mechanism GetMechanism(Exception exception, int id, int? parentId, string? source)
     {
         var mechanism = new Mechanism();
 
@@ -167,6 +205,16 @@ private static Mechanism GetMechanism(Exception exception)
             mechanism.Data[key] = exception.Data[key]!;
         }
 
+        mechanism.ExceptionId = id;
+        mechanism.ParentId = parentId;
+        mechanism.Source = source;
+        mechanism.IsExceptionGroup = exception is AggregateException;
+
+        if (source != null)
+        {
+            mechanism.Type = "chained";
+        }
+
         return mechanism;
     }
 }

src/Sentry/Protocol/Mechanism.cs

@@ -56,6 +56,16 @@ public string Type
     /// </summary>
     public string? Description { get; set; }
 
+    /// <summary>
+    /// An optional value to explain the source of the exception.
+    /// </summary>
+    /// <remarks>
+    /// For chained exceptions, this should be the property name where the exception was retrieved from its parent
+    /// exception.  In .NET, either &quot;<see cref="Exception.InnerException"/>&quot; or <c>&quot;InnerExceptions[i]&quot;</c>
+    /// (where <c>i</c> is replaced with the numeric index within <see cref="AggregateException.InnerExceptions"/>).
+    /// </remarks>
+    public string? Source { get; set; }
+
     /// <summary>
     /// Optional fully qualified URL to an online help resource, possible interpolated with error parameters.
     /// </summary>
@@ -71,6 +81,31 @@ public string Type
     /// </summary>
     public bool Synthetic { get; set; }
 
+    /// <summary>
+    /// Whether the exception represents an exception group.
+    /// In .NET, an <see cref="AggregateException"/>.
+    /// </summary>
+    public bool IsExceptionGroup { get; set; }
+
+    /// <summary>
+    /// A numeric identifier assigned to the exception by the SDK.
+    /// </summary>
+    /// <remarks>
+    /// The SDK should assign a different ID to each exception in an event, starting with the root exception as 0,
+    /// and incrementing thereafter. This ID can be used with <see cref="ParentId"/> to reconstruct the logical
+    /// structure of an exception group.  When <c>null</c>, Sentry will assume that all exceptions in an event are
+    /// in a single chain.
+    /// </remarks>
+    public int? ExceptionId { get; set; }
+
+    /// <summary>
+    /// The parent exception's identifier, or <c>null</c> for the root exception.
+    /// </summary>
+    /// <remarks>
+    /// This ID can be used with <see cref="ExceptionId"/> to reconstruct the logical structure of an exception group.
+    /// </remarks>
+    public int? ParentId { get; set; }
+
     /// <summary>
     /// Optional information from the operating system or runtime on the exception mechanism.
     /// </summary>
@@ -95,9 +130,13 @@ public void WriteTo(Utf8JsonWriter writer, IDiagnosticLogger? logger)
 
         writer.WriteString("type", Type);
         writer.WriteStringIfNotWhiteSpace("description", Description);
+        writer.WriteStringIfNotWhiteSpace("source", Source);
         writer.WriteStringIfNotWhiteSpace("help_link", HelpLink);
         writer.WriteBooleanIfNotNull("handled", Handled);
         writer.WriteBooleanIfTrue("synthetic", Synthetic);
+        writer.WriteBooleanIfTrue("is_exception_group", IsExceptionGroup);
+        writer.WriteNumberIfNotNull("exception_id", ExceptionId);
+        writer.WriteNumberIfNotNull("parent_id", ParentId);
         writer.WriteDictionaryIfNotEmpty("data", InternalData!, logger);
         writer.WriteDictionaryIfNotEmpty("meta", InternalMeta!, logger);
 
@@ -111,19 +150,27 @@ public static Mechanism FromJson(JsonElement json)
     {
         var type = json.GetPropertyOrNull("type")?.GetString();
         var description = json.GetPropertyOrNull("description")?.GetString();
+        var source = json.GetPropertyOrNull("source")?.GetString();
         var helpLink = json.GetPropertyOrNull("help_link")?.GetString();
         var handled = json.GetPropertyOrNull("handled")?.GetBoolean();
         var synthetic = json.GetPropertyOrNull("synthetic")?.GetBoolean() ?? false;
+        var isExceptionGroup = json.GetPropertyOrNull("is_exception_group")?.GetBoolean() ?? false;
+        var exceptionId = json.GetPropertyOrNull("exception_id")?.GetInt32();
+        var parentId = json.GetPropertyOrNull("parent_id")?.GetInt32();
         var data = json.GetPropertyOrNull("data")?.GetDictionaryOrNull();
         var meta = json.GetPropertyOrNull("meta")?.GetDictionaryOrNull();
 
         return new Mechanism
         {
             Type = type,
             Description = description,
+            Source = source,
             HelpLink = helpLink,
             Handled = handled,
             Synthetic = synthetic,
+            IsExceptionGroup = isExceptionGroup,
+            ExceptionId = exceptionId,
+            ParentId = parentId,
             InternalData = data?.WhereNotNullValue().ToDictionary(),
             InternalMeta = meta?.WhereNotNullValue().ToDictionary()
         };
@@ -132,9 +179,13 @@ public static Mechanism FromJson(JsonElement json)
     internal bool IsDefaultOrEmpty() =>
         Handled is null &&
         Synthetic == false &&
+        IsExceptionGroup == false &&
+        ExceptionId is null &&
+        ParentId is null &&
         Type == DefaultType &&
         string.IsNullOrWhiteSpace(Description) &&
         string.IsNullOrWhiteSpace(HelpLink) &&
+        string.IsNullOrWhiteSpace(Source) &&
         !(InternalData?.Count > 0) &&
         !(InternalMeta?.Count > 0);
 }

src/Sentry/SentryClient.cs

@@ -306,13 +306,15 @@ private SentryId DoSendEvent(SentryEvent @event, Hint? hint, Scope? scope)
             return new[] { exception };
         }
 
-        if (exception is AggregateException aggregate &&
-            aggregate.InnerExceptions.All(e => ApplyExceptionFilters(e) != null))
+        if (exception is AggregateException aggregate)
         {
-            // All inner exceptions of the aggregate matched a filter, so the event should be filtered.
-            // Note that _options.KeepAggregateException is not relevant here.  Even if we want to keep aggregate
-            // exceptions, we would still never send one if all of its children are supposed to be filtered.
-            return aggregate.InnerExceptions;
+            // Flatten the tree of aggregates such that all the inner exceptions are non-aggregates.
+            var innerExceptions = aggregate.Flatten().InnerExceptions;
+            if (innerExceptions.All(e => ApplyExceptionFilters(e) != null))
+            {
+                // All inner exceptions matched a filter, so the event should be filtered.
+                return innerExceptions;
+            }
         }
 
         // The event should not be filtered.

src/Sentry/SentryExceptionExtensions.cs

@@ -1,4 +1,3 @@
-using Sentry;
 using Sentry.Internal;
 using Sentry.Protocol;
 
@@ -56,40 +55,4 @@ public static void AddSentryContext(this Exception ex, string name, IReadOnlyDic
             ex.Data[Mechanism.HandledKey] = handled;
         }
     }
-
-    /// <summary>
-    /// Recursively enumerates all <see cref="AggregateException.InnerExceptions"/> and <see cref="Exception.InnerException"/>
-    /// Not for public use.
-    /// </summary>
-    [EditorBrowsable(EditorBrowsableState.Never)]
-    public static IEnumerable<Exception> EnumerateChainedExceptions(this Exception exception, SentryOptions options)
-    {
-        if (exception is AggregateException aggregateException)
-        {
-            foreach (var inner in EnumerateInner(options, aggregateException))
-            {
-                yield return inner;
-            }
-
-            if (!options.KeepAggregateException)
-            {
-                yield break;
-            }
-        }
-        else if (exception.InnerException != null)
-        {
-            foreach (var inner in exception.InnerException.EnumerateChainedExceptions(options))
-            {
-                yield return inner;
-            }
-        }
-
-        yield return exception;
-    }
-
-    private static IEnumerable<Exception> EnumerateInner(SentryOptions options, AggregateException aggregateException)
-    {
-        return aggregateException.InnerExceptions
-            .SelectMany(exception => exception.EnumerateChainedExceptions(options));
-    }
 }

src/Sentry/SentryOptions.cs

@@ -900,10 +900,14 @@ public StackTraceMode StackTraceMode
     public Func<bool>? CrashedLastRun { get; set; }
 
     /// <summary>
-    /// Keep <see cref="AggregateException"/> in sentry logging.
-    /// The default behaviour is to only log <see cref="AggregateException.InnerExceptions"/> and not include the root <see cref="AggregateException"/>.
-    /// Set KeepAggregateException to true to include the root <see cref="AggregateException"/>.
+    /// This property is no longer used.  It will be removed in a future version.
     /// </summary>
+    /// <remarks>
+    /// All exceptions are now sent to Sentry, including <see cref="AggregateException"/>s.
+    /// The issue grouping rules in Sentry have been updated to accomodate "exception groups",
+    /// such as <see cref="AggregateException"/> in .NET.
+    /// </remarks>
+    [Obsolete("This property is no longer used.  It will be removed in a future version.")]
     public bool KeepAggregateException { get; set; }
 
     /// <summary>

test/Sentry.NLog.Tests/IntegrationTests.Simple.Core3_1.verified.txt

@@ -53,7 +53,8 @@
                 Mechanism: {
                   Type: generic,
                   Handled: true,
-                  Synthetic: false
+                  Synthetic: false,
+                  IsExceptionGroup: false
                 }
               }
             ],

test/Sentry.NLog.Tests/IntegrationTests.Simple.DotNet6_0.verified.txt

@@ -53,7 +53,8 @@
                 Mechanism: {
                   Type: generic,
                   Handled: true,
-                  Synthetic: false
+                  Synthetic: false,
+                  IsExceptionGroup: false
                 }
               }
             ],

test/Sentry.NLog.Tests/IntegrationTests.Simple.DotNet7_0.verified.txt

@@ -53,7 +53,8 @@
                 Mechanism: {
                   Type: generic,
                   Handled: true,
-                  Synthetic: false
+                  Synthetic: false,
+                  IsExceptionGroup: false
                 }
               }
             ],