fix: add missing metric descriptors to Describe() in vGPUmonitor - #2240
Conversation
|
Welcome @devGPP23! It looks like this is your first PR to Project-HAMi/HAMi 🎉 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe collector’s ChangesMetrics descriptor registration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
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 |
|
lgtm. you can also consider adding a unit test using |
mesutoezdil
left a comment
mesutoezdil
left a comment
There was a problem hiding this comment.
pls add "fixes #2247" to the desc so it auto closes.
9a4f864 to
76a8f6f
Compare
|
Thanks for the reviews @mesutoezdil @Eshiv-Pandey Added fixes #2247 to the PR description so the issue auto-closes. |
![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_test.go`:
- Line 1: Add the repository-standard Apache 2.0 license header at the beginning
of metrics_test.go, before the package main declaration, matching the formatting
used by other Go files.
- Around line 10-12: Update the pedantic registry tests around
NewPedanticRegistry to populate deterministic fixture data, including PodLister
and containerLister, then invoke reg.Gather() and regLegacy.Gather() after
registration. Assert or propagate gathering errors so Collect output is
validated against the declared 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: 09168c65-89a1-485b-9f51-60c1a5e2ed4c
📒 Files selected for processing (2)
cmd/vGPUmonitor/metrics.gocmd/vGPUmonitor/metrics_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/vGPUmonitor/metrics.go
76a8f6f to
7f3d7c4
Compare
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_test.go`:
- Around line 19-28: Reorder the import block in metrics_test.go so
standard-library imports come first, followed by external dependencies such as
Prometheus and Kubernetes, with the github.com/Project-HAMi/HAMi imports last;
apply the grouping used by goimports with the HAMi module configured as local.
- Around line 35-36: Update the test setup around NodeNameEnvName to register
cleanup that restores its prior environment value after the test, rather than
leaving os.Setenv’s process-wide change in place. Use the test’s cleanup
mechanism and preserve whether the variable was originally unset.
🪄 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: 301fe3d4-ed3b-43f0-835e-f8598f28439d
📒 Files selected for processing (2)
cmd/vGPUmonitor/metrics.gocmd/vGPUmonitor/metrics_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/vGPUmonitor/metrics.go
7f3d7c4 to
9644bce
Compare
The Describe() method is missing ctrDeviceLastKernelDesc and ctrDeviceMigInfo descriptors that Collect() emits. This violates the prometheus.Collector interface contract. The legacy metrics branch correctly includes both, confirming this is an oversight. Signed-off-by: devGP7 <gauravpatil232005@gmail.com>
9644bce to
4ef1fc0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 31 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
/lgtm |
|
/lgtm cancel |
Signed-off-by: devGP7 <gauravpatil232005@gmail.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, devGPP23 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
I was reading the code to learn how
HAMisends GPU metrics to Prometheus. I noticed a small bug in theDescribe()method.In Prometheus,
Describe()must list all the metrics that the program can send. If the program sends a metric that is not listed inDescribe(), it is a rule violation.I checked all the metrics that
HAMisends in theCollect()method. I found that two metrics are sent but not listed inDescribe():ctrDeviceLastKernelDesc(sent at line 554)ctrDeviceMigInfo(sent at line 602)I know this is a issue/mistake because the older legacy metrics code (lines 237-238) includes both of these. The new code just forgot them.
Right now, this doesn't break the app because it uses a relaxed setting (
prometheus.NewRegistry()). But if a developer tries to use the strict setting (prometheus.NewPedanticRegistry(), which is commented out inmain.go), the app will crash.I fixed this by adding the two missing metrics to
Describe().How Has This Been Tested?
I checked the code to make sure every metric sent by
Collect()is now listed inDescribe(). I did not need to add any new files or imports because the variables were already defined.AI Assistance Disclosure
I used AI tools to help me find this bug while I was reading the code. All the text, code changes, and commit messages are my own work. I understand every line of this PR.
fixes #2247
Summary by CodeRabbit
Summary by CodeRabbit