I read through this carefully — it's a meaningful architectural shift, and the reasoning behind it is sound throughout.
What changed and why it matters. Previously, the app-level rollup was driven by the entity store: you could only see apps that had live sandboxes. This PR flips the source of truth for figures to the metrics store (querying by (miren_app, miren_kind) group key), so a historical window — covering apps whose sandboxes were swept an hour after they died — can answer at all. The entity pass now plays a supporting role: it contributes the live flag, the appID, and the stale signal for apps that exist but aren't reporting. This is a well-motivated design and the comments explain every tradeoff.
The three-query app listing. Replacing the per-sandbox fan-out with three cluster-wide queries (cpu, memory, count) is the key scalability improvement. The sandboxCountQuery is a nested count that avoids double-counting multi-container sandboxes. The appKeyOf / appMetrics.keys() pair handles the two-label grouping cleanly, and the sorted-key determinism is the right call for stable ordering of historical rows.
Contributors. The appContributors path uses increase(cpu_seconds[window]) rather than a rate, so a sandbox that ran for ten minutes can still be compared to one that ran all week. The tfirst_over_time / tlast_over_time choice over tmin/tmax is explicitly justified and correct. One small edge case: firstSeenQuery and lastSeenQuery group by miren_sandbox only, so the rows they return have no service/version/node/kind labels. The collect helper passes these through identityOf, which constructs a contributorIdentity with those fields empty, then looks up the tally by sandbox id. If a sandbox somehow appears only in the seen queries (no CPU or memory row, e.g. because those queries errored), it creates a tally with a blank identity that becomes a row. The comment says this won't happen because the seen queries are intentionally ordered after cpu/memory, and that's true in the success path — but if cpu/memory fail and seen queries succeed, you get contributors with no usage data. That's arguably fine (it surfaces the sandbox, just with zeroes), and it's a degraded-backend scenario, not a normal-path bug.
Tests. Coverage is real and purposeful. TestAppRowsIncludeAppsOnlyTheStoreRemembers directly exercises the historical-app case. TestAppRowsPlaceLiveAppsBeforeHistoricalOnes pins the ordering guarantee. TestAppSeriesSumsKindsPerTimestamp verifies the addition-by-timestamp rather than concatenation. The range-query stub going through an actual HTTP test server means the query shape is tested end-to-end. The query_test.go additions covering sandboxCountQuery, contributorCPUSecondsQuery, etc. close the query-generation loop.
No blockers. The security-relevant path — labelSelector — already escapes values with %q and is unchanged. The step-resolution logic is simple and has test coverage. The appDetailResult.apply pattern mirrors the existing sandboxDetailResult.apply idiom and is consistent.
This is ready to merge.
Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1215,
"verdict": "ready",
"event": "approve",
"summary": "I read through this carefully — it's a meaningful architectural shift, and the reasoning behind it is sound throughout.\n\n**What changed and why it matters.** Previously, the app-level rollup was driven by the entity store: you could only see apps that had live sandboxes. This PR flips the source of truth for figures to the metrics store (querying by `(miren_app, miren_kind)` group key), so a historical window — covering apps whose sandboxes were swept an hour after they died — can answer at all. The entity pass now plays a supporting role: it contributes the `live` flag, the `appID`, and the `stale` signal for apps that exist but aren't reporting. This is a well-motivated design and the comments explain every tradeoff.\n\n**The three-query app listing.** Replacing the per-sandbox fan-out with three cluster-wide queries (cpu, memory, count) is the key scalability improvement. The `sandboxCountQuery` is a nested count that avoids double-counting multi-container sandboxes. The `appKeyOf` / `appMetrics.keys()` pair handles the two-label grouping cleanly, and the sorted-key determinism is the right call for stable ordering of historical rows.\n\n**Contributors.** The `appContributors` path uses `increase(cpu_seconds[window])` rather than a rate, so a sandbox that ran for ten minutes can still be compared to one that ran all week. The `tfirst_over_time` / `tlast_over_time` choice over `tmin`/`tmax` is explicitly justified and correct. One small edge case: `firstSeenQuery` and `lastSeenQuery` group by `miren_sandbox` only, so the rows they return have no service/version/node/kind labels. The `collect` helper passes these through `identityOf`, which constructs a `contributorIdentity` with those fields empty, then looks up the tally by sandbox id. If a sandbox somehow appears only in the seen queries (no CPU or memory row, e.g. because those queries errored), it creates a tally with a blank identity that becomes a row. The comment says this won't happen because the seen queries are intentionally ordered after cpu/memory, and that's true in the success path — but if cpu/memory fail and seen queries succeed, you get contributors with no usage data. That's arguably fine (it surfaces the sandbox, just with zeroes), and it's a degraded-backend scenario, not a normal-path bug.\n\n**Tests.** Coverage is real and purposeful. `TestAppRowsIncludeAppsOnlyTheStoreRemembers` directly exercises the historical-app case. `TestAppRowsPlaceLiveAppsBeforeHistoricalOnes` pins the ordering guarantee. `TestAppSeriesSumsKindsPerTimestamp` verifies the addition-by-timestamp rather than concatenation. The range-query stub going through an actual HTTP test server means the query shape is tested end-to-end. The `query_test.go` additions covering `sandboxCountQuery`, `contributorCPUSecondsQuery`, etc. close the query-generation loop.\n\n**No blockers.** The security-relevant path — `labelSelector` — already escapes values with `%q` and is unchanged. The step-resolution logic is simple and has test coverage. The `appDetailResult.apply` pattern mirrors the existing `sandboxDetailResult.apply` idiom and is consistent.\n\nThis is ready to merge.",
"posted_to_pr": true
}