Preserve durable trace context in SQL history

[chandramouleswaran]

Summary

• preserve orchestration replay span identity when SQL round-trips DistributedTraceContext
• preserve sub-orchestration ClientSpanId so parent-side client spans can be recreated with the same span ID the child execution points to
• add regression tests for continuation stability and listener-observed parent/child hierarchy

Why this is needed

DurableTask.SqlServer flattens history-event trace data into the TraceContext SQL column instead of serializing the full data contract graph. Before this change, that projection only preserved W3C traceparent and optional tracestate.

That was not enough for the Activity-based tracing contract in DurableTask.Core:

• DistributedTraceContext.Id and DistributedTraceContext.SpanId are used to restore the same orchestration execution span across reloads/replays/continuations.
• SubOrchestrationInstanceCreatedEvent.ClientSpanId is used to recreate the parent-side client span for a sub-orchestration with the exact span ID that the child orchestration server span references as its parent.

When SQL dropped those values:

• later activity-completion and timer spans could parent themselves to orchestration span IDs that no longer matched the replayed orchestration span visible in the exported trace
• child sub-orchestration spans could carry a parentSpanId that the parent side never actually emitted, which shows up as a missing parent/orphaned span in trace viewers

What changed

• extend the SQL TraceContext encoding to preserve extra durable trace fields without a schema change
  • @tracestate=
  • @id=
  • @spanid=
  • @clientspanid=
• keep the legacy trace-context format readable for backward compatibility
• rehydrate ClientSpanId when materializing SubOrchestrationInstanceCreatedEvent
• add unit tests for:
  • extended DistributedTraceContext round-tripping
  • legacy compatibility
  • sub-orchestration ClientSpanId round-tripping
• add integration tests for:
  • stable orchestration span IDs across continuations
  • nested sub-orchestration hierarchies
  • repeated sibling sub-orchestrations
  • no emitted ParentSpanId pointing to a missing span in the captured Activity graph

Scope note

This change fixes newly persisted SQL histories. It does not retroactively backfill older in-flight histories that were already stored in the legacy trace-context format before the provider change.

Validation

dotnet test .\test\DurableTask.SqlServer.Tests\DurableTask.SqlServer.Tests.csproj --nologo -v minimal --filter "FullyQualifiedName~SqlUtilsTraceContextTests|FullyQualifiedName~TraceContextFlowCorrectly|FullyQualifiedName~TraceContextMaintainsStableOrchestrationSpanAcrossContinuations|FullyQualifiedName~ActivityListenerCapturesNestedSubOrchestrationHierarchyWithoutMissingParents|FullyQualifiedName~ActivityListenerCapturesRepeatedSubOrchestrationsWithoutMissingParents"

Sample screenshot for illustration of the problem - span's with incorrect parents ID.

[fanyirobin]

overall LGTM, the failed CI test might be flaky. I run into this failure often as well.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Preserves DurableTask trace/span identity when SQL persists history by extending the TraceContext encoding (without a schema change) and adding regression tests that validate stable span IDs and correct parent/child hierarchies across replays/continuations and sub-orchestrations.

Changes:

• Extend SQL TraceContext serialization/parsing to preserve DistributedTraceContext.Id / SpanId and sub-orchestration ClientSpanId.
• Rehydrate ClientSpanId when materializing SubOrchestrationInstanceCreatedEvent.
• Add unit + integration tests that lock down round-tripping and emitted Activity parent/child relationships.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File	Description
test/DurableTask.SqlServer.Tests/Unit/SqlUtilsTraceContextTests.cs	Adds unit coverage for extended/legacy trace-context round-tripping and ClientSpanId persistence.
test/DurableTask.SqlServer.Tests/Integration/Orchestrations.cs	Adds integration regressions that validate exported Activity graphs have stable orchestration span IDs and no missing parents.
src/DurableTask.SqlServer/SqlUtils.cs	Implements the extended trace-context encoding/decoding and sub-orchestration ClientSpanId hydration.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[cgillum]

Please fix the analyzer warnings in test/DurableTask.SqlServer.Tests/Integration/Orchestrations.cs.

[chandramouleswaran]

Thanks for the review @cgillum! Pushed a68f9d1 to address both pieces of feedback:

Analyzer warnings (xUnit2031) on Orchestrations.cs — switched the two Assert.Single(collection.Where(predicate)) calls to Assert.Single(collection, predicate) as the analyzer recommends. Orchestrations.cs is now warning-free.

Rolling-upgrade safety for the TraceContext column (this came up from @fanyirobin's review of the previous revision) — the prior wire format added an @tracestate= prefix on line 2, which would have caused older workers reading new-format rows to store the literal @tracestate=foo string as the tracestate value and propagate it through OpenTelemetry.

The new layout is fully backward-compatible in both directions without any version flag:

• Line 1: traceparent (unchanged)
• Line 2: tracestate, raw with no prefix — matches the legacy format. When tracestate is empty but Id/SpanId follow, line 2 is an empty placeholder so the newer @-prefixed lines never land in the tracestate slot.
• Line 3+: @id=, @spanid=, @clientspanid= — older readers stop at line 2 and ignore these.

Locked down with two new unit tests:

• GetTraceContext_NewWriterPlacesRawTraceStateOnLineTwo_ForOldReaderCompat
• GetTraceContext_NewWriterEmitsEmptyTraceStateLine_WhenOnlyIdAndSpanIdSet

All 6 SqlUtilsTraceContextTests pass locally.

[cgillum]

[Copilot] Rolling-upgrade tracestate corruption is not actually fixed

Thanks for the iteration on the wire format. Unfortunately I don't think the latest revision closes the rolling-upgrade hazard — the design assumes the pre-PR reader will preserve empty placeholder lines, but it won't, because that reader uses RemoveEmptyEntries:

// pre-PR reader
string[] parts = text.Split(new[] { '\n' }, count: 2, StringSplitOptions.RemoveEmptyEntries);

Example that still breaks

For an event with Id / SpanId set and TraceState = "vendor=value", the new writer emits:

00-<trace>-<span>-01\nvendor=value\n@id=...\n@spanid=...

A worker on the previous release reads this with Split('\n', 2, RemoveEmptyEntries) and gets:

• parts[0] = "00-<trace>-<span>-01" ✅
• parts[1] = "vendor=value\n@id=...\n@spanid=..." ❌

That entire multi-line string lands in TraceState, flows into Activity.TraceStateString, and gets propagated through OpenTelemetry — the same W3C-non-compliant value we were trying to avoid. The "no-tracestate but Id/SpanId set" case is even worse: RemoveEmptyEntries drops the empty line 2, so parts[1] becomes "@id=...\n@spanid=...".

For sub-orchestration payloads ("\n\n@clientspanid=..."), both leading empty lines get removed, leaving parts[0] = "@clientspanid=..." which an old reader would assign to TraceParent — a regression vs. behavior on main.

Why the new tests don't catch this

The two new "old-reader compat" tests simulate the legacy reader as payload.Split('\n'), but the actual pre-PR code uses Split('\n', 2, RemoveEmptyEntries). Those produce different results on these exact payloads:

// GetTraceContext_NewWriterPlacesRawTraceStateOnLineTwo_ForOldReaderCompat
string[] parts = payload.Split('\n');           // not what the old reader does
Assert.Equal("vendor=value", parts[1]);         // passes under plain split
Assert.DoesNotContain(parts[1], "@");           // would FAIL with the real legacy split

Same issue in GetTraceContext_NewWriterEmitsEmptyTraceStateLine_WhenOnlyIdAndSpanIdSet — the empty placeholder it asserts on simply doesn't survive RemoveEmptyEntries. The tests pass, but they're not exercising the legacy semantics they claim to.

A more faithful test would inline the literal pre-PR call:

string[] parts = payload.Split(new[] { '\n' }, 2, StringSplitOptions.RemoveEmptyEntries);

…and assert that parts[0] is a valid W3C traceparent and parts[1] (if present) is a valid W3C tracestate — no embedded newlines, no @-prefixed internals.

Suggestion

Newline-delimited extension fields can't be made backwards-safe by adding empty padding, because the existing reader collapses empties. A couple of approaches that would actually work without a version flag:

• Carry the new fields inside tracestate as a private vendor key (e.g. dtmssql=id:...;span:...;client:...). Old readers grab the whole tracestate string unchanged and forward it; W3C tracestate is explicitly designed to allow unknown vendor keys.
• Move the wire format off newlines entirely (single line, escape-aware key=value list, or length-prefixed). Old readers would see the entire blob as traceparent — still wrong, but easy to detect and fall back to null rather than emit malformed telemetry.

Happy to help iterate on either direction. The core blocker is just that any encoding which leaves @-prefixed text reachable by parts[0] or parts[1] of the old Split('\n', 2, RemoveEmptyEntries) will leak into emitted spans during a rolling upgrade.

[chandramouleswaran]

@cgillum You were 100% right — I verified by reading the pre-PR reader directly. It uses Split('\n', count: 2, StringSplitOptions.RemoveEmptyEntries), and the previous attempt completely fell apart under those semantics:

• The empty placeholder line on row 2 was stripped by RemoveEmptyEntries.
• count: 2 left the entire remainder of the string in parts[1], so @id=...\n@spanid=... was landing wholesale in TraceState.
• The previous "old reader compat" tests used a plain Split('\n') and therefore failed to exercise the actual legacy semantics — your call-out on that was exactly correct.

I empirically confirmed the failure mode with a small console repro, then pushed a2f69da taking your first suggestion: carry the extended fields inside the W3C tracestate as a private vendor key named durabletask-mssql.

traceparent\ndurabletask-mssql=id:...;span:...[;client:...][,user-tracestate]

Why this is rolling-upgrade safe (and now actually proven, not asserted):

Writer	Reader	Outcome
New	Old	parts[0] is a clean traceparent, parts[1] is a clean single-line tracestate that includes our vendor key. W3C explicitly requires unknown vendor keys to be propagated, so it flows through Activity.TraceStateString unchanged. No newlines, no @ garbage.
Old	New	No durabletask-mssql= entry to extract, tracestate is left alone, Id/SpanId come back null — identical behavior to current main.
New	New	Vendor key parsed, user tracestate preserved in original order, fields round-trip exactly.
Old	Old	Unchanged (rows with no Id/SpanId are still emitted byte-for-byte identical to pre-PR).

The new tests simulate the real legacy split (Split('\n', 2, RemoveEmptyEntries)) and assert (1) parts[0] has no embedded newlines and no vendor noise and (2) parts[1] is a clean single-line tracestate. Theory cases also cover the vendor key appearing in any position within the user tracestate.

Sub-orchestration rows had TraceContext = NULL on main and the legacy reader's switch statement never invokes GetTraceContext for them, so the wire format there is a single-line durabletask-mssql=client:<spanId> that is only ever parsed by the new GetSubOrchestrationClientSpanId helper. Confirmed by reading the pre-PR GetHistoryEvent switch in upstream main.

11/11 unit tests pass locally. Thanks for the catch.