You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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::
( ToJSONcontext,
KatipContextm
) =>Text->--^ keycontext->--^ valuema->ma
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 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 text was updated successfully, but these errors were encountered:
jappeace
changed the title
Add katipSetContext
Add katipSetContext that doesn't leak under recursion
Aug 26, 2024
@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?
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).
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:
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 AnyLogContext
by translating each key value pair inlogContextsSetContexts
into aSimpleLogPayload
.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 text was updated successfully, but these errors were encountered: