Add heartbeat client & reporter to verifier #582
Conversation
verifier/verification_coordinator.go
Outdated
| monitoring Monitoring, | ||
| chainStatusManager protocol.ChainStatusManager, | ||
| heartbeatClient heartbeatpb.HeartbeatServiceClient, | ||
| heartbeatInterval time.Duration, |
There was a problem hiding this comment.
should we add this to CoordinatorConfig instead?
There was a problem hiding this comment.
for heartbeatInterval, yes, it's better to move it to config, but for the heartbeatClient i don't think it's possible. We want to have a separate client instance that is different from the current aggregator client, to decouple responsibilities. Is that what you mean?
There was a problem hiding this comment.
Yes, I meant only the heartbeatInterval
| } | ||
|
|
||
| // Add block height for this chain if available. | ||
| // TODO: change to use latest seen block height instead of finalized when available. |
There was a problem hiding this comment.
let's create a ticket for this so we don't forget about it.
| hr.logger.Infow("Heartbeat sent successfully", | ||
| "verifierId", hr.verifierID, | ||
| "aggregatorId", resp.AggregatorId, | ||
| "chainCount", len(blockHeightsByChain), |
There was a problem hiding this comment.
is the chainCount useful by itself?
There was a problem hiding this comment.
Since I've added the verifier_heartbeat_sent_chain_heads metric, it's less important, only for informational purposes -- some middle ground between nothing and including full blockHeightsByChain, but we can remove it if needed.
There was a problem hiding this comment.
In my experience just having the count is rarely useful while debugging tbh, doesn't harm but not very benefitial.
| lggr logger.Logger, | ||
| chainStatusManager protocol.ChainStatusManager, | ||
| heartbeatClient heartbeatpb.HeartbeatServiceClient, | ||
| allSelectors []protocol.ChainSelector, |
There was a problem hiding this comment.
Any reason for relying on this being injected? the status manager gets you all chains anyways with every call. if it's empty and no heart beat should be created should be checked in coordinator IMO
There was a problem hiding this comment.
Yeah, so ChainStatusManager does not have a method to get all chain statuses, so since we already need to pass a list of chainselectors, i think it's better to make the HeartBeat Reporter configurable as well, so that we can explicitly set on which chains we want to send latest heads informations.
There was a problem hiding this comment.
ah okay, I thought we had an endpoint to return all. that makes sense then.
|
|
||
| // Record what we're sending in the request. It will be used for monitoring of the lag. | ||
| for chainSelector, blockHeight := range req.ChainDetails.BlockHeightsByChain { | ||
| chainMetrics := metrics.With("chain_selector", fmt.Sprintf("%d", chainSelector)) |
There was a problem hiding this comment.
Can this explode cardinality?
There was a problem hiding this comment.
It's a lot of chain selectors and the block height is always increasing.
There was a problem hiding this comment.
it's only one metric per chainselector, so cardinality shouldn't be a problem
emate
force-pushed
the
emate/ccip-8200-add-heartbeat-client-to-verifier
branch
2 times, most recently
from
62baac3 to
7cb3220
Compare
…eartbeat-client-to-verifier
…eartbeat-client-to-verifier
emate
force-pushed
the
emate/ccip-8200-add-heartbeat-client-to-verifier
branch
from
3238c06 to
c960c18
Compare
* Add heartbeat client & reporter to verifier * Fix tests * Add basic unit tests * Fix linter issues * Fixes * Use NoopHeartbeatClient instead of nil * Pass heartbeat interval setting through coordinator config * Update .gitignore to include some dashboards * Add initial dashboards for observing heartbeats * Add dashboards for heartbeat overview * fix * Remove unnecessary dependencies for heartbeat client * Fix circular dependency * Fix implementation * Fix
|
Code coverage report:
|
2 participants
This PR implements heartbeat reporting from the verifier to the aggregator, establishing a unified observability framework that tracks both the verifier's chain state snapshot and the aggregator's perspective on block heights and adaptive scores.
The verifier now exposes comprehensive heartbeat metrics with verifier_id and chain_selector labels:
metrics published upon heartbeat send
verifier_heartbeats_sent_total- Total successfully sent heartbeatsverifier_heartbeats_failed_total- Total failed heartbeat attemptsverifier_heartbeat_duration_seconds- Request latency histogramverifier_heartbeat_sent_chain_heads- Block height snapshot sent in request per chainmetrics published based on the aggregator's response
verifier_heartbeat_timestamp- Aggregator's timestamp from responseverifier_heartbeat_chain_heads- Aggregator's reported block height per chainverifier_heartbeat_score- Adaptive score per chain from aggregator