fix(scheduler): standardize metric descriptor label key from node to node_name - #2343
Conversation
…node_name Signed-off-by: swastikCommits <textswastik.alt@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: swastikCommits 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 |
📝 WalkthroughWalkthroughScheduler metrics now handle missing collection dependencies safely, use ChangesScheduler metrics
Estimated code review effort: 3 (Moderate) | ~20 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: 2
🤖 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/scheduler/metrics_test.go`:
- Around line 148-180: Extend TestSchedulerMetricDescriptors with a
collected-metric assertion for hami_node_gpu_overview using promtestutil.
Configure newFakeMetricsProvider to return a non-zero DeviceUsage.Used value,
collect and compare the metric, and verify the emitted shared_containers label
contains that value while preserving the existing descriptor checks.
- Around line 171-172: Update both old-label matcher predicates in the
descriptor validation test to match Prometheus’s brace-delimited variableLabels
format and detect node as a complete label, while avoiding partial matches such
as node_name. Keep the existing error behavior unchanged.
🪄 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: 2662bf36-1297-4c42-839f-4c3fa86aef0a
📒 Files selected for processing (2)
cmd/scheduler/metrics.gocmd/scheduler/metrics_test.go
| func TestSchedulerMetricDescriptors(t *testing.T) { | ||
| cm := &ClusterManager{ | ||
| Zone: "test-zone", | ||
| LegacyMetrics: false, | ||
| } | ||
| collector := ClusterManagerCollector{ | ||
| ClusterManager: cm, | ||
| metricsProvider: newFakeMetricsProvider(), | ||
| } | ||
|
|
||
| ch := make(chan *prometheus.Desc, 50) | ||
| collector.Describe(ch) | ||
| close(ch) | ||
|
|
||
| foundDescriptors := 0 | ||
| for desc := range ch { | ||
| foundDescriptors++ | ||
| descStr := desc.String() | ||
| // Ensure standard GPU descriptors (excluding namespace-scoped quota metrics) contain node_name and do not contain old 'node' | ||
| if strings.Contains(descStr, "fqName: \"hami_") && !strings.Contains(descStr, "hami_resource_quota_used") { | ||
| if !strings.Contains(descStr, "node_name") { | ||
| t.Errorf("standard descriptor %s does not contain node_name label", descStr) | ||
| } | ||
| if strings.Contains(descStr, "variableLabels: [node ") || strings.Contains(descStr, "variableLabels: [node,") { | ||
| t.Errorf("standard descriptor %s still contains old 'node' label", descStr) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if foundDescriptors == 0 { | ||
| t.Error("expected at least 1 descriptor from scheduler collector") | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a collected-metric assertion for shared_containers.
TestSchedulerMetricDescriptors validates descriptors only. It does not verify that hami_node_gpu_overview emits the new shared_containers label with DeviceUsage.Used.
Set Used to a non-zero value in newFakeMetricsProvider. Then use promtestutil.CollectAndCompare for hami_node_gpu_overview. Assert the shared_containers label value. This protects the new scheduler-to-vGPUmonitor join contract.
🤖 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/scheduler/metrics_test.go` around lines 148 - 180, Extend
TestSchedulerMetricDescriptors with a collected-metric assertion for
hami_node_gpu_overview using promtestutil. Configure newFakeMetricsProvider to
return a non-zero DeviceUsage.Used value, collect and compare the metric, and
verify the emitted shared_containers label contains that value while preserving
the existing descriptor checks.
| if strings.Contains(descStr, "variableLabels: [node ") || strings.Contains(descStr, "variableLabels: [node,") { | ||
| t.Errorf("standard descriptor %s still contains old 'node' label", descStr) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the resolved client_golang version and Desc.String() representation.
rg -n -C2 'github.com/prometheus/client_golang' go.mod go.sum
curl -fsSL https://raw.githubusercontent.com/prometheus/client_golang/v1.24.1/prometheus/desc.go |
rg -n -A10 'func \(d \*Desc\) String'Repository: Project-HAMi/HAMi
Length of output: 1880
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the metric descriptor output format and test predicates without running repository code.
FILE="cmd/scheduler/metrics_test.go"
if [ -f "$FILE" ]; then
echo "== file lines 120-225 =="
cat -n "$FILE" | sed -n '120,225p'
fi
echo "== relevant Prometheus Desc.String format examples =="
python3 - <<'PY'
# Directly model Prometheus github.com/prometheus/client_golang v1.24.1 Desc.String() variableLabels formatting.
# The implementation appends names with fmt.Sprintf("%s=%q", name, ...) joined by ", ".
def variable_labels_str(names):
return ", ".join(f'{{name={n!r}}}' for n in names)
cases = [
[],
["node"],
["node_name"],
["node_name", "node"],
["node", "node_name"],
["node", "other"],
]
for names in cases:
s = f"Desc(dname=\"dname\", fqName: \"fqName\", help: \"\", variableLabels: {variable_labels_str(names)}, constLabelPairs: []constLabelPair, err: )"
print("names=", names)
print("contains_variableLabels: [node ", "variableLabels: [node " in s)
print("contains_variableLabels: [node,", "variableLabels: [node," in s)
print("contains_variableLabels: {node}", "variableLabels: " in s and "{node}" in s)
print("contains node boundaries", any(tok in s for tok in ['{node}', ',node,']))
print("contains exact node_name node substring", "node_name" in s and "node" in s)
print()
PY
echo "== all occurrences of the old-label predicate =="
rg -n "variableLabels: \\[node" cmd/scheduler/metrics_test.goRepository: Project-HAMi/HAMi
Length of output: 5117
🌐 Web query:
prometheus client_golang v1.24.1 Desc String variableLabels names
💡 Result:
In Prometheus client_golang v1.24.1, the Desc struct manages metric descriptors, which include both constant and variable labels. The internal field responsible for storing the names of variable labels is named variableLabels [1][2]. Specifically, the Desc struct maintains a variableLabels field, which is a structure containing a names slice (a []string) [1][2]. When NewDesc is called, the provided variable label names are stored within this structure [1]. The String method of the Desc struct provides a string representation of the descriptor [2]. When displaying variable labels, this method iterates through d.variableLabels.names to format them [2]. If a variable label has an associated constraint (such as a custom validation or normalization function), the String method prefixes the label name with c to indicate that the label is constrained (e.g., c(label_name)) [3][2]. Otherwise, it simply lists the label name [2]. In summary, the variable label names are stored in the names slice within the variableLabels field of the Desc struct, and this field is accessed by the String method to generate a human-readable representation of the metric's descriptor [2].
Citations:
- 1: https://github.com/prometheus/client_golang/blob/master/prometheus/desc.go
- 2: https://code.asyncmachine.dev/src/github.com/prometheus/client_golang/prometheus/desc.go.html
- 3: https://github.com/prometheus/client_golang/blob/main/prometheus/labels.go
🌐 Web query:
site:github.com/prometheus/client_golang "func (d *Desc) String" variableLabels names fmt.Sprintf
💡 Result:
The code snippet func (d *Desc) String() string { ... } refers to the String() method of the prometheus.Desc struct within the official Prometheus Go client library (prometheus/client_golang) [1][2]. This method generates a string representation of the descriptor for debugging or logging purposes [1][3]. It utilizes fmt.Sprintf to format the descriptor's fields, including its fully qualified name (fqName), help text, constant labels, and variable labels [1]. Key aspects of its implementation include: 1. Constant Labels: It iterates over d.constLabelPairs and formats each as key=value using fmt.Sprintf("%s=%q", ...) [1][2]. 2. Variable Labels: It iterates over the names stored within d.variableLabels (specifically d.variableLabels.names) [1][2]. If a label is constrained, it denotes this by wrapping the name in c(...) [1]. 3. Safety: Modern versions of the client library include a nil check for d.variableLabels before accessing its names field to prevent a nil-pointer dereference, a fix introduced following issues where NewInvalidDesc resulted in variableLabels being nil [1][2][4]. Example output of this method looks like: Desc{fqName: "net_node_up", help: "Is node up...", constLabels: {}, variableLabels: [node]} [3]
Citations:
- 1: https://github.com/prometheus/client_golang/blob/master/prometheus/desc.go
- 2: prometheus.InvalidDesc.String() is formatted incorrectly prometheus/client_golang#1684
- 3: feature: get descriptor attributes functions prometheus/client_golang#516
- 4: https://github.com/prometheus/client_golang/blob/master/CHANGELOG.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import urllib.request
url = "https://raw.githubusercontent.com/prometheus/client_golang/v1.24.1/prometheus/desc.go"
text = urllib.request.urlopen(url).read().decode()
start = text.find('func (d *Desc) String()')
end = text.find('\nfunc ', start + 1)
print(text[start:end if end != -1 else len(text)])
PY
echo "== raw URL source around variableLabels formatting =="
python3 - <<'PY'
import urllib.request
text = urllib.request.urlopen("https://raw.githubusercontent.com/prometheus/client_golang/v1.24.1/prometheus/desc.go").read().decode()
for i, line in enumerate(text.splitlines(), 1):
if 'variableLabels' in line or 'names' in line:
print(f"{i}: {line}")
PYRepository: Project-HAMi/HAMi
Length of output: 2441
🏁 Script executed:
#!/bin/bash
set -euo pipefail
GOCACHE="$(go env GOCACHE 2>/dev/null || true)"
echo "GOCACHE=$GOCACHE"
find "$GOCACHE" \
-path '*prometheus/client_golang@v1.24.1/prometheus/desc.go' \
-type f 2>/dev/null | while read -r f; do
echo "== $f =="
sed -n '200,235p' "$f"
done
echo "== local module cache occurrences across downloaded tags =="
find "$GOCACHE" \
-path '*/prometheus/client_golang@*/prometheus/desc.go' \
-type f 2>/dev/null | while read -r f; do
echo "-- $f --"
rg -n 'variableLabels:|variableLabels\.names|variableLabels: \{' "$f" || true
done
echo "== deterministic model comparison =="
# Use the exact formatting from the v1.24.1 source snippet if it uses {name}.
declare -a inputs=("node" "node_name" "node_name,node" "node,node_name" "node,other")
for labels in "${inputs[@]}"; do
s="Desc{fqName: \"hami_x\", help: \"\", variableLabels: {$labels}, constLabels: {}, constLabelPairs: []constLabelPair, err: }"
echo "labels=$labels"
echo " contains variableLabels: [node space -> ${s#*(contains variableLabels: [node space -> *false, skipped)}"; if echo "$s" | grep -q 'variableLabels: \[node '; then echo " bracket-space true"; else echo " bracket-space false"; fi
echo " contains variableLabels: [node comma -> "; if echo "$s" | grep -q 'variableLabels: \[node,'; then echo "true"; else echo "false"; fi; echo
doneRepository: Project-HAMi/HAMi
Length of output: 194
Correct the old-label matcher.
prometheus.Desc.String() formats variableLabels with braces, so both current variableLabels: [...] predicates are false. Match node as a complete label in the brace-delimited list so these tests can catch descriptors containing both node_name and the obsolete node label. Apply the same update at both predicate locations.
🤖 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/scheduler/metrics_test.go` around lines 171 - 172, Update both old-label
matcher predicates in the descriptor validation test to match Prometheus’s
brace-delimited variableLabels format and detect node as a complete label, while
avoiding partial matches such as node_name. Keep the existing error behavior
unchanged.
|
cmd/vgpumonitor/metrics.go has no node or node_name label at all. the join example in pr body still would not work, vgpumonitor side is missing it too. separate followup, or should this pr add it there too? |
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:
|
here: #2398 |
|
Thanks for addressing metric-label consistency. This PR competes with #2398 and #2153 over the same public metric-label contract. Keeping multiple incompatible implementations open would duplicate review work before maintainers have selected one schema. This branch is also conflicting, substantially behind master, and failing patch coverage. We are closing this implementation so the label choice can first be made once in #2126; any requirement not covered by the selected path should be documented there. |
|
Explained in #2161 |
Rename the 'node' label to 'node_name' on hami_host_gpu_memory_used_bytes and hami_host_gpu_utilization_ratio for consistency with the scheduler metrics label key standardized in Project-HAMi#2343. Addresses review feedback from archlitchi on Project-HAMi#2398. Signed-off-by: ipsitapp8 <ipsitapp8@gmail.com>
5 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
In
cmd/scheduler/metrics.go, standard Prometheus metric descriptors currently export the node label asnode, whereascmd/vGPUmonitor/metrics.goexportsnode_name.This label key inconsistency prevents operators from performing Prometheus cross-component joins such as:
Here we standardize scheduler metric descriptors to use the
node_namelabel key while preserving legacy mode descriptors for backward compatibility. It also injects a mock metrics provider into unit tests and adds assertions verifying both the presence of node_name and the absence of the old node label.Which issue(s) this PR fixes:
Fixes #2161
AI Assistance disclosure:
I used AI assistance to analyze codebase patterns and structure tests, but all changes were manually inspected, written, and verified.
Special notes for your reviewer:
Supersedes #2170
Does this PR introduce a user-facing change?:
Summary by CodeRabbit
Bug Fixes
Metrics
node_name.