Skip to content

Conversation

@chatton
Copy link
Contributor

@chatton chatton commented Jan 19, 2026

Overview

ref #2956

Add tracing for forced inclusion

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().
@github-actions
Copy link
Contributor

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

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedJan 19, 2026, 2:29 PM

@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 integrates OpenTelemetry tracing into the mechanism responsible for retrieving forced inclusion transactions from the Data Availability (DA) layer. By introducing a tracing wrapper and refactoring the retriever into an interface, the system gains improved visibility into the performance and flow of forced inclusion events, allowing for easier debugging and monitoring without altering the core retrieval logic. The tracing is enabled conditionally based on the application's instrumentation configuration.

Highlights

  • Tracing Integration: Introduced OpenTelemetry tracing for the ForcedInclusionRetriever to enhance observability of forced inclusion transaction retrieval.
  • Interface Refactoring: The ForcedInclusionRetriever has been refactored into an interface, with the concrete implementation renamed to forcedInclusionRetriever.
  • Tracing Wrapper: A new tracedForcedInclusionRetriever struct and withTracingForcedInclusionRetriever function were added to conditionally wrap the base retriever with tracing capabilities.

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

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


Code Review Complete ✓

I've completed a comprehensive review of this PR. Overall, the implementation is clean, well-structured, and follows idiomatic Go patterns. The changes successfully add OpenTelemetry tracing to the forced inclusion retriever using the decorator pattern, which aligns well with the existing tracing approach in the codebase.


Summary

This PR introduces tracing instrumentation for forced inclusion transaction retrieval by:

  1. Converting ForcedInclusionRetriever from a concrete struct to an interface
  2. Adding a tracedForcedInclusionRetriever wrapper that implements the interface
  3. Conditionally wrapping the base retriever based on instrumentation configuration

The implementation is solid with no blocking issues. I have a few minor suggestions for enhancement below.


✅ Strengths

Architecture & Design:

  • Clean use of the decorator/wrapper pattern consistent with other tracing implementations in the codebase
  • Proper interface extraction following Go best practices (small, focused interface)
  • Non-intrusive: tracing is purely additive and doesn't modify core logic
  • Configuration-driven: tracing is enabled based on cfg.Instrumentation.IsTracingEnabled()

Code Quality:

  • Follows idiomatic Go style with proper naming conventions
  • Compile-time interface verification using var _ ForcedInclusionRetriever = (*tracedForcedInclusionRetriever)(nil)
  • Clean error handling with proper span status updates
  • Appropriate span attributes provide good observability

Consistency:

  • Matches the pattern used in DA retriever and DA client tracing
  • Properly integrates with the existing OpenTelemetry infrastructure

💡 Minor Suggestions

1. Consider Adding Span Events for Key Operations

The current implementation tracks success/failure well, but adding span events could provide richer debugging context for long-running operations.

Location: block/internal/da/forced_inclusion_tracing.go:34-38

Current:

event, err := t.inner.RetrieveForcedIncludedTxs(ctx, daHeight)
if err != nil {
    span.RecordError(err)
    span.SetStatus(codes.Error, err.Error())
    return event, err
}

Suggestion:

event, err := t.inner.RetrieveForcedIncludedTxs(ctx, daHeight)
if err != nil {
    span.RecordError(err)
    span.SetStatus(codes.Error, err.Error())
    span.AddEvent("forced_inclusion_retrieval_failed")
    return event, err
}

span.AddEvent("forced_inclusion_retrieval_completed")

Rationale: Span events are useful for marking significant points in a trace timeline and can help diagnose performance issues or understand operation flow, especially when retrievals are slow due to cache misses or sync fetching.


2. Add Attribute for Epoch Boundary Detection

Since the retriever only fetches at epoch boundaries, it would be valuable to trace whether a given height represents an epoch end.

Location: block/internal/da/forced_inclusion_tracing.go:41-47

Suggestion:

if event != nil {
    span.SetAttributes(
        attribute.Int64("event.start_da_height", int64(event.StartDaHeight)),
        attribute.Int64("event.end_da_height", int64(event.EndDaHeight)),
        attribute.Int("event.tx_count", len(event.Txs)),
        attribute.Bool("event.at_epoch_end", len(event.Txs) > 0 || event.StartDaHeight != event.EndDaHeight),
    )
}

Rationale: This helps distinguish between "no transactions because we're not at epoch end" vs "no transactions at epoch end". It provides better observability for understanding forced inclusion patterns.


3. Trace Empty Transaction Sets More Explicitly

When event.Txs is empty, it's useful to know why - either we're not at an epoch boundary, or there were no forced inclusion transactions at that epoch.

Location: block/internal/da/forced_inclusion_tracing.go:41-47

Suggestion:

if event != nil {
    attrs := []attribute.KeyValue{
        attribute.Int64("event.start_da_height", int64(event.StartDaHeight)),
        attribute.Int64("event.end_da_height", int64(event.EndDaHeight)),
        attribute.Int("event.tx_count", len(event.Txs)),
    }
    
    // Add context for empty transaction sets
    if len(event.Txs) == 0 {
        if event.StartDaHeight == event.EndDaHeight {
            attrs = append(attrs, attribute.String("empty_reason", "not_at_epoch_end"))
        } else {
            attrs = append(attrs, attribute.String("empty_reason", "no_transactions_in_epoch"))
        }
    }
    
    span.SetAttributes(attrs...)
}

Rationale: Makes traces more self-documenting and reduces the need to correlate with logs to understand behavior.


4. Consider Adding Cache Hit/Miss Information

The underlying retriever uses caching with fallback to sync fetching. Surfacing cache performance in traces would be valuable.

Location: This would require changes to the interface or event structure

Current limitation: The ForcedInclusionEvent doesn't carry cache hit/miss statistics from the underlying retrieval process.

Suggestion (if feasible): Consider adding cache statistics to ForcedInclusionEvent or creating a separate metrics struct:

type ForcedInclusionEvent struct {
    Timestamp     time.Time
    StartDaHeight uint64
    EndDaHeight   uint64
    Txs           [][]byte
    // New fields for observability
    CacheHits     int
    CacheMisses   int
}

Then in the tracing wrapper:

if event != nil {
    span.SetAttributes(
        // ... existing attributes
        attribute.Int("cache.hits", event.CacheHits),
        attribute.Int("cache.misses", event.CacheMisses),
        attribute.Float64("cache.hit_ratio", float64(event.CacheHits)/float64(event.CacheHits+event.CacheMisses)),
    )
}

Rationale: Cache performance directly impacts retrieval latency. Understanding cache behavior helps optimize prefetch window sizing and diagnose performance issues.

Note: This is a bigger change that affects the data structure and would require modifications to the base retriever implementation. Consider this for a future enhancement if observability into cache performance becomes important.


🔍 Additional Observations

Error Handling:

  • Error propagation is correct - errors are recorded in spans but don't affect the return behavior ✓
  • The implementation properly preserves context cancellation behavior ✓

Testing:

  • No tests were added for the tracing wrapper
  • Recommendation: Consider adding unit tests similar to other tracing wrappers in the codebase to verify:
    • Span creation and naming
    • Attribute setting for various scenarios (empty txs, errors, etc.)
    • Error recording behavior
    • Context propagation

Documentation:

  • The comment in block/public.go:80 correctly notes that "Tracing is automatically enabled when configured" ✓
  • The interface definition is well-documented ✓

Performance:

  • The decorator pattern adds minimal overhead (function call + defer) ✓
  • Attribute allocation is reasonable and bounded ✓
  • No goroutines or additional allocations that could cause issues ✓

📋 Checklist for Merge

  • ✅ Follows idiomatic Go patterns
  • ✅ Consistent with existing tracing implementations
  • ✅ No breaking changes to public API
  • ✅ Proper error handling
  • ✅ Documentation updated appropriately
  • ⚠️ Tests not included (consider adding)
  • ✅ Linting appears clean (CI passed)
  • ✅ No security concerns
  • ✅ No obvious performance issues

Final Verdict

Status: Approved with minor suggestions

The implementation is production-ready as-is. The suggestions above are enhancements that would improve observability but are not blockers. Consider implementing them in this PR or as follow-up improvements based on priority.

Great work maintaining consistency with the existing codebase patterns! 🎉


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 for the forced inclusion transaction retrieval process. This is achieved by refactoring ForcedInclusionRetriever from a struct into an interface and then applying the decorator pattern to wrap the core implementation with a tracing layer. The changes are well-structured, consistent with existing tracing patterns in the codebase, and correctly propagate the tracing context. The implementation is clean, and I found no issues with the proposed changes.

@codecov
Copy link

codecov bot commented Jan 19, 2026

Codecov Report

❌ Patch coverage is 15.62500% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.26%. Comparing base (a31cf8c) to head (f791038).

Files with missing lines Patch % Lines
block/internal/da/forced_inclusion_tracing.go 0.00% 25 Missing ⚠️
block/internal/da/forced_inclusion_retriever.go 71.42% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@                         Coverage Diff                         @@
##           tracing-part-11-da-submitter-v2    #2997      +/-   ##
===================================================================
- Coverage                            59.42%   59.26%   -0.16%     
===================================================================
  Files                                  107      108       +1     
  Lines                                10075    10103      +28     
===================================================================
+ Hits                                  5987     5988       +1     
- Misses                                3458     3484      +26     
- Partials                               630      631       +1     
Flag Coverage Δ
combined 59.26% <15.62%> (-0.16%) ⬇️

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 tracing-part-11-da-submitter-v2 to main January 19, 2026 15:39
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.

2 participants