Skip to content

fix(metrics): add device_type label to node memory ratio metrics (#2370) - #2554

Merged
archlitchi merged 2 commits into
Project-HAMi:masterfrom
princexpoddar:fix/metrics-memory-ratio-missing-device-type
Aug 18, 2026
Merged

fix(metrics): add device_type label to node memory ratio metrics (#2370)#2554
archlitchi merged 2 commits into
Project-HAMi:masterfrom
princexpoddar:fix/metrics-memory-ratio-missing-device-type

Conversation

@princexpoddar

@princexpoddar princexpoddar commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?
/kind bug

What this PR does / why we need it:
In the scheduler's Prometheus metrics in cmd/scheduler/metrics.go, hami_node_gpu_memory_allocated_ratio did not have the device_type label that every other device metric in the file carries....On running the scheduler on a node with different devices(lets say NVIDIA and Ascend) sharing the same device_index...scraping /metrics produced metric series for hami_node_gpu_memory_allocated_ratio where we cannot tell the NVIDIA and Ascend apart without already knowing which device_uuid belonged to which one.

Added device_type to nodeGPUMemoryPercentage in cmd/scheduler/metrics to be consistent with all other sibling metrics....also updated the corresponding tests in cmd/scheduler/metrics_test.go

Which issue(s) this PR fixes:
Fixes #2370

Special notes for your reviewer:

  • Verified with unit tests passing
  • AI Disclosure : Used Claude for assistance

Does this PR introduce a user-facing change?:
N/A

Summary by CodeRabbit

  • Enhancements
    • Added a device_type label to node-level and legacy GPU memory utilization metrics.
    • Prometheus metric samples now identify the type of device associated with reported memory usage.
    • This provides clearer visibility into GPU memory allocation across different device types.

@hami-robot hami-robot Bot added kind/bug Something isn't working dco-signoff: yes labels Aug 10, 2026
@hami-robot hami-robot Bot added the size/S label Aug 10, 2026
@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The scheduler now adds a device_type label to the node GPU memory-percentage metric. Tests update the modern metric expectation to include device_type="NVIDIA".

Changes

GPU memory metric labels

Layer / File(s) Summary
Add device type label to memory metrics
cmd/scheduler/metrics.go, cmd/scheduler/metrics_test.go
The metric descriptor and emitted samples now include the device type. The modern metric test expects the NVIDIA label, while the legacy expectation remains unchanged.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Possibly related PRs

Suggested reviewers: archlitchi, fouof

Poem

A rabbit checks the metric line,
The device type now shines.
NVIDIA joins the sample bright,
Tests confirm the label is right.
Hop, hop, metrics align!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The standard metric is updated, but the required legacy nodeGPUMemoryPercentage metric does not retain the device_type label from issue #2370. Update nodeGPUMemoryPercentage and its tests to include the device_type label, or document an approved exception to the issue requirement.
✅ Passed checks (4 passed)
Check name Status Explanation
Out of Scope Changes check ✅ Passed The changes are limited to the scheduler metric label and its corresponding test expectation, with no unrelated code changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main metric label change covered by the pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai
coderabbitai Bot requested a review from FouoF August 10, 2026 20:49
@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
cmd/scheduler/metrics.go 50.00% 0 Missing and 1 partial ⚠️
Flag Coverage Δ
unittests 62.36% <50.00%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
cmd/scheduler/metrics.go 77.62% <50.00%> (+1.39%) ⬆️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mesutoezdil mesutoezdil left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left one small question inline.

Comment thread cmd/scheduler/metrics.go
…yPercentage

Signed-off-by: Prabal Poddar <prabalpoddar73@gmail.com>
@mesutoezdil

Copy link
Copy Markdown
Contributor

legacyMemoryPercentage (metrics.go:190, sendLegacyMetric call at 277) still has no device_type. pr body says both got fixed, only the new metric did.

@princexpoddar

Copy link
Copy Markdown
Contributor Author

as per your suggestion, to preserve backward compatibility for legacy dashboards which expect a fixed label set...i removed the changes from legacyMemoryPercentage while keeping the new metric changes as it is. I have updated the outdated PR body

@archlitchi archlitchi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@hami-robot

hami-robot Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: archlitchi, princexpoddar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@hami-robot hami-robot Bot added the approved label Aug 18, 2026
@archlitchi
archlitchi merged commit 68abfa7 into Project-HAMi:master Aug 18, 2026
16 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(metrics): hami_node_gpu_memory_allocated_ratio and its legacy equivalent are missing device_type label

3 participants