feat(autoscaler): replace 60s sliding window with 24h archive for pre…#1373
feat(autoscaler): replace 60s sliding window with 24h archive for pre…#1373vivek41-glitch wants to merge 1 commit into
Conversation
|
[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 |
There was a problem hiding this comment.
Code Review
This pull request updates the MetricCollector initialization in pkg/autoscaler/autoscaler/metric_collector.go to increase the historical archive window for cold-starts to 24 hours. However, passing the raw value 86400 directly to NewSnapshotSlidingWindow incorrectly sets the window duration to 86.4 seconds instead of 24 hours, as the constructor expects milliseconds. It is recommended to use util.SecondToTimestamp(86400) to perform the correct conversion.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| return &MetricCollector{ | ||
| PastHistograms: datastructure.NewSnapshotSlidingWindow[map[string]HistogramInfo](util.SecondToTimestamp(util.SloQuantileSlidingWindowSeconds), util.SecondToTimestamp(util.SloQuantileDataKeepSeconds)), | ||
| // FIX: Swapped from 60s (SloQuantileSlidingWindowSeconds) to 24h (86400 seconds) for cold-start historical archive | ||
| PastHistograms: datastructure.NewSnapshotSlidingWindow[map[string]HistogramInfo](86400, 86400), |
There was a problem hiding this comment.
The NewSnapshotSlidingWindow constructor expects parameters in milliseconds (freshMilliseconds and expireMilliseconds). Passing 86400 directly means the window will expire after 86400 milliseconds (86.4 seconds) instead of 24 hours (86400 seconds). You should use util.SecondToTimestamp(86400) to correctly convert the duration to milliseconds.
| PastHistograms: datastructure.NewSnapshotSlidingWindow[map[string]HistogramInfo](86400, 86400), | |
| PastHistograms: datastructure.NewSnapshotSlidingWindow[map[string]HistogramInfo](util.SecondToTimestamp(86400), util.SecondToTimestamp(86400)), |
There was a problem hiding this comment.
Pull request overview
This PR changes the autoscaler’s in-memory histogram snapshot window in pkg/autoscaler to retain longer history (intended: 24 hours) so future forecasting/regression work (Issue #1361) can use historical patterns instead of only very recent data.
Changes:
- Updates
MetricCollector’sPastHistogramssliding window configuration from the prior short window to a 24h-oriented setting. - Adds an inline comment describing the intended cold-start historical archive behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // FIX: Swapped from 60s (SloQuantileSlidingWindowSeconds) to 24h (86400 seconds) for cold-start historical archive | ||
| PastHistograms: datastructure.NewSnapshotSlidingWindow[map[string]HistogramInfo](86400, 86400), |
| // FIX: Swapped from 60s (SloQuantileSlidingWindowSeconds) to 24h (86400 seconds) for cold-start historical archive | ||
| PastHistograms: datastructure.NewSnapshotSlidingWindow[map[string]HistogramInfo](86400, 86400), |
|
hi could you please check if the autoscaler algo will not gonna break or cause scaling decisions delay ,, one more thing since the current algo is using the max recommendation from all there value - counter , histogram and gauge , so with the 24 hr timestamp the histogram will have nothing for the initial 24 hours and even after 24 hours it will gonna be a maybe cause false signal because it could be a larger no. then others all the decision based on the other too only , |
…dictive cold-start Signed-off-by: vivek41-glitch <viveksantoshkumardubey@gmail.com>
1b4aeaa to
6543521
Compare
| // FIX: Swapped from 60s (SloQuantileSlidingWindowSeconds) to 24h (86400 seconds) for cold-start historical archive | ||
| PastHistograms: datastructure.NewSnapshotSlidingWindow[map[string]HistogramInfo](util.SecondToTimestamp(86400), util.SecondToTimestamp(86400)), |
| return &MetricCollector{ | ||
| PastHistograms: datastructure.NewSnapshotSlidingWindow[map[string]HistogramInfo](util.SecondToTimestamp(util.SloQuantileSlidingWindowSeconds), util.SecondToTimestamp(util.SloQuantileDataKeepSeconds)), | ||
| // FIX: Swapped from 60s (SloQuantileSlidingWindowSeconds) to 24h (86400 seconds) for cold-start historical archive | ||
| PastHistograms: datastructure.NewSnapshotSlidingWindow[map[string]HistogramInfo](util.SecondToTimestamp(86400), util.SecondToTimestamp(86400)), |
| // FIX: Swapped from 60s (SloQuantileSlidingWindowSeconds) to 24h (86400 seconds) for cold-start historical archive | ||
| PastHistograms: datastructure.NewSnapshotSlidingWindow[map[string]HistogramInfo](util.SecondToTimestamp(86400), util.SecondToTimestamp(86400)), |
thanks for the thorough review, @nXtCyberNet.and u r right, first 24 hours will have no historical data. However, NewSnapshotSlidingWindow will still return the current pod's lifetime histogram during that gap, so the autoscaler won't break or scale to zero and about false signals Since the algorithm uses max, a larger 24h P95 might dominate the scaling decision. |
and just clarifying that i have raise this pr by intentionally to just minimal ask (#1361): proving the window can be extended. the actual 30 days presistent archive and the logic to handle max combination properly will added in follow-up prs , once datastore become ready , this is just foundation , not a full a algo . |
|
didnot you think you should do everything in single pr instead of creating a small ones. |
@nXtCyberNet I got u bro! Actually my previous pr #1364 become messy cause of go tests failure and ai bot comments and things become messed , so decided to break problem into steps n proceed , but u also right. We should maintain one pr only . |
LiZhenCheng9527
left a comment
There was a problem hiding this comment.
Autoscaler now supports Prometheus. So, regarding the situation mentioned in the issue, wouldn't it be possible to retrieve the 24-hour P99 metric using Prometheus query? Does this require any code changes?
Exactly @LiZhenCheng9527 but this changes proving that the autoscaler internal sliding window can be successfully extended to 24h baseline using existing pastHistograms logic without adding new dependencies. And the prometheus approach is currently architecture. I think we can accept this foundational in memory patch .then i can iterate and in further pr . |
plss review , so i can move forward or just guide ,i should make fix furthur |
This PR implements the foundational first step for #1361. Currently, the histogram only uses the last 60 seconds of data. This PR extends the sliding window to 24 hours (86400 seconds), acting as a cold-start offset to allow scaling based on longer-term patterns. The full regression model and 30-day archive requested in #1361 can be built in a follow-up PR once the datastore infrastructure is added."