feat(blocklistener): Block height metrics for canonical chain vs node - #216
feat(blocklistener): Block height metrics for canonical chain vs node#216onelapahead wants to merge 3 commits into
Conversation
Signed-off-by: hfuss <hayden.fuss@kaleido.io>
Signed-off-by: hfuss <hayden.fuss@kaleido.io>
| for { | ||
| bl.emitChainStateMetrics() | ||
| select { | ||
| case <-bl.ctx.Done(): | ||
| return | ||
| case <-time.After(bl.BlockPollingInterval): | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func (bl *blockListener) emitChainStateMetrics() { | ||
| if bl.getMetrics() == nil { | ||
| return // never drive any query of the node when metrics are not enabled | ||
| } | ||
|
|
||
| // The canonical head is free to read - note in light chain tracking mode there is no canonical chain, | ||
| // so the listen loop emits the head it dispatches to consumers instead | ||
| if bl.ChainTrackingMode != ffcapi.ChainTrackingModeLight { | ||
| if canonicalHeight, ok := bl.getCanonicalBlockHeight(); ok { | ||
| bl.setBlockHeightMetric(metricCanonicalBlockHeight, canonicalHeight) | ||
| } | ||
| } | ||
|
|
||
| // The target height requires a query of the node | ||
| head, err := bl.refreshHighestBlockFromRPC() | ||
| if err != nil { | ||
| // Purely a metrics query - the listen loop has its own error handling for the chain state | ||
| log.L(bl.ctx).Warnf("Failed to query target block height for metrics: %s", err) | ||
| return | ||
| } | ||
| bl.setBlockHeightMetric(metricTargetBlockHeight, head) | ||
| } |
There was a problem hiding this comment.
@onelapahead can you elaborate on the thinking behind here? The metrics-emitting logic does functional logic (updates the chain head), rather than purely capturing metrics.
Have you considered making the metrics-emitting logic do a no-op or emit an "unavailable/error" status metric when the main block height setting loop is in an unready state?
There was a problem hiding this comment.
That is not the case nor intent - getCanonicalBlockHeight() acquires a RLock to read the listener's state but otherwise does not update anything as I understand it.
I've renamed refreshHighestBlockFromRPC() bc that was misleading, its named queryBlockHeightFromRPC and its just purely an eth_blokcNumber call, but it doesn't change any listener state.
There was a problem hiding this comment.
@onelapahead Thanks for the renaming.
So what was added in the PR is a separate goroutine that
- runs on the same timer as the main listener goroutine.
- doing a separate & extra
eth_blockNumbercall on the same timer
That approach has its merit, which is very easy to understand as a small patch, but doesn't feel like the most efficient way of capturing the diagnostic information. Also, lacks the next level of granularity to pin down the problem for diagnosis, because the metrics are not associated with any variable that's driving the functional logic.
Breaking down the situation you are trying to resolve, I'd give the following suggestions:
- is the target block height not incrementing (node out of sync) - depending on the underlying consensus/EVM implementation, this might indicate the node is failing to sync the chain.
it's a node running problem, metrics and alert should be added in the JSON-RPC node.
- are the block listener canonical height and the target height out of sync within some toleration/expectation - this would either indicate the endpoint has a bad filter/subscription and is failing to notify the listener of new blocks, or the listener itself is somehow failing to track the chain otherwise.
metrics should be added to track those failures. e.g.
checkAndSetHighestBlock(and/orqueryBlockHeightFromRPC) should emit failure metrics and gauge metrics for new head, so when the head stays stale, it's obvious. It can be used as a trigger to escalate to the JSON-RPC node provider ops to check their metrics as well if the metric is fromqueryBlockHeightFromRPC.
- if we are able to monitor the chain height itself via other means (a synced node we're monitoring, a block explorer, etc.), we can detect the difference of the target/canonical with the monitor to detect any lag.
this should be achieveable by adding metrics to
checkAndSetHighestBlockandqueryBlockHeightFromRPC
So I'd suggest replacing the new goroutine with metrics in checkAndSetHighestBlock and queryBlockHeightFromRPC. If you feel there is a need to do queryBlockHeightFromRPC in full-tracking mode, we could think about where to add it in the existing logic to improve the function logic. (Suggest a separate PR for this to make the current PR single-purposed)
Chengxuan
left a comment
Chengxuan
left a comment
There was a problem hiding this comment.
@onelapahead thanks for adding metrics. Agree with the intention of detecting out-of-sync / unreilable block listener head update. However, adding block head update logic in the metrics logic voilates single responsibilty practice, which make the logic more complex and bugs in the block listener logic harder to spot.
Signed-off-by: hfuss <hayden.fuss@kaleido.io>
| @@ -400,17 +407,20 @@ func (bl *blockListener) listenLoop() { | |||
| } | |||
|
|
|||
| if bl.ChainTrackingMode == ffcapi.ChainTrackingModeLight { | |||
There was a problem hiding this comment.
Can I just check the metric emission hasn't been made specific to light mode
There was a problem hiding this comment.
No - no matter what the metrics are emitted in all modes.
What you're seeing is a metric behavior difference between light mode and full mode - bc light mode does not track a canonical chain, the height it reports for the canonical chain height is the RPC's own block height.
3 participants
To help to detect if either the underlying node infrastructure or the block listener itself is unreliable / out-of-sync, we add two new gauges:
eth_blockNumberUsing such metrics we can alert on several different scenarios: