CAD-2371 Metrics for blocks lost due to switching #2321

newhoggy · 2021-01-29T06:25:43Z

No description provided.

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

nfrisby · 2021-02-15T05:48:41Z

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

@@ -969,19 +997,23 @@ data ChainInformation = ChainInformation
  , slotInEpoch :: Word64
    -- ^ Relative slot number of the tip of the current chain within the
    -- epoch.
+  , blocksLost :: Int64
+    -- ^ Blocks lost due to switch at fork


If you do end up following my overall proposal, then this comment is wrong and also the name is misleading.

I'd call it myBlocksSelected, an the comment would say something along the lines of: it's the difference between the numbers of my blocks on the latest selection minus that same number as of the last restart. Thus comparing myBlocksSelected to the number of blocks I've forged since the last restart shows how many blocks I've forged for naught.

Looks like that ^^^ plan would require another new metric that simply increments with each TraceForgedBlock event.

I've updated the code to what I think you mean.

OK, after a little more thinking, I actually think one metric will suffice, and it can be "blocks forged since last restart but not currently selected". (Edit: that's a bit of an overstatement, but it's a reasonable interpretation of this metric as a heuristic.)

TraceForgedBlock should increment the metric. SwitchedToAFork and AddedToCurrentChain should both increment it for each block of ours on the old chain and also decrement it for each block of ours on the new chain.

If we restart and then immediately switch to a fork with more of ours blocks, then this metric might dip negative. But over time, we should generally see this metric gradually increase.

Does that make sense? I'm headed to bed now -- talk to you tomorrow.

I feel like TraceForgedBlock shouldn't increment the metric because when we forge a block it is appended to the current chain and is therefore not one of the unselected ones?

The TraceForgedBlock just means we created the block and are about to add it to the ChainDB. Once it's added, then the ChainDB might select it, which would incur a separate AddedToCurrentChain event. So decrement for the forge, then increment for the subsequent selection, thus giving us the desired delta of 0.

Done. I've used the term "uncoupled" to mean blocks forged not on the current chain, which is the thing being counted.

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

nfrisby · 2021-02-16T23:03:42Z

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

    traceCounter "blocksForgedNum" tr
      =<< mapForgingCurrentThreadStats fStats
            (\fts -> (fts { ftsBlocksForgedNum = ftsBlocksForgedNum fts + 1 },
                       ftsBlocksForgedNum fts + 1))
+    meta <- mkLOMeta Critical Public
+    let tBlocksUncoupled = fsBlocksUncoupled fStats
+    traceI tr meta "mySelectedBlocks" =<< STM.modifyReadTVarIO tBlocksUncoupled (+ 1)


Yep, it's a +1 to the TVar because blocksUncoupledDelta counts blocks not on the current chain. And even though we just forged it, it's not on the chain until we later (though very soon) get a corresponding AddedToCurrentChain, which is handled higher up in the diff. 👍

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

nfrisby · 2021-02-24T01:45:52Z

@newhoggy Have you discussed this concern with @disassembler ? I haven't.

#2321 (comment)

newhoggy · 2021-02-24T22:57:31Z

Reproducing this comment from IntersectMBO/ouroboros-network#2930 (comment)

Thanks for the update. You mentioned we were using the cold keys, but @disassembler says we should be using the vrf keys.

This makes sense because other nodes use the vrf keys to verify that blocks a legit and it isn't cycled as I understand.

nfrisby · 2021-02-26T00:53:28Z

After a group convo on Slack, I merged IntersectMBO/ouroboros-network#2930 as-is. So I think this PR will also be ready once it bumps the ouroboros-network dep to something including the current tip of master there:5IntersectMBO/ouroboros-network@7cb1380

My summary of the slack thread: we only use the verification key of the cold key (and we're already using that for Shelley block selection), and using the cold key as "self" for this metric is appropriate.

…id chaing parser implementations causing tests to fail.

newhoggy · 2021-03-05T10:25:33Z

bors merge

iohk-bors · 2021-03-05T10:42:41Z

Build succeeded:

nfrisby · 2021-03-08T21:19:40Z

I re-reviewed the new metric itself: still LGTM. I did not review the other eg testing changes.

newhoggy commented Jan 29, 2021

View reviewed changes

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

newhoggy changed the title ~~Metrics for blocks gained and lost due to switching~~ [CAD-2371] Metrics for blocks gained and lost due to switching Feb 1, 2021

newhoggy changed the title ~~[CAD-2371] Metrics for blocks gained and lost due to switching~~ CAD-2371 Metrics for blocks gained and lost due to switching Feb 1, 2021

newhoggy force-pushed the CAD-2371-metrics-for-blocks-gained-and-lost-due-to-switching branch from b1f6803 to b5c5938 Compare February 1, 2021 20:10

newhoggy commented Feb 1, 2021

View reviewed changes

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

newhoggy force-pushed the CAD-2371-metrics-for-blocks-gained-and-lost-due-to-switching branch 4 times, most recently from 9fc85dc to 5a15096 Compare February 15, 2021 04:11

newhoggy requested a review from nfrisby February 15, 2021 04:35

newhoggy force-pushed the CAD-2371-metrics-for-blocks-gained-and-lost-due-to-switching branch from 5a15096 to ae05e7c Compare February 15, 2021 04:37

newhoggy marked this pull request as ready for review February 15, 2021 04:37

newhoggy requested review from DavidEichmann, dcoutts, erikd, intricate and Jimbo4350 as code owners February 15, 2021 04:37

erikd reviewed Feb 15, 2021

View reviewed changes

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

erikd approved these changes Feb 15, 2021

View reviewed changes

newhoggy force-pushed the CAD-2371-metrics-for-blocks-gained-and-lost-due-to-switching branch from ae05e7c to 2955cfd Compare February 15, 2021 05:15

nfrisby reviewed Feb 15, 2021

View reviewed changes

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

newhoggy changed the title ~~CAD-2371 Metrics for blocks gained and lost due to switching~~ CAD-2371 Metrics for blocks lost due to switching Feb 15, 2021

nfrisby reviewed Feb 15, 2021

View reviewed changes

newhoggy force-pushed the CAD-2371-metrics-for-blocks-gained-and-lost-due-to-switching branch 4 times, most recently from 4c48111 to 483c688 Compare February 15, 2021 08:14

nfrisby reviewed Feb 16, 2021

View reviewed changes

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

nfrisby reviewed Feb 16, 2021

View reviewed changes

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

nfrisby mentioned this pull request Feb 24, 2021

Add a self-issued predicate to the Consensus interface IntersectMBO/ouroboros-network#2930

Merged

newhoggy requested a review from nfrisby February 24, 2021 22:41

newhoggy force-pushed the CAD-2371-metrics-for-blocks-gained-and-lost-due-to-switching branch 2 times, most recently from 0ea8343 to 5839b30 Compare February 25, 2021 01:03

newhoggy force-pushed the CAD-2371-metrics-for-blocks-gained-and-lost-due-to-switching branch 16 times, most recently from 7454b25 to d192ace Compare March 5, 2021 07:03

Run golden tests at the value level rather than the text level to avo…

e918a65

…id chaing parser implementations causing tests to fail.

newhoggy force-pushed the CAD-2371-metrics-for-blocks-gained-and-lost-due-to-switching branch from d192ace to e918a65 Compare March 5, 2021 07:30

iohk-bors bot merged commit cc3c3cc into master Mar 5, 2021

iohk-bors bot deleted the CAD-2371-metrics-for-blocks-gained-and-lost-due-to-switching branch March 5, 2021 10:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CAD-2371 Metrics for blocks lost due to switching #2321

CAD-2371 Metrics for blocks lost due to switching #2321

newhoggy commented Jan 29, 2021

nfrisby Feb 15, 2021

nfrisby Feb 15, 2021 •

edited

newhoggy Feb 15, 2021

nfrisby Feb 15, 2021 •

edited

newhoggy Feb 15, 2021

nfrisby Feb 15, 2021

newhoggy Feb 15, 2021

nfrisby Feb 16, 2021 •

edited

nfrisby commented Feb 24, 2021

newhoggy commented Feb 24, 2021

nfrisby commented Feb 26, 2021 •

edited

newhoggy commented Mar 5, 2021

iohk-bors bot commented Mar 5, 2021

nfrisby commented Mar 8, 2021

CAD-2371 Metrics for blocks lost due to switching #2321

CAD-2371 Metrics for blocks lost due to switching #2321

Conversation

newhoggy commented Jan 29, 2021

nfrisby Feb 15, 2021

Choose a reason for hiding this comment

nfrisby Feb 15, 2021 • edited

Choose a reason for hiding this comment

newhoggy Feb 15, 2021

Choose a reason for hiding this comment

nfrisby Feb 15, 2021 • edited

Choose a reason for hiding this comment

newhoggy Feb 15, 2021

Choose a reason for hiding this comment

nfrisby Feb 15, 2021

Choose a reason for hiding this comment

newhoggy Feb 15, 2021

Choose a reason for hiding this comment

nfrisby Feb 16, 2021 • edited

Choose a reason for hiding this comment

nfrisby commented Feb 24, 2021

newhoggy commented Feb 24, 2021

nfrisby commented Feb 26, 2021 • edited

newhoggy commented Mar 5, 2021

iohk-bors bot commented Mar 5, 2021

nfrisby commented Mar 8, 2021

nfrisby Feb 15, 2021 •

edited

nfrisby Feb 15, 2021 •

edited

nfrisby Feb 16, 2021 •

edited

nfrisby commented Feb 26, 2021 •

edited