-
Notifications
You must be signed in to change notification settings - Fork 245
chore: inject W3C headers into engine client and eth client #2958
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
Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
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 enhances the system's observability by integrating OpenTelemetry W3C Trace Context header propagation into the Ethereum and Engine RPC clients. When tracing is active, this change ensures that tracing information is automatically propagated across HTTP calls, enabling better visibility into distributed transaction flows. The underlying RPC client creation logic has been updated to support this new configuration. 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
|
|
Claude finished @chatton's task in 1m 47s —— View job Code Review CompleteI've performed a comprehensive code review of this PR. Overall, the implementation is solid and follows good Go practices. Below are my findings across the six focus areas: Todo List
SummaryThis PR successfully adds W3C trace context propagation to the Engine and Eth RPC clients. The implementation follows OpenTelemetry best practices and integrates cleanly with the existing codebase. However, there are several improvements needed around nil handling, testing, and documentation. Detailed Findings1. Code Quality & Architecture ✅Strengths:
Issues:
2. Error Handling & Resilience
|
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.
Code Review
This pull request introduces W3C trace context propagation for the engine and eth clients by injecting a custom HTTP client when tracing is enabled. The implementation is mostly correct, introducing a new otelRoundTripper to handle header injection. I've provided a couple of suggestions: one is a high-severity fix for a potential panic in the new telemetry helper, and the other is a medium-severity suggestion to simplify the code in the execution client constructor by removing a redundant slice copy.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2958 +/- ##
==========================================
- Coverage 58.90% 58.67% -0.24%
==========================================
Files 92 93 +1
Lines 8846 8863 +17
==========================================
- Hits 5211 5200 -11
- Misses 3046 3074 +28
Partials 589 589
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:
|
|
@tac0turtle sorry ended up making a change in the 3rd PR and just pulled it into this one to make the next diff smaller. |
Overview
Part of #2956
Injects W3C headers into engine client and eth client, this will allow us to set up proper traces (with some changes to ev-reth to respect them)
I will add additional spans in a follow up.