Add group_measurements_by_instance_label flag to perfmon configuration #8688

ramenjosh · 2018-10-23T06:16:37Z

This flag will send all perfmon measurements with a matching instance label as part of the same event (i.e. all metrics for C:, Processor X, etc.). This addresses some of the issues raised in #6584.

In most cases enabling this flag considerably reduces the number of events sent by metricbeat.

…n. This flag will send all perfmon measurements with a matching instance label as part of the same event (i.e. all metrics for C:\, Processer X, etc.). Enabling this flag considerably reduces the number of events sent by metricbeat.

elasticmachine · 2018-10-23T06:16:38Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elasticcla · 2018-10-23T06:25:11Z

Hi @smi9cm, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

…les.

jsoriano

@smi9cm thanks working on this! This looks like a good addition. Code looks mostly good to me, only some details in the config reference and with error handling.
There is also an error in CI builds that should be fixed by make update.

You will also need to sign the CLA before we can merge this.

jsoriano · 2018-11-07T11:16:14Z

metricbeat/module/windows/_meta/config.reference.yml

@@ -3,6 +3,7 @@
  enabled: true
  period: 10s
  perfmon.ignore_non_existent_counters: true
+  perfmon.group_measurements_by_instance: true


These two options should be false here as this is its default and this file is used for reference. Could you change it too in the ignore_non_existent_counters?

metricbeat/module/windows/perfmon/pdh_windows.go

jsoriano · 2018-11-07T11:29:35Z

Oh, and could you please add a changelog entry?

…rs fields to false. Update changelog and ensure windows.asciidoc is up to date.

andyhoffman12 · 2019-01-10T20:10:00Z

@smi9cm Any chance you can sign off on this? I really would like to see this feature implemented.

…dual event. This solves the problem of only keeping the error contained in the first event. Also added simple unit test.

metricbeat/module/windows/perfmon/pdh_integration_windows_test.go

ramenjosh · 2019-01-11T07:25:49Z

@jsoriano, sorry for the delay in getting back to this. I've updated the docs like you've suggested.

As for error handling, I think the cleanest approach is to always send counters that contain an error as individual events. This is the same as the original behaviour (i.e. without grouping) and ensures we always capture the errors.

Also, I've signed the CLA a couple of times now but the bot keeps reporting that I haven't - is there any chance you could check what's going on there?

jsoriano · 2019-01-11T12:02:44Z

@smi9cm regarding CLA, you need to use the same email in your commits, in your commits I see Joshua Smith <joshuasmith@Joshuas-MacBook-Pro.local> as author, you need to change this to an email you used to sign the CLA.

ramenjosh · 2019-01-13T04:55:56Z

@jsoriano too easy, should be fixed now 👍

jsoriano

Ok, it looks good to me, I have added some more comments mostly about code style, not important. Let me know what you think.

jsoriano · 2019-01-14T16:32:46Z

metricbeat/module/windows/perfmon/pdh_integration_windows_test.go

+	config.CounterConfig[2].InstanceLabel = "processor.name"
+	config.CounterConfig[2].MeasurementLabel = "processor.time.idle.average.ns"
+	config.CounterConfig[2].Query = `\Processor Information(_Total)\Average Idle Time`
+	config.CounterConfig[2].Format = "float"


You can define the list of counter configs inline, something like this:

counterConfigs := []CounterConfig{ { InstanceLabel: "processor.name", MeasurementLabel: "processor.time.pct", Query: `\Processor Information(_Total)\% Processor Time`, Format: "float", }, ... } config := Config{ CounterConfig: counterConfigs, GroupMeasurements: true, }

jsoriano · 2019-01-14T16:34:02Z

metricbeat/module/windows/perfmon/pdh_integration_windows_test.go

+
+	values, _ := handle.Read()
+
+	time.Sleep(time.Millisecond * 1000)


Suggested change

time.Sleep(time.Millisecond * 1000)

time.Sleep(time.Second)

🙂

It'd be nice though if this could be done without depending on Sleep.

jsoriano · 2019-01-14T16:36:35Z

metricbeat/module/windows/perfmon/pdh_integration_windows_test.go

+	values, err = handle.Read()
+	if err != nil {
+		t.Fatal(err)
+	}


These ifs can be replaced with require.NoError(t, err)

require is available as "github.com/stretchr/testify/require"

andyhoffman12 · 2019-02-27T23:27:40Z

@smi9cm @jsoriano I'm anxiously waiting for this, is there anything I can do to get this moved along?

jsoriano · 2019-02-28T13:44:44Z

Ok, I am going to merge this and open a follow up for the pending changes. Thanks!

andrewkroh · 2019-02-28T18:55:44Z

I think this merge is causing failures (noticed it on a different PR):

module\windows\perfmon\pdh_integration_windows_test.go:384:14: no new variables on left side of :=
module\windows\perfmon\pdh_integration_windows_test.go:390:14: no new variables on left side of :=

This PR was never tested on Jenkins so it received no Windows testing.

jsoriano · 2019-02-28T19:32:35Z

@andrewkroh 🤦‍♂️ ok, let me revert it by now

…iguration (elastic#8688)" This reverts commit fbee6a2.

jsoriano · 2019-02-28T19:40:41Z

PR to revert this #11001

…iguration (#8688)" (#11001) This reverts commit fbee6a2.

andrewkroh · 2019-02-28T21:24:11Z

metricbeat/module/windows/perfmon/pdh_integration_windows_test.go

+	}
+	assert.True(t, pctKey)
+
+	t.Log(values)


@smi9cm I tested this and it's producing more than one event, but the assertion is expecting only one. There are a few compilation errors in the test code so we are going to revert this the merge in #11001. I'd love it if you could reopen the PR after you have looked into the failing test case. Thanks!

pdh_integration_windows_test.go:378: { "processor": { "name": "_Total", "time": { "idle": { "average": { "ns": 10932693.47826087 } }, "pct": 0 } } } pdh_integration_windows_test.go:378: { "disk": { "bytes": { "read": { "total": 0 } } } }

@smi9cm, Me and @jsoriano discussed the test case and made some changes and opened up a PR #11002.

New PR looks good to me, apologies for breaking the build there! Thanks @jsoriano as well.

elastic#8688) This flag will send all perfmon measurements with a matching instance label as part of the same event (i.e. all metrics for C:, Processor X, etc.). This addresses some of the issues raised in elastic#6584. In most cases enabling this flag considerably reduces the number of events sent by metricbeat.

fix: Add group_measurements_by_instance flag to reference and docs fi…

0e95428

…les.

ruflin added module review Metricbeat Metricbeat labels Oct 23, 2018

jsoriano added the :Windows label Nov 7, 2018

jsoriano reviewed Nov 7, 2018

View reviewed changes

Joshua Smith added 3 commits November 8, 2018 13:39

Default group_measurements_by_instance and ignore_non_existent_counte…

9ee48b5

…rs fields to false. Update changelog and ensure windows.asciidoc is up to date.

Merge remote-tracking branch 'upstream/master'

a64fe5d

Use correct string conversion.

f7c07de

ruflin added the Team:Integrations Label for the Integrations team label Nov 21, 2018

alvarolobato assigned jsoriano and unassigned jsoriano Dec 10, 2018

andyhoffman12 mentioned this pull request Jan 10, 2019

Problem with aggregation of Performance Counters within same category #4944

Closed

Joshua Smith added 2 commits January 11, 2019 15:38

Merge remote-tracking branch 'upstream/master'

62f7e2a

If a counter contains an error, it should always be sent as an indivi…

663431d

…dual event. This solves the problem of only keeping the error contained in the first event. Also added simple unit test.

ramenjosh requested review from a team as code owners January 11, 2019 07:18

houndci-bot reviewed Jan 11, 2019

View reviewed changes

metricbeat/module/windows/perfmon/pdh_integration_windows_test.go Outdated Show resolved Hide resolved

fix missing comma

6bb4187

urso removed the request for review from a team January 11, 2019 15:46

Update metricbeat reference in x-pack folder

6e1be87

jsoriano reviewed Jan 14, 2019

View reviewed changes

jsoriano approved these changes Feb 28, 2019

View reviewed changes

jsoriano merged commit fbee6a2 into elastic:master Feb 28, 2019

jsoriano added a commit to jsoriano/beats that referenced this pull request Feb 28, 2019

Revert "Add group_measurements_by_instance_label flag to perfmon conf…

63e236b

…iguration (elastic#8688)" This reverts commit fbee6a2.

jsoriano mentioned this pull request Feb 28, 2019

Revert "Add group_measurements_by_instance_label flag to perfmon configuration (#8688)" #11001

Merged

jsoriano added a commit that referenced this pull request Feb 28, 2019

Revert "Add group_measurements_by_instance_label flag to perfmon conf…

70fd801

…iguration (#8688)" (#11001) This reverts commit fbee6a2.

andrewkroh reviewed Feb 28, 2019

View reviewed changes

jsoriano mentioned this pull request Feb 28, 2019

Add group_measurements_by_instance_label flag to perfmon configuration #11002

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add group_measurements_by_instance_label flag to perfmon configuration #8688

Add group_measurements_by_instance_label flag to perfmon configuration #8688

ramenjosh commented Oct 23, 2018

elasticmachine commented Oct 23, 2018

elasticcla commented Oct 23, 2018

jsoriano left a comment

jsoriano Nov 7, 2018

jsoriano commented Nov 7, 2018

andyhoffman12 commented Jan 10, 2019

ramenjosh commented Jan 11, 2019

jsoriano commented Jan 11, 2019

ramenjosh commented Jan 13, 2019

jsoriano left a comment

jsoriano Jan 14, 2019

jsoriano Jan 14, 2019

jsoriano Jan 14, 2019

jsoriano Jan 14, 2019

andyhoffman12 commented Feb 27, 2019

jsoriano commented Feb 28, 2019

andrewkroh commented Feb 28, 2019

jsoriano commented Feb 28, 2019

jsoriano commented Feb 28, 2019

andrewkroh Feb 28, 2019 •

edited

Loading

andrewkroh Feb 28, 2019

ramenjosh Feb 28, 2019


		values, _ := handle.Read()

		time.Sleep(time.Millisecond * 1000)

Add group_measurements_by_instance_label flag to perfmon configuration #8688

Add group_measurements_by_instance_label flag to perfmon configuration #8688

Conversation

ramenjosh commented Oct 23, 2018

elasticmachine commented Oct 23, 2018

elasticcla commented Oct 23, 2018

jsoriano left a comment

Choose a reason for hiding this comment

jsoriano Nov 7, 2018

Choose a reason for hiding this comment

jsoriano commented Nov 7, 2018

andyhoffman12 commented Jan 10, 2019

ramenjosh commented Jan 11, 2019

jsoriano commented Jan 11, 2019

ramenjosh commented Jan 13, 2019

jsoriano left a comment

Choose a reason for hiding this comment

jsoriano Jan 14, 2019

Choose a reason for hiding this comment

jsoriano Jan 14, 2019

Choose a reason for hiding this comment

jsoriano Jan 14, 2019

Choose a reason for hiding this comment

jsoriano Jan 14, 2019

Choose a reason for hiding this comment

andyhoffman12 commented Feb 27, 2019

jsoriano commented Feb 28, 2019

andrewkroh commented Feb 28, 2019

jsoriano commented Feb 28, 2019

jsoriano commented Feb 28, 2019

andrewkroh Feb 28, 2019 • edited Loading

Choose a reason for hiding this comment

andrewkroh Feb 28, 2019

Choose a reason for hiding this comment

ramenjosh Feb 28, 2019

Choose a reason for hiding this comment

andrewkroh Feb 28, 2019 •

edited

Loading