[input-output-hk/cardano-sl] [CBR-97] structured logging (#3332)

parsonsmatt requested changes on this pull request.

I got about halfway through the PR before it was force pushed and removed the commits I was reviewing. Please mark PRs as WIP if they’re not ready for review.

>
-instance {-# OVERLAPPING #-} CanJsonLog AuxxMode where
— jsonLog = realModeToAuxx … jsonLog
+— instance {-# OVERLAPPING #-} CanJsonLog AuxxMode where
+— jsonLog = realModeToAuxx … jsonLog

Dead code to delete :smile:

> return res

—————————————————————————-
— Local
—————————————————————————-

-type SscLocalQuery a =
— forall m . (MonadReader SscLocalData m, WithLogger m) => m a
+type SscLocalQuery a = forall m . Monad m =>
+ TraceNamed m ->
+ ReaderT SscLocalData m a

This is going to reverse the current convention of putting the logging argument first. If we’re going to establish a convention of passing the logger manually, I would *really* prefer it remain as the first argument, if at all possible.

>
type SscGlobalUpdate a =
— forall m . (MonadState SscGlobalState m, WithLogger m, Rand.MonadRandom m) => m a
+ WriterT (DList LoggedItem) (StateT SscGlobalState (Rand.MonadPseudoRandom Rand.ChaChaDRG)) a

I’m very skeptical of `WriterT` due to it’s propensity for space leaks (both in thunk accumulation and lack-of-streaming). It would be preferable to use `StateT` to avoid thunk accumulation.

> @@ -27,7 +26,7 @@ import Pos.Core.Update (BlockVersionData)

— | Type class which provides functions necessary for read-only
— verification of SSC data.
-class (Monad m, WithLogger m) =>
+class (Monad m {-, WithLogger m -}) =>

dead code

> @@ -40,16 +47,14 @@ type MultiRichmenSet = HashMap EpochIndex RichmenSet
— them with the same seed every time is insecure and must not be done.
newtype PureToss a = PureToss
{ getPureToss :: StateT SscGlobalState (
— NamedPureLogger (
+ WriterT (DList (LogNamed LogItem)) (

noo, not `WriterT` again :smile:

> @@ -70,6 +70,8 @@ verifyToil pm bvd lockedAssets curEpoch verifyAllIsKnown =

— | Apply transactions from one block. They must be valid (for
— example, it implies topological sort).
+
+

dead space

> @@ -149,12 +148,12 @@ memPoolSize = use $ ltsMemPool . mpSize
type ExtendedLocalToilM extraEnv extraState =
ReaderT (UtxoLookup, extraEnv) (
StateT (LocalToilState, extraState) (
— NamedPureLogger Identity
+ Identity

Why keep `Identity` around? `State s ~ StateT s Identity`

> ))

— | Natural transformation from ‘LocalToilM to ‘ExtendedLocalToilM’.
extendLocalToilM :: LocalToilM a -> ExtendedLocalToilM extraEnv extraState a
-extendLocalToilM = mapReaderT (mapStateT lift . zoom _1) . magnify _1
+extendLocalToilM = mapReaderT (mapStateT identity . zoom _1) . magnify _1

iirc `mapStateT identity ~ identity`

> @@ -348,7 +348,8 @@ instance (MonadPollRead m) =>
adoptedBVD <- getAdoptedBVData case bvs of Nothing ->
— logWarning $ «setAdoptedBV: unknown version » <> pretty bv — can’t happen actually
+ —logWarning $ «setAdoptedBV: unknown version » <> pretty bv — can’t happen actually
+ return ()

«can’t happen actually» would love to have a justification :|

Why have it commented out?

> , getSecretKeys
, getSecretKeysPlain
, addSecretKey
, deleteAllSecretKeys
, deleteSecretKeyBy
— , newSecretKey
+ —, newSecretKey — never accessed

let’s delete the dead code then :slightly_smiling_face:

> jsonLogDefault jlc x =
case jlc of
JsonLogDisabled -> return ()
JsonLogConfig v decide -> do
event <- toEvent <$> timedIO x
b <- liftIO (decide event) - `catchAny` \e -> do
— logWarning $ sformat («error in deciding whether to json log: «%shown) e
+ `catchAny` \_ -> do

Is this still a TODO?

> import qualified Formatting.Buildable
-import Formatting.Internal (Format (..))
-import qualified Language.Haskell.TH as TH
-import System.Wlog (CanLog (..), HasLoggerName (..), Severity (..),
— logMCond)
-import System.Wlog.LogHandler (LogHandlerTag (HandlerFilelike))
+— import Formatting.Internal (Format (..))

Dead imports

> import Pos.Core.Txp (TxId)
import Pos.Crypto (PassPhrase)

——————————————————————————
— Logging
——————————————————————————

I assume the code here is going to be coming from `Pos.Util.Log.LogSafe` instead?

>
instance {-# OVERLAPPABLE #-}
( MonadBListener m, Monad m, MonadTrans t, Monad (t m)) =>
MonadBListener (t m)
where
— onApplyBlocks = lift . onApplyBlocks
— onRollbackBlocks = lift . onRollbackBlocks
+ onApplyBlocks logTrace blunds = {-lift $-} onApplyBlocks logTrace blunds
+ onRollbackBlocks logTrace blunds = {-lift $-} onRollbackBlocks logTrace blunds

dead code

> import UnliftIO (MonadUnliftIO)

import Pos.DB.Class (MonadDBRead)
import Pos.DB.GState.Stakes (getRealTotalStake)
import Pos.DB.Txp (sanityCheckStakes, sanityCheckUtxo)
import Pos.Util.AssertMode (inAssertMode)
+—import Pos.Util.Log (WithLogger)

dead import

> @@ -40,16 +39,15 @@ import Pos.Core.Block (BlockHeader (..), Blockchain (..),
import qualified Pos.Core.Block as BC
import Pos.Core.Block.Constructors (mkGenesisBlock, mkMainBlock)
import Pos.Core.Context (HasPrimaryKey, getOurSecretKey)
-import Pos.Core.Exception (assertionFailed, reportFatalError)
-import Pos.Core.JsonLog (CanJsonLog (..))
+import Pos.Core.Exception (assertionFailed, traceFatalError)
+—import Pos.Core.JsonLog (CanJsonLog (..))

dead import

Добавить комментарий