opened
maddin2016
username_0
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.
New module windows for metricbeat
issue
created
elasticmachine
username_1
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.
comment
created
elasticmachine
username_1
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.
comment
created
maddin2016
username_0
Is there some flag which says this module is only for windows?
comment
created
maddin2016
username_0
Hi @username_2, thanks for your review. I'm also not happy with the naming, but  i think i get something better.
comment
created
maddin2016
username_0
Hi @username_2, 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
      }
    }
  }
}
```
comment
created
maddin2016
username_0
Todo:

- [ ] Adding tests
comment
created
maddin2016
username_0
How can i access the error throwing in `New` if there is a non existing counter while testing?
comment
created
ruflin
username_2
@username_0 Can you share some more details on what do you mean by accesssing error and new? Link to the code?
comment
created
maddin2016
username_0
[Here](https://github.com/username_0/beats/blob/9047b36f61ebf6a8dba0968dc4df5e023534e62f/metricbeat/module/windows/perfmon/perfmon.go#L59) you get an error if something going wrong while adding your counters. How can i access this error while testing. I'm currently can access errors while fetching.
comment
created
ruflin
username_2
@username_0 Are you are doing unit or integration tests, you could directly call getHandle()  as this is where the error is coming from: `https://github.com/elastic/beats/pull/3758/files#diff-8abb9df0c384f0541a13e24de768868aR32` Or are you talking about the system tests?
comment
created
maddin2016
username_0
I think have mixed up integration with system tests. Do i have to create both or only integration tests?
comment
created
ruflin
username_2
@username_0 I think for this PR integration tests make more sense as in the system tests we mostly check if the docs and structure are correct. But as the structure will be dynamic anyways, this is not needed. So all tests will be golang based ;-)
comment
created
maddin2016
username_0
Hi @username_3, i have changed the error handling for the syscall's. With signature `err error` i ran into different problems. For example it returns always `invalid argument` for `_PdhOpenQuery`, or if you define an non existing counter, it doesn't return that error because it only checks for `0`. So i think its better to return the values and maybe translate them into Error Codes. See [here](https://msdn.microsoft.com/en-us/library/windows/desktop/aa373046%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396) for all codes.
comment
created
andrewkroh
username_3
Regarding the error handling, we can look into improvements after we get this initial PR merged in. If some of the functions deviate from the conventional non-zero indicates failure semantics there is a way to tell the code generator this ([link](https://github.com/golang/go/blob/master/src/syscall/mksyscall_windows.go#L29-L34)).
comment
created
andrewkroh
username_3
Regarding moving this to the system module, I still think it belongs in a windows module because there isn't an equivalent implementation for other operating systems. The current metricsets in the system module have implementations for multiple operating systems, and provide an abstraction on top of a lower level OS data source.

Another possible metricset that I think would go into the windows module is for WMI (like [wmibeat](https://github.com/eskibars/wmibeat)).
comment
created
andrewkroh
username_3
You'll need to add a `doc.go` file to the perfmon package to make it build on non-windows systems.
comment
created
maddin2016
username_0
Do i have to add a `windows_windows..go` file to the module folder? Or is it fine to only have the metricset.
comment
created
ruflin
username_2
+1 on having this in the windows module.
comment
created
ruflin
username_2
@username_0 Could you update the CHANGELOG.asciidoc ? About the config options structure not sure what we should do. Would like to get the input from @username_3 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.
comment
created
maddin2016
username_0
I update the asciidoc. What about the `windows.go` file in the module folder. Is this this necessary? 
```
- windows
    - perfmon
    windows_windows.go????
```
comment
created
ruflin
username_2
You need a `doc.go` in the module folder and one in the metricset folder. What would `windows_windows.go` be for?
comment
created
maddin2016
username_0
The `doc.go` is present. I thought there have to be an `windows.go` file like `system.go` in system to initialize the module.
comment
created
ruflin
username_2
As long as there is no special implementation on the module level, the file is not needed and a "default module" will be initialised.
comment
created
maddin2016
username_0
What about the failing tests. Seems it tries to build for linux and mac. Something is have missed?
comment
created
ruflin
username_2
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.
comment
created
ruflin
username_2
The crosscompile error is because you are missing the `doc.go` in the `perfmon` directory.
comment
created
maddin2016
username_0
@username_2, can you rebuild travis again. I don't see any output whats going wrong.
comment
created
ruflin
username_2
@username_0 I think travis is a bit grumpy today. Have the same issue on other PRs. Probably we just have to wait :-(
comment
created
maddin2016
username_0
Ok 👍
comment
created
maddin2016
username_0
How i can `pdh_windows_integration_test.go` only test on windows?
comment
created
ruflin
username_2
You can either call it `pdh_integration_windows_test.go` or add a build flag on top of the file to specify only windows.
comment
created
maddin2016
username_0
On top i had specified `//+build integration windows`. But that doesn't seem to work. I will rename the file and see what happens.
comment
created
maddin2016
username_0
🎉
comment
created
andrewkroh
username_3
@username_0 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](https://github.com/elastic/beats/issues/3828) to track some changes to the module. Hopefully you can help us with these changes. Thanks for the contribution!!
comment
created
maddin2016
username_0
Of course!!
comment
created
sy43165
username_4
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?
comment
created
andrewkroh
username_3
@username_4 Please use https://discuss.elastic.co/beats.
comment