From 3153b0e6c5b1a3cccdee14d2b0dd9e6df48087ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan-Otto=20Kr=C3=B6pke?= Date: Fri, 9 Oct 2026 01:43:05 +0200 Subject: [PATCH] pdh: preserve duplicate counter instances (#2564) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jan-Otto Kröpke --- internal/pdh/collector.go | 95 +++++++-- internal/pdh/collector_internal_test.go | 244 +++++++++++++++++++++++- 2 files changed, 317 insertions(+), 22 deletions(-) diff --git a/internal/pdh/collector.go b/internal/pdh/collector.go index 902cb0b1..86636475 100644 --- a/internal/pdh/collector.go +++ b/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 -// instance is only collected if every counter has a valid value for it in that +// The optional field Name (string) of T receives the instance name. Occurrence +// 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, - dst: dst, - index: map[string]int{}, - nameCache: map[string]string{}, - valid: state.valid[:0], + c: c, + dst: dst, + index: map[instanceKey]int{}, + nameCache: map[string]string{}, + 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 == "*" { - instanceName = InstanceEmpty - } - - if index, ok := r.index[instanceName]; ok { + key := instanceKey{name: instanceName, occurrence: occurrence} + if index, ok := r.index[key]; 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 diff --git a/internal/pdh/collector_internal_test.go b/internal/pdh/collector_internal_test.go index 57c64231..742f526a 100644 --- a/internal/pdh/collector_internal_test.go +++ b/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, - dst: &dst, - index: map[string]int{}, - nameCache: map[string]string{}, + c: c, + dst: &dst, + index: map[instanceKey]int{}, + 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()