feat(scheduler): add filter_duration_seconds and filter_denials_total… - #2459
feat(scheduler): add filter_duration_seconds and filter_denials_total…#2459Rickydama3 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: leondon3 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe scheduler routes package adds filter duration and denial metrics. Predicate filtering records duration by node and result, and denial counts by node and reason. Scheduler metrics initialization registers these metrics with the Prometheus registry. ChangesScheduler filter metrics
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
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/routes/metrics.go`:
- Line 30: Update the bucket configuration in the metrics definition to use a
finite range ending at 10 seconds, such as ExponentialBucketsRange with 0.01 as
the minimum and 10 as the maximum across 10 buckets; preserve the existing
metric and bucket count.
In `@pkg/scheduler/routes/route.go`:
- Line 76: Declare err in the surrounding scope before the extender filter call,
then update the assignment in the Filter flow to use that declaration while
preserving the existing error handling.
- Around line 87-89: Update the failed-node metrics handling in the scheduler
route to translate each raw Filter failure reason into a bounded hami.io denial
code before passing it to FilterDenials.WithLabelValues. Preserve the existing
failedNode label and duration observation, and ensure device errors, node-unfit
messages, and node-unregistered messages map to their corresponding stable codes
rather than raw text.
🪄 Autofix
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: 80df2507-7ab3-4f3c-89df-3df2602aa0dc
📒 Files selected for processing (3)
cmd/scheduler/metrics.gopkg/scheduler/routes/metrics.gopkg/scheduler/routes/route.go
… metrics Signed-off-by: Ricky Dama <rickydama2006@gmail.com>
39a09fb to
e8fba4f
Compare
|
Thanks for the observability proposal. Under the default Chart path, Filter returns NodeNames, but this implementation reads only Nodes.Items, so successful/allowed metrics are not recorded. It also observes the latency of one Filter call once per candidate node and derives rejection reasons from message strings, without behavior tests or hosted CI. These values would be misleading as a public metric contract, so we are closing the current PR. A replacement should begin with exact event/value semantics and exposition tests. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds two new Prometheus metrics to the HAMi scheduler to track the latency and outcomes of the filtering phase:
hami_scheduler_filter_duration_seconds(Histogram)hami_scheduler_filter_denials_total(Counter)These metrics are essential for giving operators visibility into scheduling latency and error rates (e.g., debugging contention issues like #1662).
Fix/Implementation:
pkg/scheduler/routes/metrics.goto define the new metrics.time.Now()capture inroute.go:PredicateRouteright befores.Filter(extenderArgs)is called.extenderFilterResult.FailedNodesmap to correctly increment the denials counter with the specific node andreasonlabel.cmd/scheduler/metrics.go.Which issue(s) this PR fixes:
Part of #2126
Fixes #2458
Special notes for your reviewer:
This fulfills a major requirement of the LFX Mentorship observability gap analysis (#2126). The histogram buckets are configured exponentially from 10ms to 10s to accurately capture typical scheduling latency profiles.
Does this PR introduce a user-facing change?: