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

Metricbeat system module updates for Windows #2884

Merged
merged 3 commits into from
Oct 31, 2016

Conversation

andrewkroh
Copy link
Member

@andrewkroh andrewkroh commented Oct 29, 2016

@andrewkroh andrewkroh added bug Metricbeat Metricbeat needs_backport PR is waiting to be backported to other branches. labels Oct 29, 2016
@andrewkroh andrewkroh force-pushed the feature/update-gosigar branch 3 times, most recently from 9147dac to ad04204 Compare October 29, 2016 15:22
@andrewkroh
Copy link
Member Author

I would like to have elastic/gosigar#58 and #2282 (Metricbeat AppVeyor) merged first. Then I will update this PR with those fixes.

@ruflin
Copy link
Member

ruflin commented Oct 31, 2016

@andrewkroh Both PR's were merged.

@@ -8,3 +8,4 @@ This metricset is available on:
- FreeBSD
- Linux
- OpenBSD
- Windows
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -1,58 +0,0 @@
########################## Metricbeat Configuration ###########################
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that change on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is on purpose, but I did want to ask you about it. I removed this file. Where is this file used?

@@ -17,10 +17,10 @@ metricbeat.modules:
- cpu

# System Load stats
- load
#- load
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we mention that we changed the defaults in the config file? Why is load disabled now by default?

@@ -16,7 +16,7 @@
- filesystem

# File system summary stats
#- fsstat
- fsstat
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just making sure. We had this disabled before because it wasn't working on Windows and now it is?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was working on Windows previously. So maybe it was commented out to reduce the amount of data Metricbeat sends? But the fsstats data is used on the Metricbeat overview dashboard which is why I think we should enabled it by default.

@@ -4,10 +4,10 @@
- cpu

# System Load stats
- load
#- load
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can leave this enabled. We have code to disable it automatically on WIndows: https://github.com/elastic/beats/blob/master/metricbeat/Makefile#L61

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'll revert this. For some reason I thought we removed all of the OS specific before-build changes.


# Per CPU core stats
#- core
- core
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also not sure about this one, I think we had it disabled by default in an effort to keep disk usage down?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can revert this too. I was thinking that it was only commented out because of it not working on Windows.

@andrewkroh andrewkroh force-pushed the feature/update-gosigar branch 2 times, most recently from 3b59ad4 to a1308f9 Compare October 31, 2016 13:02
@andrewkroh
Copy link
Member Author

I updated the PR to contain the gosigar fixes and rebased it so that it gets built on AppVeyor. I reverted the changes to load and cores in the config file. I have left fsstats enabled by default.

…6bc8887f99

This will fix issues with WMI queries on Windows XP and 2003. Fixes elastic#2885
- Added system core metricset for Windows. Per core metrics were implemented
  in elastic/gosigar.
- Added logging of process/system details on Windows to aid in debugging (user,
  arch, cores, sid, privs).
- Fixes elastic#2860 (PPID is zero on Windows).
- Fixes elastic#1704 (Server 2003 - PID’s not recognized). The command line arguments
  for the process will not be reported on XP and 2003.
- Fixes elastic#1897 (OpenProcess access denied on Windows). Added code to enable the
  SeDebugPrivilege when it is available.
- Fixes elastic#2885 (diskio metricset fails on XP and 2003).
- Enabled fsstats by default in Metricbeat config.
@andrewkroh andrewkroh force-pushed the feature/update-gosigar branch from a1308f9 to eb88c2c Compare October 31, 2016 13:28
@ruflin ruflin merged commit b44bdd6 into elastic:master Oct 31, 2016
@andrewkroh andrewkroh removed the needs_backport PR is waiting to be backported to other branches. label Nov 1, 2016
@andrewkroh andrewkroh deleted the feature/update-gosigar branch November 2, 2016 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants