fix(traces): keep the inbound trace context through nested spans - #4843
Closed
turnipdabeets wants to merge 1 commit into
Closed
fix(traces): keep the inbound trace context through nested spans#4843turnipdabeets wants to merge 1 commit into
turnipdabeets wants to merge 1 commit into
Conversation
An inbound `traceparent` now parents a span that names no parent, on both the recording and inert paths, so a nested span no longer starts a new trace. Also accepts a one-element header array and trims an over-long `tracestate` instead of dropping it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BjT26chjUzuPSyJZqh4AWQ
turnipdabeets
force-pushed
the
fix/traces-context-followups
branch
from
September 8, 2026 14:18
18907eb to
5757a09
Compare
turnipdabeets
changed the base branch from
feat/traces-node-mvp
to
fix/traces-event-attribute-cap
September 8, 2026 14:18
Contributor
📝 No Changeset FoundThis PR doesn't include a changeset. A changeset is required to release a new version. How to add a changesetRun this command and follow the prompts: pnpm changesetRemember: Never use |
Contributor
|
Contributor
Contributor
|
Size Change: +2.92 kB (+0.01%) Total Size: 20.8 MB 📦 View Changed
ℹ️ View Unchanged
|
Contributor
Author
|
Folded into the PRs the changes belong to, so they release with tracing rather than after it — this was never really a follow-up.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Three trace-context gaps from @jonmcwest's second review pass on #4579. Each one loses inbound context that the SDK already has in hand.
These ship with the first traces release, not after it. The pass-through behaviour below is something #4579 introduced, so releasing without this means shipping a chain that severs one level in and then correcting it in a patch — on an API still marked
@experimental, where we can just get it right.A nested span drops the inbound trace (comment). #4579 made a service with tracing off forward the
traceparentit received, but only from the span that received it. A span started inside that one names no parent, and neither path looked for one:_resolveParentendsisOwnSpan(active) ? active.childContext() : undefined, so an active pass-through handle is not a parent.inertSpan(options)with noparent, which returns the sharedNOOP_SPAN.So
withSpan('outer', { parent: header }, () => withSpan('inner', (span) => span.traceparent()))returnsnull, and everything downstream of the inner span starts a fresh trace. This is the same severed chain #4579 set out to fix, one level in.A one-element header array is ignored (comment).
req.headersDistinct.traceparentis['00-...']for a single inbound header. That fell through to "Ignoring an unusable span parent" and started a new trace. The comment justifying it was also wrong:req.headersjoins a repeated header into one comma-separated string, so astring[]there means two inbound values, not one.An over-long
tracestateis dropped whole (comment). Past 512 characters or 32 members the entire header was discarded, losing valid vendor state. W3C makes those limits a reason to drop members from the end.Plus one diagnostic gap (comment): nothing told a user when span limits discarded their data.
Changes
inertSpantakes the active handle as the implicit parent, and_resolveParentadopts an activePassThroughSpanas a remote parent. Both halves matter: the first covers tracing being off, the second covers tracing being on when the span that received the header could not be recorded — its child is then the first recorded span of that trace and must join it.traceparentHeaderunwraps a one-element array on both the recording and inert paths. A longer array is two different inbound values and picking either would be a guess, so it is still ignored. The inaccurate comment aboutreq.headersis corrected.sanitizeTracestatetrims instead of discarding. Malformed input (a member with no=, CRLF, a lone surrogate) still rejects the whole header — that is validation, not size. Size now keeps the members that fit. A single member longer than the limit still yields nothing, because there is nothing to keep.beforeSpanSenddrops are included, and covering event-level counters as well as the span's own.The string-to-parent conversion is now one
remoteContexthelper shared by the explicit-header and active-handle paths, rather than two copies.Tests retimed
starts a fresh root when the parent is not a span, as a duplicated header isandparents to the active span when the parent is not a spanboth passed a one-element array to assert it was ignored. They now use two headers, which is the case that is genuinely unusable, and a new test covers the one-element case.Stacking: based on
fix/traces-event-attribute-cap(#4792), which is based onfeat/traces-node-mvp(#4579). Nothing here depends on #4726.Release info Sub-libraries affected
Libraries affected
@posthog/corecarries the change;posthog-nodebumps as a dependent. Both are already declared in the existingnode-distributed-tracing.mdchangeset on the base branch.Checklist
If releasing new changes
pnpm changesetto generate a changeset fileAmended the existing base-branch entry rather than adding one — traces has not shipped, so the feature and its follow-ups are one changelog entry. If tracing releases before this merges, this becomes its own
patchentry instead.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Claude Code (Opus 5), session.
Decisions along the way:
getActiveSpan()inside a no-opwithSpanstill reads the outer handle. The gap is in the handle given to the callback, so the test assertsspan.traceparent(). Each half is pinned by a test that fails when only that half is reverted.tracestate. Jon's ask was to separate size from validation, not to salvage bad syntax. A member with no=still rejects the header.Retry-Afterwindow, and separating a locally measured oversized batch from a 413, are in fix(traces): hold spans back during Retry-After and split locally measured batches #4847 — they sit on fix(core): honor Retry-After on the OTLP export queues #4726 rather than this branch. They release with tracing too.Verified locally:
packages/core1648 pass,packages/node995 pass,turbo buildclean for@posthog/core,@posthog/typesandposthog-node, oxlint and oxfmt clean. No manual/device testing.🤖 Generated with Claude Code
https://claude.ai/code/session_01BjT26chjUzuPSyJZqh4AWQ