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

Add support for ExceptionGroup #1788

Closed
rodolfoBee opened this issue Dec 16, 2022 · 10 comments · Fixed by #2025
Closed

Add support for ExceptionGroup #1788

rodolfoBee opened this issue Dec 16, 2022 · 10 comments · Fixed by #2025
Assignees
Labels
Enhancement New feature or request

Comments

@rodolfoBee
Copy link
Member

rodolfoBee commented Dec 16, 2022

Problem Statement

ExceptionGroup was introduced in Python 3.11:
https://peps.python.org/pep-0664/#features-for-3-11
https://peps.python.org/pep-0654/

Our current SDK v1.12.0 does not support capturing all exceptions separately with its own stack traces so they can be shown in Sentry UI.

This issue was originally raised here in a general manner to expand the feature to other SDKs: getsentry/sentry#37716

@rodolfoBee rodolfoBee added the Enhancement New feature or request label Dec 16, 2022
@github-actions
Copy link

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@antonpirker
Copy link
Member

In Sentry we can visualize chained exceptions in the Issues UI. ExceptionGroups could be visualized similarly for the start.

Did some poking around in the Python SDK code:

We have walk_exception_chain() but this is only used:

  • in executing and pure_eval integrations (not enabled by default)
  • and django and spark integrations.

So chained exceptions are not handled by default for the fast majority of users.

@antonpirker
Copy link
Member

The goal of a first step to support ExceptionGroups:

  • Chained exceptions should probably work out of the box without integrations.
  • If exception group is captured: just pick first exceptions to be showing up as issue in Sentry.
  • if this picking is happening, add the full group visualization from Python somewhere to the event sent to Sentry (probably best in a context)
  • Add new option keep_exception_groups by default False.
  • If keep_exception_groups is set to True do not pick first exception, but send group as-is.
  • make sure that chaining of this exception still works inside the group.

@mattjohnsonpint
Copy link
Contributor

This makes sense, and aligns mostly with my thoughts on getsentry/sentry#37716 (comment). Thanks!

@mattjohnsonpint
Copy link
Contributor

We do still need to decide what we mean by "just pick first exception".

Currently (without exception groups) the order of precedence is:

  1. __cause__ unless it is None
  2. __context__ unless it is None
  3. (nothing to chain)

My gut instinct says that we should insert this in the middle, as in:

  1. __cause__ unless it is None
  2. exceptions[0] unless it's empty (or not an exception group)
  3. __context__ unless it is None
  4. (nothing to chain)

@mattjohnsonpint
Copy link
Contributor

Working on the final spec, but just FYI, I think we will put the full exception group on extras instead of contexts due to size constraints.

@mattjohnsonpint
Copy link
Contributor

mattjohnsonpint commented Apr 1, 2023

FYI, the plan has changed considerably. Please read the RFC.

As it relates to Python:

  • A keep_exception_groups option should not be added.
  • The question of precedence I raised above is no longer an issue. The new plan is to send everything, including __context__, __cause__, and exceptions. Send all of them (unless None or empty).
  • The exceptions list will be used to send each exception, as it does today.
  • The tree structure will be represented by exception_id and parent_id values assigned to each exception's mechanism attribute.
  • The source of the exception will be delineated by mechanism.source.
  • Each exception of type ExceptionGroup will have mechanism.is_exception_group:true.

Please see the detailed Python example in the RFC .

This is ultimately a modification to how chained exceptions are represented. As to whether the chained exceptions integration is enabled by default or not for the Python SDK, I'm not sure. Do keep in mind that switching it on by default might create a lot of noise for those that are used to it being off. (@mitsuhiko might have an opinion on this.)

@mattjohnsonpint
Copy link
Contributor

I just ran a small bit of test code with the current Python SDK and it seems that chained exceptions in __context__ or __cause__ (but not both) are already coming through. So it does appear to be enabled by default already.

@antonpirker
Copy link
Member

Hey @mattjohnsonpint !
You are right. I found the place where this is done. Thanks for the pointer. (now I also know where to implement exception groups :-) )

@mattjohnsonpint
Copy link
Contributor

This feature is now available for SaaS (sentry.io) customers. It will also be included in the next Sentry Self-Hosted release (23.6.0, est. June 2023).

On the client side, it requires a supporting SDK. For Python, that's Sentry Python 1.24.0.

A similar update was made for .NET, and one is coming soon for JavaScript as well.

Please give it a try and let us know what you think. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants