refactor(zetaclient): move rpc latency logs to metrics #3313
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a comprehensive refactoring of RPC latency handling across multiple blockchain observers. The primary focus is transitioning from an alert-based latency mechanism to a metrics-driven approach. This involves removing Changes
Assessment against linked issues
Suggested Labels
Suggested ReviewersThe implementation elegantly transforms RPC latency monitoring from a localized alert system to a centralized, configurable metrics approach, enhancing system observability and flexibility. Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3313 +/- ##
===========================================
- Coverage 61.67% 61.63% -0.05%
===========================================
Files 433 433
Lines 31011 30981 -30
===========================================
- Hits 19126 19095 -31
- Misses 11026 11027 +1
Partials 859 859
|
gartnera
force-pushed
the
zetaclient-remove-rpc-latency-logs
branch
from
8b857e6 to
0e7f610
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
zetaclient/chains/evm/observer/rpc_status.go (1)
39-39: Consider enhancing log context for better observabilityWhile the transition to metrics is correct, consider adding debug logs to provide additional context about the latency being reported.
+ ob.Logger().Chain.Debug().Dur("block_latency", blockTime).Msg("EVM block latency reported") ob.ReportBlockLatency(blockTime)zetaclient/chains/bitcoin/observer/rpc_status.go (1)
Line range hint
11-11: Remove unused context parameterThe context parameter is marked as unused with
_. If context isn't needed, consider removing it from the method signature.-func (ob *Observer) watchRPCStatus(_ context.Context) error { +func (ob *Observer) watchRPCStatus() error {zetaclient/chains/solana/observer/rpc_status.go (1)
43-43: Consider enhancing log context for better observabilityWhile the transition to metrics is correct, consider adding debug logs to provide additional context about the latency being reported, including the privnet status.
+ ob.Logger().Chain.Debug(). + Dur("block_latency", blockTime). + Bool("is_privnet", privnet). + Msg("Solana block latency reported") ob.ReportBlockLatency(blockTime)zetaclient/chains/base/observer.go (1)
427-432: Consider enhancing method documentation.While the implementation is correct, the documentation could be more specific about the time units being reported (seconds).
Consider updating the comment to:
-// ReportBlockLatency records the latency between the current time -// an the latest block time for a chain as a metric +// ReportBlockLatency records the latency in seconds between the current time +// and the latest block time for a chain as a metric
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
zetaclient/chains/base/observer.go(1 hunks)zetaclient/chains/base/observer_test.go(25 hunks)zetaclient/chains/bitcoin/observer/observer.go(0 hunks)zetaclient/chains/bitcoin/observer/observer_test.go(0 hunks)zetaclient/chains/bitcoin/observer/outbound_test.go(1 hunks)zetaclient/chains/bitcoin/observer/rpc_status.go(1 hunks)zetaclient/chains/evm/observer/observer.go(0 hunks)zetaclient/chains/evm/observer/observer_test.go(0 hunks)zetaclient/chains/evm/observer/rpc_status.go(1 hunks)zetaclient/chains/evm/signer/signer_test.go(0 hunks)zetaclient/chains/solana/observer/inbound_test.go(2 hunks)zetaclient/chains/solana/observer/observer.go(0 hunks)zetaclient/chains/solana/observer/observer_test.go(0 hunks)zetaclient/chains/solana/observer/outbound_test.go(1 hunks)zetaclient/chains/solana/observer/rpc_status.go(1 hunks)zetaclient/chains/ton/observer/observer.go(1 hunks)zetaclient/chains/ton/observer/observer_test.go(0 hunks)zetaclient/config/config_chain.go(2 hunks)zetaclient/config/types.go(1 hunks)zetaclient/metrics/metrics.go(1 hunks)zetaclient/orchestrator/bootstap_test.go(1 hunks)zetaclient/orchestrator/bootstrap.go(0 hunks)
💤 Files with no reviewable changes (9)
- zetaclient/chains/evm/signer/signer_test.go
- zetaclient/chains/ton/observer/observer_test.go
- zetaclient/chains/solana/observer/observer_test.go
- zetaclient/chains/bitcoin/observer/observer_test.go
- zetaclient/chains/evm/observer/observer.go
- zetaclient/chains/evm/observer/observer_test.go
- zetaclient/chains/bitcoin/observer/observer.go
- zetaclient/chains/solana/observer/observer.go
- zetaclient/orchestrator/bootstrap.go
✅ Files skipped from review due to trivial changes (1)
- zetaclient/config/config_chain.go
🧰 Additional context used
📓 Path-based instructions (12)
zetaclient/chains/solana/observer/rpc_status.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/observer/rpc_status.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/observer/outbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/metrics/metrics.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/config/types.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/rpc_status.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/orchestrator/bootstap_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/observer/inbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/ton/observer/observer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/base/observer_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/outbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/base/observer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (9)
zetaclient/chains/ton/observer/observer.go (1)
189-189: LGTM: Successfully migrated to metrics-based latency reporting.
The change aligns with the PR objective of moving from alert-based to metrics-based latency monitoring.
zetaclient/chains/solana/observer/inbound_test.go (1)
75-75: LGTM: Successfully removed deprecated RPC alert latency parameter.
The test modifications correctly align with the observer's new initialization signature, maintaining the same test coverage and functionality.
Also applies to: 116-116
zetaclient/config/types.go (2)
41-41: LGTM: Configuration types updated to reflect metrics-based approach.
The removal of RPCAlertLatency fields and reorganization of BTCConfig fields improve code clarity while maintaining all necessary configuration options.
Also applies to: 47-50, 55-55
41-41: Verify complete removal of RPCAlertLatency references.
Let's ensure all references to the removed configuration fields have been updated.
zetaclient/metrics/metrics.go (1)
82-88: LGTM: Well-structured metric definition.
The new LatestBlockLatency metric follows the established patterns and naming conventions, with clear documentation and appropriate metric type selection (GaugeVec).
zetaclient/orchestrator/bootstap_test.go (1)
219-219: LGTM: Clean removal of deprecated config.
The removal of RPCAlertLatency from the TON configuration aligns with the transition to metrics-based latency monitoring while maintaining comprehensive test coverage.
zetaclient/chains/base/observer_test.go (1)
Line range hint 39-61: LGTM: Test suite initialization refactored correctly.
The removal of the alertLatency parameter aligns with the PR objective of moving RPC latency handling to metrics. The test suite initialization remains clean and maintains all essential parameters.
zetaclient/chains/solana/observer/outbound_test.go (1)
64-64: LGTM: Observer initialization aligned with new signature.
The removal of the timeout parameter is consistent with the broader refactoring effort to simplify observer initialization and move latency handling to metrics.
zetaclient/chains/bitcoin/observer/outbound_test.go (1)
46-46: LGTM: Bitcoin observer initialization updated correctly.
The removal of the hardcoded timeout value (60) aligns with the PR's objective of transitioning RPC latency handling to metrics.
| @@ -41,6 +41,5 @@ | |||
| return | |||
| } | |||
|
|
|||
| // alert if RPC latency is too high | |||
| ob.AlertOnRPCLatency(blockTime, rpc.RPCAlertLatency) | |||
| ob.ReportBlockLatency(blockTime) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider propagating context for better cancellation support
The RPC status check could benefit from context propagation for proper cancellation handling.
-func (ob *Observer) checkRPCStatus() {
+func (ob *Observer) checkRPCStatus(ctx context.Context) {
tssAddress, err := ob.TSS().PubKey().AddressBTC(ob.Chain().ChainId)
if err != nil {
ob.Logger().Chain.Error().Err(err).Msg("unable to get TSS BTC address")
return
}
- blockTime, err := rpc.CheckRPCStatus(ob.btcClient, tssAddress)
+ blockTime, err := rpc.CheckRPCStatus(ctx, ob.btcClient, tssAddress)Committable suggestion skipped: line range outside the PR's diff.
Remove all "rpc latency" logs and move them to metrics instead. We definitely prefer to set alert thresholds in the monitoring stack rather than in a config file.
Closes #3292
Summary by CodeRabbit
LatestBlockLatency, for tracking the latency of the last block across observed chains.rpcAlertLatencyparameter from various observer initialization functions, simplifying the observer's configuration.rpcAlertLatencyparameter and ensure consistent observer initialization.