CAD-1131: add a UTxO size trace to TraceStartLeadershipCheck messages & lots of cleanups #1329

deepfire · 2020-06-23T18:16:40Z

Factors the Cardano.Tracing.Kernel module for NodeKernel access.
Cardano.Config.LedgerQuery for a ledger queries typeclass.
Enhance the TraceStartLeadershipCheck message with a UTxO size value.
Switch over to the newly available Shelley.Spec.Ledger.BaseTypes.StrictMaybe.
Clean up the overall tracer setup somewhat.

Sample messages:

{"at":"2020-06-23T22:30:54.43Z","env":"1.14.0:00000","ns":["cardano.node.Forge"],"data":{"kind":"TraceStartLeadershipCheck","slot":0},"app":[],"msg":"","pid":"12379","loc":null,"host":"andromed","sev":"Info","thread":"37"}
{"at":"2020-06-23T22:30:54.63Z","env":"1.14.0:00000","ns":["cardano.node.Forge"],"data":{"kind":"TraceStartLeadershipCheck","slot":1,"utxoSize":3},"app":[],"msg":"","pid":"12379","loc":null,"host":"andromed","sev":"Info","thread":"37"}

Note the absence of utxoSize for slot 0, when there's no NodeKernel available yet.

cardano-config/src/Cardano/Config/LedgerQueries.hs

cardano-node/src/Cardano/Tracing/Kernel.hs

erikd · 2020-06-23T23:00:03Z

cardano-node/src/Cardano/Tracing/Tracers.hs

@@ -465,7 +478,7 @@ mkConsensusTracers' elidingFetchDecision measureBlockForging
  tracingVerbosity :: TracingVerbosity
  tracingVerbosity = traceVerbosity traceConf

-mkConsensusTracers' _ _ _ _ TracingOff _ = Consensus.Tracers
+mkConsensusTracers' _ _ _ _ _ TracingOff _ = Consensus.Tracers


Surely there is a less horrible way to do that?

That would take a refactor, and people complain about large changes -- making them hard to review.

I already took some risks with this one..

Suggestion: fix it in a separate commit (before the other changes)?

@mrBliss, done!

I didn't clean up all things I could, but that's already a large change.
We have quite a huge mess there..

cardano-node/src/Cardano/Tracing/Kernel.hs

cardano-config/src/Cardano/Config/LedgerQueries.hs

cardano-node/src/Cardano/Node/Run.hs

cardano-node/src/Cardano/Tracing/Kernel.hs

cardano-node/src/Cardano/Tracing/Tracers.hs

cardano-config/src/Cardano/Config/LedgerQueries.hs

cardano-node/src/Cardano/Tracing/Kernel.hs

cardano-node/src/Cardano/Node/Run.hs

cardano-node/src/Cardano/Tracing/Tracers.hs

mrBliss · 2020-06-24T06:24:47Z

cardano-node/src/Cardano/Tracing/Tracers.hs

 import           Data.IORef (IORef, atomicModifyIORef', readIORef)
 import qualified Data.Text as Text
+import qualified Data.HashMap.Strict as Map


Why not an ordered Map like everywhere else?

We're creating an Aeson object manually, which uses this HashMap type under the hood.

cardano-node/src/Cardano/Tracing/Tracers.hs

mrBliss · 2020-06-24T06:28:20Z

cardano-config/src/Cardano/Config/LedgerQueries.hs

+
+
+class LedgerQueries blk where
+  ledgerUtxoSize :: LedgerState blk -> Int


I have seen UTxO (vs. Utxo) in a few places, although Utxo follows Haskell's capitalisation rules better.

Yes, that was one the unstated questions I had, actually, thank you!

So, Thomas, if you think we should change it, I'd need a slightly less subtle suggestion. : -)

For the record, ledger code uses:

UTXO

UTxO

Utxo

utxo

mrBliss · 2020-06-24T06:32:06Z

cardano-node/src/Cardano/Tracing/Kernel.hs

+
+newtype NodeKernelData blk =
+  NodeKernelData
+  { unNodeKernelData :: IORef (SMaybe (NodeKernel IO RemoteConnectionId LocalConnectionId blk))


This IORef will only be filled in once, when the NodeKernel has started (I know that was already the case before this PR), so that IORef + SMaybe is only used to break the circular dependency right?

Yes, the tracers have to be created to make NodeKernel available.

mrBliss · 2020-06-24T06:32:39Z

cardano-node/src/Cardano/Tracing/Tracers.hs

@@ -465,7 +478,7 @@ mkConsensusTracers' elidingFetchDecision measureBlockForging
  tracingVerbosity :: TracingVerbosity
  tracingVerbosity = traceVerbosity traceConf

-mkConsensusTracers' _ _ _ _ TracingOff _ = Consensus.Tracers
+mkConsensusTracers' _ _ _ _ _ TracingOff _ = Consensus.Tracers


Suggestion: fix it in a separate commit (before the other changes)?

cardano-node/src/Cardano/Tracing/Tracers.hs

Looks like its been done.

deepfire · 2020-06-25T07:57:07Z

bors r+

1329: CAD-1131: add a UTxO size trace to TraceStartLeadershipCheck messages & lots of cleanups r=deepfire a=deepfire 1. Factors the `Cardano.Tracing.Kernel` module for `NodeKernel` access. 2. `Cardano.Config.LedgerQuery` for a ledger queries typeclass. 3. Enhance the `TraceStartLeadershipCheck` message with a UTxO size value. 4. Switch over to the newly available `Shelley.Spec.Ledger.BaseTypes.StrictMaybe`. 5. Clean up the overall tracer setup somewhat. Sample messages: ``` {"at":"2020-06-23T22:30:54.43Z","env":"1.14.0:00000","ns":["cardano.node.Forge"],"data":{"kind":"TraceStartLeadershipCheck","slot":0},"app":[],"msg":"","pid":"12379","loc":null,"host":"andromed","sev":"Info","thread":"37"} {"at":"2020-06-23T22:30:54.63Z","env":"1.14.0:00000","ns":["cardano.node.Forge"],"data":{"kind":"TraceStartLeadershipCheck","slot":1,"utxoSize":3},"app":[],"msg":"","pid":"12379","loc":null,"host":"andromed","sev":"Info","thread":"37"} ``` Note the absence of `utxoSize` for slot 0, when there's no `NodeKernel` available yet. Co-authored-by: Kosyrev Serge <kosyrev.serge@protonmail.com>

iohk-bors · 2020-06-25T08:27:12Z

Build failed

ci/hydra-build:required

deepfire · 2020-06-25T08:49:57Z

bors r+

iohk-bors · 2020-06-25T09:43:48Z

Build succeeded

deepfire force-pushed the cad-1131-utxo-size-trace branch 6 times, most recently from 5cc1b15 to e1fdcfa Compare June 23, 2020 22:23

deepfire requested review from mrBliss, intricate and erikd June 23, 2020 22:33

deepfire self-assigned this Jun 23, 2020

deepfire marked this pull request as ready for review June 23, 2020 22:34

deepfire requested review from dcoutts and Jimbo4350 as code owners June 23, 2020 22:34

erikd requested changes Jun 23, 2020

View reviewed changes

deepfire force-pushed the cad-1131-utxo-size-trace branch from e1fdcfa to 657f2f6 Compare June 23, 2020 23:36

erikd requested changes Jun 24, 2020

View reviewed changes

cardano-node/src/Cardano/Tracing/Kernel.hs Outdated Show resolved Hide resolved

cardano-config/src/Cardano/Config/LedgerQueries.hs Outdated Show resolved Hide resolved

Jimbo4350 previously requested changes Jun 24, 2020

View reviewed changes

mrBliss reviewed Jun 24, 2020

View reviewed changes

deepfire force-pushed the cad-1131-utxo-size-trace branch 3 times, most recently from 7ae573b to 1113e36 Compare June 24, 2020 10:20

Jimbo4350 self-requested a review June 24, 2020 10:25

deepfire force-pushed the cad-1131-utxo-size-trace branch 2 times, most recently from 073a981 to a1436ec Compare June 24, 2020 11:18

CAD-1131 config: LedgerQueries class, for UTxO size queries

d07dd90

deepfire force-pushed the cad-1131-utxo-size-trace branch 3 times, most recently from aaca4a1 to 0b0271a Compare June 24, 2020 15:18

deepfire added 2 commits June 24, 2020 18:24

CAD-1131 tracing: factor Cardano.Tracing.Kernel

36e91b0

CAD-1131 tracing: clean up tracer setup somewhat

44c4e1a

deepfire force-pushed the cad-1131-utxo-size-trace branch from 0b0271a to aa763bd Compare June 24, 2020 15:24

deepfire changed the title ~~CAD-1131: add a UTxO size trace to TraceStartLeadershipCheck messages~~ CAD-1131: add a UTxO size trace to TraceStartLeadershipCheck messages & lots of cleanups Jun 24, 2020

erikd requested changes Jun 25, 2020

View reviewed changes

cardano-node/src/Cardano/Tracing/Tracers.hs Outdated Show resolved Hide resolved

deepfire force-pushed the cad-1131-utxo-size-trace branch from aa763bd to ac3467a Compare June 25, 2020 07:52

CAD-1131 tracing: trace UTxO size in TraceStartLeadershipCheck messages

70d0c88

deepfire force-pushed the cad-1131-utxo-size-trace branch from ac3467a to 70d0c88 Compare June 25, 2020 07:53

erikd approved these changes Jun 25, 2020

View reviewed changes

iohk-bors bot merged commit f8a2da1 into master Jun 25, 2020

iohk-bors bot deleted the cad-1131-utxo-size-trace branch June 25, 2020 09:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CAD-1131: add a UTxO size trace to TraceStartLeadershipCheck messages & lots of cleanups #1329

CAD-1131: add a UTxO size trace to TraceStartLeadershipCheck messages & lots of cleanups #1329

deepfire commented Jun 23, 2020 •

edited

Loading

erikd Jun 23, 2020

deepfire Jun 23, 2020 •

edited

Loading

mrBliss Jun 24, 2020

deepfire Jun 24, 2020 •

edited

Loading

mrBliss Jun 24, 2020

deepfire Jun 24, 2020

mrBliss Jun 24, 2020

deepfire Jun 24, 2020

deepfire Jun 24, 2020

deepfire Jun 24, 2020

mrBliss Jun 24, 2020

deepfire Jun 24, 2020

mrBliss Jun 24, 2020

deepfire commented Jun 25, 2020

iohk-bors bot commented Jun 25, 2020

deepfire commented Jun 25, 2020

iohk-bors bot commented Jun 25, 2020



		class LedgerQueries blk where
		ledgerUtxoSize :: LedgerState blk -> Int

CAD-1131: add a UTxO size trace to TraceStartLeadershipCheck messages & lots of cleanups #1329

CAD-1131: add a UTxO size trace to TraceStartLeadershipCheck messages & lots of cleanups #1329

Conversation

deepfire commented Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

deepfire Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepfire Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepfire commented Jun 25, 2020

iohk-bors bot commented Jun 25, 2020

Build failed

deepfire commented Jun 25, 2020

iohk-bors bot commented Jun 25, 2020

Build succeeded

deepfire commented Jun 23, 2020 •

edited

Loading

deepfire Jun 23, 2020 •

edited

Loading

deepfire Jun 24, 2020 •

edited

Loading