Perfmon metricset output format option #3976

martinscholz83 · 2017-04-10T14:31:30Z

This PR adds the a new config option format for the perfmon metricset. Possible values are float, uint64 and uint32. The default value is float. My first approach was to set the format for all counters. But i think to define it on counter level gives the user more flexibility.

elasticmachine · 2017-04-10T14:32:02Z

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

elasticmachine · 2017-04-10T14:32:13Z

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

andrewkroh · 2017-04-12T15:30:25Z

metricbeat/module/windows/perfmon/defs_pdh_windows.go

 	// PdhFmtLong returns data as a long integer.
-	PdhFmtLong = C.PDH_FMT_LONG
+	PdhFmtLong PdhCounterFormat = C.PDH_FMT_LONG


Looks like other formatters below also should have the same PdhCounterFormat type applied.

andrewkroh · 2017-04-12T15:35:22Z

metricbeat/module/windows/perfmon/pdh_windows.go

 	}, nil
 }

-func (q *Query) AddCounter(counterPath string) error {
+func (q *Query) AddCounter(counterPath string, format string) error {


From an API perspective I would not allow a free-form string for format. I would declare an enum for the allowed format types. And then inside of the function reject unknown format enum values with an error.

andrewkroh · 2017-04-12T15:37:05Z

metricbeat/module/windows/perfmon/perfmon.go

@@ -3,15 +3,20 @@
 package perfmon

 import (
+	"strings"
+


Remove the newline between stdlib imports and sort them.

andrewkroh · 2017-04-12T15:40:29Z

metricbeat/module/windows/perfmon/perfmon.go

@@ -37,6 +42,18 @@ func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
 		return nil, err
 	}

+	for _, value := range config.CounterConfig {
+		form := strings.ToLower(value.Format)


Consider using a switch here.

switch strings.ToLower(value.Format) { case "": // use float case "float", "uint32", "uint64": default: // error }

andrewkroh · 2017-04-12T15:44:17Z

metricbeat/module/windows/perfmon/_meta/docs.asciidoc

@@ -8,6 +8,8 @@ performance counters.

 You must configure queries for the Windows performance counters that you wish
 to collect. The example below collects processor time and disk writes.
+With `format` you can set the output format for an specific counter. Possible values are


I don't think it makes sense to expose the intricacies of uint32 vs uint64 because JSON doesn't make any distinction between these types. Maybe just offer float and long (and long uses uint64 under the hood).

typo: should say "for a specific counter"

andrewkroh · 2017-04-12T15:49:02Z

metricbeat/module/windows/perfmon/pdh_windows.go

+	switch format {
+	case "float":
+		q.counters[counterPath].format = PdhFmtDouble
+	case "uint64":


Shouldn't these actually be called int64 and int32? I didn't see any mention that the values are unsigned in the docs (possibly I missed it). And you do cast to int64 and int32 later in the code.

andrewkroh · 2017-04-12T15:51:31Z

metricbeat/module/windows/perfmon/pdh_integration_windows_test.go

@@ -18,7 +18,7 @@ func TestQuery(t *testing.T) {
 	}
 	defer q.Close()

-	err = q.AddCounter(processorTimeCounter)
+	err = q.AddCounter(processorTimeCounter, "float")


We need some additional test cases for the new formats.

andrewkroh · 2017-04-12T15:55:49Z

metricbeat/module/windows/perfmon/pdh_windows.go

@@ -118,20 +133,28 @@ func (q *Query) Execute() error {
 }

 type Value struct {
-	Num float64
+	Num interface{}


From a user perspective I'm not really liking that I have to type assert Num to get interpret the value. Maybe in a separate PR we could improve the API for Query. One option would be to have separate FloatValues() and Int64Values() methods that return map[string]FloatValue and map[string]Int64Value respectively.

martinscholz83 · 2017-04-19T08:58:17Z

Hi @andrewkroh. I have changed the format option you proposed. Now there are only long and float option. And some other small changes.

andrewkroh · 2017-04-21T14:14:43Z

jenkins, test it

andrewkroh · 2017-04-21T14:17:04Z

metricbeat/module/windows/perfmon/pdh_windows.go

+type Format int
+
+const (
+	FLOAT Format = iota


I would probably call these FloatFormat and LongFormat since the names are rather generic. Constants should use upper-camel-case (I usually ignore this rule for constants brought over from C code).

andrewkroh · 2017-04-25T12:13:25Z

There is a merge conflict that needs resolved.

dedemorton · 2017-04-25T16:25:53Z

metricbeat/module/windows/perfmon/_meta/docs.asciidoc

@@ -8,6 +8,8 @@ performance counters.

 You must configure queries for the Windows performance counters that you wish
 to collect. The example below collects processor time and disk writes.
+With `format` you can set the output format for an specific counter. Possible values are
+`float`, and `long`. If nothing is selected the default value is `float`.


Minor: should say float and long (no comma)

@dedemorton, many thanks!!

dedemorton · 2017-04-25T16:27:14Z

metricbeat/module/windows/perfmon/_meta/docs.asciidoc

@@ -8,6 +8,8 @@ performance counters.

 You must configure queries for the Windows performance counters that you wish
 to collect. The example below collects processor time and disk writes.
+With `format` you can set the output format for an specific counter. Possible values are


typo: should say "for a specific counter"

andrewkroh · 2017-05-04T13:01:48Z

metricbeat/module/windows/perfmon/perfmon.go

+		case "float", "long":
+		default:
+			err := fmt.Errorf("format '%s' for counter '%s' are not valid", value.Format, value.Alias)
+			return nil, errors.Wrap(err, "initialization failed")


You could use errors.Errorf() here.

martinscholz83 added 2 commits April 10, 2017 14:51

Add output format option

b9555b5

Add default value

c1c9aa8

Fix integration tests

3fd5fc0

ruflin added Metricbeat Metricbeat review labels Apr 12, 2017

andrewkroh reviewed Apr 12, 2017

View reviewed changes

martinscholz83 added 4 commits April 17, 2017 11:36

Set formatters type

fc39228

Change format types

5da6dc2

WIP

5ec3e44

Add format enum

4a8ef0d

Add integration tests

beb670d

andrewkroh reviewed Apr 21, 2017

View reviewed changes

Rename format enums

abb4d65

Merge branch 'master' into perfmon_output-format

a14599f

dedemorton reviewed Apr 25, 2017

View reviewed changes

martinscholz83 added 2 commits April 26, 2017 07:59

Fix typo docs.asciidoc

44377f0

Fix tests

e2ace44

andrewkroh reviewed May 4, 2017

View reviewed changes

andrewkroh approved these changes May 4, 2017

View reviewed changes

andrewkroh merged commit fdcfdd4 into elastic:master May 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perfmon metricset output format option #3976

Perfmon metricset output format option #3976

martinscholz83 commented Apr 10, 2017

elasticmachine commented Apr 10, 2017

elasticmachine commented Apr 10, 2017

andrewkroh Apr 12, 2017

andrewkroh Apr 12, 2017

andrewkroh Apr 12, 2017

andrewkroh Apr 12, 2017

andrewkroh Apr 12, 2017

dedemorton Apr 25, 2017

andrewkroh Apr 12, 2017

andrewkroh Apr 12, 2017

andrewkroh Apr 12, 2017

martinscholz83 commented Apr 19, 2017

andrewkroh commented Apr 21, 2017

andrewkroh Apr 21, 2017

andrewkroh commented Apr 25, 2017

dedemorton Apr 25, 2017

martinscholz83 Apr 26, 2017

dedemorton Apr 25, 2017

andrewkroh May 4, 2017

Perfmon metricset output format option #3976

Perfmon metricset output format option #3976

Conversation

martinscholz83 commented Apr 10, 2017

elasticmachine commented Apr 10, 2017

elasticmachine commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinscholz83 commented Apr 19, 2017

andrewkroh commented Apr 21, 2017

Choose a reason for hiding this comment

andrewkroh commented Apr 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment