Refactored Windows perfmon metricset: replaced method to retrieve counter paths with PdhExpandWildCardPathW, separated code by responsibility, removed unused functions

CHANGELOG.next.asciidoc

@@ -127,6 +127,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
 - Change some field type from scaled_float to long in aws module. {pull}11982[11982]
 - Fixed RabbitMQ `queue` metricset gathering when `consumer_utilisation` is set empty at the metrics source {pull}12089[12089]
 - Fix direction of incoming IPv6 sockets. {pull}12248[12248]
+- Small refactoring of perfom metricset, removal of unused functions, separation of responsibility {pull}12212[12212]
 - Validate that kibana/status metricset cannot be used when xpack is enabled. {pull}12264[12264]
 - Ignore prometheus metrics when their values are NaN or Inf. {pull}12084[12084] {issue}10849[10849]
 

metricbeat/module/windows/perfmon/defs_pdh_windows.go

@@ -232,11 +232,3 @@ const (
 	// Multiply the actual value by 1,000.
 	PdhFmtMultiply1000 PdhCounterFormat = C.PDH_FMT_1000
 )
-
-// PdhCounterValue is the structure that receives the counter value.
-type PdhCounterValue C.PDH_FMT_COUNTERVALUE
-
-// PdhRawCounter is the structure that receives the raw counter.
-type PdhRawCounter C.PDH_RAW_COUNTER
-
-type PdhFileTime C.FILETIME

metricbeat/module/windows/perfmon/mkpdh_defs.go

@@ -94,14 +94,6 @@ const (
 	// Multiply the actual value by 1,000.
 	PdhFmtMultiply1000 PdhCounterFormat = C.PDH_FMT_1000
 )
-
-// PdhCounterValue is the structure that receives the counter value.
-type PdhCounterValue C.PDH_FMT_COUNTERVALUE
-
-// PdhRawCounter is the structure that receives the raw counter.
-type PdhRawCounter C.PDH_RAW_COUNTER
-
-type PdhFileTime C.FILETIME
 `
 
 var (

metricbeat/module/windows/perfmon/pdh_integration_windows_test.go

@@ -20,9 +20,9 @@
 package perfmon
 
 import (
+	"strings"
 	"testing"
 	"time"
-	"unsafe"
 
 	"github.com/elastic/beats/libbeat/common"
 
@@ -105,25 +105,26 @@ func TestCounterWithNoInstanceName(t *testing.T) {
 }
 
 func TestQuery(t *testing.T) {
-	q, err := NewQuery("")
+	var q Query
+	err := q.Open()
 	if err != nil {
 		t.Fatal(err)
 	}
 	defer q.Close()
-
-	err = q.AddCounter(processorTimeCounter, FloatFormat, "")
+	counter := CounterConfig{Format: "float", InstanceName: "TestInstanceName"}
+	err = q.AddCounter(processorTimeCounter, counter, false)
 	if err != nil {
 		t.Fatal(err)
 	}
 
 	for i := 0; i < 2; i++ {
-		err = q.Execute()
+		err = q.CollectData()
 		if err != nil {
 			t.Fatal(err)
 		}
 	}
 
-	values, err := q.Values()
+	values, err := q.GetFormattedCounterValues()
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -136,6 +137,7 @@ func TestQuery(t *testing.T) {
 	}
 
 	assert.NoError(t, value[0].Err)
+	assert.Equal(t, "TestInstanceName", value[0].Instance)
 }
 
 func TestExistingCounter(t *testing.T) {
@@ -146,7 +148,7 @@ func TestExistingCounter(t *testing.T) {
 	config.CounterConfig[0].MeasurementLabel = "processor.time.total.pct"
 	config.CounterConfig[0].Query = processorTimeCounter
 	config.CounterConfig[0].Format = "float"
-	handle, err := NewPerfmonReader(config)
+	handle, err := NewReader(config)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -168,7 +170,7 @@ func TestNonExistingCounter(t *testing.T) {
 	config.CounterConfig[0].MeasurementLabel = "processor.time.total.pct"
 	config.CounterConfig[0].Query = "\\Processor Information(_Total)\\not existing counter"
 	config.CounterConfig[0].Format = "float"
-	handle, err := NewPerfmonReader(config)
+	handle, err := NewReader(config)
 	if assert.Error(t, err) {
 		assert.EqualValues(t, PDH_CSTATUS_NO_COUNTER, errors.Cause(err))
 	}
@@ -188,7 +190,7 @@ func TestIgnoreNonExistentCounter(t *testing.T) {
 	config.CounterConfig[0].MeasurementLabel = "processor.time.total.pct"
 	config.CounterConfig[0].Query = "\\Processor Information(_Total)\\not existing counter"
 	config.CounterConfig[0].Format = "float"
-	handle, err := NewPerfmonReader(config)
+	handle, err := NewReader(config)
 
 	values, err := handle.Read()
 
@@ -212,7 +214,7 @@ func TestNonExistingObject(t *testing.T) {
 	config.CounterConfig[0].MeasurementLabel = "processor.time.total.pct"
 	config.CounterConfig[0].Query = "\\non existing object\\% Processor Performance"
 	config.CounterConfig[0].Format = "float"
-	handle, err := NewPerfmonReader(config)
+	handle, err := NewReader(config)
 	if assert.Error(t, err) {
 		assert.EqualValues(t, PDH_CSTATUS_NO_OBJECT, errors.Cause(err))
 	}
@@ -224,64 +226,66 @@ func TestNonExistingObject(t *testing.T) {
 }
 
 func TestLongOutputFormat(t *testing.T) {
-	query, err := NewQuery("")
+	var query Query
+	err := query.Open()
 	if err != nil {
 		t.Fatal(err)
 	}
 	defer query.Close()
-
-	err = query.AddCounter(processorTimeCounter, LongFormat, "")
+	counter := CounterConfig{Format: "long"}
+	err = query.AddCounter(processorTimeCounter, counter, false)
 	if err != nil && err != PDH_NO_MORE_DATA {
 		t.Fatal(err)
 	}
 
-	err = query.Execute()
+	err = query.CollectData()
 	if err != nil {
 		t.Fatal(err)
 	}
 
 	time.Sleep(time.Millisecond * 1000)
 
-	err = query.Execute()
+	err = query.CollectData()
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	values, err := query.Values()
+	values, err := query.GetFormattedCounterValues()
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	_, okLong := values[processorTimeCounter][0].Measurement.(int64)
+	_, okLong := values[processorTimeCounter][0].Measurement.(int32)
 
 	assert.True(t, okLong)
 }
 
 func TestFloatOutputFormat(t *testing.T) {
-	query, err := NewQuery("")
+	var query Query
+	err := query.Open()
 	if err != nil {
 		t.Fatal(err)
 	}
 	defer query.Close()
-
-	err = query.AddCounter(processorTimeCounter, FloatFormat, "")
+	counter := CounterConfig{Format: "float"}
+	err = query.AddCounter(processorTimeCounter, counter, false)
 	if err != nil && err != PDH_NO_MORE_DATA {
 		t.Fatal(err)
 	}
 
-	err = query.Execute()
+	err = query.CollectData()
 	if err != nil {
 		t.Fatal(err)
 	}
 
 	time.Sleep(time.Millisecond * 1000)
 
-	err = query.Execute()
+	err = query.CollectData()
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	values, err := query.Values()
+	values, err := query.GetFormattedCounterValues()
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -291,61 +295,53 @@ func TestFloatOutputFormat(t *testing.T) {
 	assert.True(t, okFloat)
 }
 
-func TestRawValues(t *testing.T) {
-	query, err := NewQuery("")
-	if err != nil {
-		t.Fatal(err)
+func TestWildcardQuery(t *testing.T) {
+	config := Config{
+		CounterConfig: make([]CounterConfig, 1),
 	}
-	defer query.Close()
-
-	err = query.AddCounter(processorTimeCounter, FloatFormat, "")
-	if err != nil && err != PDH_NO_MORE_DATA {
+	config.CounterConfig[0].InstanceLabel = "processor.name"
+	config.CounterConfig[0].InstanceName = "TestInstanceName"
+	config.CounterConfig[0].MeasurementLabel = "processor.time.pct"
+	config.CounterConfig[0].Query = `\Processor Information(*)\% Processor Time`
+	config.CounterConfig[0].Format = "float"
+	handle, err := NewReader(config)
+	if err != nil {
 		t.Fatal(err)
 	}
+	defer handle.query.Close()
 
-	var values []float64
-
-	for i := 0; i < 2; i++ {
-		if err = query.Execute(); err != nil {
-			t.Fatal(err)
-		}
-
-		_, rawvalue1, err := PdhGetRawCounterValue(query.counters[processorTimeCounter].handle)
-		if err != nil {
-			t.Fatal(err)
-		}
-
-		time.Sleep(time.Millisecond * 1000)
+	values, _ := handle.Read()
 
-		if err = query.Execute(); err != nil {
-			t.Fatal(err)
-		}
+	time.Sleep(time.Millisecond * 1000)
 
-		_, rawvalue2, err := PdhGetRawCounterValue(query.counters[processorTimeCounter].handle)
-		if err != nil {
-			t.Fatal(err)
-		}
+	values, err = handle.Read()
+	if err != nil {
+		t.Fatal(err)
+	}
 
-		value, err := PdhCalculateCounterFromRawValue(query.counters[processorTimeCounter].handle, PdhFmtDouble|PdhFmtNoCap100, rawvalue2, rawvalue1)
-		if err != nil {
-			t.Fatal(err)
-		}
+	pctKey, err := values[0].MetricSetFields.HasKey("processor.time.pct")
+	if err != nil {
+		t.Fatal(err)
+	}
+	assert.True(t, pctKey)
 
-		values = append(values, *(*float64)(unsafe.Pointer(&value.LongValue)))
+	pct, err := values[0].MetricSetFields.GetValue("processor.name")
+	if err != nil {
+		t.Fatal(err)
 	}
+	assert.NotEqual(t, "TestInstanceName", pct)
 
 	t.Log(values)
 }
 
-func TestWildcardQuery(t *testing.T) {
+func TestWildcardQueryNoInstanceName(t *testing.T) {
 	config := Config{
 		CounterConfig: make([]CounterConfig, 1),
 	}
-	config.CounterConfig[0].InstanceLabel = "processor.name"
-	config.CounterConfig[0].MeasurementLabel = "processor.time.pct"
-	config.CounterConfig[0].Query = `\Processor Information(*)\% Processor Time`
-	config.CounterConfig[0].Format = "float"
-	handle, err := NewPerfmonReader(config)
+	config.CounterConfig[0].InstanceLabel = "process_private_bytes"
+	config.CounterConfig[0].MeasurementLabel = "process.private.bytes"
+	config.CounterConfig[0].Query = `\Process(*)\Private Bytes`
+	handle, err := NewReader(config)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -360,12 +356,20 @@ func TestWildcardQuery(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	pctKey, err := values[0].MetricSetFields.HasKey("processor.time.pct")
+	pctKey, err := values[0].MetricSetFields.HasKey("process.private.bytes")
 	if err != nil {
 		t.Fatal(err)
 	}
 	assert.True(t, pctKey)
 
+	for _, s := range values {
+		pct, err := s.MetricSetFields.GetValue("process_private_bytes")
+		if err != nil {
+			t.Fatal(err)
+		}
+		assert.False(t, strings.Contains(pct.(string), "*"))
+	}
+
 	t.Log(values)
 }
 
@@ -389,7 +393,7 @@ func TestGroupByInstance(t *testing.T) {
 	config.CounterConfig[2].Query = `\Processor Information(_Total)\Average Idle Time`
 	config.CounterConfig[2].Format = "float"
 
-	handle, err := NewPerfmonReader(config)
+	handle, err := NewReader(config)
 	if err != nil {
 		t.Fatal(err)
 	}