chore: adding syncing tracing

[chatton]

Overview

Follow up which adds tracing to the block syncer

[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 15, 2026, 2:27 PM

[claude]

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

---

Code Review: Adding Syncing Tracing

I've completed a comprehensive review of this PR. Overall, the implementation is excellent - it follows established patterns in the codebase, uses proper decorator design, and has thorough test coverage. Here are my findings:

---

✅ Strengths

1. Architecture & Design

• Clean decorator pattern: The BlockSyncer interface with tracedBlockSyncer wrapper follows the exact pattern used elsewhere (BlockProducer/tracedBlockProducer)
• Minimal intrusion: Changes to existing code are minimal and non-breaking
• Proper separation of concerns: Tracing logic is completely isolated from business logic
• Conditional enablement: Tracing is only activated when config.Instrumentation.IsTracingEnabled() returns true (block/components.go:154-156)

2. Context Propagation

• All methods properly accept and propagate context.Context
• Context is correctly enhanced with trace spans and passed to inner methods
• This enables distributed tracing and proper cancellation handling

3. Test Coverage

• 92.3% patch coverage according to Codecov
• Comprehensive test suite with 8 test cases covering:
  • Success paths for all operations
  • Error handling and span status codes
  • Attribute verification
  • Mock-based isolation
• Tests use proper setup/teardown with t.Cleanup()

4. Observability

• Rich span attributes: Height, DA height, tx count, state root, source
• Proper error recording: Uses span.RecordError() and span.SetStatus(codes.Error, ...)
• Consistent naming: Span names follow BlockSyncer.MethodName convention

---

🔍 Issues & Recommendations

Issue 1: Inconsistent Span Naming (Medium Priority)

Location: block/internal/syncing/tracing.go:33

The span name "BlockSyncer.SyncBlock" doesn't match the method name TrySyncNextBlock. This inconsistency could confuse operators reviewing traces.

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

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

Why it matters:

• Trace analysis tools rely on consistent naming
• Makes it harder to correlate spans with source code
• The Gemini review comment flagged this same issue

Alternative consideration: If "SyncBlock" is intentionally shorter for readability in trace UIs, document this decision in a comment.

---

Issue 2: Missing Context Handling in Error Path (Low Priority)

Location: block/internal/syncing/tracing.go:63

When ApplyBlock returns an error, it returns types.State{} instead of the actual state returned by the inner call:

state, err := t.inner.ApplyBlock(ctx, header, data, currentState)
if err != nil {
    span.RecordError(err)
    span.SetStatus(codes.Error, err.Error())
    return types.State{}, err  // ← Should return 'state', not empty State
}

Why it matters:

• The inner method might return partial state even on error
• Current code discards that information
• Inconsistent with how errors are handled elsewhere in this file

However: Looking at the actual ApplyBlock implementation (syncer.go:668), it appears to always return types.State{} on error anyway, so this may be acceptable. Still, it's better to preserve the actual return value.

---

Issue 3: Nil Pointer Risk in Attribute Setting (Low Priority)

Location: block/internal/syncing/tracing.go:67

After an error in ApplyBlock, the code still tries to set state_root attribute:

if err != nil {
    span.RecordError(err)
    span.SetStatus(codes.Error, err.Error())
    return types.State{}, err
}

span.SetAttributes(
    attribute.String("state_root", hex.EncodeToString(state.AppHash)),
)

If state.AppHash is nil, hex.EncodeToString will encode it as an empty string, which is fine. However, the attribute is only set on success, which could be clearer. Consider moving the attribute inside the success path or making it conditional.

---

Issue 4: Method Signature Inconsistency (Observation)

Location: block/internal/syncing/syncer.go:578, 667, 722, 825

The PR makes internal methods (TrySyncNextBlock, ApplyBlock, ValidateBlock, VerifyForcedInclusionTxs) accept context.Context as the first parameter. This is excellent for tracing, but note:

1. ValidateBlock and VerifyForcedInclusionTxs implementations don't actually use the context (they use _ context.Context)
2. This is fine as it leaves room for future use, but worth noting

Not a bug, just an architectural observation. The underscore _ clearly signals this is intentional.

---

📊 Code Quality Assessment

Go Best Practices: ✅ Excellent

• Idiomatic Go style throughout
• Proper use of interfaces and composition
• Clean error handling
• No goroutine leaks or concurrency issues in new code

Error Handling: ✅ Good

• Errors are properly propagated
• Trace spans correctly record errors
• Context cancellation is respected

Security: ✅ No Issues

• No credentials or sensitive data in traces
• Read-only decorator doesn't modify behavior
• No new attack surface introduced

Performance: ✅ Excellent

• Zero overhead when tracing is disabled (checked at initialization)
• Minimal overhead when enabled (just span creation)
• No blocking operations or allocations in hot paths
• Context is passed efficiently

Testing: ✅ Excellent

• 92.3% coverage on new code
• Tests are well-structured and use table-driven patterns where appropriate
• Proper use of test helpers and mocks
• Edge cases are covered (errors, empty data, etc.)

Documentation: ⚠️ Good, Minor Gaps

• Interface and types have clear doc comments
• Function purposes are documented
• Missing: No explanation of why this tracing approach was chosen
• Missing: No mention of performance impact in comments

---

🎯 Recommendations Summary

Must Fix (None)

No blocking issues found. The code is production-ready.

Should Fix

1. Span name consistency: Change "BlockSyncer.SyncBlock" → "BlockSyncer.TrySyncNextBlock" (tracing.go:33)

Nice to Have

1. Return actual state from ApplyBlock error path instead of empty State (tracing.go:63)
2. Move state_root attribute setting inside success-only path for clarity
3. Add a package-level comment explaining the decorator pattern and when it's enabled

---

🧪 Test Coverage Analysis

Files with missing coverage (from Codecov):

• block/internal/syncing/syncer.go: 3 missing, 1 partial (75% coverage on changes)
• block/components.go: 1 missing, 1 partial (0% coverage on changes)

The low coverage on block/components.go is expected since it's just wiring code that would only be tested in integration tests. The syncer.go coverage is good at 75%.

---

📝 Additional Observations

1. Consistent with Codebase: This PR follows the same tracing pattern as the block executor (PR feat(tracing): adding block production tracing #2980)
2. Good Git Hygiene: The PR is focused solely on adding tracing without unrelated changes
3. Clean Diff: The interface extraction (BlockSyncer) is minimal and doesn't break existing code
4. Proper Integration: The tracing is wired up correctly in NewSyncComponents with proper configuration checks

---

✅ Final Verdict

APPROVED - This is high-quality code that follows established patterns and best practices. The implementation is clean, well-tested, and ready for production.

The only recommended change is fixing the span name inconsistency, but this is not blocking. The code can be merged as-is if the team is okay with the current naming.

Action Items (Optional)

• Update span name in tracing.go:33 to match method name
• Consider adding a brief package doc comment explaining the decorator pattern

---

Excellent work on this PR! The decorator pattern is exactly the right approach for cross-cutting concerns like tracing.

[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 (01dcada) to head (4fc6805).
⚠️ Report is 1 commits behind head on main.

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             @@
##             main    #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.