chore: adding syncing tracing

[chatton]

Overview

[github-actions]

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

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	⏩ skipped	✅ passed	✅ passed	Jan 14, 2026, 3:21 PM

[claude]

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

---

Code Review Complete ✅

I've completed a comprehensive review of the syncing tracing additions in this PR. Overall, the implementation is well-structured and follows good patterns. Below are my findings organized by focus area.

Tasks

• Read all changed files to understand implementation
• Review code quality & architecture
• Review error handling & resilience
• Review security considerations
• Review performance & resource efficiency
• Review testing & reliability
• Review documentation & developer experience
• Provide comprehensive feedback

---

Summary

This PR adds OpenTelemetry tracing instrumentation to the block syncing component using a decorator pattern. The implementation is clean, well-tested, and follows Go best practices. I found no critical issues, but have several suggestions for improvements.

Key Strengths:

• Clean decorator pattern implementation
• Comprehensive test coverage (329 lines of tests for 103 lines of implementation)
• Proper error propagation in traces
• Thread-safe implementation

Areas for Improvement:

• Minor optimization opportunities (context propagation)
• Documentation enhancements
• Test assertion improvements

---

Detailed Findings

1. Code Quality & Architecture ⭐

Excellent Implementation

• The decorator pattern is properly implemented with the BlockSyncer interface (block/internal/syncing/block_syncer.go:13-25)
• Clean separation of concerns - tracing is non-invasive
• Proper interface implementation verification with var _ BlockSyncer = (*tracedBlockSyncer)(nil) (block/internal/syncing/tracing.go:16)
• Dependency injection via SetBlockSyncer allows flexible composition (block/internal/syncing/syncer.go:190-192)

Minor Improvement Suggestion:

The BlockSyncer interface comment could be more detailed about the purpose and guarantees:

// BlockSyncer defines operations for block synchronization that can be traced.
// The Syncer implements this interface, and a tracing decorator can wrap it
// to add OpenTelemetry spans to each operation.

Consider adding information about error handling expectations and whether operations are idempotent.

---

2. Error Handling & Resilience ✅

Proper Error Propagation

• All error paths correctly record errors in spans (tracing.go:44-46, 60-63, 81-84, 98-101)
• Status codes properly set using codes.Error on failure
• Errors are not swallowed or modified by the tracing layer
• Context cancellation is properly handled

No Issues Found - Error handling is robust.

---

3. Security 🔒

Good Practices

• No sensitive data exposed in span attributes
• Block heights and transaction counts are safe to log
• State root hash is hex-encoded before adding to attributes (tracing.go:67)
• No credentials or private keys in traces

Potential Consideration:

The state root hash is exposed in traces (tracing.go:67). While this is generally safe, confirm this doesn't leak any information in your threat model. If state roots could be used for targeted attacks, consider adding a configuration flag to redact them.

---

4. Performance & Resource Efficiency 🚀

Good Practices:

• Minimal overhead - only span creation and attribute setting
• No blocking operations introduced by tracing
• Efficient attribute usage - only essential data captured

Optimization Opportunity:

Context propagation could be slightly optimized. Currently, the traced context is created but the result is passed to the inner call:

func (t *tracedBlockSyncer) TrySyncNextBlock(ctx context.Context, event *common.DAHeightEvent) error {
    ctx, span := t.tracer.Start(ctx, "BlockSyncer.SyncBlock", ...)
    defer span.End()
    
    err := t.inner.TrySyncNextBlock(ctx, event)  // ✅ Correct - using traced context

This is actually correct as-is - I initially thought this might be an issue, but reviewing again, the traced context IS being passed to the inner call. Good work!

Minor Note:
The span names use different conventions:

• "BlockSyncer.SyncBlock" (method style)
• "BlockSyncer.ApplyBlock" (matches method name)
• "BlockSyncer.ValidateBlock" (matches method name)
• "BlockSyncer.VerifyForcedInclusion" (abbreviated)

Consider standardizing to match the actual method names exactly: TrySyncNextBlock instead of SyncBlock, and VerifyForcedInclusionTxs instead of VerifyForcedInclusion.

---

5. Testing & Reliability 🧪

Excellent Test Coverage

• Comprehensive success and error cases for all methods
• Uses test doubles (mockBlockSyncer) appropriately
• Proper test isolation with SpanRecorder
• Tests verify both span creation AND attribute values

Improvement Suggestions:

1. Test Attribute Verification (tracing_test.go:309-329):
   The requireAttribute helper could benefit from better error messages:

   require.True(t, found, "attribute %s not found", key)

   Consider also printing the available attributes when an assertion fails for easier debugging.

2. Coverage Gap - Context Propagation:
   No test explicitly verifies that the traced context is properly passed to the inner implementation. Consider adding a test that verifies context propagation:

   func TestTracedBlockSyncer_ContextPropagation(t *testing.T) {
       var capturedCtx context.Context
       mock := &mockBlockSyncer{
           applyBlockFn: func(ctx context.Context, header types.Header, data *types.Data, currentState types.State) (types.State, error) {
               capturedCtx = ctx
               return types.State{}, nil
           },
       }
       syncer, _ := setupBlockSyncerTrace(t, mock)
       
       syncer.ApplyBlock(context.Background(), types.Header{}, &types.Data{}, types.State{})
       
       // Verify the context has tracing info
       span := trace.SpanFromContext(capturedCtx)
       require.True(t, span.IsRecording(), "context should contain active span")
   }

3. Test Data Quality:
   Tests use hardcoded heights like 100, 50, 75. Consider using named constants for better readability:

   const (
       testBlockHeight = 100
       testDAHeight = 50
   )

---

6. Documentation & Developer Experience 📚

Good Documentation

• Clear package-level documentation in interface
• Test setup helper is well-documented
• Component wiring is clear in block/components.go:154-156

Enhancement Suggestions:

1. Add godoc to WithTracingBlockSyncer (tracing.go:24-30):

   // WithTracingBlockSyncer decorates the provided BlockSyncer with tracing spans.
   // Each BlockSyncer operation will create an OpenTelemetry span with relevant attributes:
   //   - TrySyncNextBlock: captures block.height, da.height, source
   //   - ApplyBlock: captures block.height, tx.count, state_root
   //   - ValidateBlock: captures block.height
   //   - VerifyForcedInclusionTxs: captures block.height, da.height
   //
   // Errors are automatically recorded in spans with status codes.Error.
   // The tracer is created with name "ev-node/block-syncer".
   func WithTracingBlockSyncer(inner BlockSyncer) BlockSyncer {

2. Example Usage:
   Consider adding a code example in the package documentation showing how to enable tracing (though block/components.go serves as a good reference).

3. Span Attribute Documentation:
   Document what each span attribute means in the context of block syncing. This helps with observability dashboards and debugging.

---

Specific File Comments

block/components.go

• Lines 154-156: Clean conditional tracing enablement. ✅
• The check config.Instrumentation.IsTracingEnabled() is clear and explicit.

block/internal/syncing/block_syncer.go

• Line 14: Minor typo - "TrySyncNextBlock" comment says "attempts to sync the next available block" but doesn't specify from where. Consider: "attempts to sync the next available block from DA or P2P."

block/internal/syncing/tracing.go

• Line 28: Good use of a descriptive tracer name
• Lines 33-47: Consider extracting attribute keys as constants to prevent typos:

  const (
      AttrBlockHeight = "block.height"
      AttrDAHeight = "da.height"
      AttrSource = "source"
      AttrTxCount = "tx.count"
      AttrStateRoot = "state_root"
  )

block/internal/syncing/tracing_test.go

• Line 91: Good test name following Go conventions
• Lines 64-98: Excellent test structure with clear arrange-act-assert
• Lines 131-165: Good coverage of both success and error paths

block/internal/syncing/syncer.go

• Line 184: Setting s.blockSyncer = s is clear - the Syncer is its own default implementation
• Lines 190-192: Good encapsulation of the decorator injection

Test File Updates

• syncer_test.go & syncer_forced_inclusion_test.go: The updates properly use the blockSyncer field instead of calling methods directly on s. This is correct and maintains the decorator pattern. ✅

---

Recommendations Priority

High Priority

• None - no critical issues found

Medium Priority

1. Standardize span names to match actual method names
2. Add test for context propagation
3. Consider documenting span attributes for observability

Low Priority

1. Extract span attribute keys as constants
2. Add more detailed godoc to WithTracingBlockSyncer
3. Improve test attribute verification error messages
4. Consider named constants in test data

---

Conclusion

This is high-quality work that follows Go best practices and the repository's patterns. The tracing implementation is non-invasive, well-tested, and production-ready. The decorator pattern is cleanly implemented and the conditional enablement in components.go is elegant.

Approval Recommendation: ✅ Approve with minor suggestions for future improvements.

The suggestions above are mostly enhancements for maintainability and observability - nothing that should block merging.

---

[gemini-code-assist]

Summary of Changes

Hello @chatton, I'm Gemini Code Assist¹! 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 block synchronization mechanism by integrating OpenTelemetry tracing. It achieves this through a refactoring that introduces a BlockSyncer interface and a tracing decorator. This architectural change allows for granular monitoring of block processing, validation, and forced inclusion verification, providing valuable insights into the system's behavior without altering the core logic, and can be enabled or disabled via configuration.

Highlights

• OpenTelemetry Tracing Integration: Introduced OpenTelemetry tracing for key block synchronization operations, allowing for detailed observability into the block processing lifecycle.
• BlockSyncer Interface: Defined a new BlockSyncer interface to abstract the core synchronization logic, enabling the use of decorators like the new tracing implementation.
• Syncer Refactoring: The Syncer struct now implements the BlockSyncer interface, and its internal methods (trySyncNextBlock, applyBlock, validateBlock, verifyForcedInclusionTxs) have been made public and updated to accept a context.Context.
• Conditional Tracing: Tracing for the block syncer is conditionally applied based on the config.Instrumentation.IsTracingEnabled() setting, ensuring performance overhead is only incurred when desired.
• Tracing Decorator: A tracedBlockSyncer decorator has been added, which wraps an existing BlockSyncer and automatically adds OpenTelemetry spans with relevant attributes to each operation.

🧠 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. ↩

[gemini-code-assist]

Code Review

This pull request introduces OpenTelemetry tracing to the block synchronization process using a decorator pattern. The changes are well-structured, defining a BlockSyncer interface, modifying the Syncer to use this interface, and implementing a tracedBlockSyncer decorator. This approach effectively adds tracing as a cross-cutting concern without altering the core synchronization logic. The accompanying tests for the tracing decorator are comprehensive and ensure proper span creation, attribute setting, and error handling.

[gemini-code-assist]

Consider making the span name more explicit to match the method name, for example, BlockSyncer.TrySyncNextBlock. While SyncBlock is concise, TrySyncNextBlock would provide a clearer indication of the specific operation being traced, especially when reviewing traces.

Suggested change

	ctx, span := t.tracer.Start(ctx, "BlockSyncer.SyncBlock",
	ctx, span := t.tracer.Start(ctx, "BlockSyncer.TrySyncNextBlock",

[codecov]

Codecov Report

❌ Patch coverage is 91.02564% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.72%. Comparing base (0bc03f0) to head (eda1139).

Files with missing lines	Patch %	Lines
block/internal/syncing/syncer.go	68.75%	5 Missing ⚠️
block/components.go	0.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##           tracing-part-7    #2981      +/-   ##
==================================================
+ Coverage           58.48%   58.72%   +0.24%     
==================================================
  Files                 100      101       +1     
  Lines                9619     9685      +66     
==================================================
+ Hits                 5626     5688      +62     
- Misses               3382     3385       +3     
- Partials              611      612       +1     

Flag	Coverage Δ
combined	58.72% <91.02%> (+0.24%)	⬆️

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.