feat(tracing): adding block production tracing #2980
Conversation
|
Claude finished @chatton's task in 1m 54s —— View job Code Review CompleteI've completed a comprehensive review of the block production tracing implementation. Overall, this is a well-designed and well-executed PR that successfully integrates OpenTelemetry tracing using a clean decorator pattern. The code quality is high, tests are thorough, and the implementation follows Go best practices. SummaryStrengths:
Issues Found: Critical IssuesNone found. High Priority Issues1. Context Propagation Issue in Location: The Checking if err := e.blockProducer.ValidateBlock(ctx, currentState, header, data); err != nil {✅ Good news: The context IS being properly propagated! The interface already has Verification: The trace hierarchy should work correctly because:
No action needed - this is already correctly implemented. Medium Priority Issues2. Missing Context Propagation in One Call Path Location: The func (e *Executor) ValidateBlock(_ context.Context, lastState types.State, header *types.SignedHeader, data *types.Data) error {The context parameter is explicitly ignored with
Recommendation: Change the signature to accept func (e *Executor) ValidateBlock(ctx context.Context, lastState types.State, header *types.SignedHeader, data *types.Data) error {Impact: Low - cosmetic improvement for future maintainability. 3. Tracer Name Convention Location: tracer: otel.Tracer("ev-node/block-executor"),The tracer name uses Recommendation: Consider using a consistent naming convention that aligns with the module path or a documented convention: tracer: otel.Tracer("github.com/evstack/ev-node/block/executor"),Impact: Low - affects observability consistency but not functionality. Low Priority / Suggestions4. Span Attribute Consistency Location: Various in The span attributes use different naming conventions:
Recommendation: Adopt OpenTelemetry semantic conventions or establish a consistent pattern:
Reference: OpenTelemetry Semantic Conventions Impact: Low - improves observability consistency. 5. Error Wrapping in Traced Methods Observation: The traced methods don't wrap errors with additional context before returning them. While the errors are recorded in spans, the returned error messages don't indicate they came from the traced layer. This is actually correct behavior - the decorator pattern should be transparent and not modify error messages. The spans already capture the necessary context. ✅ No action needed - current implementation is correct. Code Quality & Architecture ✅
Error Handling & Resilience ✅
Security ✅
Performance & Resource Efficiency
|
|
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 significantly enhances the observability of the block production process by integrating OpenTelemetry tracing. It achieves this through a clean architectural change, introducing a 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
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## tracing-part-6 #2980 +/- ##
==================================================
+ Coverage 58.11% 58.41% +0.29%
==================================================
Files 99 100 +1
Lines 9543 9619 +76
==================================================
+ Hits 5546 5619 +73
- Misses 3388 3389 +1
- Partials 609 611 +2
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:
|
![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 the block production process, which is a great addition for observability. The implementation uses a clean decorator pattern by defining a BlockProducer interface and wrapping the Executor with a tracedBlockProducer. This approach is well-executed and enhances modularity. Additionally, the changes include several important fixes for context propagation, improving the overall robustness of the code. The new tracing logic is also well-tested. I have one suggestion to improve trace context propagation for the ValidateBlock step to ensure the trace hierarchy is correctly maintained.
block/internal/executing/tracing.go
Outdated
| func (t *tracedBlockProducer) ValidateBlock(lastState types.State, header *types.SignedHeader, data *types.Data) error { | ||
| _, span := t.tracer.Start(context.Background(), "BlockExecutor.ValidateBlock", | ||
| trace.WithAttributes( | ||
| attribute.Int64("block.height", int64(header.Height())), | ||
| ), | ||
| ) | ||
| defer span.End() | ||
|
|
||
| err := t.inner.ValidateBlock(lastState, header, data) | ||
| if err != nil { | ||
| span.RecordError(err) | ||
| span.SetStatus(codes.Error, err.Error()) | ||
| } | ||
| return err | ||
| } |
There was a problem hiding this comment.
Using context.Background() here creates a new root span for ValidateBlock, which means it won't appear as a child of the ProduceBlock span in traces. This breaks the trace context propagation.
To fix this, the ValidateBlock method should accept a context.Context argument. This will require updating the BlockProducer interface and all its implementations (Executor, tracedBlockProducer, and mockBlockProducer in tests).
I've provided a suggestion for this file. You'll also need to update:
block/internal/executing/block_producer.go: Addctx context.ContexttoValidateBlockin the interface.block/internal/executing/executor.go: Addctxto theValidateBlockimplementation and pass it fromProduceBlock.block/internal/executing/tracing_test.go: Update themockBlockProducerto match the new interface.
This change will ensure ValidateBlock spans are correctly parented under ProduceBlock spans.
| func (t *tracedBlockProducer) ValidateBlock(lastState types.State, header *types.SignedHeader, data *types.Data) error { | |
| _, span := t.tracer.Start(context.Background(), "BlockExecutor.ValidateBlock", | |
| trace.WithAttributes( | |
| attribute.Int64("block.height", int64(header.Height())), | |
| ), | |
| ) | |
| defer span.End() | |
| err := t.inner.ValidateBlock(lastState, header, data) | |
| if err != nil { | |
| span.RecordError(err) | |
| span.SetStatus(codes.Error, err.Error()) | |
| } | |
| return err | |
| } | |
| func (t *tracedBlockProducer) ValidateBlock(ctx context.Context, lastState types.State, header *types.SignedHeader, data *types.Data) error { | |
| ctx, span := t.tracer.Start(ctx, "BlockExecutor.ValidateBlock", | |
| trace.WithAttributes( | |
| attribute.Int64("block.height", int64(header.Height())), | |
| ), | |
| ) | |
| defer span.End() | |
| err := t.inner.ValidateBlock(ctx, lastState, header, data) | |
| if err != nil { | |
| span.RecordError(err) | |
| span.SetStatus(codes.Error, err.Error()) | |
| } | |
| return err | |
| } |
Overview