feat(tracing): adding tracing to DA client

[chatton]

Overview

[claude]

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

---

Code Review Complete

I've completed a comprehensive review of the tracing implementation for the DA client. Overall, this is a well-structured and clean implementation with good test coverage. Below are my findings organized by focus area.

---

1. Code Quality & Architecture ✅

Strengths:

• Clean decorator pattern implementation following Go idioms
• Proper interface implementation with all FullClient methods
• Good separation of concerns - tracing logic isolated from business logic
• Consistent naming conventions and code structure
• Non-traced methods (namespace getters) correctly delegate without instrumentation

Issues:

Minor: Inconsistent attribute naming

Location: block/internal/da/tracing.go:52-53

The attribute naming is inconsistent:

• Submit uses: blob.count, blob.total_size_bytes, da.namespace
• Retrieve uses: ns.length (inconsistent with blob.count), da.namespace

Recommendation: Standardize on a consistent prefix pattern. Suggest:

// Retrieve should use:
attribute.Int("namespace.length", len(namespace))
// instead of:
attribute.Int("ns.length", len(namespace))

---

2. Error Handling & Resilience ✅

Strengths:

• Proper error propagation - returns original errors unchanged
• Correct span status handling for both success and error cases
• Differentiates between StatusNotFound (not an error) and StatusError in Retrieve

Issues:

Critical: Namespace conversion may panic on invalid UTF-8

Location: block/internal/da/tracing.go:34, 54, 73, 92, 111

attribute.String("da.namespace", string(namespace))

Converting arbitrary byte slices to strings can produce invalid UTF-8, which may cause issues in some tracing backends. Namespaces are binary data, not necessarily UTF-8 strings.

Recommendation:

// Use hex encoding for binary data
attribute.String("da.namespace", hex.EncodeToString(namespace))
// Or use binary attribute if supported
attribute.String("da.namespace_hex", hex.EncodeToString(namespace))

Minor: Custom submitError type is unnecessary

Location: block/internal/da/tracing.go:134-136

The submitError type wraps a string but adds no additional functionality. This creates an extra allocation for every error span.

Recommendation:

// In Submit method, line 41:
span.RecordError(fmt.Errorf("DA submit failed: %s", res.Message))
// Instead of:
span.RecordError(&submitError{msg: res.Message})

// Remove the submitError type entirely

Minor: Missing height attribute on error

Location: block/internal/da/tracing.go:40-46

On success, the DA height is recorded as an attribute (da.height), but on error it's not. The res.Height field may still contain useful debugging information even on failure.

Recommendation:

res := t.inner.Submit(ctx, data, gasPrice, namespace, options)
if res.Code != datypes.StatusSuccess {
    span.RecordError(fmt.Errorf("DA submit failed: %s", res.Message))
    span.SetStatus(codes.Error, res.Message)
    if res.Height > 0 {
        span.SetAttributes(attribute.Int64("da.height", int64(res.Height)))
    }
} else {
    span.SetAttributes(attribute.Int64("da.height", int64(res.Height)))
}

---

3. Security ✅

Strengths:

• No sensitive data (private keys, credentials) in trace attributes
• Namespace data appropriately included (public information)
• No credential leakage in error messages

Considerations:

• Blob sizes and counts are included in traces - ensure this doesn't leak sensitive business information in your deployment context
• Error messages are passed through directly - verify they don't contain sensitive details

---

4. Performance & Resource Efficiency ⚠️

Issues:

Minor: String conversion overhead

Location: block/internal/da/tracing.go:34, 54, 73, 92, 111

Converting byte slices to strings (string(namespace)) creates allocations on every call. While namespaces are typically small, this could be optimized.

Recommendation: Consider caching namespace string conversions if namespaces are reused frequently, or use hex encoding (see error handling section).

Minor: Total size calculation could overflow

Location: block/internal/da/tracing.go:26-29

total := 0  // int type could overflow on 32-bit systems
for _, b := range data {
    total += len(b)
}

Recommendation:

total := int64(0)
for _, b := range data {
    total += int64(len(b))
}
// Then use attribute.Int64 instead of attribute.Int

---

5. Testing & Reliability ✅

Strengths:

• Excellent test coverage with both success and error paths
• Good use of test helpers (setupDATrace, requireAttribute)
• Mock implementation is clean and functional
• Tests verify span names, status codes, and attributes
• Proper cleanup with t.Cleanup()

Issues:

Critical: Missing test coverage

Location: block/internal/da/tracing_test.go

Missing tests for:

1. GetProofs - No test for success or error cases
2. Validate - No test for success or error cases
3. Namespace getter methods - Should verify pass-through behavior
4. Retrieve with StatusNotFound - Should verify it's not treated as error
5. Context cancellation - Should verify spans are created even when context is cancelled

Recommendation: Add tests for all missing methods:

func TestTracedDA_GetProofs_Success(t *testing.T) { /* ... */ }
func TestTracedDA_GetProofs_Error(t *testing.T) { /* ... */ }
func TestTracedDA_Validate_Success(t *testing.T) { /* ... */ }
func TestTracedDA_Validate_Error(t *testing.T) { /* ... */ }
func TestTracedDA_Retrieve_NotFound(t *testing.T) { 
    // Should verify codes.Unset, not codes.Error
}
func TestTracedDA_NamespaceGetters(t *testing.T) { /* ... */ }

Minor: Test assertion inconsistency

Location: block/internal/da/tracing_test.go:93-101

The test manually searches for da.namespace attribute and validates byte length (line 98), which is inconsistent with using the requireAttribute helper elsewhere.

Recommendation: Enhance requireAttribute to support byte length validation or create a separate helper for consistency.

---

6. Documentation 📝

Strengths:

• Clear comments on exported types and functions
• Good package-level documentation

Issues:

Minor: Missing godoc for attribute semantics

Location: block/internal/da/tracing.go:21-23, 25

The tracing attributes are not documented. Developers and operators need to understand what each attribute means for debugging and monitoring.

Recommendation:

// WithTracingClient decorates the provided client with tracing spans.
// The following span attributes are recorded:
//   - DA.Submit: blob.count, blob.total_size_bytes, da.namespace, da.height (on success)
//   - DA.Retrieve: ns.length, da.namespace, blob.count (on success)
//   - DA.Get: id.count, da.namespace, blob.count (on success)
//   - DA.GetProofs: id.count, da.namespace, proof.count (on success)
//   - DA.Validate: id.count, da.namespace, result.count (on success)
func WithTracingClient(inner FullClient) FullClient {
    return &tracedClient{inner: inner, tracer: otel.Tracer("ev-node/da")}
}

---

7. Repository-Specific Issues ⚠️

Critical: Test binary committed to repository

Location: da.test (root directory)

A test binary file (da.test) was committed. This is already covered by .gitignore with the *.test pattern, but the file exists in the commit.

Recommendation: Remove this file:

git rm da.test

This file should never have been tracked. The .gitignore update correctly prevents this in the future.

---

Summary

Overall Assessment: This is a solid implementation with good architecture and test coverage. The main issues are:

1. Must Fix:

   • Remove da.test binary from repository
   • Add missing test coverage (GetProofs, Validate, NotFound case)
   • Fix namespace string conversion (potential UTF-8 issues)

2. Should Fix:

   • Remove unnecessary submitError type
   • Standardize attribute naming
   • Document traced attributes in godoc

3. Nice to Have:

   • Add height attribute on errors
   • Fix potential int overflow in size calculation
   • Test context cancellation behavior

The code follows Go best practices and integrates cleanly with the existing architecture. With the critical issues addressed, this will be production-ready.

---

[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 12, 2026, 2:38 PM

[codecov]

Codecov Report

❌ Patch coverage is 56.52174% with 40 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.83%. Comparing base (32db6c8) to head (d3bdb1e).

Files with missing lines	Patch %	Lines
block/internal/da/tracing.go	59.09%	36 Missing ⚠️
block/public.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@                     Coverage Diff                     @@
##           cian/add-tracing-part-4    #2968      +/-   ##
===========================================================
+ Coverage                    57.76%   57.83%   +0.06%     
===========================================================
  Files                           97       98       +1     
  Lines                         9306     9397      +91     
===========================================================
+ Hits                          5376     5435      +59     
- Misses                        3326     3358      +32     
  Partials                       604      604              

Flag	Coverage Δ
combined	57.83% <56.52%> (+0.06%)	⬆️

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.