Fix multiple plugin loading in win_perf_counters

[skburgart]

Fixes #1137

The root of the problem for #1137 is that all items are maintained in a global map, but the map is only populated by one config file. Once one config file is parsed, then a flag is set and the plugin will not populate items from any other config.

To fix this, I've removed the flag that checks if a config file has been parsed. Instead, the check for duplicates is done within the AddItem function. An item is considered a duplicate if it has the same query, objectName, counter, and instance.

To fix this, I've removed the global map and instead store items in the main plugin struct.

I've also cleaned up a couple other aspects of this plugin:

• Removed a local metrics variable
  • In the code, this variable was being populated as if it was going to be used for iterating and querying, but it never was. It was populated but never used, and the global variable was used instead.
    - Converted the global gItemList from map[int]*item to itemList, which is an already defined type equivalent to map[int]*item

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)

[danielnelson]

Since an instance of Win_PerfCounters is created for each top level plugin, won't this cause every instance to gather stats for the union of all per counters?

On other input plugins, we store the data for each plugin config in the main plugin struct to avoid cross talk.

[skburgart]

Ah that's a good point, I hadn't thought of that. I'll do some more work on this to remove the global map and cache all of the PDH handles as part of the main struct. Does that sound reasonable?

[skburgart]

I've pushed the latest changes - it feels much better now! The plugin no longer collects the same counters multiple times, and it doesn't rely on any global variables.

[skburgart]

One part of this plugin that I'm still unsure of is the code related to test. e.g. the globals testConfigParsed, testObject, and the struct variable TestName. There is no documentation on them in the README, and I can't seem to find their purpose. Are these variables maybe related to the -test flag for Telegraf? I want to make sure the changes in this PR don't mess with their intended function.

[danielnelson]

It looks to me that it is used by the unittests, not the -test flag, to clean up the old global variables. I think it could probably be gotten rid of after your changes. You can try to remove it if you like, so long that the unittests still work then I would consider it correct.

@PierreF can you review?

[PierreF]

Looks good to me.

The removal of unused Cleanup function made me think that there is probably a leak of resource (of PDH query handle). For example if we reload telegraf multiple time, it will re-open new query handle without closing old one.
I've not tested that and only guess it due to existence of PdhCloseQuery function that is never called (I don't see who would call Cleanup). Anyway if true, is was already present before this PR.

[danielnelson]

I opened #2843 for the handle leak. Is it even possible currently to reload plugins in Windows? Still should be fixed either by Switching to a ServiceInput or closing every Gather.