Skip to content

fix(observability): retire http_request_errors_total (ADR-0049 enforce-or-remove) - #10226

Merged
os-elon merged 2 commits into
mainfrom
claude/issue-9834-retire-http-request-errors-total
Aug 21, 2026
Merged

fix(observability): retire http_request_errors_total (ADR-0049 enforce-or-remove)#10226
os-elon merged 2 commits into
mainfrom
claude/issue-9834-retire-http-request-errors-total

Conversation

@os-elon

Copy link
Copy Markdown
Collaborator

Part of #9834 — this is the error-counter half of that card; the duration half shipped in #10004.

What the maintainer ruled

Option C: retire http_request_errors_total under ADR-0049 enforce-or-remove
(2026-08-20, comment 5354029742, verbatim 「4 张 同意」). Remove the counter and
its emission; point the docs' 5xx-rate guidance at
http_requests_total{status=~"5.."}, which has covered every inbound surface
since #9650 / #10004; register the ADR-0087 conversion for the removed published
name; announce the removal loudly in the changeset for any external dashboard.

Escape clause — checked, not tripped

The ruling carries a binding escape clause: if implementation or any operator
report surfaces real deployed alerting keyed on this name, STOP — the ruling
flips to D (declare-and-rename).

Nothing surfaced. Swept for consumers of both the wire string and the symbol
across objectstack and the sibling objectui checkout, plus a scan for any
dashboard / alert-rule / Prometheus config in-tree (there are none). Every hit
was the emitter, its own tests, prose docs, or historical CHANGELOG.md rows.
The known-and-accepted limit is unchanged and worth restating: this repo is
structurally blind to a Grafana panel or alert rule in a deployed environment.
That gap was stated when C was presented and is what the loud changeset and the
ledger entry exist for — it is not new information, and it is not an operator
report.

The premise, re-measured on origin/main2d3860df9

Re-derived by symbol rather than by line number, and it stands:

sitewhat it was
packages/observability/src/semconv.tsthe SEMCONV.httpRequestErrorsTotal declaration and its RUNTIME_METRICS re-export — two sites, not one
packages/runtime/src/observability/instrument.tsthe only emission: metrics.counter(...) inside instrumentRouteHandler's catch
instrument.test.ts · metrics.test.tsfour assertion sites + one name pin
dispatcher-plugin.ts · docs/OBSERVABILITY.md · content/docs/deployment/production-readiness.mdx · packages/runtime/README.mdprose

⚠️ One site the dispatch brief's list missed: semconv.ts has two, the
SEMCONV member (:45) and the RUNTIME_METRICS alias (:120). The alias is
the name @objectstack/runtime actually re-publishes, so leaving it would have
kept the string exported from the package the docs point hosts at.

What changed

Removed — the SEMCONV declaration, the RUNTIME_METRICS alias member, and
the metrics.counter call in the catch block.

Deliberately NOT removed, and pinned so: the catch still records
err.statusCode ?? 500 (which reaches http_requests_total{status} in the
finally), still calls errorReporter.captureException for 5xx, still
re-throws; request-id echo and the res.__obsRecordedError side channel are
untouched. The retirement removes a series, not the error path.

Tests — triaged one by one, not batch-edited. Four assertion sites asserted
the counter incremented. Each was replaced by the assertion that survives at
that site, plus a tombstone: a retiredErrorCounterSamples() probe asserting no
sample is ever written under the retired name. Two negative pins were added — one
on the emission path (instrument.test.ts) and one on the published constant
(metrics.test.ts, plus a new packages/observability/src/__tests__/semconv.test.ts
guarding SEMCONV itself, in both directions: the member key and the wire
string, so a re-add under a different member name is caught too).

One new case records the half of the old population a status-class replacement
would have dropped: a thrown 4xx was counted by the retired counter and is
counted by nothing now.

ADR-0087 conversion — registered as a SemanticMigration
(http-request-errors-total-retired) at protocol 17, not a MetadataConversion
and not a retiredKey() tombstone: a metric name is a response surface, so
no stack, example or template carries it and there is no authored source for a
D2 chain to rewrite. Precedents in the same table: runtime-httpserver-wrapper-retired,
enhanced-api-error-field-errors-renamed. Registered at 17 rather than 18
because the removal takes effect on the 17.x line, matching ups-delegated-from-column-retired
(#9998, landed 2026-08-19). spec-changes.json and docs/protocol-upgrade-guide.md
are regenerated projections. ⛔ No governed ADR file is touched — the ADR-0087
surface here is entirely the code registries.

Verification

Everything below is on head 54809697d. Exit codes were captured by
redirecting to a file first (cmd > log 2>&1; rc=$?), never through a pipe.

Tests — full package suites, not a targeted subset:

runresult
pnpm --filter @objectstack/observability exec vitest run --maxWorkers=2Test Files 7 passed (7) · Tests 85 passed (85) (6 files / 83 before — +1 file, +2 cases: the new semconv.test.ts)
pnpm --filter @objectstack/runtime exec vitest run --maxWorkers=2Test Files 177 passed (177) · Tests 2651 passed (2651)
pnpm --filter @objectstack/runtime typecheck · pnpm --filter @objectstack/spec typecheckRC 0 · RC 0

⚠️@objectstack/observability declares no typecheck script (build + test only) — a
pre-existing ledgered state, not a gap opened here; check:type-check-coverage accounts
for it.

Reverse verification — three legs, each mutated and restored, each stating its resolution path.
packages/runtime's vitest config aliases core/rest/spec/platform-objects/… but
not@objectstack/observability, so anything reading RUNTIME_METRICS resolves through
exportsdist/. Legs 2 and 3 therefore rebuild and are proven to have reached the built
artifact with scripts/ablation-dist-preflight.mjs (present on the mutation, --absent on
the restore); leg 1 mutates runtime source that vitest reads directly, and says so rather
than claiming a rebuild it did not need.

legmutationdist proofmutatedrestored
1 — the emissionre-add metrics.counter(...) in instrument.ts's catch (runtime src, source-resolved)n/a, statedinstrument.test.ts5 failed | 17 passed (22)22 passed (22)
2 — the SEMCONV declarationre-add the member, rebuild observabilitypreflight PRESENT RC 0 → ABSENT RC 0semconv.test.ts1 failed | 1 passed (2)2 passed (2)
3 — the RUNTIME_METRICS aliasre-add the alias member, rebuild observabilitypreflight PRESENT RC 0 → ABSENT RC 0metrics.test.ts1 failed | 9 passed (10)10 passed (10)

⭐ Leg 3 exists because leg 2 did not turn metrics.test.ts red: mutating SEMCONV
alone leaves the RUNTIME_METRICS alias without the member, so that pin correctly stayed
green. Reporting leg 2 as proof of all three pins would have been a false green about a
pin that had never been shown able to fail — so the alias got its own leg.

Gates — every exit code captured by redirecting to a file first (cmd > log 2>&1; rc=$?),
never through a pipe. Re-derived with node scripts/pm/dispatch-gates.mjs (no paths — the
script derives its own change set; 14 paths, 115 families across 26 workflow files), which
named 10 families the dispatch brief could not: check:changeset-gate-self-tests,
check:doc-formula-expressions, check:merge-driver, check:objectui-changeset,
check:spec-parsed-alias, check:type-source-resolution, check-adr-0087-registration,
check-changeset-no-major, check-empty-changeset, check-dev-prereqs.

All green, RC 0: pnpm lint (repo-wide) · check:nul-bytes · check:doc-anchors ·
check:docs-redirects · check:docs-audit-scope · check:published-readme-links ·
check:role-word · check:route-envelope · check:slot-lookup ·
check:cross-package-test-inputs · check:test-source-alias · check:type-source-resolution ·
check:spec-parsed-alias · check:merge-driver · check:objectui-changeset ·
check:changeset-gate-self-tests · check:engine-double-contract · check:where-matcher ·
check:query-options-erasure · check:type-check-coverage · check:type-check-debt ·
check-dev-prereqs · spec check:liveness / check:empty-state / check:strictness-ledger /
check:variant-docs / check:migration-registry / check:spec-changes / check:upgrade-guide ·
scripts/check-adr-0087-registration.mjs · scripts/check-changeset-no-major.mjs ·
scripts/check-empty-changeset.mjs · scripts/check-cross-package-test-inputs.mjs ·
scripts/docs-audit/check-affected-docs.mjs.

No ratchet moved, no baseline raised, no test skipped. The two that would have said so
print their own verdicts:

  • check:type-check-debt"check-type-check-coverage --re-measure: OK — 33 ledger entr(ies)
    re-measured in 364.6s, 1924 raw tsc error(s) total, none above its recorded number. surplus:
    none — every entry sits exactly at its measurement, so any new error is red."
    Measured on a
    fully built workspace (turbo run build --filter='!@objectstack/docs' RC 0, then
    check-dev-prereqs RC 0 — it had refused before the build, which is NOT MEASURED, not
    "not applicable").
  • check:where-matcher / check:query-options-erasure"baseline key set verified against
    2d3860d: no files added."

Verify-lock discipline: every build, test and heavy gate ran through
scripts/pm/os-verify-lock.sh; no hand-rolled flock. Contention was heavy (four devs) — six
acquisitions and seven VERDICT queue-timeout (exit 99) returns, longest successful wait
6m05s, longest hold 6m06s (check:type-check-debt --re-measure, 364.6s of it inside the gate).

⛔ Not enqueueable

The card carries needs:contract-review (Clause-② yes — published-surface
removal). Draft only: no ready-flip, no auto-merge, and the label is not
mine to clear. A different seat clears contract review.

Generated by Claude Code


Generated by Claude Code

…e-or-remove)
The counter was DECLARED in SEMCONV as part of a stable namespace hosts wire
alerts against, but emitted only from the runtime dispatcher's per-route
wrapper, on a THROWN handler — so it never saw auth's getRawApp() mount, the
REST data API, or any error answered politely through errorResponseBase, while
counting thrown 4xx as errors. Its two siblings moved to the transport seam;
this one could not, because HttpResponseObservation carries no throw signal at
all, so every transport-side shape would have counted a different population.
Maintainer ruling 2026-08-20 (option C of four presented): remove the counter
and its emission, point the docs' 5xx-rate guidance at
http_requests_total{status=~"5.."} — which the transport emits for every
inbound surface — and announce the removal loudly for external dashboards.
ADR-0087 semantic ledger entry http-request-errors-total-retired registered at
protocol 17; spec-changes.json and the upgrade guide regenerated.
Part of #9834
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019bmVFqoQPq63zhKrxdYG1r
… for strict typing
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019bmVFqoQPq63zhKrxdYG1r
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 3 package(s): @objectstack/observability, @objectstack/runtime, @objectstack/spec, touching 8 documentable anchor(s).

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/deployment/production-readiness.mdx(via http_request_errors_total (literal))
What this run could not see
  • 2 changed file(s) yielded no anchor (packages/runtime/README.md, packages/spec/spec-changes.json) — pages documenting those are invisible to this run
  • 4 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 45 of 221 client-bound route-ledger rows — the other 176 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

Coarse fallback — 121 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 04096f17e14dccd9c8c9f1e5e924e1fc4dd99398packageMentionDocs.

Which tree this was computed on

This run read content/docs from b511a223eecf1e8fab3e37b435366ed52bed7ef2 — the merge of head 54809697dfe842aa9d9370e6477851ab7c5f59e2 into base 04096f17e14dccd9c8c9f1e5e924e1fc4dd99398, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin b511a223eecf1e8fab3e37b435366ed52bed7ef2 && git checkout b511a223eecf1e8fab3e37b435366ed52bed7ef2
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 04096f17e14dccd9c8c9f1e5e924e1fc4dd99398 54809697dfe842aa9d9370e6477851ab7c5f59e2 && git checkout -B drift-repro 04096f17e14dccd9c8c9f1e5e924e1fc4dd99398 && git merge --no-ff 54809697dfe842aa9d9370e6477851ab7c5f59e2
node scripts/docs-audit/affected-docs.mjs --json 04096f17e14dccd9c8c9f1e5e924e1fc4dd99398

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 04096f17e14dccd9c8c9f1e5e924e1fc4dd99398 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@os-elon@claude