Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New module windows for metricbeat #3758

Merged
merged 18 commits into from Mar 28, 2017

Conversation

@martinscholz83
Copy link
Contributor

commented Mar 16, 2017

This PR implements a new module called windows for metricbeat. The first metricset within this module is perfmonbeat which is able to collect data from windows performance counters.

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Mar 16, 2017

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

1 similar comment
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Mar 16, 2017

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

@martinscholz83

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2017

Is there some flag which says this module is only for windows?

@martinscholz83 martinscholz83 force-pushed the martinscholz83:module-perfmonbeat branch from c610ab3 to 331a56f Mar 16, 2017

@andrewkroh andrewkroh assigned ruflin and andrewkroh and unassigned ruflin and andrewkroh Mar 16, 2017

@andrewkroh andrewkroh requested review from ruflin and andrewkroh Mar 16, 2017

@andrewkroh
Copy link
Member

left a comment

Thanks for opening up a PR for this. 👍

I left some minor feedback about error handling. After you address that I'll probably take the code for a test drive.

Could you please add some tests (like a pdh_windows_test.go file) that exercises the code by querying some counters. And maybe a negative test case where a counter does not exist.

It looks like you accidentally committed a copy of libbeat/dashboards/import_dashboards. Please remove that.

@@ -0,0 +1,83 @@
package perfmon

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 16, 2017

Member

You can use // +build windows to make files like this compile only for Windows. Or you can rename them to include a _windows.go suffix. This behavior is described here: https://golang.org/pkg/go/build/

}

for _, v := range config.CounterConfig {
if len(v.Name) <= 0 {

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 16, 2017

Member

ucfg can apply this sort of validation automatically if you annotate a the fields with validation info. (see docs) For example,

type CounterConfig struct {
    Name  string               `config:"group" validate:"required"`
    Group []CounterConfigGroup `config:"collectors" validate:"required"`
}

if err != 0 {
logp.Err("%v", err)
return nil, errors.New("Initialization fails with error: " + strconv.Itoa(err))

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 16, 2017

Member

It's not a best practice to log errors and return them. I would just return the error and let the caller decide if it is supposed to be logged or not. Please review the whole PR and make the appropriate changes.


//go:generate go run $GOROOT/src/syscall/mksyscall_windows.go -output pdh_windows.go pdh.go
// Windows API calls
//sys _PdhOpenQuery(dataSource uintptr, userData uintptr, query *uintptr) (err int) = pdh.PdhOpenQuery

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 16, 2017

Member

I recommend changing the method signature of all these functions to return (err error). The newly generated code will then check the int return value and return the appropriate error. This make all the code that uses theses function conform to the standard Go conventions.

This comment has been minimized.

Copy link
@martinscholz83

martinscholz83 Mar 17, 2017

Author Contributor

I have change the signature to error. Now it generate this function

func _PdhOpenQuery(dataSource uintptr, userData uintptr, query *uintptr) (err error) {
	r1, _, e1 := syscall.Syscall(procPdhOpenQuery.Addr(), 3, uintptr(dataSource), uintptr(userData), uintptr(unsafe.Pointer(query)))
	if r1 == 0 {
		if e1 != 0 {
			err = error(e1)
		} else {
			err = syscall.EINVAL
		}
	}
	return
}

Now i always get Invalid argument error. How should i handle this error. Is it not better to return nil?

This comment has been minimized.

Copy link
@martinscholz83

martinscholz83 Mar 20, 2017

Author Contributor

Done


for _, v := range config.CounterConfig {
if len(v.Name) <= 0 {
err := errors.New("Group cannot be empty")

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 16, 2017

Member

Error strings should not be capitalized. explanation

- group: "processor"
collectors:
- alias: "processor_perfomance"
query: "\\Processor Information(_Total)\\% Processor Performance"

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Mar 16, 2017

Member

If you use single quotes here, can the double backslashes be replaced with a single backslash? I would prefer that, unless of course using double backslashes is common in the perfmon world.

metricsets: ["perfmon"]
enabled: true
period: 1s
counters:

This comment has been minimized.

Copy link
@ruflin

ruflin Mar 17, 2017

Collaborator

For metricset specific configs we (recently) created the convention to have it under a namespace. Means here it would be perfmon.counters:

This comment has been minimized.

Copy link
@martinscholz83

martinscholz83 Mar 17, 2017

Author Contributor

In source i then have to write this config:"perfmon.counters" validate:"required"?

This comment has been minimized.

Copy link
@ruflin

ruflin Mar 17, 2017

Collaborator

yes

@ruflin
Copy link
Collaborator

left a comment

Thanks a lot for taking this on. Having platform specific modules is something new in metricbeat.

I'm not too happy about the metricset name yet, but I couldn't come up with an alternative name yet.

Any chance you could share some example outputs for the example config you created. I'm curious to see how the events will look.

I left you a few comments which are mostly related to the assumption, that this is a dynamic metricset.

// descriptive error must be returned.
func (m *MetricSet) Fetch() (common.MapStr, error) {

data, err := m.handle.readData()

This comment has been minimized.

Copy link
@ruflin

ruflin Mar 17, 2017

Collaborator

As far as I understand, this is a "dynamic" metricset. If yes, this would need also a namespace. See https://github.com/elastic/beats/blob/master/metricbeat/module/jolokia/jmx/jmx.go#L93 for an example.

This comment has been minimized.

Copy link
@martinscholz83

martinscholz83 Mar 19, 2017

Author Contributor

With dynamic metricset i suppose you mean that there can be multiple groups of counters, right?

This comment has been minimized.

Copy link
@ruflin

ruflin Mar 20, 2017

Collaborator

I mean that the data structure / output depends on the user configuration. In most metricsets we know in advance the exact data structure. With metricsets like jmx it depends on the user configuration and we can't ship a predefined mapping.

This comment has been minimized.

Copy link
@martinscholz83

martinscholz83 Mar 20, 2017

Author Contributor

Ok, then it's definitely a dynamic metricset.

This comment has been minimized.

Copy link
@ruflin

ruflin Mar 20, 2017

Collaborator

Yeah, but I need to find a better name for the dynamic part, as I already confused @andrewkroh with that ...

@@ -0,0 +1,26 @@
- name: perfmon

This comment has been minimized.

Copy link
@ruflin

ruflin Mar 17, 2017

Collaborator

For dynamic metricsets, this will change so what we nomrally do is just define windows (module) part and leave the rest empty. Check the jmx metricset as an example.

metricsets: ["perfmon"]
enabled: true
period: 1s
perfmon.counters:

This comment has been minimized.

Copy link
@ruflin

ruflin Mar 17, 2017

Collaborator

It's great to have these example coutners which can be used. The way we handled it in the jmx metricset is that we defined only perfmon.counters: but left it empty (https://github.com/elastic/beats/blob/master/metricbeat/module/jolokia/_meta/config.yml#L8) and move the examples to the docs: https://github.com/elastic/beats/blob/master/metricbeat/module/jolokia/jmx/_meta/docs.asciidoc

@@ -0,0 +1,9 @@
- key: windows
title: "Windows"
description: >

This comment has been minimized.

// Part of new is also setting up the configuration by processing additional
// configuration entries if needed.
func New(base mb.BaseMetricSet) (mb.MetricSet, error) {

This comment has been minimized.

Copy link
@ruflin

ruflin Mar 17, 2017

Collaborator

A log message should be added to make it clear it is beta: https://github.com/elastic/beats/blob/master/metricbeat/module/jolokia/jmx/jmx.go#L49 We do this for all new modules / metricsets to test them first.

- module: windows
metricsets: ["perfmon"]
enabled: true
period: 1s

This comment has been minimized.

Copy link
@ruflin

ruflin Mar 17, 2017

Collaborator

Our default is 10s

@martinscholz83

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2017

Hi @ruflin, thanks for your review. I'm also not happy with the naming, but i think i get something better.

@martinscholz83

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2017

Hi @ruflin, here is an example output

 {
  "@timestamp": "2017-03-20T08:26:55.241Z",
  "beat": {
    "hostname": "4201halwsd00001",
    "name": "4201halwsd00001",
    "version": "6.0.0-alpha1"
  },
  "metricset": {
    "host": "localhost",
    "module": "windows",
    "name": "perfmon",
    "rtt": 0
  },
  "type": "metricsets",
  "windows": {
    "perfmon": {
      "disk": {
        "bytes_written": 289881.870059
      },
      "processor": {
        "processor_performance": 90.531335,
        "processor_time": 100.000000
      }
    }
  }
}
{
  "@timestamp": "2017-03-20T08:26:56.241Z",
  "beat": {
    "hostname": "4201halwsd00001",
    "name": "4201halwsd00001",
    "version": "6.0.0-alpha1"
  },
  "metricset": {
    "host": "localhost",
    "module": "windows",
    "name": "perfmon",
    "rtt": 0
  },
  "type": "metricsets",
  "windows": {
    "perfmon": {
      "disk": {
        "bytes_written": 163831.444077
      },
      "processor": {
        "processor_performance": 36.804798,
        "processor_time": 100.000000
      }
    }
  }
}
@martinscholz83

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2017

Todo:

  • Adding tests
mark first fetch
Most counters require two sample values in order to compute a
displayable value. This means to display the first result we need two
values
@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Mar 28, 2017

@maddin2016 Could you update the CHANGELOG.asciidoc ? About the config options structure not sure what we should do. Would like to get the input from @andrewkroh too. I tend to the in the direction of the "flatter" one as it makes the code simpler and has less indentations to get wrong, but we loose the grouping concept. The nice thing about having it in beat, we still have the possibility to change it later in case we see people use it very differently.

@martinscholz83

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2017

I update the asciidoc. What about the windows.go file in the module folder. Is this this necessary?

- windows
    - perfmon
    windows_windows.go????
@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Mar 28, 2017

You need a doc.go in the module folder and one in the metricset folder. What would windows_windows.go be for?

@martinscholz83

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2017

The doc.go is present. I thought there have to be a windows.go file like system.go in system to initialize the module.

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Mar 28, 2017

As long as there is no special implementation on the module level, the file is not needed and a "default module" will be initialised.

@martinscholz83

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2017

What about the failing tests. Seems it tries to build for linux and mac. Something is have missed?

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Mar 28, 2017

The first part which is failing is because of the linting. Make sure to run make fmt and make update before the next push. Looking into the other ones now.

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Mar 28, 2017

The crosscompile error is because you are missing the doc.go in the perfmon directory. Adding it should also solve the other build error.

@martinscholz83

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2017

@ruflin, can you rebuild travis again. I don't see any output whats going wrong.

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Mar 28, 2017

@maddin2016 I think travis is a bit grumpy today. Have the same issue on other PRs. Probably we just have to wait :-(

@martinscholz83

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2017

Ok 👍

@martinscholz83

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2017

How i can pdh_windows_integration_test.go only test on windows?

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Mar 28, 2017

You can either call it pdh_integration_windows_test.go or add a build flag on top of the file to specify only windows.

@martinscholz83

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2017

On top i had specified //+build integration windows. But that doesn't seem to work. I will rename the file and see what happens.

@martinscholz83

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2017

🎉

@ruflin ruflin referenced this pull request Mar 28, 2017
7 of 10 tasks complete
@andrewkroh

This comment has been minimized.

Copy link
Member

commented Mar 28, 2017

@maddin2016 All green, great! So we are going to merge this is and would like to iterate on it with some additional (smaller) PRs. We're starting a meta issue to track some changes to the module. Hopefully you can help us with these changes. Thanks for the contribution!!

@andrewkroh andrewkroh merged commit 3d2b0fa into elastic:master Mar 28, 2017

3 checks passed

CLA Commit author has signed the CLA
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@martinscholz83

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2017

Of course!!

@sy43165

This comment has been minimized.

Copy link

commented May 23, 2017

I'm experimenting with the metricbeat-6.0.0-alpha1 and I have an issue/concern. Forgive the noob question but do I report that here?

@andrewkroh

This comment has been minimized.

Copy link
Member

commented May 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.