refactor(core): share one OTLP attribute encoder across logs, metrics and traces - #4708
Merged
Merged
Conversation
… and traces Moves toOtlpAnyValue/toOtlpKeyValueList out of logs into core/utils and widens them to unknown. Adds a bigint branch, so a bigint attribute encodes as an int64 rather than falling through to a string.
23 tasks
Contributor
posthog-node Compliance ReportDate: 2026-08-31 19:46:59 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
|
turnipdabeets
marked this pull request as ready for review
August 31, 2026 18:51
Contributor
Contributor
Prompt To Fix All With AI### Issue 1
packages/core/src/utils/otlp-any-value.spec.ts:51-52
**Incomplete Logger test mock**
The new bigint test defines only `debug` and bypasses the required `Logger` interface with `as any`, preventing the test from detecting interface drift and leaving the mock unrepresentative of a valid caller.
```suggestion
const logger = {
debug: jest.fn(),
info: jest.fn(),
warn: jest.fn(),
error: jest.fn(),
critical: jest.fn(),
}
expect(toOtlpAnyValue(18446744073709551616n as unknown as LogAttributeValue, logger)).toEqual({
```
---
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): share one OTLP attribute..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
posthog-js Compliance ReportDate: 2026-08-31 19:53:10 UTC ✅ All Tests Passed!26/26 tests passed Capture Tests✅ 26/26 tests passed View Details
|
Contributor
|
Size Change: +6.16 kB (+0.03%) Total Size: 20.8 MB 📦 View Changed
ℹ️ View Unchanged
|
…gger mock The changeset named metrics and spans, where the encoding change cannot occur.
turnipdabeets
force-pushed
the
refactor/shared-otlp-attribute-encoder
branch
from
August 31, 2026 19:39
aee91f8 to
61a4865
Compare
Contributor
|
dustinbyrne
approved these changes
Aug 31, 2026
This was referenced Sep 5, 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.
What this does
PostHog sends logs, metrics and (soon) spans to the same ingestion endpoint, in the same wire format. Turning a JavaScript value like
42or"hello"into that format is done by one function,toOtlpAnyValue.That function lives inside the logs folder, even though metrics already imports it and traces is about to. This PR moves it to
packages/core/src/utils/otlp-any-value.ts, where all three can own it equally.No signal gets its own copy. That's the whole point — see below for why a copy would be dangerous.
Why it matters
The encoder's job is to make sure a value the application handed us can't break the request. If a single value is encoded in a way the server rejects, the server rejects the entire batch — every other log or span travelling with it is lost, and the rejection is not retriable.
So the function is full of hard-won guards: circular references, values whose getters throw, functions, symbols,
NaN, dates with a brokentoISOString, numbers too large for the field they land in. Three copies of that would drift, and the drift would show up as silently dropped data.The one behavior change
A
bigintattribute now sends as an integer instead of a string:Previously there was no
bigintcase at all, so a bigint fell through to a generic "turn it into a string" fallback at the bottom of the function.Nobody will notice. I sent one batch to a real project with the same attribute under both encodings and read it back:
{"stringValue":"123"}orderId: "123"{"intValue":"123"}orderId: "123"Identical. The ingestion service flattens attribute values to strings whichever field they arrive in, so nothing in the product can tell the two apart — no dashboard, no filter, no saved view.
The changeset lists
posthog-js,posthog-react-nativeandposthog-nodealongside@posthog/core, because their users reach this encoder through logs and metrics. Abigintis legal in a log attribute today without any cast —LogAttributeValueincludesRecord<string, unknown>, so{ meta: { orderId: 123n } }type-checks and hits the new branch.How I checked the move didn't change anything else
Behavior was verified by output, not by reading the diff. I ran the old and the new encoder over the same corpus — strings, numbers, floats,
NaN, booleans,null, dates, arrays, nested objects, bigints at several depths — and compared the JSON.Everything matched except the three bigint cases:
packages/core1090 tests pass across 55 suites, typecheck clean, lint clean, build clean.Why it's a separate PR
It was part of #4579 (distributed tracing). It came out because it needs a different kind of review: this one asks "does the move keep logs and metrics behaving exactly as before?", which is a line-by-line comparison of already-shipped code. #4579 asks "is this tracing design right?". Bundled together, the second question tends to crowd out the first.
Splitting it also takes #4579 from 40 changed files to 33, and stops it touching
logs/andmetrics/at all.This lands on
mainfirst; #4579 stacks on top.Release info Sub-libraries affected
Libraries affected
@posthog/coreonly (patch); it has no checkbox above.Checklist
toOtlpAnyValueandtoOtlpKeyValueListkeep their exported names from@posthog/core; only the file they live in moved. The parameter type widens fromLogAttributeValuetounknown, which is safe — a wider parameter accepts everything the narrower one did.If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Built with Claude Code, directed by @turnipdabeets. Split out of #4579 during a review pass; the bigint storage check above was run against a live project rather than assumed.