fix(metrics): include all metric descriptors in Describe - #2248
fix(metrics): include all metric descriptors in Describe#2248veyron-kairo wants to merge 2 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: veyron-kairo 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 |
|
Welcome @veyron-kairo! It looks like this is your first PR to Project-HAMi/HAMi 🎉 |
|
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)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesPrometheus descriptor contract
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
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 |
Signed-off-by: Shridhar Panigrahi <198173519+veyron-kairo@users.noreply.github.com>
Signed-off-by: Shridhar Panigrahi <198173519+veyron-kairo@users.noreply.github.com>
![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_test.go`:
- Around line 80-90: Update TestDescribeIncludesLegacyDescriptors to assert
descriptor identities, not only the total count: build the expected set of eight
legacy descriptors using the same descriptor symbols referenced by the legacy
metrics, then verify each is present in collectDescribe(cc), following the
comparison approach used by TestDescribeCoversAllCollectedDescriptors.
🪄 Autofix (Beta)
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: 7097d066-ae5c-4aa6-99ed-9812852e4a8c
📒 Files selected for processing (2)
cmd/vGPUmonitor/metrics.gocmd/vGPUmonitor/metrics_test.go
| // TestDescribeIncludesLegacyDescriptors verifies the legacy branch still | ||
| // announces its eight additional descriptors when legacy metrics are enabled. | ||
| func TestDescribeIncludesLegacyDescriptors(t *testing.T) { | ||
| initLegacyDescriptors() | ||
| cc := ClusterManagerCollector{ClusterManager: &ClusterManager{LegacyMetrics: true}} | ||
|
|
||
| const wantCount = 11 + 8 // modern superset + legacy descriptors | ||
| if got := len(collectDescribe(cc)); got != wantCount { | ||
| t.Errorf("Describe() with legacy metrics emitted %d descriptors, want %d", got, wantCount) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the legacy descriptor identities.
The count assertion does not verify the eight required legacy descriptors. The test passes if a legacy descriptor is missing and another descriptor keeps the total at 19. Build an expected descriptor set and verify every legacy descriptor, as TestDescribeCoversAllCollectedDescriptors does for modern descriptors.
🤖 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 80 - 90, Update
TestDescribeIncludesLegacyDescriptors to assert descriptor identities, not only
the total count: build the expected set of eight legacy descriptors using the
same descriptor symbols referenced by the legacy metrics, then verify each is
present in collectDescribe(cc), following the comparison approach used by
TestDescribeCoversAllCollectedDescriptors.
22a8e59 to
6fb8613
Compare
|
Flagging that this PR overlaps with #2240, which also adds the missing descriptors to Describe(). Mine additionally includes a regression test (metrics_test.go) that fails on the pre-fix code and passes after the change, plus a correction to the outdated DescribeByCollect doc comment. Happy to defer to whichever the maintainers prefer — if #2240 is the one to merge, I'd gladly add the regression test on top of it so the fix stays covered. Just flagging the overlap to avoid duplicated review effort. 🙂 |
|
closing as a dup, #2240 was opened 5h earlier w/ the same fix and the test idea was already suggested there, pls check existing prs before opening a new one. |
2 participants
What type of PR: /kind bug
What this PR does / why: Describe() in cmd/vGPUmonitor/metrics.go omitted two descriptors that Collect() emits conditionally (hami_container_last_kernel_elapsed_seconds, hami_mig_device_info). This aligns Describe() with the Prometheus superset contract and the legacy branch, corrects a stale comment, and adds a regression test that fails on the old code and passes now.
Which issue(s) this PR fixes: Fixes #2247
Special notes for reviewer: No change to scrape output — client_golang already tolerates undescribed metrics — so this is contract hygiene, not a runtime fix. make verify and go test ./cmd/vGPUmonitor/ pass locally.
AI assistance disclosure: I used AI assistance (Claude Code) to identify this Describe/Collect gap .
Did implemented the fix and test my myself . I reviewed and understand the change, wrote the commit messages myself, ran make verify and the tests locally, and take responsibility for the PR and for addressing review feedback.
Does this PR introduce a user-facing change?: NONE
Summary by CodeRabbit
Bug Fixes
Tests