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

Group all cpu usage per core statistics under cpus #496

Merged
merged 2 commits into from
Dec 14, 2015

Conversation

monicasarbu
Copy link
Contributor

The goal of this PR is to group all the cpu usage per core statistics under cpus and add a configuration option cpu_per_core to enable it. By default it is set to false as it exports a lot of data.

The number of fields that are exports depends on the number of cores, so for these data we are not providing sample dashboards as it depends on the number of cores.

This is the first change out of multiple changes requested by #424.

System *bool
Proc *bool
Filesystem *bool
Cpu_per_core *bool
Copy link
Member

Choose a reason for hiding this comment

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

Can you use here `CpuPerCore *bool yaml:"cpu_per_core" to make sure we get the right config? We should also add it for the above 3 configs as we did in the other beats to make it clear, the values are coming from a yaml doc.

In the example above the backticks are missing, as they are also use to mark some code :-( here is an example: https://github.com/elastic/filebeat/blob/master/config/config.go#L39

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree with you it's better to add "yaml". I didn't reach the point to make nicer the code.

Copy link
Member

Choose a reason for hiding this comment

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

@monicasarbu Sorry, was too early. No review label yet ;-)

@monicasarbu monicasarbu force-pushed the group_cpu_usage_per_core branch 2 times, most recently from 2f3bb9e to 3c4c394 Compare December 10, 2015 17:40
@monicasarbu monicasarbu mentioned this pull request Dec 10, 2015
3 tasks
@monicasarbu
Copy link
Contributor Author

This feature is implemented only for Unix systems, so I am disabling the test for windows in order for the tests to pass on windows.

Add a configuration option to enable adding cpu usage per core. By default is false as there
are too many data exported.
Rename proc option with process (as the type it got changed to process in the last release).
Add an example JSON output
Add a test for cpu_per_core option only on Unix systems
@monicasarbu
Copy link
Contributor Author

Ready for review.


=== cpui Fields

This group contains cpu usage statistics of the core i, where i is the core number
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 it would look better to use cpuX with X being the variable rather then "i".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

tsg added a commit that referenced this pull request Dec 14, 2015
Group all cpu usage per core statistics under cpus
@tsg tsg merged commit aca072e into elastic:master Dec 14, 2015
@monicasarbu monicasarbu mentioned this pull request Dec 16, 2015
monicasarbu added a commit to monicasarbu/beats that referenced this pull request Dec 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants