pdh: preserve duplicate counter instances (#2564)

Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>

internal/pdh/collector.go

@@ -70,10 +70,11 @@ type Row struct {
 	Values []float64
 }
 
-// rowAccessor creates rows of type T and sets their counter values.
+// rowAccessor creates rows of type T and sets their names and counter values.
 // field is the index the counterField of the counter was created with.
 type rowAccessor[T any] struct {
 	newRow   func(instance string) T
+	setName  func(row *T, instance string)
 	setValue func(row *T, field int, value float64)
 }
 
@@ -101,8 +102,11 @@ type Counter struct {
 
 // NewCollector creates a collector for the counters declared by the perfdata tags of the struct T.
 //
-// The optional field Name (string) of T receives the instance name. An
+// The optional field Name (string) of T receives the instance name. Occurrence
-// instance is only collected if every counter has a valid value for it in that
+// suffixes distinguish duplicate names without colliding with literal names.
+// These suffixes are not stable identities across samples.
+//
+// An instance is only collected if every counter has a valid value for it in that
 // sample. Use [Collector.MetricType] for the metric type of a counter.
 //
 // If an error is returned together with a non-nil Collector, some counters
@@ -153,6 +157,11 @@ func NewCollector[T any](logger *slog.Logger, resultType CounterType, object str
 
 			return row
 		},
+		setName: func(row *T, instance string) {
+			if nameIndex != -1 {
+				reflect.ValueOf(row).Elem().Field(nameIndex).SetString(instance)
+			}
+		},
 		setValue: func(row *T, field int, value float64) {
 			reflect.ValueOf(row).Elem().Field(field).SetFloat(value)
 		},
@@ -175,6 +184,9 @@ func NewDynamicCollector(logger *slog.Logger, resultType CounterType, object str
 		newRow: func(instance string) Row {
 			return Row{Name: instance, Values: make([]float64, len(counters))}
 		},
+		setName: func(row *Row, instance string) {
+			row.Name = instance
+		},
 		setValue: func(row *Row, field int, value float64) {
 			row.Values[field] = value
 		},
@@ -432,11 +444,13 @@ func (c *Collector[T]) collect(dst *[]T, state *collectState) (err error) {
 	*dst = (*dst)[:0:0]
 
 	rows := rowSet[T]{
-		c:         c,
+		c:           c,
-		dst:       dst,
+		dst:         dst,
-		index:     map[string]int{},
+		index:       map[instanceKey]int{},
-		nameCache: map[string]string{},
+		nameCache:   map[string]string{},
-		valid:     state.valid[:0],
+		occurrences: map[string]int{},
+		seen:        map[string]int{},
+		valid:       state.valid[:0],
 	}
 
 	for counterIndex := range c.counters {
@@ -538,13 +552,23 @@ func (c *Collector[T]) setRawValue(row *T, counter *Counter, value RawCounter) b
 	return true
 }
 
+// instanceKey matches the nth occurrence of an instance name across counter arrays.
+type instanceKey struct {
+	name       string
+	occurrence int
+}
+
 // rowSet appends one row per instance to dst and tracks which counters have
 // a valid value for each row.
 type rowSet[T any] struct {
 	c         *Collector[T]
 	dst       *[]T
-	index     map[string]int
+	index     map[instanceKey]int
 	nameCache map[string]string
+	// occurrences includes invalid items to reserve all literal names and keep occurrence positions.
+	occurrences map[string]int
+	// seen counts names in the current counter array and is cleared between arrays.
+	seen map[string]int
 	// valid has len(c.counters) entries per row.
 	valid []bool
 }
@@ -553,32 +577,37 @@ type rowSet[T any] struct {
 // appending the row if needed. ok is false if the item has no valid data or
 // its instance is not collected.
 func (r *rowSet[T]) row(counter *Counter, szName *uint16, status uint32) (int, bool) {
+	instanceName := decodeInstanceName(r.nameCache, szName)
+	if instanceName == "" || instanceName == "*" {
+		instanceName = InstanceEmpty
+	}
+
+	// Count before checking status so invalid items do not shift subsequent duplicates.
+	occurrence := r.seen[instanceName]
+	r.seen[instanceName]++
+	r.occurrences[instanceName] = max(r.occurrences[instanceName], occurrence+1)
+
 	if status != CstatusValidData && status != CstatusNewData {
 		r.c.logger.Debug("skipping counter item with invalid data status",
 			slog.String("counter", counter.Name),
-			slog.String("instance", windows.UTF16PtrToString(szName)),
+			slog.String("instance", instanceName),
 			slog.Uint64("status", uint64(status)),
 		)
 
 		return 0, false
 	}
 
-	instanceName := decodeInstanceName(r.nameCache, szName)
-
 	if strings.HasSuffix(instanceName, InstanceTotal) && !r.c.totalCounterRequested {
 		return 0, false
 	}
 
-	if instanceName == "" || instanceName == "*" {
+	key := instanceKey{name: instanceName, occurrence: occurrence}
-		instanceName = InstanceEmpty
+	if index, ok := r.index[key]; ok {
-	}
-
-	if index, ok := r.index[instanceName]; ok {
 		return index, true
 	}
 
 	index := len(*r.dst)
-	r.index[instanceName] = index
+	r.index[key] = index
 
 	*r.dst = append(*r.dst, r.c.rows.newRow(instanceName))
 
@@ -598,6 +627,8 @@ func (r *rowSet[T]) addRawItems(counterIndex int, buf []byte, itemCount uint32)
 	counter := &r.c.counters[counterIndex]
 	items := unsafe.Slice((*RawCounterItem)(unsafe.Pointer(unsafe.SliceData(buf))), itemCount)
 
+	clear(r.seen)
+
 	for _, item := range items {
 		row, ok := r.row(counter, item.SzName, item.RawValue.CStatus)
 		if ok && r.c.setRawValue(&(*r.dst)[row], counter, item.RawValue) {
@@ -611,6 +642,8 @@ func (r *rowSet[T]) addFormattedItems(counterIndex int, buf []byte, itemCount ui
 	counter := &r.c.counters[counterIndex]
 	items := unsafe.Slice((*FmtCounterValueItemDouble)(unsafe.Pointer(unsafe.SliceData(buf))), itemCount)
 
+	clear(r.seen)
+
 	for _, item := range items {
 		row, ok := r.row(counter, item.SzName, item.FmtValue.CStatus)
 		if !ok {
@@ -625,11 +658,37 @@ func (r *rowSet[T]) addFormattedItems(counterIndex int, buf []byte, itemCount ui
 	}
 }
 
+// nameDuplicates assigns suffixes after all literal names in the sample are known.
+func (r *rowSet[T]) nameDuplicates() {
+	for name, count := range r.occurrences {
+		suffix := 0
+
+		for occurrence := 1; occurrence < count; occurrence++ {
+			var instanceName string
+
+			for {
+				suffix++
+				instanceName = name + "#" + strconv.Itoa(suffix)
+
+				if _, literalName := r.occurrences[instanceName]; !literalName {
+					break
+				}
+			}
+
+			if row, ok := r.index[instanceKey{name: name, occurrence: occurrence}]; ok {
+				r.c.rows.setName(&(*r.dst)[row], instanceName)
+			}
+		}
+	}
+}
+
 // finish handles rows that lack a valid value for some counter. Counters
 // without any instance, e.g. those skipped by perfdata_min_build, are not
 // required. Without partial rows, such rows are removed. Otherwise, the
 // missing values are set to NaN.
 func (r *rowSet[T]) finish() {
+	r.nameDuplicates()
+
 	rows := *r.dst
 	n := len(r.c.counters)
 	kept := 0

internal/pdh/collector_internal_test.go

@@ -18,10 +18,14 @@
 package pdh
 
 import (
+	"fmt"
 	"log/slog"
 	"math"
 	"reflect"
+	"runtime"
+	"slices"
 	"testing"
+	"unsafe"
 
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/stretchr/testify/require"
@@ -46,6 +50,9 @@ func newRowSetTestCollector(partialRows bool) *Collector[rowSetValues] {
 			newRow: func(instance string) rowSetValues {
 				return rowSetValues{Name: instance}
 			},
+			setName: func(row *rowSetValues, instance string) {
+				row.Name = instance
+			},
 			setValue: func(row *rowSetValues, field int, value float64) {
 				reflect.ValueOf(row).Elem().Field(field).SetFloat(value)
 			},
@@ -67,10 +74,12 @@ func fillRows(t *testing.T, c *Collector[rowSetValues]) []rowSetValues {
 	var dst []rowSetValues
 
 	rows := rowSet[rowSetValues]{
-		c:         c,
+		c:           c,
-		dst:       &dst,
+		dst:         &dst,
-		index:     map[string]int{},
+		index:       map[instanceKey]int{},
-		nameCache: map[string]string{},
+		nameCache:   map[string]string{},
+		occurrences: map[string]int{},
+		seen:        map[string]int{},
 	}
 
 	for _, name := range []string{"complete", "partial"} {
@@ -80,6 +89,8 @@ func fillRows(t *testing.T, c *Collector[rowSetValues]) []rowSetValues {
 		rows.setValid(row, 0)
 	}
 
+	clear(rows.seen)
+
 	_, ok := rows.row(&c.counters[1], windows.StringToUTF16Ptr("partial"), CstatusInvalidData)
 	require.False(t, ok)
 
@@ -119,6 +130,231 @@ func TestRowSetPartialRows(t *testing.T) {
 	require.Zero(t, rows[1].Optional)
 }
 
+func TestRowSetDuplicateInstances(t *testing.T) {
+	t.Parallel()
+
+	nan := math.NaN()
+
+	for _, tc := range []struct {
+		name        string
+		names       []string
+		secondNames []string
+		a           []float64
+		b           []float64
+		repeatArray bool
+		want        []rowSetValues
+	}{
+		{
+			name:  "duplicates",
+			names: []string{"A", "A", "A"},
+			a:     []float64{1, 2, 3},
+			b:     []float64{10, 20, 30},
+			want: []rowSetValues{
+				{Name: "A", A: 1, B: 10},
+				{Name: "A#1", A: 2, B: 20},
+				{Name: "A#2", A: 3, B: 30},
+			},
+		},
+		{
+			name:        "interleaved_names",
+			names:       []string{"A", "B", "A"},
+			secondNames: []string{"B", "A", "A"},
+			a:           []float64{1, 2, 3},
+			b:           []float64{20, 10, 30},
+			want: []rowSetValues{
+				{Name: "A", A: 1, B: 10},
+				{Name: "B", A: 2, B: 20},
+				{Name: "A#1", A: 3, B: 30},
+			},
+		},
+		{
+			name:        "literal_suffix",
+			names:       []string{"A", "A", "A#1"},
+			secondNames: []string{"A#1", "A", "A"},
+			a:           []float64{1, 2, 3},
+			b:           []float64{30, 10, 20},
+			want: []rowSetValues{
+				{Name: "A", A: 1, B: 10},
+				{Name: "A#2", A: 2, B: 20},
+				{Name: "A#1", A: 3, B: 30},
+			},
+		},
+		{
+			name:  "invalid_middle_item",
+			names: []string{"A", "A", "A"},
+			a:     []float64{1, nan, 3},
+			b:     []float64{10, 20, 30},
+			want: []rowSetValues{
+				{Name: "A", A: 1, B: 10},
+				{Name: "A#1", A: nan, B: 20},
+				{Name: "A#2", A: 3, B: 30},
+			},
+		},
+		{
+			name:  "invalid_first_item",
+			names: []string{"A", "A", "A"},
+			a:     []float64{1, 2, 3},
+			b:     []float64{nan, 20, 30},
+			want: []rowSetValues{
+				{Name: "A", A: 1, B: nan},
+				{Name: "A#1", A: 2, B: 20},
+				{Name: "A#2", A: 3, B: 30},
+			},
+		},
+		{
+			name:  "invalid_literal_suffix",
+			names: []string{"A", "A", "A#1"},
+			a:     []float64{1, 2, nan},
+			b:     []float64{10, 20, nan},
+			want: []rowSetValues{
+				{Name: "A", A: 1, B: 10},
+				{Name: "A#2", A: 2, B: 20},
+			},
+		},
+		{
+			name:        "overlapping_counter_arrays",
+			names:       []string{"A", "A"},
+			a:           []float64{1, 2},
+			b:           []float64{10, 20},
+			repeatArray: true,
+			want: []rowSetValues{
+				{Name: "A", A: 1, B: 10},
+				{Name: "A#1", A: 2, B: 20},
+			},
+		},
+	} {
+		for _, resultType := range []CounterType{CounterTypeRaw, CounterTypeFormatted} {
+			for _, partialRows := range []bool{false, true} {
+				t.Run(fmt.Sprintf("%s/%s/partial=%t", tc.name, resultType, partialRows), func(t *testing.T) {
+					t.Parallel()
+
+					c := newRowSetTestCollector(partialRows)
+					c.counters[1].FieldIndexSecondValue = -1
+
+					var dst []rowSetValues
+
+					rows := rowSet[rowSetValues]{
+						c:           c,
+						dst:         &dst,
+						index:       map[instanceKey]int{},
+						nameCache:   map[string]string{},
+						occurrences: map[string]int{},
+						seen:        map[string]int{},
+					}
+
+					addDuplicateTestItems(
+						&rows,
+						resultType,
+						0,
+						tc.names,
+						tc.a,
+					)
+
+					if tc.repeatArray {
+						addDuplicateTestItems(
+							&rows,
+							resultType,
+							0,
+							tc.names,
+							tc.a,
+						)
+					}
+
+					secondNames := tc.secondNames
+					if secondNames == nil {
+						secondNames = tc.names
+					}
+
+					addDuplicateTestItems(
+						&rows,
+						resultType,
+						1,
+						secondNames,
+						tc.b,
+					)
+					rows.finish()
+
+					want := slices.DeleteFunc(slices.Clone(tc.want), func(row rowSetValues) bool {
+						return !partialRows && (math.IsNaN(row.A) || math.IsNaN(row.B))
+					})
+					require.Len(t, dst, len(want))
+
+					for _, expected := range want {
+						i := slices.IndexFunc(dst, func(row rowSetValues) bool { return row.Name == expected.Name })
+						require.NotEqual(t, -1, i, "missing instance %s", expected.Name)
+
+						for field, value := range map[string]float64{"A": expected.A, "B": expected.B} {
+							got := reflect.ValueOf(dst[i]).FieldByName(field).Float()
+							if math.IsNaN(value) {
+								require.True(t, math.IsNaN(got), "%s/%s: %v", expected.Name, field, got)
+
+								continue
+							}
+
+							require.InDelta(t, value, got, 0, "%s/%s", expected.Name, field)
+						}
+					}
+				})
+			}
+		}
+	}
+}
+
+// addDuplicateTestItems supplies counter arrays without querying PDH. NaN marks an invalid item.
+func addDuplicateTestItems(
+	rows *rowSet[rowSetValues],
+	resultType CounterType,
+	counterIndex int,
+	names []string,
+	values []float64,
+) {
+	if resultType == CounterTypeRaw {
+		items := make([]RawCounterItem, len(names))
+		for i, name := range names {
+			status := CstatusValidData
+			if math.IsNaN(values[i]) {
+				status = CstatusInvalidData
+			}
+
+			items[i] = RawCounterItem{
+				SzName: windows.StringToUTF16Ptr(name),
+				RawValue: RawCounter{
+					CStatus:    status,
+					FirstValue: int64(values[i]),
+				},
+			}
+		}
+
+		itemSize := int(unsafe.Sizeof(RawCounterItem{}))
+		buf := unsafe.Slice((*byte)(unsafe.Pointer(unsafe.SliceData(items))), len(items)*itemSize)
+		rows.addRawItems(counterIndex, buf, uint32(len(items)))
+		runtime.KeepAlive(items)
+
+		return
+	}
+
+	items := make([]FmtCounterValueItemDouble, len(names))
+	for i, name := range names {
+		status := CstatusNewData
+		if math.IsNaN(values[i]) {
+			status = CstatusInvalidData
+		}
+
+		items[i] = FmtCounterValueItemDouble{
+			SzName: windows.StringToUTF16Ptr(name),
+			FmtValue: FmtCounterValueDouble{
+				CStatus:     status,
+				DoubleValue: values[i],
+			},
+		}
+	}
+
+	itemSize := int(unsafe.Sizeof(FmtCounterValueItemDouble{}))
+	buf := unsafe.Slice((*byte)(unsafe.Pointer(unsafe.SliceData(items))), len(items)*itemSize)
+	rows.addFormattedItems(counterIndex, buf, uint32(len(items)))
+	runtime.KeepAlive(items)
+}
+
 func TestSetRawValueElapsedTime(t *testing.T) {
 	t.Parallel()