New windows service metricset #5332

martinscholz83 · 2017-10-05T09:44:48Z

As discussed in #5256, i created a new metricset to collect information about windows services.
To do

uptime for services

elasticmachine · 2017-10-05T09:44:50Z

Can one of the admins verify this patch?

martinscholz83 · 2017-10-05T12:39:59Z

@andrewkroh, currently i find no way to open the processes to get the uptime without to run as administrator or give the current user SeDebugPrivileges. Even with PROCESS_QUERY_LIMITED_INFORMATION i can't call OpenProcess. Do you have any other ideas?

andrewkroh

Looks like you are off to a good start. I left comments and will take a closer look after you do some more cleanup and refactoring. Some tests would be helpful too.

andrewkroh · 2017-10-05T15:53:59Z

metricbeat/module/windows/services/run.go

@@ -0,0 +1,61 @@
+// +build ignore


Rather than duplicating this can you move it up a level and share it between both metricsets.

andrewkroh · 2017-10-05T15:55:57Z

metricbeat/module/windows/services/services.go

+func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
+	logp.Warn("EXPERIMENTAL: The windows services metricset is experimental")
+
+	config := struct{}{}


If it's not used it should be removed.

andrewkroh · 2017-10-05T15:57:26Z

metricbeat/module/windows/services/services.go

+	}
+
+	reader, err := NewServiceReader()
+


It seems to be conventional in Go to put the error check on the immediately following line. Please remove the newline separator.

andrewkroh · 2017-10-05T15:59:30Z

metricbeat/module/windows/services/services.go

+// It returns the event which is then forward to the output. In case of an error, a
+// descriptive error must be returned.
+func (m *MetricSet) Fetch() ([]common.MapStr, error) {
+


Please don't start methods with a newline. We try to not do this in Beats (with one exception for functions where the params span multiple lines).

andrewkroh · 2017-10-05T16:00:46Z

metricbeat/module/windows/services/services_windows.go

+	"unicode/utf16"
+	"unsafe"
+
+	"github.com/elastic/beats/libbeat/common"


All github.com/elastic imports should be in the third group (it goes stdlib, third-party, elastic).

andrewkroh · 2017-10-05T16:04:57Z

metricbeat/module/windows/services/services_windows.go

+//sys _CloseServiceHandle(handle uintptr) (err error) = advapi32.CloseServiceHandle
+
+var (
+	sizeOfEnumServiceStatusProcess = (int)(unsafe.Sizeof(EnumServiceStatusProcess{}))


Writing sizeof instead of sizeOf is most common (b/c that's the name of the C operator that generates the value).

andrewkroh · 2017-10-05T16:15:12Z

metricbeat/module/windows/services/services_windows.go

+	return serviceHandle, nil
+}
+
+func getServiceStates(handle ServiceDatabaseHandle, state ServiceEnumState) ([]ServiceStatus, error) {


This function is quite long. I recommend breaking some of the pieces into smaller functions then calling those from this function.

It's definitely better now, but it still scores a 13 by gocyclo (<= 10 is what I would aim for). Can you see if you can do a bit more refactoring to this method. Thanks

andrewkroh · 2017-10-05T16:28:34Z

metricbeat/module/windows/services/services_windows.go

+					//Get uptime for service
+					if ServiceState(serviceTemp.ServiceStatusProcess.DwCurrentState) != ServiceStopped {
+
+						var processCreationTime syscall.Filetime


This is what the system process metricset does so you can reuses the same code. See ProcTime.Get() in elastic/gosigar. They system module has logic for attempting to enable the SeDebugPrivilege. We'll need to figure out how to reuse code and only activate it once. But if the system module is used along side this metricset it should work (assuming the appropriate privs exist).

What about to move the logic from system_windows.go into a helper class ...\metricebeat\helper\windows.go?

andrewkroh · 2017-10-05T16:29:50Z

metricbeat/module/windows/services/services_windows.go

+							return nil, err
+						}
+
+						if err := syscall.OpenProcessToken(currentProcess, syscall.TOKEN_ADJUST_PRIVILEGES, &token); err != nil {


This probably shouldn't hard fail on a permission issue. I think it would be better to gracefully degrade such that the uptime info isn't available.

andrewkroh

Nice progress.

What about to move the logic from system_windows.go into a helper class ...\metricebeat\helper\windows.go?

Yeah, but how about privileges_windows.go. I think the helper function should have some kind of logic such that only the first call does any real work. This way if both the system module and the windows-services metricset are used that this code only runs once.

There's a metricbeat/debug file in your PR that looks to have been accidentally added. Can you please remove that.

andrewkroh · 2017-10-23T12:52:25Z

metricbeat/module/windows/services/services_windows.go

+
+var (
+	sizeofEnumServiceStatusProcess = (int)(unsafe.Sizeof(EnumServiceStatusProcess{}))
+	sizeofQueryServiceConfig       = (int)(unsafe.Sizeof(QueryServiceConfig{}))


You are right. I delete this one

andrewkroh · 2017-10-23T12:53:00Z

metricbeat/module/windows/services/services_windows.go

+	sizeofQueryServiceConfig       = (int)(unsafe.Sizeof(QueryServiceConfig{}))
+)
+
+type enumServiceStatusProcess struct {


andrewkroh · 2017-10-23T12:58:01Z

metricbeat/module/windows/services/services_windows.go

+	return nil
+}
+
+func getServiceUptime(processId uint32) (gosigar.ProcTime, error) {


I recommend encapsulating the logic for the uptime calculation into this helper function. I would change the return params to be (time.Duration, error).

~~You mean privileges_windows.go? Or should we create another helper function.~~
Sorry. I misunderstood you 🙈

andrewkroh · 2017-10-23T13:02:53Z

metricbeat/module/windows/services/services_windows.go

+	return serviceHandle, nil
+}
+
+func getServiceStates(handle ServiceDatabaseHandle, state ServiceEnumState) ([]ServiceStatus, error) {


It's definitely better now, but it still scores a 13 by gocyclo (<= 10 is what I would aim for). Can you see if you can do a bit more refactoring to this method. Thanks

andrewkroh · 2017-10-23T13:13:47Z

metricbeat/module/windows/services/services_windows.go

+			"service_name": service.ServiceName,
+			"state":        service.CurrentState,
+			"start_type":   service.StartType,
+			"uptime":       service.Uptime,


What are the units for this? I would embed them into the field name like uptime.ms. (This needs to be a nested object rather than a field name containing a dot.)

andrewkroh · 2017-10-23T13:17:56Z

metricbeat/module/windows/services/_meta/fields.yml

+  description: >
+    `services` contains the status for windows services.
+  fields:
+    - name: status


Can you please update this to include all of the fields being sent.

For an example of how to use a field formatter for the uptime field see ./metricbeat/module/system/uptime/_meta/fields.yml. This will cause the uptime field to be rendered nicely in Kibana (like "2 days").

andrewkroh · 2017-10-23T13:19:57Z

metricbeat/module/windows/services/_meta/data.json

@@ -0,0 +1,19 @@
+{


Can you add a TestData function (example) to the package that will generate this file when you run go test -data . from within this package's directory.

martinscholz83 · 2017-10-24T07:37:51Z

I think the helper function should have some kind of logic such that only the first call does any real work.

You mean some simple bool if the check has already run?

andrewkroh

Looks good. I think it needs only a few more things

make update - To regenerate the docs.
make fmt - To fix the imports. Then you probably need to manually fix things to ensure they are grouped as stdlib, third-party, then elastic.

andrewkroh · 2017-10-24T13:38:09Z

metricbeat/helper/privileges_windows.go

+
+// CheckAndEnableSeDebugPrivilege checks if the process's token has the
+// SeDebugPrivilege and enables it if it is disabled.
+func CheckAndEnableSeDebugPrivilege() error {


You mean some simple bool if the check has already run?

I'm thinking this should be checkAndEnableSeDebugPrivilege() and a separate method named CheckAndEnableSeDebugPrivilege exists that calls checkAndEnableSeDebugPrivilege() only once by using sync.Once.

This way only the system module or the windows module will do the initialization, but not both.

andrewkroh · 2017-10-24T13:45:24Z

metricbeat/module/windows/services/services_windows.go

+	}
+
+	tm := time.Now().UnixNano()
+	uptime := time.Duration((tm / int64(time.Millisecond)) - int64(processCreationTime.StartTime))


I'm not sure if this is any more clear, but there is the time.Since() function for the purpose of calculating elapsed times.

uptime := time.Since(time.Unix(0, int64(processCreationTime.StartTime) * int64(time.Millisecond)))

andrewkroh · 2017-10-24T13:46:49Z

metricbeat/module/windows/services/services_windows.go

+}
+
+func NewServiceReader() (*ServiceReader, error) {
+


Remove newline from beginning of method. (BTW I built a tool to find these called nonewlines.)

andrewkroh · 2017-10-24T13:47:03Z

metricbeat/module/windows/services/services_windows.go

+
+	//var state ServiceEnumState
+
+	// configState := strings.ToLower(config.State)


Can you please remove the commented out code.

martinscholz83 · 2017-10-25T07:23:01Z

In commit make fmt i removed in line 111 the -local switch from .../libbeat/scripts/Makefile because goimport doesn't know this switch. Can you confirm this?!

andrewkroh · 2017-10-25T10:39:41Z

goimports has a -local flags. Try updating go get -u golang.org/x/tools/cmd/goimports.

andrewkroh

Looks good. Almost there.

I recommend trying gometalinter. It helps me catch things in my code.

go get -u github.com/alecthomas/gometalinter
gometalinter --install
cd to/your/package/directory
gometalinter --deadline=30s --disable=gotype

andrewkroh · 2017-10-25T10:45:41Z

metricbeat/module/windows/services/services_windows.go

+		return time.Duration(processCreationTime.StartTime), err
+	}
+
+	uptime := time.Since(time.Unix(0, int64(processCreationTime.StartTime)*int64(time.Millisecond))) / time.Millisecond


Don't divide by time.Milliseconds. Leave the value stored as nanoseconds because this is what a time.Duration value is defined as. Do the conversion to ms later when you need to write the value to an event.

andrewkroh · 2017-10-25T10:49:11Z

metricbeat/helper/privileges_windows.go

+	return nil
+}
+
+func CheckAndEnableSeDebugPrivilege() (err error) {


We generally try to avoid named result parameters. https://github.com/golang/go/wiki/CodeReviewComments#named-result-parameters

andrewkroh · 2017-10-25T10:50:48Z

metricbeat/helper/privileges_windows.go

+	return err
+}
+
+// CheckAndEnableSeDebugPrivilege checks if the process's token has the


The function name should be lower-case now.

andrewkroh · 2017-10-25T11:00:12Z

metricbeat/module/windows/services/_meta/fields.yml

+  description: >
+    `services` contains the status for windows services.
+  fields:
+    - name: uptime


This can be simplified if you use a dotted key name. The tooling with expand it for the purposes of the index template.

fields: - name: uptime.ms type: long format: duration input_format: milliseconds description: > The service uptime in milliseconds. - name: service_name

andrewkroh · 2017-10-25T11:04:31Z

metricbeat/module/windows/services/services_windows.go

+			"service_name": service.ServiceName,
+			"state":        service.CurrentState,
+			"start_type":   service.StartType,
+			"uptime": map[string]interface{}{


If service.Uptime is 0 then let's not add it to the event. It should only be zero if an error occurred which would mean the data is invalid, right?

Or the service is stopped. ~~So maybe a check for 0 should help.~~

andrewkroh · 2017-10-25T11:14:43Z

metricbeat/module/windows/services/_meta/fields.yml

+    - name: display_name
+      type: keyword
+      description: >
+        The display name of the service.


For the fields that are enums it would be nice to document what the possible values are. I would simply add a sentence that says "The possible values are x, y, and z".

Some special format for the values like x, y and z?

If you like 👍

andrewkroh · 2017-10-25T11:58:59Z

metricbeat/module/windows/services/_meta/fields.yml

@@ -29,8 +24,11 @@
      type: keyword
      description: >
        The start type of the service.
+        The possible values are `ServiceAutoStart`, `ServiceBootStart`, `ServiceDemandStart`, `ServiceDisabled` and `ServiceSystemStart`.


I believe we adhere to the "Oxford comma" rule in our documentation. As such there should be a comma before the "and". Same for the other lists.

andrewkroh · 2017-10-25T12:22:48Z

metricbeat/module/windows/services/services_windows.go

+		}
+
+		if service.Uptime > 0 {
+			ev.Put("uptime", map[string]interface{}{"ms": service.Uptime})


Use ev.Put("uptime.ms", service.Uptime). It will automatically do the right thing.

andrewkroh · 2017-10-25T12:31:12Z

metricbeat/module/windows/services/_meta/data.json

+  "windows": {
+    "services": {
+      "uptime": {
+        "ms": 0


I assume this needs to be updated now that 0 values are filtered.

andrewkroh · 2017-10-25T12:56:31Z

There are some formatting issues that Travis CI is detecting. See https://travis-ci.org/elastic/beats/jobs/292592948#L590-L593

Also can you please squash your commits. Then rebase it on master (make sure your master is up-to-date with the remote first). There were some changes in master that are missing from your branch to allow Jenkins CI to run. Let me know if you need help with the git stuff.

martinscholz83 · 2017-10-26T13:16:31Z

These formatting errors are really frustating. I have run gometalinter and goimports over these files. @andrewkroh, can you take a look what's wrong. Thank you!

andrewkroh · 2017-10-26T13:19:20Z

Use make fmt then view the git diff locally. After you commit the changes do one last make check.

martinscholz83 · 2017-10-27T07:05:59Z

I've figured out why goimports was not working. I had a ubuntu package installed golang-golang-x-tools 😒

andrewkroh · 2017-10-27T08:46:27Z

jenkins, test it

martinscholz83 · 2017-10-27T09:24:27Z

@andrewkroh, before you merge. One problem i actually run into is the part here. Sometimes i get an error from sys.UTF16BytesToUTF8Bytes input buffer must have an even length. This is different from machine to machine. That's why i have to do lastOffset = uintptr(len(servicesBuffer)) - 1 to make it even. Do you have any ideas. From understadnig i think its the same as in pdh_windows.go that the service name are appended to the end of the buffer.

andrewkroh

Sometimes i get an error from sys.UTF16BytesToUTF8Bytes input buffer must have an even length

It looks like Windows appends a single byte 0x00 to ensure the buffer is null-terminated. This is why the overall buffer length is odd. I made some changes in andrewkroh@7ea1e2f. Can you pull that commit into this branch or apply the change yourself.

andrewkroh · 2017-10-27T12:46:24Z

metricbeat/docs/fields.asciidoc

+
+
+[float]
+=== `windows.services.service_name`


As I read through these keys I am thinking we should call this the windows service metricset (not plural). Similar to how the system process metricset is not plural. Then the field names here make more sense.

Additionally, if it is renamed then I would change windows.services.service_name to windows.service.name to removing the stuttering.

Made change to account for null-terminators that are 0x00 when we were expecting 0x0000. Fix misspelling in ServiceRunning

andrewkroh

LGTM. Can you please fix the one typo I just found then we can merge this. 🎉

andrewkroh · 2017-11-02T12:30:30Z

metricbeat/module/windows/service/_meta/fields.yml

+      type: keyword
+      description: >
+        The actual state of the service.
+        The possible values are `ServiceContinuePending`, `ServicePausePending`, `ServicePaused`, `ServiceRuning`, `ServiceStartPending`,


s/ServiceRuning/ServiceRunning/

andrewkroh · 2017-11-02T12:43:08Z

You'll need a make update to rebuild the docs from fields.yml.

martinscholz83 · 2017-11-02T12:44:09Z

Jap. Switch to my linux machine

andrewkroh · 2017-11-02T13:16:43Z

jenkins, test it

andrewkroh · 2017-11-02T14:16:35Z

@maddin2016 Thanks for another great Windows monitoring contribution!

martinscholz83 · 2017-11-02T14:27:47Z

Anytime! 👍

andrewkroh reviewed Oct 5, 2017

View reviewed changes

andrewkroh added enhancement Metricbeat Metricbeat labels Oct 5, 2017

andrewkroh reviewed Oct 23, 2017

View reviewed changes

andrewkroh reviewed Oct 24, 2017

View reviewed changes

andrewkroh reviewed Oct 25, 2017

View reviewed changes

martinscholz83 added 14 commits October 26, 2017 09:36

Create new metricset

1ef849c

Add Close function for handler

d414d86

Add description

b9878d7

make update

d7d0aa1

Add service start types

a5bd839

Add service access rights

65ea2f7

Add OpenService function

a0c7f13

Add more funtions

d5be9ed

Add errors

8fea616

Query service config

6fd5d02

Correct return type for QueryServiceConfig

eb3ecd2

Remove services config

73ef011

Add process access rights

3fe19b7

Add process sycall functions

51f64db

Fix formatting

4ff7381

make fmt

12b78b2

martinscholz83 added 2 commits October 27, 2017 09:11

make update

55751ae

Build only on windows

4eb1120

Add integration test

99fbfd1

andrewkroh reviewed Oct 27, 2017

View reviewed changes

andrewkroh and others added 7 commits November 2, 2017 08:28

Refactor getServiceStates

563f487

Made change to account for null-terminators that are 0x00 when we were expecting 0x0000. Fix misspelling in ServiceRunning

Fix const Running

9d83f1e

Rename service metricset

aecd50b

Rename json output

e9409e8

make update

17ac114

go generate

cb4cf55

go test -data

e743b68

andrewkroh approved these changes Nov 2, 2017

View reviewed changes

Fix typo

5ca5ebe

make update

b98d69b

andrewkroh merged commit 7b4dd0d into elastic:master Nov 2, 2017

monicasarbu mentioned this pull request Dec 11, 2017

Close changelog for 6.1.0 release #5853

Merged


		//var state ServiceEnumState

		// configState := strings.ToLower(config.State)

New windows service metricset #5332

New windows service metricset #5332

Conversation

martinscholz83 commented Oct 5, 2017

elasticmachine commented Oct 5, 2017

martinscholz83 commented Oct 5, 2017

andrewkroh 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

andrewkroh Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

andrewkroh 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

martinscholz83 Oct 24, 2017 • edited Loading

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

martinscholz83 commented Oct 24, 2017

andrewkroh left a comment

Choose a reason for hiding this comment

andrewkroh Oct 24, 2017 • edited Loading

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

martinscholz83 commented Oct 25, 2017

andrewkroh commented Oct 25, 2017

andrewkroh 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

martinscholz83 Oct 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinscholz83 Oct 25, 2017 • edited Loading

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

andrewkroh commented Oct 25, 2017 • edited Loading

martinscholz83 commented Oct 26, 2017

andrewkroh commented Oct 26, 2017 • edited Loading

martinscholz83 commented Oct 27, 2017

andrewkroh commented Oct 27, 2017

martinscholz83 commented Oct 27, 2017 • edited Loading

andrewkroh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh commented Nov 2, 2017

martinscholz83 commented Nov 2, 2017

andrewkroh commented Nov 2, 2017

andrewkroh commented Nov 2, 2017

martinscholz83 commented Nov 2, 2017

andrewkroh Oct 5, 2017 •

edited

Loading

martinscholz83 Oct 24, 2017 •

edited

Loading

andrewkroh Oct 24, 2017 •

edited

Loading

martinscholz83 Oct 25, 2017 •

edited

Loading

martinscholz83 Oct 25, 2017 •

edited

Loading

andrewkroh commented Oct 25, 2017 •

edited

Loading

andrewkroh commented Oct 26, 2017 •

edited

Loading

martinscholz83 commented Oct 27, 2017 •

edited

Loading