pbrisbin / bugsnag-haskell

Bugsnag error reporter for Haskell
10 stars 7 forks source link

BeforeNotify based on OriginalException #26

Closed pbrisbin closed 6 years ago

pbrisbin commented 6 years ago

The library can't be aware of all possible Exception types (not without dependencies on all libraries that provide them). So there needs to be support for the end-user to specify how to create an information-rich BugsnagException given local-context Exception types (e.g. SqlError).

beOriginalException (as a Maybe SomeException) has been added, and updateException is available, so this is at least possible, but the code to do this over a few local exception types becomes cluttered quickly.

The following is how we're doing it in our app today, which includes a function that would, at least superficially, make sense to be upstreamed:

updateExceptionFromException
  :: Exception e
  => (e -> BugsnagException -> BugsnagException)
  -> BeforeNotify
updateExceptionFromException f = updateException $ \ex ->
  ($ ex) . maybe id f $ fromException =<< beOriginalException ex

asSqlError :: SqlError -> BugsnagException -> BugsnagException
asSqlError SqlError{..} ex = ex
  { beErrorClass = decodeUtf8 $ "SqlError-" <> sqlState
  , beMessage = Just $ decodeUtf8 sqlErrorMsg
  }

asHttpException :: HttpException -> BugsnagException -> BugsnagException
asHttpException e@(HttpExceptionRequest _ _) ex = ex
  { beErrorClass = "HttpExceptionRequest"
  , beMessage = Just $ -- ...
  }
asHttpException e@(InvalidUrlException _ _) ex = ex
  { beErrorClass = "InvalidUrlException"
  , beMessage = Just $ -- ...
  }

The reason I'm not adding it in the library yet is (naming, and) I'm struggling with GroupingHash. Ideally, this would be the place to also (optionally) define a grouping hash, since you've already got a handle on the original exception post-fromException.

pbrisbin commented 6 years ago

Some answers after more consideration:

  • Should the user function return (BugsnagException, Maybe Text)?
  • Should there be a beGroupingHash on BugsnagException?
  • If the above, how do we deal with the name-clash on Event?

No's all around!

For better or worse, Bugsnag considers groupingHash a property of the (possibly multi-Exception) Event. This is a constant annoyance, but I think we should respect it.

In general, setting grouping hash and updating the Exception(s) based on the original exception(s) should be done by composing two BeforeNotifys with (.).

We could standardize on how to get Exception data out of the Event in a way useful to building these two (separately or together), though. I'm picturing something like this in user-land:

myBeforeNotify event =
  let mException = fromException =<< beOriginalException (NE.head $ beExceptions event)
  in maybe id (\ex -> setGroupingHash (f ex) . updateException (g ex)) mException

f :: SqlError -> Text

g :: SqlError -> BugsnagException -> BugsnagException

I think, as the library, we should aim to provide the fromException =<< beOriginalException (NE.head $ beExceptions event) part, since it gets into the murkiness of multi-Exception Events.

See here.

  • Should we still allow a grouping function on Settings?

We've already moved away from this.

  • Should we allow anything related to BeforeNotify from Settings?

Yes, but we will have only bsBeforeNotify, with all the other stuff expressed as BeforeNotify functions to be composed with (.)

pbrisbin commented 6 years ago

This is on its way to resolution in #36.