fix(metrics): show the latest complete data point with --latest

[pjcdawkins]

The metrics API returns 1-minute points before every container has reported, so --latest often lists only some containers (#45).

Sampling live projects every 15s showed new points take ~60-80s to include all containers, and the previous completed minute is sometimes still missing some.

--latest now picks the newest point that has as many services as any point in the response, instead of the newest point with any services. The default grain is unchanged.

This supersedes #49, which used a 5-minute grain. The API aligns 5-minute buckets to clock boundaries and returns the in-progress bucket, so that bucket has the same gap for about a minute after each boundary, and it turns "latest" into a 5-minute average.

Also fixes the invalid --interval error message, which referred to --range (from #49).

Adds an integration test with a mocked observability endpoint.

Closes #45

🤖 Generated with Claude Code

[upsun-dispatch]

Warning

Changes suggested — 🟡 1 warning · 🔵 1 minor point

🔍 Full review · 2 files reviewed

Verification

• max([0, ...]) keeps the filter safe when data is empty or every point lacks a 'services' key: maxServices is 0 and the list is left untouched.
• The $maxServices > 0 guard means the in-progress point (no 'services' key, count 0) is never selected by --latest.
• The reversed iteration picks the newest point among ties, matching the test's expectation of the 19:02 point over 19:01.
• The --interval error message now names --interval, and it is on the empty($interval) branch reached by an unparseable value.
• The new integration test's fixed epoch timestamps render deterministically because testEnv() sets TZ=UTC.

The diff adds integration-tests/metrics_test.go, which exercises --latest against a mocked observability endpoint and is run by the integration-test job in .github/workflows/ci.yml (make integration-test); the PHP change is covered by the legacy-php job's php-cs-fixer/phpstan lint only. No test covers the case where no point contains the full service set, or where a service disappears mid-range.

Review details

• Commit: 7b5165f
• Model: claude-opus-5

---

Review 1 of 10 for this pull request · View the full run

[upsun-dispatch]

Note

Reviewed — No new issues found · 2 still open

🔁 Incremental · 2 files reviewed

Outstanding from earlier reviews:

• 🟡 #4086856131 — legacy/src/Command/Metrics/MetricsCommandBase.php:192: --latest can show hours-old metrics as if current after a service is removed. — Bounded but not closed: the candidate window is 3 points regardless of grain, so after a service is permanently removed --latest still returns a point up to two intervals old — ~2 minutes at grain 60, but hours with e.g. --range 8h --interval 1h, with nothing in the output flagging it as not the newest point.
• 🔵 #4086856146 — legacy/src/Command/Metrics/MetricsCommandBase.php:100: Help text overstates the guarantee, misleading users reading incomplete output. — The overstatement is gone, but 'skipping up to two newer points that are missing services' still misdescribes the code: trailing points with no 'services' key are dropped before the 3-point slice (so more than two newer points can be skipped), and a newer point is also skipped when it merely has fewer services than an older one rather than being incomplete.

Verification

• The new selection loop only inspects points that passed the !empty('services') filter, so \count($item['services']) is never called on the in-progress point that has no 'services' key.
• The >= tie-break makes the newest point win when several of the last three candidates have the same service count, matching the test's expected 19:02:00 row.
• array_values() before array_slice(-3) keeps the slice positional, so gaps left by array_filter's preserved keys cannot shift which points are considered.
• When no point has services, $latest stays null and $items['data'] is left untouched, which is the same behaviour as the previous max-based code.
• The integration test guards every read and write of the shared data slice with mu, so the handler goroutine and the test goroutine do not race.

integration-tests/metrics_test.go covers both --latest scenarios against a mocked observability endpoint and runs in the integration-test CI job (.github/workflows/ci.yml, make integration-test, which builds the CLI first); there is no PHP unit test for MetricsCommandBase, so the selection loop is exercised only through that end-to-end test.

Review details

• Commits: 7b5165f..f57036a
• Model: claude-opus-5

---

Review 2 of 10 for this pull request · View the full run

[upsun-dispatch]

Note

Reviewed — No blocking findings · 🔵 1 minor point

🔁 Incremental · 2 files reviewed

🔵 Minor point

• legacy/src/Command/Metrics/MetricsCommandBase.php:204 — The skip window is a fixed 120 seconds, but the loop can only stop on a whole data point, so the amount of staleness it can introduce is one grain, not two minutes. With the default 60s grain that matches the help text, but the grain grows with --range (the API divides the range when --interval is not given). Running metrics:cpu --range 8h --latest after a worker is removed or scaled to zero: the newest bucket legitimately has one service fewer than the previous bucket, \count($item['services']) > \count($latest['services']) therefore selects the previous bucket, and the loop breaks there because its end is <= _to-120. The command prints a point one grain (potentially tens of minutes) old while describing it as the latest. The same happens with a historical --to, where no settling is needed at all yet the last two points are still compared.

Verification

• With the test's first dataset (_to=1790190300, _grain=60) the loop evaluates the 19:03, 19:02 points and breaks at 19:01, so the asserted 19:02 row is what the code produces.
• The second dataset breaks at 1790190120 with app never exceeded, so --latest returns the newest point 19:04 rather than the older two-service point — the earlier unbounded-staleness behaviour is gone.
• Points with no services key are skipped before \count(), so the in-progress point {"timestamp": 1790190240} cannot raise a count-of-null error.
• _to and _grain are unix seconds elsewhere in the tree (Query sends from/to as ints; commands pass _grain to Duration::humanize), so the integer arithmetic on them is consistent.
• The --interval error message now names --interval and the time range, not --range.

integration-tests/metrics_test.go drives both branches (settle-window skip, and a service that stopped reporting) against a mocked observability endpoint; it runs in the integration-test CI job via make integration-test, and no PHPUnit test in legacy covers the new selection loop.

Review details

• Commits: f57036a..22a079c
• Model: claude-opus-5

---

Review 3 of 10 for this pull request · View the full run

[upsun-dispatch]

Warning

Changes suggested — 🟡 2 warnings

🔁 Incremental · 2 files reviewed

Verification

• The reversed scan keeps strict > on service counts, so among equal-sized points the newest one is still selected.
• Points with no services key (the in-progress minute) are skipped before the settle check, so they can never be returned by --latest.
• The new row() helper's +00:00 is a literal in the Go layout and matches the CLI's output because the test harness sets TZ=UTC (integration-tests/tests.go:160).
• Switching --format tsv to add --no-header matches the expected strings, which no longer contain the Timestamp\tService... header line.
• The third dataset (points 60/61 minutes old) is settled on the first iteration, so --latest returns the newest point without skipping.

The diff rewrites integration-tests/metrics_test.go to use now-relative timestamps and adds a third case (older points are not skipped); it runs in the integration-test job of .github/workflows/ci.yml via make integration-test. No test covers a grain larger than 60 seconds, which is where dropping _grain from the settle check changes behaviour.

Review details

• Commits: 22a079c..0997ba4
• Model: claude-opus-5

---

Review 4 of 10 for this pull request · View the full run

[upsun-dispatch]

Note

Reviewed — No blocking findings · 🔵 1 minor point

🔁 Incremental · 1 file reviewed

🔵 Minor point

• integration-tests/metrics_test.go:49 — Dropping Truncate(time.Minute) narrows but does not remove the timing dependency in the first assertion. ts(1) is exactly 60s before now, and the PHP selector settles a point when timestamp <= time() - 120; the loop breaks on the first settled point it sees. The first latest() call therefore only yields the ts(2) rows if the PHP process reaches processQuery less than 60s after now was computed. That first invocation runs --version for command validation and then the legacy path, which extracts the PHP binary and phar into the cache dir on a cold cache; if that plus process startup exceeds 60s (loaded/parallel CI runner, cold cache, slow disk), ts(1) becomes settled, the loop breaks there and the command prints only the db 0.4 row, failing the first assertTrimmed. The later assertions are unaffected because their expected point is the newest one either way.

Verification

• ts(2) is always settled for the PHP check (floor(now)-120 <= time()-120), so the first assertion's chosen point is deterministic on the older side.
• Row timestamps still match: PropertyFormatter::formatDate preserves seconds and TZ=UTC is set in testEnv, so non-truncated now renders identically on both sides.
• The second and third data sets select the newest point regardless of elapsed time, so removing Truncate does not make those assertions time-sensitive.
• data is read and written under mu, so setData and the observability handler do not race.

The change is itself test code: integration-tests/metrics_test.go (TestMetricsLatest), run by the integration-test job in .github/workflows/ci.yml via make integration-test after make single; no other check covers the PHP --latest selection logic.

Review details

• Commits: 0997ba4..f531f96
• Model: claude-opus-5

---

Review 5 of 10 for this pull request · View the full run

[upsun-dispatch]

Note

Reviewed — No blocking findings · ⚪ 1 nitpick

🔁 Incremental · 1 file reviewed

⚪ Nitpick

• integration-tests/metrics_test.go:88 — The mocked response advertises _from: ts(10) (ten minutes back, matching the CLI's default 10-minute range request), but the last case serves points at 60 and 61 minutes ago, i.e. entirely outside the window the handler itself claims to cover. The real observability API never returns points outside [_from,_to], so the "Older points are not skipped" case exercises a response shape that cannot occur; using points a few minutes old with a wider --range would test the same settle-window behaviour realistically.

Verification

• Moving now into the handler and computing row(...) after each f.Run removes the stale-timestamp ordering hazard: expectations now use the same now as the response they assert on.
• now and data are written under mu in the handler and read under mu in row/setData, and ts/point are only invoked while the lock is held, so there is no unguarded access or self-deadlock.
• With the handler's per-request now, the point at ts(2) is exactly time()-120 when PHP checks it, so it always counts as settled and the loop deterministically stops there — matching the first expectation.
• The 60/61-minute case still passes the PHP loop: buildRows and processQuery never filter points against _from/_to, so the older points are returned rather than dropped.
• go vet on the integration-tests package accepts the new closure-typed data/setData signatures.

Verified by the new TestMetricsLatest in integration-tests/metrics_test.go, which mocks the observability overview endpoint and is executed by the integration-test job in .github/workflows/ci.yml (make integration-test); no PHP-side unit test covers the --latest selection loop in MetricsCommandBase::processQuery.

Review details

• Commits: f531f96..84d669e
• Model: claude-opus-5

---

Review 6 of 10 for this pull request · View the full run