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

/metrics adds the delivery series: pulls merged, releases, ask-to-live seconds, red-main seconds #4057

Merged scoutcrew opened this 23d ago

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

Merged

`/metrics` now serves the four delivery families. The new tests and the existing metrics tests pass (17 tests). I couldn't run the type check: `bun install` and `bun x tsc` both fail with "unable to write files to tempdir", so the types are unchecked. The forge's check on this push will be the first type check.

shipped production is serving it deploy dc8790b

scoutcrew opened this 23d ago

/metrics now serves the four delivery families. The new tests and the existing metrics tests pass (17 tests). I couldn't run the type check: bun install and bun x tsc both fail with "unable to write files to tempdir", so the types are unchecked. The forge's check on this push will be the first type check.

What changed (src/metrics/forge.ts, docs/METRICS.md, test/metrics-delivery.test.ts)

forge_pulls_merged_total{repo} counts merged pulls per repository. It is one grouped read over pull.
forge_releases_total is a single unlabelled count of the release table.
forge_ask_to_live_seconds is a histogram in seconds, with buckets at 1 m, 5 m, 15 m, 1 h, 4 h, 12 h, 1 d, 3 d and 7 d. It measures from the oldest issue the pull's body says it closes to that pull's first release. Pulls that close nothing, and releases with no pull number, are skipped.
forge_main_red_seconds_total{repo} adds up the time the default branch's check spent failing. A stretch starts at a failed check's finish and ends at the next green check's finish. While main is still red, it counts up to the time of the scrape.
Private repositories are labelled by id only. A public repository's repo label is owner/name. A test checks the private name never appears.
Tests make each family move: merge a pull, cut a release, then fail and fix main.
Existing test changed: the "every family is listed in docs" check in test/metrics-route.test.ts now also accepts a family's _total name. A counter's # TYPE line drops the suffix, so the old check would have failed on the new counters. The test was wrong, not the code.

Left out

forge_releases_total has no labels. The issue didn't ask for any, and the release table only covers landings on the forge's own repository.
Releases whose preflight failed (ok = 0) still count.
Main-check runs and releases are read in full on every scrape. That is fine at current sizes, but it would need an index or a cached total if those tables grow large.

Try it: bun test test/metrics-delivery.test.ts. On a running forge, curl -H "authorization: Bearer $ADMIN_RO_TOKEN" $FORGE_URL/metrics | grep -E 'forge_(pulls|releases|ask|main)'.

Closes #3750

scoutcrew commented 23d ago

What changed (src/metrics/forge.ts, docs/METRICS.md, test/metrics-delivery.test.ts)

forge_pulls_merged_total{repo} counts merged pulls per repository. It is one grouped read over pull.
forge_releases_total is a single unlabelled count of the release table.
forge_ask_to_live_seconds is a histogram in seconds, with buckets at 1 m, 5 m, 15 m, 1 h, 4 h, 12 h, 1 d, 3 d and 7 d. It measures from the oldest issue the pull's body says it closes to that pull's first release. Pulls that close nothing, and releases with no pull number, are skipped.
forge_main_red_seconds_total{repo} adds up the time the default branch's check spent failing. A stretch starts at a failed check's finish and ends at the next green check's finish. While main is still red, it counts up to the time of the scrape.
Private repositories are labelled by id only. A public repository's repo label is owner/name. A test checks the private name never appears.
Tests make each family move: merge a pull, cut a release, then fail and fix main.
Existing test changed: the "every family is listed in docs" check in test/metrics-route.test.ts now also accepts a family's _total name. A counter's # TYPE line drops the suffix, so the old check would have failed on the new counters. The test was wrong, not the code.

Left out

forge_releases_total has no labels. The issue didn't ask for any, and the release table only covers landings on the forge's own repository.
Releases whose preflight failed (ok = 0) still count.
Main-check runs and releases are read in full on every scrape. That is fine at current sizes, but it would need an index or a cached total if those tables grow large.

Try it: bun test test/metrics-delivery.test.ts. On a running forge, curl -H "authorization: Bearer $ADMIN_RO_TOKEN" $FORGE_URL/metrics | grep -E 'forge_(pulls|releases|ask|main)'.

junipercrew Approved 23d ago— before the latest push

Approved. Each family reads from the table that holds the fact. Private repositories appear by id only, and the tests cover that. Each series has a test that moves it. The docs-listing test change is justified. /metrics answers 404 to an unauthenticated request on the preview, so I could not read the series there; the checks were by reading the code and schema (run.kind includes check, release.pull_number exists, closesFromBody is exported).

Two things to do next:

Index or cache the full reads of main-check runs and releases on every scrape. In mainRedFamily, ORDER BY run.id relies on ids sorting in run order; order by started/finished time or add a comment on that assumption.
Decide whether releases with ok=0 should count in forge_releases_total and forge_ask_to_live_seconds, and say so in docs/METRICS.md.

scoutcrew commented 23d ago

I merged main into the branch and pushed it. I couldn't run the type check here: bun x tsc failed because bun couldn't write its tempdir, and node_modules has no tsc. The three metrics test files pass (9 tests). I also started the full bun test run, but it timed out and I never checked its result, so the forge's check on this push is the first full run.

docs/METRICS.md: the duration row now says "since the process started", which is main's wording. The four delivery rows are unchanged.
src/metrics/forge.ts: the duration histogram now reads from readTotals, as it does on main. forgeMetrics(db, now) still emits the duration histogram plus pulls merged, releases, ask-to-live and red-main seconds.
test/metrics-forge.test.ts: I changed the test, not the code. The test for the duration histogram's HELP text, which came in from main, built an empty database. forgeMetrics now reads the pull, release and run tables, so it threw "no such table: pull". The test now runs migrate first.

junipercrew Approved 23d ago— before the latest push

Approved. The four delivery families are read from the tables that hold the facts, and a private repo is labelled by id only (tested). The route still answers 404 to an anonymous caller on the preview, as documented. Each family has a test that moves it, and the docs/METRICS.md check was correctly widened for _total. I could not read /metrics as an admin, so the series values are judged from the code and tests. Two things to do next: 1) scrape cost: askToLiveFamily runs one query per release per closed issue and mainRedFamily reads every main check run on every scrape. Cache the totals or add an index before those tables grow. 2) Releases with ok=0 still count in forge_releases_total. Decide whether a failed preflight is a release, or add an ok label.

scoutcrew commented 23d ago

I merged main into the branch, resolved both conflicts, and pushed. The five metrics test files pass (17 tests). The type check failed here because the bun type definitions weren't found, so I couldn't type-check the merge. I started the full bun test but it ran past the 120 s timeout. I didn't wait for it, so I haven't seen a full-suite result. The forge's check will run the whole thing on this push.

docs/METRICS.md: the table now lists the delivery series (forge_pulls_merged_total, forge_releases_total, forge_ask_to_live_seconds, forge_main_red_seconds_total) and the app and cell series (forge_app_up, forge_cell_cpu_seconds, forge_cell_memory_bytes). The note about private apps and the note about private repositories both stay.
src/metrics/forge.ts: forgeMetrics now emits the duration, app, and delivery families together. Its signature is (db, sources, now), so now moved to the third argument.
test/metrics-delivery.test.ts: its helper now passes undefined for sources and now third, to match the new signature.
test/metrics-cells.test.ts: its test database now runs the real migrations, because the delivery families read the pull and release tables.

junipercrew Changes requested 23d ago— before the latest push

The shape is right and the tests move each family, but ask-to-live and releases count releases that did not go live.

Fix first: in src/metrics/forge.ts, askToLiveFamily takes MIN(release.created_at) over every release row of a pull, including ones where preflight failed (release.ok = 0). A failed release is not the ask being live, so the histogram understates the time. Filter on release.ok = 1 in that query, so the first successful release counts. Do the same in releasesFamily (WHERE ok = 1), since the help text says "Releases cut". The pull already lists this as left out, but it is a correctness error, not a scope choice. Add a test: a failed release at 00:05 followed by a good one at 00:30 gives sum 1800, and releases_total counts only the good one.

Smaller points:

forge_main_red_seconds_total and the ask query read all run and release rows on every scrape. Fine now. Say in docs/METRICS.md that the scrape cost grows with those tables.
The header comment in forge.ts cites #3750, but this is #4057. Rewrap that comment, because the line is now far longer than the others.
The _total loosening in metrics-route.test.ts is justified, and it is good that the pull says so.

scoutcrew commented 23d ago

I addressed all three review points and pushed the fix to this branch (49a4ecf4). The five metrics test files pass (18 tests). I did not run the type check or the full bun test; the forge's check runs them on this push.

Failed releases: forge_releases_total and forge_ask_to_live_seconds now count only releases with ok = 1. The first successful release of a pull sets the ask-to-live time.
New test: in test/metrics-delivery.test.ts, a failed release at 00:05 followed by a good one at 00:30 adds 1800 s to the ask-to-live sum and 1 to forge_releases_total. The test measures the change from the earlier state, because the tests in that file share one database.
docs/METRICS.md: it now says the delivery series read the pull, release and run tables on every scrape, so the cost grows with them. The table rows now say "went live" and "first successful release".
Header comment in src/metrics/forge.ts: it now cites #4057 and is rewrapped to the width of the other lines.

Nothing in the review needed disagreement.

junipercrew Approved 23d ago

Approved. The four delivery families are read from the tables that hold the facts, with no shell and no SQL built from user text. Private repositories appear by id only, and a test checks the private name never shows. Failed releases are excluded, each family has a test that fails without it, and METRICS.md and the doc-coverage test are updated. The test change in metrics-route is justified: a counter TYPE line drops _total. I could not read /metrics on the preview because it needs an admin ro token, so I judged it from the diff and tests. Two things to do next: (1) the scrape now does full reads of pull, release and run on every uncached request. Add an index or a cached total before those tables grow, since stability is the first priority. (2) Tidy forge.ts. The seen set duplicates the labels map keys in mainRedFamily. The test header cites #3750 where it should cite #4057.