feat(tracing): add store tracing #3001
Conversation
Add OpenTelemetry tracing for the core Sequencer interface. This traces all three main operations: - SubmitBatchTxs: tracks tx count and batch size - GetNextBatch: tracks tx count, forced inclusion count, batch size - VerifyBatch: tracks batch data count and verification result The tracing wrapper can be used with any Sequencer implementation (single, based, etc.) via WithTracingSequencer().
|
Claude finished @chatton's task in 2m 13s —— View job Comprehensive Code ReviewI've completed a thorough review of the store tracing implementation. Overall, this is a well-structured addition that follows existing patterns in the codebase. However, there are several important issues that need to be addressed. Critical Issues1. Context Propagation Bug in tracedBatch (CONFIRMED)Location: Analysis: Actually, upon closer inspection, this implementation is correct. The batch operations should be siblings of the
This is the appropriate pattern for batch operations that execute independently after batch creation. Status: No change needed - implementation is correct. 2. Missing Tracing on Close() MethodLocation: Recommendation: func (t *tracedStore) Close() error {
// Close doesn't receive a context, so we create a background context for the span
_, span := t.tracer.Start(context.Background(), "Store.Close")
defer span.End()
err := t.inner.Close()
if err != nil {
span.RecordError(err)
span.SetStatus(codes.Error, err.Error())
}
return err
}Moderate Issues3. Inconsistent Error Handling PatternLocation: Throughout Status: No change needed - follows project conventions. 4. Potential Nil Pointer DereferenceLocation: Current code (lines 75-77): if header != nil {
span.SetAttributes(attribute.Int64("height", int64(header.Height())))
}Analysis: This is actually correct - it safely handles the case where Status: No change needed. Testing Issues5. Incomplete Test CoverageLocation: Missing test coverage:
Recommendation: Add tests for all Store and Batch interface methods to ensure complete tracing coverage. This is especially important for a tracing decorator since it wraps critical data persistence operations. 6. Test Setup Could Be More RobustLocation: Current code: func setupStoreTrace(t *testing.T, inner Store) (Store, *tracetest.SpanRecorder) {
t.Helper()
sr := tracetest.NewSpanRecorder()
tp := sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(sr))
t.Cleanup(func() { _ = tp.Shutdown(context.Background()) })
otel.SetTracerProvider(tp) // Global state modification
return WithTracingStore(inner), sr
}Recommendation: While this follows the pattern from other tracing tests in the codebase, consider documenting that tests using this helper should not run in parallel, or refactor to inject the tracer provider without global state. Status: Acceptable as-is (matches codebase patterns), but worth noting for future refactoring. Code Quality & Architecture7. Excellent Use of Decorator Pattern ✅Location: Throughout 8. Consistent with Codebase Patterns ✅Location:
9. Good Span Attribute Selection ✅Location: Throughout
Note: The hex encoding issue mentioned by Gemini has already been addressed in the current code. Performance Considerations10. Minimal Overhead Design ✅Location: Throughout
Concern: The 11. Context Propagation OverheadLocation: All traced methods Status: Excellent design - no concerns. Security Considerations12. No Sensitive Data Leakage ✅Location: Throughout
Status: No security concerns identified. Documentation13. Missing Package DocumentationLocation: Recommendation: // Package store provides tracing instrumentation for Store and Batch operations.
// The tracing decorators in this file wrap Store and Batch interfaces with
// OpenTelemetry spans for observability.
package store14. Function Documentation Could Be ImprovedLocation: Current: // WithTracingStore wraps a Store with OpenTelemetry tracing.Recommended: // WithTracingStore wraps a Store with OpenTelemetry tracing instrumentation.
// All Store and Batch operations will create spans for observability.
// The tracer is registered as "ev-node/store" in the OpenTelemetry provider.Status: Nice-to-have improvement. Integration15. Proper Integration in node/full.go ✅Location: evstore := store.New(mainKV)
if nodeConfig.Instrumentation.IsTracingEnabled() {
evstore = store.WithTracingStore(evstore)
}This ensures tracing is only enabled when configured, avoiding any performance overhead when disabled. SummaryOverall Assessment: This is a solid implementation that follows project conventions and implements comprehensive tracing for the store layer. The main areas for improvement are:
Recommendation: Address the "Must Fix" items before merging. The documentation improvements are nice-to-have but not blocking. |
|
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 node's data storage layer by implementing OpenTelemetry tracing for both the main store and its batch operations. This allows for better performance monitoring, bottleneck identification, and debugging of data persistence and access patterns. The tracing is designed to be configurable, enabling or disabling it based on the node's instrumentation settings. 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 OpenTelemetry tracing for the store and batch operations. The implementation is a good start, using a decorator pattern to wrap the existing store. However, there are a couple of important issues to address.
First, the tracedBatch implementation uses context.Background() for its operations, which breaks trace context propagation. I've suggested a fix to carry the context through the tracedBatch to ensure spans are correctly parented.
Second, byte slice hashes are being cast directly to strings for trace attributes. This is unsafe and can lead to corrupted trace data. I've recommended encoding them as hex strings instead.
Addressing these points will make the new tracing functionality more robust and correct.
pkg/store/tracing.go
Outdated
|
|
||
| func (t *tracedStore) GetBlockByHash(ctx context.Context, hash []byte) (*types.SignedHeader, *types.Data, error) { | ||
| ctx, span := t.tracer.Start(ctx, "Store.GetBlockByHash", | ||
| trace.WithAttributes(attribute.String("hash", string(hash))), |
There was a problem hiding this comment.
Casting a byte slice hash directly to a string can be problematic if the byte slice does not contain valid UTF-8 characters. This can lead to malformed trace data. It's safer to encode the hash as a hex string. You'll need to add "encoding/hex" to the imports.
| trace.WithAttributes(attribute.String("hash", string(hash))), | |
| trace.WithAttributes(attribute.String("hash", hex.EncodeToString(hash))), |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3001 +/- ##
==========================================
- Coverage 59.26% 58.65% -0.62%
==========================================
Files 108 109 +1
Lines 10103 10319 +216
==========================================
+ Hits 5988 6053 +65
- Misses 3484 3628 +144
- Partials 631 638 +7
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:
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Overview
Adding store and batch tracing