serokell / log-warper

Logging library to provide more convenient, extremely configurable but simple monadic interface with pretty output
MIT License
19 stars 11 forks source link

Move configuration from global mutable variable to Reader context or at least pass explicitly #47

Closed chshersh closed 6 years ago

chshersh commented 6 years ago

Currently all IO configuration is done using global MVar with unsafePerformIO. This is not really good :(

chshersh commented 6 years ago

Here I will describe plan to migrate to newer interface.

What we have now:

We can ask LoggerName

newtype LoggerNameBox m a = LoggerNameBox { loggerNameBoxEntry :: ReaderT LoggerName m a }

class HasLoggerName m where
    askLoggerName    :: m LoggerName
    modifyLoggerName :: (LoggerName -> LoggerName) -> m a -> m a

We can dispatch messages

class Monad m => CanLog m where
    dispatchMessage :: LoggerName -> Severity -> Text -> m ()

instance CanLog IO where dispatchMessage = logM

Global configuration stored in global mutable variable

{-# NOINLINE logInternalState #-}
logInternalState :: MVar LogInternalState
-- note: only kick up tree if handled locally
logInternalState = unsafePerformIO $ do ...

What I propose

We probably want to switch to some another approach, for example: caps. But at least LogInternalState should be stored in ReaderT context. Things become more complex because we want to support pure logging as well and we don't need LogInternalState there.

So let's have:

newtype LoggerContext = ... -- old `LogInternalState`

newtype LoggerT m a = LoggerT { runLoggerT :: ReaderT (TVar LoggerContext) m a }

This will make transition more smooth. We are not going to remove all IO functions, they just will work inside LoggerT monad.

Here is the plan to do this:

  1. Rename LogInternalState to LoggerContext.
  2. Add LoggerT newtype with all required derivings somewhere in its own module with LogInternalState.
  3. Refactor all needed functions inside IOLogger to user LoggerT type.
  4. IO can no longer have CanLog instance. Instead LoggerT should have this instance implemented.
  5. Remove buildAndSetupYamlLogging.
  6. Now, the interesting and most difficult part: setupLogging function cannot longer exist in a way it exists now. The only ways to run logging should be the following:
    • launchWithConfig
    • launchFromFile
    • launchSimpleLogging Inside these functions we want to:
    • Create TVar with empty internal state which is here now (logInternalState variable should be removed after that)
    • Run setupLogging which updates this TVar
    • Clears this TVar after everything is finished
    • bracket still should be used

Probably some technical details will arise during implementation..

chshersh commented 6 years ago

Fixed in version 2.0.