performancecounter: fix explicit instance lists and startup panics on invalid config (#2511)

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>

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))
 

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)
 	})
 }

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()
 

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 {

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)
 	})
 }
 

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
 

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)
+		})
+	}
+}