fix(metrics): resolve high-cardinality leak in memory_allocated_bytes - #2142
Conversation
|
Welcome @Aryanbhargava18! 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:
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 (2)
📝 WalkthroughWalkthroughGPU allocation and overview metrics now use aligned label sets in current and legacy modes. Overview metrics no longer expose shared-container labels, allocation labels use total device cores, and metric documentation lists the updated core and memory-limit labels. ChangesGPU metric label alignment
Estimated code review effort: 1 (Trivial) | ~2 minutes 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 |
1f22126 to
2aff8e7
Compare
Signed-off-by: Aryanbhargava18 <aryanbhargava644@gmail.com>
2aff8e7 to
4d93beb
Compare
|
Better remove these changing labels. Actually other metrics can fully replace them. |
32a4d76 to
ceb69cd
Compare
|
Makes sense. I've completely removed the |
Signed-off-by: Aryanbhargava18 <aryanbhargava644@gmail.com>
ceb69cd to
4760bda
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, Aryanbhargava18 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 |
What happened
The
hami_gpu_memory_allocated_bytesmetric (and its legacy counterpartGPUDeviceMemoryAllocated) defines adevice_coreslabel but incorrectly injectsfmt.Sprint(devs.Device.Usedcores)as its value instead ofTotalcore.Because
Usedcoresis a dynamic state variable (changing every time a pod is scheduled or unscheduled), this causes a severe Prometheus High Cardinality Leak. Every time the used cores change, Prometheus is forced to create a completely new time series, abandoning the old one. On a cluster with high pod churn, this combinatorial explosion of stale time series will eventually cause Prometheus OOMs.What you expected to happen
Prometheus best practices dictate that labels must represent stable identity attributes, not dynamic state. The
device_coreslabel should represent the static core capacity of the device (Totalcore), while the dynamic state should remain as metric values (e.g., viahami_gpu_core_allocated_ratio).How to reproduce
:31993/metrics.hami_gpu_memory_allocated_bytes.hami_gpu_memory_allocated_byteswith a differentdevice_coreslabel, instead of updating the existing series.Root cause
In
cmd/scheduler/metrics.go:Fix
Changed
devs.Device.Usedcorestodevs.Device.Totalcorewhen populating thedevice_coreslabel, stabilizing the time series identity.Note: This aligns directly with the gap analysis currently open in #2126 regarding metric cardinality risks.
Summary by CodeRabbit
device_coreslabel now reflects total device cores (while the underlying memory allocation value remains based on used memory).device_cores/device_memory_limitlabels now report total cores and memory limits.