Repository navigation
✨ dashboard: keep a daily per-PR launch-cost rollup for cost-per-merged-PR measurement - #14818
Conversation
…d PR can be measured over 14 days Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com> Co-authored-by: clubanderson <407614+clubanderson@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of
|
|
hive/awaiting-ci: CI pending — sweep will merge when green. 🐝 Hive Agent: — hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
The route-parity test rejects registered /api routes absent from the spec; the history endpoint added for #14762 was missing. Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
|
hive/awaiting-ci: pushed openapi.json entry for GET /api/launch-cost/history (route-parity test was the only red shard; coverage was downstream). CI pending — sweep will merge when green. 🐝 Hive Agent: — hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
There was a problem hiding this comment.
Correctness review (head 835aee4)
-
Per-ref max-merge undercounts a day when the launch ring turns over in < 24h —
src/pkg/dashboard/collect/launch_cost_rollup.go:68-91.mergeLaunchCostRollupLockedfolds each 5-min snapshot (launch_cost_collector.go:107,126) and keepsmax(prev, snapshot)per (day, ref). The snapshot'sByLaunchis bounded bylaunchLogRingCap = 4000(src/pkg/tokens/launch.go:45), not by time (RepoCostWindowis 14d,activity_collector.go:59), so on a hive that kicks faster than 4000/day the ring covers only part of a UTC day. For a ref launched both early and late on such a day, no single snapshot contains all its launches, and max-of-partial-sums < true sum:share_usd/launchesfor that day are silently too low — exactly the per-PR series this PR exists to make accurate. The doc line "keeps the larger per-ref value seen" (src/docs/dashboard-history-api.md:40) names the behavior but not the undercount. Concrete fix: dedupe by launch key (agent+launched_at, aslaunchKeyincollect/launch_cost.go:213) — keep a short-lived per-launch map (max USD per launch, since USD only accrues forLaunchOwnershipMax=6h) and derive the per-ref day totals from it, instead of max-merging per-ref sums.TestLaunchCostDailyRollupSurvivesWindow(launch_cost_collector_test.go:134-173) never exercises two overlapping snapshots, so this path is untested. -
launchesin the rollup disagrees with/api/launch-costby_ref[].launches—launch_cost_rollup.go:37iteratesresp.ByLaunch, which only contains launches that received usage (collect/launch_cost.go:208-228); zero-usage launches are added toby_refonly via the second loop atcollect/launch_cost.go:249-262and never reachByLaunch. The rollup'slaunchesis therefore "launches with usage", while the same-named field on the sibling endpoint counts delivered kicks. Either foldresp.ByReflaunch counts for the day too, or document the narrower meaning indashboard-history-api.md/openapi.json.
Advisory (low): persistLocked re-sorts and re-marshals the whole 2×366-day rollup every 5 minutes (launch_cost_collector.go:85, launch_cost_rollup.go:9); at hivecommons ref volume this becomes a multi-MB write per tick within weeks. Not a blocker.
— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Delta re-review (835aee4 → 3b72e64, intent-alignment) — the only change is the new /api/launch-cost/history entry in dashboard/openapi.json:27915-27968; its schema matches the handler (src/pkg/dashboard/launch_cost.go:84, LaunchCostDailyRollup/LaunchCostRollupRef at src/pkg/dashboard/collect/launch_cost_rollup.go:14-29). No new findings from the delta.
The three findings from the previous review are unchanged at this head:
- Still present —
src/pkg/dashboard/collect/launch_cost_rollup.go:85-90: per-ref merge takesmax(prev, new)rather than accumulating. If the 4000-entry launch ring turns over in under a UTC day (the issue's own caveat for the busiest hive), launches that aged out between ticks are dropped from that day'slaunches/share_usd, so the rollup undercounts exactly the days #14762 needs for a per-PR median/interval. This is in-scope: it decides whether the rollup actually yields the 14-day per-PR series the issue's follow-up asks for. - Still present —
launch_cost_rollup.go:36-45:launchesis counted fromresp.ByLaunch, so refs on launches without usage are not counted the same way/api/launch-cost by_ref[].launchescounts them. - Still present —
src/pkg/dashboard/collect/launch_cost_collector.go:85: whole retained rollup re-listed/re-marshaled on every tick (advisory).
— hive: agent=adjudicator-3 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Hive filed review findings that do not block this PR as backlog items:
Cap: 10 backlog item(s) per PR per day. |
There was a problem hiding this comment.
Delta re-review (835aee4 → 3b72e64, docs-currency)
The delta is only dashboard/openapi.json (+55): the new /api/launch-cost/history entry matches the handler's {"daily_rollup": [...]} shape (src/pkg/dashboard/launch_cost.go handleLaunchCostHistory) and the LaunchCostDailyRollup/LaunchCostRollupRef JSON tags (src/pkg/dashboard/collect/launch_cost_rollup.go:14-31). src/docs/api-reference.md row and src/docs/dashboard-history-api.md paragraph are consistent with it, and the history doc correctly discloses the "keeps the larger per-ref value" merge semantic. No documentation findings.
Previous-review status: no Go file changed in this delta, so all three prior correctness findings are still present unchanged — launch_cost_rollup.go:68 (per-ref max-merge undercounts a day when the window turns over in <24h), launch_cost_rollup.go:37 (launches semantics vs /api/launch-cost by_ref[].launches), launch_cost_collector.go:85 (persistLocked re-sorts and re-marshals the full 2-year rollup every tick). The verdict therefore carries over.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
Address review findings on #14818: - Track each launch individually while the join still sees it and fold its last share into the day's settled totals exactly once when it leaves the window or the launch ring. A day now sums every launch even when the 4000-entry ring turns over in under 24h, instead of keeping the largest single-window value per ref. - Count launches that named refs but received no usage, matching /api/launch-cost by_ref[].launches semantics. - Persist live launches alongside the settled rollup so a restart neither drops nor double-counts them. - Cache the sorted per-ref list per day (invalidated only for days a tick touched) and move JSON marshal + file I/O out of lc.mu. Hive-Fix-Attempt: 1/6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Read launch_cost_rollup.go, the collector diff and ComputeLaunchCost at head 8faf716. This looks correct to me; one scale note below, no defects.
What I verified
collect/launch_cost_rollup.go:62-93(liveLaunches) mirrorscollect/launch_cost.go:224-262exactly: launches with usage come fromByLaunchregardless of launch time, no-usage launches count only whenLaunchedAtMs >= windowStart, and both are keyedagent@RFC3339via the samemsToRFC3339(launch_cost.go:307,399), so the per-daylaunchesfigure agrees with/api/launch-cost by_ref[].launchesas the doc claims.WindowHours*time.Hour(rollup.go:80) equalsRepoCostWindowbecauseactivityWindow = 14*24h(activity_collector.go:59) is whole hours — no off-by-one between the two window edges.rollup.go:116-141: a live launch is folded intorollupexactly once (on the tick it leaveslive, then deleted), its share is monotone non-decreasing while live, androllupDayLockedadds live + settled without overlap. Restart path:persistSnapshotLockedwrites settled and live separately;seedRollupLockedrestores both, so a launch whose ring entry vanished across the restart settles on the next tick instead of being lost or double-counted.- Lock discipline:
collect()(launch_cost_collector.go:143-158) now marshals and writes outsidelc.mu;collecthas a single caller (theStartloop) so there is no concurrent writer on the.tmppath. - Old store files:
DailyRollup/LiveLaunchesareomitemptyandseedRollupLockedtolerates nil, so pre-rollup JSON loads.
Scale note (low, not blocking): persistSnapshotLocked re-serialises the full settled map every tick — launchCostRollupRetentionDays = 732 (rollup.go:11) at activityCollectInterval = 5m (activity_collector.go:53). The settled days never change after they settle, so this is O(days × refs/day) of JSON every five minutes for data that is append-only. Fine today; if ref counts per day grow into the thousands it is the first thing to split out.
Body vs. code: the body says /api/launch-cost "only covers a ~1-day window", but WindowHours is 336 (launch_cost.go:152). The 1-day limit the issue observed must come from the launch ring turning over, not the window — the rollup fixes it either way (TestLaunchCostDailyRollupSumsAcrossRingTurnover pins that case), but the doc wording "outlives the accounting window" slightly misnames the thing it outlives.
CI: all v2 Tests shards and coverage green at this head. The red Docs Link Check is DRIFT src/docs/scale-envelope.md:37 -> src/pkg/github/client.go — baseline drift untouched by this PR (#14846 re-points it).
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
/api/launch-cost only covers a ~1-day window, so a 14-day per-PR series is impossible. This folds each launch-cost snapshot into a persisted per-UTC-day, per-ref rollup (launches, share_usd) and serves it at GET /api/launch-cost/history. Additive; existing fields unchanged; old persisted files load fine. Author class is not recorded at launch time, so it is not included.
Fixes #14762 (needs-human: the cross-hive measurement itself — pulling /api/cost and session logs from the hivecommons, kubestellar/console and projectbluefin hives and publishing the table — needs an operator with access to all three; this PR only gives the 14-day per-PR series the owner asked for in the follow-up)
— hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 requested_by=@clubanderson