procstats patterns without pgrep #3559

vrecan · 2017-12-08T20:46:24Z

I have pulled in a new dep for dealing with cross platform pattern matching for processes. I have validated everything on linux and I am going through some steps to validate it all on windows. Let me know if I did something incorrectly when I added the dependencies.

This fixes #3558

This is my first PR for telegraf. I have signed the CLA.

Required for all PRs:

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

…atterns_no_pgrep Conflicts: Godeps

…into patterns_no_pgrep

vrecan · 2017-12-15T05:36:42Z

I have reverted my change to gopsutil and just implemented the WMI queries here to make it work. WMI is the only way that I know to do this without using CGO. Let me know if there is another way you would like me to implement this or if there is anything you would like me to change!

I've also updated the readme with a section for using it in windows.

Thanks!

danielnelson · 2017-12-15T20:01:52Z

I think we shoud create another PIDFinder, we can call it NativeFinder. We can implement the interface using regular gopsutil commands but for name/cmdline we can have it use the specialized WMI calls on Windows. Then in the procstat config we can introduce an option: finder = "pgrep" or finder = "native". Then we can move the regex matching into the NativeFinder.

There are a few spots in the code where there are small unneeded changes, can you also clean these up so that we only modify lines related to the patchset.

vrecan · 2017-12-20T21:04:32Z

OK so you want me to create a new finder that fully implements everything for all platforms using gopsutil only? And then in that version it will use the special WMI calls for windows?

Just trying to make sure I understand what you want me to do before I implement it. I am open to implementing it however you want :)

vrecan · 2017-12-20T21:17:11Z

There are a few things that are different for windows then unix. Windows has no pid files and it requires another WMI call to get processes by owner, it's a name though and not a UID.

danielnelson · 2017-12-22T01:42:48Z

For the pid files, even though no one really uses them in Windows, maybe we can just use the same implementation. Then someone could use it if they wanted for some reason and we don't have to explain that this option doesn't work.

danielnelson · 2017-12-22T02:55:21Z

A little more clarification, a user should be able to use either the native or pgrep finder on any plaform. We should have the default implementation use gopsutil, but we can have a windows specialization that uses the faster wmi calls as needed.

vrecan · 2017-12-22T18:06:56Z

sounds good, thanks I will start working on that

vrecan · 2017-12-23T00:00:26Z

@danielnelson do you mind looking at my current implementation. Does this look like what you want? If so I am going to add some more tests and update the readme

resource intesive

danielnelson · 2017-12-28T18:40:54Z

plugins/inputs/procstat/nativeFinder.go

+		user, err := p.Username()
+		if err != nil {
+			//skip, this can happen if we don't have permissions or
+			//the pid no longer exists


Needs continue?

danielnelson · 2017-12-28T19:05:03Z

plugins/inputs/procstat/nativeFinder_unix.go

@@ -0,0 +1,59 @@
+// +build !windows


Call this file native_finder_notwindows.go

danielnelson · 2017-12-28T19:05:30Z

plugins/inputs/procstat/nativeFinder_windows.go

@@ -0,0 +1,109 @@
+// +build windows


Call this file native_finder_windows.go

danielnelson · 2017-12-28T19:07:23Z

plugins/inputs/procstat/nativeFinder_windows.go

+var Timeout = 5 * time.Second
+
+func init() {
+	wmi.DefaultClient.AllowMissingFields = true


We should try to arrange for a new wmi.Client to be created and used once for each plugin. This will avoid needing to modify the default client which might have unwanted effects on other pieces of code due to it being global.

should I have a seperate native finder struct for windows so that I can store it there?

I was able to remove this and my queries still work. I took this from the gopsutil implementation and it looks like it's not really needed for our queries.

danielnelson · 2017-12-28T19:08:37Z

plugins/inputs/procstat/nativeFinder_windows.go

+// }
+
+//Pattern matches on the process name
+func (pg *NativeFinder) Pattern(pattern string) ([]PID, error) {


I think this looks the same as the notwindows version, lets put shared implementation in a native_finder.go file.

danielnelson · 2017-12-28T19:10:55Z

plugins/inputs/procstat/nativeFinder_windows.go

+func getWin32ProcsByVariable(variable string, qType queryType, value string, timeout time.Duration) ([]process.Win32_Process, error) {
+	var dst []process.Win32_Process
+	var query string
+	query = fmt.Sprint("WHERE ", variable, " ", qType, " \"", value, "\"")


Use fmt.Sprintf for readability. I'm nervous about escaping here, variable and value need to be escaped more rigorously.

I changed to sprintf, how would you like me to protect against escaping? based on this I should be able to just escape backslash and single quote for the value. On the variable I can just fail anytime any of the non standard characters are in it. What do you think?

In this case you can probably use the %q pattern, it will escape double quote and backslash using backslash, and it will add the outer quotation marks:

"WHERE %s %s %q"

danielnelson · 2017-12-28T19:14:36Z

plugins/inputs/procstat/procstat.go

+  ## pgrep tries to exec pgrep
+  ## native will work on all platforms, unix systems will use regexp. 
+  ## Windows will use WMI calls with like queries
+  pid_finder = native


Needs quotes: pid_finder = "native"

danielnelson · 2017-12-28T19:15:07Z

plugins/inputs/procstat/procstat.go

+		fmt.Println("PIDFINDER: ", p.PidFinder)
+		switch p.PidFinder {
+		case "native":
+			fmt.Println("pid finder is now native")


Don't forget to cleanup debugging statements

danielnelson · 2017-12-28T19:16:26Z

plugins/inputs/procstat/procstat_test.go

@@ -1,3 +1,5 @@
+// +build !windows


Need to remove this build flag

when I remove the build tag I get this error on windows:

--- FAIL: TestGather_cgroupPIDs (0.00s) Error Trace: procstat_test.go:363 Error: Received unexpected error: open \sys\fs\cgroup\C:\Users\Vrecan\AppData\Local\Temp\565977507\cgroup.procs: The filename, directory name, or volume label syntax is incorrect.

I addressed this by skipping the test in windows

Not sure if it will be enough, but we should use filepath.Join here: https://github.com/influxdata/telegraf/blob/master/plugins/inputs/procstat/procstat.go#L336

danielnelson · 2017-12-28T19:17:29Z

plugins/inputs/procstat/README.md

@@ -27,8 +27,28 @@ Additionally the plugin will tag processes by their PID (pid_tag = true in the c
 * pid
 * process_name

+
+### Windows


We will have to remember to take a second pass at this documentation, so that it matches the current design

vrecan · 2018-01-12T00:36:14Z

thanks for the comments, I am going to take a look at this today or tomorrow and update the PR

danielnelson · 2018-01-12T20:32:27Z

Will need to rebase to fix the test error in cratedb

…patterns_no_pgrep

vrecan · 2018-01-24T21:22:00Z

I pulled in master and I am still getting test failures. This time in leofs. What should I do to get this resolved and merged? What kind of documentation are you looking for?

Thanks for your help!

danielnelson

Tests were just an intermittent failure, sorry about that. We also still need to rename the camel case files like nativeFinder.go to native_finder.go

danielnelson · 2018-01-29T22:35:21Z

plugins/inputs/procstat/procstat_test.go

@@ -349,6 +350,10 @@ func TestGather_systemdUnitPIDs(t *testing.T) {
 }

 func TestGather_cgroupPIDs(t *testing.T) {
+	//no cgroups in windows
+	if runtime.GOOS == "windows" {
+		t.SkipNow()


You can put the comment in here like: .Skip("no cgroups in windows")

danielnelson · 2018-01-29T22:46:04Z

plugins/inputs/procstat/nativeFinder_windows.go

+	var dst []process.Win32_Process
+	var query string
+	// should look like "WHERE CommandLine LIKE "procstat"
+	query = fmt.Sprintf("WHERE %s %s '%s'", variable, qType, value)


Use %q here to get quoting with escaping of value. "WHERE %s %s %q"

danielnelson · 2018-01-29T22:48:26Z

plugins/inputs/procstat/nativeFinder.go

+}
+
+//getPidsByUser ...
+func getPidsByUser(username string) ([]PID, error) {


Just merge this into func (pg *NativeFinder) Uid(user string) ([]PID, error) since it doesn't add any new abstraction.

…patterns_no_pgrep

vrecan · 2018-01-30T22:45:49Z

@danielnelson I believe I have addressed all of your comments.

danielnelson · 2018-02-02T00:14:22Z

@vrecan Merged in for 1.6, thanks. I also did some updates to the procstat README so if you have a chance could you take a look and make sure it is correct?

vrecan · 2018-02-02T17:28:17Z

Thanks for merging this, I will look at the docs now.

* master: (59 commits) Added additional SQL Server performance counters (influxdata#3770) Update changelog Fix ping plugin not reporting zero durations (influxdata#3778) Adjust time of nightly build Update changelog Add TLS support to the mesos input plugin (influxdata#3769) Install new requirements for fpm gem install Update changelog Add additional metrics and reverse metric names option to openldap (influxdata#3722) Update paho mqtt to latest release Update changelog Remove userinfo from url tag in prometheus input (influxdata#3743) Update sample config in contributing docs Run nightly build sequentially Fix Makefile on Windows and use in AppVeyor build (influxdata#3748) Fix example source_override values in wavefront output (influxdata#3744) Update gitignore Update changelog Improve procstat readme Add native Go method for finding pids to procstat (influxdata#3559) ...

Ben Aldrich and others added 4 commits December 8, 2017 20:31

remove pgrep for pattern matches, use go-ps instead

9d4c282

add dep

a54fd8d

remove debug

c47eaef

working implementation using the new gopsutil

a2f429b

vrecan force-pushed the patterns_no_pgrep branch from c47eaef to a2f429b Compare December 10, 2017 02:25

vrecan and others added 11 commits December 9, 2017 19:26

windows files and build changes for reg pgrep

4b26b52

update the make file to test the new windows tests

8b66be9

update gopsutil with new wmi queries

54ffbd2

Merge branch 'patterns_no_pgrep' of github.com:vrecan/telegraf into p…

62285d9

…atterns_no_pgrep Conflicts: Godeps

add docs

0722c4a

doc cleanup

aff075b

move things around in the doc

e402310

more cleanup on the docs

d85d89b

make doc a little more clear

56a0296

revert godeps to old version of gopsutil and implement it directly

452f496

Merge branch 'patterns_no_pgrep' of https://github.com/vrecan/telegraf …

e1e32ef

…into patterns_no_pgrep

remove extra dep

93fc937

revert change to pgrep

e2f374c

vrecan added 4 commits December 22, 2017 15:45

windows version of nativeFinder is working

37c6416

begin unix native finder

e97d339

update comments

ed23fe3

fully working end to end

8f008d3

use native Name function instead, much better performance and less

aab50e7

resource intesive

danielnelson reviewed Dec 28, 2017

View reviewed changes

vrecan added 5 commits January 11, 2018 20:23

address comments

78d4544

add continue

50fd95a

change cgroup test to skip on windows

690ef25

move to single quotes

0ecb670

add missing file

f115e18

Merge branch 'master' of https://github.com/influxdata/telegraf into …

797ec89

…patterns_no_pgrep

danielnelson reviewed Jan 29, 2018

View reviewed changes

vrecan added 6 commits January 30, 2018 12:44

Merge branch 'master' of https://github.com/influxdata/telegraf into …

fc0f21c

…patterns_no_pgrep

address comments, rename files and general cleanup

5a4c56e

update name for not_windows

80c07ae

add missing file

20470f8

rename again to see if that fixes the test

d75e3db

Merge branch 'master' of https://github.com/influxdata/telegraf into …

529fd53

…patterns_no_pgrep

danielnelson added this to the 1.6.0 milestone Feb 1, 2018

danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Feb 1, 2018

danielnelson merged commit a7571d5 into influxdata:master Feb 1, 2018

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

Add native Go method for finding pids to procstat (#3559)

551c771

procstats patterns without pgrep #3559

procstats patterns without pgrep #3559

Conversation

vrecan commented Dec 8, 2017 • edited

Required for all PRs:

vrecan commented Dec 15, 2017 • edited

danielnelson commented Dec 15, 2017

vrecan commented Dec 20, 2017

vrecan commented Dec 20, 2017

danielnelson commented Dec 22, 2017

danielnelson commented Dec 22, 2017

vrecan commented Dec 22, 2017

vrecan commented Dec 23, 2017

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

vrecan Jan 12, 2018 • edited

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

vrecan commented Jan 12, 2018

danielnelson commented Jan 12, 2018

vrecan commented Jan 24, 2018 • edited

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

vrecan commented Jan 30, 2018

danielnelson commented Feb 2, 2018

vrecan commented Feb 2, 2018

vrecan commented Dec 8, 2017 •

edited

vrecan commented Dec 15, 2017 •

edited

vrecan Jan 12, 2018 •

edited

vrecan commented Jan 24, 2018 •

edited