Soostone / katip

A structured logging framework for Haskell
205 stars 70 forks source link

Add `katipSetContext` that doesn't leak under recursion #153

Open jappeace opened 3 weeks ago

jappeace commented 3 weeks ago

The current implementation of katipAddContext leaks under recursion, this caused me a 5-ish day chase to finding the source of the leak (and a lot of stress).

As far as I can tell, it's not possible to change katipAddContext such that it doesn't leak without introducing a breaking change (which would be causing a lot of breakage). But I think it's possible to add an alternative API that doesn't leak, at least. For example:

-- | like `katipAddContext` but doesn't memory leak under recursion,
--   as long as log messages are emitted.
katipSetContext ::
  ( ToJSON context,
    KatipContext m
  ) =>
  Text -> -- ^ key
  context -> -- ^ value
  m a ->
  m a

I would realize this by changing the log context a bit:

newtype LogContexts = LogContexts { logContextsSetContexts :: (Map Text Value), logContextsAddedContexts  ::  (Seq AnyLogContext) } deriving (Monoid, Semigroup)

Then in the consumers of the log contexts, the setContext get prepended to the Seq AnyLogContext by translating each key value pair in logContextsSetContexts into a SimpleLogPayload.

The reason this doesn't leak under recursion is that the same key is overridden in the logContextsSetContexts field. And every time a log message get emitted, the content of contexts fields get forced due to "natural" serialization process. (so yes this may still leak if you don't ever emit a log message, but that's a degenerate case).

I think a disadvantage of this new api is that you lose the verbosity level mechanism, but I think this can be added back as well by introducing another settings field for example, just for this api. That addition maybe a bit more powerful as well because no longer is verbosity attached to a type class but can be modified at runtime.

Would this be appreciated as a pull request? I'm asking first before doing the implementation because I suspect it'd be a fair bit of work.

MichaelXavier commented 3 weeks ago

@jappeace I think it's probably a good idea. I think I've been bitten by this as well and came to a similar conclusion that we've painted ourselves into a corner. If you can figure out an alternative way to do it that has an equivalent verbosity mechanism I think that would be great. Would it make sense to deprecate the old version at that point?

jappeace commented 3 weeks ago

yeah, using katipAddContext is less safe, although all the warnings generated in commercial projects maybe a bit annoying. I'd want to be able to disable those specific deprecation warnings (if at all possible).