Skip to content

Conversation

@chatton
Copy link
Contributor

@chatton chatton commented Jan 19, 2026

Overview

ref #2956

Adding tracing for da retrieval, needed to bump sonic dep after go version bump.

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

github-actions bot commented Jan 19, 2026

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

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedJan 19, 2026, 11: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 enhances the system's observability by integrating OpenTelemetry tracing into the Data Availability (DA) retriever. It introduces a tracing decorator that wraps the existing DARetriever to automatically instrument its RetrieveFromDA method. This allows for the collection of detailed telemetry data, including operational attributes and error information, which is crucial for monitoring performance and diagnosing issues within the DA retrieval process. The tracing is conditionally applied based on the system's instrumentation configuration, ensuring that this feature can be enabled or disabled as needed without altering core logic.

Highlights

  • OpenTelemetry Tracing for DARetriever: Introduced OpenTelemetry tracing for the DARetriever interface by adding a tracedDARetriever decorator. This decorator instruments the RetrieveFromDA method to capture execution details.
  • Detailed Span Attributes: The tracing implementation records relevant attributes such as da.height, event.count, and block.heights within the span, providing granular context for each DA retrieval operation.
  • Error Handling in Tracing: Errors encountered during DA retrieval are now recorded in the span, and the span's status is set to Error, facilitating easier identification and debugging of failures.
  • Conditional Tracing Integration: The Syncer component has been updated to conditionally apply the WithTracingDARetriever decorator, ensuring that tracing is only enabled when instrumentation is configured to do so.
  • Comprehensive Unit Tests: New unit tests have been added for the tracedDARetriever to verify its behavior across various scenarios, including successful retrievals, cases with no events, and error conditions.

🧠 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 41s —— View job


Code Review Complete ✓

I've completed a comprehensive review of this PR which adds OpenTelemetry tracing for the DA retriever. Overall, this is a well-implemented and valuable addition to the observability infrastructure. The code follows established patterns and includes solid test coverage.


Summary

What's Good:

  • Clean decorator pattern implementation following existing tracing conventions
  • Comprehensive test coverage with multiple scenarios
  • Proper error handling and span status management
  • Meaningful trace attributes for observability
  • Conditional tracing application based on instrumentation config

Areas for Improvement:

  • Test coverage gap for block.heights attribute verification
  • Minor code organization opportunity in syncer.go

Detailed Findings

1. Code Quality & Architecture ✓

Strengths:

  • The decorator pattern is correctly applied, consistent with tracedClient in block/internal/da/tracing.go
  • Interface compliance is verified with compile-time check (var _ DARetriever = (*tracedDARetriever)(nil))
  • Tracer naming follows convention: "ev-node/da-retriever"
  • Integration in syncer.go:204-206 is clean and conditionally applied

Observations:
The implementation correctly wraps the RetrieveFromDA method and propagates the traced context to the inner retriever, enabling distributed tracing.

2. Error Handling & Resilience ✓

Strengths:

  • Errors are properly recorded with span.RecordError(err) (line 40)
  • Span status correctly set to codes.Error with error message (line 41)
  • Error is returned unchanged, preserving error chain (line 42)
  • Context propagation is correct, allowing for cancellation/timeout handling

Good Practice:
The implementation follows OpenTelemetry best practices by recording both the error event and setting the span status, which is crucial for error tracking in observability platforms.

3. Security ✓

No Issues Found:

  • No sensitive data exposed in trace attributes
  • DA height and event counts are safe to trace
  • Block heights are metadata, not sensitive information
  • No credential or PII leakage risks

4. Performance & Resource Efficiency ✓

Strengths:

  • Minimal overhead: only creates span when tracing is enabled
  • Efficient attribute setting after successful retrieval
  • Block heights slice allocation is reasonable (line 49-52)
  • Deferred span.End() ensures proper cleanup (line 36)

Minor Observation:
The heights slice allocation could be optimized with pre-sizing, but given typical event counts, this is negligible.

5. Testing & Reliability ⚠️

Strengths:

  • Three test scenarios cover success, no events, and error cases
  • Test helper setupDARetrieverTrace provides clean test setup
  • Mock implementation is straightforward and reusable
  • Span recording verification is thorough

Issue - Missing Test Coverage:
The block.heights attribute (set on line 53 of da_retriever_tracing.go) is not verified in tests. This was also noted by Gemini Code Assist's review.

Recommendation:
Add verification for the block.heights attribute in TestTracedDARetriever_RetrieveFromDA_Success. Here's the suggested test enhancement:

// In da_retriever_tracing_test.go:79, after line 79:
testutil.RequireAttribute(t, attrs, "event.count", 2)

// Add verification for block.heights
var heightsFound bool
for _, attr := range attrs {
    if attr.Key == "block.heights" {
        heightsFound = true
        require.Equal(t, attribute.INT64SLICE, attr.Value.Type())
        require.Equal(t, []int64{100, 101}, attr.Value.AsInt64Slice())
        break
    }
}
require.True(t, heightsFound, "block.heights attribute not found")

Coverage Report Analysis:
Codecov shows 92% patch coverage with 2 lines missing in syncer.go:204-205. These lines are the conditional tracing wrapper - they may not be covered if integration tests don't enable instrumentation. This is acceptable for initialization code, but consider adding a test that verifies the tracing wrapper is applied when Instrumentation.IsTracingEnabled() returns true.

6. Documentation & Developer Experience ✓

Strengths:

  • Clear function and type comments
  • Tracer name is descriptive and follows naming conventions
  • Integration point is well-documented in syncer.go
  • PR description explains the purpose and dependency bump

Minor Enhancement:
Consider adding a package-level comment in da_retriever_tracing.go explaining the tracing decorator pattern for developers unfamiliar with this approach.


Dependency Update

The sonic version bump (v1.14.2 in test/docker-e2e/go.mod:20) is noted as necessary for Go 1.25 compatibility. This is a transitive dependency update and appears safe.


Recommendations

High Priority

  1. Add test verification for block.heights attribute (see recommendation in Testing section above)
    • File: block/internal/syncing/da_retriever_tracing_test.go:79
    • This ensures the implementation matches the observed behavior

Low Priority

  1. Consider adding integration test that verifies tracing decorator is applied when instrumentation is enabled

    • This would improve coverage of lines 204-205 in syncer.go
    • Not critical as the logic is straightforward
  2. Package-level documentation - Add comment explaining the decorator pattern

    • Helps onboard new developers to the tracing infrastructure

Conclusion

This PR represents solid engineering work that follows project conventions and adds valuable observability. The tracing implementation is correct, efficient, and follows OpenTelemetry best practices. The test gap for block.heights is minor but should be addressed for completeness.

Recommendation: Approve with minor test enhancement

The code is production-ready, but adding the block.heights attribute verification would ensure complete test coverage of the tracing behavior. Once CI passes and the optional test enhancement is added, this is ready to merge.


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 DARetriever, which is a great addition for observability. The implementation of the tracedDARetriever decorator is clean and follows best practices for tracing in Go. The new code is well-structured and includes corresponding unit tests.

My main feedback is a suggestion to enhance the tests to ensure all new tracing attributes are verified. Overall, this is a solid contribution.


attrs := span.Attributes()
testutil.RequireAttribute(t, attrs, "da.height", int64(50))
testutil.RequireAttribute(t, attrs, "event.count", 2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This test is great for verifying the success case. To make it more comprehensive, it would be beneficial to also assert that the block.heights attribute is correctly set on the span when events are returned.

The testutil.RequireAttribute helper doesn't seem to support slice attributes, so you might need to perform this check manually. Here's a suggestion to add this verification:

Suggested change
testutil.RequireAttribute(t, attrs, "event.count", 2)
testutil.RequireAttribute(t, attrs, "event.count", 2)
// Check for block.heights attribute
var heightsValue attribute.Value
for _, attr := range attrs {
if attr.Key == "block.heights" {
heightsValue = attr.Value
break
}
}
require.NotNil(t, heightsValue, "attribute 'block.heights' not found")
require.Equal(t, attribute.INT64SLICE, heightsValue.Type())
require.Equal(t, []int64{100, 101}, heightsValue.AsInt64Slice())

@codecov
Copy link

codecov bot commented Jan 19, 2026

Codecov Report

❌ Patch coverage is 92.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.25%. Comparing base (50b35a9) to head (65fda65).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
block/internal/syncing/syncer.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2991      +/-   ##
==========================================
+ Coverage   59.17%   59.25%   +0.08%     
==========================================
  Files         105      106       +1     
  Lines        9998    10023      +25     
==========================================
+ Hits         5916     5939      +23     
- Misses       3454     3456       +2     
  Partials      628      628              
Flag Coverage Δ
combined 59.25% <92.00%> (+0.08%) ⬆️

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 requested a review from tac0turtle January 19, 2026 10:58
@chatton chatton marked this pull request as ready for review January 19, 2026 10:58
@chatton chatton added this pull request to the merge queue Jan 19, 2026
Merged via the queue into main with commit 66ac65a Jan 19, 2026
46 of 47 checks passed
@chatton chatton deleted the tracing-part-10-da-retriever-v2 branch January 19, 2026 12:29
alpe added a commit that referenced this pull request Jan 19, 2026
* main:
  chore: update calculator for strategies  (#2995)
  chore: adding tracing for da submitter (#2993)
  feat(tracing): part 10 da retriever tracing (#2991)
  chore: add da posting strategy to docs (#2992)
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