feat(node): distributed tracing spans - #4579
Open
turnipdabeets wants to merge 7 commits into
Open
Conversation
Contributor
posthog-node Compliance ReportDate: 2026-09-01 18:36:13 UTC ✅ All Tests Passed!111/111 tests passed Capture_V1 Tests✅ 94/94 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
Contributor
23 tasks
Contributor
posthog-js Compliance ReportDate: 2026-09-01 18:43:32 UTC ✅ All Tests Passed!26/26 tests passed Capture Tests✅ 26/26 tests passed View Details
|
Contributor
|
Size Change: +88.6 kB (+0.43%) Total Size: 20.9 MB 📦 View Changed
ℹ️ View Unchanged
|
This was referenced Aug 20, 2026
turnipdabeets
force-pushed
the
feat/traces-node-mvp
branch
from
August 21, 2026 12:45
a9ff420 to
5c7301c
Compare
23 tasks
turnipdabeets
force-pushed
the
feat/traces-node-mvp
branch
from
August 24, 2026 11:25
e36495f to
9365a6c
Compare
23 tasks
Contributor
|
turnipdabeets
force-pushed
the
feat/traces-node-mvp
branch
4 times, most recently
from
August 28, 2026 20:49
bfb08e8 to
cc7b6d8
Compare
This was referenced Aug 28, 2026
turnipdabeets
force-pushed
the
feat/traces-node-mvp
branch
5 times, most recently
from
August 31, 2026 18:23
b946de6 to
ab4048c
Compare
23 tasks
turnipdabeets
force-pushed
the
feat/traces-node-mvp
branch
from
August 31, 2026 18:45
ab4048c to
caf83b0
Compare
turnipdabeets
changed the base branch from
main
to
refactor/shared-otlp-attribute-encoder
August 31, 2026 18:45
turnipdabeets
force-pushed
the
feat/traces-node-mvp
branch
from
August 31, 2026 19:39
caf83b0 to
075298b
Compare
Base automatically changed from
refactor/shared-otlp-attribute-encoder
to
main
August 31, 2026 20:04
turnipdabeets
force-pushed
the
feat/traces-node-mvp
branch
9 times, most recently
from
August 31, 2026 22:51
3554295 to
bc7a0ba
Compare
turnipdabeets
marked this pull request as ready for review
September 1, 2026 01:26
Contributor
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
turnipdabeets
force-pushed
the
feat/traces-node-mvp
branch
from
September 1, 2026 01:32
bc7a0ba to
e1458e9
Compare
Adds startSpan, withSpan and getActiveSpan to posthog-node behind a new traces client option, encoding spans as OTLP JSON without an OpenTelemetry dependency.
turnipdabeets
force-pushed
the
feat/traces-node-mvp
branch
from
September 1, 2026 01:40
e1458e9 to
e5e089e
Compare
Member
|
@PostHog/team-apm i think it makes sense for you all to give an initial look to make sure this works well within the product/ui/ingestion |
Adds maxLiveSpans and maxSpanAgeMs. Live accounting keeps a span id and a monotonic timestamp, never the span, so leaked handles stay collectable.
jonmcwest
reviewed
Sep 1, 2026
jonmcwest
left a comment
jonmcwest
left a comment
Contributor
There was a problem hiding this comment.
I could not review the full diff line by line, so I compared the design and the trace-context behavior against the OTel specification and OTel JS, and validated each comment against the code (at 5f7b2ff, re-checked at b07c9eb).
The implementation matches or exceeds the reference behavior on clocks, id generation, batching defaults, 413 handling, backpressure, and hostile input. No comment below blocks the MVP. Comments 1 and 2 are trace-context design questions, and comment 1 deserves an explicit decision before the API stabilizes. The rest are follow-ups and nits.
| updateName(): this { | ||
| return this | ||
| } | ||
| traceparent(): string | null { |
Contributor
There was a problem hiding this comment.
The SDK drops trace context when tracing is off. This severs distributed traces at services that do not have tracing enabled yet.
startSpan returns the shared NOOP_SPAN before it parses parent (packages/core/src/traces/index.ts:107, packages/node/src/client.ts:683 and :723). NoopSpan.traceparent() returns null. Thus a service in the middle of a traced chain, with traces unconfigured, forwards nothing. The propagation example in the withSpan docstring (getActiveSpan()?.traceparent()) sends no header there. Downstream services then start fresh traces.
This diverges from the OTel spec, which is normative here. With no SDK installed, "the API MUST return a non-recording Span with the SpanContext in the parent Context" (trace/api.md, "Behavior of the API in the absence of an installed SDK"). The propagated context then "will be propagated through to any child span and ultimately also Inject". The spec makes this the one exception to no-op behavior, exactly so that a pass-through service keeps the chain intact.
The NoopSpan comment gives this reason: null makes sure the SDK cannot propagate "an id that was never recorded". Pass-through does not do that. When the SDK forwards the inbound header unchanged, it propagates the span id of the upstream caller, and upstream recorded that id. The SDK invents no ids.
Suggestion: when parent is a valid traceparent string but tracing is off, return a per-call inert span. Make its traceparent() echo the inbound header, the tracestate, and the original flags. Activate it in withSpan so that getActiveSpan() works. The shared NOOP_SPAN can stay for the no-parent case.
If you keep the current behavior on purpose, add a test that passes parent on the disabled path, and add a docs note. Today no test covers this case.
This does not block an experimental MVP. A later move to pass-through is compatible with the documented traceparent ? ... : {} guard. Decide before the API becomes stable.
Contributor
Author
There was a problem hiding this comment.
Addressed in bf5e7af — inert spans now pass the inbound traceparent through, and withSpan activates them so getActiveSpan() works.
| * Builds the `traceparent` header value for a span. The sampled flag is always | ||
| * set, because a span we exported is by definition recorded. | ||
| */ | ||
| export function formatTraceparent(traceId: string, spanId: string): string { |
Contributor
There was a problem hiding this comment.
The SDK drops the inbound sampled flag and always sends 01 outbound.
The spec position first: this is legal. W3C Trace Context lists "Update sampled" as a permitted mutation when parent-id changes, and it changes here. The spec also says a definitive recording decision "SHOULD be reflected in the sampled flag". PostHog records everything, so 01 is truthful. OTel with a bare AlwaysOnSampler does the same thing.
The cost is interop with parent-based samplers from other vendors. Service A samples a trace out (00). A PostHog-traced service B continues it and propagates 01. A downstream OTel service C with the default ParentBased sampler then records a trace that its own head sampler rejected. Its backend gets paid-for fragments with no root.
Suggestion, as a follow-up and not a blocker: use the OTel RECORD_ONLY shape for this case. Store the inbound flag on RemoteSpanContext. Record and export the span as now, but propagate the inbound byte in traceparent() and set OTLP flags from it. That keeps the header and the wire consistent. First make sure that ingestion does not treat flags=0 spans differently.
At minimum, document the current behavior. It will surprise users who integrate with head-sampled OTel fleets.
Contributor
Author
| * Extracts the OTel `exception.type` / `exception.message` pair from whatever was | ||
| * thrown. Anything can be thrown in JS, so non-Errors are described by type. | ||
| */ | ||
| export function describeError(error: unknown): { type: string; message: string } { |
Contributor
There was a problem hiding this comment.
recordException and the withSpan error path record only exception.type and exception.message. Do you drop error.stack on purpose? OTel semconv marks exception.stacktrace as Recommended, and OTel JS captures it in recordException. People usually debug from the stack.
There is a case for a delay. No beforeSpanSend hook or attribute cap exists yet (both deferred to #4584), so users could not scrub or bound stacks. A deep async stack is a few KB per span. Also, if a later release adds stacks silently, it changes what leaves customer servers. Such a change must be visible either way.
Suggestion: if this is a deferral, say so in the recordException doc in packages/types/src/traces.ts and in the PR body. Land stack capture with #4584, so that the caps and the scrub hook ship first. If it is not deliberate, describeError can read error.stack behind the existing try/catch today.
Contributor
Author
| continue | ||
| } | ||
|
|
||
| if (outcome.kind === 'too-large') { |
Contributor
There was a problem hiding this comment.
A span with one multi-MB attribute takes an expensive path here. The 413 halving retries the same head batch. Thus the SDK uploads the oversized body up to 11 times before it isolates and drops the span. To drain a full 512-span queue around it takes 35 to 47 POSTs (simulated against this exact loop). The sticky shrink then leaves _maxExportBatchSize near 22, and the +1 ramp needs about 480 successful batches to recover.
The OTel default is no better on the value itself. AttributeValueLengthLimit defaults to Infinity, and the OTel exporter drops the whole batch on failure. This path drops only the poison span, which is the right call. But #4584 adds count caps (128 attributes, 128 events) and no value-length cap, so a single huge attribute still lands here after it merges.
Ask: add a client-side size check before the POST. _sendOtlpBatch already serializes the payload, and the client knows the ~2MB server cap (see the comment in traces-defaults.ts). Treat a single span whose serialized size exceeds the cap as too-large, without a send. That removes every redundant upload and most of the halving churn. It can ship as a small follow-up.
Contributor
Author
There was a problem hiding this comment.
Half addressed: f587e48 in #4584 adds maxAttributeValueLength (8192), which bounds the value that causes this. The pre-send size check still needs a number — the cap in the code comment says ~2 MB but this PR measured 5.75 MB accepted and 11.5 MB rejected, so I don't want to guess it. Tracking separately.
Contributor
Author
| } | ||
| } | ||
|
|
||
| function looksLikeSpan(value: unknown): boolean { |
Contributor
There was a problem hiding this comment.
Nit: an OTel span passed as parent has no traceparent method. It fails looksLikeSpan, and the SDK drops it silently with "Ignoring an unusable span parent". The new span then parents to the active PostHog span, or to a new root. That breaks the trace the caller expected. Inside withSpan this is wrong parentage, not just a fresh trace. A follow-up could duck-type spanContext() and adopt traceId/spanId after isValidTraceId/isValidSpanId. That fits the deferred soft-detect PR. The inline comment "Not a span at all" is also inaccurate for this case.
Contributor
Author
There was a problem hiding this comment.
Comment fixed in bf5e7af — it now says a span from another tracer exposes spanContext() instead. The duck-typing itself stays with the soft-detect PR.
| kind: spanKindToOtlp(record.kind), | ||
| startTimeUnixNano: msToUnixNanoString(record.startTime), | ||
| endTimeUnixNano: msToUnixNanoString(record.endTime), | ||
| flags: TRACE_FLAGS_SAMPLED, |
Contributor
There was a problem hiding this comment.
Optional nit: flags is always 1, so bits 8-9 (parent-remoteness, 0x100/0x200) stay unset and the row records "unknown". OTel SDKs set these bits, and this SDK knows the answer: a traceparent-string parent is remote, a handle parent is local. The server stores flags verbatim but reads nothing from it today, so this is optional. If you set the bits now, you avoid a gap that no backfill can repair, in case service-entry detection ever uses them.
Contributor
Author
| if (record.traceState) { | ||
| span.traceState = wireString(record.traceState) | ||
| } | ||
| const attributes = toOtlpKeyValueList(record.attributes, logger) |
Contributor
There was a problem hiding this comment.
Nit: an empty attribute key passes assignUserAttributes and encodeKeyValueList unexamined and ships as { "key": "" }. The OTel spec requires non-empty keys. The server stores the key verbatim, so it surfaces as a nameless attribute in filters. Skip empty keys with a debug log in encodeKeyValueList (packages/core/src/utils/otlp-any-value.ts, outside this diff). The same fix covers logs and metrics, because all three signals share this encoder.
Contributor
Author
22 tasks
An inert span started with a `parent` header now echoes that header from `traceparent()` and is activated by `withSpan`, so a service that records nothing keeps a distributed trace whole instead of severing it.
This was referenced Sep 1, 2026
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
3 participants
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
Developers instrumenting a Node service with PostHog have no way to record spans. They can send events, logs and metrics, but nothing that shows where time went in a request or how work fans out across services. Pointing an OpenTelemetry SDK at PostHog works, but it means adding an OTel dependency and wiring the join to PostHog identity by hand — so traces end up disconnected from the person and session they belong to.
Part of a stacked series adding first-class tracing to the JS SDKs (Q3 Goal 4). Scoped to be releasable on its own, one commit, on top of the shared OTLP attribute encoder from #4708 (merged).
Changes
Adds
startSpan,withSpanandgetActiveSpantoposthog-node, behind a newtracesclient option. Spans are encoded as OpenTelemetry-shaped OTLP JSON and POSTed to/i/v1/traces— without an OpenTelemetry dependency.tracesoption means no spans. Every span API still returns a working (inert) handle, so calling code never branches on whether tracing is on.posthogDistinctIdandsessionId, taken fromwithContextor the existing Express/NestJS middleware. This is what makes a trace reachable from a person or session.parentaccepts an inboundtraceparentstring to continue a remote trace;span.traceparent()gives you the header to propagate onward.tracestateis preserved opaquely.awaiton Node, viaAsyncLocalStorageinjected at the Node entrypoint. Core stays runtime-agnostic (nonode:async_hooks), so the edge build and future browser/RN hosts work off the same engine.flush()drains spans, concurrently with the event queue. Serverless handlers callflush(), notshutdown(), so leaving spans on their own timer would silently lose them for the most commonposthog-nodedeployment. A span ending also refreshes thewaitUntilcycle, so a handler that only traces still holds its invocation open.maxLiveSpans(default 10000) caps how many spans may be open at once andmaxSpanAgeMs(default one hour) stops accounting for one that stays open longer, so code that starts spans and never ends them cannot grow the SDK's bookkeeping without limit. At the capstartSpanreturns an inert handle; an evicted span is never exported. Both drops go through the existing span-drop warning.Reviewer notes
awaitthere begin a new trace. Passparentexplicitly to nest them. This is the documented browser limitation too.IPostHoggains three required members (startSpan,withSpan,getActiveSpan). Consumers using it as a type annotation are unaffected; anyone implementing it (hand-written test doubles, DI wrappers) will get a compile error. Same shape asmetricsin feat(metrics): wire posthog.metrics into posthog-node #4117, which shipped under aminorbump._sendOtlpBatch(from fix(core): send logs and metrics through one OTLP batch sender #4623) rather than a third copy of the retry policy — the bearer-auth branch is reintroduced there for traces.flush()andshutdown()as span drains.Live span boundsrequirement is a registry of live span objects, which would keep every leaked handle alive for the length of the age bound. Instead the engine keeps aMapof span id to monotonic start — ids and numbers, never the span — so a handle the caller drops is still collected like any other object, and the count bound can be generous because a slot costs tens of bytes rather than a whole span.packages/core/src/traces/live-spans.spec.tskeeps itsWeakRefprobe, now as the guard that stops a later change from turning this into a registry of spans. Eviction is lazy atstartSpan, and sweeps before reading the bound, so a process that has leaked its way to the cap recovers on the first call after the leaks age out rather than losing tracing for the rest of its life.Retry-Afteris not honoured — the backoff is purely exponential, capped at 30s. This is a shared gap with the logs and metrics senders rather than something this PR introduces, and the service does not emit 429 yet. Tracked separately.beforeSpanSendand per-span caps (feat(node): beforeSpanSend hook and per-span limits #4584), OpenTelemetry soft-detect, browser host, and log/exception correlation.Verification
End-to-end against a real project (381971), twice: first a scripted run, then a real Express app using
setupExpressRequestContextwith two routes, a genuine HTTP hop between them, and an error path. Spans read back out of the product assembled correctly —POST /checkoutroot withdb.queryandhttp.post paymentsbeneath it,GET /payments/chargecontinued across the hop viatraceparent, and a separate trace for the thrown route carryingstatus_code: 2.posthogDistinctId/sessionIdcame from theX-POSTHOG-*request headers on every in-context span, with no manual plumbing.Also exercised against production ingestion: an oversized batch really does 413 (
11.5 MB → 413, halved to5.75 MB → 200twice, all spans queryable afterwards), and 50k spans push through at ~310k/sec with 204 KB average batches — the first real evidence that the 512-span default sits under the body cap. With the endpoint down, 200k spans cost exactly 1 request rather than one per span.packages/core1305 pass (62 suites),packages/node1000 pass (35 suites) — identical under the edge runtime environment — lint clean, public API references regenerated.Release info Sub-libraries affected
Libraries affected
@posthog/coreis also bumped (minor); it has no checkbox above.Checklist
Backwards compatibility caveat: all runtime surface is additive, but
IPostHoggains three required members — a compile-time break for implementors only, matching themetricsprecedent. Bundle: the traces module is core-resident and the browser is not wired up in this PR.If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Built with Claude Code, directed by @turnipdabeets, from the merged
tracescapability spec insdk-specs.Decisions worth flagging for review:
posthog.traces.*namespace — matches how the OTel API reads and keeps the common call short.OtlpSpanKeyValueis an alias of the logs/metricsOtlpKeyValuerather than a separate declaration — one encoder produces all three payloads. The span-flavoured name stays so the span types read as span types.@posthog/typesonly, matching how the logs and metrics wire types are already organised.Put through five rounds of independent fresh-context review, each reviewer required to demonstrate a finding by running it rather than by reading. Bugs they caught, all fixed here and each now pinned by a test that fails when the fix is reverted:
status.messageandtraceStateskipping the sanitiser attributes go through, so one lone surrogate would 400 an entire 512-span batchresourceAttributesrethrowing on every flush, exporting nothing, foreverwaitUntilserverless path having no traces coverage at all, and a span-only handler never registering with itSeveral of those were introduced by earlier rounds' own fixes, which is why the loop ran as long as it did.