From 1f693d9ab573c6e7a9a9e3dcad09492afada7a41 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 09:01:04 +0000 Subject: [PATCH 1/3] fix(cli): register ObservabilityServicePlugin in serve so observability:metrics resolves --- packages/cli/src/commands/serve.ts | 72 +++++++++++++++++++++++++++--- 1 file changed, 66 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index 213930832c..934d502946 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -1743,6 +1743,63 @@ export default class Serve extends Command { // No translations and no explicit i18n plugin — this is fine, kernel fallback works } + // ── Observability backends (#9832) ────────────────────────────────── + // Auto-wire observability from env so production deployments can ship + // metrics / errors to OTLP backends (Grafana Cloud, Honeycomb, …) + // without app-level glue. Falls back to noop when OS_OBS_EXPORTER is + // unset / unknown — zero-cost when off, and never crashes boot if + // exporter init throws. + // + // Built EXACTLY ONCE per serve process, and this is the only call site. + // The dispatcher block further down reads THIS binding rather than + // calling again: a second `buildServeObservability()` would construct a + // SECOND exporter — for `OS_OBS_EXPORTER=otlp` two + // `OtlpHttpMetricsRegistry` instances with two independent flush timers, + // double-exporting every series to the same backend. + // + // Registered as a SERVICE here — ahead of the transport (immediately + // below), the dispatcher, and the capability providers (cache, storage) + // — because every consumer resolves the canonical chain (explicit + // option → `observability:metrics` service → nothing) inside its OWN + // `init()`, and the kernel runs all of Phase 1 in resolved order with + // registration order preserved between plugins that have no edge + // (`resolvePluginOrder`). Registering after a consumer is registering + // too late: that consumer has already fallen through to its no-op and + // will never look again. This is exactly what `ObservabilityServicePlugin`'s + // own JSDoc tells hosts to do ("Register this plugin BEFORE any plugin + // that wants to consume the services"). + // + // Deliberately NOT gated on `flags.server`: cache and storage are + // registered on the `--no-server` path too, and their metrics are just + // as real there. + // + // Registered ONLY when a backend is actually configured. An all-noop + // registration would still be truthy at every consumer's step 2 and + // would arm the transport's per-request middleware seam on deployments + // that export nothing — the per-request cost #9650 deliberately kept off + // an unconfigured deployment. + const observability = await buildServeObservability(); + // Skip if the config already mounts its own — `ctx.registerService` + // throws on a duplicate name, which would turn a host that wired its own + // backends into a boot failure. + const configHasObservabilityService = plugins.some( + (p: any) => p.name === 'com.objectstack.observability.service' + || p.constructor?.name === 'ObservabilityServicePlugin' + ); + if (observability && !configHasObservabilityService) { + try { + const { ObservabilityServicePlugin } = await import('@objectstack/runtime'); + await kernel.use(new ObservabilityServicePlugin({ + metrics: observability.metrics, + errors: observability.errorReporter, + })); + trackPlugin('ObservabilityService'); + } catch { + // @objectstack/runtime unavailable — every consumer keeps its no-op + // default, exactly as before this block existed. + } + } + // Add HTTP server plugin BEFORE config plugins so that the // http-server service is available for any plugin that needs it // during init/start (e.g. AuthPlugin). @@ -2556,12 +2613,15 @@ export default class Serve extends Command { // Register Dispatcher plugin (auth, graphql, analytics, packages, hub, storage, automation) try { const { createDispatcherPlugin } = await import('@objectstack/runtime'); - // Auto-wire observability from env so production deployments - // can ship metrics / errors to OTLP backends (Grafana Cloud, - // Honeycomb, etc.) without app-level glue. Falls back to noop - // when OS_OBS_EXPORTER is unset / unknown — zero-cost when - // off, and never crashes boot if exporter init throws. - const observability = await buildServeObservability(); + // `observability` is the ONE block built above (#9832), reused + // here rather than rebuilt: a second `buildServeObservability()` + // call would stand up a second exporter with its own flush timer. + // Passing it explicitly keeps the dispatcher on step 1 of the + // resolution chain, so its behaviour is unchanged by the service + // registration above — and because `armHttpRequestCounter` latches + // per server object (#9835), the transport having armed the same + // registry in Phase 1 makes this plugin's arming a no-op instead of + // a second observer on the same series. await kernel.use( createDispatcherPlugin({ scoping: { enableProjectScoping, projectResolution }, From d637847b79e54a504147eb1f5a9f57efa5dd4b37 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 09:27:27 +0000 Subject: [PATCH 2/3] test(cli): pin observability registration by emission, ordering and the serve.ts deployment facts Adds .changeset entry. --- .../serve-registers-observability-service.md | 11 + .../serve-observability-registration.test.ts | 284 ++++++++++++++++++ 2 files changed, 295 insertions(+) create mode 100644 .changeset/serve-registers-observability-service.md create mode 100644 packages/cli/src/commands/serve-observability-registration.test.ts diff --git a/.changeset/serve-registers-observability-service.md b/.changeset/serve-registers-observability-service.md new file mode 100644 index 0000000000..1b0bcc1a21 --- /dev/null +++ b/.changeset/serve-registers-observability-service.md @@ -0,0 +1,11 @@ +--- +"@objectstack/cli": patch +--- + +`objectstack serve` now registers `ObservabilityServicePlugin`, so the `observability:metrics` service actually resolves for every consumer that follows the canonical resolution chain. + +`serve.ts` built one metrics registry from `OS_OBS_EXPORTER` and threaded it into a single consumer (the dispatcher). Nothing in the repo registered the service itself, so the cache and storage adapters walked the documented chain — explicit option, then `observability:metrics`, then a no-op — and held a `NoopMetricsRegistry` in every shipped deployment, however `OS_OBS_EXPORTER` was set. + +The registry is now built once and registered as a service ahead of the transport, the dispatcher and the capability providers, because every consumer resolves the chain during its own `init()`. Measured on a booted showcase app with `OS_OBS_EXPORTER=console`: `storage_operations_total` and `storage_operation_duration_ms` now emit where they previously emitted nothing, and the cache adapter now holds the configured registry instead of the no-op. `http_requests_total` is unchanged — it was already armed transport-wide by the dispatcher through the `IHttpServer.afterResponse` seam, and the per-server latch keeps it at exactly one observer. + +Deployments that leave `OS_OBS_EXPORTER` unset or set to `noop` are unaffected: nothing is registered, and the transport still installs no per-request middleware. diff --git a/packages/cli/src/commands/serve-observability-registration.test.ts b/packages/cli/src/commands/serve-observability-registration.test.ts new file mode 100644 index 0000000000..83466ec3f5 --- /dev/null +++ b/packages/cli/src/commands/serve-observability-registration.test.ts @@ -0,0 +1,284 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `os serve` registers the host's observability backends, so + * `observability:metrics` actually resolves for every consumer (#9832). + * + * ## The defect + * + * `OBSERVABILITY_METRICS_SERVICE` (`observability:metrics`) has exactly one + * registrar in the repo — `ObservabilityServicePlugin.init()` — and, measured + * on the ref this card was filed against, **no host called it**. `serve.ts` + * built one registry from `OS_OBS_EXPORTER` and threaded it into a single + * consumer (the dispatcher), so every OTHER consumer walked the canonical + * chain `ObservabilityServicePlugin` documents — explicit option, then + * `observability:metrics`, then a no-op — and landed on the no-op. The cache + * and storage adapters held a `NoopMetricsRegistry` in every shipped + * `objectstack serve` deployment, however `OS_OBS_EXPORTER` was set. + * + * ## Why this file asserts EMISSION, not registration + * + * #9650's entire lesson was a seam that was correct in code and inert in the + * deployment: the transport counter landed, passed its own tests, and emitted + * nothing in a shipped `serve` because neither link of the resolution chain + * existed at the host. A test that asserted "the plugin is registered" would + * repeat exactly that mistake one layer up — registration is the mechanism, + * emission is the claim. So §1 boots a kernel and reads metric samples that a + * real HTTP request and a real cache lookup produced. + * + * §2 is the half that makes §1 mean something. The fix is an ORDERING fact — + * every consumer resolves the chain inside its own `init()`, and Phase 1 runs + * in `resolvePluginOrder()` order, which preserves registration order between + * plugins with no dependency edge. So registering the service after a consumer + * is registering too late, and §2 measures that by re-booting the identical + * composition with only the registration MOVED. Without §2, §1 would stay + * green if the ordering requirement silently stopped being real, and the + * comment in `serve.ts` explaining the placement would be the only thing left + * holding it. + * + * §3 pins the zero-cost guarantee that made the "register only when a backend + * is configured" branch necessary: an all-noop registration would still be + * truthy at every consumer's step 2, arming the transport's per-request + * middleware on deployments that export nothing. + * + * §4 pins the DEPLOYMENT facts on `serve.ts` itself — the half no booted + * composition can see, because a hand-composed kernel proves what the chain + * does, never what the shipped host asks of it. That gap IS #9650. + * + * ## Scope of the booted section, stated rather than implied + * + * The consumers exercised here are the transport (`HonoServerPlugin`) and the + * real `CacheServicePlugin`. `StorageServicePlugin` resolves through a + * `resolveMetrics(ctx, override)` helper that is character-identical to + * `service-cache`'s, and it was verified in the BOOTED CLI rather than + * modelled here: booting the showcase app through the real `serve.ts` with + * `OS_OBS_EXPORTER=console` logs + * + * CacheServicePlugin: registered memory cache adapter (metrics=ConsoleMetricsRegistry) + * StorageServicePlugin: registered local storage adapter (swappable, metrics=ConsoleMetricsRegistry) + * + * against `metrics=NoopMetricsRegistry` for both on the parent commit, and a + * real upload then emitted `storage_operations_total{adapter=local,op=put,result=ok}` + * where the parent commit emitted nothing. That evidence is recorded in the PR; + * standing a full storage stack up in a CLI unit suite would buy a third copy + * of the same chain, not a third fact. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { dirname, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { LiteKernel, type Plugin, type PluginContext } from '@objectstack/core'; +import { HonoServerPlugin } from '@objectstack/plugin-hono-server'; +import { ObservabilityServicePlugin } from '@objectstack/runtime'; +import { CacheServicePlugin } from '@objectstack/service-cache'; +import { InMemoryMetricsRegistry, SEMCONV } from '@objectstack/observability'; +import type { IHttpServer } from '@objectstack/spec/contracts'; +import type { ICacheService } from '@objectstack/spec/contracts'; + +const HERE = dirname(fileURLToPath(import.meta.url)); + +/** The host under test, read as source — same package, no build required. */ +const SERVE_SOURCE = readFileSync(resolve(HERE, 'serve.ts'), 'utf8'); + +const HTTP_REQUESTS_TOTAL = SEMCONV.httpRequestsTotal; +const CACHE_LOOKUPS_TOTAL = SEMCONV.cacheLookupsTotal; + +/** A `getRawApp()` mount, the shape `plugin-auth` uses (`auth-plugin.ts:1622`). */ +const PROBE_BASE = '/probe'; + +function rawMountPlugin(): Plugin { + return { + name: 'com.objectstack.test.raw-mount', + version: '1.0.0', + init: async () => {}, + start: async (ctx: PluginContext) => { + const httpServer = ctx.getService('http.server'); + const rawApp = (httpServer as unknown as { getRawApp(): any }).getRawApp(); + rawApp.all(`${PROBE_BASE}/*`, async (c: any) => c.json({ ok: true }, 200)); + }, + }; +} + +type Placement = 'before-consumers' | 'after-consumers' | 'absent'; + +/** + * Boot the consumer set in the order `serve.ts` composes it, varying ONLY + * where the observability registration sits. Everything else — the transport, + * the cache plugin, the raw mount — is identical across placements, so a + * difference in what gets emitted is attributable to the placement alone. + */ +async function bootWithPlacement(placement: Placement, metrics: InMemoryMetricsRegistry) { + const kernel = new LiteKernel(); + + if (placement === 'before-consumers') { + kernel.use(new ObservabilityServicePlugin({ metrics })); + } + + // The shipped order: transport first (serve.ts registers it before the + // config plugins so `http.server` exists for everything downstream), then + // the raw mount, then the capability providers. + kernel.use(new HonoServerPlugin({ port: 0, cors: false })); + kernel.use(rawMountPlugin()); + kernel.use(new CacheServicePlugin()); + + if (placement === 'after-consumers') { + kernel.use(new ObservabilityServicePlugin({ metrics })); + } + + await kernel.bootstrap(); + const httpServer = kernel.getService('http.server'); + const baseUrl = `http://127.0.0.1:${httpServer.getPort!()}`; + + // One real inbound request, and one real cache lookup — the two consumers + // whose emission is the claim. + await fetch(`${baseUrl}${PROBE_BASE}/anything`); + const cache = kernel.getService('cache'); + await cache.get('a-key-that-is-absent'); + + return kernel; +} + +let booted: LiteKernel | undefined; + +afterEach(async () => { + if (booted) { + await Promise.race([ + booted.shutdown(), + new Promise((r) => setTimeout(r, 10_000)), + ]); + booted = undefined; + } +}, 30_000); + +describe('#9832 §1 — registered BEFORE its consumers, both of them emit', () => { + it('emits http_requests_total from the transport and cache_lookups_total from the cache', async () => { + const metrics = new InMemoryMetricsRegistry(); + booted = await bootWithPlacement('before-consumers', metrics); + + // The transport counts a `getRawApp()` mount, labelled by the + // registered PATTERN — the surface the dispatcher's own proxy could + // never see, and the reason the counter lives at the transport. + expect(metrics.totalCounter(HTTP_REQUESTS_TOTAL, { route: `${PROBE_BASE}/*` })).toBe(1); + + // The cache adapter got the REAL registry rather than its no-op + // default. This is the family that was dark in every shipped + // deployment; `result: 'miss'` because the key was never set. + expect( + metrics.totalCounter(CACHE_LOOKUPS_TOTAL, { adapter: 'memory', result: 'miss' }), + ).toBe(1); + }, 60_000); + + it('counts the request exactly ONCE — one registry, one armed observer', async () => { + const metrics = new InMemoryMetricsRegistry(); + booted = await bootWithPlacement('before-consumers', metrics); + + // `armHttpRequestCounter` latches per server object (#9835), so a + // second layer offering the same registry re-arms nothing. If this + // ever reads 2, the host is building or arming a second emitter and + // every HTTP series in the deployment is inflated. + const all = metrics.samples.filter((s) => s.name === HTTP_REQUESTS_TOTAL); + expect(all.length).toBe(1); + }, 60_000); +}); + +describe('#9832 §2 — registered AFTER its consumers, the same composition emits nothing', () => { + /** + * The ordering claim, executable. Consumers resolve the chain in their own + * `init()`, so a service registered later is a service they will never see + * — they have already fallen through to the no-op and never look again. + * This is why `serve.ts` places the registration ahead of the transport + * rather than beside the dispatcher that first needed it. + */ + it('neither the transport nor the cache sees a service registered after them', async () => { + const metrics = new InMemoryMetricsRegistry(); + booted = await bootWithPlacement('after-consumers', metrics); + + expect(metrics.totalCounter(HTTP_REQUESTS_TOTAL)).toBe(0); + expect(metrics.totalCounter(CACHE_LOOKUPS_TOTAL)).toBe(0); + }, 60_000); +}); + +describe('#9832 §3 — no backend configured, nothing is registered and nothing is armed', () => { + /** + * The zero-cost guarantee #9650 built into the transport seam: no backend + * means no middleware installed, so an unconfigured deployment pays no + * per-request cost. `serve.ts` protects it by registering the service ONLY + * when `buildServeObservability()` returned a backend — an all-noop + * registration would be truthy at step 2 of every consumer's chain and + * would arm the seam on deployments that export nothing. + */ + it('emits nothing when no observability plugin is registered at all', async () => { + const metrics = new InMemoryMetricsRegistry(); + booted = await bootWithPlacement('absent', metrics); + + expect(metrics.samples.length).toBe(0); + }, 60_000); +}); + +describe('#9832 §4 — the deployment facts, pinned on serve.ts itself', () => { + /** + * ⭐ The half a booted composition cannot reach. §1–§3 prove what the chain + * DOES; only this section proves the shipped host asks for it. #9650 is + * precisely the failure of having the former without the latter. + */ + + /** Where `serve.ts` registers the service, and where it builds the transport. */ + const registrationIndex = SERVE_SOURCE.indexOf('new ObservabilityServicePlugin('); + const transportIndex = SERVE_SOURCE.indexOf('new HonoServerPlugin('); + + it('registers ObservabilityServicePlugin at all', () => { + expect(registrationIndex).toBeGreaterThan(-1); + }); + + it('registers it BEFORE the transport is constructed', () => { + // Both must be found for the comparison to mean anything — a missing + // needle is -1, which would satisfy a naive `<` against any real index. + expect(transportIndex).toBeGreaterThan(-1); + expect(registrationIndex).toBeLessThan(transportIndex); + }); + + /** + * ⚠️ The trap the card names explicitly. `buildServeObservability()` builds + * an exporter; calling it twice builds a SECOND one — with + * `OS_OBS_EXPORTER=otlp` that is two `OtlpHttpMetricsRegistry` instances + * with two independent flush timers, double-exporting every series to the + * same backend. The registration site and the dispatcher must therefore + * share ONE binding, which is only true while there is one call. + */ + it('calls buildServeObservability() exactly once', () => { + // Counted over CODE lines only. The first cut of this pin matched the + // raw source and read 3 — the call plus the two comments that explain + // why there must only be one. A pin that counts its own rationale is + // not measuring the thing it names. + const lines = SERVE_SOURCE.split('\n').map((l) => l.trim()); + const isProse = (t: string) => + t.startsWith('//') || t.startsWith('*') || t.startsWith('/*'); + const isDeclaration = (t: string) => + t.startsWith('function ') || t.startsWith('async function '); + + const mentions = lines.filter((t) => t.includes('buildServeObservability(')); + const declarations = mentions.filter((t) => !isProse(t) && isDeclaration(t)); + const callSites = mentions.filter((t) => !isProse(t) && !isDeclaration(t)); + + // Control: a rename that made both counts 0 would otherwise read as a + // clean single-call file rather than as this pin having gone blind. + expect(declarations.length).toBe(1); + expect(callSites.length).toBe(1); + }); + + /** + * Cache and storage are registered on the `--no-server` path too, and + * their metrics are as real there as anywhere. A registration that drifted + * inside the `if (flags.server)` block would leave that path dark again + * while every assertion above stayed green. + */ + it('does not gate the registration behind flags.server', () => { + const serverBlockIndex = SERVE_SOURCE.indexOf( + '// Register REST API and Dispatcher plugins', + ); + expect(serverBlockIndex).toBeGreaterThan(-1); + expect(registrationIndex).toBeLessThan(serverBlockIndex); + }); +}); From 4632b8109d6798e4fa6374cd4c18af23dc0cf01f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 10:12:11 +0000 Subject: [PATCH 3/3] test(cli): alias @objectstack/service-cache to source so the observability pin reads the checkout, not dist --- packages/cli/vitest.config.ts | 59 +++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 packages/cli/vitest.config.ts diff --git a/packages/cli/vitest.config.ts b/packages/cli/vitest.config.ts new file mode 100644 index 0000000000..417ccba38b --- /dev/null +++ b/packages/cli/vitest.config.ts @@ -0,0 +1,59 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +// This package had NO vitest config until #9832, and that is the fact this +// header exists to keep visible: adding one changes how every test file in +// `packages/cli` is configured, not just the file that needed it. So the +// config is deliberately minimal — a single anchored `resolve.alias` entry and +// no `test` block at all, because the 135 test files here run on vitest's +// defaults (`globals: false`, `environment: 'node'`) and a `test` block would +// silently re-specify them. Sibling configs in this repo do carry +// `test: { globals: true, … }`; copying that shape here would have flipped +// `globals` for every existing file in the package. +// +// ## Why the one entry +// +// `serve-observability-registration.test.ts` (#9832) boots the REAL +// `CacheServicePlugin` to prove that a consumer registered after +// `ObservabilityServicePlugin` actually resolves `observability:metrics` and +// EMITS through it. Without an alias that import resolves through the +// package's `exports` to `@objectstack/service-cache`'s **dist**, which makes +// the test a verdict about build state rather than about the source in the +// checkout — and the dangerous half of that is not a loud error but a test +// that passes GREEN against a stale artifact with nothing in the output +// saying so. `scripts/check-test-source-alias.mjs` carries the measured +// history (#7668, #7778, #7849); it named this import and is the gate that +// fails without the entry below. +// +// ⚠️ The registry in that script is SHRINK-ONLY, so widening +// `KNOWN_UNALIASED_TEST_IMPORTS['@objectstack/cli']` was never an option — and +// it is deliberately left untouched here. The other 28 entries in it are real +// and still unaliased; this change removes nothing from that ledger and adds +// nothing to it. Measured before writing this: `@objectstack/service-cache` +// was reachable from NO other file in `packages/cli` (which is why it was +// absent from the ledger in the first place), so this entry re-resolves +// exactly one file — the new test — and cannot move any existing suite. +// Measured again after: 135 files / 1470 tests, unchanged. +// +// Crossing into `service-cache/src` also makes ITS value imports reachable to +// the gate's walk. That is one package, `@objectstack/observability`, which is +// already in this package's ledger entry — so the required set is unchanged in +// both directions. +import { defineConfig } from 'vitest/config'; +import path from 'path'; + +export default defineConfig({ + resolve: { + // Array form with an ANCHORED pattern, per the trap the gate documents: + // the object form matches by PREFIX, so a bare key whose replacement is a + // FILE also swallows every subpath and resolves it to `…/index.ts/` + // (`ENOTDIR`, at run time, in a config that looks right). `service-cache` + // is imported bare and has no subpath exports, but the anchored form is + // what keeps that true when one is added. + alias: [ + { + find: /^@objectstack\/service-cache$/, + replacement: path.resolve(__dirname, '../services/service-cache/src/index.ts'), + }, + ], + }, +});