fix(core): send all three OTLP signals through one batch sender - #4575
Merged
Conversation
Contributor
Author
posthog-node Compliance ReportDate: 2026-08-21 13:21:24 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
Author
Contributor
Author
posthog-js Compliance ReportDate: 2026-08-21 13:29:38 UTC ✅ All Tests Passed!26/26 tests passed Capture Tests✅ 26/26 tests passed View Details
|
turnipdabeets
changed the title
turnipdabeets
force-pushed
the
posthog-watcher/issue-4570
branch
from
August 21, 2026 13:24
1f81cda to
c19be0d
Compare
turnipdabeets
marked this pull request as ready for review
August 21, 2026 13:26
turnipdabeets
self-requested a review
August 21, 2026 13:26
turnipdabeets
approved these changes
Aug 21, 2026
Contributor
Author
|
Size Change: 0 B Total Size: 20 MB ℹ️ View Unchanged
|
dustinbyrne
approved these changes
Aug 21, 2026
turnipdabeets
force-pushed
the
posthog-watcher/issue-4570
branch
from
August 24, 2026 11:25
c19be0d to
6ec3163
Compare
turnipdabeets
requested review from
TueHaulund,
ablaszkiewicz,
arnohillen,
cat-ph,
fasyy612,
hpouillot and
ksvat
and removed request for
a team
August 24, 2026 11:25
turnipdabeets
force-pushed
the
feat/traces-node-mvp
branch
from
August 24, 2026 11:25
e36495f to
9365a6c
Compare
Contributor
Author
📝 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 |
23 tasks
Contributor
|
Closed by GitHub, not by a merge to Reopened as #4623, retargeted at |
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
2 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
Fixes #4570.
PostHogCoreStatelessgrew three near-identical OTLP senders —_sendLogsBatch,_sendMetricsBatchand_sendTracesBatch— each around 50 lines, differing only in endpoint path and auth style. Nothing kept them in sync and they had already drifted: logs classified an exhausted 408/429/5xx asfatal, soPostHogLogsadvanced its persisted queue and dropped the batch, while metrics and traces returnedretry-laterand kept theirs.Probing each sender through
createTestClientwith retries exhausted:_sendLogsBatch(before)_sendMetricsBatch_sendTracesBatchfatal(dropped)retry-laterretry-laterThe existing specs in
logs/index.spec.tsalready pin the consequence: afataloutcome leaves the queue at length 0, aretry-lateroutcome leaves it at 1.Logs was the odd one out in a second way. The browser's own logs adapter in
packages/browser/src/posthog-logs.tsalready returnedretry-laterfor 429/5xx, so the same product dropped records on React Native and retried them on web. That, more than the metrics comparison, is what makes the old logs behaviour a bug rather than a deliberate choice — though it was a deliberate choice once: the original docblock said logs used "the same policy the events_flush()uses", and the events flush really does drop anything that isn't aPostHogFetchNetworkError.Changes
One sender. The three bodies collapse into a private
_sendOtlpBatch({ path, auth, payload }); the three public methods become one-line wrappers that keep their existing host-contract return types.posthog-core-stateless.tsgoes from ~165 lines of sender to 55 plus three 2-line wrappers. Every duplicated concern now exists once: the disabled check,JSON.stringify, the gzip-unless-disableCompressionbranch, custom headers, thefetchWithRetrycall with its 413-excludingretryCheck, and the four-way outcome mapping.One policy. Everything routes through
isPostHogFetchRetryableError, so exhausted 408/429/5xx areretry-laterfor all three signals. This deliberately diverges from the events_flush(): every OTLP queue is bounded by its buffer cap and retried with exponential backoff, so holding a batch through an outage can neither grow without limit nor spin on a poison payload — the property the events queue can't rely on.Browser parity. The browser logs and metrics adapters classified 408 as a plain 4xx and dropped the batch. They now treat it as transient, matching
isPostHogFetchRetryableError.Coverage. The single-signal test from the first commit becomes a 3 × 7 table in
posthog.flush.spec.tspinning every sender to the same classification, including the 413 →too-largeand non-retryable-4xx →fatalcases that had no coverage at all before.logs/index.spec.tsgains a block drivingPostHogLogsover a real core host instead of a stubbed_sendLogsBatch, so the sender classification and the queue bookkeeping are exercised together — that is the seam where the records were actually lost.Both guards were checked by reverting the classification in
_sendOtlpBatch: 12 matrix failures and 4 end-to-end failures. The browser 408 tests fail without the adapter change.Notes for reviewers
A project over its quota gets a 429, so logs now retry it with backoff until the buffer cap evicts rather than dropping it immediately. That is bounded, and it is already how metrics, traces and web logs behave.
The three exported outcome types (
SendLogsBatchOutcome,SendMetricsBatchOutcome,SendTracesBatchOutcome) are still three identical unions and were left that way on purpose: they appear expanded inline in the generated public API reference snapshots, so aliasing them to one type would rewrite those snapshots and show an opaque alias in the docs for no behavioural gain. The drift risk is closed instead by each wrapper returning the shared value directly, which makes any of the three going out of shape a compile error.Based on #4579 because
_sendTracesBatch— the third sender — is introduced there.posthog-nodedoesn't wirePostHogLogs, so nothing changes for it.Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
The first commit is the original posthog-watcher/pi draft: a one-condition change to the logs outcome mapping. Reviewing that draft (Claude Code) found the title promised an extraction it did not perform, it shipped without a changeset, it deleted the comment recording the old rationale rather than replacing it, and it lacked both the 413/4xx assertions its own description claimed and any test at the layer where records were actually lost.
The second commit does the extraction the issue asked for and closes those gaps. The behaviour fix was kept as its own commit so it stays reviewable independently, as the issue requested. An alternative — extracting first with a per-caller retry classifier to preserve the logs behaviour exactly — was rejected: it would have added a knob to the new helper whose only purpose was preserving the bug.
Verification was local (
jestacrosspackages/core,packages/browser,packages/node, pluseslint); no manual device testing was done.