fix: treat prometheus counters as rates in autoscaling signals#1042
fix: treat prometheus counters as rates in autoscaling signals#1042nXtCyberNet wants to merge 9 commits 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the metric collector to calculate rates for Prometheus counters by comparing current values with previous snapshots, introducing a new GetLastUnfreshSnapshotWithTimestamp method in the sliding window structure to track scrape intervals. The reviewer suggested improving the precision of rate calculations by using a single consistent timestamp per pod scrape rather than calling the timestamp function multiple times, and recommended refactoring GetLastUnfreshSnapshot to utilize the new method to reduce code duplication.
| collector.processPrometheusString(result, pastHistogramMap, pastCounterMap, currentHistogramMap, currentCounterMap, pastScrapeTimestamp, instanceInfo.MetricsMap) | ||
| (*currentHistograms)[pod.Name] = HistogramInfo{ | ||
| PodStartTime: pod.Status.StartTime, | ||
| HistogramMap: currentHistogramMap, | ||
| PodStartTime: pod.Status.StartTime, | ||
| HistogramMap: currentHistogramMap, | ||
| CounterMap: currentCounterMap, | ||
| ScrapeTimestamp: util.GetCurrentTimestamp(), | ||
| } |
There was a problem hiding this comment.
For improved precision in rate calculation, it's better to determine the current scrape timestamp once per pod and use it consistently. Currently, util.GetCurrentTimestamp() is called inside processPrometheusString for each metric, and again when creating HistogramInfo. This can introduce minor inaccuracies because the timestamp used for the rate calculation (now) will be slightly different from the timestamp stored for the next cycle (ScrapeTimestamp).
To improve this, you can get the timestamp once before processing the metrics and use it in both places. This ensures the elapsed_seconds for the rate calculation is based on the exact interval between the stored scrape timestamps.
Example of the proposed change:
// In fetchMetricsFromPods:
...
result := string(bodyStr)
now := util.GetCurrentTimestamp()
collector.processPrometheusString(result, pastHistogramMap, pastCounterMap, currentHistogramMap, currentCounterMap, pastScrapeTimestamp, now, instanceInfo.MetricsMap)
(*currentHistograms)[pod.Name] = HistogramInfo{
PodStartTime: pod.Status.StartTime,
HistogramMap: currentHistogramMap,
CounterMap: currentCounterMap,
ScrapeTimestamp: now,
}
...
// And update processPrometheusString signature and body:
func (c *MetricCollector) processPrometheusString(..., pastScrapeTimestamp int64, now int64, instanceMetricMap algorithm.Metrics) {
// ...
// inside case ..._COUNTER:
// remove: now := util.GetCurrentTimestamp()
// ...
}| func (window *SnapshotSlidingWindow[T]) GetLastUnfreshSnapshotWithTimestamp() (value T, timestamp int64, ok bool) { | ||
| if window.freshMilliseconds == 0 { | ||
| return value, 0, false | ||
| } | ||
| currentTimestamp := window.getCurrentTimestamp() | ||
| window.expire(currentTimestamp) | ||
| if window.pool.Len() == 0 { | ||
| return value, 0, false | ||
| } | ||
| front := window.pool.Front() | ||
| if isFresh(window.freshMilliseconds, currentTimestamp, front.timestamp) { | ||
| return value, 0, false | ||
| } | ||
| return front.value, front.timestamp, true | ||
| } |
There was a problem hiding this comment.
The logic inside this new function is nearly identical to GetLastUnfreshSnapshot. To reduce code duplication and improve maintainability, GetLastUnfreshSnapshot could be refactored to call this new function and discard the returned timestamp. This would make the code more aligned with the DRY (Don't Repeat Yourself) principle.
|
Thanks for the analysis, can you add some test coverage |
|
From another view, it reflects the current api does not fit all. |
|
Hi @hzxuzhonghu, thanks for the review. |
|
@nXtCyberNet I donot have a good suggestion now, but will deep dive into other scalers first |
|
Okay, I'll wait for your response. In the meantime, I'll add the test coverage you requested. Thanks! |
|
@hzxuzhonghu any updates ? |
|
@kube-gopher hi , just want to know your thoughts on this , since you already the Observability part of the project , Thanks! |
|
@nXtCyberNet It looks feasible for now, but I haven't looked into it deeply yet. I'm planning to add a feature to the scaler later—I'll open an issue when the time comes, and we can discuss it together then! |
|
can we finish it and make it release in v1.0 by end of this month |
|
@hzxuzhonghu That should work. I'll run a test tomorrow and see how it goes |
|
@hzxuzhonghu @kube-gopher , Yup okay I will complete it asap, Thanks for the confirmation. |
nXtCyberNet
force-pushed
the
issue/counter
branch
from
17fb007 to
a328a23
Compare
I think this issue may already be solved. Looking at |
|
@hzxuzhonghu could you please take a look. Thanks! |
| currentHistogramMap := currentHistograms[pod.Name].HistogramMap | ||
| if currentHistogramMap == nil { | ||
| currentHistogramMap = make(map[string]*histogram.Snapshot) | ||
| } |
There was a problem hiding this comment.
From a visual standpoint, the current design isn't as good as before
There was a problem hiding this comment.
yup i agree , however it didnot create very big change just reuse the already created map , i.e its just a memory optimization. But i will do this , Thanks for pointing out ,
Signed-off-by: nXtCyberNet <rohantech2005@gmail.com>
Signed-off-by: nXtCyberNet <rohantech2005@gmail.com>
Signed-off-by: nXtCyberNet <rohantech2005@gmail.com>
Signed-off-by: nXtCyberNet <rohantech2005@gmail.com>
Signed-off-by: nXtCyberNet <rohantech2005@gmail.com>
Signed-off-by: nXtCyberNet <rohantech2005@gmail.com>
nXtCyberNet
force-pushed
the
issue/counter
branch
from
fafd93f to
ec0bc62
Compare
| } | ||
| currentHistograms := make(map[string]HistogramInfo) | ||
|
|
||
| pastCounters, ok := collector.PastCounters.GetLastUnfreshSnapshot() |
There was a problem hiding this comment.
Have you considered this issue: when counting in a window-based manner, within the first minute after the collector starts or the Pod restarts, even though the counter itself is increasing, all auto-scaling metrics based on that counter will be reported as 0, potentially causing scaling-out to be skipped?
There was a problem hiding this comment.
My suggestion is to have the counter immediately obtain the baseline from the last scrape
There was a problem hiding this comment.
@nXtCyberNet I suggest that you ultimately submit a clean commit history
There was a problem hiding this comment.
yup i agree i willl rebase it later in single commit
There was a problem hiding this comment.
but i want when the full system starts then the baseline is also 0 so why did it a problem , but if you thinking if the router is restarted then also since this metrics is gone so how can it get it and if the vllm pod crashed then also all the metrics get flushed out so , isnt it a current tradeoff , and also we are finding average rate over a minute not the instantaneus rate . so could you explain a bit more about this . and also didnot it gonna break the autoscaler algo as the rate we get is a noise
cc @hzxuzhonghu
There was a problem hiding this comment.
@nXtCyberNet I don't quite understand your language. Please express your thoughts in an affirmative tone or a declarative mood
There was a problem hiding this comment.
i will use ai to finetune this ,
There was a problem hiding this comment.
Thanks for the suggestion, but I have a few concerns:
On the "baseline = 0" issue: When the whole system starts fresh, the baseline is also 0 — so why is that actually a problem in that case?
On restarts/crashes: If the router restarts, its metrics are gone anyway. Same if the vLLM pod crashes — all its metrics get flushed. Isn't this just an inherent tradeoff we already have, regardless of the baseline approach? Could you explain in more detail what specific case you're worried about?
On time drift: If there's any time drift, the counter resets back to 0 as well — this happens in both of the above cases too.
On averaging: We're computing the average rate over a full minute, not the instantaneous rate. Won't that introduce noise into what the autoscaler sees, and potentially break its scaling logic?
cc @hzxuzhonghu
| currentCounterMap[policyKey] = v | ||
| rate := 0.0 | ||
| now := util.GetCurrentTimestamp() | ||
| if pastCounterMap != nil && pastScrapeTimestamp > 0 { |
There was a problem hiding this comment.
With this logic, the first scrape after the collector starts or a pod restarts always reports 0 for counter metrics because there is no previous baseline yet. If traffic is already increasing during that first window, scale-out can be skipped until the next scrape. Please seed the baseline from the first scrape while still allowing the next scrape to produce a rate, or document why this delay is acceptable for autoscaling.
There was a problem hiding this comment.
@nXtCyberNet Based on your previous response, I think you should be able to understand the maintainer's suggestion
There was a problem hiding this comment.
I'll state my view directly: seed on first scrape, compute from the next scrape, and keep the 60s
GetLastUnfreshSnapshot() logic only for histogram/SLO metrics
There was a problem hiding this comment.
okay i have done this even with 15 sec scrap time the rate became noisey but that i think is not a big issue . now you can take a look . Thanks for help
|
@nXtCyberNet Here's a question for you to think about: Prometheus has a default scrape interval of 15 seconds, and the counter snapshot staleness window only reverts to a "stale" baseline after 60 seconds
You may use AI to assist you, but for the final answer, I want you to phrase it in your own words for me |
|
Hi, so for your answer, considering the case where the router pod restarts, this is how I understand the timeline: So you're right that during this initial ~60s period the autoscaler will see the request rate as 0. My question is whether this is actually a problem or simply the expected trade-off of the current design. On a router restart, the collector loses its in-memory sliding window, so there is no previous sample available to compute a rate from. Similarly, if the vLLM pod restarts, the Prometheus counters themselves reset. In both cases, there isn't enough historical information to compute a meaningful rate immediately after restart. Also, since this metric is intentionally defined as a rolling one-minute average rather than an instantaneous 15s rate, waiting until a full one-minute baseline exists seems consistent with what the metric is trying to represent. So is the concern specifically that the autoscaler observes |
Currently, the issue revolves around the fact that it returns 0 instead of unknown |
Signed-off-by: nXtCyberNet <rohantech2005@gmail.com>
kube-gopher
left a comment
kube-gopher
left a comment
There was a problem hiding this comment.
you are adding a unit test case yourself
| if mf.GetType() == io_prometheus_client.MetricType_COUNTER { | ||
| currentCounterMap[policyKey] = v | ||
| rate := 0.0 | ||
| now := util.GetCurrentTimestamp() | ||
| if pastCounterMap != nil && pastScrapeTimestamp > 0 { | ||
| if prev, ok := pastCounterMap[policyKey]; ok { | ||
| if v >= prev { | ||
| elapsedSec := float64(now-pastScrapeTimestamp) / 1000.0 | ||
| if elapsedSec > 0 { | ||
| rate = (v - prev) / elapsedSec | ||
| } | ||
| } | ||
| } | ||
| } | ||
| values[policyKey] += rate | ||
| } else { | ||
| if gotValue { | ||
| values[policyKey] += v | ||
| } |
There was a problem hiding this comment.
if mf.GetType() == io_prometheus_client.MetricType_COUNTER {
if !gotValue {
continue
}
currentCounterMap[policyKey] = v
prev, ok := pastCounterMap[policyKey]
if !ok || pastScrapeTimestamp <= 0 {
continue
}
if v < prev {
continue
}
elapsedSec := float64(util.GetCurrentTimestamp()-pastScrapeTimestamp) / 1000.0
if elapsedSec <= 0 {
continue
}
values[policyKey] += (v - prev) / elapsedSec
} else {
if gotValue {
values[policyKey] += v
}
}
There was a problem hiding this comment.
yup i agree 0 could cause fake baseline , instead insufficient metrics as it will already managed by the getDesiredInstancesForSingleInstanceMetric
| err = collector.collectPodMetrics(context.Background(), pod, podSource, wanted, values, pastHistograms, currentHistograms, pastCounters, currentCounters) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Equal(t, 0.0, values["my_policy_key"]) |
There was a problem hiding this comment.
_, ok := values["my_policy_key"]
assert.False(t, ok)
| err = collector.collectPodMetrics(context.Background(), pod, podSource, wanted, values, pastHistograms, currentHistograms, pastCounters, currentCounters) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.InDelta(t, tc.want.rate, values["my_policy_key"], 1e-2) |
There was a problem hiding this comment.
add
} else {
_, ok := values["my_policy_key"]
assert.False(t, ok)
}
|
@nXtCyberNet Here's my thinking |
|
|
||
| currentCounters[pod.Name] = CounterInfo{ | ||
| PodStartTime: pod.Status.StartTime, | ||
| CounterMap: currentCounterMap, | ||
| ScrapeTimestamp: util.GetCurrentTimestamp(), | ||
| } |
There was a problem hiding this comment.
Counter-rate calculations are inaccurate when a pod is scraped through more than one metric source because the implementation retains only one timestamp per pod. suggest optimizing it
There was a problem hiding this comment.
I agree this would make the collector more robust. I do have one question, though.
My understanding was that for autoscaling, the counter metric would typically come directly from the vLLM metrics endpoint, which acts as the single source of truth. In that case, I would expect each pod to expose only one relevant counter source for the autoscaler.
Could you elaborate on the scenarios where we'd intentionally scrape counter metrics from multiple endpoints of the same pod? If both the vLLM container and a sidecar exposed a similar counter, wouldn't we generally want to rely on the vLLM metric as the authoritative source? I'm wondering if I'm missing a valid use case or misunderstanding the intended design.
I understand that the API allows this configuration, but I'm trying to understand the motivation and intended deployment scenarios for supporting it. I may be missing a use case here.
Signed-off-by: nXtCyberNet <rohantech2005@gmail.com>
| currentCounterMap[policyKey] = v | ||
|
|
||
| prev, ok := pastCounterMap[policyKey] | ||
| if !ok || pastScrapeTimestamp <= 0 { |
There was a problem hiding this comment.
When one selected pod has no baseline, this only skips that pod. values can still contain the same policy key from another pod, so the autoscaler emits a partial rate as though every ready replica were represented. Please suppress the policy metric for the whole pod group until every selected pod has a valid delta, and cover the mixed baseline/no-baseline case.
What type of PR is this?
/kind bug
/kind enhancement
What this PR does / why we need it:
Prometheus counter metrics are monotonically increasing cumulative values.
The autoscaler was treating these raw cumulative values as instantaneous load
signals, causing two critical failures:
Scale-up runaway: A pod that handled 50 total requests since startup would
trigger scaling to 5 replicas (ceil(50 / target)), regardless of current traffic.
No scale-down: Even after traffic stops, the counter value persists at 50,
preventing the autoscaler from ever scaling back down.
The fix: Track per-pod counter snapshots across scrape cycles and compute the
rate of change (delta/elapsed_seconds) instead of the raw cumulative value.
This correctly reflects instantaneous demand and enables both scale-up and scale-down.
Implementation Details:
CounterMapandScrapeTimestampfields toHistogramInfoto maintainper-pod/metric baseline state across scrape cycles.
GetLastUnfreshSnapshotWithTimestamp()toSnapshotSlidingWindowto expose precise per-pod scrape timestamps (more accurate than window-level timestamps).
CounterMapguards protect in-memory snapshotscreated before this change.
Which issue(s) this PR fixes:
Fixes #1037
Special notes for your reviewer:
Why per-pod timestamps? The sliding-window-level timestamp is too coarse;
per-pod scrape times give us the actual elapsed duration for each counter,
improving rate precision when scrape intervals vary.
Counter reset handling: If a pod restarts, its counter resets to 0.
Detecting
current < previousavoids reporting a massive negative rate;returning 0 is safe and gives the pod a grace period to re-accumulate load signals.
Backward compatibility: Any in-memory snapshots from before this change
will have
CounterMap == nil. These are safely handled with a nil-check guard.Testing recommendation: Add bench tests for rate calculation under
varying scrape intervals and counter reset scenarios.
Does this PR introduce a user-facing change?:
Yes. Autoscaling behavior for counter-based metrics will change—scaling will now
respond to instantaneous rate of change rather than cumulative totals, enabling
proper scale-down behavior.