π fix(dashboard): report the count window separately from the freshness window - #4878
Conversation
β¦ss window /api/repo-activity advertised window_hours: 12 while the per-repo counts were accumulated over 14 days (activity_collector.go:214 uses activityWindow = 336h; :289 labelled the snapshot with activityHealthWindowHours = 12). Any consumer treating Count as "output within window_hours" β an events-per-hour rate, most obviously β overstates activity by 28x. Both values are legitimate and both are needed. The wide accumulation window is deliberate: the hub computes freshness itself from NewestAt, and a slightly-late collect must not drop a just-inside-12h event. The bug was reporting one number for two different questions. Adds count_window_hours alongside the existing window_hours rather than changing what window_hours means. The hub's health verdict already recomputes recency from NewestAt and owns its own 12h constant, so it was never misled and its semantics are untouched β no clamp change needed either, the sanitizer already tolerates 720h. Carried through the heartbeat and hub registry so hub-side consumers get the same disambiguation, and stated in the endpoint's own limitations strings, because a caller reading the JSON should not have to find this commit. A test pins the two windows apart: equal values fail, and a count window narrower than the freshness window fails, since that would let a "fresh" event fall outside the counted range. This matters beyond the cosmetic. Phase 1 of the per-repo cost epic (#4836) produces these counts, and Phase 3 attaches spend to them β at which point a 28x mislabel stops being a wrong dashboard number and becomes a wrong budget figure. It also contradicted that epic's own stated rule that the window must be reported alongside the number. While here, the limitations now state that counts are bounded by audit-log retention: rotated and compressed backups ARE read, but only MaxBackups are kept, so a busy hive's effective lookback can be shorter than the nominal count window. Fixes #4860 Signed-off-by: Andy Anderson <andy@clubanderson.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 |
|
Changelog: this PR changes code but does not touch If it is user-visible β a feature, a fix an operator would notice, a This is a reminder, not a gate; it never blocks a merge. |
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
GET /api/repo-activityadvertisedwindow_hours: 12while the per-repo counts were accumulated over 14 days:activityWindow= 336hactivity_collector.go:214activityHealthWindowHours= 12activity_collector.go:289Any consumer treating
Countas "output withinwindow_hours" β an events-per-hour rate, most obviously β overstates activity by 28Γ.Both values are legitimate; reporting one for both questions was the bug
The wide accumulation window is deliberate and documented: the hub computes freshness itself from
NewestAt, and a slightly-late collect must not drop a just-inside-12h event. So this addscount_window_hoursalongsidewindow_hoursrather than redefining either.The hub's health verdict already recomputes recency from
NewestAtand owns its own 12h constant (health_verdict.go:33-35), so it was never misled and its semantics are untouched. No clamp change was needed βsanitizeRepoActivityalready tolerates 720h.What's included
CountWindowHoursonActivitySnapshot, populated fromactivityWindowrepo_activity_count_window_hours) and the hub registry, so hub-side consumers get the same disambiguationlimitationsstrings β a caller reading the JSON shouldn't have to find this commitWhy this is worth fixing now rather than later
Phase 1 of the per-repo cost epic (#4836) produces these counts, and Phase 3 attaches spend to them. At that point a 28Γ mislabel stops being a wrong dashboard number and becomes a wrong budget figure. It also directly contradicted that epic's own stated rule that the attribution window must be reported alongside the number.
One correction to the reported scope
While adding a retention caveat I initially wrote that the audit log's rotated backups are not read. That is no longer true β
OutputActionsSincereads rotated and compressed.gzbackups (with a gzip-bomb cap, hardened in #4863). The limitation now states the accurate constraint: backups are read, but onlyMaxBackupsare kept, so a busy hive's effective lookback can still be shorter than the nominal count window.Fixes #4860