refactor(traces): keep the traces plumbing off the public API - #4773
Merged
turnipdabeets merged 1 commit intoSep 4, 2026
Merged
Conversation
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
|
Reviews (1): Last reviewed commit: "refactor(traces): keep the traces plumbi..." | Re-trigger Greptile |
Contributor
Moves resolveTracesConfig into core beside resolveMetricsConfig and assignUserAttributes into utils, so packages/node no longer reaches for either, and replaces the duplicated activation rule with runWithActiveSpan. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L8eFZB35NExUZyq5hGgh43
turnipdabeets
force-pushed
the
refactor/traces-internal-plumbing
branch
from
September 3, 2026 19:58
6280c6b to
93279f6
Compare
turnipdabeets
changed the base branch from
feat/traces-before-span-send
to
feat/traces-node-mvp
September 3, 2026 19:58
Contributor
|
Size Change: +148 B (0%) Total Size: 20.9 MB 📦 View Changed
ℹ️ View Unchanged
|
dustinbyrne
approved these changes
Sep 4, 2026
turnipdabeets
added a commit
that referenced
this pull request
Sep 4, 2026
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.
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
Three symbols became permanent exports of
@posthog/corein #4579, none of them things a developer should ever import:assignUserAttributes— a generic safe-copy utility, tagged@internalin its own docstring, exported only sopackages/node/src/traces-defaults.tscould reach it across the package boundary.NOOP_SPAN— a sentinel, exported only sopackages/node/src/client.tscould reproducePostHogTraces.withSpan's activation rule with an identity check.inertSpan— legitimate, but paired with the sentinel above.Reaching for
assignUserAttributesalso madepackages/core/src/utils/depend onpackages/core/src/traces/, whiletraces/otlp.tsimportsbuildOtlpResourceAttributesback out ofutils/. Two directories that point at each other, andtraces/sanitize.tspulled into every browser bundle even though the browser has no traces host yet.None of this has shipped — the stack hasn't released — so it's still cheap to fix. Once it releases, the export surface is permanent.
Found by a pre-PR review pass on #4579, and stacked directly on it, since it is #4579's own code being cleaned up.
Changes
resolveTracesConfigmoves topackages/core/src/traces/config.ts, next toresolveMetricsConfiginpackages/core/src/metrics/config.ts, which is the precedent it should have followed: metrics resolves its config in core and is consumed by bothpackages/browserandpackages/node. It was already host-agnostic — the runtime-detected OS attributes arrive as ahostResourceAttributesparameter — so the move is a file move plus import rewiring. Its 39 tests move with it.This matters beyond tidiness: the defaults (
5000,512,2048,10000,3_600_000) are promised to every SDK by the@defaultdoc comments on the publicTracesConfig. With the resolver inpackages/node, the next traces host either copies the file or picks different numbers and silently falsifies those docs.assignUserAttributesmoves topackages/core/src/utils/json-utils.ts, whereUNSERIALIZABLE_VALUE, its only dependency, already lives.utils/no longer imports fromtraces/.runWithActiveSpan(contextManager, span, fn)replaces the duplicated activation rule.PostHogTraces.withSpanandPostHogBackendClient.withSpanhad the samespan === NOOP_SPAN ? … : …line, and nothing failed if only one of them were updated. Now the rule lives once andNOOP_SPANstays internal.Net public surface: three exports out, one in (
resolveTracesConfig, matchingresolveMetricsConfig), and one renamed in purpose (runWithActiveSpaninstead of the raw sentinel).Reviewer notes
packages/core1390 pass (64 suites, up 29 as the config tests arrive),packages/node988 pass (35 suites, down 29 as they leave). Same totals as the base branch, just relocated.array.js— byte-identical to the base.buildOtlpResourceAttributesstill callsassignUserAttributes, so the function stays in the bundle; what leaves is the dependency direction, not the bytes. Worth being precise about: breaking the cycle is what a future code-split traces host would need, not something that pays off today.packages/{node,react-native}/references/*-latest.json— they were public through the@posthog/corebarrel only, which is exactly the surface that has no doc-level guard on it.Release info Sub-libraries affected
Libraries affected
@posthog/coreis also touched; both packages are already bumped by the changesets on this stack.Checklist
The removed exports have never appeared in a published version, so nothing external can be depending on them.
If releasing new changes
pnpm changesetto generate a changeset fileDeliberately none — see the reviewer note above.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Built with Claude Code, directed by @turnipdabeets.
Came out of a pre-PR review loop on #4579: two independent fresh-context reviewers, one on correctness and spec conformance, one on public API surface and maintainability. This PR is the second reviewer's public-surface findings. The first reviewer's one material finding — the retry budget being spent by a caller-driven
flush()— is fixed in #4726 instead, where the retry semantics already live.Decisions worth flagging:
runWithActiveSpanover exporting a predicate. AnisActivatable(span)helper would have kept the sentinel internal too, but left the two-line rule duplicated in both hosts. Exporting the whole rule is the version that can't drift.traces-defaults.ts, so this move touches a file it has changes in. Merging this into feat(node): beforeSpanSend hook and per-span limits #4584 was simulated: git follows the rename, so all 45 lines feat(node): beforeSpanSend hook and per-span limits #4584 adds (resolveBeforeSpanSend, the per-span constants, the extraloggerparameter) land in the newcore/traces/config.tspath automatically. The whole conflict is one hunk of five lines — the import block, where both sides edited the same two lines.SendTracesBatchOutcomein the react-native reference is left alone. It surfaces there because it's the return type of a public method on the shared core client, which is feat(node): distributed tracing spans #4579's design rather than a leak this PR introduced.🤖 Generated with Claude Code
https://claude.ai/code/session_01L8eFZB35NExUZyq5hGgh43