386fix #4076

srclosson · 2018-04-25T17:19:13Z

Required for all PRs:

[*] Signed CLA.
[*] Associated README.md updated.
[*] Has appropriate unit tests.

…re out how to separate a 32bit build from a 64 bit build. Enhancement: Allow collecting of perfmon data from a remote node Enhancement: Allow collecting data using the timestamp from the source and don't re-timestamp

Fix: 32 and 64 bit versions split out into separate files

danielnelson

Thanks @srclosson for taking the time to look at this code, getting these types right is quite tricky. Below are some comments, but also make sure take a look at the unit test failures on appveyor, they run on a 64-bit Windows system.

danielnelson · 2018-04-25T18:26:03Z

plugins/inputs/win_perf_counters/win_perf_counters.go

 		if ret == ERROR_SUCCESS {
-			ret = PdhGetFormattedCounterArrayDouble(metric.counterHandle, &bufSize,
-				&bufCount, &emptyBuf[0]) // uses null ptr here according to MSDN.
+			ret = PdhGetFormattedCounterArrayDouble(metric.counterHandle, &bufSize, &bufCount, &emptyBuf[0]) // uses null ptr here according to MSDN.


Revert this line since no change and had better wrapping, we try to wrap at about 80chars.

Roger doger

danielnelson · 2018-04-25T18:26:47Z

plugins/inputs/win_perf_counters/win_perf_counters.go

 			if ret == PDH_MORE_DATA {
 				filledBuf := make([]PDH_FMT_COUNTERVALUE_ITEM_DOUBLE, bufCount*size)
 				if len(filledBuf) == 0 {
 					continue
 				}
-				ret = PdhGetFormattedCounterArrayDouble(metric.counterHandle,
-					&bufSize, &bufCount, &filledBuf[0])
+				ret = PdhGetFormattedCounterArrayDouble(metric.counterHandle, &bufSize, &bufCount, &filledBuf[0])


Revert this line

danielnelson · 2018-04-25T18:31:47Z

plugins/inputs/win_perf_counters/win_perf_counters.go

@@ -90,6 +97,7 @@ type item struct {
 	counter       string
 	instance      string
 	measurement   string
+	nodename      string


Should we call this computer to match the PDH documentation: https://msdn.microsoft.com/en-us/library/windows/desktop/aa373193(v=vs.85).aspx

Sure. Good idea.

danielnelson · 2018-04-25T18:34:50Z

plugins/inputs/win_perf_counters/win_perf_counters.go


 					if instance == "------" {
-						query = "\\" + objectname + "\\" + counter
+						query = "\\\\" + nodename + "\\" + objectname + "\\" + counter


Based on the documentation this doesn't seem like a valid path if there is no computer name, you would get:

\\\Process(*)\% Processor Time

Can you verify if this works?

I can tell you now. The node name is required to be specified. I personally like this, because the measurement gets tagged with the node name, which is now different from the collecting host, but it's not backwards compatible.

Let me check a different idea in which would be backwards compatible.

danielnelson · 2018-04-25T18:47:02Z

plugins/inputs/win_perf_counters/win_perf_counters.go

  [[inputs.win_perf_counters.object]]
-    # Processor usage, alternative to native, reports on a per core.
+	# Processor usage, alternative to native, reports on a per core.


Use 2 space indention in the sample config

Hmmm, I must have used a tab when I meant a space.

danielnelson · 2018-04-26T00:24:30Z

plugins/inputs/win_perf_counters/README.md

@@ -38,6 +38,12 @@ It is recommended NOT to use this on OSes starting with Vista and newer because
 Example for Windows Server 2003, this would be set to true:
 `PreVistaSupport=true`

+#### UseWinTimestamps


Maybe a more descriptive name would be UsePerfCounterTime?

Yup. I like it. Changed

danielnelson · 2018-04-26T00:30:00Z

plugins/inputs/win_perf_counters/pdh.go

+}
+
+func PdhUseWinTimestamps() {
+	pdh_CollectQueryDataWithTime = libpdhDll.MustFindProc("PdhCollectQueryDataWithTime")


Let's do like above on AddEnglishCounterW?

I learn something new everyday. Much cleaner. Thank-you

danielnelson · 2018-04-26T00:45:31Z

plugins/inputs/win_perf_counters/pdh.go

+	pdh_CollectQueryDataWithTime = libpdhDll.MustFindProc("PdhCollectQueryDataWithTime")
+}
+
+func PdhConnectMachine(szMachineName string) uint32 {


I can't seem to find where this is called from, is it used?

No, and I neglected to write a comment header stating this. In experimenting I had added the call, but ended up not requiring it (it was actually orders of magnitude slower doing the connect call and then querying for the counters. I've noted this in a comment header.

Can you instead remove all the calls that are not used?

For sure. I can. I just seems a shame, the call works, and may be useful in the future. Perhaps there is an alternative place to put it? I'll remove it for now. At least it will be in my git history if I need to access it again.

danielnelson · 2018-04-26T00:47:30Z

plugins/inputs/win_perf_counters/pdh.go

+//   }
+//   fmt.Println("Converted bytes: ", n)
+//
+func WideCharToMultiByte(codePage uint32, dwFlags uint32, wchar *uint16, nwchar int32, str *byte, nstr int32) (nwrite int32, err error) {


Where is this called from?

It's not. It was one of those things I used to try and fix the win32 bug without padding the structures. The call itself works good and I thought it might be useful in the future. I've noted this in a comment, but I can remove it as well.

danielnelson · 2018-04-26T00:48:12Z

plugins/inputs/win_perf_counters/pdh.go

 }

 var (
 	// Library
 	libpdhDll *syscall.DLL
+	libkrnDll *syscall.DLL


Can you move all the kernel32 code to a new file: kernel32.go

Good idea. Done

Renamed nodename to computer moved out kernel32 calls to their own library Added tests

…g with "computer" if computer was not specified. (Backwards compatibility)

danielnelson · 2018-04-26T22:36:27Z

plugins/inputs/win_perf_counters/README.md

@@ -50,6 +56,16 @@ beneath the main win_perf_counters entry, `[[inputs.win_perf_counters]]`.

 Following this is 3 required key/value pairs and the three optional parameters and their usage.

+#### Computer
+**Required**


Mark this as optional

- Adding a computer name is optional - Removed unused code - Fixed another few instances of uint32 with [4]byte

srclosson · 2018-04-29T17:37:29Z

Can this be re-reviewed?

danielnelson · 2018-05-01T01:27:28Z

Can you take another pass and remove all code that is no actively being used right now. If we need to bring back in this code we can grab it either from your tree or from https://github.com/lxn/win (who authored most of the Windows code).

Also, it looks like #4036 conflicted with the changes here so we will need to sort that out.

danielnelson · 2018-05-07T18:16:34Z

closes #1899

danielnelson · 2018-05-08T01:34:27Z

closes #2468

park-youngjoon · 2018-05-21T00:21:43Z

Are you in progress now?

vlastahajek · 2018-05-21T10:21:59Z

plugins/inputs/win_perf_counters/pdh.go

 func UTF16PtrToString(s *uint16) string {
 	if s == nil {
 		return ""
 	}
-	return syscall.UTF16ToString((*[1 << 29]uint16)(unsafe.Pointer(s))[0:])
+
+	return syscall.UTF16ToString((*[20]uint16)(unsafe.Pointer(s))[:])


IMHO, this is not right. You have to cast pointer to large enough array, otherwise you could get out of bounds.
There is 100% probability that strings with more than 20 characters go though this function .
Keeping original will not harm 32-bit support, as array size is type int, which is at least 32 bits, so [1<<29] is OK

danielnelson · 2018-05-25T01:55:50Z

@srclosson I believe the 32-bit path is fixed by #4189, but I believe you will need to base the remote computer and counter times fixes on the new code. I'm going to close this pull request since I think it will be easier to open a new one.

vlastahajek · 2018-05-25T09:17:49Z

@danielnelson, #4189 didn't contain fix for 386 issue. It doesn't work in 32bit build, I tested it.
I deliberately skipped 32bit support, to later merge it with this PR.
As #4189 is merged now, I can:

either help @srclosson with merging of current master to 386fix branch
or create separated fix for 32bit support in different PR.

Maybe it would be more convenient to keep 386 support separated from other features (collecting from remote computers and adding timestamp to metrics)

srclosson · 2018-05-25T17:40:42Z

Okay... So what's the status here? @danielnelson would you comment on how it's best to proceed?

danielnelson · 2018-05-25T17:56:26Z

I'm not getting the crash anymore when I compile with GOARCH=386 and run on a 64-bit system, without #4189 it crashes immediately. That said I haven't tested anything more than running --test on the default Windows config.

@srclosson Depends on how much time you have available, I'm hoping to release 1.7.0-rc1 at the end of next week, and it would be great if we could have the 32-bit fix in. If your schedule is full until then it might be best for @vlastahajek to adapt your work so we can get it in for 1.7.

Ideally we would have separate pull requests for 32-bit fix and the new features.

vlastahajek · 2018-05-25T18:22:06Z

@danielnelson: Yes, it's not crashing, at least for several tens minutes, but it writes invalid values for all the stuff, measurement names, fieldnames, fieldvalues.

As discussed in the original issue, there is problem with aligning data within structures with different field types between Go and Wind 32 API.

Crash before occurred fast because there were, IMHO, wrong allocation of the buffer for PDH_FMT_COUNTERVALUE_ITEM_DOUBLE array.

I've prepared separated fix for 386problem, cause I primarily needed to fix my code for 32bit support.
It took me some time to figure out where to place padding in PDH_COUNTER_INFO structure.
It's now ready and tested and all I need to do is create a PR.

@srclosson: Now it depends on you. Did you have a time to look at changes? I did quite a large refactor.

danielnelson · 2018-05-25T22:24:55Z

@vlastahajek Can you go ahead and open a PR for the 32-bit fix?

vlastahajek · 2018-05-28T10:42:15Z

Ok, #4206

vlastahajek · 2018-05-29T09:14:05Z

I merged @srclosson's branch with master. The question is how to push it.

@srclosson, As those features are your work, I would prefer if you could add me as a contributor to your forked telegraf repo and will commit it there. We can then continue with this PR (rename it) and with comments.

My merges is purely melding my refactor and fixes with @srclosson's enhacements, except one change, that I removed the Computer param from example config, because the default value localhost doesn't work on Windows 10. You have to specify local computer name to have it working.

I would also propose a few improvements:

Rename the Computer parameter to RemoteHost, or so.
Allow to monitor multiples hosts - if we change monitoring to remote host, we loose ability to monitor host where telegraf is running easily. And if we have to monitor more such nodes? Mutiple instances of telegraf doesn't seem to be nice solution.

Both should be easy to implement. Multiple remote host can be easily added as stacking performance counters for multiple hosts, although solution using more workers would be prettier.

danielnelson · 2018-06-01T19:47:21Z

@vlastahajek In the meantime if you have time to prepare a branch with your changes that would be nice.

I think the RemoteHost change is a good one, but for multiple hosts doesn't it work with a mix of local and remote hosts?

[[inputs.win_perf_counters.object]]
  RemoteHost = "Foo"
  ObjectName = "Processor"
  Instances = ["*"]
  Counters = ["% Idle Time"]
  Measurement = "win_cpu"

[[inputs.win_perf_counters.object]]
  RemoteHost = "Bar"
  ObjectName = "Processor"
  Instances = ["*"]
  Counters = ["% Idle Time"]
  Measurement = "win_cpu"

[[inputs.win_perf_counters.object]]
  ObjectName = "LogicalDisk"
  Instances = ["*"]
  Counters = ["% Idle Time"]
  Measurement = "win_disk"

DmitryMaletin · 2018-06-01T20:41:37Z

Hi,
I play with forked version of telegraf

My config looks like

[[inputs.win_perf_counters.object]]
Servers = ["$COMPUTERNAME", "RemoteHost","RemoteHost"]
ObjectName = "Processor"
Instances = ["*"]
Counters = ["% Idle Time"]
Measurement = "win_cpu"

So it's work with mixed set of hosts
Generated path looks like
counterPath = "\\" + server + "\" + objectname + "\" + counter

vlastahajek · 2018-06-01T20:45:17Z

@danielnelson, I already have it in branch in my fork: https://github.com/bonitoo-io/telegraf/tree/386fix. It's merged imported original branch of this PR Yes, this should work as a workaround.

DmitryMaletin · 2018-06-01T20:59:29Z

@vlastahajek
If I understood correctly you propose to have a counter per host config.
In my case it's not very convenient cuz I can have a dozen of host to get exactly the same counters.

My current fork: https://github.com/DmitryMaletin/telegraf/blob/dma/plugins/inputs/win_perf_counters/win_perf_counters.go

vlastahajek · 2018-06-01T21:07:31Z

Host per counter object is as it is now.

I would vote the same as you, having global remote host(s) for all counters

danielnelson · 2018-06-01T21:17:44Z

Would plugin level be reasonable? Remember you can always have multiple plugins defined. Obviously not as fine grained as per object but in many cases this will be easier since you don't have to continually redefine the same remote hosts:

[[inputs.win_perf_counters]]
  # If empty, collect only from localhost
  Servers = ["$COMPUTERNAME", "RemoteHost","RemoteHost"]

  [[inputs.win_perf_counters.object]]
    ObjectName = "Processor"
    Instances = ["*"]
    Counters = ["% Idle Time"]
    Measurement = "win_cpu"

DmitryMaletin · 2018-06-01T21:30:48Z

yes. for me it sounds reasonable.

vlastahajek · 2018-06-04T14:55:46Z

So, how to proceed further?

@srclosson, Are going to look at it?

srclosson · 2018-06-04T16:21:44Z

@danielnelson/@vlastahajek, I can certianly look at it. @danielnelson, could we get on the phone for 30 minutes? I've got a lot of stuff to contribute, and I think it would make sense to bring you up to speed on how to best go about merging some of it. There is a lot... Everything from a GPS Nmea plugin, to generic alarm parsing, windows event logs, webdav/sftp data transfer etc... I just don't want to hold up the line and would like to just show you through the ones I've developed, get your feedback, and some ideas on how I could/should go about merging them...

srclosson · 2018-06-04T16:27:00Z

Another idea I had for the remote nodes is to do something like, (pseudocode)

for _, entry := range PerfCounterItems {
wg.Add(1)
go GetAndProcessData(entry)
}
wg.Wait()

This way, all of the requests get sent off at the same time. Probably more performant than doing each one individually...

danielnelson · 2018-06-05T21:31:01Z

Yes, this would be a good thing, ideally network operations are done concurrently with a connection limit and with timeouts. Support for cancellation using a Context is also a nice touch but Telegraf doesn't have a way to do this yet.

TECKCOMINCO\sclosso3 and others added 3 commits April 18, 2018 13:10

Fix: UseWinTimestamps is now configurable

6efde55

Fix: 32 and 64 bit versions split out into separate files

Updated readme.md

77f45ba

danielnelson reviewed Apr 26, 2018

View reviewed changes

srclosson added 5 commits April 26, 2018 02:45

Changes made wrt https://github.com/influxdata/telegraf/pull/4076/files

af43fc0

Renamed nodename to computer moved out kernel32 calls to their own library Added tests

Updated readme from nodename to computer

930ea8e

Oone more documentation update found and fixed

4559ff9

Fix: Moved item nodename declaration to computer. Made sure not to ta…

ae0c16e

…g with "computer" if computer was not specified. (Backwards compatibility)

Revert: Line fix

c271d08

danielnelson reviewed Apr 26, 2018

View reviewed changes

russorat mentioned this pull request Apr 27, 2018

win_perf_counter plugin does not work on 386 #2468

Closed

Update: Feedback from code review

cbd85c7

- Adding a computer name is optional - Removed unused code - Fixed another few instances of uint32 with [4]byte

srclosson closed this Apr 29, 2018

srclosson reopened this Apr 29, 2018

danielnelson added the area/windows Related to windows plugins (win_eventlog, win_perf_counters, win_services) label May 8, 2018

danielnelson mentioned this pull request May 8, 2018

Unable to Collect Metrics on Windows 8.1 #4119

Closed

vlastahajek reviewed May 21, 2018

View reviewed changes

danielnelson closed this May 25, 2018

danielnelson mentioned this pull request Jun 1, 2018

Allow to collect performance counter from remote host(s) #4221

Closed

danielnelson mentioned this pull request Jun 8, 2018

1.7.0-rc1 PerfMon % Processor Time discrepancy #4250

Closed

danielnelson mentioned this pull request Oct 8, 2019

It should be possible to collect WMI indicators remotely from a telegraf. #6489

Closed

386fix #4076

386fix #4076

Conversation

srclosson commented Apr 25, 2018

Required for all PRs:

danielnelson left a 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

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

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

srclosson commented Apr 29, 2018

danielnelson commented May 1, 2018

danielnelson commented May 7, 2018

danielnelson commented May 8, 2018

park-youngjoon commented May 21, 2018

vlastahajek May 21, 2018 • edited Loading

Choose a reason for hiding this comment

danielnelson commented May 25, 2018

vlastahajek commented May 25, 2018 • edited Loading

srclosson commented May 25, 2018

danielnelson commented May 25, 2018

vlastahajek commented May 25, 2018

danielnelson commented May 25, 2018

vlastahajek commented May 28, 2018

vlastahajek commented May 29, 2018 • edited Loading

danielnelson commented Jun 1, 2018

DmitryMaletin commented Jun 1, 2018 • edited Loading

vlastahajek commented Jun 1, 2018 via email • edited Loading

DmitryMaletin commented Jun 1, 2018

vlastahajek commented Jun 1, 2018

danielnelson commented Jun 1, 2018

DmitryMaletin commented Jun 1, 2018

vlastahajek commented Jun 4, 2018

srclosson commented Jun 4, 2018

srclosson commented Jun 4, 2018

danielnelson commented Jun 5, 2018

vlastahajek May 21, 2018 •

edited

Loading

vlastahajek commented May 25, 2018 •

edited

Loading

vlastahajek commented May 29, 2018 •

edited

Loading

DmitryMaletin commented Jun 1, 2018 •

edited

Loading

vlastahajek commented Jun 1, 2018 via email •

edited

Loading