fix(core): send logs and metrics through one OTLP batch sender - #4623
Merged
Conversation
Logs classified an exhausted 408/429/5xx as `fatal`, so the batch was dropped instead of held for the next flush cycle. Fixes #4570.
Collapses both senders into `_sendOtlpBatch` so the retry policy is one decision, pins the classification with a per-signal test table, and retries 408 in the browser logs and metrics adapters.
This was referenced Aug 24, 2026
Contributor
|
Contributor
Prompt To Fix All With AI### Issue 1
packages/core/src/__tests__/posthog.flush.spec.ts:779
**Retry tests bypass actual retries**
The new tests set `fetchRetryCount` to zero or invoke a later flush manually, so they verify exhausted-error classification and queue retention without exercising the complete multi-attempt retry flow. A regression in retry count, request sequencing, or the transition from transport retries to retained-queue retries would therefore pass this coverage.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "refactor(core): send logs and metrics th..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
posthog-node Compliance ReportDate: 2026-08-24 17:11:54 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
The classification cases run with retries disabled, so nothing exercised the predicate that keeps 413 out of the retry loop.
Contributor
posthog-js Compliance ReportDate: 2026-08-24 17:19:37 UTC ✅ All Tests Passed!26/26 tests passed Capture Tests✅ 26/26 tests passed View Details
|
Contributor
|
Size Change: -37.3 kB (-0.19%) Total Size: 20 MB 📦 View Changed
ℹ️ View Unchanged
|
dustinbyrne
approved these changes
Aug 24, 2026
turnipdabeets
enabled auto-merge (squash)
August 24, 2026 17:05
turnipdabeets
added a commit
that referenced
this pull request
Aug 24, 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
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. Supersedes #4575, which GitHub auto-closed when the branch it was stacked on was rewritten; the content is the same minus the traces sender, which now lands in #4579.
PostHogCoreStatelessgrew near-identical OTLP senders —_sendLogsBatchand_sendMetricsBatch, each around 50 lines, differing only in endpoint path. 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 returnedretry-laterand kept theirs.Probing each sender through
createTestClientwith retries exhausted:_sendLogsBatch(before)_sendMetricsBatchfatal(dropped)retry-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. Both bodies collapse into a private
_sendOtlpBatch({ path, payload }); the two public methods become one-line wrappers that keep their existing host-contract return types. 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 both 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 per-signal table in
posthog.flush.spec.tspinning both senders 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: the matrix and the end-to-end block both fail. 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 and web logs behave.
SendLogsBatchOutcomeandSendMetricsBatchOutcomeare still two 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 either going out of shape a compile error._sendOtlpBatchtakes no auth parameter here because both signals authenticate with?token=. The traces sender needsAuthorization: Bearer, so #4579 reintroduces anauthargument when it folds_sendTracesBatchinto the same helper — it stays out ofmainuntil there is a caller for it.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.
This work was originally stacked on the traces PR (#4579) because
_sendTracesBatchwas a third copy of the same sender. Splitting it so the shared helper lands onmainfirst and traces joins it afterwards was the reason for reopening as a new PR.Verification was local (
jestacrosspackages/core,packages/browser,packages/node, pluseslint); no manual device testing was done.