win_perf_counters: New option "Expand" expands wildcards in counter definitions. #2336

o7g8 · 2017-01-29T16:06:23Z

The change introduces a new option "Expand" to a definition of Windows counter which allows to expand wildcards in ObjectName, Instance and Counter.
Read "Remarks" in https://msdn.microsoft.com/en-us/library/windows/desktop/aa372605(v=vs.85).aspx about how the wildcards are expanded and how they can be used.

Required for all PRs:

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

…tion expands wildcards in ObjectName, Instances and Counter.

sparrc · 2017-01-29T18:47:21Z

I think this may solve #1291

@TheFlyingCorpse @discoduck2x could you review?

o7g8 · 2017-01-29T19:25:24Z

My change only expands wildcards and therefore allows to cover wider selection of counters with a compact config entry. It solves part of #1291.

Apart from wildcards the #1291 also addresses another issue - currently missing periodical "refresh" of the set of counters monitored by Telegeraf.
The "refresh" thing is independent from wildcards because the "refresh" can also be useful for non-wildcard counters: currently if we specify a process-level counter for a particular process in Telegraf config, then if the process doesn't exist at the moment when Telegraf reads config for win_perf_counters, then the counter will be ignored. Even after the "monitored" process starts, we will not get telemetry for the process with Telegeraf.

I see the "refresh" thing as a separate option in the counter definition, and it should be implemented on a separate pull request,

PierreF · 2017-01-30T10:54:01Z

@o7g8 did you know if PdhExpandCounterPath use localized name or English name for the counter ?

I ask this regarding #2261 which allow to use the same name (English name) on all system. Microsoft documentation is not always clear about this, but PdhExpandCounterPath doesn't require localized name to work ? If yes, it may disallow to use this option on non-English system as PdhExpandCounterPath will require localized name and later PdhAddEnglishCounter will require an English name.

o7g8 · 2017-01-30T11:50:58Z

@PierreF , thank you for your comment! Here is the documentation describing handling of localized names:

Note If the counter path contains a wildcard character, the non-wildcard portions of the path will be localized, but wildcards will not be expanded before adding the localized counter path to the query. In this case, you will need use the following procedure to add all matching counter names to the query. 1.Make a query
2.Use PdhAddEnglishCounter with the string containing wildcards
3.Use PdhGetCounterInfo on the counter handle returned by PdhAddEnglishCounter to get a localized full path (szFullPath.) This string still contains wildcards, but the non-wildcard parts are now localized.
4.Use PdhExpandWildCardPath to expand the wildcards.
5.Use PdhAddCounter on each of the resulting paths

BTW there is another thing which I've just noticed and which is also important - the PdhExpandCounterPath doesn't support partial wildcards! I will change the code and will use PdhExpandWildCardPath instead of PdhExpandCounterPath, because it more flexible and allows partial wildcrads (on post-Vista systems).

Seems like my code requires some more work for non-English systems.

…CounterPath.

o7g8 · 2017-01-30T23:56:15Z

I have replaced PdhExpandCounterPath with PdhExpandWildCardPath, which supports the partial wildcards. It works, but it's not as flexible as one could expect. It expands correctly following queries:

\LogicalDisk(C*)\% Free Space - expands the query to the disk "C:"
\LogicalDisk(C*)\* - returns all counters for the disk "C:"
\LogicalDisk(*:)\* - returns all counters for the disk "C:" (on my machine)

But surprisingly it doesn't expand following obvious queries with partial wildcards in Counters part:

\LogicalDisk(C:)\%* - I would expect all counters starting with %, but it fails with "The specified counter could not be found"
\LogicalDisk(C:)\Av* - I would expect all counters starting with Av (e.g. "Average..."), but it fails with "The specified counter could not be found"
\LogicalDisk(C:)\*Time - I would expect all counters ending with Time, but it returns all the C: counters instead.

From the experiments I conclude that the PdhExpandWildCardPath does a good job with partial wildcards for Instance, but it seems to support only non-partial wildcards for Counters. Not perfect, but a step forward compared to PdhExpandCounterPath.

@PierreF I haven't tried the approach with the call to PdhGetCounterInfo, quoted in my previous message. It requires allocation of PDH_COUNTER_INFO structure, which looks a bit scary.
I have found some Go examples for the structure signature and for the PdhGetCounterInfo signature. Can anyone help with adaption of the struct signature to our codebase?
I wonder if the support for non-English systems coud be treated as a separate concern on another pull request?

TheFlyingCorpse · 2017-02-08T19:16:49Z

This looks good to me!

discoduck2x · 2017-02-13T08:22:17Z

@sparrc , sorry missed the notify for the latest status on this one , will test asap and get back !

discoduck2x · 2017-02-13T11:36:28Z

@sparrc @TheFlyingCorpse can i find this commit in a binary somewhere? doesnt seem to be any nightly builds available for windows.

sparrc · 2017-03-02T12:09:44Z

@PierreF could you provide a final review on this?

PierreF · 2017-03-02T12:57:49Z

One issue: from my understanding this will not works on non-English Windows if we don't set PreVistaSupport = true.

PdhExpandWildCardPath seems to take localized name and later (if PreVistaSupport is False) we use PdhAddEnglishCounter.

I think we should write in README that when expand is true, counter need to use localized name and it would be great to change the "if m.PreVistaSupport" to include an "or PerfObject.Expand" so when Expand is enabled we use PdhAddCounter instead of PdhAddEnglishCounter.

That being said, while testing I got the following error:

panic: runtime error: slice bounds out of range

goroutine 14 [running]:
github.com/influxdata/telegraf/plugins/inputs/win_perf_counters.UTF16ToStringArray(0xc04200e480, 0x5c, 0x5c, 0xc042175f38, 0x0, 0x5c)
        /go/src/github.com/influxdata/telegraf/plugins/inputs/win_perf_counters/pdh.go:443 +0x134
github.com/influxdata/telegraf/plugins/inputs/win_perf_counters.expandCounterQuery(0xc042343c60, 0x20, 0x6, 0xc042343c60, 0xc042343c6f, 0xc042407e18, 0x0)
        /go/src/github.com/influxdata/telegraf/plugins/inputs/win_perf_counters/win_perf_counters.go:167 +0x142
github.com/influxdata/telegraf/plugins/inputs/win_perf_counters.expandQuery(0xc042343c60, 0x20, 0x1, 0x6, 0xc042343c60, 0x20, 0x0, 0x0)
        /go/src/github.com/influxdata/telegraf/plugins/inputs/win_perf_counters/win_perf_counters.go:193 +0xd7
github.com/influxdata/telegraf/plugins/inputs/win_perf_counters.(*Win_PerfCounters).ParseConfig(0xc042171600, 0xc042407ea0, 0x0, 0x0)
        /go/src/github.com/influxdata/telegraf/plugins/inputs/win_perf_counters/win_perf_counters.go:207 +0x285
github.com/influxdata/telegraf/plugins/inputs/win_perf_counters.(*Win_PerfCounters).Gather(0xc042171600, 0x16aa740, 0xc042343c00, 0x0, 0x0)
        /go/src/github.com/influxdata/telegraf/plugins/inputs/win_perf_counters/win_perf_counters.go:270 +0x822
github.com/influxdata/telegraf/agent.gatherWithTimeout.func1(0xc04232c8a0, 0xc0421716c0, 0xc042343c00)
        /go/src/github.com/influxdata/telegraf/agent/agent.go:153 +0x50
created by github.com/influxdata/telegraf/agent.gatherWithTimeout
        /go/src/github.com/influxdata/telegraf/agent/agent.go:154 +0xd3

Configuration used:

[[outputs.file]]
    files = ["stdout"]

[[inputs.win_perf_counters]]
  [[inputs.win_perf_counters.object]]
    ObjectName = "Processeur"
    Instances = ["*"]
    Counters = [
       "% d’inactivité",
    ]
    Expand = true

sparrc · 2017-03-02T13:06:59Z

so when Expand is enabled we use PdhAddCounter instead of PdhAddEnglishCounter.

@PierreF could you remind me why we ever use "PdhAddEnglishCounter"? Why can't we just always use PdhAddCounter?

PierreF · 2017-03-02T13:08:02Z

@sparrc Because with PdhAddCounter use need localized name, which means you can not provide one configuration file for all systems. You need one configuration per localization.

PierreF · 2017-03-02T13:09:59Z

plugins/inputs/win_perf_counters/pdh.go

+	stringLine := UTF16PtrToString(&buf[0])
+	for stringLine != "" {
+		strings = append(strings, stringLine)
+		nextLineStart += len(stringLine) + 1


Pretty sure that +1 should not be here.
If your string has a length of 1 character, the next string is one bytes later, not 1+1 bytes later :)
And this is very likely my issue with panic: runtime error

@PierreF The strings are NULL terminated, so that +1 is for hopping over that one I guess.

This looks very fishy to me, the length of stringLine tells us nothing about the offset into buf, because buf contains utf16 while stringLine contains utf8.

sparrc · 2017-03-02T13:11:13Z

I see....I think I misunderstood how that worked.

@PierreF In that case, why do we ever use PdhAddCounter? Is it unreasonable to ask users in other countries to use the English names? From what I can tell they are fairly similar...

PierreF · 2017-03-02T13:18:38Z

Yes, I think that too that PdhAddEnglishCounter should be always used.
The support for PdhAddCounter was to support pre-vista Windows. I would +1 a drop of this (pre-vista... means Windows XP, Vista support ends in few month, everyone should be running Vista or more :))

I haven't tried the approach with the call to PdhGetCounterInfo, quoted in my previous message. It requires allocation of PDH_COUNTER_INFO structure, which looks a bit scary.
[...]
I wonder if the support for non-English systems coud be treated as a separate concern on another pull request?

That seems reasonable (as long as we wrote in README that currently Expand require localized name as in my previous comment). But it could be a good idea to support only English name and properly implements #2336 (comment)

discoduck2x · 2017-03-05T22:17:46Z

@o7g8 @PierreF , anywhere i can get hold of a binary to test ? or will this be merged soon?

TheFlyingCorpse · 2017-03-06T09:43:54Z

A note on the English name. some environments likely use only native language. forcing use of the english counter names can be a negative impact of forcing the removal of the localized alternative (pre-vista).

matso42 · 2017-03-08T08:43:54Z

I would also be interested in a Windows binary nightly build on this.

vtsingaras · 2017-07-14T09:48:48Z

Hello, I have merged o7g8's branch into current master and tested on Windows 10 and Windows Server 2016. It seems to be working just fine. Is it possible to have this PR merged? I could open a new PR with the merges and resolved conflicts if necessary.

danielnelson

This is probably a stupid question, but what would happen if we just always treated it as if expand was enabled?

danielnelson · 2017-07-14T19:44:46Z

plugins/inputs/win_perf_counters/pdh.go

+	stringLine := UTF16PtrToString(&buf[0])
+	for stringLine != "" {
+		strings = append(strings, stringLine)
+		nextLineStart += len(stringLine) + 1


This looks very fishy to me, the length of stringLine tells us nothing about the offset into buf, because buf contains utf16 while stringLine contains utf8.

danielnelson · 2017-07-14T19:59:37Z

plugins/inputs/win_perf_counters/win_perf_counters.go

+	var buf []uint16
+	ret := PdhExpandWildCardPath(query, nil, &bufSize)
+	for ret == PDH_MORE_DATA {
+		buf = make([]uint16, bufSize)


This code does not seem defensive enough to me. Can bufSize be zero here, if so then the next line will panic. What if ret is not PDH_MORE_DATA at least once, then we will try to index it in UTF16ToStringArray and panic. In general we need to check anything that crosses an application boundary before we can assume anything about it.

vtsingaras · 2017-07-20T08:23:29Z

@danielnelson Hi, sorry for the delay. I agree with you that we should always treat that expand is enabled.

If we always expand the paths, then it would also make the code that decides if we should add the counter measurement inside Gather() simpler, as if we pre-expand when parsing config we know when inside Gather() that we want this measurement (with the exception of still filtering for _Total).
It would also make it simpler to correctly parse multiple instances with a '#' in their name.

danielnelson · 2018-05-25T02:23:37Z

This PR has been superseded by #4189

Oleg Grytsynevych added 2 commits January 29, 2017 16:44

Adds a new option "Expand" to a difinition of Windows counter. The op…

0edd85a

…tion expands wildcards in ObjectName, Instances and Counter.

Documentation is updated for the "Expand" option.

c797cb1

sparrc added this to the 1.3.0 milestone Jan 30, 2017

Use PdhExpandWildCardPath for wildcard expansion instead of PdhExpand…

2e72c15

…CounterPath.

o7g8 mentioned this pull request Jan 31, 2017

win_perf_counters: Handle "dynamic" counters with the new KeepAlive counter option #2345

Closed

1 task

PierreF reviewed Mar 2, 2017

View reviewed changes

sparrc modified the milestones: 1.4.0, 1.3.0 Mar 8, 2017

sparrc added the platform/windows label Mar 8, 2017

vtsingaras mentioned this pull request Jul 14, 2017

Win perf counters expand wildcards #3018

Closed

3 tasks

danielnelson suggested changes Jul 14, 2017

View reviewed changes

danielnelson modified the milestones: 1.4.0, 1.5.0 Aug 14, 2017

danielnelson added area/windows Related to windows plugins (win_eventlog, win_perf_counters, win_services) feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Aug 23, 2017

danielnelson modified the milestones: 1.5.0, 1.6.0 Nov 30, 2017

danielnelson removed this from the 1.6.0 milestone Jan 4, 2018

ivankudibal mentioned this pull request May 10, 2018

Cannot use instance wildcard in win_perf_counters #2879

Closed

danielnelson closed this May 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

win_perf_counters: New option "Expand" expands wildcards in counter definitions. #2336

win_perf_counters: New option "Expand" expands wildcards in counter definitions. #2336

o7g8 commented Jan 29, 2017 •

edited

Loading

sparrc commented Jan 29, 2017

o7g8 commented Jan 29, 2017 •

edited

Loading

PierreF commented Jan 30, 2017

o7g8 commented Jan 30, 2017

o7g8 commented Jan 30, 2017

TheFlyingCorpse commented Feb 8, 2017

discoduck2x commented Feb 13, 2017

discoduck2x commented Feb 13, 2017

sparrc commented Mar 2, 2017

PierreF commented Mar 2, 2017

sparrc commented Mar 2, 2017

PierreF commented Mar 2, 2017

PierreF Mar 2, 2017

vtsingaras Jul 14, 2017

danielnelson Jul 14, 2017

sparrc commented Mar 2, 2017

PierreF commented Mar 2, 2017

discoduck2x commented Mar 5, 2017

TheFlyingCorpse commented Mar 6, 2017

matso42 commented Mar 8, 2017

vtsingaras commented Jul 14, 2017

danielnelson left a comment

danielnelson Jul 14, 2017

danielnelson Jul 14, 2017

vtsingaras commented Jul 20, 2017

danielnelson commented May 25, 2018

win_perf_counters: New option "Expand" expands wildcards in counter definitions. #2336

win_perf_counters: New option "Expand" expands wildcards in counter definitions. #2336

Conversation

o7g8 commented Jan 29, 2017 • edited Loading

Required for all PRs:

sparrc commented Jan 29, 2017

o7g8 commented Jan 29, 2017 • edited Loading

PierreF commented Jan 30, 2017

o7g8 commented Jan 30, 2017

o7g8 commented Jan 30, 2017

TheFlyingCorpse commented Feb 8, 2017

discoduck2x commented Feb 13, 2017

discoduck2x commented Feb 13, 2017

sparrc commented Mar 2, 2017

PierreF commented Mar 2, 2017

sparrc commented Mar 2, 2017

PierreF commented Mar 2, 2017

PierreF Mar 2, 2017

Choose a reason for hiding this comment

vtsingaras Jul 14, 2017

Choose a reason for hiding this comment

danielnelson Jul 14, 2017

Choose a reason for hiding this comment

sparrc commented Mar 2, 2017

PierreF commented Mar 2, 2017

discoduck2x commented Mar 5, 2017

TheFlyingCorpse commented Mar 6, 2017

matso42 commented Mar 8, 2017

vtsingaras commented Jul 14, 2017

danielnelson left a comment

Choose a reason for hiding this comment

danielnelson Jul 14, 2017

Choose a reason for hiding this comment

danielnelson Jul 14, 2017

Choose a reason for hiding this comment

vtsingaras commented Jul 20, 2017

danielnelson commented May 25, 2018

o7g8 commented Jan 29, 2017 •

edited

Loading

o7g8 commented Jan 29, 2017 •

edited

Loading