feat(metrics): record scheduling duration and attempt counters in scheduler - #2342
Conversation
…eduler Signed-off-by: swastikCommits <textswastik.alt@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: swastikCommits 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 |
📝 WalkthroughWalkthroughThe scheduler now exposes Prometheus metrics for scheduling duration and labeled scheduling attempts. ChangesScheduler telemetry
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/scheduler/scheduler_test.go`:
- Around line 2339-2340: Update the test around schedulingDuration and
s.Filter(args) to capture the histogram’s _count before and after the filter
call using schedulingDuration.WithLabelValues(...) or its default labels, then
assert that the count increases. Replace the existing
CollectAndCount(schedulingDuration) > 0 assertion, which only verifies
collection rather than recording.
In `@pkg/scheduler/scheduler.go`:
- Line 940: Update the scheduling outcome accounting around filterSimulation and
Filter so every filterSimulation result increments schedulingAttempts, including
successful assignments, no-fit results, and errors with their appropriate
outcome labels; avoid double-counting the existing no_resource_requested path.
- Around line 74-77: Update init around schedulingDuration and
schedulingAttempts registration to handle prometheus.Register errors instead of
discarding them: when an AlreadyRegisteredError is returned, replace the local
collector with the existing registered collector, and explicitly fail startup
for other registration errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b3d335c1-01d6-44cc-a9c0-d5758c463d9d
📒 Files selected for processing (2)
pkg/scheduler/scheduler.gopkg/scheduler/scheduler_test.go
| histCount := testutil.CollectAndCount(schedulingDuration) | ||
| require.Greater(t, histCount, 0, "expected schedulingDuration metric to be collected") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file and surrounding code =="
git ls-files | grep -F 'pkg/scheduler/scheduler_test.go' || true
wc -l pkg/scheduler/scheduler_test.go 2>/dev/null || true
sed -n '2300,2375p' pkg/scheduler/scheduler_test.go 2>/dev/null || true
echo "== find schedulingDuration definition/usages =="
rg -n "schedulingDuration|Filter|CollectAndCount|SampleCount|_count" pkg -S || true
echo "== import aliases if present =="
cat hack/.import-aliases 2>/dev/null || trueRepository: Project-HAMi/HAMi
Length of output: 18581
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== scheduler metrics module =="
sed -n '1,130p' pkg/scheduler/scheduler.go
sed -n '900,935p' pkg/scheduler/scheduler.go
echo "== imports in scheduler_test.go =="
sed -n '1,80p' pkg/scheduler/scheduler_test.go
echo "== Prometheus go client metrics collector API imports/docs from module cache if present =="
go env GOPATH GOMODCACHE 2>/dev/null || true
find "$(go env GOMODCACHE 2>/dev/null)/github.com/prometheus/client_golang@*" -path '*prometheus.go' -o -path '*metric.go' -o -path '*testutil*' 2>/dev/null | head -100 || trueRepository: Project-HAMi/HAMi
Length of output: 7689
🌐 Web query:
prometheus/client_golang testutil CollectAndCount collector count zero values histogram
💡 Result:
The prometheus/client_golang testutil function CollectAndCount is designed to count the number of metrics (specifically, the number of metric children) produced by a collector, rather than the values of those metrics [1][2][3]. When using CollectAndCount with a Histogram, it is important to understand the following: 1. Counting Metric Children, Not Values: CollectAndCount returns the number of metric series (children) [1][4]. It does not aggregate or report the actual observed values stored in buckets, the count, or the sum of a histogram [3]. For example, if you have a single Histogram collector, it will typically contribute multiple metric series to the output (e.g., individual bucket counts, the _count, and the _sum) [5][6]. 2. Histogram Internal Structure: In Prometheus, a single Histogram is represented by several internal metrics (buckets, _sum, and _count) [5][6]. Consequently, CollectAndCount will return the total count of these individual metric elements rather than returning zero or the histogram's observed value [5][6]. 3. Intended Use: CollectAndCount is intended to verify the presence or quantity of registered metric series (often used for vectors where labels determine how many series exist) [2][7][3]. It should not be used to assert the value of a histogram [3]. To verify specific values within a histogram, you should use testutil.CollectAndCompare to match the entire output against a golden file or string, or use other testutil functions like ToFloat64 for simple gauge/counter values [2][7][3]. In summary, CollectAndCount does not ignore zero values; it simply counts the metric series instances themselves regardless of their current numerical value [1][3]. If you are seeing counts that seem unexpectedly high for a histogram, it is because you are counting the individual buckets and metadata series that constitute the histogram [5][6].
Citations:
- 1: https://github.com/prometheus/client_golang/blob/main/prometheus/testutil/testutil.go
- 2: prometheus/client_golang@9e9cc00
- 3: https://stackoverflow.com/questions/65341912/prometheus-collectandcount-always-returns-1
- 4: https://github.com/prometheus/client_golang/blob/master/prometheus/testutil/testutil.go
- 5: Bug: testutil package unexpected results for Histograms prometheus/client_golang#498
- 6: Improve CollectAndCount prometheus/client_golang#753
- 7: https://github.com/prometheus/client_golang/blob/main/prometheus/testutil/testutil_test.go
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate prometheus testutil source in module cache =="
MODGLOB="$(go env GOMODCACHE)/github.com/prometheus/client_golang@*)"
find $MODGLOB -path '*/testutil/testutil.go' -print -quit 2>/dev/null || true
find $MODGLOB -path '*/prometheus.go' -print | sed -n '1,20p' 2>/dev/null || true
find $MODGLOB -path '*/metric.go' -print | sed -n '1,20p' 2>/dev/null || true
echo "== inspect testutil implementations =="
for f in $(find $MODGLOB -path '*/testutil/testutil.go' -print 2>/dev/null); do
echo "--- $f"
rg -n -C 12 'func CollectAndCount|func ToFloat64|type Histogram|func Histogram' "$f" || true
done
echo "== inspect prometheus histogram/observable implementations =="
for f in $(find $MODGLOB -path '*/prometheus/histogram.go' -print -o -path '*/prometheus/observable.go' -print 2>/dev/null | sed -n '1,5p'); do
echo "--- $f"
rg -n -C 8 'type .*Histogram|func NewHistogram|HistogramVec|HistogramOpts|WithLabelValues|labelVals' "$f" || true
doneRepository: Project-HAMi/HAMi
Length of output: 512
Assert the histogram count increment.
testutil.CollectAndCount(schedulingDuration) > 0 checks that the registered histogram can collect metric series. Use schedulingDuration.WithLabelValues(...) / the default labels and assert that _count increases after s.Filter(args) returns.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/scheduler/scheduler_test.go` around lines 2339 - 2340, Update the test
around schedulingDuration and s.Filter(args) to capture the histogram’s _count
before and after the filter call using schedulingDuration.WithLabelValues(...)
or its default labels, then assert that the count increases. Replace the
existing CollectAndCount(schedulingDuration) > 0 assertion, which only verifies
collection rather than recording.
| func init() { | ||
| _ = prometheus.Register(schedulingDuration) | ||
| _ = prometheus.Register(schedulingAttempts) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'hami_scheduler_(scheduling_duration_seconds|scheduling_attempts_total)|prometheus\.(Register|MustRegister)|promhttp\.(Handler|HandlerFor)|Default(Registerer|Gatherer)|NewRegistry' --glob '*.go' .Repository: Project-HAMi/HAMi
Length of output: 1167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- scheduler.go relevant section ---\n'
sed -n '1,120p' pkg/scheduler/scheduler.go | cat -n
printf '\n--- cmd/scheduler/metrics.go relevant section ---\n'
sed -n '400,445p' cmd/scheduler/metrics.go | cat -n
printf '\n--- all go metric declarations with hami_scheduler ---\n'
rg -n 'hami_scheduler_(scheduling_duration_seconds|scheduling_attempts_total)|schedulingDuration|schedulingAttempts|NewVec|NewGauge|NewHistogram' --glob '*.go' .Repository: Project-HAMi/HAMi
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- scheduler.go relevant section ---'
sed -n '1,120p' pkg/scheduler/scheduler.go | cat -n
echo
echo '--- cmd/scheduler/metrics.go relevant section ---'
sed -n '400,445p' cmd/scheduler/metrics.go | cat -n
echo
echo '--- all go metric declarations with hami_scheduler ---'
rg -n 'hami_scheduler_(scheduling_duration_seconds|scheduling_attempts_total)|schedulingDuration|schedulingAttempts|NewVec|NewGauge|NewHistogram' --glob '*.go' .Repository: Project-HAMi/HAMi
Length of output: 8290
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- go.mod modules for prometheus ---'
rg -n 'github.com/prometheus/client_golang|go 1\.' go.mod
echo
echo '--- metrics usage around pkg/scheduler/scheduler.go scheduling counters ---'
sed -n '900,1035p' pkg/scheduler/scheduler.go | cat -n
echo
echo '--- imports used by pkg/scheduler/scheduler.go tests ---'
sed -n '1,40p' pkg/scheduler/scheduler_test.go | cat -nRepository: Project-HAMi/HAMi
Length of output: 8121
🌐 Web query:
Prometheus Go client RegisterAlreadyRegisteredError Register returns collector registry
💡 Result:
In the Prometheus Go client, the Register method of a Registerer (or the top-level prometheus.Register function) returns an AlreadyRegisteredError if you attempt to register a Collector that has already been registered, or if you register a different Collector that describes the same metrics (i.e., conflicts in metric descriptors) [1][2][3]. The AlreadyRegisteredError is designed to allow developers to handle this scenario gracefully rather than panicking [4][3]. The error object contains both the ExistingCollector and the NewCollector that failed registration [1][2]. The standard pattern for handling this error is to use the errors.As function to detect an AlreadyRegisteredError and, if present, switch to using the existing collector that was already registered [5]. Example usage: reqCounter:= prometheus.NewCounter(prometheus.CounterOpts{ Name: "requests_total", Help: "The total number of requests served.", }) if err:= prometheus.Register(reqCounter); err!= nil { are:= &prometheus.AlreadyRegisteredError{} if errors.As(err, are) { // A collector for that metric has been registered before. // Use the old collector from now on. reqCounter = are.ExistingCollector.(prometheus.Counter) } else { // Something else went wrong! panic(err) } } reqCounter.Inc Key considerations: - MustRegister: Unlike Register, the MustRegister method panics if an error (including AlreadyRegisteredError) occurs during registration [4][3]. - Unchecked Collectors: A Collector whose Describe method yields no descriptors is considered "unchecked" and registration will always succeed without performing duplicate checks [1][2]. - Wrapped Registries: When using wrapped registerers (e.g., via WrapRegistererWith), handling AlreadyRegisteredError can be more complex because the ExistingCollector may be wrapped, making direct type assertions to the original collector type potentially difficult [6][7]. - Metric Consistency: The registry performs these checks to ensure metric consistency according to the Prometheus data model, detecting conflicts at registration (startup) time rather than at scrape time [3].
Citations:
- 1: https://github.com/prometheus/client_golang/blob/main/prometheus/registry.go
- 2: https://github.com/prometheus/client_golang/blob/master/prometheus/registry.go
- 3: https://github.com/prometheus/client_golang/blob/master/prometheus/doc.go
- 4: https://pkg.go.dev/github.com/prometheus/client_golang/prometheus
- 5: https://github.com/prometheus/client_golang/blob/main/prometheus/examples_test.go
- 6: Wrapped Register does not play well with AlreadyRegisteredError prometheus/client_golang#605
- 7: https://github.com/prometheus/client_golang/blob/main/prometheus/wrap.go
Handle Prometheus registration errors.
prometheus.Register returns AlreadyRegisteredError when a different collector describes the same metric. Ignoring the errors on lines 75-76 lets the scheduler continue updating local collectors while a different collector may be exposed. Handle this error case by using the existing collector, or fail startup explicitly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/scheduler/scheduler.go` around lines 74 - 77, Update init around
schedulingDuration and schedulingAttempts registration to handle
prometheus.Register errors instead of discarding them: when an
AlreadyRegisteredError is returned, replace the local collector with the
existing registered collector, and explicitly fail startup for other
registration errors.
| klog.V(1).InfoS("Pod does not request any resources", | ||
| "pod", args.Pod.Name) | ||
| s.recordScheduleFilterResultEvent(args.Pod, EventReasonFilteringFailed, "", fmt.Errorf("does not request any resource")) | ||
| schedulingAttempts.WithLabelValues("failed", "no_resource_requested").Inc() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Record outcomes from filterSimulation.
This counter is incremented before the args.Nodes != nil branch, but filterSimulation returns successful assignments, no-fit results, and errors without incrementing schedulingAttempts. The total therefore omits this scheduling path and its failure reasons. Add outcome increments to each filterSimulation return, or centralize the accounting in Filter.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/scheduler/scheduler.go` at line 940, Update the scheduling outcome
accounting around filterSimulation and Filter so every filterSimulation result
increments schedulingAttempts, including successful assignments, no-fit results,
and errors with their appropriate outcome labels; avoid double-counting the
existing no_resource_requested path.
|
these metrics register to the global default registry via prometheus.Register(), but cmd/scheduler/metrics.go serves /metrics from its own separate prometheus.NewRegistry(), so they'd never actually show up in scrapes. pls understand the codes and read your codes first. |
2 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Currently,
pkg/scheduler/scheduler.godoes not record any Prometheus duration histograms or attempt counters during the Pod scheduling loop.In this PR we added control plane telemetry to
pkg/scheduler:hami_scheduler_scheduling_duration_seconds(Histogram): Measures execution duration ofFilter()andScore()scheduling decisions in seconds.hami_scheduler_scheduling_attempts_total(Counter): Tracks total Pod scheduling attempts labeled byresult("success"/"failed") andreason.This brings HAMi's scheduler extender in line with standard Kubernetes
kube-schedulermetrics conventions (scheduler_scheduling_attempt_duration_seconds).Which issue(s) this PR fixes:
Fixes #2341
AI Assistance Disclosure:
I used AI assistance to analyze codebase patterns and structure tests and format the PR description, but all changes were manually inspected, written, and verified.
Special notes for your reviewer:
Unit test
TestSchedulingMetricsRecordingadded inpkg/scheduler/scheduler_test.goand verified 100% PASS.Does this PR introduce a user-facing change?:
Summary by CodeRabbit