Skip to content

Conversation

@chatton
Copy link
Contributor

@chatton chatton commented Jan 20, 2026

Overview

This PR ensures that if a http request is made to ev-node, if that included a parent span, the ev-node trace becomes a child span instead of being a new parent span.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 20, 2026

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

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedJan 22, 2026, 8:18 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 significantly enhances the observability of the ev-node application by integrating OpenTelemetry for distributed tracing. The primary goal is to ensure that ev-node can seamlessly participate in distributed traces, allowing external services to propagate their trace context into ev-node's internal operations. This provides a more comprehensive and connected view of how requests are processed across different components and services, which is crucial for debugging and performance monitoring in a distributed system.

Highlights

  • Distributed Tracing Integration: The pull request introduces OpenTelemetry distributed tracing capabilities to the ev-node application, enabling better observability of request flows.
  • HTTP Trace Context Propagation: It implements HTTP middleware to extract W3C Trace Context headers from incoming requests, ensuring that ev-node operations become child spans of existing traces rather than initiating new ones.
  • Store Operations Tracing: Tracing has been added to the store package, wrapping Store and Batch operations with OpenTelemetry spans to monitor data access and persistence.

🧠 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 20, 2026

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


Code Review Complete ✅

I've performed a comprehensive review of the HTTP trace propagation implementation. The changes correctly implement W3C Trace Context propagation for incoming HTTP requests.


Overview

This PR successfully enables distributed tracing by extracting trace context from incoming HTTP requests. When a request includes traceparent/tracestate headers, ev-node operations become child spans of the external trace rather than starting new root traces.

Files Changed:

  • pkg/rpc/server/server.go - Wires up the middleware
  • pkg/telemetry/http_extract.go - Implements extraction logic
  • pkg/telemetry/http_extract_test.go - Test coverage

Detailed Review

Code Quality & Architecture

Strengths:

  • Clean separation of concerns - middleware is in the telemetry package
  • Proper use of OpenTelemetry's propagation API
  • Correctly uses otel.GetTextMapPropagator() which respects the global propagator configured in InitTracing (pkg/telemetry/tracing.go:59-62)
  • Middleware is conditionally applied only when tracing is enabled (server.go:418-420)
  • Integration point is well-placed - wraps the entire handler tree before h2c wrapper

Minor observations:

  • The middleware is simple and focused, which is appropriate for this use case
  • Follows idiomatic Go middleware patterns

Error Handling & Resilience

Strengths:

  • The extraction operation is inherently safe - prop.Extract() gracefully handles missing or invalid headers
  • No panics possible in this code path
  • If extraction fails, the original context is effectively used (no parent span)

Considerations:

  • No error logging if headers are malformed, but this is acceptable since:
    • OpenTelemetry's propagator silently ignores invalid headers by design
    • This prevents log spam from malicious/buggy clients
    • Valid use case: requests without trace headers should not generate warnings

Security

Strengths:

  • No security vulnerabilities introduced
  • Trace context extraction is read-only and doesn't modify request state
  • No sensitive data exposure risk
  • OpenTelemetry's W3C TraceContext propagator validates header formats

Good practices:

  • Middleware doesn't log header values (avoids potential PII in logs)
  • Uses standard, vetted OpenTelemetry libraries

Performance & Resource Efficiency

Strengths:

  • Minimal overhead - just header parsing and context wrapping
  • No allocations beyond the new context object
  • OpenTelemetry propagators are optimized for performance
  • Conditional application prevents overhead when tracing is disabled

Observations:

  • Middleware position is optimal - applied after routing decisions minimizes unnecessary work
  • No blocking operations or network calls in the hot path

Testing & Reliability

Strengths:

  • Excellent test coverage with two critical scenarios:
    1. TestExtractTraceContext_WithParentTrace - Verifies child span creation with matching trace ID
    2. TestExtractTraceContext_WithoutParentTrace - Ensures graceful handling when no headers present
  • Tests use proper OpenTelemetry test utilities (tracetest.NewSpanRecorder)
  • Assertions verify the core requirement: trace ID continuity

Suggestions for future enhancement:

  • Could add test for malformed headers (though current behavior is correct - silent ignore)
  • Could add integration test showing full request → store operation trace chain
  • Consider testing with different propagation formats if supporting multiple (though W3C is the standard)

Coverage note:
Codecov reports 80% patch coverage with 2 lines missing. Looking at server.go:418-420, the uncovered lines are likely:

  • Line 419: handler = telemetry.ExtractTraceContext(mux) - Not hit if tracing disabled in test
  • This is acceptable as the test file tests the middleware directly

Documentation & Developer Experience

Strengths:

  • Excellent godoc comment on ExtractTraceContext explaining:
    • What it does (extracts W3C headers)
    • Which headers (traceparent, tracestate)
    • Why it matters (enables child span creation)
  • Clear test names describe scenarios
  • Code is self-documenting with appropriate comments

Suggestions:

  • Consider adding a note in CLAUDE.md or docs about how to send traced requests to ev-node
  • Example: curl -H "traceparent: 00-<trace-id>-<span-id>-01" http://localhost:8080/...

Response to Gemini Review

Note: Gemini flagged the tracedBatch methods in pkg/store/tracing.go for using context.Background(). However, this review is incorrect.

Looking at the actual implementation (pkg/store/tracing.go:230-243):

return &tracedBatch{
    inner:  batch,
    tracer: t.tracer,
    ctx:    ctx,  // ← Context is stored here
}, nil

The batch operations correctly use b.ctx (line 246, 262, 278, etc.), NOT context.Background(). The context from NewBatch is properly stored and propagated to all batch operations. Trace continuity is maintained correctly.


Summary

This is a well-implemented, production-ready feature that correctly enables distributed tracing for ev-node. The implementation is:

  • ✅ Architecturally sound
  • ✅ Secure and safe
  • ✅ Well-tested
  • ✅ Performant
  • ✅ Properly documented

No blocking issues identified. The code follows Go and OpenTelemetry best practices.

Recommendation

Approve for merge. This PR successfully achieves its stated goal of making ev-node traces children of external parent spans when trace context is provided.


Optional future enhancements (not blocking):

  1. Add integration test showing full trace propagation through the stack
  2. Document trace context usage in API documentation
  3. Consider adding metrics for trace context extraction success/failure rates

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 OpenTelemetry tracing capabilities to the ev-node application, specifically for store operations and HTTP request handling. The changes correctly integrate tracing middleware for incoming HTTP requests and wrap store operations with tracing spans. New files pkg/store/tracing.go and pkg/telemetry/http_extract.go implement the core tracing logic, along with corresponding unit tests. Overall, the implementation is a good step towards enhancing observability. However, there is a significant issue regarding context propagation within batch operations in the store, which needs to be addressed to ensure proper trace continuity.

@codecov
Copy link

codecov bot commented Jan 20, 2026

Codecov Report

❌ Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.51%. Comparing base (a396437) to head (22dff4d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/rpc/server/server.go 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3000   +/-   ##
=======================================
  Coverage   58.51%   58.51%           
=======================================
  Files         110      111    +1     
  Lines       10396    10405    +9     
=======================================
+ Hits         6083     6089    +6     
- Misses       3669     3672    +3     
  Partials      644      644           
Flag Coverage Δ
combined 58.51% <80.00%> (+<0.01%) ⬆️

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.

@chatton chatton marked this pull request as ready for review January 22, 2026 08:18
@chatton chatton requested a review from tac0turtle January 22, 2026 09:01
@tac0turtle tac0turtle added this pull request to the merge queue Jan 22, 2026
Merged via the queue into main with commit 9348732 Jan 22, 2026
30 checks passed
@tac0turtle tac0turtle deleted the tracing-part-14-http-propagation branch January 22, 2026 09:37
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