feat(aspire): emit client spans for HTTP

[thomhurst]

Summary

Follow-up to #5665 focused on trace topology rather than OTLP forwarding.

• create a real TUnit.Aspire.Http client span around AspireFixture.CreateHttpClient requests
• propagate traceparent and baggage from that client span, with fallback to the ambient activity when no listener is attached
• export the new source from TUnit.OpenTelemetry.AutoStart
• update BaggagePropagationHandlerTests to assert client span creation, parent-child shape, response tagging, and fallback behavior

Why

#5665 fixes forwarding to user dashboards, but the Aspire handler still propagated the ambient test span directly. That kept correlation working while flattening the distributed trace tree in backends like the Aspire dashboard.

This change restores the normal OTel client/server shape:
test case -> test body -> TUnit.Aspire.Http client span -> SUT server span

Refs #4818

Test plan

• dotnet test TUnit.Aspire.Tests/TUnit.Aspire.Tests.csproj -c Release -- --treenode-filter "/*/*/BaggagePropagationHandlerTests/*"
• dotnet test TUnit.OpenTelemetry.Tests/TUnit.OpenTelemetry.Tests.csproj -c Release -- --treenode-filter "/*/*/AutoStartTests/*"
• dotnet build TUnit.Aspire.Tests/TUnit.Aspire.Tests.csproj -c Release --no-restore
• dotnet build TUnit.OpenTelemetry.Tests/TUnit.OpenTelemetry.Tests.csproj -c Release --no-restore

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes. Give us feedback}

[claude]

Code Review: PR #5666 — feat(aspire): emit client spans for HTTP

Summary

This PR is a follow-up to #5665. Where #5665 fixed OTLP forwarding to user-hosted Aspire dashboards, this PR restores the proper OTel client/server span topology for HTTP requests made through AspireFixture.CreateHttpClient. Previously, the handler propagated the ambient test span directly as the parent, which flattened the distributed trace tree in backends. Now it creates a proper TUnit.Aspire.Http client span so the hierarchy is:

test case -> test body -> TUnit.Aspire.Http client span -> SUT server span

The implementation mirrors the existing ActivityPropagationHandler in TUnit.AspNetCore.Core almost exactly, which is intentional and good for consistency.

---

Positive Observations

The design is correct and well-reasoned. Capturing ambientActivity before calling _startActivity(), then using activity ?? ambientActivity as the propagation source, handles both the "listener attached" and "no listener" cases cleanly without branching the hot path.

The fallback behavior is explicit and testable. Injecting Func<HttpRequestMessage, Activity?> into the internal constructor is exactly the right seam for testing. It lets tests simulate "no listener" (static _ => null) and "listener attached" (default ctor) without requiring process-global listener setup in every test.

RecordedActivity in the test is well-structured. Snapshotting the activity's data in ActivityStopped into an immutable record prevents TOCTOU races where assertions run after the Activity object is recycled. This is the correct approach.

[NotInParallel] is correctly applied. Activity.Current is an AsyncLocal so individual tests are isolated within the class, but the global ActivitySource.AddActivityListener registration in ActivityListenerScope is process-wide. Serializing the tests prevents a listener scope from one test capturing spans emitted by a concurrently running test in the same class.

The CopyBaggage guard destination.GetBaggageItem(key) is not null is correct. It avoids overwriting explicitly-set baggage on the synthesized span. The ReferenceEquals short-circuit prevents pointless self-copy when no span is created.

AspireHttpSourceName as a public const in TUnitActivitySource gives users a stable, discoverable string for configuring their own TracerProvider without magic string duplication.

---

Issues

1. (Medium) OTel HTTP Semantic Convention: !IsSuccessStatusCode is too broad for span error status

File: TUnit.Aspire/Http/TUnitBaggagePropagationHandler.cs

if (!response.IsSuccessStatusCode)
{
    activity.SetStatus(ActivityStatusCode.Error);
}

Per the OTel HTTP semantic conventions for client spans, the span status should only be set to Error for 5xx responses. 4xx responses indicate a correctly received error from the client's perspective and should not set ActivityStatusCode.Error — the http.response.status_code tag already conveys the outcome. Marking a 404 as Error produces false-positive error rates in APM dashboards.

The same issue exists in the sibling ActivityPropagationHandler in TUnit.AspNetCore.Core, so now that this PR is hardening the OTel semantics, it's worth aligning both.

Suggested fix:

if ((int)response.StatusCode >= 500)
{
    activity.SetStatus(ActivityStatusCode.Error);
}

If you make this fix, update SendAsync_ClientSpan_RecordsResponseMetadata to use a 500 response for the error-status assertion, and add a separate assertion that a 404 does not set Error status.

---

2. (Low) Missing error status and exception recording when base.SendAsync throws

File: TUnit.Aspire/Http/TUnitBaggagePropagationHandler.cs

If base.SendAsync throws (network timeout, connection refused, DNS failure), activity.SetStatus(ActivityStatusCode.Error) is never called and no exception event is recorded. The span appears as successful with no http.response.status_code tag, which is misleading.

Suggested fix:

HttpResponseMessage response;
try
{
    response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false);
}
catch (Exception ex)
{
    if (activity is not null)
    {
        activity.SetStatus(ActivityStatusCode.Error, ex.Message);
        activity.SetTag("error.type", ex.GetType().FullName);
    }
    throw;
}

The same gap exists in ActivityPropagationHandler.

---

3. (Low) W3C traceparent format and multi-item baggage format are no longer explicitly tested

The removed SendAsync_TraceparentFormat_IsValidW3C and SendAsync_MultipleBaggageItems_CommaSeparated tests covered format correctness that the new tests do not verify. The new tests implicitly rely on the format (indexing into parts[1], parts[2]), but:

• parts.Length == 4 is never asserted
• The baggage comma-separator for multi-item payloads through the new client span path is not verified

These are minor coverage gaps worth noting for future regressions if the propagator internals change.

---

4. (Nit) CreateHandler factory — null means "use real default", not "no-op"

File: TUnit.Aspire.Tests/BaggagePropagationHandlerTests.cs

The optional startActivity parameter being null invokes the real StartHttpActivity factory — to get a no-op you must pass static _ => null. Since the type is Func?, null intuitively reads as "no factory". A brief doc comment on the parameter would prevent future confusion:

// Pass static _ => null to simulate no listener; null uses the real StartHttpActivity default.

---

5. (Nit) Implicit ordering assumption on spans[0]/spans[1]

File: TUnit.Aspire.Tests/BaggagePropagationHandlerTests.cs

The assumption that spans[0] corresponds to the first request and spans[1] to the second is valid (requests are await'd sequentially and ActivityStopped fires synchronously before the next request starts), but a short comment would make this determinism explicit for future maintainers.

---

Summary

The core design is sound — the PR achieves its goal of restoring the proper client/server span topology with correct fallback behavior. The test rewrite is a meaningful improvement: the new tests actually verify span hierarchy, baggage propagation through the span, and response tagging, which the old tests did not.

The most actionable issues are the OTel spec deviation on 4xx error status (item 1) and the missing transport-error handling (item 2), both of which are shared with the sibling ActivityPropagationHandler in TUnit.AspNetCore.Core. Fixing them here without fixing the sibling handler would leave an inconsistency.

[thomhurst]

Addressed the review with a follow-up push.

Changes made:

• record thrown transport exceptions on synthesized HTTP client spans in both handlers
• add explicit coverage for thrown inner-handler failures, traceparent format, and multi-item baggage
• align HTTP status handling in both handlers to the current OTel HTTP semconv: 3xx leaves status unset, 4xx/5xx set Error with error.type

On the 4xx point specifically: the current OpenTelemetry HTTP span spec says 4xx SHOULD be Error for SpanKind.CLIENT, while 1xx/2xx/3xx stay unset unless there was another error:
https://opentelemetry.io/docs/specs/semconv/http/http-spans/

[claude]

Code Review: feat(aspire): emit client spans for HTTP

This PR does exactly what it says — it restores normal OTel client/server topology for Aspire HTTP requests by creating a real TUnit.Aspire.Http client span inside TUnitBaggagePropagationHandler. The approach is sound and the test coverage is thorough. Below are observations ranging from design concerns to minor consistency notes.

---

1. Near-identical RecordException implementations across three locations

RecordException appears identically (or near-identically) in three places:

• TUnitActivitySource.RecordException (already internal static, the canonical one)
• ActivityPropagationHandler.RecordException (private static, added in #5665 presumably)
• TUnitBaggagePropagationHandler.RecordException (private static, added in this PR)

Both handler-level copies duplicate the exact same ActivityTagsCollection construction and AddEvent/SetTag/SetStatus calls. Since TUnitActivitySource.RecordException is already internal static and accessible from both handler assemblies, the handler-level copies could simply delegate to it:

private static void RecordException(Activity? activity, Exception exception)
    => TUnitActivitySource.RecordException(activity, exception);

Or just call TUnitActivitySource.RecordException(activity, ex) directly at the call site and drop the private wrapper entirely. The duplication is low-risk today because all three copies are identical, but it becomes a maintenance hazard if the OTel semantic conventions for exception.* tags evolve.

---

2. exception.GetType().FullName can return null

Type.FullName returns null for generic type parameters and certain constructed generic types (per the BCR docs). For the exception recording use case this is unlikely to bite in practice (concrete exception types always have a FullName), but it silently sets the exception.type/error.type tags to null. The existing TUnitActivitySource.RecordException has the same pattern, so this is pre-existing, but worth a fallback while touching the code:

{ "exception.type", exception.GetType().FullName ?? exception.GetType().Name },

---

3. using var activity on the shared-handler path warrants a comment

In AspireFixture.CreateHttpClient, a single TUnitBaggagePropagationHandler instance is reused across all HttpClient instances for the life of the fixture (_httpHandler ??= ...). Each SendAsync call does using var activity = _startActivity(request), which is correct — the Activity is created and disposed per-request, not per-handler-instance. This is safe because Activity is thread-safe to create/dispose concurrently.

The comment in AspireFixture ("The handler is stateless (reads Activity.Current which is async-local/per-test)") is accurate for the old code but slightly incomplete now: the handler now also creates activities per-request. The comment should be updated to reflect that activity creation is also per-request and safe to share, e.g.:

The handler is stateless: Activity.Current is async-local (per-test), and each SendAsync call creates and disposes its own Activity span, so sharing the handler instance across tests is safe.

---

4. [NotInParallel] on the test class — correct choice, but consider whether it's too broad

Both BaggagePropagationHandlerTests and ActivityPropagationHandlerTests are annotated [NotInParallel]. This is the right call because ActivitySource.AddActivityListener is a global, process-wide side effect and the ActivityListenerScope tear-down races with other tests that might be starting listeners concurrently.

However, [NotInParallel(nameof(BaggagePropagationHandlerTests))] serializes all tests in the class relative to each other, but it still lets them run in parallel with tests in other classes that also mutate Activity.Current or register listeners on the same source names. If the ActivityListenerScope in one test accidentally captures spans from another concurrent test on the same source, assertions like HasSingleItem will fail flakily.

The current listener filters on the source name (TUnitActivitySource.AspireHttpSourceName / TUnitActivitySource.AspNetCoreHttpSourceName), which scopes capture to just that source — but since both test files use those sources in their ActivityListenerScope, any test-level parallelism within the same source would still cause interference. The [NotInParallel] serializes within the class, so this is fine as long as the two test classes don't run concurrently. Worth a comment to make the intent explicit.

---

5. Minor: InjectTraceContext signature asymmetry between the two handlers

ActivityPropagationHandler.InjectTraceContext takes HttpRequestHeaders while TUnitBaggagePropagationHandler.InjectTraceContext takes HttpRequestMessage. The Aspire handler then accesses request.Headers inside its inject callback. This is a trivial detail but it means the two otherwise-identical handlers have a slightly different signature for the same helper. No action needed unless a future unification effort is planned.

---

6. CopyBaggage is also duplicated

CopyBaggage (the helper that prevents double-writing baggage onto the synthesized activity) is identical in both handlers. Since this is internal infrastructure, extracting it to a shared location (e.g. TUnitActivitySource or a new TUnitHttpPropagationHelpers internal class) would keep both handlers DRY. Lower priority than the RecordException duplication since CopyBaggage is a more specialized concern.

---

Summary

The core change — synthesizing a proper client span and propagating its context downstream — is well-designed and correctly implemented. The test coverage is comprehensive (3xx vs 4xx status, transport exceptions, per-request span uniqueness, parent/child shape, baggage forwarding). The main structural feedback is deduplication of RecordException (and to a lesser extent CopyBaggage) by delegating to the existing TUnitActivitySource helpers, and keeping the AspireFixture comment accurate now that the handler also emits spans.

[thomhurst]

Folded in the cleanup suggestions in 4ae5f04:\n\n- reused TUnitActivitySource.RecordException(...) from both HTTP handlers instead of keeping three near-identical copies\n- added a FullName ?? Name fallback for the recorded exception type\n- updated the AspireFixture.CreateHttpClient comment to call out that the shared handler creates/disposes a per-request client span\n\nI left CopyBaggage as-is for now since that felt like a lower-priority refactor than the exception path cleanup.

[claude]

Code Review: PR #5666 — feat(aspire): emit client spans for HTTP

This PR restores proper OTel client/server span topology for AspireFixture.CreateHttpClient requests. The core goal is sound and the implementation is well-executed. Here are my observations:

---

What's done well

Correct span lifecycle design. Capturing ambientActivity before _startActivity(request) is called, then using activity ?? ambientActivity as the propagation source, handles both the "listener attached" and "no listener" (fallback) cases cleanly in a single code path.

Testing seam is exactly right. The Func<HttpRequestMessage, Activity?> _startActivity injection via the internal constructor gives tests full control over whether a span is synthesized, without requiring process-global listener setup in every test case. The distinction between static _ => null (no helper span) and the default constructor (real span) makes the test intent unambiguous.

RecordedActivity snapshot pattern is correct. Copying activity data in ActivityStopped into an immutable record prevents TOCTOU races where assertions execute after the Activity object has been stopped/recycled. This is exactly the right approach for async test assertions against spans.

[NotInParallel] is correctly applied. ActivitySource.AddActivityListener is a process-global operation. Without serialising tests within the class, a listener scope created by one test can capture spans from a concurrently running test in the same class. Scoping the constraint to the class name (rather than a global lock) is appropriately targeted.

CopyBaggage guard is correct. Checking destination.GetBaggageItem(key) is not null before overwriting prevents accidentally clobbering baggage that was set on the child span itself. The ReferenceEquals short-circuit is a nice defensive touch.

AspireHttpSourceName as public const in TUnitActivitySource gives users a stable, discoverable, zero-allocation string for configuring their own TracerProvider, and avoids magic string duplication across the assembly boundary.

---

Issues

1. OTel HTTP Semantic Convention: 4xx should not set span status to Error

Files: TUnit.Aspire/Http/TUnitBaggagePropagationHandler.cs (line 75–79) and the mirrored code in TUnit.AspNetCore.Core/Http/ActivityPropagationHandler.cs (line 68–72)

Per the OTel HTTP semantic conventions for client spans, the span Status should only be set to Error for 5xx responses. 4xx responses indicate the request was correctly received and understood by the server — the http.response.status_code attribute already conveys the outcome. Setting Error on a 404 produces false-positive error rates in APM dashboards (every "resource not found" probe becomes an error span).

Both handlers currently use >= 400:

if (statusCode >= 400)
{
    activity.SetStatus(ActivityStatusCode.Error);
    activity.SetTag("error.type", statusCode.ToString());
}

The OTel spec says the threshold should be >= 500 for client spans (unlike server spans, where >= 500 is also the boundary). The error.type tag being set for 4xx is also spec-incorrect for the same reason.

The existing tests (SendAsync_ClientSpan_4xxStatus_SetsErrorStatus) codify the current incorrect behavior and would need to be updated alongside the fix.

2. Missing span activity when exception occurs before the span is created

File: TUnit.Aspire/Http/TUnitBaggagePropagationHandler.cs (line 66–68)

catch (Exception ex)
{
    TUnitActivitySource.RecordException(activity, ex);
    throw;
}

If _startActivity(request) itself throws (unlikely but theoretically possible with a user-supplied delegate), no span exists to record to — RecordException gracefully handles null here, so this is fine. However, if base.SendAsync throws and activity is null (because no listener was attached), the exception is silently swallowed from the span perspective with no fallback to ambientActivity. This is acceptable behaviour, but worth a comment noting that exception recording only applies to the synthesized client span, not the ambient test span.

3. Minor: ActivityListenerScope is duplicated across both test files

ActivityListenerScope, RecordedActivity, CaptureHandler, and ThrowingHandler are defined identically (or near-identically) in both BaggagePropagationHandlerTests.cs and ActivityPropagationHandlerTests.cs. The only difference is which source name ShouldListenTo filters on, and RecordedActivity's BaggagePropagationHandlerTests version carries two additional fields (TraceId, ParentSpanId, DisplayName, Kind).

This isn't a blocker — test helpers duplicated in separate test projects are normal — but if both test projects share a TUnit.Tests.Common or similar test utilities project, consolidating would reduce future drift.

4. Minor: SendAsync_MultipleBaggageItems_CommaSeparated uses a listener scope it doesn't assert against

In BaggagePropagationHandlerTests.cs, SendAsync_MultipleBaggageItems_CommaSeparated creates a listenerScope but the test only asserts on the baggage header contents. The listenerScope is unused for assertions, which is misleading — a reader might expect a span assertion is coming. Consider removing the scope from that test since CreateHandler() (with the listener) can be replaced by CreateHandler(static _ => null) without changing the test's intent, which is just about the baggage header format.

---

Summary

The design is correct and the implementation is clean. Issue #1 (OTel HTTP semantic conventions for 4xx vs 5xx error status) is worth addressing before merge — it's a correctness issue with observable consequences in real dashboards. The remaining items are minor quality notes.

[thomhurst]

Took the small cleanup in 6631e07:\n\n- clarified in both handlers that exception recording only applies when a synthesized client span exists\n- tightened SendAsync_MultipleBaggageItems_CommaSeparated so the listener scope is now asserted instead of incidental\n\nOn the 4xx point, I re-checked the current OpenTelemetry HTTP span status guidance and it still says 4xx should remain unset for server spans but should be marked Error for client spans when the instrumentation has no extra request context, so I’m keeping the >= 400 client behavior in both handlers:\nhttps://opentelemetry.io/docs/specs/semconv/http/http-spans/#status

[claude]

Code Review: PR #5666 — feat(aspire): emit client spans for HTTP (Round 4)

This is the fourth review pass. The PR has been through three rounds of substantive feedback — this review checks what's been addressed, what remains, and raises one new observation.

---

Previous Feedback Resolution

All major feedback from earlier reviews has been addressed:

Item	Status
Exception recording when base.SendAsync throws	✅ Fixed — try/catch added in both handlers, delegates to TUnitActivitySource.RecordException
FullName returning null for constructed generics	✅ Fixed — FullName ?? Name fallback added to TUnitActivitySource.RecordException
RecordException duplication across three locations	✅ Fixed — both handlers now call TUnitActivitySource.RecordException directly
[NotInParallel] on both test classes	✅ Added
parts.Length == 4 assertion	✅ Added to both test files
spans[0]/spans[1] ordering assumption undocumented	✅ Comment added: "Requests are awaited sequentially, so ActivityStopped fires in request order"
null vs static _ => null confusion on CreateHandler	✅ Comment added clarifying the distinction
AspireFixture.CreateHttpClient comment accuracy	✅ Updated to describe per-request span lifecycle
MultipleBaggageItems_CommaSeparated unused listener scope	✅ Fixed — test now asserts HasSingleItem on stopped activities

On the 4xx status debate: The author is correct. Per the OTel HTTP semantic conventions for client spans, 4xx responses SHOULD set span status to Error for SpanKind.CLIENT. The >= 400 threshold is spec-compliant. Earlier review rounds on this point were wrong.

---

New Observation

Span name format includes a non-standard "HTTP " prefix

File: TUnit.Aspire/Http/TUnitBaggagePropagationHandler.cs (line ~802)

private static Activity? StartHttpActivity(HttpRequestMessage request)
{
    var path = request.RequestUri?.AbsolutePath ?? ...;
    return HttpActivitySource.StartActivity(
        $"HTTP {request.Method} {path}",  // → "HTTP GET /test"
        ActivityKind.Client);
}

The "HTTP " prefix produces span names like "HTTP GET /test". The OTel HTTP semconv for client span names specifies {method} {route-template} (e.g. "GET /users/:id") or just {method} when no template is available — not "HTTP {method} {path}". Standard .NET HTTP client instrumentation (System.Net.Http activity source) produces "GET".

The "HTTP " prefix doesn't cause a functional problem, but it deviates from OTel naming conventions and will look inconsistent alongside standard spans in Aspire/Jaeger dashboards. The mirrored ActivityPropagationHandler in TUnit.AspNetCore.Core will likely have the same name format — worth checking before AspNetCoreHttpSourceName gets wider use.

Suggested fix: drop the prefix so the span name is $"{request.Method} {path}" → "GET /test". Update the assertion in SendAsync_InjectsTraceContext_FromCreatedClientSpan_WhenHelperSpanIsCreated accordingly ("GET /test" instead of "HTTP GET /test").

---

Summary

The implementation is solid and all substantive issues from three review rounds are resolved. The 4xx-as-Error behavior is OTel spec-correct. The only remaining callout is the span name format, which is a correctness-against-spec issue rather than a blocker — it affects how spans appear in dashboards but not trace propagation or baggage correctness. The test rewrite is comprehensive and the RecordedActivity snapshot pattern is exactly right for async-safe assertions.

[thomhurst]

Took the span-name cleanup in b188f81.\n\nI ended up going a little further than just dropping the HTTP prefix: both handlers now emit method-only client span names (GET, POST, etc.), and both test projects assert that explicitly.\n\nReason: the current HTTP semconv says span names should be {method} {target} only when a low-cardinality target is available, otherwise just {method}, and instrumentation must not default the target to the raw URI path. Since these helpers don't have a route template/url template, method-only is the closest spec-aligned name here:\nhttps://opentelemetry.io/docs/specs/semconv/http/http-spans/#name

[claude]

Code Review: PR #5666 — feat(aspire): emit client spans for HTTP (Round 5)

This is the fifth and likely final review pass. All feedback from Round 4 has been addressed.

---

Previous Feedback Resolution

Item	Status
Span name: drop the "HTTP " prefix	✅ Fixed — span name is now just the HTTP method ("GET")
All items from Rounds 1–3	✅ Carried forward and confirmed resolved

On the span name choice: Round 4 suggested "GET /path" but the author chose just "GET". This is actually the better call — standard .NET System.Net.Http instrumentation also emits "GET", so the span name is now consistent with what users see from the built-in HTTP client source. The path is available via url.full tag anyway.

---

One Minor Observation

Incomplete comment on CopyBaggage in TUnitBaggagePropagationHandler

TUnitBaggagePropagationHandler.cs (line ~51):

// Aspire's CreateHttpClient bypasses DiagnosticsHandler, so when we synthesize
// a client span we also need to flow the ambient baggage onto it explicitly.
CopyBaggage(ambientActivity, activity);

The parallel comment in ActivityPropagationHandler.cs (lines 43–46) carries an additional — and more important — clarifying clause:

"Child Activities do not reliably surface parent baggage across all target frameworks, but correlation relies on the test's baggage being propagated."

Since the cross-framework reliability concern is the real reason the explicit copy is needed (both handlers target net8.0;net9.0;net10.0), the Aspire handler's comment would benefit from the same second sentence. A reader of just the Aspire handler might wonder why the explicit copy is necessary if child activities inherit parent baggage through the chain. Not a blocker, just a documentation gap.

---

Summary

The implementation is correct, the test coverage is comprehensive, and all structural issues from prior rounds have been resolved. The only callout is a one-line comment improvement. Approved.