-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
latest finalized block metrics #12339
latest finalized block metrics #12339
Conversation
# Conflicts: # common/client/mock_rpc_test.go
…smartcontractkit/chainlink into feature/BCI-2649-latest-finalized-block
…smartcontractkit/chainlink into feature/BCI-2649-latest-finalized-block
handle corner case for multiple uncle blocks at the end of the slice
FinalizedBlockPollInterval() time.Duration | ||
} | ||
|
||
type ChainConfig interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you help me reason through why this configuration belongs to ChainConfig
vs NodeConfig
or if ChainConfig
is living in the correct place (vs in MultiNode)?
I'd expect configuration here to be node specific and for chain details to live at a higher level in the abstraction hierarchy, or for node
to store a map[string]*ChainConfig
to support multiple chain configurations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FinalizedBlockPollInterval
belongs to NodeConfig
and defines how often a Node
instance should poll for a new finalized block. It's not a property of a chain; it's a property of the component that performs the health assessment of an RPC.
Node is responsible for the health assessment of a single RPC that works only with one chain. Node does not store map[string]*ChainConfig
as there is no reason for it to be aware of other chain configurations.
@@ -43,6 +44,14 @@ type NodeConfig interface { | |||
SelectionMode() string | |||
SyncThreshold() uint32 | |||
NodeIsSyncingEnabled() bool | |||
FinalizedBlockPollInterval() time.Duration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering, do we need different configs for each type of poll?
What if we just reuse the PollInterval for all polling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some chains, we may also look for new heads via polling, not via subscribe.
In that case too, we wouldn't like to poll separately for new heads and new finalized heads. Mostly just make a single batch call to get both.
So that's why I am thinking, could we club all things to be polled under a same config, and fetch them together in a batch call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a similar impression, it would be more efficient to batch whenever we can instead of introducing a new ticker that will increase pressure on the RPC. AFAIK we already poll RPC to verify if it's healthy so probably we could use that logic for adding latest finalized block. If we manage to bundle that together into a single batch then we will get finality tracking for free
Maybe reusing existing <-pollCh
?
for {
select {
case <-n.nodeCtx.Done():
return
case <-pollCh:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not in favour of merging all of the polls into a single ticker, as they check different properties of the RPC.
Poll checks that RPC is reachable, this is super basic check and we want to do it often and be aggressive with the timeouts, if an RPC needs > 1s to return it's version, it not healthy, while for finalized block it seems ok.
Regarding the new heads polling, it makes sense to batch poll in this case.
var pollFinalizedHeadCh <-chan time.Time | ||
if n.nodePoolCfg.FinalizedBlockPollInterval() > 0 { | ||
lggr.Debugw("Finalized block polling enabled") | ||
pollT := time.NewTicker(n.nodePoolCfg.FinalizedBlockPollInterval()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't the NewTicker() panic if the parameter is 0?
I think in the fallback file, you should choose a default same as Eth mainnet, that's maybe 5 seconds?
Also, in config validation, ensure that this value is more than 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't the NewTicker() panic if the parameter is 0?
Yes, the NewTicker panics if the parameter is 0, that's why we do not initialize it unless provided value is > 0
I think in the fallback file, you should choose a default same as Eth mainnet, that's maybe 5 seconds?
Sounds good.
Also, in config validation, ensure that this value is more than 0.
IMHO, it should be possible to disable the check. Other health checks are optional, I do not see why this one should be an exception
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I haven't been too involved in multinode changes but the config builder and chain client changes look good.
Added metric to track latest finalized block observed by an RPC.
TIcket has more details.
This PR only introduces metric and does not include latest finalised block into health assessment of an RPC.
We plan to introduce it separately. Here is ticket for tracking