core-libraries-committee icon indicating copy to clipboard operation
core-libraries-committee copied to clipboard

Exception backtrace proposal: Part 5: Annotation collection

Open bgamari opened this issue 1 year ago • 1 comments

Tracking ticket: https://github.com/haskell/core-libraries-committee/issues/164

In #202 @parsonsmatt raised an useful design goal for the exception annotation mechanism described in https://github.com/haskell/core-libraries-committee/issues/200: the user should be able to easily collect all annotations attached for an exception and any nested exceptions it may contain. This ensures that the exception annotation mechanism can cover all of the user-cases allowed by annotation-exceptions.

Motivation

In #202 we introduced a WhileHandling ExceptionAnnotation to capture causal chains of exceptions:

newtype WhileHandling = WhileHandling SomeException

However, this introduces a problem: How does one reliably collect all ExceptionAnnotations from a SomeException given that some may be buried in a WhileHandling annotation?

Moreover, this issue is not specific to WhileHandling: it is common for libraries is to catch exceptions and rethrow them nested within their own exception type. See, for instance, http-client's HttpException type (specifically, the InternalException constructor of HttpExceptionContent).

Being able to reliably identify all annotations (e.g. call-stacks) associated with a given exception is important for observability.

Proposal

To address this goal, I propose the following additions to the Exception and ExceptionAnnotation classes:

class Exception a where
    ...
    -- | Extract any exceptions nested within an exception.
    nestedExceptions :: a -> [SomeException]
    nestedExceptions = []

class ExceptionAnnotation a where
    ...
    -- | Extract any exceptions nested within an annotation.
    exceptionAnnotationNestedExceptions :: a -> [SomeException]
    exceptionAnnotationNestedExceptions _ = []

The default definitions here are a bit of a compromise: while they ensure that the additions do not constitute a breaking change, they also mean that nestedExceptions is liable to be incorrect for existing exception types.

This allows us to provide the following from Control.Exception.Annotation:

flattenAnnotations :: Exception a => a -> [SomeExceptionAnnotation]
flattenAnnotations e = concat
    [ exceptionAnnotations e
    , concatMap flattenAnnotations (concatMap (\(SomeExceptionAnnotation ann) -> exceptionAnnotationNestedExceptions ann) (exceptionAnnotations e))
    , concatMap flattenAnnotations (nestedExceptions e)
    ]

bgamari avatar Feb 01 '24 16:02 bgamari

I think that this is useful right now, even without any exception annotation work being ready and merged. This would be directly useful in at least two contexts - hs-opentelemetry #43 and bugsnag-haskell #76, both of which are about "digging through" the exception hierarchy to see what's underlying.

I do wonder if a Tree is a more appropriate data structure. A user could still toList it for a totally flat representation. But the tree structure would be useful for understanding the nested nature of the type.

parsonsmatt avatar Feb 06 '24 16:02 parsonsmatt

@parsonsmatt we cannot use Tree from containers in base, unfortunately. In principle one could move its definition into base and make containers to reuse it, but it's going to be a proposal of its own and a controversial one.

What's the plan here? If it's useful without #202, I guess we can go for a vote.

Bodigrim avatar May 07 '24 21:05 Bodigrim

@bgamari what's the plan here? Shall we vote?

Bodigrim avatar May 12 '24 10:05 Bodigrim

@bgamari unless there is a feedback from your side on the question above, I'll close as dormant in two weeks.

Bodigrim avatar May 20 '24 19:05 Bodigrim

I would be fine with moving ahead to a vote. To my mind the proposal is in an acceptable state.

bgamari avatar May 23 '24 15:05 bgamari

Dear CLC members, let's vote on the proposal to extend class Exception and class ExceptionAnnotation with members to extract nested exceptions. This is not a breaking change because new members have default definitions.

class Exception a where
    ...
    -- | Extract any exceptions nested within an exception.
    nestedExceptions :: a -> [SomeException]
    nestedExceptions = []

class ExceptionAnnotation a where
    ...
    -- | Extract any exceptions nested within an annotation.
    exceptionAnnotationNestedExceptions :: a -> [SomeException]
    exceptionAnnotationNestedExceptions _ = []

@tomjaguarpaw @hasufell @mixphix @parsonsmatt @angerman @velveteer


+1. I don't know much about the area, but I trust on @parsonsmatt's opinion and motivation above in https://github.com/haskell/core-libraries-committee/issues/250#issuecomment-1930345876.

Bodigrim avatar May 23 '24 22:05 Bodigrim

+1 from me. The fact that @parsonsmatt and @bgamari both consider this useful, makes me believe this is the right direction. While class changes always have me a bit uneasy, as this is not breaking anything right now. I've no objections.

angerman avatar Jun 06 '24 04:06 angerman

+1

mixphix avatar Jun 06 '24 08:06 mixphix

I abstain


I don't feel like I have a deep enough understanding of the direction of travel here to vote in favour but likewise I don't feel like I should vote against the wishes of the people who do know what they're doing. I am very uneasy about fiddling around with SomeException because, if I understand correctly, it's one element of our exception machinery that GHC is aware of and uses. I feel like everything we're doing with exceptions here ought to be done in some way that doesn't fiddle with SomeException itself, but I understand there are good reasons for doing that (especially making these new lovely exception features automatically available to existing code) and I don't have the knowledge or time to determine what any alternative should be. In conclusion, I don't think I can do anything other than abstain.

tomjaguarpaw avatar Jun 07 '24 13:06 tomjaguarpaw

+1

velveteer avatar Jun 07 '24 18:06 velveteer

Thanks all, that's enough votes to approve.

Bodigrim avatar Jun 08 '24 08:06 Bodigrim