Fix OpenTelemetry missing root span by reordering session activity lifecycle#5245
Fix OpenTelemetry missing root span by reordering session activity lifecycle#5245
Conversation
…fecycle The "test session" activity was created before [Before(TestSession)] hooks, but users set up TracerProvider (which adds the ActivityListener) in those hooks. The ActivitySource had no listeners so StartActivity returned null. Similarly, FinishSessionActivity ran after [After(TestSession)] hooks which dispose the TracerProvider, so the root span was never exported. Fix: create the session activity after before-hooks run, and stop it before after-hooks run. Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com> Agent-Logs-Url: https://github.com/thomhurst/TUnit/sessions/98881459-ab61-411c-8a31-0902db096e73
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: Fix OpenTelemetry missing root span
The fix correctly identifies and addresses the root cause — was called before hooks registered the , and was called after hooks disposed the exporter. The reordering is the right approach.
Correctness
The core logic is sound. The two-part fix is well-reasoned:
- Start session activity after before-hooks → listener is registered by the time
StartActivityis called - Stop session activity before after-hooks → exporter is still alive when the span is exported
Concerns
1. After-hook exceptions are no longer reflected in span status (inherent trade-off)
The original code:
FinishSessionActivity(hasErrors: exceptions is { Count: > 0 });The new code:
var hasTestFailures = _contextProvider.TestSessionContext.AllTests
.Any(t => t.Result is { State: TestState.Failed or TestState.Timeout or TestState.Cancelled });
FinishSessionActivity(hasErrors: hasTestFailures);Since must be called before after-hooks run, you cannot check whether those hooks will fail — so the span will never reflect after-session hook failures. This is an unavoidable consequence of the ordering fix, but it's worth documenting as a known limitation (a comment in the code, or in release notes for anyone relying on this behavior).
2. Session-level hook spans are now orphaned from the session root span
In , the hook activity's parent is set via . In the new flow:
- Before-hooks: is when hooks run (session activity hasn't been created yet), so before-session hook spans become root spans with no parent.
- After-hooks: is when hooks run (it's set to in ), so after-session hook spans are also unparented.
This creates an inconsistency with assembly/class levels, where hooks run inside their span lifetime and are correctly parented. This is again an inherent trade-off — you can't parent hook spans to a span that doesn't exist yet — but worth acknowledging. Users using Grafana will see session-level hook spans as top-level spans rather than as children of the session root.
**3. Minor: is now scoped inside **
Previously was declared at the top of the method. In the new code it's only declared inside . This is fine since the non-NET paths don't reference it, but it's a subtle structural change.
What's Good
- The fix is minimal and targeted — it changes only the session-level ordering, not the assembly/class levels (where the problem doesn't exist because users don't set up the provider there)
- Using actual test result states (, , ) for is arguably more correct than the original (which only checked after-hook exceptions and defaulted to when there were no after-hooks even if tests failed)
- The comments in the code clearly explain the rationale for the ordering
Suggestion
Consider adding a note to or the call site that after-hook exceptions are intentionally excluded from span status due to the ordering constraint:
// Note: after-hook exceptions cannot be reflected here since FinishSessionActivity
// must be called before after-hooks run (to ensure the exporter is still alive).
// The status reflects only test results.
var hasTestFailures = _contextProvider.TestSessionContext.AllTests
.Any(t => t.Result is { State: TestState.Failed or TestState.Timeout or TestState.Cancelled });
FinishSessionActivity(hasErrors: hasTestFailures);Overall
The fix is correct and the trade-offs are inherent to the problem domain — you cannot simultaneously parent spans correctly AND create them after the listener is registered. The PR fixes the more impactful user-facing issue (missing root span) and the approach is sound. The orphaned hook spans and missing after-hook error status are acceptable limitations worth documenting.
There was a problem hiding this comment.
Code Review: Fix OpenTelemetry missing root span
The fix correctly identifies and addresses the root cause — StartActivity was called before [Before(TestSession)] hooks registered the TracerProvider, and FinishActivity was called after [After(TestSession)] hooks disposed the exporter. The reordering is the right approach.
Correctness
The core logic is sound. The two-part fix is well-reasoned:
- Start session activity after before-hooks → listener is registered by the time
StartActivityis called - Stop session activity before after-hooks → exporter is still alive when the span is exported
Concerns
1. After-hook exceptions are no longer reflected in span status (inherent trade-off)
The original code checked exceptions is { Count: > 0 } for hasErrors. The new code queries actual test results instead. Since FinishSessionActivity must be called before after-hooks run, you cannot check whether those hooks will fail — so the span will never reflect after-session hook failures. This is an unavoidable consequence of the ordering fix, but worth documenting as a known limitation.
2. Session-level hook spans are now orphaned from the session root span
In ExecuteHookWithActivityAsync, the hook activity's parent is set via context.Activity?.Context ?? default. In the new flow:
- Before-hooks:
context.Activityisnullwhen hooks run (session activity hasn't been created yet), so before-session hook spans become orphaned root spans. - After-hooks:
context.Activityisnullwhen hooks run (set tonullinFinishSessionActivity), so after-session hook spans are also unparented.
This creates an inconsistency with assembly/class levels, where hooks run inside their span lifetime and are correctly parented. Users using Grafana will see session-level hook spans as top-level spans rather than children of the session root. This is inherent to the trade-off, but worth acknowledging.
3. Minor: sessionContext is now scoped inside #if NET
Previously declared at the top of the method, it's now only inside #if NET. This is fine since non-NET paths don't reference it, but it's a subtle structural change.
What's Good
- The fix is minimal and targeted — it changes only the session-level ordering, not assembly/class levels
- Using actual test result states (
Failed,Timeout,Cancelled) forhasErrorsis arguably more correct than the original (which defaulted tofalsewhen there were no after-hooks, even if tests failed) - The comments clearly explain the rationale for the ordering
Suggestion
Consider noting that after-hook exceptions are intentionally excluded from span status due to the ordering constraint:
// Note: after-hook exceptions cannot be reflected here since FinishSessionActivity
// must be called before after-hooks run (to ensure the exporter is still alive).
// The status reflects only test results.
var hasTestFailures = _contextProvider.TestSessionContext.AllTests
.Any(t => t.Result is { State: TestState.Failed or TestState.Timeout or TestState.Cancelled });
FinishSessionActivity(hasErrors: hasTestFailures);Overall
The fix is correct and the trade-offs are inherent to the problem domain — you cannot simultaneously parent spans correctly AND create them after the listener is registered. The PR resolves the more impactful user-facing issue (missing root span in Grafana Traces Drilldown) and the approach is sound. The orphaned hook spans and missing after-hook error status are acceptable limitations that warrant a brief comment.
|
@copilot should we have another span that sits above the session? |
I don't think an additional span above the session is needed. The "test session" span already serves as a clean root - it's the top of the hierarchy that all assemblies/suites/cases nest under. The fundamental issue here is a chicken-and-egg problem: users set up their Adding another layer (e.g. "test run" → "test session") would just add nesting without solving an additional problem - the ordering fix already ensures the root span is properly created and exported. That said, if you'd like a different design (e.g. renaming the root or introducing a wrapper), happy to adjust. |
The "test session" root span was never created or exported because the activity lifecycle was ordered incorrectly relative to user hooks.
Problem:
StartActivity("test session")ran before[Before(TestSession)]hooks, but users set up theirTracerProvider/ActivityListenerin those hooks — so theActivitySourcehad no listeners and returnednull. Symmetrically,FinishSessionActivityran after[After(TestSession)]hooks, which dispose theTracerProvider— so the span could never be exported. This caused Grafana Traces Drilldown to show no root spans, with empty Trace Service and Trace Name.Fix in
HookExecutor.cs:ExecuteBeforeTestSessionHooksAsync: Start the session activity after hooks run, so the listener is attached firstExecuteAfterTestSessionHooksAsync: Stop the session activity before hooks run, so the span is exported while the exporter is still aliveFailed/Timeout/Cancelled) instead of only tracking after-hook exceptionsOriginal prompt
💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.