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.
The current implementation of
katipAddContextleaks 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
katipAddContextsuch 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:
I would realize this by changing the log context a bit:
Then in the consumers of the log contexts, the setContext get prepended to the
Seq AnyLogContextby translating each key value pair inlogContextsSetContextsinto aSimpleLogPayload.The reason this doesn't leak under recursion is that the same key is overridden in the
logContextsSetContextsfield. 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.