feat(scheduler): autoscaling gauges via metrics sampler (PR 2 M5b)#1580
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces a periodic metrics sampler to the ChangesScheduler Metrics Sampling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 introduces a metrics sampler task to the priority scheduler that periodically refreshes point-in-time capacity and autoscaling gauges (such as in-flight requests, queue depth, utilization, queue size limits, and class capacity pressure) off the hot admission path. This ensures that the hot path only performs cheap counter increments. Additionally, corresponding Prometheus metrics, trait methods, and unit tests have been added. There are no review comments, so I have no feedback to provide.
CatherineSue
force-pushed
the
feat/scheduler-autoscaling-metrics
branch
from
a82a6db to
8ff44b9
Compare
slin1237
force-pushed
the
feat/scheduler-autoscaling-metrics
branch
from
8ff44b9 to
7c3e479
Compare
slin1237
force-pushed
the
feat/scheduler-metrics
branch
from
22b1a2e to
7ae2c28
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
|
|
||
| #[test] | ||
| fn test_class_pressure_takes_worse_of_queue_and_slot() { | ||
| // preempt_settings reserves 0 for every class, so higher_reserved=0 | ||
| // and slot pressure is inflight/capacity. | ||
| let sched = PriorityScheduler::new(&preempt_settings(), 100).unwrap(); | ||
| // Queue pressure (8/10) dominates slot pressure (10/100). | ||
| assert!((sched.class_pressure(Class::Default, 10, 8, 10, 100) - 0.8).abs() < 1e-9); | ||
| // Slot pressure (50/100) dominates queue pressure (1/10). | ||
| assert!((sched.class_pressure(Class::Default, 50, 1, 10, 100) - 0.5).abs() < 1e-9); | ||
| // Clamped to 1.0 when oversubscribed. | ||
| assert!((sched.class_pressure(Class::Default, 200, 100, 10, 100) - 1.0).abs() < 1e-9); | ||
| // No queue limit and no inflight → zero, no div-by-zero. | ||
| assert_eq!(sched.class_pressure(Class::Bulk, 0, 0, 0, 100), 0.0); | ||
| } | ||
|
|
||
| #[tokio::test(start_paused = true)] | ||
| async fn test_preempt_declined_when_only_victim_past_ttft() { | ||
| // The single lower-class inflight has already emitted its first |
There was a problem hiding this comment.
🟡 Nit: All four assertions use preempt_settings() which reserves 0 for every class, so higher_reserved is always 0 and the headroom calculation simplifies to capacity.max(1). A test case using settings with non-zero reservations (e.g. Interactive reserved=20, System reserved=10) would exercise the higher_reserved subtraction path in class_pressure — that's the most nuanced part of the formula.
|
Hi @CatherineSue, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
Per design §9 capacity/autoscaling table. A sampler task (spawn_sampler,
5s interval, holds only a Weak<Self>) refreshes the point-in-time gauges
off the admission path so the hot path stays counter-only:
- smg_scheduler_inflight{class}
- smg_scheduler_queue_depth{class}
- smg_scheduler_utilization (Σ inflight / capacity)
- smg_scheduler_queue_size_limit{class}
- smg_scheduler_class_capacity_pressure{class} — max of queue pressure
(depth/limit) and slot pressure (overflow past reservation over the
headroom not reserved by higher classes), clamped to 0.0-1.0
Adds ClassQueue::capacity() for the queue-size-limit gauge and a unit
test pinning the class_pressure formula.
Deferred (documented in the deviations doc): per-tenant gauges
(scheduler_tenant_inflight/queued) need tenant threaded onto every
inflight handle + waiter — disproportionate for a secondary metric — and
scheduler_queue_wait_p95_seconds is derivable from the queue-wait
histogram via histogram_quantile in PromQL.
Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
slin1237
force-pushed
the
feat/scheduler-autoscaling-metrics
branch
from
7c3e479 to
993a7a8
Compare
mergify
Bot
removed
the
needs-rebase
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 993a7a85bc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let overflow = | ||
| u32::from(inflight).saturating_sub(u32::from(self.slot_pool.reserved(class))); | ||
| let slot_pressure = f64::from(overflow) / f64::from(headroom); |
There was a problem hiding this comment.
This slot-pressure calculation only uses this class's own in-flight count, so it can report near-zero pressure while the class has no admissible slots because sibling classes are consuming the shared pool. For example, with capacity 100, System reserved 20, and Default using the other 80 slots, slots_available_to(..., Class::Bulk) is 0, but Bulk gets overflow = 0 here and pressure is driven only by depth / limit; with a large queue limit the autoscaling gauge can stay very low even though Bulk is completely capacity-blocked. The sampler should mirror the admission headroom calculation (total used plus unused higher reservations) rather than considering only same-class inflight.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
2 participants
Description
Problem
Autoscalers need point-in-time signals (utilization, per-class pressure) to drive scale up/down decisions. M5a added the operational counters; this adds the capacity/autoscaling gauges.
Solution
A sampler task (5s interval, holds only a
Weak<Self>) refreshes the gauges off the admission path, so the hot path stays counter-only. Stacked on #1579.Changes
spawn_sampler+sample_metrics+class_pressureonPriorityScheduler; sampler spawned fromAdmissionMode::from_configalongside the dispatcher.smg_scheduler_inflight{class},smg_scheduler_queue_depth{class}smg_scheduler_utilization(Σ inflight / capacity)smg_scheduler_queue_size_limit{class}smg_scheduler_class_capacity_pressure{class}—max(depth/limit, max(0, inflight−reserved)/max(1, capacity−Σ_higher_reserved)), clamped 0.0–1.0ClassQueue::capacity()accessor for the queue-size-limit gauge.class_pressureformula (queue-dominates, slot-dominates, clamp, div-by-zero guard).Deferred (documented in the deviations doc)
scheduler_tenant_inflight/queued): require threading atenantfield through everyInflightHandle+Waiter+ theadmit/register_inflightsignatures (~20 test call-site edits) for a secondary "noisy-tenant" metric. Deferred pending sign-off on the invasiveness.scheduler_queue_wait_p95_seconds: derivable from thesmg_scheduler_queue_wait_secondshistogram viahistogram_quantilein PromQL; the standalone sliding-window gauge isn't worth a bespoke estimator.Test Plan
cargo test -p smg --lib scheduler::— 108 unit tests green (incl. the newclass_pressuretest).Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit