feat: TUnit.OpenTelemetry zero-config tracing package#5602
Conversation
…ase lock during Build
- TUnitTestCorrelationProcessor now references TUnit.Core.TUnitActivitySource.TagTestId
instead of duplicating the "tunit.test.id" literal.
- AutoStart extracts AutoStartEnvVar / OtlpEndpointEnvVar / ServiceNameEnvVar as
internal consts; tests reference them by name rather than raw strings.
- Environment.GetEnvironmentVariable("OTEL_EXPORTER_OTLP_ENDPOINT") is read once
and reused by both the gate check and AddOtlpExporter branch.
- TracerProvider.Build() and user Configure callbacks now execute outside the
AutoStart lock; a CAS-like re-check inside the lock disposes the loser if two
Start calls race.
There was a problem hiding this comment.
Code Review: feat: TUnit.OpenTelemetry zero-config tracing package
Overall this is a well-conceived feature. The architecture cleanly separates lifecycle hooks (AutoStart), user configuration (TUnitOpenTelemetry), and span correlation (TUnitTestCorrelationProcessor). The refactor of TUnit.Aspire to delegate to TUnitOpenTelemetry.Configure rather than owning its own singleton is the right call — single owner, multiple contributors.
Issue 1 (Architecture) — Hard-coded TUnit.AspNetCore.Http source in AutoStart
AutoStart.cs subscribes to TUnit.AspNetCore.Http by default:
.AddSource("TUnit")
.AddSource("TUnit.Lifecycle")
.AddSource("TUnit.AspNetCore.Http") // ← cross-package couplingThis couples TUnit.OpenTelemetry to TUnit.AspNetCore's internal source name. If more integration packages ship their own activity sources (database, message bus, etc.), AutoStart becomes a growing list of magic strings it shouldn't know about.
Better approach: a registration mechanism that lets each package self-describe:
// TUnit.OpenTelemetry public API
public static class TUnitOpenTelemetry
{
public static void RegisterActivitySource(string sourceName) { ... }
}
// In TUnit.AspNetCore (its own static init or [Before(TestDiscovery)] hook):
TUnitOpenTelemetry.RegisterActivitySource("TUnit.AspNetCore.Http");AutoStart then calls .AddSource() for every registered name. Each package owns its own registration rather than the core package accumulating knowledge of all integrations.
Issue 2 (Correctness) — TOCTOU gap between the listener check and the provider lock
In AutoStart.Start() the HasListeners() probe and the _provider lock are two separate critical sections with a gap between them:
// Check outside any lock — another thread could add a listener here
if (ProbeSource.HasListeners())
return;
// Separate lock — doesn't cover the check above
lock (_lock)
{
if (_provider is not null)
return;
}
// Build happens here, after both locks released
var provider = builder.Build();Two concurrent callers can both pass the HasListeners() guard. The second lock-then-set handles the double-build correctly (the loser disposes its provider), so data integrity is preserved. But the HasListeners() check is still semantically racy — a user listener attached between the check and Build() won't be detected.
The comment in the PR description ("builder.Build() and user callbacks execute outside the AutoStart lock to avoid holding a static lock across SDK initialization") is the right rationale for not locking around Build. An alternative that avoids the race without holding a long lock:
// Inside the lock, snapshot the decision
bool shouldBuild;
lock (_lock)
{
shouldBuild = _provider is null && !ProbeSource.HasListeners() && (force || ...);
}
if (!shouldBuild) return;
// Build outside lock (long operation)
var provider = builder.Build();
// Set under lock, dispose if we lost the race
lock (_lock)
{
if (_provider is not null) { provider.Dispose(); return; }
_provider = provider;
}The lock is still not held during Build, but the listener check is now inside the first lock, eliminating the TOCTOU gap.
Issue 3 (Minor) — Spurious null-conditional on a non-nullable local
After builder.Build() returns, the local is known non-null. The null-conditional on the disposal is misleading:
var provider = builder.Build(); // non-null
// ...
provider?.Dispose(); // the ?. implies it could be null — it can'tThis should just be provider.Dispose() to signal intent clearly. If Build() can throw mid-way and leave provider in an uncertain state, wrap it in a try/finally instead.
Issue 4 (Minor) — CreateTracerProvider stranded in production code for test use only
TestTraceExporter.CreateTracerProvider is documented as "used by test-only code paths" but lives in production code:
/// Used by test-only code paths that need an owned provider; production uses AddToBuilder.
internal static TracerProvider CreateTracerProvider(...)This leaks a test concern into the library. TUnit.Aspire.Tests should build the provider directly using Sdk.CreateTracerProviderBuilder() + AddToBuilder(...) rather than relying on a convenience wrapper in the library under test. That would allow the library to drop the method entirely.
What's working well
- Double-checked locking for the provider itself is correctly implemented — the race-to-build is detected and the loser disposes cleanly.
TUnitTestCorrelationProcessoris simple and correct. The early-return on existingtunit.test.idtag avoids redundant work.- The Aspire refactor (
RegisterAspireExporterdelegating viaConfigure) is architecturally sound — single provider owner, multiple contributors. - Test isolation via
[NotInParallel("TUnitOpenTelemetryGlobalState")]with properfinallycleanup is well-structured. - Environment variable design (
TUNIT_OTEL_AUTOSTART,OTEL_EXPORTER_OTLP_ENDPOINT,OTEL_SERVICE_NAME) follows OTel conventions and is clearly documented. - Public API snapshots committed across net8/net9/net10 and the pipeline module for CI are all present.
The hard-coded source name (Issue 1) is the most significant architectural concern as it sets a precedent that doesn't scale as the integration ecosystem grows. Issues 2–4 are lower priority but worth addressing before the package ships publicly.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Comprehensibility | 1 minor |
🟢 Metrics 32 complexity
Metric Results Complexity 32
TIP This summary will be updated as you push new changes. Give us feedback
…apper - AutoStart.Start moves the HasListeners/endpoint/HasConfiguration checks inside the existing _lock section so the listener probe and the provider assignment are no longer separated by a gap (PR #5602 issue 2). Build() still runs outside the lock; the post-Build re-check disposes the loser on a race. - provider.Dispose() is called unconditionally after Build — the local is known non-null, so the ?. was misleading (issue 3). - TestTraceExporter.CreateTracerProvider is removed — it only existed for one Aspire test that duplicated AddToBuilder_ExportsTracesForRegisteredSource anyway. Production code uses AddToBuilder (issue 4). Issue 1 (AutoStart hard-coding "TUnit.AspNetCore.Http") is acknowledged but not acted on: the plan explicitly requires TUnit.AspNetCore to stay OTel-agnostic, so the suggested self-registration pattern would create the very dep we want to avoid. Users who disagree can already use TUnitOpenTelemetry.Configure(b => b.AddSource(...)) to add/override sources.
|
Thanks for the review. Addressed issues 2–4 in 532a583; pushing back on issue 1 with rationale below. Issue 2 (TOCTOU) — moved the Issue 3 (null-conditional) — Issue 4 (test wrapper in production) — Issue 1 (hard-coded
The three default sources are an intentional "known TUnit sources" list, not an extensibility hook. Users who want to add or swap sources can already do so via Rerun results:
|
There was a problem hiding this comment.
Follow-up Review — addressing previous comments
The author pushed a follow-up commit (532a583a7529) that addressed three of the four issues from the previous review. Here's the updated status:
✅ Issue 2 (TOCTOU) — Addressed
HasListeners() is now checked inside the first lock (_lock) block in AutoStart.Start(), eliminating the race window between the listener probe and the provider guard. The double-build race (loser disposes its provider) is still correctly handled in the second lock. Well done.
✅ Issue 3 (Null-conditional) — Addressed
provider.Dispose() replaces provider?.Dispose() — the intent is now clear and the spurious nullable hint is gone.
✅ Issue 4 (Test-only wrapper) — Addressed
TestTraceExporter.CreateTracerProvider has been removed. TUnit.Aspire.Tests now builds the provider directly via Sdk.CreateTracerProviderBuilder() + AddToBuilder(...), keeping production code free of test-only conveniences. The AddToBuilder_ExportsTracesForRegisteredSource test is cleaner for it.
⚠️ Issue 1 (Hard-coded cross-package source) — Still outstanding
AutoStart.cs still hard-codes TUnit.AspNetCore.Http:
var builder = Sdk.CreateTracerProviderBuilder()
.AddSource("TUnit")
.AddSource("TUnit.Lifecycle")
.AddSource("TUnit.AspNetCore.Http") // ← coupling to a sibling package's internals
.AddProcessor(new TUnitTestCorrelationProcessor());This is the only remaining architectural concern from the first review. As the integration package ecosystem grows (ASP.NET Core, database, messaging, etc.), AutoStart becomes an ever-growing list of magic strings it shouldn't need to know about. A registration hook in TUnitOpenTelemetry would let each package self-describe:
// Public API addition to TUnit.OpenTelemetry
public static void RegisterActivitySource(string sourceName) { ... }
// In TUnit.AspNetCore (its own [Before(TestDiscovery)] or static initializer):
TUnitOpenTelemetry.RegisterActivitySource("TUnit.AspNetCore.Http");AutoStart then calls .AddSource() for every registered name. This is a judgment call for the author — shipping with the fixed list is acceptable for v1 if there's a plan to add self-registration later, but it's worth noting the pattern now before more sources get added.
Summary
Three of four issues addressed; the follow-up commit was exactly the right scope. The remaining item (Issue 1) is an architectural preference rather than a correctness bug. Code quality is good and the PR is mergeable as-is if the author accepts the coupling trade-off.
CI builds with ContinuousIntegrationBuild=true produce "/_/..." source paths, which the URL regex then collapsed to bare "/_/", diverging from the Windows dev output that scrubs to "PATH_SCRUBBED". Apply the path scrubbing before the URL scrubber so both build modes produce matching snapshots.
Updated [TUnit](https://github.com/thomhurst/TUnit) from 1.35.2 to 1.37.10. <details> <summary>Release notes</summary> _Sourced from [TUnit's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.37.10 <!-- Release notes generated using configuration in .github/release.yml at v1.37.10 --> ## What's Changed ### Other Changes * docs(test-filters): add migration callout for --filter → --treenode-filter by @johnkattenhorn in thomhurst/TUnit#5628 * fix: re-enable RPC tests and modernize harness (#5540) by @thomhurst in thomhurst/TUnit#5632 * fix(mocks): propagate [Obsolete] and null-forgiving raise dispatch (#5626) by @JohnVerheij in thomhurst/TUnit#5631 * ci: use setup-dotnet built-in NuGet cache by @thomhurst in thomhurst/TUnit#5635 * feat(playwright): propagate W3C trace context into browser contexts by @thomhurst in thomhurst/TUnit#5636 ### Dependencies * chore(deps): update tunit to 1.37.0 by @thomhurst in thomhurst/TUnit#5625 ## New Contributors * @johnkattenhorn made their first contribution in thomhurst/TUnit#5628 * @JohnVerheij made their first contribution in thomhurst/TUnit#5631 **Full Changelog**: thomhurst/TUnit@v1.37.0...v1.37.10 ## 1.37.0 <!-- Release notes generated using configuration in .github/release.yml at v1.37.0 --> ## What's Changed ### Other Changes * fix: stabilize flaky tests across analyzer, OTel, and engine suites by @thomhurst in thomhurst/TUnit#5609 * perf: engine hot-path allocation wins (#5528 B) by @thomhurst in thomhurst/TUnit#5610 * feat(analyzers): detect collection IsEqualTo reference equality (TUnitAssertions0016) by @thomhurst in thomhurst/TUnit#5615 * perf: consolidate test dedup + hook register guards (#5528 A) by @thomhurst in thomhurst/TUnit#5612 * perf: engine discovery/init path cleanup (#5528 C) by @thomhurst in thomhurst/TUnit#5611 * fix(assertions): render collection contents in IsEqualTo failure messages (#5613 B) by @thomhurst in thomhurst/TUnit#5619 * feat(analyzers): code-fix for TUnit0015 to insert CancellationToken (#5613 D) by @thomhurst in thomhurst/TUnit#5621 * fix(assertions): add Task reference forwarders on AsyncDelegateAssertion by @thomhurst in thomhurst/TUnit#5618 * test(asp-net): fix race in FactoryMethodOrderTests by @thomhurst in thomhurst/TUnit#5623 * feat(analyzers): code-fix for TUnit0049 to insert [MatrixDataSource] (#5613 C) by @thomhurst in thomhurst/TUnit#5620 * fix(pipeline): isolate AOT publish outputs to stop clobbering pack DLLs (#5622) by @thomhurst in thomhurst/TUnit#5624 ### Dependencies * chore(deps): update tunit to 1.36.0 by @thomhurst in thomhurst/TUnit#5608 * chore(deps): update modularpipelines to 3.2.8 by @thomhurst in thomhurst/TUnit#5614 **Full Changelog**: thomhurst/TUnit@v1.36.0...v1.37.0 ## 1.36.0 <!-- Release notes generated using configuration in .github/release.yml at v1.36.0 --> ## What's Changed ### Other Changes * fix: don't render test's own trace as Linked Trace in HTML report by @thomhurst in thomhurst/TUnit#5580 * fix(docs): benchmark index links 404 by @thomhurst in thomhurst/TUnit#5587 * docs: replace repeated benchmark link suffix with per-test descriptions by @thomhurst in thomhurst/TUnit#5588 * docs: clearer distributed tracing setup and troubleshooting by @thomhurst in thomhurst/TUnit#5597 * fix: auto-suppress ExecutionContext flow for hosted services (#5589) by @thomhurst in thomhurst/TUnit#5598 * feat: auto-align DistributedContextPropagator to W3C by @thomhurst in thomhurst/TUnit#5599 * feat: TUnit0064 analyzer + code fix for direct WebApplicationFactory inheritance by @thomhurst in thomhurst/TUnit#5601 * feat: auto-propagate test trace context through IHttpClientFactory by @thomhurst in thomhurst/TUnit#5603 * feat: TUnit.OpenTelemetry zero-config tracing package by @thomhurst in thomhurst/TUnit#5602 * fix: restore [Obsolete] members removed in v1.27 (#5539) by @thomhurst in thomhurst/TUnit#5605 * feat: generalize OTLP receiver for use outside TUnit.Aspire by @thomhurst in thomhurst/TUnit#5606 * feat: auto-configure OpenTelemetry in TestWebApplicationFactory SUT by @thomhurst in thomhurst/TUnit#5607 ### Dependencies * chore(deps): update tunit to 1.35.2 by @thomhurst in thomhurst/TUnit#5581 * chore(deps): update dependency typescript to ~6.0.3 by @thomhurst in thomhurst/TUnit#5582 * chore(deps): update dependency coverlet.collector to v10 by @thomhurst in thomhurst/TUnit#5600 **Full Changelog**: thomhurst/TUnit@v1.35.2...v1.36.0 Commits viewable in [compare view](thomhurst/TUnit@v1.35.2...v1.37.10). </details> [](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>
Summary
TUnit.OpenTelemetryNuGet package auto-wires aTracerProviderat[Before(TestDiscovery)]and disposes at[After(TestSession)]. Install the package + setOTEL_EXPORTER_OTLP_ENDPOINT— zero boilerplate required.TUnitTestCorrelationProcessor(resolves Ship opt-in OpenTelemetry ActivityProcessor that tags spans with tunit.test.id #5591's tag-propagation workaround) so spans from broken-parent-chain libraries still carrytunit.test.id.TUnit.Aspirerefactored to delegate provider construction viaTUnitOpenTelemetry.Configure, removing its ownTracerProvidersingleton.[Before(TestDiscovery, Order = int.MaxValue)]runs last, probesActivitySource("TUnit").HasListeners(), and steps aside if a listener is already attached.TUNIT_OTEL_AUTOSTART=0opts out;=1force-enables.Resolves #5593. Bundles fix for #5591.
Architecture notes
TUnitOpenTelemetry.Configure(Action<TracerProviderBuilder>),AutoStart(hooks +AutoStartOrderconst),TUnitTestCorrelationProcessor.TUNIT_OTEL_AUTOSTART != "1"andOTEL_EXPORTER_OTLP_ENDPOINTis unset and no userConfigurecallbacks are registered, the package stays dormant (no provider built, no listener attached).service.nameresource:OTEL_SERVICE_NAME> entry assembly name >"TUnit.Tests".builder.Build()and user callbacks execute outside theAutoStartlock to avoid holding a static lock across SDK initialization.dotnet add package.Docs
docs/docs/examples/opentelemetry.mdgains Option A: Zero-config; existing manual setup demoted to Option B / Option C.docs/docs/guides/distributed-tracing.mdgains a zero-config callout pointing to the new option.TUnitTagProcessorsnippet in troubleshooting now points at the pre-registered package processor.Test plan
dotnet build TUnit.slnx -c Release— 0 errorsTUnit.OpenTelemetry.Tests— 13/13 pass (Configure, AutoStart coexistence, opt-out env var, correlation processor baggage-to-tag, default service.name, end-to-end InMemory exporter smoke test)TUnit.Aspire.Tests— 101/101 pass (no regression from the refactor)TUnit.PublicAPI— new OpenTelemetry snapshot tests pass across net8/net9/net10TUnit.Engine.Testshook filter — 17 pass, 16 skip, 0 fail (no regression from the newOrder = int.MaxValuehook)dotnet pack TUnit.OpenTelemetryproduces a valid nupkgdocs && npm run build— clean build, no broken linksOTEL_EXPORTER_OTLP_ENDPOINT=http://localhost:4317Pipeline
TUnit.Pipeline/Modules/GetPackageProjectsModule.csadds the new package to the pack/publish list.RunOpenTelemetryTestsModulemirrors the Aspire test module (no OS gate — nothing platform-specific).TUnit.slnx,TUnit.Dev.slnx,TUnit.CI.slnx) updated.Plan
Full implementation plan:
docs/plans/2026-04-17-tunit-opentelemetry-package.md(14 tasks, each committed separately).