fix(metrics): remove changing values like 'limit' from metric labels to prevent Prometheus crash - #2392
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: devGPP23 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 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 scheduler publishes quota limits separately from quota usage. The vGPU monitor removes extra labels from legacy container device-memory metrics. Tests verify both updated label sets. ChangesMetric label cleanup
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: 1
🤖 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`:
- Line 470: Remove the now-unused memoryOffset declaration and calculation near
the sendLegacyMetric call in the affected metrics function, leaving the
memoryTotal metric emission unchanged.
🪄 Autofix
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: ea4b6a58-837a-4d98-8b14-052dbfe04339
📒 Files selected for processing (4)
cmd/scheduler/metrics.gocmd/scheduler/metrics_test.gocmd/vGPUmonitor/metrics.gocmd/vGPUmonitor/metrics_test.go
c6fb49a to
3a24547
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
3a24547 to
a8f6b48
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
a8f6b48 to
81f15c3
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Eshiv-Pandey
left a comment
Eshiv-Pandey
left a comment
There was a problem hiding this comment.
remove blank lines and run gofmt locally
| "QuotaUsed", | ||
| "resourcequota usage for a certain device", | ||
| []string{"quotanamespace", "quotaName", "limit"}, nil, | ||
| []string{"quotanamespace", "quotaName"}, nil, |
There was a problem hiding this comment.
--legacy-metrics is for backward compat ig, so dropping limit here breaks old dashboards, and there's no legacy version of the new hami_resource_quota_limit. Keep this as-is limit the cleanup to hami_resource_quota_used
| "Device_memory_desc_of_container", | ||
| "Container device memory description", | ||
| []string{"podnamespace", "podname", "ctrname", "vdeviceid", "deviceuuid", "context", "module", "data", "offset"}, nil, | ||
| []string{"podnamespace", "podname", "ctrname", "vdeviceid", "deviceuuid"}, nil, |
There was a problem hiding this comment.
these were numeric values encoded as labels, so dropping them is a valid cardinality fix .. but the modern path re exposes them as separate metrics (hami_vgpu_memory_context/module/buffer_bytes) and legacy has no equivalent, so legacy consumers lose the data outright.. intended?
| desc := legacyCtrDeviceMemorydesc.String() | ||
|
|
||
| if !strings.Contains(desc, "variableLabels: [podnamespace podname ctrname vdeviceid deviceuuid]") { | ||
| t.Errorf("Expected legacyCtrDeviceMemorydesc to have exactly 5 labels, but got desc: %s", desc) |
There was a problem hiding this comment.
Desc.String() renders labels as {a,b,c} on v1.24.1, not [a b c].. this never matches, test always fails (ci has skipped this cause it failed before due to lint mostt probably). Drop the String() assertion, use testutil.CollectAndCompare like TestQuotaMetricHasNoLimitLabel
81f15c3 to
114692a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…ainer-memory metrics Signed-off-by: devGP7 <gauravpatil232005@gmail.com>
114692a to
2c878b8
Compare
2 participants
This PR fixes a high-cardinality issue in the Prometheus metrics exported by both
schedulerandvGPUmonitor.It removes dynamic values that were incorrectly being passed as metric labels, and instead exports them as standard metric values.
Why is this needed?
Exporting dynamic or configurable values (like quota
limit) as labels is a known Prometheus anti-pattern.Every time an admin updates a quota limit, Prometheus creates an entirely new time-series, leaving the old one.
In large clusters with frequent changes, this causes "Label Explosion" (High Cardinality) which consumes excessive memory and can eventually lead to Prometheus crashing (OOM kills).
To improve monitoring and dashboards , cleaning up this metric hygiene is essential so that the dashboards can run efficiently at scale.
How does this PR fix it?
limitlabel from thehami_resource_quota_usedmetric.and Created a brand new dedicated gauge
hami_resource_quota_limitto track the limit value separately.2.)vGPUmonitor Legacy Metrics: Trimmed
Device_memory_desc_of_containerfrom 9 labels down to 5.& Removed the redundant dynamic labels (
context,module,data,offset) since these values are already properly exported as standard values in the newer metrics (e.g.hami_vgpu_memory_context_bytes).Added
TestQuotaMetricHasNoLimitLabelin scheduler tests to ensure the label is successfully dropped.Added
TestLegacyCtrDeviceMemoryDescHasFiveLabelsin vGPUmonitor tests to prevent future label count regressions.Passed local tests and verified metrics output formatting.
AI Disclosure:
I used an AI assistant to help me write the code. However, the logic behind fixing the metrics, finding the root cause of the bug, and running the local tests was entirely done and verified by me to make sure everything works .
Summary by CodeRabbit