search

pbrisbin / bugsnag-haskell

Bugsnag error reporter for Haskell
10 stars 7 forks source link

Support `annotated-exception` #76

Open parsonsmatt opened 2 years ago

parsonsmatt commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

:thinking:

parsonsmatt commented 2 years ago

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 commented 2 years ago

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

parsonsmatt commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 9 months ago

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 commented 9 months ago

@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 commented 9 months ago

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 commented 9 months ago

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,

chris-martin commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

Ah yes. I think I get it now.

pbrisbin commented 9 months ago

@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?