-
Notifications
You must be signed in to change notification settings - Fork 791
fix(scheduler): standardize metric descriptor label key from node to node_name #2343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ import ( | |
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/prometheus/client_golang/prometheus" | ||
| promtestutil "github.com/prometheus/client_golang/prometheus/testutil" | ||
|
|
||
| "github.com/Project-HAMi/HAMi/pkg/device" | ||
|
|
@@ -97,12 +98,12 @@ func TestClusterManagerCollectorSkipsMemoryRatioWithNonPositiveTotalMemory(t *te | |
| want := ` | ||
| # HELP hami_gpu_core_limit_ratio Device core limit for a certain GPU | ||
| # TYPE hami_gpu_core_limit_ratio gauge | ||
| hami_gpu_core_limit_ratio{device_index="0",device_type="AWSNeuron",device_uuid="zero-memory",node="node-1"} 2 | ||
| hami_gpu_core_limit_ratio{device_index="1",device_type="test-device",device_uuid="negative-memory",node="node-1"} 2 | ||
| hami_gpu_core_limit_ratio{device_index="2",device_type="NVIDIA",device_uuid="normal-memory",node="node-1"} 2 | ||
| hami_gpu_core_limit_ratio{device_index="0",device_type="AWSNeuron",device_uuid="zero-memory",node_name="node-1"} 2 | ||
| hami_gpu_core_limit_ratio{device_index="1",device_type="test-device",device_uuid="negative-memory",node_name="node-1"} 2 | ||
| hami_gpu_core_limit_ratio{device_index="2",device_type="NVIDIA",device_uuid="normal-memory",node_name="node-1"} 2 | ||
| # HELP hami_node_gpu_memory_allocated_ratio GPU Memory Allocated Percentage on a certain GPU | ||
| # TYPE hami_node_gpu_memory_allocated_ratio gauge | ||
| hami_node_gpu_memory_allocated_ratio{device_index="2",device_uuid="normal-memory",node="node-1"} 0.25 | ||
| hami_node_gpu_memory_allocated_ratio{device_index="2",device_uuid="normal-memory",node_name="node-1"} 0.25 | ||
| # HELP nodeGPUMemoryPercentage GPU Memory Allocated Percentage on a certain GPU | ||
| # TYPE nodeGPUMemoryPercentage gauge | ||
| nodeGPUMemoryPercentage{deviceidx="2",deviceuuid="normal-memory",nodeid="node-1"} 0.25 | ||
|
|
@@ -118,3 +119,95 @@ nodeGPUMemoryPercentage{deviceidx="2",deviceuuid="normal-memory",nodeid="node-1" | |
| t.Fatalf("unexpected collecting result:\n%s", err) | ||
| } | ||
| } | ||
|
|
||
| func newFakeMetricsProvider() *fakeSchedulerMetricsProvider { | ||
| nodeUsage := map[string]*schedulerpkg.NodeUsage{ | ||
| "node-1": { | ||
| Devices: policy.DeviceUsageList{ | ||
| DeviceLists: []*policy.DeviceListsScore{ | ||
| { | ||
| Device: &device.DeviceUsage{ | ||
| ID: "dev-1", | ||
| Index: 0, | ||
| Totalmem: 1024, | ||
| Totalcore: 100, | ||
| Type: "NVIDIA", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
| return &fakeSchedulerMetricsProvider{ | ||
| nodeUsage: nodeUsage, | ||
| quotaManager: device.NewQuotaManager(), | ||
| podManager: device.NewPodManager(), | ||
| } | ||
| } | ||
|
|
||
| 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") | ||
| } | ||
| } | ||
|
Comment on lines
+148
to
+180
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Add a collected-metric assertion for
Set 🤖 Prompt for AI Agents |
||
|
|
||
| func TestSchedulerMetricDescriptorsLegacyMode(t *testing.T) { | ||
| cm := &ClusterManager{ | ||
| Zone: "test-zone", | ||
| LegacyMetrics: true, | ||
| } | ||
| 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() | ||
| 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 in legacy mode") | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: Project-HAMi/HAMi
Length of output: 1880
🏁 Script executed:
Repository: 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:
🌐 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 theString()method of theprometheus.Descstruct 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 utilizesfmt.Sprintfto 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 overd.constLabelPairsand formats each askey=valueusingfmt.Sprintf("%s=%q", ...)[1][2]. 2. Variable Labels: It iterates over the names stored withind.variableLabels(specificallyd.variableLabels.names) [1][2]. If a label is constrained, it denotes this by wrapping the name inc(...)[1]. 3. Safety: Modern versions of the client library include a nil check ford.variableLabelsbefore accessing itsnamesfield to prevent a nil-pointer dereference, a fix introduced following issues whereNewInvalidDescresulted invariableLabelsbeingnil[1][2][4]. Example output of this method looks like: Desc{fqName: "net_node_up", help: "Is node up...", constLabels: {}, variableLabels: [node]} [3]Citations:
🏁 Script executed:
Repository: Project-HAMi/HAMi
Length of output: 2441
🏁 Script executed:
Repository: Project-HAMi/HAMi
Length of output: 194
Correct the old-label matcher.
prometheus.Desc.String()formatsvariableLabelswith braces, so both currentvariableLabels: [...]predicates are false. Matchnodeas a complete label in the brace-delimited list so these tests can catch descriptors containing bothnode_nameand the obsoletenodelabel. Apply the same update at both predicate locations.🤖 Prompt for AI Agents