deprecation: remove labels marked as to be deprecated - #2021
Conversation
Signed-off-by: Jifei Wang <jifei.wang@dynamia.ai>
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe pull request shortens the ChangesDevice Memory Metric Label Change
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 simplifies the ctrDeviceMemorydesc Prometheus metric by removing deprecated labels. However, this change introduces a critical runtime error because the metric collection code still passes the old label array with nine elements instead of the updated five-element array, which will cause the Prometheus client to fail. This mismatch needs to be resolved by updating the arguments passed to the metric sender.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
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. 🚀 New features to boost your workflow:
|
Signed-off-by: Jifei Wang <jifei.wang@dynamia.ai>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/vGPUmonitor/metrics.go (1)
111-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHelp text still says "breakdown" but labels no longer carry breakdown detail.
The descriptor's label set was reduced to base container labels (no context/module/buffer/offset), so this metric now reports only a total value, not a breakdown. The help string
Container device memory usage breakdown in bytesis misleading for consumers reading/metricsoutput; the actual breakdown now only exists on the legacy metric.💡 Suggested wording fix
ctrDeviceMemorydesc = prometheus.NewDesc( "hami_container_device_memory_bytes", - `Container device memory usage breakdown in bytes`, + `Container device memory usage in bytes`, []string{"namespace", "pod", "container", "vdevice_index", "device_uuid"}, nil,🤖 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 111 - 114, The help text for the metric descriptor in ctrDeviceMemorydesc is misleading because the label set now only exposes base container labels and no longer represents a breakdown. Update the prometheus.NewDesc help string to describe the metric as a total device memory usage value rather than a breakdown, keeping the labels and behavior in sync with what /metrics actually exposes.
🤖 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.
Nitpick comments:
In `@cmd/vGPUmonitor/metrics.go`:
- Around line 111-114: The help text for the metric descriptor in
ctrDeviceMemorydesc is misleading because the label set now only exposes base
container labels and no longer represents a breakdown. Update the
prometheus.NewDesc help string to describe the metric as a total device memory
usage value rather than a breakdown, keeping the labels and behavior in sync
with what /metrics actually exposes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5163f09e-3dca-4a83-8e63-584f3ef52a69
📒 Files selected for processing (1)
cmd/vGPUmonitor/metrics.go
|
the help text still says "breakdown in bytes" but the breakdown labels are gone now. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, FouoF 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 |
3 participants
What type of PR is this?
/kind deprecation
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Summary by CodeRabbit