Uh oh!
There was an error while loading. Please reload this page.
fix(cli): serve registers ObservabilityServicePlugin so observability:metrics resolves for every consumer (#9832) - #9951
Conversation
…ty:metrics resolves
…he serve.ts deployment facts Adds .changeset entry.
📓 Docs Drift CheckThis PR changes 1 package(s): 15 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 4 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 23 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 548f2c0eb5edc36c427ed12d29eca621c1f9a930 && git checkout 548f2c0eb5edc36c427ed12d29eca621c1f9a930
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 5989b0de9018a951a62a5cbd8d0f762832124a76 4632b8109d6798e4fa6374cd4c18af23dc0cf01f && git checkout -B drift-repro 5989b0de9018a951a62a5cbd8d0f762832124a76 && git merge --no-ff 4632b8109d6798e4fa6374cd4c18af23dc0cf01f
node scripts/docs-audit/affected-docs.mjs --json 5989b0de9018a951a62a5cbd8d0f762832124a76
|
huangyiirene
commented
Aug 19, 2026
PM review — REWORK (one required check red; the work itself is sound)
⛔ Two non-options
The complication I measured before prescribing anythingThe gate's suggested fix is an anchored entry in The replacement path is valid —
What I want backPick a route, measure it, and justify it — I am not prescribing the shape because I have not measured the consequences and you can:
⛔ Whichever you take, §1 must keep asserting emission, not registration — that is the whole point of the card and #9650's lesson. If route 2 would reduce §1 to the transport alone, say so explicitly rather than quietly narrowing the claim. Everything else in this PR reviewed clean — do not redo itI verified these independently on
Process note, not your fault
PR stays draft. Re-report when the check is green. Generated by Claude Code |
…ility pin reads the checkout, not dist
huangyiirene
commented
Aug 19, 2026
PM review, rework round 1 — ACCEPTThe red gate is fixed the right way, and the measurement I asked for was actually taken rather than asserted. Route 1, chosen on a measurement — and I re-took itYou took the "create I checked that myself on ⭐ The alias is proven LIVE, not merely gate-accepted — this is the best evidence in the roundAdding an alias and reporting "the gate is green now" would have proved only that the gate stopped complaining. Instead you mutated That single experiment establishes two independent things at once: the alias really redirects resolution to source, and the cache assertion is discriminating rather than vacuous. Most "I added an alias" changes demonstrate neither. The no- |
Uh oh!
There was an error while loading. Please reload this page.
Fixes#9832.
objectstack servenow registersObservabilityServicePlugin, soobservability:metricsactually resolves for every consumer that follows the canonical chain. Triage's Option 2, implemented as ruled — one registration, ahead of every consumer, letting the documented chain do the rest.Everything below is measured on
4632b8109d, branch point45862a53d.The change
packages/cli/src/commands/serve.ts, in the plugin-registration region only:buildServeObservability()hoisted to a single call site, ahead of the transport:1781ObservabilityServicePluginregistered with that block, gated on a backend being configured and on the config not already mounting its own:1782-1798const observability = await buildServeObservability();line inside it is gonePlus
packages/cli/src/commands/serve-observability-registration.test.ts(the pin),packages/cli/vitest.config.ts(see "Test resolution" below) and a changeset.Verified by BOOTING, not by reading the chain
This card's parent lesson was a seam correct in code and inert in the deployment, so the claim here is emission, measured on the real showcase app booted through the real
serve.tswithOS_OBS_EXPORTER=console, same probe sequence on both legs (discovery, sign-in, a REST data read, a presigned upload, the actualPUTupload).Ablation discipline: predictions written to a file before the baseline run; both legs rebuilt;
scripts/ablation-dist-preflight.mjsproved the artifact the boot consumes actually changed each time —marker absent from all 416 built fileson the baseline leg,marker present in 1 built fileon the restore leg.What each consumer actually received (boot log,
--log-level info):45862a53dObservabilityServicePluginregistered metrics=ConsoleMetricsRegistry errors=ConsoleErrorReporterCacheServicePluginregistered memory cache adapter (metrics=NoopMetricsRegistry)(metrics=ConsoleMetricsRegistry)StorageServicePluginregistered local storage adapter (swappable, metrics=NoopMetricsRegistry)(swappable, metrics=ConsoleMetricsRegistry)What actually starts emitting — the blast radius, measured rather than predicted:
storage_operations_totaladapter="local" op="put" result="ok"storage_operation_duration_mscache_lookups_total/cache_writes_totalhttp_requests_totalhttp_request_duration_msThe card says this blocks #9650 end-to-end because "the transport seam is inert, so
http_requests_totalkeeps reporting dispatcher routes only". Measured on the baseline, that is no longer true.#9835 landed after this card was filed (7ff3975070) and gaveIHttpServeranafterResponseseam;dispatcher-plugin.ts:731-734now feature-detects it and callsarmHttpRequestCounter()with the dispatcher's own explicitobservability.metrics, whichserve.tsalready passed. So a shipped deployment was already counting every inbound surface — through the dispatcher, not the transport plugin. The baseline leg emits exactly this:Auth and the REST data API — the two surfaces #9650 is about — are both there, on the parent commit. This branch produces the identical five lines. What it changes is who owns the arming: the transport plugin now arms in Phase 1, and the dispatcher's later call is answered
already-armedby the per-server latch. Net coverage identical, net count identical.So the families this PR genuinely turns on are storage and cache, not three. I am not re-litigating Option 2 on that basis — it is still the right shape and still what the plugin's JSDoc instructs — but the card's "three currently-silent metric families" framing overstates it by one, and #9650's remaining blocker is narrower than the card records.
Exactly ONE registry instance — the trap, pinned
buildServeObservability()now has exactly one call site in the file; the dispatcher reads the same binding. A second call would build a second exporter — withOS_OBS_EXPORTER=otlp, twoOtlpHttpMetricsRegistryinstances with two independent flush timers, double-exporting every series.Two independent proofs:
Ordering — the assumption held, and is now executable
Registering ahead of the transport broke no boot-ordering assumption.
ObservabilityServicePlugindeclares no dependencies,resolvePluginOrder()preserves registration order between plugins with no edge, and the booted plugin list confirms the resulting Phase-1 order:ObjectQL, SqlDriver, TelemetryDatasource, ObservabilityService, HonoServer, …— 48 plugins loaded, boot clean.The pin makes the requirement measured rather than commented: §2 re-boots the identical composition with only the registration moved after the consumers and asserts both families go to zero. Without it, §1 would stay green if the ordering requirement ever stopped being real.
Two deliberate branches, both pinned:
flags.server— cache and storage are registered on the--no-serverpath too. §4 pins that the registration precedes theflags.serverblock.http_requests_totalnever sees auth routes or the REST data API — the two highest-traffic inbound surfaces are outside the only HTTP counter the docs tell operators to monitor #9650 built in. §3 pins it.ObservabilityServicePluginis skipped, becauseregisterServicethrows on a duplicate name and that would turn a host wiring its own backends into a boot failure.On
cache_lookups_totalreading 0 on both legsNot a wiring failure — the adapter demonstrably holds
ConsoleMetricsRegistryafter this change. Thecacheservice simply has no consumer exercising it in a default boot: its only production consumer is better-auth's rate limiter (auth-plugin.ts:424, viacreateLazyCacheRateLimitStorage), andauth-plugin.ts:1373enables that only when an operator explicitly setsrate_limit_maxorrate_limit_window_secondsin settings. 25 rapid failed sign-ins produced no cache lookup. The pin covers the family with a real cache call on the resolved service, so the chain is pinned even though a default deployment does not walk it.Boundary respected
packages/runtime/src/dispatcher-plugin.ts(held by #9813) was read only, never edited, and needed no change: #9835's per-server latch already makes the double count structurally impossible, so #9833's concern did not become load-bearing here. Nopackages/specchange was needed.Out-of-scope finding, filed not folded in
#9954 — the
cache_*SEMCONV families report 0 in a default deployment because nothing unconditionally consumes thecacheservice (only better-auth's opt-in rate limiter and the SMS OTP budget do), so a zero there reads as "no cache activity" rather than "not exercised". Measured during this card's verification; labelledfinding, unassigned, nopm:queue. Not touched in this PR.Test resolution — why this PR adds
packages/cli/vitest.config.tscheck:test-source-aliaswent red on the first push: the pin imports the realCacheServicePlugin, andpackages/clihad no vitest config at all, so that import resolved throughexportstoservice-cache's dist. The pin was a verdict about build state, not about the source in the checkout — and the dangerous half of that is a test passing GREEN against a stale artifact with nothing in the output saying so.⛔ Widening
KNOWN_UNALIASED_TEST_IMPORTSwas not an option (the registry is shrink-only and the gate says so itself); it is left byte-untouched here. Route chosen: one anchored alias entry. Justification, measured before writing it:@objectstack/service-cachewas reachable from no other file inpackages/cli— which is exactly why it was absent from the ledger. So the entry re-resolves one file, the new test, and cannot move any existing suite.service-cache/src's own value imports reachable to the gate's walk. That is one package,@objectstack/observability, already in this package's ledger entry — so the required set is unchanged in both directions and the ledger neither grows nor shrinks.testblock. The 135 files here run on vitest defaults (globals: false); sibling configs in this repo setglobals: true, and copying that shape would have flipped it for every existing file in the package.The before/after reading, since creating the file changes the package's configuration and not just my import:
Test Files 1 failed / 134 passed (135),Tests 1467 passed / 3 skipped (1470)Test Files 135 passed (135),Tests 1470 passed (1470)The single before-failure was not caused by the absence of the config and is unrelated to this PR:
test/serve-mcp-capability-collision.e2e.test.tsfailed as a suite, not an assertion —TypeError: fetch failed … ECONNREFUSED 127.0.0.1:49796, a port-bind race in an e2e that boots a real server on a shared, loaded container. It was re-run alone on the identical tree with the config still absent and passed (Tests 3 passed (3)), and it passed again in the full after-run. File and test totals are identical across both runs — 135 / 1470. Nothing was re-resolved.Proof the alias is actually live, not merely accepted by the gate. With the config committed,
service-cache's source was mutated (LABELS = { adapter: 'memory' }→ a probe value) and not rebuilt —distwas confirmed to still carry the old value. The pin went RED on exactly the cache assertion (expected +0 to be 1); before the alias that same edit would have changed nothing. Source restored,service-cacherebuilt, restore proved withablation-dist-preflight --absent(marker absent from all 6 built files), pin back toTests 8 passed (8). That run also proves the cache assertion is discriminating rather than vacuous.check:test-source-aliasis still not named bynode scripts/pm/dispatch-gates.mjsfor this change set — re-derived at4632b8109d, it lists the same 10 path-matched families as before and this is not among them. Run it explicitly.Verification — gate union re-run at
4632b8109dDerived with
node scripts/pm/dispatch-gates.mjsfrom the actual change set (4 paths; 10 path-matched + 5 convention-triggered), pluspnpm lintandcheck:test-source-alias, neither of which that derivation names.pnpm --filter @objectstack/cli testTest Files 135 passed (135),Tests 1470 passed (1470)check:test-source-alias(the red one)Tests 8 passed (8)pnpm --filter @objectstack/cli typecheckpnpm lint(eslint . --no-inline-config)check:type-check-debt(--re-measure, built closure)surplus: nonecheck:type-check-coveragecheck:slot-lookup45862a5check:route-envelopecheck:query-options-erasurecheck:engine-double-contractcheck:where-matchercheck:cross-package-test-inputscheck:nul-bytescheck:changeset-gate-self-tests,check-empty-changeset,check-adr-0087-registration,check-changeset-no-major,check:objectui-changesetscripts/docs-audit/check-affected-docs.mjsClosure build before the ratchet, exactly as⚠️ The ratchet twice refused to run rather than measuring a different world — once for an unbuilt
lint.ymldoes it (turbo run build --filter='./packages/*' --filter='./packages/*/*'):Tasks: 70 successful, 70 total.service-knowledge, once because the ablation leftservice-cache's sources newer than itsdist. Both refusals mean NOT MEASURED, nevernot applicable; the number above is from the run after both were fixed.Generated by Claude Code