From 7b20daa9acc23151e58ebba8ca3ed75f68ab113f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan-Otto=20Kr=C3=B6pke?= Date: Wed, 7 Oct 2026 16:34:26 +0200 Subject: [PATCH] performancecounter: fix explicit instance lists and startup panics on invalid config (#2511) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Claude Opus 5.5 Signed-off-by: Jan-Otto Kröpke --- docs/collector.performancecounter.md | 3 + .../performancecounter/performancecounter.go | 53 ++++---- .../performancecounter_test.go | 7 +- .../performancecounter_test_test.go | 119 ++++++++++++++++++ .../collector/performancecounter/types.go | 2 + internal/collector/registry/registry_test.go | 4 +- internal/pdh/collector.go | 51 +++++--- internal/pdh/collector_test.go | 64 ++++++++++ 8 files changed, 254 insertions(+), 49 deletions(-) diff --git a/docs/collector.performancecounter.md b/docs/collector.performancecounter.md index 57125df1..e484a05a 100644 --- a/docs/collector.performancecounter.md +++ b/docs/collector.performancecounter.md @@ -186,6 +186,9 @@ This key is optional. Labels is a map of key-value pairs that will be added as labels to the metric. +Counters of the same object that share a metric name must differ in their labels, like `state` in the schema example above. +Otherwise, they would produce identical series and the collector fails to start. + ### Example ``` diff --git a/internal/collector/performancecounter/performancecounter.go b/internal/collector/performancecounter/performancecounter.go index b9bf998e..d0ee5d39 100644 --- a/internal/collector/performancecounter/performancecounter.go +++ b/internal/collector/performancecounter/performancecounter.go @@ -25,6 +25,7 @@ import ( "reflect" "regexp" "slices" + "strconv" "strings" "time" @@ -151,51 +152,50 @@ func (c *Collector) Build(logger *slog.Logger, _ *mi.Session) error { } names = append(names, object.Name) - counters := make([]string, 0, len(object.Counters)) + counters := make([]Counter, 0, len(object.Counters)) fields := make([]reflect.StructField, 0, len(object.Counters)+2) + object.fieldIndex = make(map[string]int, len(object.Counters)) for j, counter := range object.Counters { if counter.Metric == "" { - c.config.Objects[i].Counters[j].Metric = sanitizeMetricName( + counter.Metric = sanitizeMetricName( fmt.Sprintf("%s_%s_%s_%s", types.Namespace, Name, object.Object, counter.Name), ) + c.config.Objects[i].Counters[j].Metric = counter.Metric } if counter.Name == "" { - errs = append(errs, errors.New("counter name is required")) - c.config.Objects = slices.Delete(c.config.Objects, i, 1) + errs = append(errs, fmt.Errorf("object %s: counter name is required", object.Name)) continue } - if slices.Contains(counters, counter.Name) { - errs = append(errs, fmt.Errorf("counter name %s is duplicated", counter.Name)) + if _, ok := object.fieldIndex[counter.Name]; ok { + errs = append(errs, fmt.Errorf("object %s: counter name %s is duplicated", object.Name, counter.Name)) continue } - counters = append(counters, counter.Name) - - field, err := func(name string) (_ reflect.StructField, err error) { - defer func() { - if r := recover(); r != nil { - err = fmt.Errorf("failed to create field for %s: %v", name, r) - } - }() - - return reflect.StructField{ - Name: strings.ToUpper(sanitizeMetricName(name)), - Type: reflect.TypeFor[float64](), - Tag: reflect.StructTag(fmt.Sprintf(`perfdata:"%s"`, name)), - }, nil - }(counter.Name) - if err != nil { - errs = append(errs, err) + if k := slices.IndexFunc(counters, func(other Counter) bool { + return other.Metric == counter.Metric && maps.Equal(other.Labels, counter.Labels) + }); k != -1 { + errs = append(errs, fmt.Errorf("object %s: counters %s and %s produce identical series %s, set a different metric name or labels", + object.Name, counters[k].Name, counter.Name, counter.Metric, + )) continue } - fields = append(fields, field) + counters = append(counters, counter) + + // Counter names are free text and may produce invalid or colliding Go identifiers, + // so the struct fields get synthetic names and are looked up by index instead. + object.fieldIndex[counter.Name] = len(fields) + fields = append(fields, reflect.StructField{ + Name: "F" + strconv.Itoa(len(fields)), + Type: reflect.TypeFor[float64](), + Tag: reflect.StructTag(`perfdata:` + strconv.Quote(counter.Name)), + }) } if object.Instances != nil { @@ -300,13 +300,14 @@ func (c *Collector) collectObject(ch chan<- prometheus.Metric, perfDataObject Ob for _, counter := range perfDataObject.Counters { val := reflect.ValueOf(sliceValue).Index(i) - field := val.FieldByName(strings.ToUpper(sanitizeMetricName(counter.Name))) - if !field.IsValid() { + fieldIndex, ok := perfDataObject.fieldIndex[counter.Name] + if !ok { errs = append(errs, fmt.Errorf("%s not found in collected data", counter.Name)) continue } + field := val.Field(fieldIndex) if field.Kind() != reflect.Float64 { errs = append(errs, fmt.Errorf("failed to cast %s to float64", counter.Name)) diff --git a/internal/collector/performancecounter/performancecounter_test.go b/internal/collector/performancecounter/performancecounter_test.go index 08c38f80..a76a9e34 100644 --- a/internal/collector/performancecounter/performancecounter_test.go +++ b/internal/collector/performancecounter/performancecounter_test.go @@ -23,12 +23,15 @@ import ( "github.com/alecthomas/kingpin/v2" "github.com/prometheus-community/windows_exporter/internal/collector/performancecounter" "github.com/prometheus-community/windows_exporter/internal/utils/testutils" + "github.com/stretchr/testify/require" ) func BenchmarkCollector(b *testing.B) { - perfDataObjects := `[{"object":"Processor Information","instances":["*"],"instance_label":"core","counters":[{"name":"% Processor Time","metric":"windows_performancecounter_processor_information_processor_time","labels":{"state":"active"}},{"name":"% Idle Time","metric":"windows_performancecounter_processor_information_processor_time","labels":{"state":"idle"}}]},{"object":"Memory","counters":[{"name":"Cache Faults/sec","type":"counter"}]}]` + perfDataObjects := `[{"name":"processor_information","object":"Processor Information","instances":["*"],"instance_label":"core","counters":[{"name":"% Processor Time","metric":"windows_performancecounter_processor_information_processor_time","labels":{"state":"active"}},{"name":"% Idle Time","metric":"windows_performancecounter_processor_information_processor_time","labels":{"state":"idle"}}]},{"name":"memory","object":"Memory","counters":[{"name":"Cache Faults/sec","type":"counter"}]}]` testutils.FuncBenchmarkCollector(b, performancecounter.Name, performancecounter.NewWithFlags, func(app *kingpin.Application) { - app.GetFlag("collector.perfdata.objects").StringVar(&perfDataObjects) + // The objects are only read by the application action, which runs on parse. + _, err := app.Parse([]string{"--collector.performancecounter.objects", perfDataObjects}) + require.NoError(b, err) }) } diff --git a/internal/collector/performancecounter/performancecounter_test_test.go b/internal/collector/performancecounter/performancecounter_test_test.go index dfdd77cd..4aa31f59 100644 --- a/internal/collector/performancecounter/performancecounter_test_test.go +++ b/internal/collector/performancecounter/performancecounter_test_test.go @@ -153,6 +153,30 @@ windows_performancecounter_processor_information_processor_time\{core="0,0",stat counters: []performancecounter.Counter{{Name: "Available Bytes", Type: "gauge"}, {Name: "Available Bytes", Type: "gauge"}}, expectedMetrics: nil, }, + { + // PDH counter names are case-insensitive, but both names sanitize to the same Go identifier. + name: "counter names with the same sanitized name", + object: "Memory", + counterType: pdh.CounterTypeRaw, + instances: nil, + buildErr: "", + counters: []performancecounter.Counter{ + {Name: "Available Bytes", Type: "gauge", Metric: "windows_performancecounter_memory_available_bytes"}, + {Name: "available bytes", Type: "gauge", Metric: "windows_performancecounter_memory_available_bytes_lowercase"}, + }, + expectedMetrics: regexp.MustCompile(`^# HELP windows_performancecounter_collector_duration_seconds windows_exporter: Duration of an performancecounter child collection. +# TYPE windows_performancecounter_collector_duration_seconds gauge +windows_performancecounter_collector_duration_seconds\{collector="counter names with the same sanitized name"} [0-9.e+-]+ +# HELP windows_performancecounter_collector_success windows_exporter: Whether a performancecounter child collector was successful. +# TYPE windows_performancecounter_collector_success gauge +windows_performancecounter_collector_success\{collector="counter names with the same sanitized name"} 1 +# HELP windows_performancecounter_memory_available_bytes windows_exporter: custom Performance Counter metric +# TYPE windows_performancecounter_memory_available_bytes gauge +windows_performancecounter_memory_available_bytes [0-9.e+-]+ +# HELP windows_performancecounter_memory_available_bytes_lowercase windows_exporter: custom Performance Counter metric +# TYPE windows_performancecounter_memory_available_bytes_lowercase gauge +windows_performancecounter_memory_available_bytes_lowercase [0-9.e+-]+`), + }, { name: "counter with spaces and brackets", object: "invalid", @@ -216,6 +240,101 @@ windows_performancecounter_processor_information_processor_time\{core="0,0",stat } } +// TestBuildInvalidConfig verifies that invalid configurations are reported as +// errors. Build runs in a goroutine without recover during exporter startup, +// so a panic here would crash the exporter. +func TestBuildInvalidConfig(t *testing.T) { + t.Parallel() + + // Build writes default metric names into the config, so every case needs its own objects. + availableBytes := func() performancecounter.Object { + return performancecounter.Object{ + Name: "available_bytes", + Object: "Memory", + Counters: []performancecounter.Counter{{Name: "Available Bytes"}}, + } + } + committedBytes := func() performancecounter.Object { + return performancecounter.Object{ + Name: "committed_bytes", + Object: "Memory", + Counters: []performancecounter.Counter{{Name: "Committed Bytes"}}, + } + } + + for _, tc := range []struct { + name string + objects []performancecounter.Object + buildErr string + }{ + { + name: "empty counter name in first object", + objects: []performancecounter.Object{ + {Name: "memory", Object: "Memory", Counters: []performancecounter.Counter{{Name: ""}, {Name: "Available Bytes"}}}, + committedBytes(), + }, + buildErr: "object memory: counter name is required", + }, + { + name: "empty counter name in third object", + objects: []performancecounter.Object{ + availableBytes(), + committedBytes(), + {Name: "memory", Object: "Memory", Counters: []performancecounter.Counter{{Name: ""}}}, + }, + buildErr: "object memory: counter name is required", + }, + { + name: "counter name starting with a digit", + objects: []performancecounter.Object{ + {Name: "memory", Object: "Memory", Counters: []performancecounter.Counter{{Name: "1 Available Bytes"}}}, + }, + buildErr: pdh.NewPdhError(pdh.CstatusNoCounter).Error(), + }, + { + name: "counter names with the same default metric name", + objects: []performancecounter.Object{ + {Name: "memory", Object: "Memory", Counters: []performancecounter.Counter{{Name: "Available Bytes"}, {Name: "available bytes"}}}, + }, + buildErr: "object memory: counters Available Bytes and available bytes produce identical series windows_performancecounter_memory_available_bytes", + }, + { + name: "counters with the same metric name", + objects: []performancecounter.Object{ + {Name: "memory", Object: "Memory", Counters: []performancecounter.Counter{ + {Name: "Available Bytes", Metric: "windows_memory_bytes"}, + {Name: "Committed Bytes", Metric: "windows_memory_bytes"}, + }}, + }, + buildErr: "object memory: counters Available Bytes and Committed Bytes produce identical series windows_memory_bytes", + }, + { + name: "counters with the same metric name and labels", + objects: []performancecounter.Object{ + {Name: "processor", Object: "Processor Information", Instances: pdh.InstancesAll, Counters: []performancecounter.Counter{ + {Name: "% Processor Time", Metric: "windows_processor_time", Labels: map[string]string{"state": "active"}}, + {Name: "% Idle Time", Metric: "windows_processor_time", Labels: map[string]string{"state": "idle"}}, + {Name: "% User Time", Metric: "windows_processor_time", Labels: map[string]string{"state": "active"}}, + }}, + }, + buildErr: "object processor: counters % Processor Time and % User Time produce identical series windows_processor_time", + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + c := performancecounter.New(&performancecounter.Config{Objects: tc.objects}) + + t.Cleanup(func() { require.NoError(t, c.Close()) }) + + var err error + + require.NotPanics(t, func() { err = c.Build(slog.New(slog.DiscardHandler), nil) }) + require.ErrorContains(t, err, tc.buildErr) + }) + } +} + func TestCollectorClose(t *testing.T) { t.Parallel() diff --git a/internal/collector/performancecounter/types.go b/internal/collector/performancecounter/types.go index 29f2c53e..70931bb0 100644 --- a/internal/collector/performancecounter/types.go +++ b/internal/collector/performancecounter/types.go @@ -32,6 +32,8 @@ type Object struct { collector *pdh.Collector perfDataObject any + // fieldIndex maps a counter name to its field index in the element type of perfDataObject. + fieldIndex map[string]int } type Counter struct { diff --git a/internal/collector/registry/registry_test.go b/internal/collector/registry/registry_test.go index 5c416044..5bf0518a 100644 --- a/internal/collector/registry/registry_test.go +++ b/internal/collector/registry/registry_test.go @@ -31,7 +31,9 @@ func BenchmarkCollector(b *testing.B) { keys := `[{"name":"windows_nt","key":"HKLM\\SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion","values":[{"name":"CurrentMajorVersionNumber"}]}]` testutils.FuncBenchmarkCollector(b, registry.Name, registry.NewWithFlags, func(app *kingpin.Application) { - app.GetFlag("collector.registry.keys").StringVar(&keys) + // The keys are only read by the application action, which runs on parse. + _, err := app.Parse([]string{"--collector.registry.keys", keys}) + require.NoError(b, err) }) } diff --git a/internal/pdh/collector.go b/internal/pdh/collector.go index e679028e..cc11fdf2 100644 --- a/internal/pdh/collector.go +++ b/internal/pdh/collector.go @@ -334,7 +334,7 @@ func (c *Collector) collectWorkerRaw() { elemValue := reflect.ValueOf(reflect.New(elemType).Interface()).Elem() indexMap := map[string]int{} - stringMap := map[*uint16]string{} + nameCache := map[string]string{} for _, counter := range c.counters { for _, instance := range counter.Instances { @@ -365,11 +365,6 @@ func (c *Collector) collectWorkerRaw() { items = unsafe.Slice((*RawCounterItem)(unsafe.Pointer(&buf[0])), itemCount) - var ( - instanceName string - ok bool - ) - for _, item := range items { if item.RawValue.CStatus != CstatusValidData && item.RawValue.CStatus != CstatusNewData { c.logger.Debug("skipping counter item with invalid data status", @@ -381,10 +376,7 @@ func (c *Collector) collectWorkerRaw() { continue } - if instanceName, ok = stringMap[item.SzName]; !ok { - instanceName = windows.UTF16PtrToString(item.SzName) - stringMap[item.SzName] = instanceName - } + instanceName := decodeInstanceName(nameCache, item.SzName) if strings.HasSuffix(instanceName, InstanceTotal) && !c.totalCounterRequested { continue @@ -501,7 +493,7 @@ func (c *Collector) collectWorkerFormatted() { elemValue := reflect.ValueOf(reflect.New(elemType).Interface()).Elem() indexMap := map[string]int{} - stringMap := map[*uint16]string{} + nameCache := map[string]string{} for _, counter := range c.counters { for _, instance := range counter.Instances { @@ -532,20 +524,12 @@ func (c *Collector) collectWorkerFormatted() { items = unsafe.Slice((*FmtCounterValueItemDouble)(unsafe.Pointer(&buf[0])), itemCount) - var ( - instanceName string - ok bool - ) - for _, item := range items { if item.FmtValue.CStatus != CstatusValidData && item.FmtValue.CStatus != CstatusNewData { continue } - if instanceName, ok = stringMap[item.SzName]; !ok { - instanceName = windows.UTF16PtrToString(item.SzName) - stringMap[item.SzName] = instanceName - } + instanceName := decodeInstanceName(nameCache, item.SzName) if strings.HasSuffix(instanceName, InstanceTotal) && !c.totalCounterRequested { continue @@ -631,6 +615,33 @@ func formatCounterPath(object, instance, counterName string) string { return counterPath } +// decodeInstanceName returns the instance name p points to. +// Decoded names are cached by their raw UTF-16 content and not by p, because p points +// into the item buffer, which is overwritten by every Get*CounterArray call. With explicit +// instances, every call writes its single name at the same address. +func decodeInstanceName(cache map[string]string, p *uint16) string { + if p == nil { + return "" + } + + n := 0 + for *(*uint16)(unsafe.Add(unsafe.Pointer(p), n*2)) != 0 { + n++ + } + + raw := unsafe.Slice((*byte)(unsafe.Pointer(p)), n*2) + + // The compiler does not allocate for the string conversion in a map lookup. + if name, ok := cache[string(raw)]; ok { + return name + } + + name := windows.UTF16ToString(unsafe.Slice(p, n)) + cache[string(raw)] = name + + return name +} + func isKnownCounterDataError(err error) bool { var pdhErr *Error diff --git a/internal/pdh/collector_test.go b/internal/pdh/collector_test.go index 98c17fc9..60937a91 100644 --- a/internal/pdh/collector_test.go +++ b/internal/pdh/collector_test.go @@ -71,3 +71,67 @@ func TestCollector(t *testing.T) { }) } } + +type processorInformation struct { + Name string + ProcessorTime float64 `perfdata:"% Processor Time"` + IdleTime float64 `perfdata:"% Idle Time"` +} + +// TestCollectorExplicitInstances verifies that a collector built with an explicit +// list of instances returns one row per requested instance. Each instance has its +// own counter handle, and PDH writes every single-item result into the same buffer, +// so the instance names must not be resolved through a cache keyed by buffer address. +func TestCollectorExplicitInstances(t *testing.T) { + t.Parallel() + + logger := slog.New(slog.DiscardHandler) + + discovery, err := pdh.NewCollector[processorInformation](logger, pdh.CounterTypeRaw, "Processor Information", pdh.InstancesAll) + require.NoError(t, err) + + t.Cleanup(discovery.Close) + + var all []processorInformation + + require.NoError(t, discovery.Collect(&all)) + + instances := make([]string, 0, 3) + + for _, row := range all { + if len(instances) == cap(instances) { + break + } + + instances = append(instances, row.Name) + } + + if len(instances) < 2 { + t.Skipf("need at least 2 Processor Information instances, got %v", instances) + } + + for _, counterType := range []pdh.CounterType{pdh.CounterTypeRaw, pdh.CounterTypeFormatted} { + t.Run(string(counterType), func(t *testing.T) { + t.Parallel() + + collector, err := pdh.NewCollector[processorInformation](logger, counterType, "Processor Information", instances) + require.NoError(t, err) + + t.Cleanup(collector.Close) + + // Formatted rate counters need two samples before they report a value. + time.Sleep(100 * time.Millisecond) + + var data []processorInformation + + require.NoError(t, collector.Collect(&data)) + + names := make([]string, 0, len(data)) + for _, row := range data { + names = append(names, row.Name) + } + + require.ElementsMatch(t, instances, names) + }) + } +}