-
Notifications
You must be signed in to change notification settings - Fork 114
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
feat(zetaclient): add generic rpc metrics #2597
Conversation
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughWalkthroughThe recent changes enhance the ZetaClient's functionality by integrating Prometheus metrics to monitor Ethereum and Solana RPC calls. This update introduces an instrumented HTTP client for better observability, error handling, and logging in RPC interactions. Additionally, tests have been updated to ensure accurate metric tracking and provide better insights into system performance. Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
29bb317
to
6677db8
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2597 +/- ##
========================================
Coverage 70.55% 70.55%
========================================
Files 335 335
Lines 18345 18373 +28
========================================
+ Hits 12943 12963 +20
- Misses 4821 4824 +3
- Partials 581 586 +5
|
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.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (4)
zetaclient/metrics/metrics_test.go (1)
41-78
: Consider adding comments for better readability.The
TestMetrics
function has been enhanced to use an instrumented HTTP client and includes additional assertions. Consider adding comments to explain the purpose of each assertion for better readability.+ // Increment metrics for different chains GetFilterLogsPerChain.WithLabelValues("chain1").Inc() GetFilterLogsPerChain.WithLabelValues("chain2").Inc() GetFilterLogsPerChain.WithLabelValues("chain2").Inc() time.Sleep(1 * time.Second) chain1Ctr := testutil.ToFloat64(GetFilterLogsPerChain.WithLabelValues("chain1")) c.Assert(chain1Ctr, Equals, 1.0) // Create an instrumented HTTP client httpClient, err := GetInstrumentedHTTPClient("http://127.0.0.1:8886") c.Assert(err, IsNil) // Perform HTTP GET requests and check responses res, err := httpClient.Get("http://127.0.0.1:8886") c.Assert(err, IsNil) defer res.Body.Close() c.Assert(res.StatusCode, Equals, http.StatusOK) res, err = httpClient.Get("http://127.0.0.1:8886/metrics") c.Assert(err, IsNil) defer res.Body.Close() c.Assert(res.StatusCode, Equals, http.StatusOK) body, err := io.ReadAll(res.Body) c.Assert(err, IsNil) metricsBody := string(body) c.Assert(strings.Contains(metricsBody, fmt.Sprintf("%s_%s", ZetaClientNamespace, "rpc_count")), Equals, true) // Assert that rpc count is being incremented at all rpcCount := testutil.ToFloat64(RPCCount) c.Assert(rpcCount, Equals, 2.0) // Assert that rpc count is being incremented correctly rpcCount = testutil.ToFloat64(RPCCount.With(prometheus.Labels{"host": "127.0.0.1:8886", "code": "200"})) c.Assert(rpcCount, Equals, 2.0) // Assert that rpc count is not being incremented incorrectly rpcCount = testutil.ToFloat64(RPCCount.With(prometheus.Labels{"host": "127.0.0.1:8886", "code": "502"})) c.Assert(rpcCount, Equals, 0.0)
zetaclient/metrics/metrics.go (1)
184-210
: Improve error handling for curried metrics.The
GetInstrumentedHTTPClient
function is well-implemented, but consider providing more context in the error messages for curried metrics.rpcCounterMetric, err := RPCCount.CurryWith(labels) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to curry RPCCount metric: %w", err) } rpcLatencyMetric, err := RPCLatency.CurryWith(labels) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to curry RPCLatency metric: %w", err) }Tools
GitHub Check: codecov/patch
[warning] 195-195: zetaclient/metrics/metrics.go#L195
Added line #L195 was not covered by tests
[warning] 199-199: zetaclient/metrics/metrics.go#L199
Added line #L199 was not covered by testszetaclient/orchestrator/bootstrap.go (2)
258-261
: Improve error message for HTTP client creation.The error message for the HTTP client creation could be more descriptive.
if err != nil { - logger.Std.Error().Err(err).Str("rpc.endpoint", evmConfig.Endpoint).Msgf("Unable to create HTTP client") + logger.Std.Error().Err(err).Str("rpc.endpoint", evmConfig.Endpoint).Msgf("Unable to create instrumented HTTP client for Ethereum") continue }Tools
GitHub Check: codecov/patch
[warning] 260-261: zetaclient/orchestrator/bootstrap.go#L260-L261
Added lines #L260 - L261 were not covered by tests
388-391
: Improve error message for HTTP client creation.The error message for the HTTP client creation could be more descriptive.
if err != nil { - logger.Std.Error().Err(err).Str("rpc.endpoint", solConfig.Endpoint).Msgf("Unable to create HTTP client") + logger.Std.Error().Err(err).Str("rpc.endpoint", solConfig.Endpoint).Msgf("Unable to create instrumented HTTP client for Solana") continue }Tools
GitHub Check: codecov/patch
[warning] 390-391: zetaclient/orchestrator/bootstrap.go#L390-L391
Added lines #L390 - L391 were not covered by tests
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- zetaclient/chains/evm/signer/signer.go (2 hunks)
- zetaclient/metrics/metrics.go (3 hunks)
- zetaclient/metrics/metrics_test.go (2 hunks)
- zetaclient/orchestrator/bootstrap.go (3 hunks)
Additional context used
Path-based instructions (4)
zetaclient/metrics/metrics_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/orchestrator/bootstrap.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/evm/signer/signer.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
GitHub Check: codecov/patch
zetaclient/metrics/metrics.go
[warning] 195-195: zetaclient/metrics/metrics.go#L195
Added line #L195 was not covered by tests
[warning] 199-199: zetaclient/metrics/metrics.go#L199
Added line #L199 was not covered by testszetaclient/orchestrator/bootstrap.go
[warning] 260-261: zetaclient/orchestrator/bootstrap.go#L260-L261
Added lines #L260 - L261 were not covered by tests
[warning] 390-391: zetaclient/orchestrator/bootstrap.go#L390-L391
Added lines #L390 - L391 were not covered by testszetaclient/chains/evm/signer/signer.go
[warning] 869-869: zetaclient/chains/evm/signer/signer.go#L869
Added line #L869 was not covered by tests
Additional comments not posted (6)
zetaclient/metrics/metrics_test.go (1)
31-40
: LGTM!The
TestCurryWith
function correctly validates the behavior of theMustCurryWith
method from theRPCCount
metric.zetaclient/metrics/metrics.go (1)
117-143
: LGTM!The new Prometheus metrics
RPCInProgress
,RPCCount
, andRPCLatency
are well-defined and will provide valuable insights into the performance and success rates of RPC calls.zetaclient/chains/evm/signer/signer.go (4)
867-867
: Integrate the instrumented HTTP client correctly.The introduction of the instrumented HTTP client aligns with best practices for monitoring and observability.
868-870
: Ensure proper error handling for HTTP client instrumentation.The error handling for the
GetInstrumentedHTTPClient
function is appropriate, but ensure that the error messages are logged for better debugging.Do you want to log the error message for better debugging?
if err != nil { log.Error().Err(err).Msg("failed to get instrumented HTTP client") return nil, nil, err }Tools
GitHub Check: codecov/patch
[warning] 869-869: zetaclient/chains/evm/signer/signer.go#L869
Added line #L869 was not covered by tests
872-872
: Utilize the instrumented HTTP client for RPC client creation.The use of
ethrpc.DialHTTPWithClient
with the instrumented HTTP client is a good practice for enhancing observability.
876-876
: Create the Ethereum client correctly using the RPC client.The creation of the Ethereum client using the RPC client is correctly implemented.
6677db8
to
2b4e2fb
Compare
* feat(zetaclient): add generic rpc metrics * feedback * changelog * fmt
* feat(zetaclient): add generic rpc metrics * feedback * changelog * fmt
* feat(zetaclient): add generic rpc metrics * feedback * changelog * fmt
* feat: parse inscription like witness data (#2524) * parse inscription like witness data * more comment * remove unused code * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: Dmitry S <[email protected]> * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Dmitry S <[email protected]> * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: Dmitry S <[email protected]> * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * pull origin * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Dmitry S <[email protected]> * review feedbacks * update review feedbacks * update make generate * fix linter * remove over flow * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * Update zetaclient/chains/bitcoin/tokenizer.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * Update zetaclient/chains/bitcoin/tokenizer.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * Update zetaclient/chains/bitcoin/tokenizer.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * Update zetaclient/chains/bitcoin/tokenizer.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * update review feedback * update code commnet * update comment * more comments * Update changelog.md --------- Co-authored-by: Dmitry S <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> fix version * feat: detect memo in btc txn from OP_RETURN and inscription (#2533) * parse inscription like witness data * more comment * remove unused code * parse inscription * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: Dmitry S <[email protected]> * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Dmitry S <[email protected]> * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: Dmitry S <[email protected]> * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * pull origin * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Dmitry S <[email protected]> * review feedbacks * update review feedbacks * add mainnet txn * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * parse inscription like witness data * more comment * remove unused code * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: Dmitry S <[email protected]> * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Dmitry S <[email protected]> * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: Dmitry S <[email protected]> * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * pull origin * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Dmitry S <[email protected]> * review feedbacks * update review feedbacks * update make generate * fix linter * remove over flow * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * Update zetaclient/chains/bitcoin/tokenizer.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * Update zetaclient/chains/bitcoin/tokenizer.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * Update zetaclient/chains/bitcoin/tokenizer.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * Update zetaclient/chains/bitcoin/tokenizer.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * update review feedback * update code commnet * update comment * more comments * Update changelog.md * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * clean up * format code --------- Co-authored-by: Dmitry S <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * refactor(zetaclient)!: improve AppContext (#2568) * Implement chain registry * Rewrite test-cases for AppContext * Drop `supplychecker` * Refactor app ctx Update worker * Refactor orchestrator * Refactor observer&signer; DROP postBlockHeaders * Fix test cases [1] * Update changelog * Allow Zeta Chain in appContext; address PR comments [1] * Fix app context update * Check for `chain.IsZeta()` * Add AppContext.FilterChains * Fix test cases [2] * Fix test cases [3] * Address PR comments [1] * Address PR comments [2] * Add tests for `slices` * Fix e2e tests [1] * Fix e2e tests [2] * Resolve conflicts, converge codebase between PRs * Add lodash; remove slices pkg * Address PR comments * Minor logging fix * Address PR comments tmp * feat(zetaclient): add generic rpc metrics (#2597) * feat(zetaclient): add generic rpc metrics * feedback * changelog * fmt * fix(zetaclient): use name in pending tx metric (#2642) * feat(pkg): add `ticker` package (#2617) * Add `pkg/ticker` * Sample ticker usage in evm observer * Change naming * Address PR comments * Address PR comments * feat(zetaclient)!: Add support for EIP-1559 gas fees (#2634) * Add Gas struct * Add EIP-1559 fees * Update changelog * Add test cases for legacy vs dynamicFee txs * Fix typo; Add E2E coverage * Address PR comments * Address PR comments * Use gasFeeCap formula * Revert "Use gasFeeCap formula" This reverts commit 2260925. * Address PR comments * Fix e2e upgrade tests * fix: adjust evm outbound tracker reporter to avoid submitting invalid hashes (#2628) * refactor and fix evm outbound tracker reporter to avoid invalid hashes; print log when outbound tracker is full of invalid hashes * add changelog entry * used predefined log fields * remove repeated fields information from log message; Devops team would configure Datadog to show the fields * remove redundant fields in log message; unified logs * remove pending transaction map from observer; the outbound tracker reporter will no longer report pending hash * use bg.Work() to launch outbound tracker reporter goroutines * bring the checking EnsureNoTrackers() back * add more rationale to EVM outbound tracker submission * sync observer and signers without wait on startup * try fixing tss migration E2E failure by increase timeout * feat: Solana relayer (fee payer) key importer, encryption and decryption (#2673) * configure observer relayer key for Solana; remove hardcoded solana test key from zetaclient code * implementation of relayer key importer, encryption and decryption * integrate relayer key into E2E and Solana signer * add relayer_key_balance metrics and unit tests * use TrimSpace to trim password * add changelog entry * use relayer account array in E2E config; a few renaming; add private key validation when importing * fix linter * remove GetNetworkName method for simplification * added PromptPassword method to prompt single password * use network name as map index to store relayer key passwords * moved relayer passwords to chain registry * airdrop SOL token only if solana local node is available --------- Co-authored-by: Lucas Bertrand <[email protected]> * ci: Set Docker Workflow to use Go 1.22 (#2722) * Set go 1.22.2 * Set go 1.22.2 * Set go 1.22 * Set go 1.22 * Refactor contrib/rpc and contrib/docker-scripts to use snapshots API (#2724) Co-authored-by: Julian Rubino <[email protected]> --------- Co-authored-by: dev-bitSmiley <[email protected]> Co-authored-by: Dmitry S <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> Co-authored-by: Charlie Chen <[email protected]> Co-authored-by: Lucas Bertrand <[email protected]> Co-authored-by: Charlie <[email protected]> Co-authored-by: Julian Rubino <[email protected]> Co-authored-by: Julian Rubino <[email protected]>
Description
Add generic rpc metrics to zetaclient. BTC metrics are not possible because our BTC RPC client will not currently accept a custom
http.Client
. I will work on that upstream.Closes #2588
Closes #1524
Summary by CodeRabbit
New Features
RPCInProgress
,RPCCount
, andRPCLatency
.Bug Fixes
Tests