Skip to content

Conversation

@iduartgomez
Copy link
Collaborator

Fix span accumulation issue in multi-peer tests where tracing spans were being inherited and accumulated across async task boundaries, making it impossible to determine which peer a log entry belonged to.

Changes

Pattern Applied

Replace with_peer_id() calls inside async blocks with .instrument() to attach isolated spans:

Before (broken):

let node = async {
    let _span = with_peer_id("gateway");
    // ...
}.boxed_local();

After (fixed):

let node = async {
    // ...
}
.instrument(tracing::info_span!("test_peer", test_node = "gateway"))
.boxed_local();

Files Modified

  • operations.rs: Fixed 3 tests (test_put_contract, test_put_merge_persists_state, test_multiple_clients_subscription)
  • connectivity.rs: Fixed all tests with multi-peer scenarios
  • test_utils.rs: Removed with_peer_id() function that caused the issue

Result

Each peer now has isolated spans in logs:

  • "spans":[{"test_node":"gateway"}] - Gateway logs only
  • "spans":[{"test_node":"peer-1"}] - Peer-1 logs only

This significantly improves debugging experience for multi-peer tests.

🤖 Generated with Claude Code

Fix span accumulation issue in multi-peer tests where tracing spans
were being inherited and accumulated across async task boundaries,
making it impossible to determine which peer a log entry belonged to.

## Changes

### Pattern Applied
Replace `with_peer_id()` calls inside async blocks with `.instrument()`
to attach isolated spans:

**Before (broken):**
```rust
let node = async {
    let _span = with_peer_id("gateway");
    // ...
}.boxed_local();
```

**After (fixed):**
```rust
let node = async {
    // ...
}
.instrument(tracing::info_span!("test_peer", test_node = "gateway"))
.boxed_local();
```

### Files Modified
- operations.rs: Fixed 3 tests (test_put_contract, test_put_merge_persists_state,
  test_multiple_clients_subscription)
- connectivity.rs: Fixed all tests with multi-peer scenarios
- test_utils.rs: Removed with_peer_id() function that caused the issue

## Result
Each peer now has isolated spans in logs:
- "spans":[{"test_node":"gateway"}] - Gateway logs only
- "spans":[{"test_node":"peer-1"}] - Peer-1 logs only

This significantly improves debugging experience for multi-peer tests.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@sanity sanity requested a review from Copilot October 27, 2025 19:48
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes span accumulation in multi-peer integration tests where tracing spans were incorrectly inherited across async task boundaries. The fix replaces with_peer_id() calls with .instrument() to ensure each peer gets isolated spans, making log entries clearly attributable to specific peers.

Key changes:

  • Replaced with_peer_id() pattern with .instrument(tracing::info_span!()) in async blocks
  • Removed the with_peer_id() helper function that caused span inheritance issues
  • Updated documentation to reflect the new instrumentation pattern

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
operations.rs Applied instrumentation fix to 3 multi-peer tests (test_put_contract, test_put_merge_persists_state, test_multiple_clients_subscription)
connectivity.rs Fixed span isolation in all connectivity tests with multiple peers; added Instrument trait import
test_utils.rs Removed problematic with_peer_id() function and updated documentation with correct instrumentation examples

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@sanity
Copy link
Collaborator

sanity commented Oct 27, 2025

Re: Copilot's comment on connectivity.rs:23 suggesting to remove use tracing::Instrument;

This import is actually required. In Rust, traits must be in scope to call their methods, even when used with method syntax. Without use tracing::Instrument;, the compiler will error with "no method named instrument found" since .instrument() is a trait method from the Instrument trait.

The import should remain.

[AI-assisted debugging and comment]

@sanity sanity enabled auto-merge October 27, 2025 20:07
@sanity sanity added this pull request to the merge queue Oct 27, 2025
Merged via the queue into main with commit bd9ab04 Oct 27, 2025
10 checks passed
@sanity sanity deleted the fix/test-span-accumulation branch October 27, 2025 20:22
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