Remove MonadIO constraint from Katip typeclass

[eckyputrady]

Currently, Katip typeclass requires the instance to be an instance of MonadIO.

class MonadIO m => Katip m where

The problem with this is that if I am to use KatipContext constraint in domain/core functions, it makes the functions also have IO capability, which is bad.

Example:

class Network m where
  read :: m Text

class DB m where
  save :: Text -> m ()

downloadAndSave :: (Network m, DB m, KatipContext m) => m ()
downloadAndSave = do
  file <- read
  save file
  $(logTM) InfoS "Saved successfully"

Katip logging functionalities are general purpose enough to be used in this context. It's unfortunate to have it depends on MonadIO.

My understanding is the IO is used to get current thread Id & time here:

logItem
    :: (A.Applicative m, LogItem a, Katip m)
    => a
    -> Namespace
    -> Maybe Loc
    -> Severity
    -> LogStr
    -> m ()
logItem a ns loc sev msg = do
    LogEnv{..} <- getLogEnv
    liftIO $ do
      item <- Item
        <$> pure _logEnvApp
        <*> pure _logEnvEnv
        <*> pure sev
        <*> (mkThreadIdText <$> myThreadId)
        <*> pure _logEnvHost
        <*> pure _logEnvPid
        <*> pure a
        <*> pure msg
        <*> _logEnvTimer
        <*> pure (_logEnvApp <> ns)
        <*> pure loc
      FT.forM_ (M.elems _logEnvScribes) $ \ ScribeHandle {..} -> atomically (tryWriteTBQueue shChan (NewItem item))

How about moving those getters behind a typeclass so that liftIO is unnecessary here?

[MichaelXavier]

Do you mean moving them under a typeclass for the monad m? Something like GetThreadId m, GetTimer m? What about the atomic operation in STM? I don't see how we get away with not having an IO capability there. You could take out the atomically and make the whole thing run in STM but that would make writes across all scribes happen in one transaction when they are actually independent.

[eckyputrady]

Do you mean moving them under a typeclass for the monad m? Something like GetThreadId m, GetTimer m?

Yeah.

What about the atomic operation in STM?

I missed that one. I thought the IO is only used for getting thread and getting the time only.

I don't have a clear solution at the moment to discuss further on how to resolve this issue (if it's indeed worthy to resolve).

[MichaelXavier]

I could maybe see moving to a typeclass so you can provide your own timer, but I'm dubious of this sort of change as it really feels like painting yourself into a corner. For instance, if we didn't have the STM constraint on katip at first release and went through these efforts to stamp out the one use of IO, then realized STM would be a big help in the design, we'd have to claw back that ability, or be even more obtuse and dependency inject the queueing mechanism and timer.

For your use case though it sounds like you're trying to do some logging inside of a pure core function. Presumably, you would want to accumulate log messages and then after your core logic ran, commit those messages to the log. You could implement this in your own code with something like Writer (Seq SomeLogStructure). I'd probably recommend writer-cps-transformers as they are constant space and from what I heard will end up in transformers proper at some point. Then, after your pure logic is run, gather the log values and commit them all at once.

[YPares]

@MichaelXavier Hi, I comment on that old issue just to add a note:
I'd too be in favour of removing the MonadIO constraint. I know we could achieve the same via a Writer, but we'd loose the namespacing and context (we'd have to handle it in a separate manner).

[puffnfresh]

I agree with removing the MonadIO constraint. I really didn't expect it. I had some code with:

check :: (Katip m, MonadIO m) => m ()

Which used a bunch of liftIO in it. I started removing the liftIO calls and then removed the constraint. I then realised that I missed some liftIO calls. How was it still compiling??? Oh wow Katip extends MonadIO??

[MichaelXavier]

So one thing I can see doing is moving MonadIO out of the superclass of Katip because the class itself does not actually extend MonadIO. There are operations that use Katip but also need MonadIO like things that read the clock or use STM. But for those we can have the constraints (Katip m, MonadIO m).

I think this would probably be a breaking version number change and require some churn on the user's end because they'll now have to re-add MonadIO in a bunch of places where Katip already implied it. I guess the question is will correcting this questionable design decision now actually make anyone happier? Almost any time where you currently use Katip is to log and that requires MonadIO so its not like this opens the door to using katip in non-IO contexts.

[puffnfresh]

@MichaelXavier I was making an instance which did nothing so I could run the code in tests, ignoring logs.

[treeowl]

I think it makes a lot of sense to remove the superclass constraint, even if a bunch of functions and instances need it. Just to control effects.