Skip to content

Conversation

@chatton
Copy link
Contributor

@chatton chatton commented Jan 7, 2026

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.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2026

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedJan 8, 2026, 11:33 AM

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @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

  • W3C Trace Context Header Injection: Introduced the capability to inject W3C Trace Context headers (traceparent, tracestate) into outbound HTTP requests made by the Ethereum and Engine RPC clients when OpenTelemetry tracing is enabled. This facilitates distributed tracing across services.
  • RPC Client Configuration Flexibility: Refactored the createExecutionClient and NewEngineExecutionClient functions to accept variadic rpc.ClientOption arguments, allowing for more flexible configuration of underlying RPC clients, including custom HTTP transports.
  • New Telemetry Component: Added a new http_propagation.go file within the pkg/telemetry package, which contains the otelRoundTripper and NewPropagatingHTTPClient for handling W3C header injection.

🧠 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 Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@claude
Copy link
Contributor

claude bot commented Jan 7, 2026

Claude finished @chatton's task in 1m 47s —— View job


Code Review Complete

I'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

  • Read and analyze all changed files
  • Review code quality and architecture
  • Check error handling and resilience
  • Assess security implications
  • Evaluate performance and resource efficiency
  • Review testing coverage and quality
  • Check documentation and developer experience
  • Post comprehensive review feedback

Summary

This 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 Findings

1. Code Quality & Architecture ✅

Strengths:

  • Clean separation of concerns with dedicated http_propagation.go file
  • Proper use of the decorator pattern (otelRoundTripper)
  • Good integration with existing RPC client configuration via variadic ClientOption
  • Consistent use of W3C Trace Context and Baggage propagators

Issues:

  • HIGH: The slice copy on line 197-198 of execution/evm/execution.go is necessary to avoid shared backing array issues, contrary to Gemini's suggestion. This is correct defensive programming when appending to a slice that was passed as a parameter.

2. Error Handling & Resilience ⚠️

Critical Issue - Nil Pointer Panic:
Location: pkg/telemetry/http_propagation.go:20

The function will panic if base is nil when calling t.base.RoundTrip(req). While the current usage always passes http.DefaultTransport, defensive programming suggests handling the nil case:

func NewPropagatingHTTPClient(base http.RoundTripper) *http.Client {
    if base == nil {
        base = http.DefaultTransport
    }
    prop := propagation.NewCompositeTextMapPropagator(
        propagation.TraceContext{},
        propagation.Baggage{},
    )
    return &http.Client{
        Transport: &otelRoundTripper{
            base:       base,
            propagator: prop,
        },
    }
}

Fix this →

Resilience:

  • ✅ Proper nil check for t.propagator in RoundTrip (line 17)
  • ✅ Context propagation handled correctly through HTTP requests

3. Security 🔒

Good:

  • ✅ No credential leakage in trace headers
  • ✅ JWT authentication preserved and not interfered with
  • ✅ HTTP transport security not weakened
  • ✅ Trace context properly scoped to request context

Minor Observation:

  • The tracing system could theoretically expose internal service topology through trace IDs, but this is expected behavior for distributed tracing and not a vulnerability in authorized environments

4. Performance & Resource Efficiency ⚡

Concerns:

  1. Propagator Allocation (http_propagation.go:26-29):

    • A new CompositeTextMapPropagator is created on every NewPropagatingHTTPClient call
    • Consider using a package-level singleton:
    var defaultPropagator = propagation.NewCompositeTextMapPropagator(
        propagation.TraceContext{},
        propagation.Baggage{},
    )
    
    func NewPropagatingHTTPClient(base http.RoundTripper) *http.Client {
        // ... nil check ...
        return &http.Client{
            Transport: &otelRoundTripper{
                base:       base,
                propagator: defaultPropagator,
            },
        }
    }
  2. Slice Copy (execution.go:197-198):

    • The copy is necessary and correctly implemented
    • Performance impact is negligible (2 options max in practice)

Good:

  • ✅ No blocking operations in hot path
  • ✅ Header injection is O(1) operation
  • ✅ Transport reused across requests

5. Testing & Reliability ❌

Critical Gap - 0% Coverage:

Codecov reports 18 lines of new code with zero test coverage:

  • pkg/telemetry/http_propagation.go: 14 lines uncovered
  • pkg/telemetry/tracing.go: 4 lines uncovered

Missing Test Cases:

  1. Unit tests for http_propagation.go:

    // Test that headers are injected when context has active span
    // Test that RoundTrip succeeds with nil propagator
    // Test that RoundTrip succeeds with nil base (after fix)
    // Test that W3C headers match expected format
  2. Integration test for tracing flow:

    • Verify headers propagate through Engine API calls
    • Verify headers propagate through Eth API calls
    • Test with tracingEnabled=true in execution_test.go

Recommendation:
Create pkg/telemetry/http_propagation_test.go with comprehensive unit tests. The existing execution/evm/test/execution_test.go should be extended to test with tracingEnabled=true.

Add tests →

6. Documentation & Developer Experience 📚

Good:

  • ✅ Clear function comments on NewPropagatingHTTPClient
  • ✅ Inline comments explaining W3C headers
  • ✅ Updated constructor documentation for NewEngineExecutionClient

Improvements Needed:

  1. Package Documentation:
    Add package comment to http_propagation.go:

    // Package telemetry provides OpenTelemetry integration utilities.
    // This includes W3C Trace Context propagation for HTTP clients and
    // trace provider initialization.
  2. Configuration Documentation:
    The relationship between InstrumentationConfig.Tracing flag and header injection should be documented in CLAUDE.md or a tracing guide.

  3. Example Usage:
    Consider adding example in godoc:

    // Example:
    //   client := telemetry.NewPropagatingHTTPClient(http.DefaultTransport)
    //   resp, err := client.Get("http://example.com")
    //   // Traceparent header automatically injected if context has active span

Additional Observations

Consistency Enhancement

In tracing.go:59-62, you updated the global propagator to use CompositeTextMapPropagator. This is good, but consider whether http_propagation.go should use the global propagator from otel.GetTextMapPropagator() instead of creating its own:

func NewPropagatingHTTPClient(base http.RoundTripper) *http.Client {
    if base == nil {
        base = http.DefaultTransport
    }
    return &http.Client{
        Transport: &otelRoundTripper{
            base:       base,
            propagator: otel.GetTextMapPropagator(), // Use global propagator
        },
    }
}

This ensures consistency with the globally configured propagation strategy.

Go Module Updates

The PR updates several go.mod and go.sum files with OpenTelemetry dependencies. These look correct, but ensure:

  • go mod tidy has been run on all modules
  • ✅ No indirect dependencies conflict
  • ⚠️ Consider documenting the new dependencies in a changelog or migration guide

Recommendations Priority

  1. HIGH: Add nil check for base parameter in NewPropagatingHTTPClient
  2. HIGH: Add unit tests for http_propagation.go (0% coverage is unacceptable)
  3. MEDIUM: Consider using singleton propagator to reduce allocations
  4. MEDIUM: Add integration test with tracingEnabled=true
  5. LOW: Add package documentation
  6. LOW: Consider using global propagator for consistency

Compliance with Repository Guidelines

Based on CLAUDE.md:

  • ✅ Follows Go conventions (gofmt, naming)
  • ✅ Uses interfaces appropriately
  • ✅ No external dependencies in core package
  • ⚠️ Missing tests (violates "Add tests following existing patterns")
  • ✅ No security issues introduced
  • ✅ Proper error wrapping

Before merge: The test coverage gap should be addressed to meet the project's quality standards (make test-all should include these new code paths).


Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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
Copy link

codecov bot commented Jan 7, 2026

Codecov Report

❌ Patch coverage is 0% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.67%. Comparing base (4ce10cd) to head (7d33625).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/telemetry/http_propagation.go 0.00% 14 Missing ⚠️
pkg/telemetry/tracing.go 0.00% 4 Missing ⚠️
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              
Flag Coverage Δ
combined 58.67% <0.00%> (-0.24%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Base automatically changed from cian/add-tracing to main January 8, 2026 10:33
@chatton chatton marked this pull request as ready for review January 8, 2026 11:00
@chatton chatton marked this pull request as draft January 8, 2026 11:03
tac0turtle
tac0turtle previously approved these changes Jan 8, 2026
@tac0turtle tac0turtle dismissed their stale review January 8, 2026 11:12

ah saw it was moved back to draft

@chatton chatton marked this pull request as ready for review January 8, 2026 11:12
@chatton
Copy link
Contributor Author

chatton commented Jan 8, 2026

@tac0turtle sorry ended up making a change in the 3rd PR and just pulled it into this one to make the next diff smaller.

@chatton chatton added this pull request to the merge queue Jan 8, 2026
Merged via the queue into main with commit 41cac58 Jan 8, 2026
27 of 28 checks passed
@chatton chatton deleted the cian/add-tracing-part-2 branch January 8, 2026 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants