Add an input plugin to monitor basic info of Windows Services #3023

vlastahajek · 2017-07-17T09:21:20Z

Info

Input plugin to report Windows services info: service name, display name, state, startup mode

Tested on Windows Server 2008 R2 Enterprise, Windows 10 Enterprise

It requires Telegraf must run under the administrator privileges

Readme includes sample TICK script

Required for all PRs:

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

…m state and fixed startup config

Measurement name made configurable Error service is indicated with state -1

- snake case names of fields/tag - simplified configuration - state and startup mode as string - better error reporting

- added sample TICK script

This reverts commit 3b4df68.

- Better script alert message

danielnelson · 2017-07-17T22:10:03Z

plugins/inputs/win_services/README.md

+It produces:
+```
+* Plugin: inputs.win_services, Collection 1
+> win_services,host=WIN2008R2H401,display_name=Server,service_name=LanmanServer state="running",startup_mode="auto_start" 15 00040669000000000


There is a stray space in 15 00040669000000000

danielnelson · 2017-07-17T22:10:37Z

plugins/inputs/win_services/README.md

+```
+It produces:
+```
+* Plugin: inputs.win_services, Collection 1


You can remove everything in this section up through this line, so the only items are the example line protocol

Ok, np.
I made it this way cause it's done so in the example plugins you advised me (ngnix, kapacitor).

danielnelson · 2017-07-17T22:12:27Z

plugins/inputs/win_services/win_services.go

+	"github.com/influxdata/telegraf"
+	"github.com/influxdata/telegraf/plugins/inputs"
+	"golang.org/x/sys/windows/svc/mgr"
+    "fmt"


Make sure you are using go fmt or the tests won't pass.

danielnelson · 2017-07-17T22:13:25Z

plugins/inputs/win_services/win_services.go

+  ]
+`
+
+var description = "Input plugin to report Windows services info: service name, display name, state, startup mode"


Don't mention the specific fields, this will be enough: "Input plugin to report Windows services info"

danielnelson · 2017-07-17T22:27:13Z

plugins/inputs/win_services/win_services.go

+            serviceInfos[i].DisplayName = srvCfg.DisplayName
+            serviceInfos[i].StartUpMode = int(srvCfg.StartType)
+        } else {
+            serviceInfos[i].Error = fmt.Errorf("Could not get config of service '%s': %s", srvName, err)


If either Query or Config fail the ServiceInfo could have zero values on some of the fields. This will cause tags/fields to be empty strings which we don't want. I would append the ServiceInfo only if there are no errors.

For error handling this will probably require you to pass the accumulator into this function. Also, you probably only want to report the first error per service.

This relates to the issue above. A weird state value will reported as an error, but for example empty display name is a valid value, as Windows API allows this.

danielnelson · 2017-07-17T22:39:38Z

plugins/inputs/win_services/win_services_test.go

+var KnownServices = []string{"LanmanServer", "TermService"}
+
+func TestList(t *testing.T) {
+	services, err := listServices(KnownServices)


We don't want to add any tests that require special permissions or certain services to running (unless it can also set them up). It would be better to use an interface with a test version.

I'm not sure what is the best goal here.

I can easily mock listServices to test the Gather method, but to properly test listServices function I will need to mock Mgr and Service from mgr package. As there is type dependency, as Mgr.OpenService returns mgr.Service, I can not just create an interface which will be satisfied by existing structs in mgr package and their methods.
I would need to create proxies of both to satisfy new interface in real implementation and this will complicate the code. Did you mean this?
Direct API calls are always harder co mock than RPC calls most current plugins use, but I don't mean I don't want to do it, in fact, it will be fun.. ;),

Cause current tests use services available on every Windows edition. I would rather create a special for test purposes, but that would be more E2E test and that's probably not suitable for such test suite.

Chronograf uses a nice pattern for mocks that can handle this. I wrote an example and added it to this gist https://gist.github.com/danielnelson/79908f0bc7145d3feb7a91e0ef56d88c

That's exactly what I meant.

You have to create on struct (proxy) that satisfies interface and redirect call to real implementation,
a lot boilerplate code .

But if it has to be that way...

Yes, we need to have unit tests. The integration tests can remain but should be guarded with a testing.Short() test.

Sure.
What is the best practise to store unit tests and integration test. I would separate them into two files.

Have win_services_test.go and win_services_integration_test.go.

danielnelson · 2017-07-17T22:43:34Z

plugins/inputs/win_services/win_services.go

+            tags["service_name"] = service.ServiceName
+
+            fields["state"] = ServiceStatesMap[service.State]
+            fields["startup_mode"] = ServiceStartupModeMap[service.StartUpMode]


Need to do something smart if the value returned is not in the map, probably return an error and skip the service. This could be tested earlier on in listServices.

As discussion in the issue leads to using numbers I will use directly what I get from Windows API and report unusual values as error, ok?

danielnelson · 2017-07-17T22:45:39Z

plugins/inputs/win_services/README.md

@@ -0,0 +1,76 @@
+# Telegraf Plugin: win_services
+Input plugin to report Windows services info: service name, display name, state, startup mode


Don't list the tags/fields here, so we won't need to keep this updated if we add anything in the future.

- Fixed few mistakes

- Extended values validation - Reporting first found service error

vlastahajek · 2017-07-19T12:36:55Z

I implemented all changes except changing the testing approach.
I would rather have a short call, or so, on this topic..

danielnelson · 2017-07-19T22:20:17Z

plugins/inputs/win_services/win_services.go

+	if err == nil {
+		state := int(srvStatus.State)
+		if !checkState(state) {
+			serviceInfo.Error = fmt.Errorf("Uknown state of Service %s: %d", serviceName, state)


typo in Uknown -> Unknown

danielnelson · 2017-07-19T22:22:03Z

plugins/inputs/win_services/win_services.go

+	return
+}
+
+//returns true of state is in valid range


This isn't super important to me, but go has the convention of starting the comment with the name of the function. https://blog.golang.org/godoc-documenting-go-code, ex:

// checkState returns true if state is valid

danielnelson · 2017-07-19T22:27:44Z

plugins/inputs/win_services/win_services.go

+			tags := make(map[string]string)
+
+			tags["display_name"] = service.DisplayName
+			tags["service_name"] = service.ServiceName


Tags should not contain the empty string, since InfluxDB will not accept them.

What is the best practice for storing a measurement in case of a tag has empty value?

Skip the tag? Store just the service_name tag and fields?

Insert a place holder text, e.g. "" or "<empty_display_name>"?

Either skip the tag or omit the entire point, depending on if the field needs to be required. For display name I would skip, since it is optional. For service name I would omit the point.

danielnelson · 2017-07-19T22:31:07Z

plugins/inputs/win_services/win_services.go

+	return serviceInfos, nil
+}
+
+func collectServiceInfo(scmgr *mgr.Mgr, serviceName string) (serviceInfo ServiceInfo) {


Recommend returning (ServiceInfo, error) then you can remove the Error field from ServiceInfo.

danielnelson · 2017-07-19T22:33:37Z

plugins/inputs/win_services/win_services.go

+	serviceInfos := make([]ServiceInfo, len(serviceNames))
+
+	for i, srvName := range serviceNames {
+		serviceInfos[i] = collectServiceInfo(scmgr, srvName)


Easiest way to deal with zero value tags/fields IMO is to return an (ServiceInfo, error) and append if err != nil

I would then need to propagate a service error to Gather, or pass Accumulator here.
I want to keep the collecting info separated from plugin API.

The ServiceInfo structure is a domain model of a service, any error occurred during the collecting of service info is a property of such model.

This allows future enhancement in case a user will want to record also service errors into db. Because I still think that if user wants to monitor a particular service and on some hosts it is not possible due to an error, user has to look into the telegraf log instead into the db.

But maybe not, we will see.

danielnelson · 2017-07-19T22:34:59Z

plugins/inputs/win_services/win_services.go

+
+	//While getting service info there could a theoretically a lot of errors on different places.
+	//However in reality if there is a problem with a service then usually openService fails and if it passes, other calls will most probably be ok
+	//So, following error checking is just for sake


Remove this comment, basically applies to almost any error :)

Network, or more generally communication, errors are for example more probable than subsequent errors from Windows Service API. I still think that checking valid ranges of state and startup_mode is not necessary cause if invalid value would be return than the function will return error anyway.

This comment was intended to explain this.

We could get rid of the checkState logic since we are now operating on ints, but we need to check all errors from the external api.

Yes, checking errors of functions is must, no doubts.

danielnelson · 2017-07-19T23:28:53Z

plugins/inputs/win_services/win_services.go

+
+var description = "Input plugin to report Windows services info."
+
+type Win_Services struct {


Rename this WinServices

danielnelson · 2017-07-21T02:53:57Z

plugins/inputs/win_services/win_services_test.go

+var KnownServices = []string{"LanmanServer", "TermService"}
+
+func TestList(t *testing.T) {
+	services, err := listServices(KnownServices)


Chronograf uses a nice pattern for mocks that can handle this. I wrote an example and added it to this gist https://gist.github.com/danielnelson/79908f0bc7145d3feb7a91e0ef56d88c

- better name of main struct - better methods description - removed useless checking - added empty service description checking

…ion tests

vlastahajek · 2017-08-04T21:05:41Z

Hi Daniel,
Thanks again for your review.
I've completed almost all required changes, except those commented.
Please, review, for the last time, hopefully ;)
Thanks,
Vlasta

danielnelson

Can you add this plugin to the test-windows makefile target? You might need to rebase since we have modified it recently.

danielnelson · 2017-08-04T18:37:07Z

plugins/inputs/win_services/win_services_test.go

@@ -0,0 +1,180 @@
+// +build windows
+
+//this test must be run under administrator account


Is this still required?

vlastahajek · 2017-08-07T21:03:23Z

Both done

vlastahajek and others added 28 commits July 10, 2017 15:08

Initial win_services plugin code: a hardcoded service reporting rando…

0db2520

…m state and fixed startup config

Newer golang.org/x/sys for mgr.ListServices

7400e87

Completed basic functionality

1d52da8

Marked with TODO

3b4df68

All services log under same measurement

b67c645

Measurement name made configurable Error service is indicated with state -1

Requirement analysis for windows services

017c131

Fixed typo, better formation to highligh questions

5f93ba1

Adjusted based on feedback from Daniel Nelson

f895ef3

Adjusted based on feedback from Daniel Nelson

570f613

Adjusted based na analysis feedback:

45fb743

- snake case names of fields/tag - simplified configuration - state and startup mode as string - better error reporting

Fix: Fixed error reporting

96af3b7

- proper readme for win_services plugin

37c2952

proof reading

03447bc

small english correction

b05aa4a

using state and startup_mode as fields and display_name as a tag

2bd9a78

Added win_services unit tests

3d4e3db

Added comment explaining condition for running

b88e0e5

Merge branch 'master' into vh-win-services

3d00f89

- Removing prefix 'service_' from state and startup mode enumerations

1ab4cdc

- added sample TICK script

Edited to reflect actual state

b5a5a1f

Removing prefix 'service_' from state and startup mode enumerations

957015c

Revert "Marked with TODO"

b54b8a0

This reverts commit 3b4df68.

- Better script description

3b3e973

- Better script alert message

proof read new section on TICK Scripts

c568b19

- Fixed TICK script

e0a8773

Delete obsolete file

d203d20

Added important note

4a0d3e8

Typo

75407cc

danielnelson suggested changes Jul 17, 2017

View reviewed changes

Reformatted with go fmt

d570894

vlastahajek added 6 commits July 18, 2017 11:31

Reformated with go fmt

3b3106f

- Implemented PR review feedback

721264a

- Fixed few mistakes

Changed state and startup mode fields from string to int

e0dbcab

- Changed state and startup mode fields from string to int

86b989a

- Extended values validation - Reporting first found service error

Changed state and startup mode fields from string to int

a96932c

Merge branch 'master' into vh-win-services

3568a10

danielnelson suggested changes Jul 21, 2017

View reviewed changes

vlastahajek added 11 commits August 1, 2017 10:08

Small improvements based on PR discussion:

5eda565

- better name of main struct - better methods description - removed useless checking - added empty service description checking

Changing existing tests, which test real service manager, to integrat…

1966f42

…ion tests

Added interfaces and real impl wrappers to enable unit testing

ddbdcfc

Added data driven unit tests with mock implementation

8c75562

Fixed getter name

5b71ba8

Merge branch 'master' into vh-win-services

e444c75

Added documentation to types

9085744

Support types made internal

547cb9b

Merge branch 'master' into vh-win-services

9f6ca38

Merged changes from vh-win-services branch from Godeps_windows

86cef94

Proper skipping of tests in short mode

215828e

danielnelson reviewed Aug 7, 2017

View reviewed changes

vlastahajek added 2 commits August 7, 2017 22:59

Unit test don't need admin permission

d307493

Win_services folder added to windows tests

2b1ee6d

danielnelson approved these changes Aug 7, 2017

View reviewed changes

danielnelson merged commit e21f2de into influxdata:master Aug 7, 2017

vlastahajek deleted the vh-win-services branch August 8, 2017 07:15

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

Add Windows Services input plugin (#3023)

09b1f7e

		@@ -0,0 +1,76 @@
		# Telegraf Plugin: win_services
		Input plugin to report Windows services info: service name, display name, state, startup mode


		var description = "Input plugin to report Windows services info."

		type Win_Services struct {

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

		//this test must be run under administrator account

Add an input plugin to monitor basic info of Windows Services #3023

Add an input plugin to monitor basic info of Windows Services #3023

Conversation

vlastahajek commented Jul 17, 2017

Info

Required for all PRs:

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

vlastahajek Jul 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlastahajek Jul 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlastahajek Jul 21, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlastahajek commented Jul 19, 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

vlastahajek Jul 21, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlastahajek commented Aug 4, 2017 • edited

danielnelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlastahajek commented Aug 7, 2017

vlastahajek Jul 18, 2017 •

edited

vlastahajek Jul 18, 2017 •

edited

vlastahajek Jul 21, 2017 •

edited

vlastahajek Jul 21, 2017 •

edited

vlastahajek commented Aug 4, 2017 •

edited