chore(metrics): remove Prometheus tutorial scaffolding and hoist scheduler descriptors - #2275
chore(metrics): remove Prometheus tutorial scaffolding and hoist scheduler descriptors#2275shivv23 wants to merge 1 commit into
Conversation
…duler descriptors Signed-off-by: shivv23 <shivamkumar0423@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: shivv23 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 collector now reuses package-level Prometheus descriptors and initializes legacy descriptors when enabled. The vGPU monitor removes unused tutorial scaffolding and updates collector documentation. ChangesMetrics collector updates
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: 2
🤖 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 `@cmd/vGPUmonitor/metrics.go`:
- Around line 41-53: Update the ClusterManager type comment to describe only the
PodLister and containerLister state it actually stores, removing the claim that
it holds informers; do not add informer fields unless required elsewhere by the
struct’s implementation.
- Around line 189-190: Update the documentation comment for the Collect method
to describe that it emits the current device, pod, and container metrics, whose
labels may change as resources change; remove the claim that every collection
returns the same metrics and descriptors.
🪄 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: 1a5b3d82-7ae9-4c20-acc3-2c9cf9bc498e
📒 Files selected for processing (3)
cmd/scheduler/metrics.gocmd/scheduler/metrics_test.gocmd/vGPUmonitor/metrics.go
| // ClusterManager models the vGPU monitoring state for a single node and holds | ||
| // the informers and listers used to discover the pods and containers running | ||
| // on that node. A custom Collector called ClusterManagerCollector collects | ||
| // information from a ClusterManager using its provided methods and turns them | ||
| // into Prometheus Metrics for collection. | ||
| type ClusterManager struct { | ||
| Zone string | ||
| // Contains many more fields not listed in this example. | ||
| // PodLister lists pods assigned to the monitored node. | ||
| PodLister listerscorev1.PodLister | ||
| containerLister *nvidia.ContainerLister | ||
| LegacyMetrics bool | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the ClusterManager documentation with the stored state.
ClusterManager stores PodLister and containerLister, but it has no informer fields. Describe the listers only, or add the informer fields if the struct should own them.
Proposed documentation fix
-// the informers and listers used to discover the pods and containers running
+// the listers used to discover the pods and containers running📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ClusterManager models the vGPU monitoring state for a single node and holds | |
| // the informers and listers used to discover the pods and containers running | |
| // on that node. A custom Collector called ClusterManagerCollector collects | |
| // information from a ClusterManager using its provided methods and turns them | |
| // into Prometheus Metrics for collection. | |
| type ClusterManager struct { | |
| Zone string | |
| // Contains many more fields not listed in this example. | |
| // PodLister lists pods assigned to the monitored node. | |
| PodLister listerscorev1.PodLister | |
| containerLister *nvidia.ContainerLister | |
| LegacyMetrics bool | |
| } | |
| // ClusterManager models the vGPU monitoring state for a single node and holds | |
| // the listers used to discover the pods and containers running | |
| // on that node. A custom Collector called ClusterManagerCollector collects | |
| // information from a ClusterManager using its provided methods and turns them | |
| // into Prometheus Metrics for collection. | |
| type ClusterManager struct { | |
| Zone string | |
| // PodLister lists pods assigned to the monitored node. | |
| PodLister listerscorev1.PodLister | |
| containerLister *nvidia.ContainerLister | |
| LegacyMetrics bool | |
| } |
🤖 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 `@cmd/vGPUmonitor/metrics.go` around lines 41 - 53, Update the ClusterManager
type comment to describe only the PodLister and containerLister state it
actually stores, removing the claim that it holds informers; do not add informer
fields unless required elsewhere by the struct’s implementation.
| // Describe sends the metric descriptors to the provided channel. The Collect | ||
| // method always returns the same metrics with the same descriptors. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
curl -fsSL \
https://raw.githubusercontent.com/prometheus/client_golang/v1.24.1/prometheus/collector.go |
grep -n -A20 -B2 'type Collector interface'Repository: Project-HAMi/HAMi
Length of output: 1454
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect the relevant comments and the actual emitted metric construction in cmd/vGPUmonitor/metrics.go.
sed -n '170,225p' cmd/vGPUmonitor/metrics.go
printf '\n--- Collect usages / metric construction evidence ---\n'
rg -n "func \\(.*Collect\\(.*chan .*Metric|Desc\\(.*chan \\*.*Desc|prometheus.New.*GaugeVec|labels|current|Devices|Pods|Containers" cmd/vGPUmonitor/metrics.goRepository: Project-HAMi/HAMi
Length of output: 5643
🏁 Script executed:
#!/usr/bin/env/bash
set -euo pipefail
# Inspect the Prometheus collector interface docs for Collect semantics in the stated version.
curl -fsSL \
https://raw.githubusercontent.com/prometheus/client_golang/v1.24.1/prometheus/collector.go |
grep -n -A25 'type Collector interface' |
sed -n '25,95p'Repository: Project-HAMi/HAMi
Length of output: 302
Describe the dynamic sample set in Collect.
Collect emits current device, pod, and container metrics, and the emitted labels change as those resources change. Update the Collect comment to say it returns current metrics rather than implying the same metrics every sample.
🤖 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 `@cmd/vGPUmonitor/metrics.go` around lines 189 - 190, Update the documentation
comment for the Collect method to describe that it emits the current device,
pod, and container metrics, whose labels may change as resources change; remove
the claim that every collection returns the same metrics and descriptors.
2 participants
/kind cleanup
What this PR does / why we need it:
Two related hygiene improvements in the metrics collectors (both requested in #2249):
cmd/vGPUmonitor/metrics.gostill shipped scaffolding from the client_golang tutorial. TheClusterManagerdoc comment described an "example" system with zones,ReallyExpensiveAssessmentOfTheSystemState()was an unused tutorial function returning made-up data (6.023e23), and a block of commented-out dead code (parseidstr,gettotalusage,getTotalUtilization) was left behind. None of it is exercised by the production monitor; the stale comments aroundDescribe/Collectreferenced the removed example system.cmd/scheduler/metrics.gore-created every metric descriptor insideCollect(). All 22prometheus.NewDesc(...)calls ran on every scrape, re-allocating identical descriptors each time. The vGPUmonitor collector already follows the recommended pattern (package-level descriptors built once); this hoists the scheduler's descriptors to package level — including the 11 legacy descriptors, which are now populated once ininitLegacyDescriptors()when legacy mode is enabled, aligning both collectors.Which issue(s) this PR fixes:
Fixes #2249
Special notes for your reviewer:
No behavior change.
go build ./cmd/scheduler/ ./cmd/vGPUmonitor/and the existing scheduler metrics unit test pass (verified via Linux CI; Windows cannot compile the CGO NVML deps locally). The legacy descriptor init moved from insideCollect()toNewClusterManager, so the unit test now callsinitLegacyDescriptors()explicitly.Does this PR introduce a user-facing change?:
This PR description was generated with the assistance of an AI coding tool. The author reviewed and verified all changes.
Summary by CodeRabbit
Performance
Documentation
Refactor