Skip to content

feat: distributed trace collection for HTML report#5059

Merged
thomhurst merged 16 commits intomainfrom
feat/distributed-trace-collection
Mar 2, 2026
Merged

feat: distributed trace collection for HTML report#5059
thomhurst merged 16 commits intomainfrom
feat/distributed-trace-collection

Conversation

@thomhurst
Copy link
Owner

Summary

  • Expands ActivityCollector to capture traces from all instrumented .NET libraries (HttpClient, ASP.NET Core, EF Core, etc.) — not just TUnit's own spans
  • Adds TestContext.Activity (public) and TestContext.RegisterTrace() API for explicit external trace correlation
  • Renders linked external traces in the HTML report timeline

How it works

Activities from instrumented libraries (e.g. System.Net.Http, Microsoft.AspNetCore) automatically become child spans when they execute under a test's activity context (via Activity.Current AsyncLocal propagation). The smart sampling approach returns AllDataAndRecorded for known traces and PropagationData for everything else — enabling context propagation with near-zero overhead for unrelated activities.

For external services that don't share the test's trace context, users can explicitly link traces via:

TestContext.Current!.RegisterTrace(externalActivity.Context.TraceId);

Changes

File Change
TUnit.Core/TraceRegistry.cs New — static bidirectional traceId↔testNodeUid registry
TUnit.Core/TestContext.cs Public Activity property + RegisterTrace method (#if NET)
TUnit.Engine/.../ActivityCollector.cs Listen to all sources, smart sampling, gated collection
TUnit.Engine/.../HtmlReportDataModel.cs AdditionalTraceIds on ReportTestResult
TUnit.Engine/.../HtmlReporter.cs Populate AdditionalTraceIds from TraceRegistry
TUnit.Engine/.../HtmlReportGenerator.cs renderExternalTrace JS + renderDetail update

Test plan

  • dotnet build TUnit.Core and TUnit.Engine — 0 errors, 0 warnings
  • Run test project with HttpClient calls, verify HTTP spans appear in test trace timeline
  • Verify TestContext.RegisterTrace links external traces as "Linked Trace" sections
  • Run existing snapshot/unit tests — no regressions
  • Verify existing TUnit-only trace timelines render unchanged

…report

Expand ActivityCollector to listen to all ActivitySources (not just TUnit),
using smart sampling to capture child spans from HttpClient, ASP.NET Core,
EF Core, etc. that execute under a test's trace context. External traces
can also be linked via the new TestContext.RegisterTrace API.

- Add TraceRegistry for cross-project traceId↔testNodeUid correlation
- Add TestContext.Activity (public) and TestContext.RegisterTrace API
- Smart sampling: AllDataAndRecorded for known traces, PropagationData
  for unknown (enables context propagation with near-zero overhead)
- Gate OnActivityStopped to only collect spans for known traces
- Add AdditionalTraceIds to ReportTestResult for linked external traces
- Add renderExternalTrace JS for "Linked Trace" sections in HTML report
Document how the HTML report automatically captures spans from
instrumented libraries (HttpClient, ASP.NET Core, EF Core), how to
link external traces via TestContext.RegisterTrace, and how to access
the test Activity for manual context propagation.

Also adds a cross-reference from the OpenTelemetry integration page.
Copy link
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Code Review — PR #5059: Distributed Trace Collection for HTML Report

Overall this is a well-designed feature with a smart sampling approach. The AsyncLocal propagation for automatic HttpClient/ASP.NET child span capture is elegant. A few architectural concerns worth addressing before merging.


1. 🔴 Root TUnit Activity Trace ID Is Never Registered in _knownTraceIds

In SampleActivity, when a TUnit source activity is sampled, you register options.Parent.TraceId:

if (IsTUnitSource(sourceName))
{
    if (options.Parent.TraceId != default)          // ← only the parent's ID
    {
        _knownTraceIds.TryAdd(options.Parent.TraceId.ToString(), 0);
    }
    return ActivitySamplingResult.AllDataAndRecorded;
}

For the root TUnit activity (e.g., test assembly), options.Parent.TraceId == default, so nothing is added to _knownTraceIds. The trace ID (T1) only enters the cache when the first TUnit child activity (test suite / test case) is sampled, which registers T1 as its parent.

This creates a correctness gap: if any instrumented code fires an activity between the root TUnit activity start and its first child activity start, that activity won't be captured (it will check _knownTraceIds, find T1 missing, and return PropagationData).

Better fix — register in OnActivityStopped for TUnit sources:

private void OnActivityStopped(Activity activity)
{
    var traceId = activity.TraceId.ToString();

    // TUnit activities always register their trace, including root activities
    if (IsTUnitSource(activity.Source.Name))
    {
        _knownTraceIds.TryAdd(traceId, 0);
    }
    else if (!_knownTraceIds.ContainsKey(traceId))
    {
        return;
    }
    // ... rest unchanged
}

This is safe because parents are always stopped after their children, so adding in OnActivityStopped ensures the cache is populated before any child span is collected.


2. 🟡 Static TraceRegistry Has No Cleanup — Memory Grows Indefinitely

TraceRegistry holds two static ConcurrentDictionary instances that are never cleared. In a long test run with thousands of tests and registered traces, this leaks. The same issue applies to ActivityCollector._knownTraceIds (though at least ActivityCollector is IDisposable).

The registry needs a cleanup path. Options:

  • Add a TraceRegistry.Clear() called at the end of the test run
  • Add individual Unregister(testNodeUid) called by the engine when a test completes
  • Make TraceRegistry a non-static service and register it with the DI container so it can be scoped/disposed

The static service approach is convenient but the lack of any disposal mechanism is a design smell.


3. 🟡 TraceRegistry Public API Design: EditorBrowsable(Never) is a Leaky Pattern

public static class TraceRegistry
{
    [EditorBrowsable(EditorBrowsableState.Never)]
    public static bool IsRegistered(string traceId) { ... }

    [EditorBrowsable(EditorBrowsableState.Never)]
    public static string[] GetTraceIds(string testNodeUid) { ... }
}

Making methods public but [EditorBrowsable(Never)] is a workaround to allow cross-assembly access without exposing them in IntelliSense. This is a red flag — it exposes an implementation detail as public API that can never be safely changed.

Since TUnit.Core and TUnit.Engine are both controlled by this project, InternalsVisibleTo would be a cleaner solution:

// In TUnit.Core/AssemblyInfo.cs:
[assembly: InternalsVisibleTo("TUnit.Engine")]

// Then TraceRegistry becomes:
internal static class TraceRegistry { ... }

Alternatively, the comment says "cross-project communication" — if a proper abstraction via an interface was used (e.g., ITraceRegistry registered in the engine's service provider), the static dependency could be eliminated.


4. 🟡 SampleActivityUsingParentId Silently Drops W3C Context for External Children

private ActivitySamplingResult SampleActivityUsingParentId(ref ActivityCreationOptions<string> options)
{
    // For string-based parent IDs we can't easily extract the trace ID
    return ActivitySamplingResult.PropagationData;
}

W3C trace context format is 00-{traceId}-{spanId}-{flags} — the trace ID is the 32-char hex segment. It's extractable:

private ActivitySamplingResult SampleActivityUsingParentId(ref ActivityCreationOptions<string> options)
{
    if (IsTUnitSource(options.Source.Name))
        return ActivitySamplingResult.AllDataAndRecorded;

    var parentId = options.ParentId;
    // W3C: "00-{32-hex-traceId}-{16-hex-spanId}-{2-hex-flags}"
    if (parentId is { Length: >= 35 } && parentId[2] == '-')
    {
        var traceIdStr = parentId.Substring(3, 32);
        if (_knownTraceIds.ContainsKey(traceIdStr) || TraceRegistry.IsRegistered(traceIdStr))
        {
            _knownTraceIds.TryAdd(traceIdStr, 0);
            return ActivitySamplingResult.AllDataAndRecorded;
        }
    }

    return ActivitySamplingResult.PropagationData;
}

This is worth considering since HTTP outbound calls from .NET 6+ use W3C format by default.


5. 🟢 ShouldListenTo = static _ => true — Performance Implication (Acceptable, But Worth Documenting)

Changing ShouldListenTo from filtering on TUnit sources to listening to all sources means the Sample callback fires for every activity in the process. For test-heavy apps using EF Core, gRPC, Polly, etc., this could be noisy.

The fast early-exit in SampleActivity (check parent trace == default) keeps the overhead minimal for unrelated traces. This is the right tradeoff for the feature. A comment noting the intentional performance tradeoff would help future maintainers not "optimise" this back to a filter.


6. 🟢 RegisterTrace Should Clarify That TestDetails.TestId == Test Node UID

In TestContext.RegisterTrace:

TraceRegistry.Register(traceId.ToString(), TestDetails.TestId);

I confirmed that TestDetails.TestId is indeed the same string used as TestNodeUid (via new TestNodeUid(testDetails.TestId) in TestExtensions.cs:209 and the tunit.test.node_uid tag in TestExecutor.cs:133). The correlation is correct. Worth a brief inline comment since the field is named TestId not TestNodeUid.


Summary

Severity Issue
🔴 Root TUnit activity trace ID not added to _knownTraceIds — gap in external span capture
🟡 TraceRegistry static state never cleared — memory growth
🟡 public + [EditorBrowsable(Never)] — use InternalsVisibleTo instead
🟡 SampleActivityUsingParentId misses W3C-formatted parent IDs
🟢 ShouldListenTo = true perf tradeoff should be documented
🟢 TestDetails.TestId == TestNodeUid — worth a comment for clarity

The architecture is sound and the feature fills a real gap in the HTML reporter. The main ask is fixing the root activity registration gap (#1) — everything else is quality improvements.

Copy link
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Code Review: feat/distributed-trace-collection

This is a well-designed feature with smart sampling logic and thorough documentation. The overall architecture is sound. Below are some concerns worth considering before merging.


🔴 Potential Key Mismatch — TestDetails.TestId vs TestNode.Uid.Value

In TestContext.RegisterTrace, the registry key is TestDetails.TestId:

TraceRegistry.Register(traceId.ToString(), TestDetails.TestId);

But in HtmlReporter.BuildReportData(), the lookup uses kvp.Key which comes from testNodeUpdateMessage.TestNode.Uid.Value:

var additionalTraceIds = TraceRegistry.GetTraceIds(kvp.Key); // kvp.Key = TestNode.Uid.Value

If TestDetails.TestId and TestNode.Uid.Value are not the same string, registered traces will silently not appear in the report — no error, just missing data. This should be verified explicitly, or an assertion/test added to confirm the two IDs are equivalent for the same test. The existing ActivityCollector.GetTestSpanLookup() uses the tunit.test.node_uid activity tag as the key, which implies the test node UID is distinct from the generated test ID — worth double-checking.


🟡 Performance: ShouldListenTo = static _ => true Is Very Aggressive

The change from:

ShouldListenTo = static source => IsTUnitSource(source),

to:

ShouldListenTo = static _ => true,

…means TUnit now subscribes as a listener to every ActivitySource in the process. The SampleActivity callback will be invoked for every activity creation across the entire runtime — including ASP.NET Core internal diagnostics, SignalR, Kestrel, etc. In integration tests using WebApplicationFactory, this could mean thousands of extra SampleActivity calls per request.

The PropagationData return for non-TUnit activities has near-zero cost after the callback is called, but the callback invocation itself adds overhead at every callsite. A better approach would be to dynamically subscribe to sources on-demand using DiagnosticListener.AllListeners to detect new sources and selectively add them. Or at minimum, maintain a fast-path allow-list for common noise sources (Kestrel, SignalR) to return PropagationData early without checking _knownTraceIds.


🟡 TraceRegistry Static State Is Never Cleared

The two static dictionaries in TraceRegistry grow throughout the entire process lifetime with no cleanup path. For test suites with many retries or re-runs in the same process (e.g., watch mode), stale trace registrations accumulate. Consider adding an internal Clear() method called after report generation, or scoping the registry to a test session object rather than using static state.

Additionally, GetTraceIds calls .Keys.ToArray() which allocates a new array per test. With thousands of tests, this adds up. Using ArraySegment or checking for empty before allocating would be a minor improvement.


🟡 TraceRegistry Public API Leaks Internal Contracts

IsRegistered and GetTraceIds are marked public + [EditorBrowsable(EditorBrowsableState.Never)], but remain callable by users and are visible in documentation/source. This is a leaky abstraction — users could call them and depend on undocumented behavior.

Since TUnit.Core and TUnit.Engine are separate assemblies, internal isn't an option without InternalsVisibleTo. Better alternatives:

  1. Event-based: TUnit.Core raises an event when a trace is registered; TUnit.Engine subscribes
  2. Injected delegate: TUnit.Engine injects a callback into TestContext during setup
  3. InternalsVisibleTo("TUnit.Engine") on TUnit.Core to allow internal access

Option 1 or 2 removes the need for [EditorBrowsable(EditorBrowsableState.Never)] as a leaky API.


🟡 Root TUnit Activity Trace ID: Implicit Timing Assumption

In SampleActivity, for a root TUnit activity (no parent):

if (IsTUnitSource(sourceName))
{
    if (options.Parent.TraceId != default) // false for root activity
    {
        _knownTraceIds.TryAdd(options.Parent.TraceId.ToString(), 0);
    }
    return ActivitySamplingResult.AllDataAndRecorded;
}

The root TUnit activity's own trace ID is never added to _knownTraceIds here. It only gets registered when the first child TUnit activity is created (the root becomes options.Parent). This works given TUnit's nesting structure (assembly → suite → test case), but it's implicit and fragile. If the activity hierarchy ever has a root with no TUnit children (e.g., a degenerate case), OnActivityStopped would drop that span.

Consider registering the activity's own trace ID after it's created (e.g., in ActivityStarted) rather than relying on the parent-child bootstrapping.


🟢 Minor: SampleActivityUsingParentId Doesn't Parse W3C Trace Context

// For string-based parent IDs we can't easily extract the trace ID,
// so use PropagationData to allow context flow
return ActivitySamplingResult.PropagationData;

The W3C Traceparent format (00-{traceId}-{spanId}-{flags}) is parseable with a simple split. This means RegisterTrace won't work for activities propagated via W3C string headers. The comment acknowledges this, but it might surprise users who expect RegisterTrace to work for all external trace scenarios. Consider at minimum documenting this limitation explicitly in the RegisterTrace XML doc.


✅ What's Well Done

  • The _knownTraceIds fast-path cache is a good optimization to avoid repeated TraceRegistry lookups for activities in the same trace
  • Smart sampling (PropagationData vs AllDataAndRecorded) correctly balances capture completeness with performance
  • The renderExternalTrace JS function correctly escapes topSrc via esc() to prevent XSS
  • Documentation is clear and the "What Gets Captured" table is a great UX touch
  • The #if NET guards are applied consistently across all affected APIs
  • Existing caps (1,000 per trace, 50,000 total) correctly apply to external spans too

- Fix root TUnit activity trace ID not registered in _knownTraceIds:
  register in OnActivityStopped for TUnit sources (root activities have
  their TraceId assigned after sampling, not available in SampleActivity)
- Make TraceRegistry internal (InternalsVisibleTo already covers TUnit.Engine)
- Add TraceRegistry.Clear() called at end of test run to release memory
- Parse W3C trace ID from string-based parent IDs in SampleActivityUsingParentId
- Add comment documenting ShouldListenTo perf tradeoff
- Add comment clarifying TestDetails.TestId == TestNodeUid correlation
Copy link
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Code Review: PR #5059 (Follow-up — Round 3)

All six issues raised in previous reviews have been addressed. However, the fix for issue #2 (add TraceRegistry.Clear()) introduced a new ordering bug that silently breaks the core RegisterTrace() feature.


✅ Previous Issues — All Addressed

# Issue Status
1 Root TUnit activity trace ID not added to _knownTraceIds ✅ Fixed via OnActivityStopped registration
2 TraceRegistry never cleared ⚠️ Fixed, but with an ordering bug (see below)
3 public + [EditorBrowsable(Never)] leaky API ✅ Fixed — class is now internal
4 SampleActivityUsingParentId missed W3C format ✅ Fixed — W3C parsing added
5 ShouldListenTo = true perf tradeoff undocumented ✅ Fixed — comment added
6 TestDetails.TestId == TestNodeUid not clarified ✅ Fixed — comment added

🔴 New Bug: TraceRegistry.Clear() Called Before BuildReportData()

In HtmlReporter.AfterRunAsync (lines 77–87):

#if NET
    _activityCollector?.Stop();
    TraceRegistry.Clear();          // ← Registry emptied here (line 79)
#endif

if (_updates.Count == 0) { return; }

var reportData = BuildReportData(); // ← BuildReportData reads the registry here (line 87)

Inside BuildReportData() (line 190):

var additionalTraceIds = TraceRegistry.GetTraceIds(kvp.Key); // Always returns [] — registry is already empty\!

The entire TestContext.RegisterTrace() feature is silently broken: calling it during a test will successfully register the trace, but Clear() runs before the HTML report reads those registrations — so additionalTraceIds will always be null and no "Linked Trace" sections will ever appear.

Fix: Move TraceRegistry.Clear() to after BuildReportData() completes:

_activityCollector?.Stop();

if (_updates.Count == 0)
{
#if NET
    TraceRegistry.Clear();
#endif
    return;
}

var reportData = BuildReportData(); // reads TraceRegistry
#if NET
TraceRegistry.Clear();              // cleared after data is captured
#endif
var html = HtmlReportGenerator.GenerateHtml(reportData);

This ensures cleanup still happens at end-of-run (addressing the memory concern from the earlier review) but after all registered trace IDs have been read into the report data.


🟢 Everything Else Looks Good

  • The OnActivityStopped approach for registering root TUnit trace IDs is correct and elegant — root activities (no parent context at sampling time) are correctly handled since they're always stopped after all their children
  • The _knownTraceIds fast-path cache correctly avoids repeated TraceRegistry lookups on the hot sampling path
  • The W3C parsing in SampleActivityUsingParentId is correct (parentId[2] == '-' and Substring(3, 32) correctly extracts the 32-char trace ID)
  • The renderExternalTrace JS properly uses esc(topSrc) to prevent XSS
  • TraceRegistry being internal with InternalsVisibleTo is the right architectural pattern for cross-assembly access
  • The documentation additions ("What Gets Captured" table, troubleshooting section) are genuinely useful

The ordering fix is a one-line move — once that's addressed, this feature is ready.

Copy link
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Code Review: PR #5059 (Follow-up — Round 3)

All six issues raised in previous reviews have been addressed. However, the fix for issue #2 (add TraceRegistry.Clear()) introduced a new ordering bug that silently breaks the core RegisterTrace() feature.


Previous Issues — All Addressed

# Issue Status
1 Root TUnit activity trace ID not added to _knownTraceIds Fixed via OnActivityStopped registration
2 TraceRegistry never cleared Fixed, but with an ordering bug (see below)
3 public + [EditorBrowsable(Never)] leaky API Fixed — class is now internal
4 SampleActivityUsingParentId missed W3C format Fixed — W3C parsing added
5 ShouldListenTo = true perf tradeoff undocumented Fixed — comment added
6 TestDetails.TestId == TestNodeUid not clarified Fixed — comment added

Bug: TraceRegistry.Clear() Called Before BuildReportData()

In HtmlReporter.AfterRunAsync (lines 77–87):

#if NET
    _activityCollector?.Stop();
    TraceRegistry.Clear();          // Registry emptied here (line 79)
#endif

if (_updates.Count == 0) { return; }

var reportData = BuildReportData(); // BuildReportData reads the registry here (line 87)

Inside BuildReportData() (line 190):

var additionalTraceIds = TraceRegistry.GetTraceIds(kvp.Key); // Always returns [] because registry is already empty

The entire TestContext.RegisterTrace() feature is silently broken: calling it during a test successfully registers the trace, but Clear() runs before the report reads those registrations — so additionalTraceIds will always be null and no "Linked Trace" sections will ever appear.

Fix: Move TraceRegistry.Clear() to after BuildReportData() completes:

_activityCollector?.Stop();

if (_updates.Count == 0)
{
#if NET
    TraceRegistry.Clear();
#endif
    return;
}

var reportData = BuildReportData(); // reads TraceRegistry
#if NET
TraceRegistry.Clear();              // cleared after data is captured
#endif
var html = HtmlReportGenerator.GenerateHtml(reportData);

This ensures cleanup still happens at end-of-run (addressing the memory concern from the earlier review) but only after all registered trace IDs have been read into the report data.


Everything Else Looks Good

  • The OnActivityStopped approach for registering root TUnit trace IDs is correct — root activities (no parent context at sampling time) are correctly handled since they're stopped after all their children
  • The _knownTraceIds fast-path cache correctly avoids repeated TraceRegistry lookups on the hot sampling path
  • The W3C parsing in SampleActivityUsingParentId is correct (parentId[2] == '-' and Substring(3, 32) correctly extracts the 32-char trace ID)
  • The renderExternalTrace JS properly uses esc(topSrc) to prevent XSS
  • TraceRegistry being internal is the right pattern now, with InternalsVisibleTo providing cross-assembly access
  • The documentation additions ("What Gets Captured" table, troubleshooting section) are genuinely useful

The ordering fix is a one-line move — once that's addressed, this feature is ready.

Clear() was called before BuildReportData(), so GetTraceIds() always
returned empty and RegisterTrace-linked traces never appeared in the
report. Now cleared after report data is captured.
Copy link
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Code Review: PR #5059 (Round 4 — Final Follow-up)

All issues raised in previous rounds have been addressed in the current commit (f341991). This is the final pass.


Previous Issues — All Resolved

# Issue Previous Status Current Status
1 Root TUnit activity trace ID not added to _knownTraceIds Raised Round 1/2 Correctly fixed via OnActivityStopped — TUnit activities register their trace ID on stop, which is always after all children stop
2 TraceRegistry never cleared Raised Round 1/2, ordering bug Round 3 Fixed — Clear() is now correctly placed after BuildReportData(), not before
3 public + [EditorBrowsable(Never)] leaky API Raised Round 1/2 Fixed — TraceRegistry is now internal
4 SampleActivityUsingParentId dropped W3C-formatted parent IDs Raised Round 1/2 Fixed — W3C parsing (parentId[2] == '-' + Substring(3, 32)) correctly extracts trace ID
5 ShouldListenTo = static _ => true perf tradeoff undocumented Raised Round 1/2 Fixed — comment explaining the intentional tradeoff is present
6 TestDetails.TestId == TestNodeUid correlation unclear Raised Round 1/2 Fixed — inline comment added

Ordering Bug (Round 3) — Confirmed Fixed

The previous commit had TraceRegistry.Clear() before BuildReportData(), silently emptying the registry before the report could read it. The current diff correctly places the two Clear() calls:

This is the correct ordering. RegisterTrace() will now work as documented.


No New Issues Found

The implementation looks clean:

  • Smart sampling logic is correct: TUnit sources get AllDataAndRecorded; activities with a known trace parent get recorded; everything else gets PropagationData
  • _knownTraceIds fast-path cache correctly avoids repeated TraceRegistry lookups on the hot sampling path
  • W3C parsing in SampleActivityUsingParentId is structurally correct for the 00-{32hex}-{16hex}-{2hex} format
  • XSS safetyrenderExternalTrace correctly uses esc(topSrc) before injecting into HTML
  • #if NET guards are applied consistently across all new APIs
  • Documentation — the "What Gets Captured" table, troubleshooting entries, and async context note are all accurate and genuinely useful

This PR is ready to merge.

The distributed trace collection feature is architecturally sound, the smart sampling approach is a good balance between observability and overhead, and all previously identified issues have been resolved correctly.

Copy link
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Code Review: PR #5059 (Round 4 — Final Follow-up)

All issues raised in previous rounds have been addressed in the current commit (f341991). This is the final pass.


Previous Issues — All Resolved

# Issue Status
1 Root TUnit activity trace ID not added to _knownTraceIds Fixed via OnActivityStopped — TUnit activities register their trace ID on stop, after all children
2 TraceRegistry never cleared — memory growth Fixed — Clear() now correctly placed after BuildReportData()
3 public + [EditorBrowsable(Never)] leaky API Fixed — TraceRegistry is now internal
4 SampleActivityUsingParentId dropped W3C-formatted parent IDs Fixed — W3C parsing added
5 ShouldListenTo = static _ => true perf tradeoff undocumented Fixed — comment explaining intentional tradeoff present
6 TestDetails.TestId == TestNodeUid correlation unclear Fixed — inline comment added

Ordering Bug (Round 3) — Confirmed Fixed

The previous commit had TraceRegistry.Clear() before BuildReportData(), silently emptying the registry before the report could read it. The current diff correctly structures the call order:

  1. If no updates: TraceRegistry.Clear() then return — safe, no report to generate
  2. Otherwise: BuildReportData() reads the registry, then TraceRegistry.Clear() runs after

RegisterTrace() now works as documented. Linked traces will correctly appear in the HTML report.


No New Issues Found

The implementation is clean:

  • Smart sampling logic is correct — TUnit sources get AllDataAndRecorded; activities with a known trace parent get recorded; everything else gets PropagationData (near-zero cost)
  • _knownTraceIds fast-path cache correctly avoids repeated TraceRegistry lookups on the hot sampling path
  • W3C parsing in SampleActivityUsingParentId correctly extracts the 32-char trace ID from 00-{traceId}-{spanId}-{flags} format
  • XSS safetyrenderExternalTrace correctly uses esc(topSrc) before injecting into HTML
  • #if NET guards applied consistently across all new APIs
  • Documentation — the "What Gets Captured" table, troubleshooting entries, and async context flow note are accurate and genuinely useful

This PR is ready to merge.

The distributed trace collection feature is architecturally sound, the smart sampling approach correctly balances observability with overhead, and all previously identified issues have been resolved correctly.

@thomhurst thomhurst enabled auto-merge (squash) March 2, 2026 20:14
@thomhurst thomhurst disabled auto-merge March 2, 2026 20:14
Copy link
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Code Review: PR #5059 (Round 5 — Post-Round-4 Snapshot Update)

This pass covers the single commit added after the Round 4 sign-off: e43a86af"test: update public API snapshots for Activity and RegisterTrace".


Status of All Prior Issues

All six issues from Rounds 1–3 and the ordering bug from Round 3 remain correctly resolved. No regressions introduced.

# Issue Status
1 Root TUnit trace ID not registered in _knownTraceIds ✅ Fixed via OnActivityStopped
2 TraceRegistry never cleared (memory growth) ✅ Fixed — Clear() after BuildReportData()
3 TraceRegistry was public ✅ Fixed — now internal
4 SampleActivityUsingParentId dropped W3C parent IDs ✅ Fixed — W3C parsing added
5 ShouldListenTo = true perf tradeoff undocumented ✅ Fixed — comment present
6 TestDetails.TestId == TestNodeUid correlation unclear ✅ Fixed — inline comment
Round 3 Clear() before BuildReportData() silently broke RegisterTrace ✅ Fixed — correct ordering

New Commit: Public API Snapshot Updates

The snapshot files (.DotNet8_0, .DotNet9_0, .DotNet10_0) correctly reflect the new surface:

public new .Activity? Activity { get; }
public void RegisterTrace(.ActivityTraceId traceId) { }

These appear in all three framework snapshots, which is consistent with the #if NET guard — net8.0, net9.0, and net10.0 all satisfy #if NET. The snapshots are accurate and complete.


Fresh Implementation Review

On a clean pass of the diff, the core implementation holds up well:

  • Smart sampling correctness: SampleActivity registers options.Parent.TraceId for TUnit sources. Since all spans in a W3C trace share the same trace ID, this is equivalent to registering the new activity's own trace ID — and it works because the actual TraceId isn't assigned until after sampling. Correct and elegant.
  • OnActivityStopped root registration: Handles the root TUnit activity case (no parent at sample time) by registering in the stop callback. This ensures HttpClient calls made as direct children of root TUnit activities are still captured via the child TUnit activity's SampleActivity call registering the shared trace ID.
  • W3C parsing in SampleActivityUsingParentId: Substring(3, 32) correctly extracts the 32-char trace ID from 00-{traceId}-{spanId}-{flags}. The format is stable and the guard (Length >= 35 && [2] == '-') is adequate.
  • XSS safety: renderExternalTrace uses esc(topSrc) ✓. The 'ext-' + tid prefix uses a hex trace ID ✓.
  • RegisterTrace for in-process external traces: The docs correctly describe this as linking traces from work that runs in the same process but under a different root trace context. The if (!sp || !sp.length) return '' guard in renderExternalTrace gracefully handles the case where a registered trace has no collected spans.

No new issues found.


This PR is ready to merge.

The distributed trace collection feature is complete, well-tested, and well-documented. The snapshot update commit is a routine and correct housekeeping step.

WebApplicationFactory.CreateClient() uses an in-memory TestServer
handler that bypasses .NET's built-in HTTP Activity instrumentation
(DiagnosticsHandler/SocketsHttpHandler). This means no HTTP spans
appear in the HTML report trace timeline.

Add ActivityPropagationHandler that creates Activity spans for HTTP
requests and injects W3C traceparent headers so server-side ASP.NET
Core spans are also correlated to the test's trace.

The new Client property on WebApplicationTest chains both
ActivityPropagationHandler and TUnitTestIdHandler, providing tracing
and test context propagation out of the box.
intellitect-bot pushed a commit to IntelliTect/EssentialCSharp.Web that referenced this pull request Mar 3, 2026
Updated [TUnit](https://github.com/thomhurst/TUnit) from 1.18.0 to
1.18.9.

<details>
<summary>Release notes</summary>

_Sourced from [TUnit's
releases](https://github.com/thomhurst/TUnit/releases)._

## 1.18.9

<!-- Release notes generated using configuration in .github/release.yml
at v1.18.9 -->

## What's Changed
### Other Changes
* perf: reduce source-gen JIT overhead via metadata factory helpers by
@​thomhurst in thomhurst/TUnit#5056
* perf: add ParameterMetadataFactory and lazy ReflectionInfo resolution
by @​thomhurst in thomhurst/TUnit#5057
* feat: distributed trace collection for HTML report by @​thomhurst in
thomhurst/TUnit#5059
### Dependencies
* chore(deps): update tunit to 1.18.0 by @​thomhurst in
thomhurst/TUnit#5052
* chore(deps): update docker/setup-docker-action action to v5 by
@​thomhurst in thomhurst/TUnit#5058


**Full Changelog**:
thomhurst/TUnit@v1.18.0...v1.18.9

Commits viewable in [compare
view](thomhurst/TUnit@v1.18.0...v1.18.9).
</details>

[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=TUnit&package-manager=nuget&previous-version=1.18.0&new-version=1.18.9)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)


</details>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.

1 participant