physical_disk: refactor collector (#1734)

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

internal/collector/logical_disk/const.go

@@ -16,7 +16,7 @@ const (
 	percentDiskWriteTime    = "% Disk Write Time"
 	percentFreeSpace        = "% Free Space"
 	percentIdleTime         = "% Idle Time"
-	SplitIOPerSec           = "Split IO/Sec"
+	splitIOPerSec           = "Split IO/Sec"
 )
 
 // Win32_PerfRawData_PerfDisk_LogicalDisk docs:

internal/collector/logical_disk/logical_disk.go

@@ -4,7 +4,6 @@ package logical_disk
 
 import (
 	"encoding/binary"
-	"errors"
 	"fmt"
 	"log/slog"
 	"regexp"
@@ -156,7 +155,7 @@ func (c *Collector) Build(_ *slog.Logger, _ *mi.Session) error {
 			percentFreeSpace,
 			freeSpace,
 			percentIdleTime,
-			SplitIOPerSec,
+			splitIOPerSec,
 			avgDiskSecPerRead,
 			avgDiskSecPerWrite,
 			avgDiskSecPerTransfer,
@@ -329,10 +328,6 @@ func (c *Collector) collectPDH(logger *slog.Logger, ch chan<- prometheus.Metric)
 		return fmt.Errorf("failed to collect LogicalDisk metrics: %w", err)
 	}
 
-	if len(perfData) == 0 {
-		return errors.New("perflib query for LogicalDisk returned empty result set")
-	}
-
 	for name, volume := range perfData {
 		if name == "_Total" ||
 			c.config.VolumeExclude.MatchString(name) ||
@@ -453,7 +448,7 @@ func (c *Collector) collectPDH(logger *slog.Logger, ch chan<- prometheus.Metric)
 		ch <- prometheus.MustNewConstMetric(
 			c.splitIOs,
 			prometheus.CounterValue,
-			volume[SplitIOPerSec].FirstValue,
+			volume[splitIOPerSec].FirstValue,
 			name,
 		)
 

internal/collector/physical_disk/const.go

@@ -0,0 +1,16 @@
+package physical_disk
+
+const (
+	CurrentDiskQueueLength = "Current Disk Queue Length"
+	DiskReadBytesPerSec    = "Disk Read Bytes/sec"
+	DiskReadsPerSec        = "Disk Reads/sec"
+	DiskWriteBytesPerSec   = "Disk Write Bytes/sec"
+	DiskWritesPerSec       = "Disk Writes/sec"
+	PercentDiskReadTime    = "% Disk Read Time"
+	PercentDiskWriteTime   = "% Disk Write Time"
+	PercentIdleTime        = "% Idle Time"
+	SplitIOPerSec          = "Split IO/Sec"
+	AvgDiskSecPerRead      = "Avg. Disk sec/Read"
+	AvgDiskSecPerWrite     = "Avg. Disk sec/Write"
+	AvgDiskSecPerTransfer  = "Avg. Disk sec/Transfer"
+)

internal/collector/physical_disk/physical_disk.go

@@ -10,8 +10,8 @@ import (
 
 	"github.com/alecthomas/kingpin/v2"
 	"github.com/prometheus-community/windows_exporter/internal/mi"
+	"github.com/prometheus-community/windows_exporter/internal/perfdata"
 	"github.com/prometheus-community/windows_exporter/internal/perfdata/perftypes"
-	v1 "github.com/prometheus-community/windows_exporter/internal/perfdata/v1"
 	"github.com/prometheus-community/windows_exporter/internal/types"
 	"github.com/prometheus/client_golang/prometheus"
 )
@@ -32,6 +32,8 @@ var ConfigDefaults = Config{
 type Collector struct {
 	config Config
 
+	perfDataCollector perfdata.Collector
+
 	idleTime         *prometheus.Desc
 	readBytesTotal   *prometheus.Desc
 	readLatency      *prometheus.Desc
@@ -107,7 +109,7 @@ func (c *Collector) GetName() string {
 }
 
 func (c *Collector) GetPerfCounter(_ *slog.Logger) ([]string, error) {
-	return []string{"PhysicalDisk"}, nil
+	return []string{}, nil
 }
 
 func (c *Collector) Close(_ *slog.Logger) error {
@@ -115,6 +117,28 @@ func (c *Collector) Close(_ *slog.Logger) error {
 }
 
 func (c *Collector) Build(_ *slog.Logger, _ *mi.Session) error {
+	counters := []string{
+		CurrentDiskQueueLength,
+		DiskReadBytesPerSec,
+		DiskReadsPerSec,
+		DiskWriteBytesPerSec,
+		DiskWritesPerSec,
+		PercentDiskReadTime,
+		PercentDiskWriteTime,
+		PercentIdleTime,
+		SplitIOPerSec,
+		AvgDiskSecPerRead,
+		AvgDiskSecPerWrite,
+		AvgDiskSecPerTransfer,
+	}
+
+	var err error
+
+	c.perfDataCollector, err = perfdata.NewCollector(perfdata.V2, "PhysicalDisk", perfdata.AllInstances, counters)
+	if err != nil {
+		return fmt.Errorf("failed to create PhysicalDisk collector: %w", err)
+	}
+
 	c.requestsQueued = prometheus.NewDesc(
 		prometheus.BuildFQName(types.Namespace, Name, "requests_queued"),
 		"The number of requests queued to the disk (PhysicalDisk.CurrentDiskQueueLength)",
@@ -204,139 +228,111 @@ func (c *Collector) Build(_ *slog.Logger, _ *mi.Session) error {
 
 // Collect sends the metric values for each metric
 // to the provided prometheus Metric channel.
-func (c *Collector) Collect(ctx *types.ScrapeContext, logger *slog.Logger, ch chan<- prometheus.Metric) error {
+func (c *Collector) Collect(_ *types.ScrapeContext, _ *slog.Logger, ch chan<- prometheus.Metric) error {
-	logger = logger.With(slog.String("collector", Name))
+	if err := c.collect(ch); err != nil {
-	if err := c.collect(ctx, logger, ch); err != nil {
+		return fmt.Errorf("failed collecting physical_disk metrics: %w", err)
-		logger.Error("failed collecting physical_disk metrics",
-			slog.Any("err", err),
-		)
-
-		return err
 	}
 
 	return nil
 }
 
-// PhysicalDisk
+func (c *Collector) collect(ch chan<- prometheus.Metric) error {
-// Win32_PerfRawData_PerfDisk_PhysicalDisk docs:
+	perfData, err := c.perfDataCollector.Collect()
-// - https://docs.microsoft.com/en-us/previous-versions/aa394308(v=vs.85) - Win32_PerfRawData_PerfDisk_PhysicalDisk class.
+	if err != nil {
-type PhysicalDisk struct {
+		return fmt.Errorf("failed to collect PhysicalDisk metrics: %w", err)
-	Name                   string
-	CurrentDiskQueueLength float64 `perflib:"Current Disk Queue Length"`
-	DiskReadBytesPerSec    float64 `perflib:"Disk Read Bytes/sec"`
-	DiskReadsPerSec        float64 `perflib:"Disk Reads/sec"`
-	DiskWriteBytesPerSec   float64 `perflib:"Disk Write Bytes/sec"`
-	DiskWritesPerSec       float64 `perflib:"Disk Writes/sec"`
-	PercentDiskReadTime    float64 `perflib:"% Disk Read Time"`
-	PercentDiskWriteTime   float64 `perflib:"% Disk Write Time"`
-	PercentIdleTime        float64 `perflib:"% Idle Time"`
-	SplitIOPerSec          float64 `perflib:"Split IO/Sec"`
-	AvgDiskSecPerRead      float64 `perflib:"Avg. Disk sec/Read"`
-	AvgDiskSecPerWrite     float64 `perflib:"Avg. Disk sec/Write"`
-	AvgDiskSecPerTransfer  float64 `perflib:"Avg. Disk sec/Transfer"`
-}
-
-func (c *Collector) collect(ctx *types.ScrapeContext, logger *slog.Logger, ch chan<- prometheus.Metric) error {
-	logger = logger.With(slog.String("collector", Name))
-
-	var dst []PhysicalDisk
-
-	if err := v1.UnmarshalObject(ctx.PerfObjects["PhysicalDisk"], &dst, logger); err != nil {
-		return err
 	}
 
-	for _, disk := range dst {
+	for name, disk := range perfData {
-		if disk.Name == "_Total" ||
+		if c.config.DiskExclude.MatchString(name) ||
-			c.config.DiskExclude.MatchString(disk.Name) ||
+			!c.config.DiskInclude.MatchString(name) {
-			!c.config.DiskInclude.MatchString(disk.Name) {
 			continue
 		}
 
 		// Parse physical disk number from disk.Name. Mountpoint information is
 		// sometimes included, e.g. "1 C:".
-		disk_number, _, _ := strings.Cut(disk.Name, " ")
+		disk_number, _, _ := strings.Cut(name, " ")
 
 		ch <- prometheus.MustNewConstMetric(
 			c.requestsQueued,
 			prometheus.GaugeValue,
-			disk.CurrentDiskQueueLength,
+			disk[CurrentDiskQueueLength].FirstValue,
 			disk_number,
 		)
 
 		ch <- prometheus.MustNewConstMetric(
 			c.readBytesTotal,
 			prometheus.CounterValue,
-			disk.DiskReadBytesPerSec,
+			disk[DiskReadBytesPerSec].FirstValue,
 			disk_number,
 		)
 
 		ch <- prometheus.MustNewConstMetric(
 			c.readsTotal,
 			prometheus.CounterValue,
-			disk.DiskReadsPerSec,
+			disk[DiskReadsPerSec].FirstValue,
 			disk_number,
 		)
 
 		ch <- prometheus.MustNewConstMetric(
 			c.writeBytesTotal,
 			prometheus.CounterValue,
-			disk.DiskWriteBytesPerSec,
+			disk[DiskWriteBytesPerSec].FirstValue,
 			disk_number,
 		)
 
 		ch <- prometheus.MustNewConstMetric(
 			c.writesTotal,
 			prometheus.CounterValue,
-			disk.DiskWritesPerSec,
+			disk[DiskWritesPerSec].FirstValue,
 			disk_number,
 		)
 
 		ch <- prometheus.MustNewConstMetric(
 			c.readTime,
 			prometheus.CounterValue,
-			disk.PercentDiskReadTime,
+			disk[PercentDiskReadTime].FirstValue,
 			disk_number,
 		)
 
 		ch <- prometheus.MustNewConstMetric(
 			c.writeTime,
 			prometheus.CounterValue,
-			disk.PercentDiskWriteTime,
+			disk[PercentDiskWriteTime].FirstValue,
 			disk_number,
 		)
 
 		ch <- prometheus.MustNewConstMetric(
 			c.idleTime,
 			prometheus.CounterValue,
-			disk.PercentIdleTime,
+			disk[PercentIdleTime].FirstValue,
 			disk_number,
 		)
 
 		ch <- prometheus.MustNewConstMetric(
 			c.splitIOs,
 			prometheus.CounterValue,
-			disk.SplitIOPerSec,
+			disk[SplitIOPerSec].FirstValue,
 			disk_number,
 		)
 
 		ch <- prometheus.MustNewConstMetric(
 			c.readLatency,
 			prometheus.CounterValue,
-			disk.AvgDiskSecPerRead*perftypes.TicksToSecondScaleFactor,
+			disk[AvgDiskSecPerRead].FirstValue*perftypes.TicksToSecondScaleFactor,
 			disk_number,
 		)
 
 		ch <- prometheus.MustNewConstMetric(
 			c.writeLatency,
 			prometheus.CounterValue,
-			disk.AvgDiskSecPerWrite*perftypes.TicksToSecondScaleFactor,
+			disk[AvgDiskSecPerWrite].FirstValue*perftypes.TicksToSecondScaleFactor,
 			disk_number,
 		)
 
 		ch <- prometheus.MustNewConstMetric(
 			c.readWriteLatency,
 			prometheus.CounterValue,
-			disk.AvgDiskSecPerTransfer*perftypes.TicksToSecondScaleFactor,
+			disk[AvgDiskSecPerTransfer].FirstValue*perftypes.TicksToSecondScaleFactor,
 			disk_number,
 		)
 	}

internal/collector/physical_disk/physical_disk_test.go

@@ -5,6 +5,7 @@ import (
 
 	"github.com/prometheus-community/windows_exporter/internal/collector/physical_disk"
 	"github.com/prometheus-community/windows_exporter/internal/testutils"
+	"github.com/prometheus-community/windows_exporter/internal/types"
 )
 
 func BenchmarkCollector(b *testing.B) {
@@ -12,7 +13,7 @@ func BenchmarkCollector(b *testing.B) {
 }
 
 func TestCollector(t *testing.T) {
-	t.Skip()
+	testutils.TestCollector(t, physical_disk.New, &physical_disk.Config{
-
+		DiskInclude: types.RegExpAny,
-	testutils.TestCollector(t, physical_disk.New, nil)
+	})
 }

internal/perfdata/v2/collector.go

@@ -74,12 +74,12 @@ func NewCollector(object string, instances []string, counters []string) (*Collec
 			// Get the info with the current buffer size
 			bufLen := uint32(0)
 
-			if ret := PdhGetCounterInfo(counterHandle, 1, &bufLen, nil); ret != PdhMoreData {
+			if ret := PdhGetCounterInfo(counterHandle, 0, &bufLen, nil); ret != PdhMoreData {
 				return nil, fmt.Errorf("PdhGetCounterInfo: %w", NewPdhError(ret))
 			}
 
 			buf := make([]byte, bufLen)
-			if ret := PdhGetCounterInfo(counterHandle, 1, &bufLen, &buf[0]); ret != ErrorSuccess {
+			if ret := PdhGetCounterInfo(counterHandle, 0, &bufLen, &buf[0]); ret != ErrorSuccess {
 				return nil, fmt.Errorf("PdhGetCounterInfo: %w", NewPdhError(ret))
 			}