fix(traces): hold spans back during Retry-After and split locally measured batches - #4847
Closed
turnipdabeets wants to merge 1 commit into
Closed
fix(traces): hold spans back during Retry-After and split locally measured batches#4847turnipdabeets wants to merge 1 commit into
turnipdabeets wants to merge 1 commit into
Conversation
The events flush timer no longer drains spans while the traces queue is honouring a Retry-After window. A batch the SDK measured as too large itself now splits that drain only, leaving the batch size the next drain starts from. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BjT26chjUzuPSyJZqh4AWQ
23 tasks
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
Author
|
Folded into #4726 in |
Contributor
|
Size Change: +1.28 kB (+0.01%) Total Size: 20.8 MB 📦 View Changed
ℹ️ View Unchanged
|
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
Two queue-behaviour gaps from @jonmcwest's second review pass on #4726. Both are in unreleased traces code paths, so they belong in the first traces release rather than after it.
The events flush timer drains spans through an open
Retry-Afterwindow (comment).PostHogTraces.flush()ignores the window on purpose — an explicit flush, or one a serverless host runs to keep an invocation alive, has to send what is queued. But core'sflushBackground()— the events interval timer and theflushAtthreshold — also callsflush(), and node's override drains spans with it. So an automatic flush, which has none of those reasons, sends spans the endpoint asked us to hold. The traces queue's own timer already waits the window out.A batch the SDK measured itself lowers the batch size kept between drains (comment). The pre-send size check and a real 413 both surface as
{ kind: 'too-large' }, so both halve_maxExportBatchSize, which then recovers one healthy batch at a time — about 479 of them from a 512 → 33 shrink. For a 413 that is right: the endpoint refused a body and we do not know why. For a locally measured refusal it is not: the SDK knows the oversized span is gone once the batch has been isolated, so later batches pay for it with no evidence against them.Changes
flushAutomatic()onPostHogCoreStateless, called byflushBackground()in place offlush()and defaulting to it, so nothing changes for browser or react-native. Node overrides it to skip the span drain whiletraces.throttledis true.PostHogTraces.throttledexposes whether the endpoint has asked this queue to wait.measuredLocallyon thetoo-largeoutcome, set by the two pre-send refusals (an unserializable payload, and one overOTLP_MAX_BODY_BYTES) and not by the 413. Traces splits a locally measured batch with a cap that lives for one drain; a 413 still lowers the persistent one.The trade-off Jon names holds: a workload whose batches are persistently oversized now re-measures on each flush instead of settling at a smaller size. That is the right way round — it costs requests on a rare workload, where the current behaviour costs throughput on the common one-poison-span case that the size check was added for.
Notes for the reviewer
flushAutomaticroutes through the same_flushKeepingRuntimeAlivehelper asflush(). Routing it aroundflush()instead would have dropped thewaitUntilregistration on the automatic path — no test covers that today, so it would have been silent.logshas the same 413/local conflation (_maxBatchRecordsPerPost, halve then+1recovery, with a comment that says the refusal is "reached either from a 413 or from the size the SDK measured"). It is not changed here: logs has shipped, so it is a behaviour change for existing users and wants its own PR.metricsis not affected — it drops the whole window ontoo-largeand keeps no batch cap.Release info Sub-libraries affected
Libraries affected
@posthog/corecarries the change;posthog-nodebumps as a dependent. Both are declared in the existingnode-distributed-tracing.mdchangeset on the base branch — traces has not shipped, so the feature and the fixes that land with it are one changelog entry. Neither change alters behaviour for logs, metrics, or the events queue.Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Claude Code (Opus 5), session.
Decisions along the way:
flushAutomaticinitially bypassedflush(), silently removing thewaitUntilregistration from the automatic path. All 995 node tests still passed, which is its own finding.Retry-Aftertest; reverting the local/413 split fails the batch-size test. The first version of the split test passed either way — the+1ramp caught back up inside the same flush — so it was rewritten with a queue depth where the two paths diverge.Verified locally:
packages/core1628 pass,packages/node997 pass,packages/react-native692 pass,turbo buildclean, oxlint and oxfmt clean. No manual/device testing.Stacking: based on
fix/otlp-honor-retry-after(#4726).🤖 Generated with Claude Code
https://claude.ai/code/session_01BjT26chjUzuPSyJZqh4AWQ