feat(monitor): Standardized OpenTelemetry and Dynamic Cluster Node Labeling - #2512
Conversation
…beling Signed-off-by: aniket866 <iamaniketkumarmaner@gmail.com>
Signed-off-by: aniket866 <iamaniketkumarmaner@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aniket866 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 |
|
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)
📝 WalkthroughWalkthroughvGPUmonitor now accepts node and cluster identifiers through CLI flags. ChangesvGPUmonitor metric identity and client initialization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as vGPUmonitor CLI
participant Manager as ClusterManager
participant Prometheus as Prometheus
CLI->>Manager: provide nodeName and clusterID
Manager->>Manager: resolve nodeName from environment when empty
Manager->>Prometheus: register metrics with optional identity labels
Prometheus-->>Manager: scrape collector metrics
Possibly related issues
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmd/vGPUmonitor/metrics_test.go (1)
71-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the Prometheus label contract.
The test checks only
NodeNameandClusterIDfields. It does not inspect the registry, so it passes ifWrapRegistererWithomits either label. Gather a metric fixture and assertnode_name="node-1"andcluster_id="cluster-1". Add coverage for node-name resolution fromutil.NodeNameEnvName.🤖 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 71 - 82, Strengthen TestNewClusterManagerNodeAndClusterIDLabels by gathering a registered metric from reg and asserting its labels include node_name="node-1" and cluster_id="cluster-1", rather than checking only struct fields. Add a test case that verifies node-name resolution when util.NodeNameEnvName is configured.
🤖 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/main.go`:
- Line 149: Update the vGPU monitor registration flow around NewClusterManager
and versionmetrics.NewBuildInfoCollector so the build-info collector is
registered through the same prometheus.WrapRegistererWith identity labels
containing node_name and cluster_id. Ensure hami_build_info is emitted with
those labels alongside ClusterManagerCollector metrics, rather than remaining
registered on the raw registry.
In `@cmd/vGPUmonitor/metrics_test.go`:
- Around line 72-74: Update TestNewClusterManagerNodeAndClusterIDLabels to avoid
passing an uninitialized &nvidia.ContainerLister{} into NewClusterManager;
construct the ContainerLister with an initialized fake clientset before creating
the manager, or use a setup that does not start the informer while preserving
the test’s label-registration assertions.
In `@cmd/vGPUmonitor/metrics.go`:
- Around line 509-512: Resolve the effective node name once in the shared
collection flow, preferring ClusterManager.NodeName and falling back to
util.NodeNameEnvName, then pass or reuse it in both
collectPodAndContainerMigInfo and collectPodAndContainerInfo. Remove the direct
NODE_NAME lookup from collectPodAndContainerInfo so --node-name consistently
controls regular and MIG container metric collection.
---
Nitpick comments:
In `@cmd/vGPUmonitor/metrics_test.go`:
- Around line 71-82: Strengthen TestNewClusterManagerNodeAndClusterIDLabels by
gathering a registered metric from reg and asserting its labels include
node_name="node-1" and cluster_id="cluster-1", rather than checking only struct
fields. Add a test case that verifies node-name resolution when
util.NodeNameEnvName is configured.
🪄 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: d395269a-367a-457f-b57e-6dd072499436
📒 Files selected for processing (4)
CHANGELOG.mdcmd/vGPUmonitor/main.gocmd/vGPUmonitor/metrics.gocmd/vGPUmonitor/metrics_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/vGPUmonitor/metrics.go (1)
575-588: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPass the effective node name into the container lister.
start()callsnvidia.NewContainerLister()beforeNewClusterManager(...), andNewContainerLister()only readsNODE_NAME; it does not consider--node-nameand returns an error whenNODE_NAMEis empty. Resolve--node-name/NODE_NAMEonce incmd/vGPUmonitor, accept that value via a lister constructor, use it forContainerLister.nodeName, and keepNewClusterManagerconsistent.🤖 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.go` around lines 575 - 588, Resolve the effective node name from --node-name with NODE_NAME as fallback once in start(), then pass it into nvidia.NewContainerLister through its constructor and use it for ContainerLister.nodeName without re-reading the environment. Pass the same resolved value to NewClusterManager, preserving existing behavior when only NODE_NAME is configured.
🧹 Nitpick comments (1)
cmd/vGPUmonitor/metrics.go (1)
598-607: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify labels at the Prometheus registry boundary.
The current test context checks
ClusterManager.NodeNameandClusterID, but it does not verify gathered metrics. Add a registry assertion that checksnode_nameandcluster_idon emitted metric families, including the documented behavior when either value is empty.🤖 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.go` around lines 598 - 607, Extend the test around the Prometheus registration flow using prometheus.WrapRegistererWith and MustRegister to gather metric families from the registry and assert emitted node_name and cluster_id labels. Cover both populated values and the documented empty-value behavior, ensuring labels are absent when their corresponding input is empty.
🤖 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`:
- Around line 590-595: Validate containerLister and its Clientset in
NewClusterManager/initMetrics before completing registration, returning an error
when either dependency is nil so Collect cannot reach PodLister or
containerLister operations with invalid state. Alternatively, ensure Collect
safely skips both pod and container metric paths when dependencies are
unavailable, while preserving successful registration for valid inputs.
---
Outside diff comments:
In `@cmd/vGPUmonitor/metrics.go`:
- Around line 575-588: Resolve the effective node name from --node-name with
NODE_NAME as fallback once in start(), then pass it into
nvidia.NewContainerLister through its constructor and use it for
ContainerLister.nodeName without re-reading the environment. Pass the same
resolved value to NewClusterManager, preserving existing behavior when only
NODE_NAME is configured.
---
Nitpick comments:
In `@cmd/vGPUmonitor/metrics.go`:
- Around line 598-607: Extend the test around the Prometheus registration flow
using prometheus.WrapRegistererWith and MustRegister to gather metric families
from the registry and assert emitted node_name and cluster_id labels. Cover both
populated values and the documented empty-value behavior, ensuring labels are
absent when their corresponding input is empty.
🪄 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: ca26452d-1ad1-46c7-a062-0ea85043d2d1
📒 Files selected for processing (3)
cmd/vGPUmonitor/metrics.gocmd/vGPUmonitor/metrics_test.gopkg/monitor/nvidia/cudevshr.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/vGPUmonitor/metrics_test.go
…st and prioritize NodeName from ClusterManager Signed-off-by: aniket866 <iamaniketkumarmaner@gmail.com>
0226b09 to
f9ad876
Compare
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Hi @aniket866, Just a quick heads-up on the PR title since this PR focuses on Prometheus labeling (using prometheus.WrapRegistererWith) rather than the OpenTelemetry SDK or tracing spans, it might be helpful to update the title to reflect the Prometheus work instead. The LFX umbrella issue (#2126) has separate deliverables for Prometheus metric improvements and an OpenTelemetry tracing PoC. Updating the title will help mentors avoid conflating the two, as the actual OTel SDK tracing PoC is being addressed in a separate PR. Thanks again for the contribution! |
|
You can view the relevant rule here. |
3 participants
vGPUmonitorrely on local metric names (hami_host_gpu_memory_used_bytes) and lack standard Kubernetes node identification labels (node_name,cluster_id).cmd/vGPUmonitor/metrics.go(in metric descriptor definitions)--node-nameand--cluster-idparameters to auto-injectnode_nameandcluster_idmetric labels across all exported Prometheus descriptors.Before vs. After Architecture
flowchart LR subgraph Before["Before: Unlabeled Metrics without Cluster Node Identity"] direction TB B1["Metric Generation in vGPUmonitor"] B2["Labels: namespace, pod, container, vdevice"] B3["Exported without Node or Cluster Identity"] B4["Central Prometheus Scrapes Hundreds of Nodes"] B5["Requires Heavy Relabeling Configurations"] B6["Incompatible with OpenTelemetry Standards"] B1 --> B2 B2 --> B3 B3 --> B4 B4 --> B5 B5 --> B6 end Before ==>|Standardized Node & Cluster Metadata Injection| After subgraph After["After: OpenTelemetry Compliant Standard Metrics"] direction TB A1["Metric Generation in vGPUmonitor"] A2["Labels: node_name, cluster_id, namespace..."] A3["Exported with Standardized Infrastructure Labels"] A4["Central Prometheus Receives Fully Labeled Series"] A5["Zero Relabeling Overhead & Instant Dashboards"] A6["Fully OpenTelemetry Compliant"] A1 --> A2 A2 --> A3 A3 --> A4 A4 --> A5 A5 --> A6 end classDef danger fill:#fee2e2,stroke:#ef4444,stroke-width:2px,color:#991b1b; classDef success fill:#dcfce7,stroke:#22c55e,stroke-width:2px,color:#166534; classDef neutral fill:#f3f4f6,stroke:#4b5563,stroke-width:1.5px,color:#1f2937; class B3,B5,B6 danger; class A2,A3,A5,A6 success; class B1,B2,B4,A1,A4 neutral;Workflow & Component Diagram
graph TD A["vGPUmonitor Initialization"] --> B["Read Node Name from Env (util.NodeNameEnvName)"] B --> C["Read Cluster ID from CLI Flag (--cluster-id)"] C --> D["Construct Base Label Map: node_name, cluster_id"] D --> E["Create Wrapped Prometheus Registerer: prometheus.WrapRegistererWith()"] E --> F["Register ClusterManagerCollector with Base Labels"] F --> G["Prometheus Scrape Triggered"] G --> H["Collector Emits Gauge Metrics"] H --> I["Base Labels Auto-Appended to All Metric Families"] I --> J["Serve Standardized Metric Stream over HTTP"]Closes #2510
Summary by CodeRabbit
New Features
Documentation