feat: add hami_host_gpu_memory_controller_utilization_ratio metric - #2615
feat: add hami_host_gpu_memory_controller_utilization_ratio metric#2615manoj-1407 wants to merge 2 commits into
Conversation
GetUtilizationRates() returns both Gpu (SM) and Memory (memory controller)
utilization. hami_host_gpu_utilization_ratio already exports util.Gpu.
util.Memory was fetched but silently discarded.
Add hami_host_gpu_memory_controller_utilization_ratio with identical label
set {device_index, device_uuid, device_type}. SM and memory controller
utilization diverge on memory-bandwidth-bound workloads (LLM inference),
making this metric independently useful for alerting on memory bus saturation.
Signed-off-by: G. Manoj Kumar <manojkumar148700@gmail.com>
Signed-off-by: manoj-1407 <manojkumar148700@gmail.com>
…er utilization metric - TestDescribeRegistersMemoryControllerUtilization: verifies hami_host_gpu_memory_controller_utilization_ratio appears in Describe() - TestCollectMemoryControllerUtilizationValue: checks the actual gauge value emitted via the descriptor, covering the sendMetric call flagged by codecov Signed-off-by: manoj-1407 <manojkumar148700@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: manoj-1407 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 |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ 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 |
|
Caution CodeRabbit couldn't update its existing comment. The review summary may be out of date. Error details |
![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_test.go (1)
82-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the collector path in this test.
This test constructs a metric directly from
hostGPUMemoryUtilizationdesc. It does not callClusterManagerCollector.collectGPUUtilizationMetricsor gather fromClusterManagerCollector. It would pass if production code omittedutil.Memory, emittedutil.Gpu, or supplied incorrect labels. Drive the collector with a knownutil.Memoryvalue, then assert the metric name, gauge value, and all three labels.🤖 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_test.go` around lines 82 - 103, Update TestCollectMemoryControllerUtilizationValue to exercise ClusterManagerCollector.collectGPUUtilizationMetrics or gather from ClusterManagerCollector using a fixture with a known util.Memory value. Assert the emitted metric’s name, gauge value, and all three labels, ensuring the collector maps util.Memory correctly instead of constructing the metric directly from hostGPUMemoryUtilizationdesc.
🤖 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_test.go`:
- Around line 82-103: Update TestCollectMemoryControllerUtilizationValue to
exercise ClusterManagerCollector.collectGPUUtilizationMetrics or gather from
ClusterManagerCollector using a fixture with a known util.Memory value. Assert
the emitted metric’s name, gauge value, and all three labels, ensuring the
collector maps util.Memory correctly instead of constructing the metric directly
from hostGPUMemoryUtilizationdesc.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 37b5db10-a282-4047-9056-7b57e6895283
📒 Files selected for processing (2)
cmd/vGPUmonitor/metrics.gocmd/vGPUmonitor/metrics_test.go
1 participant
Used Claude to help spot the gap, went through the code myself.
What type of PR is this?
/kind feature
What this PR does / why we need it:
GetUtilizationRates() already returns util.Memory alongside util.Gpu
but we were just throwing it away. added it as
hami_host_gpu_memory_controller_utilization_ratio — zero extra NVML
calls since the data was already there. useful for LLM inference
workloads where memory bandwidth saturates before SM does.
Which issue(s) this PR fixes:
Fixes #2613
Special notes for your reviewer:
tiny change — one descriptor, one Describe line, one sendMetric call.
Does this PR introduce a user-facing change?:
yes — hami_host_gpu_memory_controller_utilization_ratio gauge
Summary by CodeRabbit
New Features
Bug Fixes