Add EKG counter for number of forks seen #3305
Conversation
karknu
requested review from
DavidEichmann,
dcoutts,
erikd,
JaredCorduan,
Jimbo4350,
nc6 and
newhoggy
as code owners
There was a problem hiding this comment.
The semantics of the changes look good to me! 👍
I suggested a simplification, but I would understand if Karl isn't comfortable with it -- feel free to respond saying as much. (Notably this PR introduces enough information for us to gather empirical evidence in the mean-time that will let us later conclude that the suggested simplification is sound, in which case we could do the simplification later.)
I'm not Approving only because my review by itself isn't sufficient to unblock merge.
| @@ -686,6 +686,7 @@ instance ( ConvertRawHash blk | |||
| [ "kind" .= String "TraceAddBlockEvent.SwitchedToAFork" | |||
| , "newtip" .= renderPointForVerbosity verb (AF.headPoint new) | |||
| , "chainLengthDelta" .= new `chainLengthΔ` old | |||
| , "realFork" .= not (AF.withinFragmentBounds (AF.headPoint old) new) | |||
There was a problem hiding this comment.
Maybe rename this to "sanityCheck"? Or add a comment explaining that we're just double-checking that the SwitchedToAFork event really was incurred by a proper fork.
| traceChainMetrics Nothing _ _ _ = nullTracer | ||
| traceChainMetrics (Just _ekgDirect) _blockConfig _fStats tr = do | ||
| traceChainMetrics Nothing _ _ _ _ = nullTracer | ||
| traceChainMetrics (Just _ekgDirect) tForks _blockConfig _fStats tr = do | ||
| Tracer $ \ev -> | ||
| fromMaybe (pure ()) $ doTrace <$> chainTipInformation ev | ||
| where | ||
| chainTipInformation :: ChainDB.TraceEvent blk -> Maybe ChainInformation | ||
| chainTipInformation = \case | ||
| ChainDB.TraceAddBlockEvent ev -> case ev of |
There was a problem hiding this comment.
If we generally trust that SwitchedToAFork is only emitted when it should be, then the changes to this case expression could be simpler; we just need to pass a True to chainInformation in one branch and a False in the other.
There was a problem hiding this comment.
👍 will make that change.
| { slots = unSlotNo $ fromWithOrigin 0 (AF.headSlot frag) | ||
| , blocks = unBlockNo $ fromWithOrigin (BlockNo 1) (AF.headBlockNo frag) | ||
| , density = fragmentChainDensity frag | ||
| chainInformation newTipInfo oldFrag_m newFrag blocksUncoupledDelta = ChainInformation |
There was a problem hiding this comment.
See my related comment in traceChainMetrics: we could get by with just a new Bool argument instead.
karknu
force-pushed
the
karknu/forks_counter
branch
from
db8a380
to
d793930
Compare
Add cardano_node_metrics_forks_int for counting the number of forks seen by the node.
karknu
force-pushed
the
karknu/forks_counter
branch
from
d793930
to
dd2c21b
Compare
|
bors r+ |
|
Build succeeded: |
Add cardano_node_metrics_forks_int for counting the number of forks seen
by the node.