chore: 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 43s —— View job Code Review CompleteI've reviewed this PR which adds OpenTelemetry tracing to Ethereum RPC client interactions. The implementation is generally well-structured and follows good practices. Below is my detailed feedback organized by area. Architecture & Design ✅Strengths:
Minor Observations:
Code Quality & Go Idioms ✅Strengths:
Issues:
Error Handling & Resilience ✅Strengths:
Issues:
Testing & Reliability ⭐️Strengths:
Issues (from Gemini + my review):
Security 🔒Strengths:
Observations:
Performance & Resource Efficiency ⚡Strengths:
Minor Considerations:
Documentation 📝Issues:
Integration & Consistency 🔧Strengths:
Observations:
Summary & RecommendationsOverall Assessment: This is a well-implemented addition that follows Go and OpenTelemetry best practices. The code is clean, testable, and production-ready with minor improvements needed. Priority Fixes:
Nice-to-haves:
Testing: All modified lines are covered by tests (per Codecov), and the test cases are thorough. The suggested test improvements are about clarity and completeness, not coverage gaps. |
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.88% 57.76% -0.12%
==========================================
Files 97 97
Lines 9306 9306
==========================================
- Hits 5387 5376 -11
- Misses 3315 3326 +11
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:
|
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.