From 43d77f68691bf00a86b2a33d682f37577dc866c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan-Otto=20Kr=C3=B6pke?= Date: Fri, 16 Oct 2026 09:58:43 +0200 Subject: [PATCH] pdh: report duplicate instances by occurrence instead of renaming them (#2742) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jan-Otto Kröpke --- AGENTS.md | 13 +- UPGRADE.md | 3 +- docs/collector.hyperv.md | 4 +- docs/collector.performancecounter.md | 4 + .../exchange/exchange_ad_access_processes.go | 3 + .../hyperv/hyperv_dynamic_memory_vm.go | 51 ++-- .../hyperv_dynamic_memory_vm_internal_test.go | 107 +++++++- .../hyperv_hypervisor_virtual_processor.go | 48 +++- .../collector/hyperv/hyperv_replica_vm.go | 8 +- .../hyperv_virtual_machine_vid_partition.go | 8 +- .../collector/hyperv/hyperv_virtual_switch.go | 8 +- internal/collector/iis/AGENTS.md | 9 +- internal/collector/mssql/AGENTS.md | 8 + .../collector/netframework/netframework.go | 28 +++ .../netframework_clrexceptions.go | 5 + .../netframework/netframework_clrinterop.go | 5 + .../netframework/netframework_clrjit.go | 5 + .../netframework/netframework_clrloading.go | 5 + .../netframework_clrlocksandthreads.go | 5 + .../netframework/netframework_clrmemory.go | 5 + .../netframework/netframework_clrremoting.go | 5 + .../netframework/netframework_clrsecurity.go | 5 + .../netframework_counter_paths_test.go | 3 +- .../netframework/netframework_pdh_test.go | 36 +++ .../performancecounter/performancecounter.go | 7 + .../performancecounter_test_test.go | 40 +++ internal/pdh/collector.go | 236 ++++++++++-------- internal/pdh/collector_internal_test.go | 228 ++++++++++------- internal/pdh/collector_rows_internal_test.go | 71 +++--- internal/pdh/collector_rowset_bench_test.go | 2 + internal/pdh/collector_test.go | 64 +++++ internal/pdh/occurrence.go | 69 +++++ 32 files changed, 832 insertions(+), 266 deletions(-) create mode 100644 internal/pdh/occurrence.go diff --git a/AGENTS.md b/AGENTS.md index f7554672..806bfd70 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -113,9 +113,16 @@ exercised a live Windows role. factor. The disk and `smbclient` collectors still use the fixed 1e-7 (10 MHz) factor. Never publish an invalid sample as zero: that can look like a counter reset. -- Keep the typed PDH collectors and `perfdata` tags. Preserve duplicate - instance occurrences within a sample; their suffixes are not stable identities. - Do not remove every name ending in `_Total`: legitimate process and database +- Keep the typed PDH collectors and `perfdata` tags. `internal/pdh` passes + instance names through unchanged, so a `#N` in a name always comes from + Windows (IIS recycling, `Ethernet#2`). Some objects report one name several + times per sample (`Process`, `.NET CLR *`, Exchange ADAccess processes, + Hyper-V objects keyed by user-chosen VM or switch names). Rows without an + `Occurrence int` field get only the first valid instance of a name, and pdh + logs a warning once. Any object keyed by names users or applications choose + can repeat them: declare the field there and choose the label value, + usually with `pdh.SuffixDuplicates`. Those suffixes are not stable + identities. Do not remove every name ending in `_Total`: legitimate process and database names can use that suffix. Use the shared aggregate-instance policy. `pdh.Collector.Collect` reuses the backing array of its destination slice and `Row.Values`; do not keep rows, sub-slices or pointers into it across diff --git a/UPGRADE.md b/UPGRADE.md index 492b5741..ee037321 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -341,6 +341,7 @@ Label changes on existing metrics: - The IIS HTTP request queue metrics (`windows_iis_http_requests_*`) have the label `app` instead of `site`. The value is unchanged: it always was the application pool that owns the HTTP.sys request queue, not a site. They are now filtered with `--collector.iis.app-include` and `--collector.iis.app-exclude` instead of the site filters. - The `windows_netframework_clrmemory_*` metrics have a `process_id` label. - In the `windows_hyperv_dynamic_memory_vm_*` metrics, unnamed VM instances have `vm="(unknown)"` (or `(unknown)#N`) instead of `vm="------"`. +- VMs with the same name are published apart in the `windows_hyperv_dynamic_memory_vm_*`, `windows_hyperv_vid_*`, `windows_hyperv_hypervisor_virtual_processor_*` and `windows_hyperv_replica_vm_*` performance counter metrics: the first keeps its name, later ones have `vm="#1"`, `vm="#2"` and so on. Virtual switches with the same name get the same suffixes in the `windows_hyperv_vswitch_*` metrics. In 0.31 they overwrote each other. The 0.32 betas put the suffix of a duplicate virtual processor into the `core` label (`core="0#1"`); `core` is now always the processor number. - `windows_terminal_services_connection_broker_performance_total` no longer has a `connection="Pending"` series. It was the current number of pending connections, published as a counter. Use the gauge `windows_terminal_services_connection_broker_pending_connections`. - `windows_terminal_services_cpu_time_seconds_total` has the CPU mode in `mode` and the session name in `session_name`. In 0.31 the two label values were swapped, so queries that filter on either label need updating. @@ -370,7 +371,7 @@ These metrics keep their names, but 0.31 reported wrong values. Graphs show a st - In the `performancecounter` collector, counters without an explicit `type` now always get the type of the underlying Windows counter. In 0.31 a mixed object could report counters as gauges, or the other way around, and the type could change between scrapes. - Instances without a valid sample are left out. In 0.31 they were reported as `0`, which looked like a counter reset. - An explicit instance list such as `instances: ["C:", "D:"]` now returns one series per instance. In 0.31 all listed instances were merged into one series. -- When Windows reports the same instance name more than once, each occurrence is a separate series. In 0.31 they overwrote each other. +- When Windows reports the same instance name more than once, for example every `svchost` process of the `Process` object, the `performancecounter`, `netframework`, `hyperv` and `exchange` (ADAccess processes) collectors publish each occurrence as a separate series. The first keeps the name, later ones get the suffix `#1`, `#2` and so on (`exchange` keeps its `_2`, `_3` label suffixes), which depends on the order of the sample and does not identify the instance across scrapes. The other PDH-based collectors read objects that Windows names uniquely; if one still reports a name twice, they publish the first instance with valid data and log a warning. In 0.31 the occurrences overwrote each other. - Instances whose name ends in `_Total`, such as a process `worker_Total` or a database `Orders_Total`, are exported. In 0.31 every name ending in `_Total` was dropped as an aggregate. ### service start mode diff --git a/docs/collector.hyperv.md b/docs/collector.hyperv.md index f4956eed..03805f6d 100644 --- a/docs/collector.hyperv.md +++ b/docs/collector.hyperv.md @@ -115,7 +115,9 @@ Some metrics explained: https://learn.microsoft.com/en-us/archive/blogs/chrisavi Windows reports no instance name for VMs managed by the Host Compute Service, such as the WSL 2 VM. The first such VM has `vm="(unknown)"`, further ones `vm="(unknown)#1"`, `vm="(unknown)#2"` and so on. -The suffix only keeps the series unique and does not identify the VM across scrapes. +VMs with the same name get the same suffixes, here and in the VM Vid Partition, Hypervisor Virtual Processor and Replica VM performance counter metrics, and so do virtual switches with the same name. +All cores of one VM share its suffix; this relies on Windows listing the virtual processors of a VM together, starting with core 0. +The suffix only keeps the series unique and does not identify the VM across scrapes or between these metric groups. ### Hyper-V Host diff --git a/docs/collector.performancecounter.md b/docs/collector.performancecounter.md index fd9ef5f0..c35f77be 100644 --- a/docs/collector.performancecounter.md +++ b/docs/collector.performancecounter.md @@ -165,6 +165,10 @@ The name of the label that holds the instance name. Optional and defaults to `in The label is only added if `instances` is set. +Some objects report the same instance name more than once in a sample, for example `Process` reports every `svchost` process as `svchost`. +The first instance keeps its name, later ones get the suffix `#1`, `#2` and so on, skipping names that Windows reported itself, such as a network adapter `Ethernet#2`. +The suffix depends on the order of the sample and does not identify an instance across scrapes. + #### counters List of counters to collect from the object. See the counters sub-schema for more information. diff --git a/internal/collector/exchange/exchange_ad_access_processes.go b/internal/collector/exchange/exchange_ad_access_processes.go index c8ae39ed..f5f8fc23 100644 --- a/internal/collector/exchange/exchange_ad_access_processes.go +++ b/internal/collector/exchange/exchange_ad_access_processes.go @@ -42,6 +42,9 @@ type collectorADAccessProcesses struct { type perfDataCounterValuesADAccessProcesses struct { Name string + // Occurrence makes pdh return every process with the same name; the + // label logic in collectADAccessProcesses keeps them apart. + Occurrence int LdapReadTime float64 `perfdata:"LDAP Read Time"` LdapReadOperationsTotal float64 `perfdata:"LDAP Read Time,secondvalue"` diff --git a/internal/collector/hyperv/hyperv_dynamic_memory_vm.go b/internal/collector/hyperv/hyperv_dynamic_memory_vm.go index 0a425d37..58367886 100644 --- a/internal/collector/hyperv/hyperv_dynamic_memory_vm.go +++ b/internal/collector/hyperv/hyperv_dynamic_memory_vm.go @@ -19,7 +19,6 @@ package hyperv import ( "fmt" - "strings" "github.com/prometheus-community/windows_exporter/internal/labelcache" "github.com/prometheus-community/windows_exporter/internal/osversion" @@ -47,7 +46,8 @@ type collectorDynamicMemoryVM struct { } type perfDataCounterValuesDynamicMemoryVM struct { - Name string + Name string + Occurrence int // Hyper-V Dynamic Memory VM metrics VmMemoryAddedMemory float64 `perfdata:"Added Memory"` @@ -140,8 +140,10 @@ func (c *Collector) collectDynamicMemoryVM(ch chan<- prometheus.Metric, scrape * return fmt.Errorf("failed to collect Hyper-V Dynamic Memory VM metrics: %w", err) } + nameDynamicMemoryVMs(c.perfDataObjectDynamicMemoryVM) + for _, data := range c.perfDataObjectDynamicMemoryVM { - vmName := dynamicMemoryVMName(data.Name) + vmName := data.Name ch <- scrape.MustNewConstMetric( c.vmMemoryAddedMemory, @@ -223,19 +225,40 @@ func (c *Collector) collectDynamicMemoryVM(ch chan<- prometheus.Metric, scrape * // without a name. const vmUnknown = "(unknown)" -// dynamicMemoryVMName returns the vm label value for a Hyper-V Dynamic Memory VM -// instance. Windows reports an empty instance name for VMs managed by the Host -// Compute Service, e.g. WSL 2, which pdh exposes as -// pdh.InstanceEmpty. The occurrence suffix of duplicates is kept, so the -// series stay unique. -func dynamicMemoryVMName(name string) string { - if name == pdh.InstanceEmpty { - return vmUnknown +// nameDynamicMemoryVMs replaces the instance names of rows with their vm label +// values. Windows reports an empty instance name for VMs managed by the Host +// Compute Service, e.g. WSL 2, which pdh exposes as pdh.InstanceEmpty. Those +// VMs are named vmUnknown before suffixDuplicateVMs keeps several of them, or +// VMs with the same name, apart. +func nameDynamicMemoryVMs(rows []perfDataCounterValuesDynamicMemoryVM) { + // A VM can be named "(unknown)" itself. The unnamed VMs then count as + // further occurrences of that name, so that they get a suffix. + unknownVMs := 0 + + for _, row := range rows { + if row.Name == vmUnknown { + unknownVMs = max(unknownVMs, row.Occurrence+1) + } } - if suffix, ok := strings.CutPrefix(name, pdh.InstanceEmpty+"#"); ok { - return vmUnknown + "#" + suffix + for i := range rows { + if rows[i].Name == pdh.InstanceEmpty { + rows[i].Name = vmUnknown + rows[i].Occurrence += unknownVMs + } } - return name + suffixDuplicateVMs(rows, + func(row *perfDataCounterValuesDynamicMemoryVM) *string { return &row.Name }, + func(row *perfDataCounterValuesDynamicMemoryVM) int { return row.Occurrence }, + ) +} + +// suffixDuplicateVMs keeps VMs with the same name apart. Hyper-V allows several +// VMs with the same name, and their counter instances then have the same name +// too. The first keeps the name, later ones get the suffix "#1", "#2" and so on. +// The suffix depends on the order of the sample and does not identify a VM +// across scrapes or across counter objects. +func suffixDuplicateVMs[T any](rows []T, name func(*T) *string, occurrence func(*T) int) { + pdh.SuffixDuplicates(rows, name, occurrence) } diff --git a/internal/collector/hyperv/hyperv_dynamic_memory_vm_internal_test.go b/internal/collector/hyperv/hyperv_dynamic_memory_vm_internal_test.go index 929c895d..2c895046 100644 --- a/internal/collector/hyperv/hyperv_dynamic_memory_vm_internal_test.go +++ b/internal/collector/hyperv/hyperv_dynamic_memory_vm_internal_test.go @@ -21,20 +21,107 @@ import ( "testing" "github.com/prometheus-community/windows_exporter/internal/pdh" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/require" ) -func TestDynamicMemoryVMName(t *testing.T) { +func TestNameDynamicMemoryVMs(t *testing.T) { t.Parallel() - for name, want := range map[string]string{ - pdh.InstanceEmpty: "(unknown)", - pdh.InstanceEmpty + "#1": "(unknown)#1", - pdh.InstanceEmpty + "#12": "(unknown)#12", - "GitHubActions": "GitHubActions", - "GitHubActions#1": "GitHubActions#1", - "vm-------": "vm-------", - } { - require.Equal(t, want, dynamicMemoryVMName(name), name) + rows := []perfDataCounterValuesDynamicMemoryVM{ + {Name: pdh.InstanceEmpty}, + {Name: pdh.InstanceEmpty, Occurrence: 1}, + {Name: pdh.InstanceEmpty, Occurrence: 2}, + {Name: "GitHubActions"}, + {Name: "GitHubActions", Occurrence: 1}, + // A VM named like a suffixed duplicate keeps its name. + {Name: "dup#1"}, + {Name: "dup"}, + {Name: "dup", Occurrence: 1}, + {Name: "vm-------"}, } + + nameDynamicMemoryVMs(rows) + + names := make([]string, 0, len(rows)) + for _, row := range rows { + names = append(names, row.Name) + } + + require.Equal(t, []string{ + "(unknown)", "(unknown)#1", "(unknown)#2", + "GitHubActions", "GitHubActions#1", + "dup#1", "dup", "dup#2", + "vm-------", + }, names) +} + +// TestNameDynamicMemoryVMsLiteralUnknown checks that a VM named "(unknown)" +// and an unnamed VM are published apart. +func TestNameDynamicMemoryVMsLiteralUnknown(t *testing.T) { + t.Parallel() + + rows := []perfDataCounterValuesDynamicMemoryVM{ + {Name: pdh.InstanceEmpty}, + {Name: vmUnknown}, + {Name: pdh.InstanceEmpty, Occurrence: 1}, + } + + nameDynamicMemoryVMs(rows) + + require.Equal(t, "(unknown)#1", rows[0].Name) + require.Equal(t, "(unknown)", rows[1].Name) + require.Equal(t, "(unknown)#2", rows[2].Name) +} + +// TestVirtualProcessorDuplicateVMs checks that the virtual processors of VMs +// with the same name are published with a suffixed vm label and an unchanged +// core label. +func TestVirtualProcessorDuplicateVMs(t *testing.T) { + t.Parallel() + + c := New(nil) + c.buildHypervisorVirtualProcessorDescriptors() + + c.perfDataObjectHypervisorVirtualProcessor = []perfDataCounterValuesHypervisorVirtualProcessor{ + {Name: "dup:Hv VP 0", HypervisorVirtualProcessorDispatches: 1}, + {Name: "dup:Hv VP 1", HypervisorVirtualProcessorDispatches: 2}, + {Name: "dup:Hv VP 0", Occurrence: 1, HypervisorVirtualProcessorDispatches: 3}, + {Name: "other:Hv VP 0", HypervisorVirtualProcessorDispatches: 4}, + // The first "web" VM has one core, the second two. pdh reports the + // second VM's core 1 as the first occurrence of "web:Hv VP 1". + {Name: "web:Hv VP 0", HypervisorVirtualProcessorDispatches: 5}, + {Name: "web:Hv VP 0", Occurrence: 1, HypervisorVirtualProcessorDispatches: 6}, + {Name: "web:Hv VP 1", HypervisorVirtualProcessorDispatches: 7}, + } + + registry := prometheus.NewPedanticRegistry() + require.NoError(t, registry.Register(publishCollector(func(ch chan<- prometheus.Metric) error { + return c.publishHypervisorVirtualProcessor(ch, nil) + }))) + + families, err := registry.Gather() + require.NoError(t, err) + + got := map[string]float64{} + + for _, family := range families { + if family.GetName() != "windows_hyperv_hypervisor_virtual_processor_dispatches_total" { + continue + } + + for _, metric := range family.GetMetric() { + labels := map[string]string{} + for _, label := range metric.GetLabel() { + labels[label.GetName()] = label.GetValue() + } + + got[labels["vm"]+"/"+labels["core"]] = metric.GetCounter().GetValue() + } + } + + require.Equal(t, map[string]float64{ + "dup/0": 1, "dup/1": 2, "dup#1/0": 3, "other/0": 4, + "web/0": 5, "web#1/0": 6, "web#1/1": 7, + }, got) } diff --git a/internal/collector/hyperv/hyperv_hypervisor_virtual_processor.go b/internal/collector/hyperv/hyperv_hypervisor_virtual_processor.go index 32fbae0e..fcefe48a 100644 --- a/internal/collector/hyperv/hyperv_hypervisor_virtual_processor.go +++ b/internal/collector/hyperv/hyperv_hypervisor_virtual_processor.go @@ -44,7 +44,8 @@ type collectorHypervisorVirtualProcessor struct { } type perfDataCounterValuesHypervisorVirtualProcessor struct { - Name string + Name string + Occurrence int // The run time counters and CPU Wait Time Per Dispatch are PERF_PRECISION_100NS_TIMER. // Their raw values are cumulative times, which pdh scales to seconds. Despite its name @@ -123,12 +124,34 @@ func (c *Collector) collectHypervisorVirtualProcessor(ch chan<- prometheus.Metri return c.publishHypervisorVirtualProcessor(ch, scrape) } +// assignVirtualProcessorVMs sets the Occurrence of each virtual processor row +// to the index of its VM among the VMs with the same name, so that all cores +// of one VM get the same vm label. pdh counts occurrences per instance name +// (":Hv VP "), which would give the second core of the second VM the +// occurrence of the first VM if the first VM has a single core. Windows lists +// the virtual processors of a VM together, starting with core 0, in the order +// that the rows keep, so each core 0 starts the next VM with that name. +func assignVirtualProcessorVMs(rows []perfDataCounterValuesHypervisorVirtualProcessor, coreIDs []string) { + vms := make(map[string]int, len(rows)) + + for i := range rows { + if coreIDs[i] == "0" { + vms[rows[i].Name]++ + } + + rows[i].Occurrence = max(vms[rows[i].Name]-1, 0) + } +} + func (c *Collector) publishHypervisorVirtualProcessor(ch chan<- prometheus.Metric, scrape *labelcache.Scrape) error { - for _, data := range c.perfDataObjectHypervisorVirtualProcessor { + rows := c.perfDataObjectHypervisorVirtualProcessor + coreIDs := make([]string, len(rows)) + + for i := range rows { // The name format is :Hv VP - parts := strings.Split(data.Name, ":") + parts := strings.Split(rows[i].Name, ":") if len(parts) != 2 { - return fmt.Errorf("unexpected format of Name in Hyper-V Hypervisor Virtual Processor: %q, expected %q", data.Name, ":Hv VP ") + return fmt.Errorf("unexpected format of Name in Hyper-V Hypervisor Virtual Processor: %q, expected %q", rows[i].Name, ":Hv VP ") } coreParts := strings.Split(parts[1], " ") @@ -136,8 +159,21 @@ func (c *Collector) publishHypervisorVirtualProcessor(ch chan<- prometheus.Metri return fmt.Errorf("unexpected format of core identifier in Hyper-V Hypervisor Virtual Processor: %q, expected %q", parts[1], "Hv VP ") } - vmName := parts[0] - coreID := coreParts[2] + rows[i].Name = parts[0] + coreIDs[i] = coreParts[2] + } + + assignVirtualProcessorVMs(rows, coreIDs) + + // VMs with the same name report the same instance names, see suffixDuplicateVMs. + suffixDuplicateVMs(rows, + func(row *perfDataCounterValuesHypervisorVirtualProcessor) *string { return &row.Name }, + func(row *perfDataCounterValuesHypervisorVirtualProcessor) int { return row.Occurrence }, + ) + + for i, data := range rows { + vmName := data.Name + coreID := coreIDs[i] ch <- scrape.MustNewConstMetric( c.hypervisorVirtualProcessorTimeTotal, diff --git a/internal/collector/hyperv/hyperv_replica_vm.go b/internal/collector/hyperv/hyperv_replica_vm.go index c7be1060..920d1983 100644 --- a/internal/collector/hyperv/hyperv_replica_vm.go +++ b/internal/collector/hyperv/hyperv_replica_vm.go @@ -57,7 +57,8 @@ type collectorReplicaVM struct { } type perfDataCounterValuesReplicaVM struct { - Name string + Name string + Occurrence int AverageReplicationLatency float64 `perfdata:"Average Replication Latency"` AverageReplicationSize float64 `perfdata:"Average Replication Size"` @@ -198,6 +199,11 @@ func (c *Collector) collectReplicaVMPerfData(ch chan<- prometheus.Metric, scrape return fmt.Errorf("failed to collect Hyper-V Replica VM metrics: %w", err) } + suffixDuplicateVMs(c.perfDataObjectReplicaVM, + func(row *perfDataCounterValuesReplicaVM) *string { return &row.Name }, + func(row *perfDataCounterValuesReplicaVM) int { return row.Occurrence }, + ) + for _, data := range c.perfDataObjectReplicaVM { ch <- scrape.MustNewConstMetric( c.replicaVMAverageReplicationLatency, diff --git a/internal/collector/hyperv/hyperv_virtual_machine_vid_partition.go b/internal/collector/hyperv/hyperv_virtual_machine_vid_partition.go index 28411d9a..de4c7bc7 100644 --- a/internal/collector/hyperv/hyperv_virtual_machine_vid_partition.go +++ b/internal/collector/hyperv/hyperv_virtual_machine_vid_partition.go @@ -37,7 +37,8 @@ type collectorVirtualMachineVidPartition struct { } type perfDataCounterValuesVirtualMachineVidPartition struct { - Name string + Name string + Occurrence int PhysicalPagesAllocated float64 `perfdata:"Physical Pages Allocated"` PreferredNUMANodeIndex float64 `perfdata:"Preferred NUMA Node Index"` @@ -80,6 +81,11 @@ func (c *Collector) collectVirtualMachineVidPartition(ch chan<- prometheus.Metri return fmt.Errorf("failed to collect Hyper-V VM Vid Partition metrics: %w", err) } + suffixDuplicateVMs(c.perfDataObjectVirtualMachineVidPartition, + func(row *perfDataCounterValuesVirtualMachineVidPartition) *string { return &row.Name }, + func(row *perfDataCounterValuesVirtualMachineVidPartition) int { return row.Occurrence }, + ) + for _, data := range c.perfDataObjectVirtualMachineVidPartition { ch <- scrape.MustNewConstMetric( c.physicalPagesAllocated, diff --git a/internal/collector/hyperv/hyperv_virtual_switch.go b/internal/collector/hyperv/hyperv_virtual_switch.go index 3ee80a09..32be0715 100644 --- a/internal/collector/hyperv/hyperv_virtual_switch.go +++ b/internal/collector/hyperv/hyperv_virtual_switch.go @@ -55,7 +55,8 @@ type collectorVirtualSwitch struct { } type perfDataCounterValuesVirtualSwitch struct { - Name string + Name string + Occurrence int VirtualSwitchBroadcastPacketsReceived float64 `perfdata:"Broadcast Packets Received/sec"` VirtualSwitchBroadcastPacketsSent float64 `perfdata:"Broadcast Packets Sent/sec"` @@ -224,6 +225,11 @@ func (c *Collector) collectVirtualSwitch(ch chan<- prometheus.Metric, scrape *la return fmt.Errorf("failed to collect Hyper-V Virtual Switch metrics: %w", err) } + suffixDuplicateVMs(c.perfDataObjectVirtualSwitch, + func(row *perfDataCounterValuesVirtualSwitch) *string { return &row.Name }, + func(row *perfDataCounterValuesVirtualSwitch) int { return row.Occurrence }, + ) + for _, data := range c.perfDataObjectVirtualSwitch { ch <- scrape.MustNewConstMetric( c.virtualSwitchBroadcastPacketsReceived, diff --git a/internal/collector/iis/AGENTS.md b/internal/collector/iis/AGENTS.md index 6705c3d6..697074bb 100644 --- a/internal/collector/iis/AGENTS.md +++ b/internal/collector/iis/AGENTS.md @@ -74,7 +74,14 @@ notes were recorded on Windows Server 2025 (build 26100) with only exist only while a worker process runs; send one HTTP request to start one. `HTTP Service Request Queues` instances are app pool names plus about 40 `---` queues, which the collector skips. Recycled instances get a `#` - suffix that the collector strips. + suffix that the collector strips. `internal/pdh` adds no suffix of its own. + The IIS rows have no `Occurrence` field, because no IIS object repeats an + instance name in one sample: IIS rejects duplicate site and application + pool names (`Web Service`, `APP_POOL_WAS`, `HTTP Service Request Queues`), + `W3SVC_W3WP` instances carry the worker process ID, and the overlapping + worker of a recycle has its own PID. A sample on this host had no + duplicates in these objects. If pdh logs dropped duplicates for an IIS + object, declare the field. ## Counter reference diff --git a/internal/collector/mssql/AGENTS.md b/internal/collector/mssql/AGENTS.md index dfdba77e..07b5c5ab 100644 --- a/internal/collector/mssql/AGENTS.md +++ b/internal/collector/mssql/AGENTS.md @@ -33,6 +33,14 @@ Edition (`PatchLevel` 16.0.1000.6), named instance `NONDEFAULT`. 15 including `_Total`. `SQL Errors` instances are error classes (`User Errors`, `Info Errors`, `Kill Connection Errors`, `DB Offline Errors`); `_Total` equalled their sum (38 = 34 + 4). +- No row type declares `Occurrence` (see `internal/pdh`), because no object + repeats an instance name in one sample: database names are unique per + instance, a database belongs to at most one availability group + (`Database Replica`), `Availability Replica` instances are replicas of the + instance's availability groups, and the other instances are fixed lock + types, error classes, wait statistics or the single unnamed instance. A + sample on this host had no duplicates in any of these objects. If pdh + ever logs dropped duplicates for an `MSSQL$` object, declare the field. - `Wait Statistics` is unusual: every counter is a wait class and every instance is a statistic, `Average wait time (ms)`, `Cumulative wait time (ms) per second`, `Waits in progress` and diff --git a/internal/collector/netframework/netframework.go b/internal/collector/netframework/netframework.go index db1149d5..4ca8edf2 100644 --- a/internal/collector/netframework/netframework.go +++ b/internal/collector/netframework/netframework.go @@ -380,6 +380,34 @@ func (c *Collector) Collect(ch chan<- prometheus.Metric, maxScrapeDuration time. return g.Wait() } +// clrInstance is implemented by the rows of the .NET CLR objects. +type clrInstance interface { + instance() (name *string, occurrence int) +} + +// suffixDuplicateProcesses keeps processes with the same name apart. The .NET +// CLR objects name instances after the process image and report a name once +// per process, e.g. "powershell" twice for two PowerShell processes. Later +// processes get the suffix "#1", "#2" and so on, which depends on the order +// of the sample and does not identify a process across scrapes. +func suffixDuplicateProcesses[T any, P interface { + *T + clrInstance +}](rows []T) { + pdh.SuffixDuplicates(rows, + func(row *T) *string { + name, _ := P(row).instance() + + return name + }, + func(row *T) int { + _, occurrence := P(row).instance() + + return occurrence + }, + ) +} + // newPerfCollector adds the counters of object to query. On error, // pdh.NewQueryCollector has already removed the counters it added. Missing // objects are optional; missing individual counters remain errors. diff --git a/internal/collector/netframework/netframework_clrexceptions.go b/internal/collector/netframework/netframework_clrexceptions.go index 620cbb8f..e9a259db 100644 --- a/internal/collector/netframework/netframework_clrexceptions.go +++ b/internal/collector/netframework/netframework_clrexceptions.go @@ -65,18 +65,23 @@ func (c *Collector) buildClrExceptions() error { type perfDataClrExceptions struct { Name string + Occurrence int NumberofExcepsThrown float64 `perfdata:"# of Exceps Thrown"` NumberofFiltersPersec float64 `perfdata:"# of Filters / sec"` NumberofFinallysPersec float64 `perfdata:"# of Finallys / sec"` ThrowToCatchDepthPersec float64 `perfdata:"Throw To Catch Depth / sec"` } +func (p *perfDataClrExceptions) instance() (*string, int) { return &p.Name, p.Occurrence } + func (c *Collector) collectClrExceptions(ch chan<- prometheus.Metric, scrape *labelcache.Scrape, _ time.Duration) error { var dst []perfDataClrExceptions if err := c.perfClrExceptions.Collect(&dst); err != nil { return fmt.Errorf("failed to collect .NET CLR Exceptions: %w", err) } + suffixDuplicateProcesses(dst) + for _, process := range dst { if process.Name == "_Global_" { continue diff --git a/internal/collector/netframework/netframework_clrinterop.go b/internal/collector/netframework/netframework_clrinterop.go index 66a5c33b..c4f88a71 100644 --- a/internal/collector/netframework/netframework_clrinterop.go +++ b/internal/collector/netframework/netframework_clrinterop.go @@ -59,17 +59,22 @@ func (c *Collector) buildClrInterop() error { type perfDataClrInterop struct { Name string + Occurrence int NumberofCCWs float64 `perfdata:"# of CCWs"` NumberofStubs float64 `perfdata:"# of Stubs"` Numberofmarshalling float64 `perfdata:"# of marshalling"` } +func (p *perfDataClrInterop) instance() (*string, int) { return &p.Name, p.Occurrence } + func (c *Collector) collectClrInterop(ch chan<- prometheus.Metric, scrape *labelcache.Scrape, _ time.Duration) error { var dst []perfDataClrInterop if err := c.perfClrInterop.Collect(&dst); err != nil { return fmt.Errorf("failed to collect .NET CLR Interop: %w", err) } + suffixDuplicateProcesses(dst) + for _, process := range dst { if process.Name == "_Global_" { continue diff --git a/internal/collector/netframework/netframework_clrjit.go b/internal/collector/netframework/netframework_clrjit.go index b7309fd7..07915e30 100644 --- a/internal/collector/netframework/netframework_clrjit.go +++ b/internal/collector/netframework/netframework_clrjit.go @@ -65,18 +65,23 @@ func (c *Collector) buildClrJIT() error { type perfDataClrJIT struct { Name string + Occurrence int NumberofMethodsJitted float64 `perfdata:"# of Methods JITted"` PercentTimeinJit float64 `perfdata:"% Time in Jit"` StandardJitFailures float64 `perfdata:"Standard Jit Failures"` TotalNumberofILBytesJitted float64 `perfdata:"Total # of IL Bytes Jitted"` } +func (p *perfDataClrJIT) instance() (*string, int) { return &p.Name, p.Occurrence } + func (c *Collector) collectClrJIT(ch chan<- prometheus.Metric, scrape *labelcache.Scrape, _ time.Duration) error { var dst []perfDataClrJIT if err := c.perfClrJIT.Collect(&dst); err != nil { return fmt.Errorf("failed to collect .NET CLR Jit: %w", err) } + suffixDuplicateProcesses(dst) + for _, process := range dst { if process.Name == "_Global_" { continue diff --git a/internal/collector/netframework/netframework_clrloading.go b/internal/collector/netframework/netframework_clrloading.go index 7fe03622..6ac5fba9 100644 --- a/internal/collector/netframework/netframework_clrloading.go +++ b/internal/collector/netframework/netframework_clrloading.go @@ -95,6 +95,7 @@ func (c *Collector) buildClrLoading() error { type perfDataClrLoading struct { Name string + Occurrence int BytesinLoaderHeap float64 `perfdata:"Bytes in Loader Heap"` CurrentAssemblies float64 `perfdata:"Current Assemblies"` CurrentClassesLoaded float64 `perfdata:"Current Classes Loaded"` @@ -106,12 +107,16 @@ type perfDataClrLoading struct { Totalappdomainsunloaded float64 `perfdata:"Total appdomains unloaded"` } +func (p *perfDataClrLoading) instance() (*string, int) { return &p.Name, p.Occurrence } + func (c *Collector) collectClrLoading(ch chan<- prometheus.Metric, scrape *labelcache.Scrape, _ time.Duration) error { var dst []perfDataClrLoading if err := c.perfClrLoading.Collect(&dst); err != nil { return fmt.Errorf("failed to collect .NET CLR Loading: %w", err) } + suffixDuplicateProcesses(dst) + for _, process := range dst { if process.Name == "_Global_" { continue diff --git a/internal/collector/netframework/netframework_clrlocksandthreads.go b/internal/collector/netframework/netframework_clrlocksandthreads.go index 050c999a..275737bf 100644 --- a/internal/collector/netframework/netframework_clrlocksandthreads.go +++ b/internal/collector/netframework/netframework_clrlocksandthreads.go @@ -83,6 +83,7 @@ func (c *Collector) buildClrLocksAndThreads() error { type perfDataClrLocksAndThreads struct { Name string + Occurrence int CurrentQueueLength float64 `perfdata:"Current Queue Length"` NumberofcurrentlogicalThreads float64 `perfdata:"# of current logical Threads"` NumberofcurrentphysicalThreads float64 `perfdata:"# of current physical Threads"` @@ -92,12 +93,16 @@ type perfDataClrLocksAndThreads struct { TotalNumberofContentions float64 `perfdata:"Total # of Contentions"` } +func (p *perfDataClrLocksAndThreads) instance() (*string, int) { return &p.Name, p.Occurrence } + func (c *Collector) collectClrLocksAndThreads(ch chan<- prometheus.Metric, scrape *labelcache.Scrape, _ time.Duration) error { var dst []perfDataClrLocksAndThreads if err := c.perfClrLocksAndThreads.Collect(&dst); err != nil { return fmt.Errorf("failed to collect .NET CLR LocksAndThreads: %w", err) } + suffixDuplicateProcesses(dst) + for _, process := range dst { if process.Name == "_Global_" { continue diff --git a/internal/collector/netframework/netframework_clrmemory.go b/internal/collector/netframework/netframework_clrmemory.go index 80b46452..755153c9 100644 --- a/internal/collector/netframework/netframework_clrmemory.go +++ b/internal/collector/netframework/netframework_clrmemory.go @@ -114,6 +114,7 @@ func (c *Collector) buildClrMemory() error { type perfDataClrMemory struct { Name string + Occurrence int AllocatedBytesPersec float64 `perfdata:"Allocated Bytes/sec"` FinalizationSurvivors float64 `perfdata:"Finalization Survivors"` Gen0PromotedBytesPerSec float64 `perfdata:"Gen 0 Promoted Bytes/Sec"` @@ -136,12 +137,16 @@ type perfDataClrMemory struct { ProcessID float64 `perfdata:"Process ID"` } +func (p *perfDataClrMemory) instance() (*string, int) { return &p.Name, p.Occurrence } + func (c *Collector) collectClrMemory(ch chan<- prometheus.Metric, scrape *labelcache.Scrape, _ time.Duration) error { var dst []perfDataClrMemory if err := c.perfClrMemory.Collect(&dst); err != nil { return fmt.Errorf("failed to collect .NET CLR Memory: %w", err) } + suffixDuplicateProcesses(dst) + for _, process := range dst { if process.Name == "_Global_" { continue diff --git a/internal/collector/netframework/netframework_clrremoting.go b/internal/collector/netframework/netframework_clrremoting.go index c25399b3..38304f35 100644 --- a/internal/collector/netframework/netframework_clrremoting.go +++ b/internal/collector/netframework/netframework_clrremoting.go @@ -77,6 +77,7 @@ func (c *Collector) buildClrRemoting() error { type perfDataClrRemoting struct { Name string + Occurrence int Channels float64 `perfdata:"Channels"` ContextBoundClassesLoaded float64 `perfdata:"Context-Bound Classes Loaded"` ContextBoundObjectsAllocPersec float64 `perfdata:"Context-Bound Objects Alloc / sec"` @@ -85,12 +86,16 @@ type perfDataClrRemoting struct { TotalRemoteCalls float64 `perfdata:"Total Remote Calls"` } +func (p *perfDataClrRemoting) instance() (*string, int) { return &p.Name, p.Occurrence } + func (c *Collector) collectClrRemoting(ch chan<- prometheus.Metric, scrape *labelcache.Scrape, _ time.Duration) error { var dst []perfDataClrRemoting if err := c.perfClrRemoting.Collect(&dst); err != nil { return fmt.Errorf("failed to collect .NET CLR Remoting: %w", err) } + suffixDuplicateProcesses(dst) + for _, process := range dst { if process.Name == "_Global_" { continue diff --git a/internal/collector/netframework/netframework_clrsecurity.go b/internal/collector/netframework/netframework_clrsecurity.go index 9edd9fd2..3539503b 100644 --- a/internal/collector/netframework/netframework_clrsecurity.go +++ b/internal/collector/netframework/netframework_clrsecurity.go @@ -65,18 +65,23 @@ func (c *Collector) buildClrSecurity() error { type perfDataClrSecurity struct { Name string + Occurrence int NumberLinkTimeChecks float64 `perfdata:"# Link Time Checks"` PercentTimeinRTchecks float64 `perfdata:"% Time in RT checks"` StackWalkDepth float64 `perfdata:"Stack Walk Depth"` TotalRuntimeChecks float64 `perfdata:"Total Runtime Checks"` } +func (p *perfDataClrSecurity) instance() (*string, int) { return &p.Name, p.Occurrence } + func (c *Collector) collectClrSecurity(ch chan<- prometheus.Metric, scrape *labelcache.Scrape, _ time.Duration) error { var dst []perfDataClrSecurity if err := c.perfClrSecurity.Collect(&dst); err != nil { return fmt.Errorf("failed to collect .NET CLR Security: %w", err) } + suffixDuplicateProcesses(dst) + for _, process := range dst { if process.Name == "_Global_" { continue diff --git a/internal/collector/netframework/netframework_counter_paths_test.go b/internal/collector/netframework/netframework_counter_paths_test.go index 98be0543..86ec3233 100644 --- a/internal/collector/netframework/netframework_counter_paths_test.go +++ b/internal/collector/netframework/netframework_counter_paths_test.go @@ -40,7 +40,8 @@ func TestClrRemotingCounterPaths(t *testing.T) { "TotalRemoteCalls": "Total Remote Calls", } rowType := reflect.TypeFor[perfDataClrRemoting]() - require.Equal(t, len(expected)+1, rowType.NumField()) + // Name and Occurrence hold the instance. + require.Equal(t, len(expected)+2, rowType.NumField()) for field, counter := range expected { f, found := rowType.FieldByName(field) diff --git a/internal/collector/netframework/netframework_pdh_test.go b/internal/collector/netframework/netframework_pdh_test.go index cb461b8a..30e3fb42 100644 --- a/internal/collector/netframework/netframework_pdh_test.go +++ b/internal/collector/netframework/netframework_pdh_test.go @@ -415,6 +415,42 @@ func TestGCZeroBaseDoesNotPublishInvalidRatio(t *testing.T) { require.True(t, foundAllocated) } +// TestDuplicateProcessNames checks that processes with the same name, which +// the CLR objects report with the same instance name, are published apart. +func TestDuplicateProcessNames(t *testing.T) { + c := &Collector{} + c.describeClrExceptions() + c.perfClrExceptions = &fixturePerfCollector[perfDataClrExceptions]{rows: []perfDataClrExceptions{ + {Name: "powershell", NumberofExcepsThrown: 1}, + {Name: "powershell", Occurrence: 1, NumberofExcepsThrown: 2}, + {Name: "powershell#1", NumberofExcepsThrown: 3}, + {Name: "powershell", Occurrence: 2, NumberofExcepsThrown: 4}, + }} + + registry := prometheus.NewRegistry() + registry.MustRegister(fixturePromCollector{collect: func(ch chan<- prometheus.Metric) { + require.NoError(t, c.collectClrExceptions(ch, nil, time.Second)) + }}) + + families, err := registry.Gather() + require.NoError(t, err) + + got := map[string]float64{} + + for _, family := range families { + if family.GetName() != "windows_netframework_clrexceptions_exceptions_thrown_total" { + continue + } + + for _, metric := range family.GetMetric() { + got[metric.GetLabel()[0].GetValue()] = metric.GetCounter().GetValue() + } + } + + // "powershell#1" is the name of a process, so the suffixes skip it. + require.Equal(t, map[string]float64{"powershell": 1, "powershell#2": 2, "powershell#1": 3, "powershell#3": 4}, got) +} + func TestMissingObjectErrorClassification(t *testing.T) { missing := pdh.NewPdhError(pdh.CstatusNoObject) unrelated := errors.New("access denied") diff --git a/internal/collector/performancecounter/performancecounter.go b/internal/collector/performancecounter/performancecounter.go index 48e0d61b..d5b65443 100644 --- a/internal/collector/performancecounter/performancecounter.go +++ b/internal/collector/performancecounter/performancecounter.go @@ -284,6 +284,13 @@ func (c *Collector) collectObject(ch chan<- prometheus.Metric, perfDataObject Ob return fmt.Errorf("failed to collect data: %w", err) } + // Some objects, e.g. Process, report the same instance name more than + // once. Each occurrence is published with a "#N" suffix, see the docs. + pdh.SuffixDuplicates(rows, + func(row *pdh.Row) *string { return &row.Name }, + func(row *pdh.Row) int { return row.Occurrence }, + ) + metricTypes := counterMetricTypes(perfDataObject) var errs []error diff --git a/internal/collector/performancecounter/performancecounter_test_test.go b/internal/collector/performancecounter/performancecounter_test_test.go index 9d20fe7d..ab0a8e76 100644 --- a/internal/collector/performancecounter/performancecounter_test_test.go +++ b/internal/collector/performancecounter/performancecounter_test_test.go @@ -344,6 +344,46 @@ func TestBuildInvalidConfig(t *testing.T) { } } +// TestCollectorDuplicateInstances checks that the svchost processes, which the +// Process object reports with the same name, are published with suffixes. +func TestCollectorDuplicateInstances(t *testing.T) { + t.Parallel() + + c := performancecounter.New(&performancecounter.Config{ + Objects: []performancecounter.Object{{ + Name: "process", Object: "Process", Instances: []string{"*"}, + Counters: []performancecounter.Counter{{Name: "ID Process", Type: "gauge"}}, + }}, + }) + require.NoError(t, c.Build(slog.New(slog.DiscardHandler), nil)) + t.Cleanup(func() { require.NoError(t, c.Close()) }) + + registry := prometheus.NewRegistry() + registry.MustRegister(collectorAdapter{*c}) + + families, err := registry.Gather() + require.NoError(t, err, "every series is unique") + + instances := map[string]bool{} + + for _, family := range families { + if family.GetName() != "windows_performancecounter_process_id_process" { + continue + } + + for _, metric := range family.GetMetric() { + for _, label := range metric.GetLabel() { + if label.GetName() == "instance" { + instances[label.GetValue()] = true + } + } + } + } + + require.True(t, instances["svchost"], "svchost") + require.True(t, instances["svchost#1"], "svchost#1") +} + func TestCollectorClose(t *testing.T) { t.Parallel() diff --git a/internal/pdh/collector.go b/internal/pdh/collector.go index 7d40a81e..ea9c56a6 100644 --- a/internal/pdh/collector.go +++ b/internal/pdh/collector.go @@ -69,20 +69,25 @@ type Collector[T any] struct { // Row holds the values of one instance collected by a collector from [NewDynamicCollector]. type Row struct { + // Name is the instance name as reported by Windows. Name string + // Occurrence counts earlier instances with the same Name in the sample, see [NewCollector]. + Occurrence int // Values holds one value per counter, in the order the counters were passed to [NewDynamicCollector]. // A value is NaN if the counter has no valid value for the instance in this sample. Values []float64 } -// rowAccessor initializes rows of type T and sets their names and counter values. +// rowAccessor initializes rows of type T and sets their counter values. // field is the index the counterField of the counter was created with. type rowAccessor[T any] struct { - // resetRow initializes row for instance. row can hold the values of a - // previous sample; resetRow can reuse its memory. - resetRow func(row *T, instance string) - setName func(row *T, instance string) - setValue func(row *T, field int, value float64) + // resetRow initializes row for the occurrence of instance. row can hold + // the values of a previous sample; resetRow can reuse its memory. + resetRow func(row *T, instance string, occurrence int) + // keepsOccurrences is true if rows record their occurrence. Otherwise, only + // the first instance of each name is collected. + keepsOccurrences bool + setValue func(row *T, field int, value float64) } // counterField maps a counter to the field index passed to rowAccessor.setValue. @@ -112,9 +117,18 @@ 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. Occurrence -// suffixes distinguish duplicate names without colliding with literal names. -// These suffixes are not stable identities across samples. +// The optional field Name (string) of T receives the instance name exactly as +// Windows reports it. Some objects report the same name for several instances +// in one sample, e.g. Hyper-V VMs with the same name. The optional field +// Occurrence (int) of T receives the number of earlier instances with the same +// name in the sample: 0 for the first, 1 for the second, and so on. It follows +// the order of the counter arrays, so it is not a stable identity across +// samples. A suffix such as "#2" in Name was reported by Windows, never added +// by this package. If T has no Occurrence field, only the first instance of +// each name that has valid data is collected, so that the rows have unique +// names. Dropping instances is logged once per collector as a warning; a row +// type for an object that can report a name more than once must declare +// Occurrence. // // 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. @@ -167,6 +181,14 @@ func structFields[T any]() ([]counterField, rowAccessor[T], []error, error) { } } + occurrenceOffset := -1 + + if f, ok := valueType.FieldByName("Occurrence"); ok && f.Type.Kind() == reflect.Int { + if offset, ok := fieldOffset(valueType, f.Index); ok { + occurrenceOffset = offset + } + } + var errs []error fields := make([]counterField, 0, valueType.NumField()) @@ -217,7 +239,7 @@ func structFields[T any]() ([]counterField, rowAccessor[T], []error, error) { return nil, rowAccessor[T]{}, nil, err } - return fields, structRowAccessor[T](nameOffset), errs, nil + return fields, structRowAccessor[T](nameOffset, occurrenceOffset), errs, nil } // validateFloatOffsets checks that the index of every field is the byte @@ -267,25 +289,26 @@ func fieldOffset(t reflect.Type, index []int) (int, bool) { } // structRowAccessor returns the accessor of the struct T. nameOffset is the -// byte offset of its string field Name, or -1. The offsets passed to setValue +// byte offset of its string field Name, occurrenceOffset that of its int field +// Occurrence, each -1 if T has no such field. The offsets passed to setValue // come from [structFields] and address float64 fields of T, so values are // written without reflection. -func structRowAccessor[T any](nameOffset int) rowAccessor[T] { - setName := func(row *T, instance string) { - if nameOffset >= 0 { - *(*string)(unsafe.Add(unsafe.Pointer(row), nameOffset)) = instance - } - } - +func structRowAccessor[T any](nameOffset, occurrenceOffset int) rowAccessor[T] { return rowAccessor[T]{ - resetRow: func(row *T, instance string) { + resetRow: func(row *T, instance string, occurrence int) { var zero T *row = zero - setName(row, instance) + if nameOffset >= 0 { + *(*string)(unsafe.Add(unsafe.Pointer(row), nameOffset)) = instance + } + + if occurrenceOffset >= 0 { + *(*int)(unsafe.Add(unsafe.Pointer(row), occurrenceOffset)) = occurrence + } }, - setName: setName, + keepsOccurrences: occurrenceOffset >= 0, setValue: func(row *T, offset int, value float64) { *(*float64)(unsafe.Add(unsafe.Pointer(row), offset)) = value }, @@ -309,8 +332,9 @@ func NewDynamicCollector(logger *slog.Logger, resultType CounterType, object str // keeps its Values array if it is large enough. func dynamicRowAccessor(n int) rowAccessor[Row] { return rowAccessor[Row]{ - resetRow: func(row *Row, instance string) { + resetRow: func(row *Row, instance string, occurrence int) { row.Name = instance + row.Occurrence = occurrence if cap(row.Values) < n { row.Values = make([]float64, n) @@ -321,9 +345,7 @@ func dynamicRowAccessor(n int) rowAccessor[Row] { row.Values = row.Values[:n] clear(row.Values) }, - setName: func(row *Row, instance string) { - row.Name = instance - }, + keepsOccurrences: true, setValue: func(row *Row, field int, value float64) { row.Values[field] = value }, @@ -613,11 +635,17 @@ type collectState struct { names instanceNameCache - // index, occurrences and seen back the maps of rowSet. They are cleared - // for every sample, so their buckets are not allocated again. - index map[instanceKey]int - occurrences map[string]int - seen map[string]int + // index and seen back the maps of rowSet. They are cleared for every + // sample, so their buckets are not allocated again. + index map[instanceKey]int + seen map[string]int + + // keys backs rowSet.keys. + keys []instanceKey + + // duplicatesLogged is true once a sample dropped duplicate instances of a + // row type without an Occurrence field. + duplicatesLogged bool } // collect replaces the content of dst with one sample. @@ -675,6 +703,8 @@ func (c *Collector[T]) collect(dst *[]T, state *collectState) (err error) { rows.finish() + state.keys = rows.keys + state.valid = rows.valid if len(*dst) == 0 { @@ -810,12 +840,16 @@ type rowSet[T any] struct { dst *[]T index map[instanceKey]int nameCache *instanceNameCache - // 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 + // keys has the instance key of each row. + keys []instanceKey + // duplicates is true if a row has an occurrence above 0. + duplicates bool + // duplicatesLogged points to collectState.duplicatesLogged. + duplicatesLogged *bool } func newRowSet[T any](c *Collector[T], dst *[]T, state *collectState) rowSet[T] { @@ -823,23 +857,23 @@ func newRowSet[T any](c *Collector[T], dst *[]T, state *collectState) rowSet[T] if state.index == nil { state.index = map[instanceKey]int{} - state.occurrences = map[string]int{} state.seen = map[string]int{} } else { // A failed sample can leave entries behind. clear(state.index) - clear(state.occurrences) clear(state.seen) } return rowSet[T]{ - c: c, - dst: dst, - index: state.index, - nameCache: &state.names, - occurrences: state.occurrences, - seen: state.seen, - valid: state.valid[:0], + c: c, + dst: dst, + index: state.index, + nameCache: &state.names, + seen: state.seen, + valid: state.valid[:0], + keys: state.keys[:0], + + duplicatesLogged: &state.duplicatesLogged, } } @@ -855,7 +889,6 @@ func (r *rowSet[T]) row(counter *Counter, szName *uint16, status uint32) (int, b // 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 { // Check the level first, so the attributes are not built for every @@ -893,8 +926,10 @@ func (r *rowSet[T]) row(counter *Counter, szName *uint16, status uint32) (int, b rows = append(rows, zero) } - r.c.rows.resetRow(&rows[index], instanceName) + r.c.rows.resetRow(&rows[index], instanceName, occurrence) *r.dst = rows + r.keys = append(r.keys, key) + r.duplicates = r.duplicates || occurrence > 0 n := len(r.c.counters) r.valid = slices.Grow(r.valid, n)[:len(r.valid)+n] @@ -943,37 +978,11 @@ 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 = r.nameCache.suffixed(name, 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 @@ -1007,6 +1016,7 @@ func (r *rowSet[T]) finish() { if row != kept { rows[kept] = rows[row] + r.keys[kept] = r.keys[row] // Do not leave two rows that share a Row.Values array. var zero T @@ -1022,10 +1032,55 @@ func (r *rowSet[T]) finish() { slog.String("object", r.c.object), slog.Int("instances", dropped), ) - - clear(rows[kept:]) - *r.dst = rows[:kept] } + + r.keys = r.keys[:kept] + + if r.duplicates && !r.c.rows.keepsOccurrences { + kept = r.keepFirstOccurrences(rows[:kept]) + } + + clear(rows[kept:]) + *r.dst = rows[:kept] +} + +// keepFirstOccurrences keeps, for row types without an Occurrence field, the +// first instance of each name among the rows that remain after finish, so +// that an instance without valid data does not hide a later one with the +// same name. It returns the number of rows kept at the start of rows. +func (r *rowSet[T]) keepFirstOccurrences(rows []T) int { + first := make(map[string]int, len(rows)) + + for i, key := range r.keys { + if j, ok := first[key.name]; !ok || key.occurrence < r.keys[j].occurrence { + first[key.name] = i + } + } + + kept := 0 + + for i, key := range r.keys { + if first[key.name] != i { + continue + } + + rows[kept] = rows[i] + r.keys[kept] = key + kept++ + } + + if dropped := len(rows) - kept; dropped > 0 && !*r.duplicatesLogged { + *r.duplicatesLogged = true + + r.c.logger.Warn("Windows reported instance names more than once, only the first instance of each name is collected", + slog.String("object", r.c.object), + slog.Int("instances", dropped), + ) + } + + r.keys = r.keys[:kept] + + return kept } // panicError logs a panic recovered while collecting and returns it as an error. @@ -1098,11 +1153,8 @@ func formatCounterPath(object, instance, counterName string) string { // Names that a sample did not decode are removed at its end, because instance names that // contain process IDs, like those of GPU Engine, change all the time. type instanceNameCache struct { - names map[string]*cachedInstanceName - // suffixedNames maps a name and an occurrence suffix to the name with - // the suffix, e.g. svchost and 3 to svchost#3. - suffixedNames map[instanceKey]*cachedInstanceName - sample uint64 + names map[string]*cachedInstanceName + sample uint64 } type cachedInstanceName struct { @@ -1146,41 +1198,13 @@ func (cache *instanceNameCache) decode(p *uint16) string { return name } -// suffixed returns name with the occurrence suffix #suffix. Duplicate names, -// like those of the many svchost processes, need the same suffixed names in -// every sample. -func (cache *instanceNameCache) suffixed(name string, suffix int) string { - key := instanceKey{name: name, occurrence: suffix} - - if cached, ok := cache.suffixedNames[key]; ok { - cached.sample = cache.sample - - return cached.name - } - - if cache.suffixedNames == nil { - cache.suffixedNames = map[instanceKey]*cachedInstanceName{} - } - - suffixedName := name + "#" + strconv.Itoa(suffix) - cache.suffixedNames[key] = &cachedInstanceName{name: suffixedName, sample: cache.sample} - - return suffixedName -} - -// removeUnseen removes the names that the current sample did not use. +// removeUnseen removes the names that the current sample did not decode. func (cache *instanceNameCache) removeUnseen() { for raw, cached := range cache.names { if cached.sample != cache.sample { delete(cache.names, raw) } } - - for key, cached := range cache.suffixedNames { - if cached.sample != cache.sample { - delete(cache.suffixedNames, key) - } - } } // isKnownCounterDataStatus reports whether status means that a counter has no diff --git a/internal/pdh/collector_internal_test.go b/internal/pdh/collector_internal_test.go index 7c5d1ab2..8c20f08e 100644 --- a/internal/pdh/collector_internal_test.go +++ b/internal/pdh/collector_internal_test.go @@ -24,6 +24,7 @@ import ( "reflect" "runtime" "slices" + "strings" "testing" "unsafe" @@ -38,6 +39,8 @@ type rowSetValues struct { B float64 `perfdata:"B"` BSecond float64 `perfdata:"B,secondvalue"` Optional float64 `perfdata:"Optional"` + // Occurrence is last, so that the field indices of the counters stay the same. + Occurrence int } // newRowSetTestCollector returns a collector with the counters A and B, which @@ -48,7 +51,7 @@ func newRowSetTestCollector(partialRows bool) *Collector[rowSetValues] { return &Collector[rowSetValues]{ partialRows: partialRows, logger: slog.New(slog.DiscardHandler), - rows: structRowAccessor[rowSetValues](int(unsafe.Offsetof(row.Name))), + rows: structRowAccessor[rowSetValues](int(unsafe.Offsetof(row.Name)), int(unsafe.Offsetof(row.Occurrence))), counters: []Counter{ {Name: "A", Instances: map[string]pdhCounterHandle{"*": 1}, FieldIndexValue: int(unsafe.Offsetof(row.A)), FieldIndexSecondValue: -1}, {Name: "B", Instances: map[string]pdhCounterHandle{"*": 2}, FieldIndexValue: int(unsafe.Offsetof(row.B)), FieldIndexSecondValue: int(unsafe.Offsetof(row.BSecond))}, @@ -121,6 +124,15 @@ func TestRowSetDuplicateInstances(t *testing.T) { nan := math.NaN() + // wantRow identifies a row by name and occurrence. suffixed is the name + // that SuffixDuplicates gives the row. + type wantRow struct { + name string + occurrence int + suffixed string + a, b float64 + } + for _, tc := range []struct { name string names []string @@ -128,17 +140,17 @@ func TestRowSetDuplicateInstances(t *testing.T) { a []float64 b []float64 repeatArray bool - want []rowSetValues + want []wantRow }{ { 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}, + want: []wantRow{ + {name: "A", suffixed: "A", a: 1, b: 10}, + {name: "A", occurrence: 1, suffixed: "A#1", a: 2, b: 20}, + {name: "A", occurrence: 2, suffixed: "A#2", a: 3, b: 30}, }, }, { @@ -147,22 +159,23 @@ func TestRowSetDuplicateInstances(t *testing.T) { 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}, + want: []wantRow{ + {name: "A", suffixed: "A", a: 1, b: 10}, + {name: "B", suffixed: "B", a: 2, b: 20}, + {name: "A", occurrence: 1, suffixed: "A#1", a: 3, b: 30}, }, }, { + // A name with a suffix reported by Windows is a different instance. 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}, + want: []wantRow{ + {name: "A", suffixed: "A", a: 1, b: 10}, + {name: "A", occurrence: 1, suffixed: "A#2", a: 2, b: 20}, + {name: "A#1", suffixed: "A#1", a: 3, b: 30}, }, }, { @@ -170,10 +183,10 @@ func TestRowSetDuplicateInstances(t *testing.T) { 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}, + want: []wantRow{ + {name: "A", suffixed: "A", a: 1, b: 10}, + {name: "A", occurrence: 1, suffixed: "A#1", a: nan, b: 20}, + {name: "A", occurrence: 2, suffixed: "A#2", a: 3, b: 30}, }, }, { @@ -181,20 +194,21 @@ func TestRowSetDuplicateInstances(t *testing.T) { 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}, + want: []wantRow{ + {name: "A", suffixed: "A", a: 1, b: nan}, + {name: "A", occurrence: 1, suffixed: "A#1", a: 2, b: 20}, + {name: "A", occurrence: 2, suffixed: "A#2", a: 3, b: 30}, }, }, { + // The invalid "A#1" has no row, so its suffix is free. 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}, + want: []wantRow{ + {name: "A", suffixed: "A", a: 1, b: 10}, + {name: "A", occurrence: 1, suffixed: "A#1", a: 2, b: 20}, }, }, { @@ -203,77 +217,92 @@ func TestRowSetDuplicateInstances(t *testing.T) { 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}, + want: []wantRow{ + {name: "A", suffixed: "A", a: 1, b: 10}, + {name: "A", occurrence: 1, suffixed: "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() + for _, keepsOccurrences := range []bool{true, false} { + t.Run(fmt.Sprintf("%s/%s/partial=%t/occurrences=%t", tc.name, resultType, partialRows, keepsOccurrences), func(t *testing.T) { + t.Parallel() - c := newRowSetTestCollector(partialRows) - c.counters[1].FieldIndexSecondValue = -1 + c := newRowSetTestCollector(partialRows) + c.counters[1].FieldIndexSecondValue = -1 + c.rows.keepsOccurrences = keepsOccurrences - var dst []rowSetValues + var dst []rowSetValues - rows := newRowSet(c, &dst, &collectState{}) + rows := newRowSet(c, &dst, &collectState{}) - addDuplicateTestItems( - &rows, - resultType, - 0, - tc.names, - tc.a, - ) + 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) + 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 wantRow) bool { + return !partialRows && (math.IsNaN(row.a) || math.IsNaN(row.b)) + }) + + if !keepsOccurrences { + // Without an Occurrence field, only the first remaining instance of a name is collected. + seen := map[string]bool{} + want = slices.DeleteFunc(want, func(row wantRow) bool { + duplicate := seen[row.name] + seen[row.name] = true + + return duplicate + }) + } + + require.Len(t, dst, len(want)) + + for _, expected := range want { + i := slices.IndexFunc(dst, func(row rowSetValues) bool { + return row.Name == expected.name && row.Occurrence == expected.occurrence + }) + require.NotEqual(t, -1, i, "missing instance %s occurrence %d", expected.name, expected.occurrence) + + 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) + } + } + + if !keepsOccurrences { + return + } + + // SuffixDuplicates gives every row a unique name. + SuffixDuplicates(dst, + func(row *rowSetValues) *string { return &row.Name }, + func(row *rowSetValues) int { return row.Occurrence }, + ) + + for _, expected := range want { + i := slices.IndexFunc(dst, func(row rowSetValues) bool { return row.Name == expected.suffixed }) + require.NotEqual(t, -1, i, "missing suffixed instance %s", expected.suffixed) + require.Equal(t, expected.occurrence, dst[i].Occurrence, expected.suffixed) + } + }) + } } } } @@ -563,6 +592,35 @@ func TestInstanceNameCache(t *testing.T) { // TestRowSetReusesState checks that a sample does not see the instances of an // earlier sample that shared its state, even if that sample did not finish. +// TestRowSetWarnsOnceAboutDroppedDuplicates checks that dropping duplicate +// instances of a row type without an Occurrence field is logged once. +func TestRowSetWarnsOnceAboutDroppedDuplicates(t *testing.T) { + t.Parallel() + + var logs strings.Builder + + c := newRowSetTestCollector(true) + c.logger = slog.New(slog.NewTextHandler(&logs, &slog.HandlerOptions{Level: slog.LevelWarn})) + c.rows.keepsOccurrences = false + state := &collectState{} + + for range 3 { + var dst []rowSetValues + + rows := newRowSet(c, &dst, state) + addDuplicateTestItems(&rows, CounterTypeRaw, 0, []string{"a", "a", "b"}, []float64{1, 2, 3}) + addDuplicateTestItems(&rows, CounterTypeRaw, 1, []string{"a", "a", "b"}, []float64{4, 5, 6}) + rows.finish() + + state.keys = rows.keys + + require.Len(t, dst, 2) + require.InDelta(t, 1, dst[0].A, 0, "the first instance of a is kept") + } + + require.Equal(t, 1, strings.Count(logs.String(), "level=WARN"), logs.String()) +} + func TestRowSetReusesState(t *testing.T) { t.Parallel() diff --git a/internal/pdh/collector_rows_internal_test.go b/internal/pdh/collector_rows_internal_test.go index db187db7..2c51e7e3 100644 --- a/internal/pdh/collector_rows_internal_test.go +++ b/internal/pdh/collector_rows_internal_test.go @@ -69,16 +69,15 @@ func TestStructFields(t *testing.T) { var row structFieldsValues row.Invalid = 42 - rows.resetRow(&row, "instance") + rows.resetRow(&row, "instance", 0) require.Equal(t, structFieldsValues{Name: "instance"}, row) rows.setValue(&row, offsets["Inner"], 1) rows.setValue(&row, offsets["Outer"], 2) rows.setValue(&row, offsets["Outer,secondvalue"], 3) - rows.setName(&row, "renamed") require.Equal(t, structFieldsValues{ - Name: "renamed", + Name: "instance", Inner: 1, Outer: 2, Base: 3, @@ -172,9 +171,9 @@ func TestStructRowAccessorWithoutName(t *testing.T) { require.Len(t, fields, 1) row := values{Value: 1} - rows.resetRow(&row, "instance") - rows.setName(&row, "instance") + rows.resetRow(&row, "instance", 1) require.Equal(t, values{}, row) + require.False(t, rows.keepsOccurrences) rows.setValue(&row, fields[0].index, 2) require.Equal(t, values{Value: 2}, row) @@ -188,20 +187,44 @@ func TestDynamicRowAccessor(t *testing.T) { rows := dynamicRowAccessor(2) values := []float64{1, 2, 3} - row := Row{Name: "old", Values: values} + row := Row{Name: "old", Occurrence: 3, Values: values} - rows.resetRow(&row, "new") - require.Equal(t, Row{Name: "new", Values: []float64{0, 0}}, row) + rows.resetRow(&row, "new", 1) + require.Equal(t, Row{Name: "new", Occurrence: 1, Values: []float64{0, 0}}, row) require.Same(t, unsafe.SliceData(values), unsafe.SliceData(row.Values), "Values allocated again") rows.setValue(&row, 1, 4) require.Equal(t, []float64{0, 4}, row.Values) - row = Row{Name: "old", Values: []float64{1}} - rows.resetRow(&row, "new") + row = Row{Name: "old", Occurrence: 2, Values: []float64{1}} + rows.resetRow(&row, "new", 0) require.Equal(t, Row{Name: "new", Values: []float64{0, 0}}, row) } +// TestStructRowAccessorOccurrence checks that a reused struct row gets the +// occurrence of its new instance, not that of the previous sample. +func TestStructRowAccessorOccurrence(t *testing.T) { + t.Parallel() + + type values struct { + Name string + Value float64 `perfdata:"Value"` + Occurrence int + } + + _, rows, errs, err := structFields[values]() + require.NoError(t, err) + require.Empty(t, errs) + require.True(t, rows.keepsOccurrences) + + row := values{Name: "old", Value: 1, Occurrence: 3} + rows.resetRow(&row, "new", 1) + require.Equal(t, values{Name: "new", Occurrence: 1}, row) + + rows.resetRow(&row, "first", 0) + require.Equal(t, values{Name: "first"}, row) +} + // TestRowSetReusesRows checks that a sample reuses the backing array of dst // and that no value of an earlier sample remains in a reused row. func TestRowSetReusesRows(t *testing.T) { @@ -260,42 +283,20 @@ func TestRowSetSampleDoesNotAllocate(t *testing.T) { rows.finish() state.names.removeUnseen() + state.keys = rows.keys state.valid = rows.valid } sample() require.Len(t, dst, instances) - require.Equal(t, "instance_A#3", dst[75].Name) + require.Equal(t, "instance_A", dst[75].Name) + require.Equal(t, 3, dst[75].Occurrence) require.Zero(t, testing.AllocsPerRun(10, sample)) runtime.KeepAlive(items) } -// TestInstanceNameCacheSuffixed checks that suffixed names are built once -// while their instances exist. -func TestInstanceNameCacheSuffixed(t *testing.T) { - t.Parallel() - - var cache instanceNameCache - - cache.startSample() - - first := cache.suffixed("svchost", 2) - require.Equal(t, "svchost#2", first) - require.Equal(t, "svchost#3", cache.suffixed("svchost", 3)) - cache.removeUnseen() - - cache.startSample() - - second := cache.suffixed("svchost", 2) - require.Same(t, unsafe.StringData(first), unsafe.StringData(second), "name built again") - cache.removeUnseen() - - require.Len(t, cache.suffixedNames, 1, "name of a vanished instance kept") - require.Contains(t, cache.suffixedNames, instanceKey{name: "svchost", occurrence: 2}) -} - // TestCollectReusesRows checks that Collect keeps the backing array of dst and // releases the rows a sample does not use. func TestCollectReusesRows(t *testing.T) { diff --git a/internal/pdh/collector_rowset_bench_test.go b/internal/pdh/collector_rowset_bench_test.go index 5a1db4b7..c761746a 100644 --- a/internal/pdh/collector_rowset_bench_test.go +++ b/internal/pdh/collector_rowset_bench_test.go @@ -60,6 +60,7 @@ func BenchmarkRowSetInstanceNames(b *testing.B) { rows.finish() state.names.removeUnseen() + state.keys = rows.keys state.valid = rows.valid } @@ -171,6 +172,7 @@ func BenchmarkRowSetProcessShape(b *testing.B) { rows.finish() state.names.removeUnseen() + state.keys = rows.keys state.valid = rows.valid } diff --git a/internal/pdh/collector_test.go b/internal/pdh/collector_test.go index 367fd115..7f6d6b53 100644 --- a/internal/pdh/collector_test.go +++ b/internal/pdh/collector_test.go @@ -74,6 +74,70 @@ func TestCollector(t *testing.T) { } } +type processOccurrence struct { + Name string + Occurrence int + ProcessID float64 `perfdata:"ID Process"` +} + +// TestCollectorOccurrences checks the duplicate names of the Process object, +// which reports every svchost process as "svchost". +func TestCollectorOccurrences(t *testing.T) { + t.Parallel() + + logger := slog.New(slog.DiscardHandler) + + withOccurrence, err := pdh.NewCollector[processOccurrence](logger, pdh.CounterTypeRaw, "Process", pdh.InstancesAll) + require.NoError(t, err) + + defer withOccurrence.Close() + + withoutOccurrence, err := pdh.NewCollector[process](logger, pdh.CounterTypeRaw, "Process", pdh.InstancesAll) + require.NoError(t, err) + + defer withoutOccurrence.Close() + + var rows []processOccurrence + + require.NoError(t, withOccurrence.Collect(&rows)) + + occurrences := map[string][]int{} + for _, row := range rows { + occurrences[row.Name] = append(occurrences[row.Name], row.Occurrence) + } + + require.Greater(t, len(occurrences["svchost"]), 1, "svchost is reported more than once") + + for name, seen := range occurrences { + require.NotContains(t, name, "#", "the collector adds no suffix") + require.ElementsMatch(t, seen, seq(len(seen)), "occurrences of %s", name) + } + + var unique []process + + require.NoError(t, withoutOccurrence.Collect(&unique)) + + names := map[string]int{} + for _, row := range unique { + names[row.Name]++ + } + + require.Equal(t, 1, names["svchost"], "without an Occurrence field only the first instance of a name is collected") + + for name, count := range names { + require.Equal(t, 1, count, name) + } +} + +func seq(n int) []int { + s := make([]int, n) + for i := range s { + s[i] = i + } + + return s +} + func TestCollectorConcurrentCollect(t *testing.T) { t.Parallel() diff --git a/internal/pdh/occurrence.go b/internal/pdh/occurrence.go new file mode 100644 index 00000000..075a372f --- /dev/null +++ b/internal/pdh/occurrence.go @@ -0,0 +1,69 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Copyright The Prometheus Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build windows + +package pdh + +import "strconv" + +// SuffixDuplicates makes the names of rows unique within a sample for +// collectors that publish every occurrence of a duplicate instance name. +// +// name returns a pointer to the name of a row, occurrence its Occurrence (see +// [NewCollector]). The first occurrence keeps its name. Later occurrences get +// the suffix "#N", where N counts from 1 and skips every N for which +// "#N" is the name of another row, for example a suffix that Windows +// added itself. Only the names of rows reserve suffixes: an instance that +// Windows reported without valid data has no row and reserves nothing. N +// depends on the order of the sample and is not a stable identity. +// +// Collectors can map names, e.g. an empty name to a placeholder, before they +// call SuffixDuplicates, so that the suffix is added to the published name. +func SuffixDuplicates[T any](rows []T, name func(*T) *string, occurrence func(*T) int) { + var names map[string]struct{} + + for i := range rows { + if occurrence(&rows[i]) == 0 { + continue + } + + if names == nil { + names = make(map[string]struct{}, len(rows)) + + for j := range rows { + names[*name(&rows[j])] = struct{}{} + } + } + + instanceName := name(&rows[i]) + base := *instanceName + remaining := occurrence(&rows[i]) + + for suffix := 1; ; suffix++ { + candidate := base + "#" + strconv.Itoa(suffix) + if _, literal := names[candidate]; literal { + continue + } + + remaining-- + if remaining == 0 { + *instanceName = candidate + + break + } + } + } +}