feat(node): beforeSpanSend hook and per-span limits - #4584
Conversation
|
Size Change: +33.5 kB (+0.16%) Total Size: 20.9 MB 📦 View Changed
ℹ️ View Unchanged
|
a9ff420 to
5c7301c
Compare
9d824fd to
2f1c9aa
Compare
e36495f to
9365a6c
Compare
2f1c9aa to
2204f06
Compare
8192514 to
2fb2797
Compare
babae47 to
1d01943
Compare
2fb2797 to
c3b0a82
Compare
1d01943 to
a6d394d
Compare
cc7b6d8 to
ce6e01a
Compare
a6d394d to
f7df34d
Compare
ce6e01a to
a4e6d3f
Compare
f7df34d to
de9e7cd
Compare
a4e6d3f to
dcd14c4
Compare
de9e7cd to
ea15b7f
Compare
ea15b7f to
8bfae9e
Compare
dcd14c4 to
b946de6
Compare
8bfae9e to
f628066
Compare
b946de6 to
ab4048c
Compare
f628066 to
86bb18e
Compare
ab4048c to
caf83b0
Compare
86bb18e to
eb8fda8
Compare
caf83b0 to
075298b
Compare
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
orderedKeys tested `key in attributes`, which walks the prototype chain, so an attribute named `constructor` or `toString` survived the hook deleting it and read back as the inherited member. At the cap those ghosts took the slots: a hook returning a scrubbed bag exported ["toString","valueOf"] and dropped the one attribute it meant to keep. Three more from the same review: - The new name bound cut the SDK's own `exception` event name below a bound of 9, so the reserve stopped recognising it and dropped the event it exists to keep. `eventNameBound` floors it, at both sanitize call sites. - `_startFlush` installed its in-flight slot only after `_flushInner` had run its synchronous prefix, which this PR lengthened by bounding the resource attributes. A getter there that ends a span re-entered with no pass recorded and re-sent the head batch — 3070 times in a probe, not once. The pass now starts a microtask later, after the slot is installed. - A `false` entry from `[featureEnabled && scrub]` no longer reports an inert redaction hook, and the maxAttributeValueLength doc lists what it now bounds.
…to HEAD # Conflicts: # packages/core/src/traces/span.ts
The encoder drops a nullish value without charging its budget, so charging one here made this walk the stricter of the two and broke the invariant the rest of the walk relies on: a value with 10,000 null leaves ahead of a large string exhausted this budget while the encoder still had room, and the string shipped whole — 2 MB under a bound of 8, with no backstop on either side. Nullish leaves are free again; the shared-subtree cost that charging fixed stays fixed. Also from the same review: - `_startFlush` samples the generation before the microtask, so a `reset()` in that window marks the pending pass stale rather than letting it drain the post-reset queue alongside the pass `reset()` started. - `orderedKeys` uses `propertyIsEnumerable`, the predicate the encoder itself uses, so a key a hook hid by making it non-enumerable cannot take a cap slot. - The `beforeSpanSend` filter reports only on a value meant to be a hook, so `[items.length && scrub]` and `[name && scrub]` are quiet too.
`_flushEventsAndSpans` combined the two flushes with `Promise.all`, which rejects the moment the event flush does. A serverless host treats the returned promise as the end of the invocation, so an event endpoint failing mid-request let the platform freeze the handler with the span POST still open. `allSettled` waits for both and still surfaces the events rejection to the caller. `reset()` also discarded whatever was queued without a word, while the only line the operator had seen was the export failure promising a retry on a flush that will never come. It now names the count at `critical`, the one level posthog-node does not gate behind `debug`.
Budget parity was explained in four places; keep it in the function doc. Rewrite the three changesets as one-line, outcome-first entries. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013ERsAtgPJqKF46yur8bm6K
A drain sends one batch per loop iteration, so the user could opt out while a batch was in flight and the batches behind it would still export. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L8eFZB35NExUZyq5hGgh43
dustinbyrne
left a comment
There was a problem hiding this comment.
Requesting changes for the three confirmed correctness issues called out inline. Each is reachable through the public tracing API and was reproduced against 8997a166:
- stateful
toJSON()can bypassmaxAttributeValueLength; - incomplete hook return values can be exported as malformed spans;
- fractional numeric options are floored instead of using their documented defaults.
The broader ownership looks sound: live spans enforce limits on write, limits are reapplied after hooks, queue/retry/consent remain in PostHogTraces, and cross-signal flushing remains in PostHogBackendClient.
I also called out one non-blocking chained-hook identity edge and one contract decision concerning the exception-event reserve.
Changeset provenance looks correct: the hook, limit, and stack-trace changesets belong to this PR; the other tracing changesets are inherited from #4579. The branch is currently stacked cleanly and does not need a rebase at the reviewed commit.
Agent-assisted review provenance: Dustin requested an independent review plus changeset and stacking checks without seeding suspected code defects. We discussed the resulting findings and architecture during the review. Dustin has been good to the agent.
Reclassifying this review as non-blocking feedback.
ioannisj
left a comment
There was a problem hiding this comment.
Approving to unblock one more review cycle but #4584 (comment) is my finding as well, so worth addressing
Takes #4773's move of resolveTracesConfig into core. The beforeSpanSend resolver and the three per-span limit knobs move with it; the logger this PR threads through the resolver stays a core-internal type.
Keeping the object left the encoder to probe toJSON a second time, so a serializer that answered null under the bound could answer with a megabyte over it. Stores the string the encoder builds from the same result instead, which leaves the wire unchanged and gives it nothing left to re-probe. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MTHb6UUMhVJSWC2cWBzSxc
Flooring made maxAttributesPerSpan: 1.5 resolve to 1, capping a span an order of magnitude below what the caller wrote and saying nothing. Every numeric traces option now falls back the way the spec describes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MTHb6UUMhVJSWC2cWBzSxc
Carrying attributes and events was the whole shape check, so a hook returning only those two exported a span named unknown at a fallback time with no join keys, silently. A record missing any field the public SpanRecord requires is now a counted drop, as the rest of the hook contract already is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MTHb6UUMhVJSWC2cWBzSxc
An earlier hook that forged an id and froze what it returned refused the restoring writes, so the next hook in the chain sampled on the forged id. Hands it a corrected view built from the record's own descriptors, which keeps the prototype and the keys the hook returned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MTHb6UUMhVJSWC2cWBzSxc
The second call is the encoder's, so the guarantee is worth asserting past the record. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MTHb6UUMhVJSWC2cWBzSxc
The reserve is for what the SDK records on your behalf, so an event named `exception` by the caller was claiming it too. Marks SDK-recorded events with an internal symbol the hook cannot see and the wire cannot carry, and keys both enforcement points on that. Removes eventNameBound with it: the name no longer decides anything, so it no longer needs a floor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MTHb6UUMhVJSWC2cWBzSxc
Drops the four reserved slots for exception events. The reserve bought a case we cannot show occurs — a span holding 128 events that then throws — at the cost of the most intricate code in this change, which had already carried one bug. The cap now matches the spec's number exactly. A span that fills its events and then throws keeps its error status and reports the loss through droppedEventsCount, so the case is measurable once traces ships and the reserve can be added back additively if it turns out to matter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MTHb6UUMhVJSWC2cWBzSxc
Resolves the import conflict in traces/index.ts: keeps this branch's applySpanLimits and truncateAttributes, and drops NOOP_SPAN, which the base replaced with inertSpan at the pass-through-parent site so an inbound context carries to the child. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J1XjgPzJ4zbHBcYmEDymA2
The reserve was removed and the event cap is now absolute, but the changeset still promised it. Also names the dropped counters, which are how a caller sees that a span was truncated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J1XjgPzJ4zbHBcYmEDymA2
Resolves three conflicts in the traces queue against #4584: - `_startFlush` keeps the microtask defer and generation sampling, re-arming through the ratchet so a flush cannot pull a Retry-After wait forward. - The two drop paths keep both resets: the consecutive-failure counter and the head batch's own budget. - A batch of one refused for size is now reached from the SDK's own measurement as well as a 413, so the drop reason names neither. Also corrects the body-cap comment #4584 adds: the caps bound a span's attributes but not how many attributes its events carry, so a span is not bounded overall. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YE1TyDWGyLwUXEX3ffy83K
…tracing #4584 folded into this branch, so `beforeSpanSend` and the caps go out in the same minor as `startSpan`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RWoFx4BctNmXnpnKS79Qgz
Problem
Two gaps in what a finished span may carry, which only make sense together:
Nothing scrubs a span. Spans carry whatever attributes the application puts on them, and a service that instruments its HTTP layer ends up with headers, query strings and user fields on them. There was no way to strip any of it before it left the process, and no way to drop a span you don't want exported at all (health checks, noisy internal polling).
Nothing bounded a span. A loop calling
span.addEvent()per iteration, or an instrumentation layer copying every request header onto a span, grows it without limit until the ingestion endpoint rejects the payload as too large — at which point the 413 path shrinks the batch to a single span, that span is still too big, and it is dropped entirely. You lose the whole span rather than the excess.Implements the
Gating and beforeSpanSendandSpan limitsrequirements of the traces capability spec.Changes
beforeSpanSendruns on every finished span before it is queued.nullto drop the span. Nothing is enqueued and no error surfaces.userId: 42, not{ intValue: "42" }. It runs after auto-context attributes are attached, soposthogDistinctIdandsessionIdare visible and scrubbable.traceId,spanIdandparentSpanIdare typedreadonly, and an assignment that slips past the type system is reverted with a debug warning — rewriting ids after children have already shipped corrupts parentage.PostHogLogs._runBeforeSend, which also catches and drops — the spec describes the logs hook as fail-open, but this SDK's implementation is not.nullstops the chain. Non-callable entries are ignored rather than called.Per-span caps at OpenTelemetry's defaults.
droppedAttributesCount/droppedEventsCount, so a truncated span is visibly truncated rather than quietly wrong. Both are omitted when nothing was dropped.posthogDistinctId,sessionId,url.full,screen.nameandapp.statedo not count toward the cap, so a span at the limit still links back to its person and session. The exemption is from the count only — a hook can still delete them, which is the point of a scrubbing hook.exceptionevent the SDK records spends an ordinary slot like any other, so a span that fills its events and then throws keeps itserrorstatus but not the exception detail.droppedEventsCountreports the loss. See The exception reserve, and why it isn't here below.A per-value length bound,
maxAttributeValueLength, default 8192.exception.stacktraceon spans.recordExceptionand a throwingwithSpancallback now attach the stack alongsideexception.typeandexception.message. Stacks ship by default and carry your server's file paths — the changeset says so, andbeforeSpanSendcan remove them.The caps re-apply after
beforeSpanSend, which is why these ship together.Why one PR
These were opened as #4584 and #4586, two branches off the same base. The spec requires "The caps SHALL be re-applied after
beforeSpanSend" — and neither branch can satisfy that alone, because neither has both halves in its tree. Split, the best either could do was leave a note asking whichever merged second to remember.The failure that requirement prevents is real and not theoretical: the hook writes to the plain record, not through the span's guarded writer, so a hook that enriches a span — adding a customer tier, a region, a git SHA, the second most obvious use after scrubbing — pushes it straight back past the cap, with no trim and no dropped count. It then 413s and the span is lost whole, which is exactly what the cap exists to prevent.
They also conflicted in six files, so there was never a version of this where the two could be reviewed independently.
Reviewer notes
applySpanLimits, called at the end of the hook chain. Earliest-set entries win, matching the span-side rule, SDK-attached keys stay exempt, and the counts add to whatever the span itself already dropped. A hook that only removes attributes cannot invent a dropped count.PostHogSpan's end callback now passes the auto-attached keys alongside the record. This is internal — the publicSpanRecordis deliberately unchanged, so the exempt-key set never becomes API surface.PostHogSpan, not at encode time, so an over-cap span never occupies memory in the first place — the point is to bound growth, not to trim on the way out.beforeSpanSend. Only then does the queue-depth check run, so a dropped span never counts against the queue.posthogDistinctIdthemselves gets the exempt treatment for that key.Object.freeze({ ...span })scrubber returned.WeakSetand a 10,000-node cap — not just a depth limit. Depth alone is not a bound when siblings share a subtree.name/startTime/endTime. A hook that appends or rebuildseventsbypasses the checksaddEventapplies, so an event with no timestamp encoded astimeUnixNano: "NaN000NaN"and one scaled by1e6encoded as a 25-digit value. Neither throws in_encodeBatch, so the span shipped, the server rejected the request, and_flushInner's fatal branch spliced the whole batch — up tomaxExportBatchSizeunrelated spans lost.What review changed
Two fresh-context reviewers went at the branch, one required to demonstrate every finding by running it. Each fix below is pinned by a test that fails when it is reverted.
truncateAttributeValuehad a depth cap and nothing else, so a value whose children point back at their siblings costfanout ** depthvisits: two self-references measured 254 ms of synchronous work inside the caller's ownsetAttribute, three did not finish in two minutes. It now carries the sameancestorsWeakSet and node budget asencodeAnyValue, which it was supposed to mirror. This was a regression against the base, where the raw value went straight to the guarded encoder.toJSONbypassed the bound entirely. The walk read own keys; the encoder preferstoJSON. A dayjs value, a Decimal, or an ORM document could put a megabyte on the wire. The walk now resolvestoJSONthe way the encoder does.for...in), so a pollutedObject.prototypekey became an attribute of every span, defeating the encoder's ownpropertyIsEnumerableguard.__proto__key was lost and swapped the copy's prototype — the copy used assignment where the rest of the codebase usesdefinePropertyfor exactly this.recordExceptionset from the same string it had just truncated for the event.status.codeencoded as an empty status object, silently losing an error the span really had. An inventeddroppedAttributesCountreached the wire uncoerced.The exception reserve, and why it isn't here
Review found that a span already at
maxEventsPerSpanwhich then throws keeps itserrorstatus but loses theexceptionevent, stack included. This PR carried a fix for a while: four slots reserved past the cap, on the reading that the spec caps "user-supplied" content and an exception the SDK attaches on your behalf isn't user-supplied.It has been removed, and the cap is now absolute.
Two rounds of review went into the reserve and it produced two bugs of its own — first a reserve that was really a smaller exclusive cap (twenty
recordExceptioncalls on an empty span kept four), then eligibility granted by event name, so a caller's ownaddEvent('exception')could put 132 events past a documented 128. Fixing the second properly meant an internal provenance marker on the event record, which worked but added a third thing to carry.Set against that: nobody can show the case occurs. It needs a single span holding 128 events and then throwing. Traces has not shipped, so there is no usage to appeal to.
The decisive asymmetry is that adding the reserve later is purely additive — no caller breaks when a cap grows — while shipping it now and removing it later is a behaviour change. And the case is measurable in production without it: a span that fills up and throws still exports
status: errorand a non-zerodroppedEventsCount, sostatus = error AND droppedEventsCount > 0finds exactly this population once traces is live. If it turns out to be real, the reserve comes back with numbers behind it.Removing it deleted 95 lines and one whole class of duplication: the rule no longer has two independent implementations in the span writer and the post-hook re-apply, so they cannot drift. It also means
maxEventsPerSpanmatches the spec's stated number exactly, with nosdk-specschange needed to ship.Span and event names remain unbounded. That is 4579's path and unchanged here.
Known residuals, each rated NIT and deferred deliberately:
A hook can still mutate a queued span through a nested value it kept a reference to. The walk returns the same reference when nothing needed shortening, so a hook that holds a nested object and edits it after returning reaches the wire. This is the window
setAttributehas always had rather than something the hook introduces, but the "edit in place" documentation invites the assumption that returning ends it.An array whose own
slicethrows escapes the length bound.Integer-like attribute keys break "earliest-set wins" in the post-hook re-apply.
Object.keysfront-loads array-index keys, so a hook adding an attribute named"0"jumps the queue and can evict a real one. Fixing it properly means tracking insertion order in aMaprather than an object, which is a data-structure change for a case that needs an attribute literally named"0".A span that fills its events and then throws loses the exception detail. Deliberate, as above: the cap is absolute and the loss is reported through
droppedEventsCount. RaisingmaxEventsPerSpanis the workaround for a span that both records many events and can fail.Second review round
Two more fresh-context reviewers went over the post-fix branch, one required to demonstrate every finding by running it. Both independently found the same two blocking defects, and both are now fixed and pinned by tests:
maxAttributeValueLength. The traversal budget was charged before the string leaf was bounded, and the walk had no per-container item cap where the encoder stops at 1,000. So{ rows: [20k ints], html: 2MB string }spent the whole 10,000-node budget on elements the encoder never emits and shipped the sibling string whole — 2,000,000 characters against a bound of 8192, on default config with no hook. Strings are now bounded before the budget is consulted, only containers are charged, and arrays are walked to the encoder's own item cap. The same repro now yields 8192 characters and a 27 KB body.Also fixed from that round:
status.codein place defeated the fallback, because the snapshot held a reference to the object the hook was editing — and in-place mutation is the style this PR's own doc example uses. The same bug applied to the dropped counters. Both are now snapshotted by value before the chain runs.eventsthrough a prototype getter lost every span to the fail-closed branch. Naming the fields also means nothing a hook attached beyond them can reach the wire, and the SDK's own dropped counters are taken from the span rather than from the hook's return value.eventscost the whole span; it now costs that entry._writeAttributebounded values before checking the cap, spending ~354 ms of a 389 mssetAttributescall walking values it was about to drop. The cap is checked first.toJSONran twice, and a value that answered differently the second time escaped the bound entirely. It now resolves once and the resolved form is what is stored.Third review round
Two more independent reviewers: one hunting bugs, one looking only for accidental damage to already-shipped code.
One more blocking defect, fixed and pinned:
maxAttributeValueLengthentirely. The walk read child properties inside the parent'stry, so one throw abandoned the whole value and the catch returned it untruncated. On default config,{ body: <5 MB string>, get lazyRelation() { throw } }shipped a 5,000,717-byte payload; the same value without the getter shipped 8,843. That is a lazy ORM relation or a disposed resource sitting next to a large field — the shapeassignUserAttributesalready guards at depth 0, but only at depth 0. Per-key reads are now guarded individually: a throwing accessor costs its own key, which gets the encoder's[Unserializable]marker, and its siblings are still bounded. Re-measured: 8,979 bytes.The test covering this asserted the value was left alone, so the bug was pinned as intended behaviour. It now asserts the sibling is bounded.
Also fixed:
nullineventscost the whole span, not one event: the sanitising pass ran before the guard meant to catch it. Each event is now read behind its own guard.attributeswas not repaired the wayeventswas, so a hook setting it tonulldropped the span.toJSONresolving toundefinedconsumed a cap slot while encoding to nothing, and an invalidDatevanished where it previously shipped"Invalid Date". A nullish result now keeps the original value.status.messagefrom a hook bypassed the length bound and was stringified at full length by the encoder.No regressions to shipped code. Measured, not assumed: the
posthog-jsbrowser bundle is +0 bytes (un-minified bundles byte-identical; none of the traces symbols appear in either),packages/core/src/utilsis untouched,describeError's newstackfield reaches nothing outside traces (it is on no barrel), neitherSpanRecordnorBeforeSpanSendFncollides with an existing export, and there is no import cycle. Full suites across 24 packages: ~13,370 passing, 0 failures. One@posthog/nuxtfixture fails identically on the base commit — upstream nuxt-nightly breakage, unrelated.Fifth review round
A pass over the branch after merging #4579's latest base into it. One conflict, in
traces/index.ts's./spanimport: this branch'sapplySpanLimitsandtruncateAttributeskept,NOOP_SPANdropped, because the base replaced that call site withinertSpan(options)so a pass-through parent's inbound context carries to the child. Everything else auto-merged.positiveIntegernow falls back to the default for a fractional knob rather than flooring it, so that residual is gone:maxAttributesPerSpan: 1.5resolves to128, not1.The span-limits changeset still promised "a small reserve for exception events" after the reserve was removed, which would have shipped a wrong entry in the published changelog for three packages. Corrected, and it now names the dropped counters, which are how a caller sees a truncated span.
Re-verified on the merged tree:
packages/core1530 pass / 1 skipped (64 suites),packages/node995 pass (35 suites) plus the edge-runtime config,@posthog/types3 pass, oxfmt and oxlint clean, public API references regenerate with no drift. Run as a real service against a local ingestion endpoint, reading the decoded OTLP off the wire — 45 assertions covering the hook (drop, scrub, forge, throw, frozen return,asyncreturn, prototype-getter return), the caps and their dropped counts, the join-key exemption under a realwithContext, the value bound through nesting /toJSON/ a cyclic value / a throwing sibling getter, bounded status messages and resource attributes, and an inbound distributedtraceparentreaching the hook with its real ids.Fourth review round
Two reviewers on the ground earlier rounds had not covered: lifecycle and concurrency, and the thing run as a real application.
Three fixes, all of them consequences of round three's own repairs:
attributesto{}when it was not an object meant anasynchook — whose Promise is truthy and readsundefinedfor every field — exported a span namedunknowncarrying no attributes at all: noposthogDistinctId, nosessionId, joinable to nobody, and silently. Before that repair it threw and was a warned drop. A return value missing either collection is now dropped and counted, which is the fail-closed rule the rest of the hook contract already follows. This also coversattributes: nulland an array, which would have encoded as{ "0": … }.toJSONresolved nullish let the walk bypass a redaction. The value fell through to the own-key walk, which produced a plain copy the encoder no longer recognises as self-describing — soclass Redacted { toJSON() { return null } }put its internals on the wire atmaxAttributeValueLength: 10and redacted correctly at 100. A self-describing value is now left whole.status.messagecould throw and cost the span. A{ toString() { throw } }message exported nothing, where the encoder downstream only marks the field. The coercion is guarded.Ran as a real Express service against production ingestion (
cycle4-svc-1788369331, project 381971) withsetupExpressRequestContext, a scrubbing hook, nested spans, a genuine HTTP hop and an error route — 17 spans across 4 traces read back out of the product. Parentage held across the hop on one trace id;posthogDistinctIdandsessionIdon 17/17 spans including downstream ones; the scrubbed header absent from all of them; zero/healthspans from five requests; error spans atstatus_code: 2. The hook wrotetraceId = '0'.repeat(32)on every span and every exported id is real — identity immutability demonstrated in production, not just in a unit test.Spec conformance walked scenario by scenario: 24 checks over
Span limits,Gating and beforeSpanSend,Configuration knobsand the exception-recording scenarios — 0 failures, withcontext manager formnot applicable (Python-only).Cost
Measured against the base over 200k spans, three repeats, medians: a plain span goes 478k/s → 403k/s (2.09 → 2.48 µs), a span with 20 attributes 249k/s → 151k/s (4.02 → 6.62 µs), and a configured hook costs about the same again. No heap regression.
Most of the attribute-heavy cost is not the new work. It was attributed rather than guessed: neutering truncation, the cap accounting, and the ancestors set each moved it by ~5k/s, while swapping the null-prototype attribute store for
{}moved it to 195k/s — V8 puts null-prototype objects in dictionary mode. That store is the__proto__-pollution guard, so this is a deliberate trade, kept: the worst case is 6.6 µs per span, 0.66% of a core for a service producing a thousand spans a second.Verification
packages/core1461 pass (63 suites),packages/node1034 pass (36 suites), oxfmt and oxlint clean,@posthog/types/@posthog/core/posthog-nodebuild clean, public API references regenerate with no drift.Checked end-to-end against a real project (381971) with the caps set low so the behaviour is visible — attributes 5, events 3, values 64 — and read back out of the product, not just off the wire:
GET /healthdropped by the hook: absent from the payload and from the product.http.request.header.authorizationdeleted by a hook that then froze what it returned: gone, span still exported,enriched.tierpresent.The hook forged
traceIdto all zeros on every span; the exported ids are the real ones.overflow: keptattr-0..attr-4plus both join keys,droppedAttributesCount: 16(15 user attributes plus the hook's own enrichment, trimmed by the post-hook re-apply),droppedEventsCount: 7.long-values: a 5000-character string, the same string nested inside an array, atoJSONreturning 5000 characters, a doubly self-referencing object and a 5000-character status message — all bounded to 64, nothing hung.A 5000-character
resourceAttributesvalue bounded to 64 on the batch resource.retries, eightrecordExceptioncalls with the event cap at three: three kept,droppedEventsCount: 5.throwing, on a span that added ten events before it threw with the event cap at three:step-0..step-2kept,droppedEventsCount: 8,status_code: 2, original error propagated unchanged. Theexceptionevent does not fit, which is the documented behaviour of an absolute cap.These two figures are recomputed against the implementation, not re-observed: the product read-back described in this section was run before the reserve was removed, so its
retriesandthrowingspans reflect the reserve's behaviour. Everything else in the list is unaffected by that change and stands as recorded.Both counters omitted on the spans that dropped nothing.
Carried over from #4579's review
Consent is now re-checked between span batches. A drain sends one batch per loop iteration and gates consent only before the first, so a user opting out while a batch was in flight left the batches behind it exporting
posthogDistinctIdandsessionIdafterwards._flushInnernow re-checksisDisabled/optedOutat the top of every iteration and discards the rest of the queue. It is the existing discard block moved into one method and called from a second place, not a new mechanism. Two regression tests: a backlog built during an outage, thenoptOut()— and separatelydisable()— landing while the retried head batch is in flight, both verified to fail with the loop-body check removed.Fixed here rather than in #4579 because that PR is approved and this one is stacked on it, so both ship in the same release.
disable()was already covered before this:_sendOtlpBatchshort-circuits ondisabledand returns{ kind: 'fatal' }, which the loop splices away without a request. The genuine hole wasoptOut(), which nothing downstream checked.No changeset: the behaviour it corrects is inside the traces feature #4579 introduces, which has not shipped.
Release info Sub-libraries affected
Libraries affected
@posthog/coreis also bumped (minor); it has no checkbox above.Checklist
All surface is additive on top of #4579, which has not shipped, so nothing here can break released code.
If releasing new changes
pnpm changesetto generate a changeset fileDocs: PostHog/posthog.com#19837 covers this surface too — a Scrubbing and dropping spans section, a Span limits section, and the new configuration rows. It stays a draft until this and #4579 ship, and documents them as one release, since this PR merges into #4579's branch.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Built with Claude Code, directed by @turnipdabeets, from the merged
tracescapability spec insdk-specs. The merge of #4586 into this PR, and the cap re-apply, came out of a review pass that checked the stack against that spec and found the requirement no single PR could satisfy.