use metrics.RPCClientMetrics in multinode#97
Conversation
|
👋 guandali, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
dhaidashenko
left a comment
dhaidashenko
left a comment
There was a problem hiding this comment.
Why do we need to inject it on the framework level?
All the latency metrics are captured on rpc client level here.
Is there a reason to use a different approach?
vlfig
left a comment
vlfig
left a comment
There was a problem hiding this comment.
I think we can improve the ergonomics for callers.
| FinalizedBlockPollInterval() time.Duration | ||
| } | ||
|
|
||
| type RPCClientBaseMetricsConfig struct { |
There was a problem hiding this comment.
It doesn't make sense IMO to create a config that contains a RPCClientMetrics which the caller needs to construct with a RPCClientMetricsConfig that itself contains the chainFamily and ID.
I think it is a worthy change to have the caller call an interface instead of the metric directly so we can take care of things like moving from promauto to beholder and other details, but we should have the rpc_client care about it the least possible amount, and that'd be simply providing only the necessary config once.
So, if the URL and SendOnly are constant throughout the lifetime of the client, they can be part of RPCClientMetricsConfig, and that's it.
Hey @dhaidashenko yes, there is already latency instrumentation at the client layer. The client-level latency metrics capture raw RPC method timings, but they do not capture the multinode semantic operation (latest_block / latest_finalized_block) or post-call validation failures like invalid heads. We inject metrics at the framework level which gives us a consistent, chain-agnostic selection metric that reflects whether an RPC can return a valid, usable head for multinode, rather than only whether some underlying RPC method responded quickly. |
Makes sense. Will approve once Vasco's comments are resolved. |
📊 API Diff Results
|
|
| return | ||
| } | ||
|
|
||
| m.rpcMetrics.RecordRequest(ctx, rpcMetricsDefaultURL, rpcMetricsDefaultIsSendOnly, callName, time.Since(startedAt), err) |
There was a problem hiding this comment.
Why do we always pass the rpcMetricsDefaultURL? This way we won't be able to differentiate the metrics for different rpc instances
* adding jovay's nonce too low regex This reverts commit d5cad0f. * bump chainlink-framework * fixing API breaking change from smartcontractkit/chainlink-framework#97
4 participants
Description
This PR integrates RPCClientMetrics into multinode so RPC head polling calls emit observability data directly from RPCClientBase. It records latency for LatestBlock and LatestFinalizedBlock with labels for chainFamily, chainID, rpcUrl, isSendOnly, success, and rpcCallName, allowing error rate to be derived from rpc_call_latency_count{success="false"} instead of maintaining a separate error counter.
It also adds test coverage for successful requests, failed RPC calls, and invalid-head responses
Instrument RPC client with Beholder metrics for latency and error rates (v1)
Requires Dependencies