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.
| cfg RPCClientBaseConfig, ctxTimeout time.Duration, log logger.Logger, | ||
| latestBlock func(ctx context.Context) (HEAD, error), | ||
| latestFinalizedBlock func(ctx context.Context) (HEAD, error), | ||
| rpcMetrics *RPCClientBaseMetricsConfig, |
There was a problem hiding this comment.
Here I think it makes more sense to accept an already RPCClientMetrics. The caller might (should?) itself want to call RecordRequest in other chain-specific places.
3 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