Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add katipSetContext that doesn't leak under recursion #153

Open
jappeace opened this issue Aug 26, 2024 · 2 comments · May be fixed by #154
Open

Add katipSetContext that doesn't leak under recursion #153

jappeace opened this issue Aug 26, 2024 · 2 comments · May be fixed by #154

Comments

@jappeace
Copy link

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.

@jappeace jappeace changed the title Add katipSetContext Add katipSetContext that doesn't leak under recursion Aug 26, 2024
@Soostone Soostone deleted a comment Aug 26, 2024
@Soostone Soostone deleted a comment Aug 26, 2024
@MichaelXavier
Copy link
Collaborator

@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
Copy link
Author

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).

@jappeace jappeace linked a pull request Aug 28, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants