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

Handle AggregateErrors #5469

Closed
Tracked by #37716
mortargrind opened this issue Jul 26, 2022 · 6 comments · Fixed by #8463
Closed
Tracked by #37716

Handle AggregateErrors #5469

mortargrind opened this issue Jul 26, 2022 · 6 comments · Fixed by #8463
Assignees

Comments

@mortargrind
Copy link

mortargrind commented Jul 26, 2022

Problem Statement

Hi,

I think both on the JS SDK side and Sentry side things like AggregateError needs to be handled a bit more specifically in a built-in way. Basically AggregateError is a type of error that groups other kinds of errors together and can have an errors property on it, which itself is an array of errors.

On the SDK side: they should be sent in the report request and in the Sentry UI they should be shown in a given error detail page. I think these can be handled in the same or similar way to linked errors.

Also in Python we have ExceptionGroup, in .NET we have AggregateException, and for the languages that does not have this kind of thing directly; something similar can be implemented in the user-land.

Solution Brainstorm

Most probably a new integration like AggregateErrors would do the job on the SDKs sides via both handling the language specific built-in ones and/or allowing custom implementations (so people that dont care about them dont need to enable/add it to their SDK instance).

@AbhiPrasad
Copy link
Member

Hey, thanks for writing in. I agree this would be a cool integration to include, backlogging for now, but PRs are welcome for those who would like to help out!

It would probably work similar to the LinkedErrors integration: https://github.com/getsentry/sentry-javascript/blob/master/packages/browser/src/integrations/linkederrors.ts

@mortargrind
Copy link
Author

I directly opened a feature request instead of a pull request because I think there are some product & design decisions to be made for the Sentry itself first. Because having only a functionality on the JS SDK means nothing by itself without the intention and clear expectations on the UI side since this requires a new way of displaying the aggregated errors, different than but compatible with the current linked errors: otherwise you wouldn't be able to differentiate linked and aggregated errors in the event detail page.

As for the implementation ideas:

It would work similar to LinkedErrors, but not exactly the same. I think it needs to work in harmony with LinkedErrors. See a complex but possible scenario below:

Legend:

  • Error1 => Error2 means Error1 caused by Error2 (handled by the existing LinkedErrors)
  • Error1 [Error2, Error3] means Error1 aggregates Error2 and Error3 (would be handled by the hypothetical AggregatedErrors)

Sample error chain:

HighLevelError => AggregateError [LowLevelError1, LowLevelError2 => LowerLevelError1, LowLevelError3] => LowestLevelError

If Sentry team is down with these suggestions & possibilities there in and has an idea about what kind of updates will/would need to happen on the UI side then I can try to create PoC PR and you can use it to implement & test the UI? I can most probably even open a PR for the Sentry UI itself but still would need to know what kind of a UI is needed first.

@AbhiPrasad
Copy link
Member

Great point. We don't have support for this at the moment, since our event schema expects a list of Exceptions, which means we can only represent chained exceptions. A POC for this would be more involved, since we would have to make event ingestion changes, and think about how we index this also (I assume you would like to query for these exceptions 😄).

https://develop.sentry.dev/sdk/event-payloads/exception

I'm going to circle this around internally and see what folks think! Thank you for the quick response :)

@AbhiPrasad
Copy link
Member

Hey as an update - we are looking to try to solve this across all the SDKs internally. We’ll update this issue when we have more information related to that!

@AbhiPrasad
Copy link
Member

We're tracking this at Sentry: getsentry/sentry#37716 as it requires some product changes. Won't be prioritized anytime soon, but will be in the future!

@mattjohnsonpint
Copy link
Contributor

Hi. Just wanted to ask anyone who's interested in this topic to chime in on the other issue, starting here:
getsentry/sentry#37716 (comment). Thanks.

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

Successfully merging a pull request may close this issue.

4 participants