feat: generalize OTLP receiver for use outside TUnit.Aspire

[thomhurst]

Summary

• Extracts OtlpReceiver + OtlpLogParser from TUnit.Aspire into TUnit.OpenTelemetry, adds OtlpTraceParser, and auto-starts the receiver at test discovery so non-Aspire tests (plain WebApplicationFactory, spawned processes, testcontainers, external services) can export spans into TUnit's HTML report over OTLP/HTTP.
• ActivityCollector gains RegisterExternalTrace / IngestExternalSpan + a process-wide Current pointer so the receiver routes spans without explicit wiring. Unknown trace IDs are dropped; registered traces capped at 100 external spans. Opt out with TUNIT_OTEL_RECEIVER=0.
• Docs: new "Capturing spans from out-of-process SUTs" section in distributed-tracing.md; comparison table updated.

Closes #5595. Builds on #5602.

Test plan

• TUnit.OpenTelemetry.Tests — 27 tests, 3 consecutive runs green (covers ingestion, caps, auto-start hook, OTLP parser, receiver round-trip, upstream forwarding).
• TUnit.Aspire + TUnit.Aspire.Tests still build — Aspire consumes the extracted receiver via the new namespace.
• Full solution build clean.
• CI green on PR.

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 critical · 1 high · 2 minor

Alerts:
⚠ 4 issues (≤ 0 issues of at least minor severity)

Results:
4 new issues

Category	Results
Security	1 critical 1 high
CodeStyle	2 minor

View in Codacy

🟢 Metrics 266 complexity

Metric	Results
Complexity	266

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[claude]

Code Review — PR #5606: Generalize OTLP receiver for use outside TUnit.Aspire

Overall this is a well-motivated refactor that makes the OTLP receiver broadly useful. The extraction from TUnit.Aspire, the opt-out env var, and the test coverage are all solid. A few architectural concerns are worth addressing before merge.

---

Major: Tight coupling via InternalsVisibleTo + static singleton

The core design issue is that TUnit.OpenTelemetry (a library) reaches into TUnit.Engine internals via ActivityCollector.Current?.IngestExternalSpan(span). This requires InternalsVisibleTo entries in both TUnit.Core and TUnit.Engine, and creates a compile-time dependency from the library to the runner's internal types.

This inverts the correct dependency direction. The engine is the host; the library should not know about the engine's internals.

Better approach: Define a public interface in TUnit.Core (already a dependency of both):

// TUnit.Core/IOtlpSpanSink.cs
public interface IOtlpSpanSink
{
    void IngestExternalSpan(SpanData span);
    void RegisterExternalTrace(string traceId);
}

ActivityCollector implements it. OtlpReceiver accepts an IOtlpSpanSink? callback (injected by AutoReceiver, which is allowed to see the engine). This eliminates the static singleton, removes the InternalsVisibleTo hacks, and makes the data flow explicit.

---

Major: ActivityCollector._current is a mutable process-wide static

private static ActivityCollector? _current;
public static ActivityCollector? Current => _current;

Two concerns:

1. Thread safety: The PR description mentions Interlocked.CompareExchange but the field is just directly assigned. A naive assignment is torn on 32-bit platforms (unlikely in practice, but the intent was an atomic swap). Either use Volatile.Write/Volatile.Read or Interlocked.Exchange consistently.

2. Multiple sessions in one process: If the host ever runs back-to-back test sessions in the same process (e.g., in test-on-save tooling), Stop() clears _current but a new Start() must win the race before the next session emits spans. The current code handles this with a null check in Start(), but stopping the receiver and clearing _current are two separate steps — a window exists where a span from the new session is dropped.

Both issues go away with the interface injection approach above.

---

Moderate: Hand-rolled 429-line protobuf parser

OtlpTraceParser.cs is a significant maintenance liability:

• Field numbers are magic integers (case 1, case 9, case 15) with no reference to the proto field names. Future maintainers cannot easily verify correctness.
• ParseAnyValue silently skips array_value (field 5) and kvlist_value (field 6), which are legal in OTLP attributes. A span with a nested-array attribute produces an empty string with no warning.
• If the OTLP spec evolves (new wire fields, packed repeated fields for timestamps), this parser will silently misparse.

At minimum, add a link to the proto definition in a header comment:

// Parses opentelemetry.proto.trace.v1.ExportTraceServiceRequest
// Spec: https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/trace/v1/trace.proto

Longer term, consider consuming OpenTelemetry.Proto.* NuGet packages if they're already transitive — they provide generated, versioned types with no parsing risk.

---

Minor: Hardcoded cap of 100 is silent data loss

private const int MaxExternalSpansPerTest = 100;
private const int MaxExternalSpansPerTrace = 100;

A SUT with many child spans (e.g., a database-heavy integration test) silently loses spans beyond 100 with no log warning and no way to raise the limit. Consider:

• Adding a one-time warning log when the cap is hit: "[TUnit.OpenTelemetry] External span cap (100) reached for trace {traceId}; additional spans dropped."
• Making the limit configurable via environment variable (consistent with the TUNIT_OTEL_RECEIVER=0 opt-out pattern).

---

Minor: Duplicate test helper

OtlpTraceCaptureServer exists in both TUnit.Aspire.Tests/Helpers/ and TUnit.OpenTelemetry.Tests/Helpers/ with the same interface but different implementations. The Aspire one was simplified to call LoopbackHttpListenerFactory.Create(), so the duplication is already partially consolidated — but the capture/wait logic is still duplicated. Consider moving it to a TUnit.OpenTelemetry.Testing or TUnit.TestUtilities shared project.

---

Positive notes

• The LoopbackHttpListenerFactory extraction to deduplicate port-binding retry logic is clean.
• [Before(HookType.TestDiscovery)] ordering ensures the receiver is up before any test-startup code runs.
• The TUNIT_OTEL_RECEIVER=0 opt-out is idiomatic and discoverable.
• Test coverage for cap enforcement and unknown-trace dropping is exactly the right edge-case coverage.
• Docs section with end-to-end example is helpful.

---

Summary: The extraction is the right architectural move. The main ask is to decouple TUnit.OpenTelemetry from TUnit.Engine internals — the InternalsVisibleTo+static-singleton coupling is the kind of thing that compounds over time. The interface injection approach resolves that and the thread-safety concern together.

[thomhurst]

Thanks for the review. Addressed two concrete items in 3e73e07; responding inline to the rest.

Fixed

• Moderate (protobuf docs) — added proto spec URLs + field-name comments at the top of OtlpTraceParser / OtlpLogParser so magic field numbers can be cross-referenced.
• Minor (configurable cap) — external span cap now overridable via TUNIT_OTEL_MAX_EXTERNAL_SPANS, with a one-time stderr warning when first hit so users can discover the knob.

Discussing

• Major 1 (IVT + static singleton) — keeping as-is. TUnit.OpenTelemetry and TUnit.Engine are sibling assemblies shipped together; IVT is the established pattern here (same relationship as TUnit ↔ TUnit.Engine). An interface + service-locator would add public API surface for a single implementation, and the whole point of this PR is zero-wiring auto-ingestion. Interlocked.CompareExchange(ref _current, this, null) already gives first-wins semantics.
• Major 2 Repeat attributes  #1 (singleton race) — I think there's a misread: Start() uses Interlocked.CompareExchange(ref _current, this, null) (line 44) and Stop() uses Interlocked.CompareExchange(ref _current, null, this) (line 125). CompareExchange is already there.
• Major 2 Retry attribute  #2 (multi-session swap vs. in-flight spans) — acknowledged but theoretical in practice: HtmlReporter creates one collector per session before any test runs, so within a single process the slot is claimed and held. Cross-session swapping would require multiple concurrent HtmlReporter instances in one process, which isn't a supported scenario today. Happy to revisit if someone hits it.
• Minor Retry attribute  #2 (helper duplication) — the two OtlpTraceCaptureServer copies already share the extracted LoopbackHttpListenerFactory for the tricky port-binding logic. Full consolidation would require a new shared test helper assembly; the remaining duplication is thin glue (HttpListener lifecycle + waiter plumbing).

[claude]

Code Review

Thanks for addressing the previous feedback — the configurable span cap with a one-time warning is a nice UX touch, and the proto spec URL references make the hand-rolled parser much more maintainable.

Bug: Case-sensitivity mismatch in RegisterExternalTrace

Convert.ToHexString() (used in the parser to decode span/trace IDs) returns uppercase hex. Activity.TraceId.ToString() returns lowercase hex. The docs tell users to call .ToUpperInvariant(), but if they don't, spans are silently dropped with no warning or error — a hard-to-diagnose footgun.

The fix belongs in the API, not in the caller:

// ActivityCollector.cs
internal void RegisterExternalTrace(string traceId)
{
    _knownTraceIds.TryAdd(traceId.ToUpperInvariant(), 0);
}

Or initialise the dictionary with StringComparer.OrdinalIgnoreCase. Either way, callers shouldn't need to know that the parser produces uppercase hex internally.

---

Design question: SpanType = span.Name in ToSpanData

In OtlpReceiver.cs:

return new SpanData
{
    Name = span.Name,
    SpanType = span.Name,  // same value as Name?
    Kind = MapSpanKind(span.Kind),
    ...
};

If the HTML report renders SpanType as a separate column or label from Name, this is redundant/confusing. If SpanType is meant to communicate the instrumentation scope or kind, it should be span.ScopeName or MapSpanKind(span.Kind). Worth a quick check against how the HTML template consumes SpanType on in-process spans to confirm the right mapping.

---

Addressed from previous round ✅

• Proto spec URLs added to OtlpTraceParser/OtlpLogParser — excellent
• TUNIT_OTEL_MAX_EXTERNAL_SPANS override + one-time stderr warning — good UX
• LoopbackHttpListenerFactory extraction eliminates the duplicated port-probing logic cleanly

Author's responses acknowledged

The defence of the IVT + Interlocked.CompareExchange-based singleton is reasonable given that TUnit.OpenTelemetry and TUnit.Engine ship together as sibling assemblies. The one-session-one-collector invariant is a sound constraint, and the CompareExchange already achieves first-wins semantics correctly.

The case-sensitivity issue above is the only concrete bug I can see in the new code. Everything else looks solid.

[claude]

Code Review — feat: generalize OTLP receiver for use outside TUnit.Aspire

This is a well-structured PR that delivers real value: non-Aspire tests can now capture out-of-process spans into the HTML report with near-zero ceremony. The extraction of OtlpReceiver/OtlpLogParser into TUnit.OpenTelemetry, the hand-rolled OtlpTraceParser (no external proto dep, AOT-safe), the LoopbackHttpListenerFactory refactor, the Interlocked.CompareExchange one-shot warning latch, and the configurable cap via env var are all solid. Test coverage is thorough. One real bug and a few architectural points below.

---

Bug: span ID case mismatch in IngestExternalSpan

_testCaseSpanIds is populated with lowercase keys:

// ActivityCollector.cs line 168
_testCaseSpanIds.TryAdd(activity.SpanId.ToString(), 0);
//                                    ^^ lowercase hex

But OtlpTraceParser produces uppercase IDs:

// OtlpTraceParser.cs — fields 2 and 4
parentSpanId = Convert.ToHexString(parentBytes); // UPPERCASE

The per-test cap check in IngestExternalSpan therefore never matches for OTLP-received spans:

if (span.ParentSpanId is { } parentSpanId && _testCaseSpanIds.ContainsKey(parentSpanId))
//                                                              ^^ dict keyed lowercase
//                                       parentSpanId is UPPERCASE  ^^

All OTLP-external spans silently fall through to the per-trace cap path. The behaviour is still bounded, but the per-test capping granularity is wrong and any future code that relies on this path will be confused.

Note that _knownTraceIds already uses StringComparer.OrdinalIgnoreCase to handle exactly this kind of mismatch — the same fix should apply to _testCaseSpanIds and _externalSpanCountsByTest. Alternatively, normalize Convert.ToHexString calls in OtlpTraceParser to lowercase to stay consistent with how Activity returns IDs.

---

Architectural concern: layer violation via InternalsVisibleTo

OtlpReceiver now references TUnit.Engine internal types (ActivityCollector, SpanData, ReportKeyValue, SpanEvent, SpanLink) directly. This means:

• Any change to those types breaks TUnit.OpenTelemetry
• TUnit.OpenTelemetry can never be shipped independently of TUnit.Engine's internal API stability
• The ToSpanData mapper in OtlpReceiver.cs is a 60-line method that knows the HTML reporter's internal shape

A better approach would be a thin IExternalSpanSink interface in TUnit.Engine (or TUnit.Core) that ActivityCollector implements:

public interface IExternalSpanSink
{
    void RegisterExternalTrace(string traceId);
    void IngestExternalSpan(string traceId, string spanId, string? parentSpanId, string name, /* ... */);
}

Then OtlpReceiver takes an IExternalSpanSink? (defaulting to ActivityCollector.Current) and the ToSpanData mapping stays in TUnit.Engine where it belongs. This doesn't have to block this PR but is worth noting before more code builds on the current coupling.

---

Silent exception swallowing in ProcessTraces

try { spans = OtlpTraceParser.Parse(body); }
catch { return; }  // no diagnostic output

Compare with the existing ForwardAsync pattern which emits Trace.WriteLine. A malformed payload would be silently dropped with no way to diagnose it. Even a low-cost Trace.WriteLine($"[TUnit.OpenTelemetry] Failed to parse /v1/traces body: {ex.GetType().Name}") would make debugging far easier.

---

Minor: [EditorBrowsable] on an internal member

[EditorBrowsable(EditorBrowsableState.Never)]
internal static bool HasReceiverForTesting { get; }

internal already prevents external consumers from seeing this. [EditorBrowsable] only suppresses IDE completion for public members — on internal it is a no-op. This can just be internal.

---

Summary

Bug	Span ID case mismatch makes per-test cap path dead code for OTLP spans — needs fix
Architecture	InternalsVisibleTo coupling is workable but worth an interface before it grows
Minor	Silent parse failure in ProcessTraces; redundant [EditorBrowsable]

The core feature is sound and the test suite covers the important paths. The case-mismatch bug is the only thing that needs addressing before merge.

[thomhurst]

Thanks — both bugs are real. Fixed in d76bfee.

Case-sensitivity — You're right that Convert.ToHexString returns uppercase and Activity.TraceId.ToString() returns lowercase. The root fix is in the parser: it now emits lowercase (via a small HexLower helper that uses Convert.ToHexStringLower on net9+ and falls back to Convert.ToHexString(…).ToLowerInvariant() on net8). That way external spans naturally share dictionary keys with in-process spans without caller ceremony.

As defense-in-depth I also flipped the remaining trace/span-ID dicts in ActivityCollector (_spansByTrace, _externalSpanCountsByTest/ByTrace, _testCaseSpanIds) to OrdinalIgnoreCase — _knownTraceIds was already that way. Added a regression test IngestExternalSpan_TraceIdCaseMismatch_StillCorrelates (register lowercase, ingest uppercase, verify correlation). Docs and existing tests no longer ask callers to .ToUpperInvariant().

SpanType = span.Name — You're right, it's redundant. SpanType is the TUnit-only classifier (test_case, test_suite, test_assembly) used by the HTML template to apply category styling to first-party spans. No OTLP analogue exists, so external spans now set SpanType = null.

[claude]

Code Review — feat: generalize OTLP receiver for use outside TUnit.Aspire (final pass)

All three bugs from previous rounds have been cleanly resolved in d76bfee. The PR is in good shape. A couple of small stragglers worth a quick fix before merge.

---

Previous issues — resolved ✅

• Case-sensitivity mismatch — Fixed correctly. helper produces lowercase to match serialization; all dicts in ActivityCollector flipped to OrdinalIgnoreCase as defense-in-depth. The regression test IngestExternalSpan_TraceIdCaseMismatch_StillCorrelates covers exactly the right scenario.
• SpanType = span.Name — Fixed. Now SpanType = null for external spans, which is correct since the OTLP spec has no equivalent concept.
• Configurable span cap — TUNIT_OTEL_MAX_EXTERNAL_SPANS + EnvironmentConstants central home + Interlocked.CompareExchange one-shot warning latch — all good.
• Architecture (IVT + singleton) — Author's rationale stands for sibling assemblies shipping together. CompareExchange-based first-wins semantics are already in place.

---

Remaining: silent exception swallowing in ProcessTraces

This was raised in the third review and wasn't addressed:

try { spans = OtlpTraceParser.Parse(body); }
catch { return; }

Every other error path in OtlpReceiver emits a Trace.WriteLine diagnostic. A malformed payload silently disappears here with no trace. One line is all that's needed:

catch (Exception ex)
{
    Trace.WriteLine($"[TUnit.OpenTelemetry] Failed to parse /v1/traces body: {ex.GetType().Name}: {ex.Message}");
    return;
}

---

Remaining: [EditorBrowsable] on an internal member

[EditorBrowsable(EditorBrowsableState.Never)]
internal static bool HasReceiverForTesting { get; }

[EditorBrowsable] only suppresses IDE completion for public members. On internal it is a complete no-op — the IDE already hides internal from external projects by default. The attribute can just be removed.

---

Minor: cap test hardcodes default

// ActivityCollectorIngestionTests.cs
await Assert.That(ours.Count).IsEqualTo(100); // hardcoded default

If TUNIT_OTEL_MAX_EXTERNAL_SPANS is set in the CI environment this test will fail with a confusing mismatch. Reading from DefaultMaxExternalSpans (or making the const accessible to tests) would make the test environment-independent.

---

Summary

Bug	None remaining
Two-line fixes	Silent catch in ProcessTraces; remove no-op [EditorBrowsable]
Nice-to-have	Cap test against constant rather than literal 100

Good feature — solid approach, thorough test coverage, and the case-fix correctly roots the problem in the parser rather than patching callers. The two remaining items are small enough they could go in a follow-up, but the catch {} in particular takes 30 seconds to fix and pays off in the first time someone hits a corrupt payload in CI.

[claude]

Code Review — feat: generalize OTLP receiver for use outside TUnit.Aspire (final pass)

All three bugs from previous rounds have been cleanly resolved in d76bfee. The PR is in good shape. A couple of small stragglers worth a quick fix before merge.

---

Previous issues — resolved

• Case-sensitivity mismatch — Fixed correctly. HexLower helper produces lowercase to match Activity serialization; all dicts in ActivityCollector flipped to OrdinalIgnoreCase as defense-in-depth. The regression test IngestExternalSpan_TraceIdCaseMismatch_StillCorrelates covers exactly the right scenario.
• SpanType = span.Name — Fixed. Now SpanType = null for external spans, which is correct since the OTLP spec has no equivalent concept.
• Configurable span cap — TUNIT_OTEL_MAX_EXTERNAL_SPANS + EnvironmentConstants central home + Interlocked.CompareExchange one-shot warning latch — all good.
• Architecture (IVT + singleton) — Author's rationale stands for sibling assemblies shipping together. CompareExchange-based first-wins semantics are already in place.

---

Remaining: silent exception swallowing in ProcessTraces

This was raised in the third review and wasn't addressed:

try { spans = OtlpTraceParser.Parse(body); }
catch { return; }

Every other error path in OtlpReceiver emits a Trace.WriteLine diagnostic. A malformed payload silently disappears here with no trace. One line is all that's needed:

catch (Exception ex)
{
    Trace.WriteLine($"[TUnit.OpenTelemetry] Failed to parse /v1/traces body: {ex.GetType().Name}: {ex.Message}");
    return;
}

---

Remaining: [EditorBrowsable] on an internal member

[EditorBrowsable(EditorBrowsableState.Never)]
internal static bool HasReceiverForTesting { get; }

[EditorBrowsable] only suppresses IDE completion for public members. On internal it is a complete no-op — the IDE already hides internal from external projects by default. The attribute can just be removed.

---

Minor: cap test hardcodes default

// ActivityCollectorIngestionTests.cs
await Assert.That(ours.Count).IsEqualTo(100); // hardcoded default

If TUNIT_OTEL_MAX_EXTERNAL_SPANS is set in the CI environment this test will fail with a confusing mismatch. Reading from DefaultMaxExternalSpans (or making the const accessible to tests) would make the test environment-independent.

---

Summary

Bugs	None remaining
Two-line fixes	Silent catch in ProcessTraces; remove no-op [EditorBrowsable]
Nice-to-have	Cap test against constant rather than literal 100

Good feature — solid approach, thorough test coverage, and the case-fix correctly roots the problem in the parser rather than patching callers. The two remaining items are small enough they could go in a follow-up, but the catch {} in particular takes 30 seconds to fix and pays off the first time someone hits a corrupt payload in CI.

[thomhurst]

All three addressed in cd5c6b0.

• Silent catch — ProcessTraces and ProcessLogs now emit Trace.WriteLine with exception type + message, matching the convention used elsewhere in OtlpReceiver.
• [EditorBrowsable] on internal — dropped (and the now-unused System.ComponentModel using). You're right, it's a no-op.
• Hardcoded cap — ActivityCollector.MaxExternalSpans is now internal (IVT already covers TUnit.OpenTelemetry.Tests), and the test reads from it so CI-overridden caps don't produce a confusing mismatch.