Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Most exceptions are logged as System.AggregateException #270

Closed
skolima opened this issue Aug 29, 2019 · 9 comments · Fixed by #1672
Closed

Most exceptions are logged as System.AggregateException #270

skolima opened this issue Aug 29, 2019 · 9 comments · Fixed by #1672

Comments

@skolima
Copy link

skolima commented Aug 29, 2019

Running with .NET 4.7.2, with heavy use of Parallel.ForEach. After moving to sentry-dotnet from Raven, most of our exceptions now show up in Sentry dashboard as "System.AggregateException: One or more errors occurred.". Raven used to unpack those if there was just a single inner exception, which made the dashboard a lot more "glanceable".

Now I understand this might be an explicit design choice not to unpack them: in this case, could you please provide a code sample how to preprocess them in the client code (I'm guessing somewhere with filters?)

@bruno-garcia
Copy link
Member

Although some of .NET's API will wrap exceptions inAggregateException, simply because more than one exception could have happened at that point (i.e TPL), it's also a valid scenario for a user to throw it's own instance, with a meaningful message and location (stacktrace).
Also, a catch block in between the throw and Sentry capturing it could have added useful stuff into Data. That would also be lost if we simply dropped the AggregateException

With that in mind, it was a design idea to not throw this information away automatically but perhaps shortsighted as for many usages of the TPL like yours this is likely noise only.

It's a fair request to add the possibility to drop AggregateException altogether by default (on 2.0.0 which is breaking) although that will affect event grouping and could be a showstopper to many, I'm afraid.

Another option is to add an opt-in: UnwrapOnlyAggregateException or something with a much better name.

Code doing all that is here:

internal IEnumerable<SentryException> CreateSentryException(Exception exception)
{
Debug.Assert(exception != null);
if (exception is AggregateException ae)
{
foreach (var inner in ae.InnerExceptions.SelectMany(CreateSentryException))
{
yield return inner;
}
}
else if (exception.InnerException != null)
{
foreach (var inner in CreateSentryException(exception.InnerException))
{
yield return inner;
}
}
var sentryEx = new SentryException
{
Type = exception.GetType()?.FullName,
Module = exception.GetType()?.Assembly?.FullName,
Value = exception.Message,
ThreadId = Thread.CurrentThread.ManagedThreadId,
Mechanism = GetMechanism(exception)
};
if (exception.Data.Count != 0)
{
foreach (var key in exception.Data.Keys)
{
if (key is string keyString)
{
sentryEx.Data[keyString] = exception.Data[key];
}
}
}

In case you or someone else in the community like to help with a PR proposing a change. I'd be happy to review and discuss it further.

Until then:

The work around you're looking for would look like this:

private class NoAggregateExceptionSentryEventProcessor : ISentryEventProcessor
{
    public SentryEvent Process(SentryEvent @event)
    {
        @event.SentryExceptions = @event.SentryExceptions
            .Where(e => e.Type != typeof(AggregateException).FullName).ToList();
        return @event;
    }
}

@bruno-garcia bruno-garcia added Feature New feature or request Sentry labels Sep 3, 2019
@bruno-garcia bruno-garcia added this to To do in 3.0 Sep 17, 2020
@Tyrrrz
Copy link
Contributor

Tyrrrz commented Oct 2, 2020

I think a good solution here would be to automatically flatten aggregate exceptions when captured at system boundaries (i.e. via ASP.NET Core middlware, UnhandledExceptionHandler, or whatever) but not when captured directly using CaptureEvent.

@bruno-garcia
Copy link
Member

bruno-garcia commented Jan 1, 2021

We should tackle this on 3.0 since we're adding arg names it'll affect grouping.

Lets flatten in in the MainExceptionProcessor with an option to opt-out (back to the original behavior).

@bruno-garcia bruno-garcia added this to Backlog in Mobile Platform Team Archived via automation Jan 1, 2021
@bruno-garcia bruno-garcia added this to the 3.0.0 milestone Jan 1, 2021
@Tyrrrz Tyrrrz mentioned this issue Jan 13, 2021
10 tasks
@Tyrrrz
Copy link
Contributor

Tyrrrz commented Jan 15, 2021

@bruno-garcia I'm trying to understand the code in MainExceptionProcessor but it looks like it already handles aggregate exceptions? Or is it doing something else?

if (exception is AggregateException ae)
{
foreach (var inner in ae.InnerExceptions.SelectMany(CreateSentryException))
{
yield return inner;
}
}
else if (exception.InnerException != null)
{
foreach (var inner in CreateSentryException(exception.InnerException))
{
yield return inner;
}
}

@bruno-garcia
Copy link
Member

The problem is that below it returns the exception itself regardless:

var sentryEx = new SentryException
{
Type = exception.GetType()?.FullName,
Module = exception.GetType()?.Assembly?.FullName,
Value = exception.Message,
ThreadId = Thread.CurrentThread.ManagedThreadId,
Mechanism = GetMechanism(exception)
};

@Tyrrrz Tyrrrz moved this from To do to In progress in 3.0 Jan 19, 2021
@Tyrrrz
Copy link
Contributor

Tyrrrz commented Jan 19, 2021

So the goal is to drop AggregateException in this case then?

@bruno-garcia
Copy link
Member

@Tyrrrz I believe this is what most folks expect. Would be nice to have a switch to go back to the old behavior.

@Tyrrrz Tyrrrz removed this from In progress in 3.0 Feb 4, 2021
@Tyrrrz Tyrrrz removed this from the 3.0.0 milestone Feb 4, 2021
@mattjohnsonpint
Copy link
Contributor

@bruno-garcia - let's chat about this. Thanks.

@mattjohnsonpint
Copy link
Contributor

To summarize, we would like to add a client-side option to the SDK that will filter out aggregate exceptions, but would still keep all the inner exceptions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature or request Up-For-Grabs
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants