Skip to content

chore(metrics): remove leftover client_golang tutorial scaffolding - #2263

Merged
hami-robot[bot] merged 2 commits into
Project-HAMi:masterfrom
veyron-kairo:cleanup/vgpumonitor-remove-dead-code
Aug 4, 2026
Merged

chore(metrics): remove leftover client_golang tutorial scaffolding#2263
hami-robot[bot] merged 2 commits into
Project-HAMi:masterfrom
veyron-kairo:cleanup/vgpumonitor-remove-dead-code

Conversation

@veyron-kairo

@veyron-kairo veyron-kairo commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Removes dead scaffolding left over from the client_golang tutorial in cmd/vGPUmonitor/metrics.go: the unused ReallyExpensiveAssessmentOfTheSystemState mock function (returned fabricated data, no callers), a block of commented-out functions (parseidstr, gettotalusage, getTotalUtilization), and two stale doc comments referencing them. No functional change — the Zone field and all metric output are untouched.

Which issue(s) this PR fixes:
Addresses the dead-code item of #2249 (per-scrape descriptor hoisting will follow as a separate PR).

Special notes for your reviewer:
No behavior change. make verify and go build ./cmd/vGPUmonitor/ pass locally.

AI assistance disclosure:
I used AI assistance (Claude Code) to identify the dead code . I reviewed and verified the change (confirmed zero callers), wrote the commit message myself, ran make verify locally, and take responsibility for it.

Does this PR introduce a user-facing change?:
NONE

Summary by CodeRabbit

  • Documentation
    • Updated monitoring documentation to accurately describe GPU, pod, and container metric collection.
    • Clarified concurrency requirements and the current monitoring behavior.
    • Removed outdated examples and obsolete guidance to make the documentation easier to follow.
    • Improved descriptions of cluster monitoring and metric collection for clearer reference.

Signed-off-by: Shridhar Panigrahi <198173519+veyron-kairo@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5330fb33-73c4-489f-bc73-1f89f39e2252

📥 Commits

Reviewing files that changed from the base of the PR and between 17d59d8 and 929b74b.

📒 Files selected for processing (2)
  • cmd/vGPUmonitor/main.go
  • cmd/vGPUmonitor/metrics.go
💤 Files with no reviewable changes (1)
  • cmd/vGPUmonitor/main.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • cmd/vGPUmonitor/metrics.go

📝 Walkthrough

Walkthrough

The change removes obsolete example code and comments from the vGPU monitor. It updates documentation for monitor state, metric collection, shared-informer lookups, and zone-labeling registration. It also removes the mock assessment method.

Changes

vGPU monitor documentation cleanup

Layer / File(s) Summary
Monitor state and collection contract
cmd/vGPUmonitor/metrics.go
ClusterManager now documents active monitor state. Collect documents GPU, pod, and container metrics and concurrent-call safety. The obsolete assessment method and helper comments are removed.
Cluster registration documentation and wiring cleanup
cmd/vGPUmonitor/metrics.go, cmd/vGPUmonitor/main.go
NewClusterManager documents shared-informer pod lookups and zone-labeling registration. Stale commented registration examples are removed.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

  • Project-HAMi/HAMi#2249 — Covers the vGPU monitor scaffolding cleanup and removal of obsolete assessment code.

Possibly related PRs

  • Project-HAMi/HAMi#2275 — Makes the same vGPU monitor cleanup changes, including documentation updates and removal of ReallyExpensiveAssessmentOfTheSystemState.

Suggested reviewers: dsfans2014

Poem

A rabbit tidies monitor lore,
Mock methods hop out the door.
GPUs, pods, and containers align,
Shared lookups read clear and fine.
Clean comments now mark the way.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the removal of obsolete client_golang tutorial scaffolding.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this 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.

@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.

lgtm. nit: two more tutorial scraps remain, the commented NewClusterManager("ca") in main.go and the "Prometheus-ignorant" comment above NewClusterManager, mind sweeping those too?

Removes the commented-out NewClusterManager("ca", reg) call and the
"in real code" preamble above it, and rewrites the NewClusterManager doc
comment, which described a tutorial-era constructor that no longer matches
the function.

No functional change.

Signed-off-by: Shridhar Panigrahi <198173519+veyron-kairo@users.noreply.github.com>
@veyron-kairo

Copy link
Copy Markdown
Contributor Author

@mesutoezdil done, swept both.

@mesutoezdil

Copy link
Copy Markdown
Contributor

/lgtm

@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 4, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: archlitchi, veyron-kairo

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 4, 2026
@codecov

codecov Bot commented Aug 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
unittests 64.26% <ø> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 1 file with indirect coverage changes

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

@hami-robot
hami-robot Bot merged commit 1309298 into Project-HAMi:master Aug 4, 2026
17 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.

3 participants