feat(tracing): add tracing to EngineClient #2959
Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @chatton's task in 2m 23s —— View job Code Review CompleteI've completed a comprehensive review of the tracing implementation for EngineClient. Overall, this is a well-structured PR that cleanly adds OpenTelemetry tracing through the decorator pattern. Below are my findings organized by focus area. 1. Code Quality & Architecture ⭐Strengths:
Issues: Minor: Inconsistent attribute batching (Low Priority)File: Gemini's review correctly identified that attributes could be batched more efficiently. There's an inconsistency in the implementation:
However, there's a minor style inconsistency: Recommendation: Consider standardizing the pattern across all three methods to use the slice-building approach when there are conditional attributes. This is a nice-to-have, not critical. 2. Error Handling & Resilience ✅Strengths:
No issues found - error handling is solid. 3. Security 🔍Observations: Potential sensitive data in traces (Medium Priority)File: The tracing implementation captures several pieces of data that could be considered sensitive in certain contexts:
Analysis: In the context of a blockchain node, this data is typically public. However:
Recommendation: Document in code comments or security documentation what trace data is captured and whether it's considered sensitive in your deployment model. This is informational - the current implementation is reasonable for a blockchain node. 4. Performance & Resource Efficiency ⚡Concerns: Minor allocation overhead from conditional attributes (Low Priority)Files: The conditional attribute building using attributes := []attribute.KeyValue{
attribute.String("payload_status", result.PayloadStatus.Status),
}
if result.PayloadID != nil {
attributes = append(attributes, attribute.String("payload_id", result.PayloadID.String()))
}Impact: Minimal - these are cold paths (RPC calls), and the allocations are tiny. The code prioritizes readability over micro-optimization, which is appropriate. Recommendation: No change needed unless profiling reveals this as a hotspot (unlikely). The readability is more valuable. Context overheadEach traced call adds span creation/end overhead. Given that these wrap RPC calls (which are expensive), the relative overhead is negligible. No action needed - performance impact is acceptable. 5. Testing & Reliability
|
| Category | Status | Critical Issues |
|---|---|---|
| Code Quality & Architecture | ✅ Excellent | 0 |
| Error Handling | ✅ Solid | 0 |
| Security | 0 (informational) | |
| Performance | ✅ Good | 0 |
| Testing | 1 - Missing tests | |
| Documentation | ⭐ Good | 0 |
Critical Action Items:
- Add tests for
tracedEngineRPCClient(high priority) - see link above - Consider documenting trace attribute contracts (nice-to-have)
- Consider security documentation for trace data (informational)
Overall Assessment: This is a clean, well-architected PR that successfully adds tracing without disrupting existing code. The main gap is test coverage for the tracing wrapper. The implementation follows Go best practices and OpenTelemetry conventions correctly.
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 interactions with the Ethereum execution engine by integrating OpenTelemetry tracing into the 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 tracing to the EngineClient by extracting an EngineRPCClient interface and implementing a tracing decorator. The changes are well-structured and effectively add observability to the engine RPC calls. My feedback focuses on a minor optimization in the tracing implementation to improve efficiency and code style by batching attribute setting.
execution/evm/engine_rpc_tracing.go
Outdated
| span.SetAttributes( | ||
| attribute.String("payload_status", result.PayloadStatus.Status), | ||
| attribute.Bool("has_payload_id", result.PayloadID != nil), | ||
| ) | ||
| if result.PayloadStatus.LatestValidHash != nil { | ||
| span.SetAttributes(attribute.String("latest_valid_hash", result.PayloadStatus.LatestValidHash.Hex())) | ||
| } |
There was a problem hiding this comment.
The attributes for the span are being set in two separate calls to span.SetAttributes. It would be more efficient and readable to gather all attributes into a slice and make a single call.
| span.SetAttributes( | |
| attribute.String("payload_status", result.PayloadStatus.Status), | |
| attribute.Bool("has_payload_id", result.PayloadID != nil), | |
| ) | |
| if result.PayloadStatus.LatestValidHash != nil { | |
| span.SetAttributes(attribute.String("latest_valid_hash", result.PayloadStatus.LatestValidHash.Hex())) | |
| } | |
| attrs := []attribute.KeyValue{ | |
| attribute.String("payload_status", result.PayloadStatus.Status), | |
| attribute.Bool("has_payload_id", result.PayloadID != nil), | |
| } | |
| if result.PayloadStatus.LatestValidHash != nil { | |
| attrs = append(attrs, attribute.String("latest_valid_hash", result.PayloadStatus.LatestValidHash.Hex())) | |
| } | |
| span.SetAttributes(attrs...) |
execution/evm/engine_rpc_tracing.go
Outdated
| span.SetAttributes( | ||
| attribute.String("payload_status", result.Status), | ||
| ) | ||
| if result.LatestValidHash != nil { | ||
| span.SetAttributes(attribute.String("latest_valid_hash", result.LatestValidHash.Hex())) | ||
| } |
There was a problem hiding this comment.
The attributes for the span are being set in two separate calls to span.SetAttributes. It's more efficient to collect all attributes and make a single call. This is similar to a previous comment.
| span.SetAttributes( | |
| attribute.String("payload_status", result.Status), | |
| ) | |
| if result.LatestValidHash != nil { | |
| span.SetAttributes(attribute.String("latest_valid_hash", result.LatestValidHash.Hex())) | |
| } | |
| attrs := []attribute.KeyValue{ | |
| attribute.String("payload_status", result.Status), | |
| } | |
| if result.LatestValidHash != nil { | |
| attrs = append(attrs, attribute.String("latest_valid_hash", result.LatestValidHash.Hex())) | |
| } | |
| span.SetAttributes(attrs...) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2959 +/- ##
==========================================
- Coverage 58.81% 58.67% -0.15%
==========================================
Files 93 93
Lines 8863 8863
==========================================
- Hits 5213 5200 -13
- Misses 3062 3074 +12
- Partials 588 589 +1
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 extracts a small interface with methods to call into
engine_forkchoiceUpdatedV3,engine_getPayloadV4andengine_newPayloadV4.Creates an implementation with the existing logic, and then a tracing wrapper implementation.
In this case, each of these will be child spans as they use the existing context which will have a parent span.
We will get traces like