Fix #1827 - multi instances in win_perf_counters #2352

bullshit · 2017-02-01T15:14:11Z

Required for all PRs:

CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
Sign CLA (if not already signed)
README.md updated (if adding a new plugin)

This PR fixes #1827. If you are using multiple instances identifier like "w3wp#1" the phd.dll returns only 2 characters in the SzName var. To be sure it is an error from the dll it self, i try to use phd.dll with c++
c++ source

Screenshot

Instance=chrome#4	Instance=chrome

As a fix i look if the configured instance name contains a "#". If this is true i look if the instance name starts with returned instance name.

bullshit · 2017-03-06T11:45:38Z

Is there anything i can do, to support the contributors to merge this PR?

danielnelson · 2017-03-17T01:08:51Z

plugins/inputs/win_perf_counters/win_perf_counters.go

@@ -265,6 +265,11 @@ func (m *Win_PerfCounters) Gather(acc telegraf.Accumulator) error {
 					} else if metric.instance == s {
 						// Catch if we set it to total or some form of it
 						add = true
+					} else if strings.Contains(metric.instance, "#") && strings.HasPrefix(metric.instance, s) {


Is this function iterating over all processes on the system? If so and if metric.instance = "chrome#4", what about processes named, for instance, cha? Would it be misnamed by this?

No this function iterates over all process that are retrieved for the specific counter

What about the possibility of a process prefix collision, can it happen if matching all instances? Should we test that s is length 2?

In my opinion it is impossible, because telegraf creates for every configured instance and WPC combination a unique query and handle to fetch the counter data.

for example:

[[inputs.win_perf_counters.object]] ObjectName = "Process" Counters = ["% Processor Time"] Instances = ["chrome","chrome#4"] Measurement = "win_chrome_cpu"

Valid: \Process(chrome)\% Processor Time Valid: \Process(chrome#4)\% Processor Time win_chrome_cpu,instance=chrome,objectname=Process,host=winston-pc Percent_Processor_Time=0.16016027331352234 1490095720000000000 win_chrome_cpu,instance=chrome#4,objectname=Process,host=winston-pc Percent_Processor_Time=0 1490095720000000000

for the instance "chrome" there are no problems, but because phd returns for the query \Process(chrome#4)\% Processor Time only ch as instance name the if statment at win_perf_counters.go#L265 isn't working. Thats the reason why i check if the configured instance name contains an hash and, just to be sure, the returned name from phd starts like the configured name. If phd.dll will be fixed in the future my changes won't be executed because of the previous else if statment on line win_perf_counters.go#L265. I don't see any reason why there should be a process prefix collision. A check if the length is 2 would be possible but what if there are cases where phd returns maybe more than 2 characters. I think a check with hasPrefix is the best solution. I hope this answers your questions.

danielnelson · 2017-03-17T01:09:37Z

plugins/inputs/win_perf_counters/win_perf_counters.go

@@ -265,6 +265,11 @@ func (m *Win_PerfCounters) Gather(acc telegraf.Accumulator) error {
 					} else if metric.instance == s {
 						// Catch if we set it to total or some form of it
 						add = true
+					} else if strings.Contains(metric.instance, "#") && strings.HasPrefix(metric.instance, s) {
+						// FIX: #1827


Remove this line

danielnelson · 2017-03-17T01:10:05Z

plugins/inputs/win_perf_counters/win_perf_counters.go

@@ -265,6 +265,11 @@ func (m *Win_PerfCounters) Gather(acc telegraf.Accumulator) error {
 					} else if metric.instance == s {
 						// Catch if we set it to total or some form of it
 						add = true
+					} else if strings.Contains(metric.instance, "#") && strings.HasPrefix(metric.instance, s) {
+						// FIX: #1827
+						// phd.dll only returns first 2 characters of the instance name


Please expand on this a bit, when does it only return 2 characters of the instance name?

If you are using multiple instances identifier like "w3wp#1" the phd.dll returns only 2 characters in the SzName var. To be sure it is an error from the dll it self, i try to use phd.dll with c++

Okay, so change the comment to: "If you are using a multiple instance identifier such as "w3wp#1" phd.dll returns only the first 2 characters of the identifier."

danielnelson · 2017-03-17T01:11:34Z

If anyone running windows can test and report back it would be very helpful.

bullshit · 2017-03-20T15:19:04Z

Currently, i'm running my changes in a production environment since january on 14 windows servers without any troubles

danielnelson · 2017-03-21T21:39:46Z

Can you update the CHANGELOG.md before I merge?

discoduck2x · 2017-03-23T08:08:43Z

@danielnelson @bullshit is this now included on the telegraf download page for win? or do i have to wait for next proper version release (our build myself) ?

danielnelson · 2017-03-23T18:30:18Z

It will be in the upcoming 1.3.0 release. We have nightly builds available though, this link should always have the latest build: https://dl.influxdata.com/telegraf/nightlies/telegraf-nightly_windows_amd64.zip

dudusakharovich · 2017-07-02T14:04:38Z

@danielnelson - I have the same issue, where telegraf can't retrieve data from all "w3wp" instances.
This is my configuration:

[[inputs.win_perf_counters.object]]
ObjectName = "Process"
Counters = ["% Processor Time","Handle Count","Private Bytes","Thread Count","Virtual Bytes","Working Set"]
Instances = ["*"]
Measurement = "win_proc"
IncludeTotal=true

I can only see data retrieved for one "w3wp" instance instead of 3 (w3wp, w3wp#1 & w3wp#2).
What is wrong with my configuration ?

bullshit added 2 commits February 1, 2017 15:54

Fix influxdata#1827 - multi instances in win perf counters

d71ad94

format source code

0772bfa

bullshit changed the title ~~Fix #1827 - multi instances in win perf counters~~ Fix #1827 - multi instances in win_perf_counters Feb 21, 2017

sparrc added this to the 1.3.0 milestone Mar 6, 2017

danielnelson suggested changes Mar 17, 2017

View reviewed changes

Remove comment

4bc22f4

bullshit mentioned this pull request Mar 20, 2017

Telegraf issues with Windows multi instance processes #1827

Closed

Update comment

7961248

danielnelson approved these changes Mar 21, 2017

View reviewed changes

bullshit and others added 2 commits March 22, 2017 08:04

Update changelog

7d17927

Merge branch 'master' into fix-1827-win_perf_counters

d59d742

danielnelson merged commit 616b66f into influxdata:master Mar 22, 2017

ssorathia pushed a commit to ssorathia/telegraf that referenced this pull request Mar 25, 2017

Multi instances in win_perf_counters (influxdata#2352)

065ec87

calerogers pushed a commit to calerogers/telegraf that referenced this pull request Apr 5, 2017

Multi instances in win_perf_counters (influxdata#2352)

b1b6033

vlamug pushed a commit to vlamug/telegraf that referenced this pull request May 30, 2017

Multi instances in win_perf_counters (influxdata#2352)

ab47801

bullshit mentioned this pull request Jul 14, 2017

UTF16PtrToString invalid for multi-instance win_perf_counters #1703

Closed

maxunt pushed a commit that referenced this pull request Jun 26, 2018

Multi instances in win_perf_counters (#2352)

6588c4a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1827 - multi instances in win_perf_counters #2352

Fix #1827 - multi instances in win_perf_counters #2352

bullshit commented Feb 1, 2017

bullshit commented Mar 6, 2017

danielnelson Mar 17, 2017

bullshit Mar 20, 2017 •

edited

Loading

danielnelson Mar 21, 2017

bullshit Mar 21, 2017 •

edited

Loading

danielnelson Mar 17, 2017

danielnelson Mar 17, 2017

bullshit Mar 20, 2017 •

edited

Loading

danielnelson Mar 20, 2017

danielnelson commented Mar 17, 2017

bullshit commented Mar 20, 2017 •

edited

Loading

danielnelson commented Mar 21, 2017

discoduck2x commented Mar 23, 2017

danielnelson commented Mar 23, 2017

dudusakharovich commented Jul 2, 2017

Fix #1827 - multi instances in win_perf_counters #2352

Fix #1827 - multi instances in win_perf_counters #2352

Conversation

bullshit commented Feb 1, 2017

Required for all PRs:

Screenshot

bullshit commented Mar 6, 2017

danielnelson Mar 17, 2017

Choose a reason for hiding this comment

bullshit Mar 20, 2017 • edited Loading

Choose a reason for hiding this comment

danielnelson Mar 21, 2017

Choose a reason for hiding this comment

bullshit Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

danielnelson Mar 17, 2017

Choose a reason for hiding this comment

danielnelson Mar 17, 2017

Choose a reason for hiding this comment

bullshit Mar 20, 2017 • edited Loading

Choose a reason for hiding this comment

danielnelson Mar 20, 2017

Choose a reason for hiding this comment

danielnelson commented Mar 17, 2017

bullshit commented Mar 20, 2017 • edited Loading

danielnelson commented Mar 21, 2017

discoduck2x commented Mar 23, 2017

danielnelson commented Mar 23, 2017

dudusakharovich commented Jul 2, 2017

bullshit Mar 20, 2017 •

edited

Loading

bullshit Mar 21, 2017 •

edited

Loading

bullshit Mar 20, 2017 •

edited

Loading

bullshit commented Mar 20, 2017 •

edited

Loading