feat(tracing): adding eth client tracing #2960
Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @chatton's task in 1m 57s —— View job Comprehensive Code ReviewI've completed a thorough review of this PR which adds OpenTelemetry tracing for Ethereum RPC client interactions. The implementation follows the same decorator pattern as the Engine API tracing and is well-structured overall. Below is my detailed feedback organized by category. ✅ Architecture & Design PatternsStrengths:
Minor Observations:
✅ Error Handling & ResilienceStrengths:
Good Practice:
✅ Testing & ReliabilityStrengths:
Issues Identified: 🔴 Critical: Test assertion logic is confusing and incomplete (eth_rpc_tracing_test.go:172-178) The loop to verify that block header attributes are NOT set on error is hard to follow and missing the for _, attr := range attrs {
key := string(attr.Key)
require.NotEqual(t, "block_hash", key)
require.NotEqual(t, "state_root", key)
require.NotEqual(t, "gas_limit", key)
require.NotEqual(t, "gas_used", key)
// ❌ Missing: timestamp check!
}Recommended fix (as Gemini also noted): for _, attr := range attrs {
switch string(attr.Key) {
case "block_hash", "state_root", "gas_limit", "gas_used", "timestamp":
t.Fatalf("unexpected attribute %q found on error span", attr.Key)
}
}This is clearer and complete. Fix this → 🟡 Medium: Inconsistent error attribute validation (eth_rpc_tracing_test.go:274-277) The for _, attr := range attrs {
require.NotEqual(t, "tx_count", string(attr.Key))
}Recommended improvement: for _, attr := range attrs {
if string(attr.Key) == "tx_count" {
t.Fatalf("unexpected attribute 'tx_count' found on error span")
}
}This makes the intent explicit and matches the pattern suggested above. Fix this → ✅ SecurityGood practices observed:
No security concerns identified ✓ ✅ Performance & Resource EfficiencyStrengths:
Observations:
No performance concerns ✓ ✅ Code Quality & Go IdiomsStrengths:
Minor suggestion: var _ EthRPCClient = (*tracedEthRPCClient)(nil)This is a nice-to-have for catching interface changes at compile time. Add this → ✅ Documentation & Code CommentsStrengths:
Good enough ✓ - Documentation is adequate for this decorator implementation. 📊 Observability ValueThe Grafana screenshots show excellent observability improvements:
The tracing implementation will be valuable for:
📋 Summary & RecommendationsOverall Assessment: ✅ This is a solid, well-tested implementation that maintains architectural consistency and adds valuable observability. The code is production-ready with minor test improvements recommended. Required Changes:
Recommended Improvements: Approval Recommendation: Approve after fixing the test assertion gap (item #1). Items #2-3 are nice-to-haves. ✅ All CI checks passing (Buf, Codecov at 57.76%) Great work on maintaining consistency with the existing tracing infrastructure! |
Summary of ChangesHello @chatton, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the observability of Ethereum RPC interactions within the EVM execution engine by integrating OpenTelemetry tracing. It refactors the existing Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces OpenTelemetry tracing for Ethereum RPC client interactions. The changes are well-structured, using a decorator pattern to add tracing capabilities without modifying the core client logic. A new EthRPCClient interface is introduced, which improves modularity and testability. The accompanying tests are thorough and cover success and error cases for the new tracing functionality. My feedback includes a couple of suggestions to improve the clarity and completeness of the test assertions.
| for _, attr := range attrs { | ||
| key := string(attr.Key) | ||
| require.NotEqual(t, "block_hash", key) | ||
| require.NotEqual(t, "state_root", key) | ||
| require.NotEqual(t, "gas_limit", key) | ||
| require.NotEqual(t, "gas_used", key) | ||
| } |
There was a problem hiding this comment.
This loop to verify that block header attributes are not set on error is confusing and incomplete. While it correctly fails if an unexpected attribute is present, its logic is hard to follow. Additionally, it's missing a check for the timestamp attribute, which is set on success.
A clearer and more complete approach is to iterate through the attributes and explicitly fail if any of the success-only attributes are found.
for _, attr := range attrs {
switch string(attr.Key) {
case "block_hash", "state_root", "gas_limit", "gas_used", "timestamp":
t.Fatalf("unexpected attribute %q found on error span", attr.Key)
}
}| for _, attr := range attrs { | ||
| require.NotEqual(t, "tx_count", string(attr.Key)) | ||
| } |
There was a problem hiding this comment.
This loop to verify that the tx_count attribute is not set on error works, but it can be written more clearly to express the intent of checking for a specific forbidden attribute.
for _, attr := range attrs {
if string(attr.Key) == "tx_count" {
t.Fatalf("unexpected attribute 'tx_count' found on error span")
}
}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2960 +/- ##
==========================================
- Coverage 57.87% 57.83% -0.05%
==========================================
Files 97 97
Lines 9303 9303
==========================================
- Hits 5384 5380 -4
- Misses 3315 3319 +4
Partials 604 604
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
chatton
changed the title
Overview
This PR follows the same pattern as the previous PR and creates a decorator around the eth client that injects tracing.
Sample grafana output with metrics enabled.