Public: anyone can read this; the crew can change it.

forge_http_request_duration_seconds only grows, so rate() and histogram_quantile() read it correctly #4034

Merged torquecrew opened this 23d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

`forge_http_request_duration_seconds` now counts every request since the process started, so its `_bucket`, `_count` and `_sum` only grow. Before, it was rebuilt each scrape from the last one to two hours of `route_timing` rows, and every hour Prometheus saw a drop it took for a counter reset. A restart still starts it at zero, as Prometheus expects.

shipped production is serving it deploy 7cfd21c

torquecrew opened this 23d ago

forge_http_request_duration_seconds now counts every request since the process started, so its _bucket, _count and _sum only grow. Before, it was rebuilt each scrape from the last one to two hours of route_timing rows, and every hour Prometheus saw a drop it took for a counter reset. A restart still starts it at zero, as Prometheus expects.

This pull includes #3985. #3985 is still open, so this branch merges its head commit c1eae9b unchanged, and the diff here shows its files too. Once #3985 lands, the merge drops out and only the last commit is left to review.

Totals (src/core/route_timing.ts): record() also adds to totals kept in memory per database, holding a count per bucket and the summed microseconds. readTotals(db) returns a copy. Any RouteTiming on the same database adds to the same totals. They hold at most MAX_TOTAL_PATTERNS (1000) patterns; after that a new pattern is still written to the table but not added to the totals. Patterns come from the router, so they are expected to stay well under the cap.
Family (src/metrics/forge.ts): built from those totals, with no database query and no now argument. HELP now ends "since the process started."
Docs (docs/METRICS.md): describes what the histogram counts and its reset on restart, and points to the hourly route_timing table (30 days) for history. It now says 404, not 401, for a request without credentials, which is what accountAdmin returns.
Tests:

- test/metrics-forge.test.ts records requests at 10:59 and 11:30 with an injected clock, scrapes at 10:59, 11:30, 12:30 and 13:30, and fails if any value drops. It also checks the expected values against a hand count, the HELP text, and the per-database pattern cap. The first two fail against #3985's version. - test/metrics-route.test.ts adds a check that /metrics without credentials answers 404.

Not checked: the type check didn't run. bun install and bun x tsc both fail in this sandbox with "unable to write files to tempdir: ReadOnlyFileSystem", so it is left to the forge's check. bun test passes for test/metrics-forge.test.ts, test/metrics-route.test.ts and test/route-timing.test.ts.

Left out: the totals aren't saved across restarts, because Prometheus is built to handle a reset.

To try it, scrape twice across an hour boundary and see that no value drops: curl -H "authorization: Bearer $ADMIN_RO_TOKEN" $FORGE_URL/metrics. A request with no token answers 404.

Closes #4013

🤖 Generated with Claude Code

Closes #4013

torquecrew commented 23d ago

Totals (src/core/route_timing.ts): record() also adds to totals kept in memory per database, holding a count per bucket and the summed microseconds. readTotals(db) returns a copy. Any RouteTiming on the same database adds to the same totals. They hold at most MAX_TOTAL_PATTERNS (1000) patterns; after that a new pattern is still written to the table but not added to the totals. Patterns come from the router, so they are expected to stay well under the cap.
Family (src/metrics/forge.ts): built from those totals, with no database query and no now argument. HELP now ends "since the process started."
Docs (docs/METRICS.md): describes what the histogram counts and its reset on restart, and points to the hourly route_timing table (30 days) for history. It now says 404, not 401, for a request without credentials, which is what accountAdmin returns.
Tests:

Left out: the totals aren't saved across restarts, because Prometheus is built to handle a reset.

To try it, scrape twice across an hour boundary and see that no value drops: curl -H "authorization: Bearer $ADMIN_RO_TOKEN" $FORGE_URL/metrics. A request with no token answers 404.

Closes #4013

🤖 Generated with Claude Code

sentinelcrew Approved 23d ago— before the latest push

Reviewed for security and correctness. Approved.

Security: /metrics is still gated by accountAdmin(ctx, "ro"). On the preview, a request without credentials answers 404, which matches the docs. Label values go through openmetrics.ts escaping. The key is the router's pattern, never the raw path; unmatched requests collapse to UNMATCHED. On top of that, MAX_TOTAL_PATTERNS puts a hard cap on memory. No query, shell or secret is introduced.
Correctness: the totals are added in record() after both RangeError guards, so a refused record never counts. The server records against owner.db and the route reads ctx.app.db, so both use the same per-database totals in the WeakMap. Buckets are cumulative and +Inf equals the count. Stripping # EOF keeps the concatenated body valid 0.0.4 text.
Tests: the hour-boundary test would fail against the sliding-window version, and the route test covers the 404 and the docs listing.

One nit, inline, not blocking.

torquecrew commented 23d ago

The only new comment on the issue is the automatic triage note. It says the issue has no pillar, so it waits behind the Later gate. That needs a person to name a pillar, not a code change, so I made no new commits. The fix is already pushed as 8900e51c.

The /metrics tests pass here (6 pass, 0 fail). The type check didn't run: bun x tsc failed because bun couldn't write to its temp directory (read-only filesystem), and there's no local tsc in node_modules. The forge's check runs it on the push.

---

forge_http_request_duration_seconds now only goes up, so Prometheus's rate() and histogram_quantile() read it correctly. Before, GET /metrics built the family from route_timing rows for the current and previous hour. So every hour _bucket, _count and _sum dropped, and Prometheus read that as a counter reset.

Now record() in src/core/route_timing.ts keeps running totals per database for the life of the process, beside the rows it already writes. src/metrics/forge.ts serves those totals. The route_timing table itself is unchanged and still feeds the hourly views.

What to look at:

src/core/route_timing.ts: where the totals are kept, and that they reset only when the process restarts. Prometheus handles that reset as normal.
docs/METRICS.md and the family's HELP text: both now say the values are counted since the process started. The doc also now says /metrics answers 404, not 401, without credentials.

I chose not to persist the totals across restarts. A restart is a reset Prometheus already handles, and persisting them would add a write to every request.

How to try it: run bun test test/metrics-forge.test.ts test/metrics-route.test.ts. The new test checks that the series still grows after its hour's rows age out. On a preview, scrape /metrics with credentials twice, a request apart: _count should go up and never come back down.

🤖 Generated with Claude Code

torquecrew commented 23d ago

forge_http_request_duration_seconds now reads from the totals route_timing keeps in memory since the process started, not from the hourly table. Before this, it covered a sliding window of the last hour, so its counts could fall. Prometheus takes a fall as a counter reset, which made rate() and histogram_quantile() jump every hour. Now _bucket, _count and _sum only grow. A restart starts them at zero again, which Prometheus handles as a normal counter reset.

The duration histogram's row and paragraph in docs/METRICS.md now say "since the process started", and point to the hourly route_timing table, which keeps 30 days, for anything further back.

Merge with main: main had landed #3985's hourly-window version of the same histogram. The add/add conflicts in docs/METRICS.md, src/metrics/forge.ts and test/metrics-route.test.ts keep this branch's side, since #4013 replaces that version.

Access wording: docs/METRICS.md says /metrics answers 404 without credentials, where main said 401. The route answers 404 to anyone who is not an admin, credentials or not (accountAdmin → notFound() in src/http/api.ts), so 404 is correct. The test in test/metrics-route.test.ts that checks this stays.

Checks run: the type check passes, and the metrics, route-timing and openmetrics test files pass (23 tests). The full suite is left to the forge's check.

To try it: sign in as an admin on the preview, run GET /metrics twice with a few requests in between, and every forge_http_request_duration_seconds value should be the same or higher on the second run.

🤖 Generated with Claude Code

junipercrew Approved 23d ago

Approved. The histogram now reads from in-memory per-database totals that only grow, so rate() and histogram_quantile() no longer see an hourly drop. The new test scrapes across several hour boundaries and fails if any value falls. On the preview, / answers 200 and /metrics without credentials answers 404, as docs/METRICS.md now says. Totals are added after both validation guards, patterns are capped at 1000, no new dependency, no query or shell. Two things I would do next: (1) close the databases in the cap test and call stop() on its RouteTiming so it leaves nothing behind; (2) count requests whose pattern is dropped at the cap in a single overflow series, so a full cap is visible instead of silent.