Fix consumer OTel trace isolation

[thomhurst]

Fixes consumer spans for messages that arrive without a propagated TraceParent.

Previously those spans inherited the long-lived pump activity, so unrelated consumed messages could share one trace. This starts those consumer spans as roots while preserving propagated message context when it exists.

Validated with the new regression test and the core observability test filter.

[claude]

Review: Fix consumer OTel trace isolation (#4166)

Thanks for this — the fix is well-targeted and the intent is clear. Starting consumer spans as roots when no TraceParent is propagated is the right way to stop unrelated messages from collapsing into the long-lived pump trace, and the propagated-context path is correctly preserved. The helper extraction reads cleanly and the comment explaining the Activity.Current = null trick is genuinely helpful. Removing the dead baggage local is a nice tidy-up too.

A few things worth considering:

1. Trace fragmentation / loss of correlation (design — medium)

With no TraceParent, the receive span (CreateReceiveSpan -> StartRootActivity) and the process span (CreateSpan -> root) for the same message now land in two entirely separate root traces, with no link to each other or back to the pump's Begin span. Each message is correctly isolated, but you also lose the ability to correlate "which receive corresponds to which process," or to tie a message to the consumer's pump lifecycle.

The usual OTel pattern for "break the parent but keep the breadcrumb" is to attach an ActivityLink to the previous/pump context rather than fully orphaning the span. CreateSpan<TRequest> already accepts links, so the plumbing exists. Worth deciding whether full isolation or linked-isolation is the desired behavior here — if full isolation is intended, a sentence in the method's XML doc would save the next reader the surprise.

2. EndSpan restores Activity.Current unconditionally (robustness — low)

```csharp
if (previousActivity is not null)
Activity.Current = previousActivity;
```
This restores the stored previous activity regardless of whether span is actually the current activity, and regardless of whether previousActivity has since been ended. In the pump's strictly-nested ordering this is fine, but it's a latent footgun if spans are ever ended out of order (you'd repoint Activity.Current at a stale/disposed span). Consider guarding it (e.g. only restore when Activity.Current == span) or documenting the nesting assumption.

3. Strong reference via custom property (minor)

The custom property pins a strong reference to the previous activity for the lifetime of the new span. Harmless given these are short-lived, just noting it.

Test coverage

• The new test is solid and clearly asserts distinct trace IDs + null parents, and nicely verifies Activity.Current is restored to the pump span after each EndSpan.
• It drives BrighterTracer directly rather than through the Reactor pump. The existing When_A_Message_Is_Processed... test exercises the real pump for the propagated-context path; a pump-level test for the no-traceparent path would guard against regressions in the wiring, not just the tracer.
• Consider also asserting that baggage / correlationId propagation still works on a root process span, since that path runs after StartConsumerActivity.
• Stale comment alert: in When_A_Message_Is_Processed_It_Should_Have_A_Process_Span, the comment // receive span is local (parent = pump begin span) is now inaccurate — the receive span is a root with no parent. The Assert.NotEqual(...ParentId) still passes (null != traceparent), but the comment should be updated to reflect the new root behavior.

Note: I was not able to execute dotnet test in this environment, so the above is from code analysis (the new test's assertions trace through correctly). Please confirm the suite is green locally/CI.

Overall a good, focused change — the main thing I'd want a maintainer call on is whether per-message full isolation (no link back to receive/pump) is intended, vs. linked isolation.

[claude]

Claude Code Review — PR 4166: Fix consumer OTel trace isolation

Reviewed against the diff and the surrounding BrighterTracer / Reactor / Proactor flow. Overall this is a focused, well-reasoned fix for a real correctness problem (unrelated consumed messages sharing the long-lived pump trace). The helper extraction reads cleanly and stays in scope.

What is good

• Correct root-cause fix. Routing the no-TraceParent path through StartRootActivity (suppressing the ambient pump span) genuinely isolates traces, while the propagated-context path is preserved via StartConsumerActivity parentId branch. Good separation.
• Activity.Current restoration via the PreviousActivity custom property is a nice touch — it keeps the pump span as the ambient activity between messages, and the try/finally around the temporary Activity.Current = null is the right way to do this safely (it is an AsyncLocal, and the block is synchronous, so the swap cannot leak).
• Dead-code cleanup: removing the unused var baggage = message.Header.Baggage.ToString(); is correct — it had no consumer.
• The new test correctly asserts distinct TraceIds, null ParentId on the process spans, and that pumpSpan is re-established as Activity.Current after each EndSpan.

Issue: asymmetric Activity.Current restoration on the propagated-context path
StartRootActivity records the PreviousActivity custom property, but StartConsumerActivity parentId branch (message HAS a TraceParent) does not. Because that branch starts the activity from a remote parent id (string), Activity.Parent is null, so EndSpan -> span.Dispose() sets Activity.Current to null rather than back to the pump span.

Net effect: after processing a message that carries a TraceParent, Activity.Current is null for the remainder of the pump loop, whereas after a no-TraceParent message it is correctly the pump span. Trace isolation still holds (subsequent receive/process spans are created as roots regardless), so this is not a functional break — but it is an inconsistency that undercuts the symmetry the PR is establishing, and a latent surprise for any future code that assumes the pump span is ambient between messages. Consider preserving PreviousActivity in the parentId branch too (capture Activity.Current before StartActivity and SetCustomProperty when both are non-null), matching what StartRootActivity already does.

Test coverage gaps (minor)

• No test asserts that two messages with distinct propagated TraceParents land in distinct traces (each descending from its own producer) — that exercises the parentId branch. The existing single-message test covers parentage but not multi-message isolation.
• The new test drives the tracer API directly rather than through Reactor.Run(). Fine for a focused unit test, but the no-TraceParent fix is not exercised end-to-end through the real pump loop the way the existing MessagePumpProcessSpanObservabilityTests is.

Nits

• EndSpan now does a GetCustomProperty lookup on every span end — negligible cost, just noting it is on the hot path.
• The custom property holds a strong reference from each receive/process span to the pump span; harmless given span lifetimes, but worth being aware of if span retention ever changes.

Style/conventions

• License header on the new test matches repo convention; source registration (Paramore.Brighter) is consistent with the tracer ActivitySource name. Change stays within the requested scope per CLAUDE.md — no incidental default changes.

Nice, targeted fix. Addressing the propagated-context asymmetry would make the behavior fully consistent across both paths.

Generated with Claude Code

[claude]

Code Review: Fix consumer OTel trace isolation

Thanks for this — the fix is well-targeted and the reasoning in the PR description is sound. Consumer spans for messages without a propagated TraceParent were inheriting the long-lived pump activity, collapsing unrelated consumed messages into a single trace. Starting them as roots is the correct fix. A few notes below, mostly observations and one design question.

What is good

• Root-span technique is idiomatic. Suppressing Activity.Current (set to null, start, restore in finally) is the standard way to force a root span via ActivitySource.StartActivity, and the try/finally guarantees the ambient activity is restored even if StartActivity throws.
• EndSpan restoration is thoughtfully scoped. Because the new root spans have no in-process Parent, disposing them would otherwise leave Activity.Current == null rather than reverting to the pump span. Stashing the prior activity in a custom property and restoring it preserves the exact pre-fix "current reverts to pump" behavior that the Reactor/Proactor loops depend on between messages. The guard Activity.Current == span correctly avoids clobbering an unrelated current activity — and the out-of-order test is a nice cover for that edge.
• Dead-code cleanup. The removed var baggage = message.Header.Baggage.ToString(); was genuinely unused in CreateSpan — good catch.
• Behavior is unchanged for spans that never set the custom property (all non-root spans), since GetCustomProperty returns null, so the EndSpan change is safe across the board.

Design question — receive vs. process for the same message
With this change, for a message without a TraceParent, the receive span (always root now) and the process span (root when no traceparent) end up in two separate root traces. Previously they at least shared the pump trace. Isolating different messages from each other is clearly desirable, but fragmenting the same message receive and dispatch into disconnected traces may make it harder for an operator to navigate from broker-receive to handler-dispatch for one message.

Was linking considered instead of (or in addition to) rooting — e.g. an OTel span link from the process span to the receive span (and/or to the pump span)? Links would keep each message its own trace root while preserving navigability. Not blocking, but worth a sentence in the ADR/PR if it was a deliberate trade-off.

Asymmetry worth noting (pre-existing, not a regression)
For a message that does carry a TraceParent, StartConsumerActivity takes the parentId branch, which does not set the custom property. On EndSpan/dispose the activity has no in-process Parent, so Activity.Current becomes null — whereas the no-traceparent path restores the pump. This matches the prior behavior (the with-parentId path always did this), and handlers get their span explicitly via InitRequestContext(processSpan, ...) rather than from ambient Activity.Current, so nothing breaks. Just flagging the asymmetry since the restoration logic only engages for rooted spans.

Test coverage

• Good regression coverage: distinct TraceIds across messages, null ParentId on process spans, and Activity.Current correctly restored to the pump after each EndSpan.
• Minor / low-confidence: the new MessagePumpTraceIsolationObservabilityTests class is not in [Collection("Observability")], which the CommandProcessor observability tests use to serialize against cross-provider export interference. It also asserts over the shared collection via Assert.All(_exportedActivities.Where(... Process ...)). In practice the existing MessagePumpProcessSpanObservabilityTests uses the same MyTopic / MT_EVENT and a SingleOrDefault filter and passes, which suggests concurrent TracerProviders are not cross-contaminating here — so this is likely fine. Still, adding the collection attribute for consistency with the repo convention would be cheap insurance against flakiness.
• Nit: in the out-of-order test, an explicit Assert.Same(interveningActivity, Activity.Current) after ending the receive span would document the intended did-not-restore outcome more precisely than NotSame(pumpSpan, ...).

Overall this looks like a solid, well-reasoned fix with good guard-rail tests. The main thing I would like to understand is the receive-to-process correlation trade-off for no-traceparent messages.

[iancooper]

Let me know when you need a review?

[claude]

Code Review — PR #4166: Fix consumer OTel trace isolation

Thanks for this — the core change is well-motivated and the implementation is mostly clean. Forcing consumer spans to roots (with Activity.Current suppressed inside a try/finally) is the right way to break the unwanted shared-trace behavior, and expressing the pump relationship as an ActivityLink rather than parent/child is the correct reading of the OTel messaging semantic conventions. Good cleanup of the dead baggage local in CreateSpan, and the XML docs are thorough.

A few things to look at, one of which I think is a real defect.

🐛 Pump link is lost for producer-parented messages after the first loop iteration (likely bug)

The restore mechanism relies on StartRootActivity stamping PreviousActivityCustomPropertyName so that EndSpan can put the pump back into Activity.Current. But the producer-parented branch of StartConsumerActivity (when parentId is non-empty) does not go through StartRootActivity and never sets that custom property — it calls ActivitySource.StartActivity(... parentId: parentId, links: GetMessagePumpLinks(Activity.Current) ...) with no SetCustomProperty.

When an activity is started with an explicit parentId string, its Activity.Parent is null. So when EndSpan calls span.Dispose(), .NET sets Activity.Current = span.Parent = null. Tracing it through the real Reactor.Run() loop (Reactor.cs:97/117/185):

• Iteration 1: pump is current -> receive span (root) ends and restores pump -> CreateSpan(Process) for a producer-parented message links the pump correctly, then EndSpan(processSpan) leaves Activity.Current == null.
• Iteration 2+: CreateReceiveSpan now sees Activity.Current == null, so GetMessagePumpLinks(null) returns null -> no pump link, and the process span likewise gets no link.

Net effect: for the propagated-context (push) path, only the first message in a pump lifetime gets linked to the pump; every subsequent message silently loses the link. The no-TraceParent path is fine because it goes through StartRootActivity and restores correctly.

Suggested fix: have the producer-parented branch also capture the ambient pump and stamp the custom property so EndSpan restores it — e.g. activity.SetCustomProperty(PreviousActivityCustomPropertyName, current) after starting it (same as StartRootActivity does). That keeps Activity.Current == pump stable across iterations for both paths.

🧪 Test coverage gap that hides the above

When_Messages_Without_TraceParent_Are_Consumed_Should_Have_Distinct_Traces correctly drives two messages and asserts Assert.Same(pumpSpan, Activity.Current) after each EndSpan — that is exactly what catches the restore behavior on the root path. But When_A_Producer_Parented_Process_Span_Is_Created_Should_Link_The_Message_Pump_Span only processes a single message and never asserts Activity.Current after EndSpan, so the loop regression above is invisible. Worth adding a two-message producer-parented test asserting the pump link survives on the second message (it should fail today).

🔎 Minor

• Asymmetry by design? Root receive/process spans restore the pump; producer-parented process spans do not. Even if you decide the link-only-on-first-message behavior is acceptable, a comment in StartConsumerActivity documenting the intent would stop it reading as an oversight.
• Test file naming: When_Messages_Without_TraceParent_Are_Consumed_Should_Have_Distinct_Traces.cs also contains an unrelated second method (When_A_Root_Consumer_Span_Is_Ended_Out_Of_Order_Should_Not_Restore_Previous_Activity). The repo convention is one When_... behavior per file (matching the filename) — consider splitting it out.
• GetMessagePumpLinks allocates a fresh single-element array per consumer span; negligible, just noting it is on the hot path.

✅ Things that look correct

• try/finally in StartRootActivity restores Activity.Current even if StartActivity throws or returns null (no listeners).
• The Activity.Current == span guard in EndSpan correctly avoids clobbering current when spans end out of order — and it is tested.
• AddSource("Paramore.Brighter") matches BrighterSemanticConventions.SourceName, and ActivityContext value-equality makes the link.Context == pumpSpan.Context assertions valid.

Overall: solid direction; I would just like the producer-parented loop case addressed (or explicitly accepted with a test + comment) before merge.

[claude]

Code Review — Fix consumer OTel trace isolation (#4166)

Thanks for tackling this — the trace-pollution problem (unrelated consumed messages sharing one long-lived pump trace) is a real and worthwhile fix. Starting context-free consumer spans as roots while keeping a link back to the pump is the right shape and matches OTel messaging semantic conventions. The out-of-order EndSpan guard (Activity.Current == span) and its dedicated test are a nice defensive touch.

I do see one substantive correctness gap and a couple of smaller points.

🐞 Producer-parented spans don't restore Activity.Current → pump links lost after the first message

StartRootActivity carefully stashes the suppressed pump in PreviousActivityCustomPropertyName so EndSpan can restore Activity.Current back to the pump. But the producer-parented branch of StartConsumerActivity (when parentId is non-empty) does not set that custom property:

if (!string.IsNullOrEmpty(parentId))
{
    return ActivitySource.StartActivity(
        name: spanName, kind: kind, parentId: parentId,
        tags: tags, links: GetMessagePumpLinks(Activity.Current), startTime: startTime);
    // ^ no SetCustomProperty(PreviousActivity...) here
}

Because the activity's parent is a remote producer (a parentId string, not a live Activity), Activity.Dispose() in EndSpan sets Activity.Current to Parent → null, and the restore path is skipped (previousActivity is null). Walking the Reactor loop (single pump span created before the do loop, with per-iteration receive/process spans) for messages that do carry a TraceParent:

• Iter 1: receive span (root) restores Current = pump ✓ → process span links pump ✓ → EndSpan(process) leaves Current = null ✗
• Iter 2+: CreateReceiveSpan/CreateSpan now see Current = null, so GetMessagePumpLinks(null) returns null — every subsequent receive and process span loses its link back to the pump.

Trace isolation still holds (they're roots), but the pump correlation — the thing the link exists to provide — silently disappears from the second message onward whenever producer context is propagated, which is the common case. Suggested fix: have the producer-parented branch stash the pump and restore it the same way the root path does (e.g. set the custom property to Activity.Current and let EndSpan restore it), so Current returns to the pump after the span ends.

🧪 Test coverage gap

The isolation test (When_Messages_Without_TraceParent_Are_Consumed...) processes two messages and asserts Activity.Current == pump after each EndSpan — great. But the producer-parented test (When_A_Producer_Parented_Process_Span_Is_Created...) only processes one message and never asserts Activity.Current after EndSpan. That's exactly why the bug above goes uncaught. A two-message test where both carry a TraceParent, asserting the second iteration's spans still link the pump (and Current is restored to the pump between iterations), would lock this down.

Minor

• Unrelated cleanup: removing var baggage = message.Header.Baggage.ToString(); is correct (it was dead — baggage propagation goes through OpenTelemetry.Baggage.SetBaggage at line 215), but per the repo's change-scope guidance it's slightly outside the stated fix. Harmless; just flagging.
• Allocation: GetMessagePumpLinks allocates a fresh single-element ActivityLink[] per consume. Negligible relative to the surrounding tag serialization, so not worth changing — noting for completeness.
• XML docs / interface comments are clear and the updated summaries accurately describe the new root-span behavior. 👍

Summary

Direction and design are sound. The producer-parented restoration gap is the one item I'd want addressed before merge, since it regresses pump correlation for the propagated-context path (the typical one) after the first message, and it's currently untested.

[codescene-delta-analysis]

Code Health Improved (1 files improve in Code Health)

Our agent can fix these. Install it.

Gates Passed
4 Quality Gates Passed

View Improvements

File	Code Health Impact	Categories Improved
BrighterTracer.cs	7.09 → 7.53	Code Duplication, Overall Code Complexity

Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.

[claude]

Review: Fix consumer OTel trace isolation (#4166)

Thanks for tackling this — starting non-propagated consumer spans as roots (instead of letting them inherit the long-lived pump activity) is the right fix, and expressing the pump relationship as an ActivityLink rather than parent/child matches the OTel messaging semantic conventions. The new tests are well-structured and clearly commented. A few points below, one of which I think is a real correctness gap worth confirming.

🔴 Producer-parented process spans don't record PreviousActivity, so the pump link is likely lost after the first such message

StartConsumerActivity has two branches:

if (!string.IsNullOrEmpty(parentId))
{
    return ActivitySource.StartActivity(name, kind, parentId, tags,
        links: GetMessagePumpLinks(Activity.Current), startTime);   // (1) producer-parented
}
return StartRootActivity(spanName, kind, tags, startTime);          // (2) root

Only the root branch (StartRootActivity) records the PreviousActivity custom property, which is what EndSpan uses to restore the pump as Activity.Current:

var previousActivity = span.GetCustomProperty(PreviousActivityCustomPropertyName) as Activity;
var shouldRestorePreviousActivity = previousActivity is not null && Activity.Current == span;
...
if (shouldRestorePreviousActivity) Activity.Current = previousActivity;

The producer-parented branch (1) never sets that property. So when such a process span ends, the PR's restore logic is skipped and we fall back to .NET's own behavior: Activity.Stop() sets Activity.Current = span.Parent. For a span created from a remote parent id, Parent (the local Activity?) is null — so after the span ends, Activity.Current becomes null, not the pump.

In the real pump loop (Reactor.Run / Proactor.Run) the consequence is:

• Message 1 (propagated): receive span links the pump, process span links the pump, then process ends → Activity.Current == null.
• Message 2+ (propagated): CreateReceiveSpan → StartRootActivity reads Activity.Current (now null) → GetMessagePumpLinks(null) returns null → no pump link. Same for the process span.

So for a stream of propagated messages, only the first iteration links back to the pump. That is exactly what When_propagated_messages_are_consumed_should_keep_linking_the_message_pump_span asserts against — please double-check that test actually goes green in CI, because by my reading of Activity.Stop() semantics ambientBetweenIterations would be null rather than pumpSpan. (I could not run the suite in this environment to confirm.)

The root-span path works only because StartRootActivity explicitly suppresses Activity.Current to null and records PreviousActivity so EndSpan can put the pump back. The producer-parented path needs the same treatment.

Suggested fix: record PreviousActivity in the producer-parented branch too (or, more simply, capture Activity.Current once in CreateSpan and restore it uniformly in EndSpan regardless of which branch created the span). That keeps the "pump stays ambient across iterations" invariant for both propagated and non-propagated messages.

🟡 Minor

• Dead-variable cleanup: dropping the unused var baggage = message.Header.Baggage.ToString(); is a good catch (it was assigned but never read). Worth a mention given the repo's "keep changes scoped" guidance, but no objection.
• GetMessagePumpLinks allocates a fresh single-element ActivityLink[] per consumed message. Negligible on its own.
• Test source registration inconsistency: ProducerParentedProcessSpanLinkObservabilityTests and PropagatedMessagesKeepLinkingMessagePumpObservabilityTests register AddSource("Paramore.Brighter.Tests", "Paramore.Brighter"), while MessagePumpLinkObservabilityTests / MessagePumpTraceIsolationObservabilityTests register only "Paramore.Brighter". Not wrong, just inconsistent — worth aligning.

✅ Nice

• StartRootActivity correctly restores Activity.Current in a finally, so a throw inside StartActivity can't leave the ambient context suppressed.
• The out-of-order guard (Activity.Current == span before restoring) and its dedicated test (When_A_Root_Consumer_Span_Is_Ended_Out_Of_Order_...) are a thoughtful touch.
• Trace-isolation assertions (NotEqual(pumpSpan.TraceId, ...) and Null(processSpan.ParentId)) directly pin the behavior the PR sets out to fix.

Overall the approach is sound; the producer-parented restore gap is the one item I'd want resolved (and the propagated-context test re-verified) before merge.

🤖 Generated with Claude Code

[thomhurst]

Confirming the 🔴 "producer-parented spans don't restore Activity.Current" concern is a false positive — the suite is green, and the test that targets exactly this scenario passes.

When_propagated_messages_are_consumed_should_keep_linking_the_message_pump_span (added in e74e0e2) passes on both target frameworks:

Passed!  - Failed: 0, Passed: 1, Skipped: 0, Total: 1 - Paramore.Brighter.Core.Tests.dll (net9.0)
Passed!  - Failed: 0, Passed: 1, Skipped: 0, Total: 1 - Paramore.Brighter.Core.Tests.dll (net10.0)

The reasoning gap is the model of Activity.Stop(). It does not restore Activity.Current to span.Parent. It restores Activity.Current to whatever was current when the span started. The producer-parented branch never suppresses Activity.Current (only StartRootActivity does, via Activity.Current = null), so the pump span is the captured restore target and Activity.Current returns to the pump when the span ends — no PreviousActivity property is needed on that path.

Probing the exact two-propagated-message loop (Begin pump span, then receive + process per message, both messages carrying a TraceParent):

m1.traceparent / m2.traceparent : both set (producer-parented branch taken)
afterProcess1 == pump?           : True   (not null)
receive2.linksPump?              : True
process2.linksPump?              : True

So pump correlation is retained on every iteration, not just the first. The PreviousActivity restore mechanism is required only by StartRootActivity, precisely because that is the one path that nulls Activity.Current during creation.

(The minor note about test source registration is correct but pre-existing; the two-source registration is required here so the synthetic producer activity samples and writes a real TraceParent.)

[iancooper]

Thanks @thomhurst