Support annotated-exception

[parsonsmatt]

I just released a package annotated-exception which can be used to add things like a CallStack (and other info) to an exception transparently.

Integrating this into bugsnag-haskell at the reportException level could be useful to add some of this additional data.

Alternatively, we could expose a function that accepts the [Caster], so end-users could write a conversion function for LocatedException e -> BugsnagException.

[pbrisbin]

I had this exact same thought when I saw your tweet. Happy to look at any PR you create, or I'll try to spend some time on this soon.

Beware, there's a rewrite brewing in #75, so depending on how invasive this would be it might be better to wait. If it's just a small tweak in the report step it's probably OK to do sooner and port into the rewrite branch though.

[parsonsmatt]

Oh, nice!

Well, the "easy" thing is to add a new function that accepts an additionalCasters :: [Caster] argument that gets <>ed onto the defaults. Then I can write a function Caster $ \(AnnotatedException anns (SomeException e)) -> ... which does what I want.

A deeper integration could do more clever things, but that can wait until I've got more ideas to hash out.

[pbrisbin]

Hmm, that use-case implies you are trying to catch an AnnotatedException coming through the system and add the annotations to the Event we're going to report. That's not really a new use-case and is supported today (and in the rewrite) through updateEventFromOriginalException.

f `catch` notifyBugsnagWith myBeforeNotify settings

myBeforeNotify :: BeforeNotify
myBeforeNotify = updateEventFromOriginalException asAnnotatedException

asAnnotatedException :: AnnotatedException ex -> BeforeNotify
asAnnotatedException = -- what you want

What I was picturing was somehow using AnnotatedException internally to get callstacks on any exception being reported to Bugsnag by us without users having to do anything.

But I'm now thinking that's not possible -- a user will still have to add the catch-and-rethrow gate required for any annotations to exist. At that point, they might as well do the updateEventFromOriginalException themselves too.

🤔

[parsonsmatt]

Oh, nice, that makes sense! Thanks 😄

Yeah, we can't retroactively grab a CallStack. If they're doing throwWithCallStack then we're good. But by the time Bugsnag gets to something, it's probably too late to do anything other than read what annotations have been provided.

[pbrisbin]

Closing this since the idea is possible through existing interfaces. Please let me know if that's not the case.

[parsonsmatt]

OK, so I think it might be worthwhile to support it directly, but I am not sure the best way to do so.

I'm building this stuff out now at work, and we've got something like:

class BugsnagAnnotation ann where
   annotationToBeforeNotify :: ann -> BeforeNotify

At reporting sites, we're calling:

reportAnnotated (Annotated anns e) = 
    notifyBugsnagWith (foldr (.) id $ map annotationToBeforeNotify anns) bugsnagSettings (toException e)

I've been made aware that we can actually touch the original exception, so I have another thing that's called in our Bugsnag initialization:

addAnnotationsFromAnnotatedException :: BugsnagEvent -> BugsnagEvent
addAnnotationsFromAnnotatedException =
  updateEventFromOriginalException $ \(AnnotatedException anns exn) event ->
    (annotationsToBugsnagModifier anns event)
      { beException =
          (beException event)
            { beOriginalException =
                Just exn
            }
      }

The trick at play here is that we have a special sub-annotation MercuryAnnotation that we wrap everything in, that carries the instance around.

data MercuryAnnotation where
   MercuryAnnotation :: BugsnagAnnotation ann => ann -> MercuryAnnotation

mercuryCheckpoint :: (MercuryAnnotation ann) => ann -> m a -> m a
mercuryCheckpoint ann = checkpoint (Annotation (MercuryAnnotation ann))

Then our handling code separates our annotations into known and unknown:

import Data.Annotation (tryAnnotations)

separateAnnotations :: [Annotation] -> ([MercuryAnnotation], [Annotation])
separateAnnotations anns = 
  tryAnnotations anns

And then we get our special handling on the MercuryAnnotation while the regular Annotation just get shown and put in a metadata field for other annotations.

So, here's a problem.

If we have done notifyBugsnag bs (toException $ AnnotatedException anns e), then the current exception machinery is gonna fromException against an AnnotatedException and fail. This isn't really what we want here. So that means all of our updateEventFromOriginalException calls are going to stop working if we start reporting annotated exceptions directly.

bugsnag-haskell can support this by doing the Control.Exception.Annotated-style of unwrapping:

fromException' :: Exception e => SomeException -> Maybe e
fromException' exn = asum
    [ fromException exn
    , do
        AnnotatedException _ e <- fromException exn
        pure e
    ]

Then we don't have to worry about this.

---

So, bugsnag-haskell can support it directly, or I can write a library that wraps this stuff and provide bugsnag-annotated-exception that does it for you automagically. Can also probably provide the BugsnagAnnotation type and class through there, and probably even extend it a bit for an Exception-like subtyping hierarchy.

[pbrisbin]

Hmm, when I see,

fromException' :: Exception e => SomeException -> Maybe e
fromException' exn = asum
    [ fromException exn
    , do
        AnnotatedException _ e <- fromException exn
        pure e
    ]

I can't tell why that's substantially better than having a before-notify that calls updateFromOriginalException twice: once at e and once at AnnotatedException e. Can you explain why that's not sufficient?

So that means all of our updateEventFromOriginalException calls are going to stop working if we start reporting annotated exceptions directly

I'm probably misunderstanding, but when I read this ^ I think the solution is pretty easy:

 myBeforeNotify =
   updateEventFromOriginalException asMyException
+   . updateEventFromOriginalException asAnnotatedMyException

?

---

So, bugsnag-haskell can support it directly, or I can write a library that wraps this stuff

I'm not against adding direct support on principle, but I do generally feel that if something can be accomplished on top of a library through the interfaces it already exposes, there's little downside to making a separate package on top. I recently deprecated the monolithic bugsnag-haskell package for separate bugsnag{,-wai,-yesod} packages by just this reasoning. In this world, a bugsnag-annotated-exceptions makes even more sense to me.

That said, if AnnotatedException were to be an implementation detail for some kind of checkpoint feature that automatically updated context, or something like that, that'd be a compelling reason reason to more tightly integrate, IMO.

WDYT?

[parsonsmatt]

Yeah, that's exactly what I've settled on 😄

stripAnnotatedException =
  updateEventFromOriginalException $ \case
    AnnotatedException anns e ->
      addAnnotationsToEvent anns . updateException (setOriginalException e)

We've also got a MonadBugsnag with localBugnsagSettings which we've embedded on our local checkpoint:

myCheckpoint :: (ExceptionAnnotation ann, MonadUnliftIO m, MonadBugsnag m) => ann -> m a -> m a
myCheckpoint ann =
    localBugsnagSettings (\bs -> bs { bsBeforeNotify = annToBeforeNotify ann . bsBeforeNotify bs })
      . checkpoint (Annotation (OurAnnotation ann))

Which ensures that exceptions that are reported inside the action get the annotation + exceptions that escape the action are annotated.

[pbrisbin]

Your and my (at Freckle) usages of this library are converging.

We do MonadReader env m, HasBugsnagSettings env and use local to modify the settings for a block, almost identically as you are there.

See #65 ;)

[pbrisbin]

I'm re-opening this as Freckle has started using annotated-exceptions and run into a few edge-cases with Bugsnag.

I was incorrect originally that this could be entirely handled from the outside, as at least bugsnag-yesod needed an update already (#81). I suspect a tighter integration between the two libraries would be valuable.

/cc @chris-martin .

[chris-martin]

@pbrisbin - I believe I've broken the uses of updateEventFromOriginalException. I had it in mind to apply the fix in freckle-app here, but could push the fix further up if you'd like. (Perhaps at a later time.)

@parsonsmatt - I just wrote a little utility for this that might be nice for annotated-exception to provide:

import Control.Applicative ((<|>))
import Control.Exception.Annotated
import Data.Function (($))
import Data.Functor ((<$>))
import Data.Maybe (Maybe (..))

-- | Like 'fromException', but matches both @e@ and @'AnnotatedException' e@
--
-- If the exception type is @e@, the value returned is an 'AnnotatedException'
-- with no annotations.
fromExceptionAnnotated
  :: forall e. Exception e => SomeException -> Maybe (AnnotatedException e)
fromExceptionAnnotated e = annotated <|> notAnnotated
 where
  annotated = fromException @(AnnotatedException e) $ toException e
  notAnnotated =
    (\e' -> AnnotatedException {exception = e', annotations = []})
      <$> fromException @e (toException e)

(edit) Oh nevermind I just noticed you already wrote just about the same thing above!

[parsonsmatt]

Part of the Magic of AnnotatedException is that what you just wrote is equivalent to fromException, if I read it right - "Catch an e or an AnnotatedException e, and promote the e with the empty set of annotations."

λ> import Control.Exception as Exception
λ> :set -XDeriveAnyClass
λ> data Exn = Exn deriving (Show, Exception)
λ> import Control.Exception.Annotated
λ> fromException (toException Exn) :: Maybe (AnnotatedException Exn)
Just (AnnotatedException {annotations = [], exception = Exn})

The other half of the magic is the definitions of catch etc all also catch AnnotatedException e in addition to e. This combination allows the annotated-exception package to work nicely for the usual catching and throwing of exceptions when you don't care about annotations, but give you the opportunity to inspect the annotations when you do.

The main problem with the library is that Control.Exception.catch does not know about this. So if you do:

Control.Exception.try @MyException do
    Control.Exception.Annotated.throw MyException

then the MyException will escape, because Annotated.throw will wrap in AnnotatedException. However, you can do:

Control.Exception.try @(AnnotatedException MyException) do
    Control.Exception.throwIO MyException

Because the Exception instance of AnnotatedException e can work catch both e and AnnotatedException e.

[pbrisbin]

Looking over the work in freckle-app and just want to capture some notes here.

I think there are two things happening over there that we could provide automatically from here instead,

• Calling each updateFromOriginalException @e on @(AnnotatedException e) too

  I think our updateFromOriginalException, when called @e could just internally operate on @e and @(AnnotatedException e), then freckle-app would need no changes and it would Just Work. If I understand Matt's comment, all that requires is that we use AnnotatedException.fromException in that function?

• Looking for an AnnotatedException and using its callstack

  Doing that from this library could happen in one of three ways: globalBeforeNotify, bugsnagExceptionFromSomeException, or as a new default for settings_beforeNotify.

  Using bugsnagExceptionFromSomeException has some appeal because it's where we deal with automatic callstack handling for error and throwString -- however, I kind of think that only happens there because of historical accident (that code preceded our current BeforeNotify setup). Looking at it now, it feels like any automatic modifications to events should just be consistently BeforeNotifys composed in with any that are user-provided. So, ideally, I think I would move the error/throwString stuff into wherever we decide to put this one.

  Between globalBeforeNotify and settings_beforeNotify I'm open to opinions. My memory is fuzzy, but I think the idea was that globalBeforeNotify is for things that shouldn't ever be opt-out-able, and a settings_beforeNotify would be for things that are good by default, but not required all the time. I don't know if we need that distinction, or in which camp this behavior should go.

[chris-martin]

Part of the Magic of AnnotatedException is that what you just wrote is equivalent to fromException

Ahh okay, I didn't notice that this happens in the instance, cool. I can simplify a little then.

[chris-martin]

If I understand Matt's comment, all that requires is that we use AnnotatedException.fromException in that function?

He's saying keep using the fromException class method, but that if you want to cast SomeException to either e or to AnnotatedException e, all you have to do is fromException @(AnnotatedException e), because the Exception instance for AnnotatedException will do the work of promoting e to AnnotatedException e as necessary.

[pbrisbin]

all you have to do is fromException @(AnnotatedException e)...

Ah yes. I think I get it now.

[pbrisbin]

@chris-martin I know you've done some work here, is this Issue well enough addressed to be closed? Are there any gaps left in using Bugsnag and annotated-exception together?