Add interface for libraries to register exception enrichment

[iand675]

This is a stab at supporting improved exception reporting in a fashion that can mostly reduce the work needed to wire things up. This is to support things like annotated-exceptions better. Feedback is appreciated.

[parsonsmatt]

I think a function in the record will work better than the class magic. The class stuff works for terminal types, but not for wrappers.

[parsonsmatt]

hrm, I found the name shadowing a bit confusing here. Would probably prefer to inline some of that:

Suggested change

	catchesHandler :: ExceptionEnricher -> SomeException -> [(Text, Attribute)]
	catchesHandler handler e = tryHandler handler
	where
	tryHandler (ExceptionEnricher handler _)
	= case fromException e of
	Just e' -> handler e'
	Nothing -> []
	catchesHandler :: ExceptionEnricher -> SomeException -> [(Text, Attribute)]
	catchesHandler (ExceptionEnricher handler _) e =
	case fromException e of
	Just e' -> handler e'
	Nothing -> []

[parsonsmatt]

Hmm. Thinking about how I'd write this for annotated-exception.

instance (HasExceptionEnricher e) => HasExceptionEnricher (AnnotatedException e) where
  exceptionEnricher _ = ExceptionEnricher
    { exceptionEnricherAdditionalFields = \(AnnotatedException anns er) ->
        exceptionEnricherAdditionalFields (exceptionEnricher (pure er)) e
        <> makeAdditionalFields anns
    , exceptionEnricherName = "AnnotatedException " <> show (typeRep @e)
    }

makeAdditionalFields is pretty limited- it can only access the Annotation, which can only really be shown into a String. So the only real impl would be something like map (\(Annotation ann) -> (Text.pack (show $ typeRep (pure ann)), show ann)) annotations.

I would expect that apps would want to customize this directly, for their own app. So having a type class may not be the best way to do that.

discoverInstances also only works "transitively" in the same package, otherwise you do need the type itself imported in a module (and used!) or it won't discover the instance.

[iand675]

Yeah, so that's why I took the approach of providing tracerProviderOptionsExceptionEnrichers.

There are two things I'm trying to solve here:

1. This is the hs-opentelemetry-api package, which is intended to be usable by libraries (such as persistent) in such a fashion that they have no effect if you aren't using hs-opentelemetry-sdk in your application. I'd like to provide a mechanism for domain-specific exceptions to provide additional attributes to the span, which is why the ExceptionEnricher + type class + discoverExceptionEnricher mechanism exists. I know it's a bit brittle in that you have to import the right things in the right module, but I'm not sure how else to provide any amount of automatic hookup for library-supplied enrichers.
2. I don't want the type class instance auto-discovery bits to be required to use the library, which is why any discovered enrichers ultimately have to be set in the tracerProviderOptionsExceptionEnrichers :: [ExceptionEnricher] record field. In the case of wanting to do more than rendering annotations to a string, this would presumably be where you'd be able to append a domain-specific ExceptionEnricher that can leverage Typeable to do more useful annotations.

[parsonsmatt]

Yeah, I think we do need the function parameter to have a [Enricher] parameter - otherwise, we can only deal with the things that are known that that point in time. I ran into this same problem when considering a plain function approach - you can recurse into wrappers just fine, and you can compose SomeException -> [(Text, Attribute)] fine with <>, but you can't make a wrapper that can see through other wrappers defined later, unless you accept the final :: [Enricher] as a late bound function argument.

Playing with it this afternoon and I got:

newtype Enricher = Enricher ([Enricher] -> SomeException -> [(Text, Attribute)])
    deriving newtype (Semigroup, Monoid)

enrich :: SomeException -> [Enricher] -> [(Text, Attribute)]
enrich exception enrichers = 
    foldMap (\(Enricher k) -> k enrichers exception)

simpleEnricher :: Exception e => (e -> [(Text, Attribute)]) -> Enricher
simpleEnricher k =
    Enricher \_ someException -> 
        foldMap k (fromException someException)

nestedEnricher :: Exception e => ([Enricher] -> e -> [(Text, Attribute)]) -> Enricher
nestedEnricher k =
    Enricher \final exception -> 
        foldMap (k final) (fromException exception) 

Each library can define an Enricher for a specific exception, then mconcat them all into a single Enricher.

Figuring out the exact right implementation for this is subtle, though. cast and fromException need to be chosen with care, or you may get into infinite loops. So I think it's important to figure out some basic tests (SomeException, SomeAsyncException, SyncExceptionWrapper, AsyncExceptionWrapper, AnnotatedException, etc) and ensure we're getting the right behavior.

Figuring out the plugin-UX side of things can happen at a later step. IMO it's fine to say "You need to import the Enricher for a given library and plug it in to the settings." Very few libraries work in any other way.

[parsonsmatt]

I feel like what I usually see is a configurable function, like data Settings = Settings { settingsOnException :: SomeException -> IO () }.

For this pattern, we can do:

data TracerProvider = TracerProvider 
  { ...
  , tracerProviderExceptionToAttributes :: SomeException -> [(Text, Attr)]
  }

Which removes the need for the ExceptionEnricher complexity.

But, is this equivalent?

Let's write one that pulls annotations off an annotated exception.

tracerProvider = TracerProvicer {..}
  where
    tracerProviderExceptionToAttributes somE@(SomeException inner) 
      -- because fromException always succeeds, don't want to infinite recursion
      | Just (AnnotatedException anns err) <- cast inner 
      = convertAnnotations anns <> tracerProvicerExceptionToAttributes (toException err)
      | Just (SomeAsyncException inner) <- fromException somE
      = ("Async Exception", _) : tracerProviderExceptionToAttributes (SomeException inner)
      -- use `SomeException` instead of `toException` because `toException` will rewrap in `SomeAsyncException`

Hmm, already getting pretty complex...

But I think it's probably better than the ExceptionEnricher class, which doesn't locally know what the other things are doing.

instance HasExceptionEnricher SomeAsyncException where
    exceptionEnricher _ = ExceptionEnricher 
      do \(SomeAsyncException e) -> 
           -- well, we can't really do anything interesting here
      do "SomeAsyncException"

AnnotatedException is able to summon the canonical ExceptionEnricher for a type, since it has the type parameter. But no other exception wrapper would work.

[iand675]

I suppose we could work around the wrapper issue by passing in known exception enrichers to the function:

data ExceptionEnricher = forall e. Exception e => ExceptionEnricher
  { exceptionEnricherAdditionalFields :: [ExceptionEnricher] {- ^ all known enrichers -} -> e -> [(Text, Attribute)]
  , exceptionEnricherName :: Text
  }

That would allow us in the case of wrappers to feed the inner exception through the same process.