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

Add normalized CPU values and number of cores #4553

Merged
merged 3 commits into from Jun 26, 2017

Conversation

Projects
None yet
4 participants
@andrewkroh
Copy link
Member

andrewkroh commented Jun 23, 2017

This include the contents of #4550 (so merge that first and the amount to review will be smaller).

  • Added normalized CPU metrics for core, cpu, and process metricsets that are divided by the number of CPU cores.
  • Refactored helper code used by core, cpu, and load metricsets.
  • Deprecated use of cpu_ticks in favor of per metricset settings.
# if true, exports the CPU usage in ticks, together with the percentage values
#cpu_ticks: false
# Configure the metric types that are included by these metricsets.
cpu.metrics: [percentages] # The other available options are normalized_percentages and ticks.

This comment has been minimized.

Copy link
@ruflin

ruflin Jun 26, 2017

Collaborator

Should we add "quotes" around percentages? Seems we do this everywhere else with strings.

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Jun 26, 2017

Author Member

I added the quotes for consistency. 3b23220

"idle": {
"pct": 0.9063,
"ticks": 22204290
"pct": 0,

This comment has been minimized.

Copy link
@ruflin

ruflin Jun 26, 2017

Collaborator

If you run it 2-3 more times, you should also get some values for the pct.

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Jun 26, 2017

Author Member

Fixed in 96a5b91

// Config for the system core metricset.
type Config struct {
Metrics []string `config:"core.metrics"`
CPUTicks *bool `config:"cpu_ticks"` // Deprecated.

This comment has been minimized.

Copy link
@ruflin

ruflin Jun 26, 2017

Collaborator

Should we deprecate it in 5.6 and remove it here?

This comment has been minimized.

Copy link
@monicasarbu

monicasarbu Jun 26, 2017

Contributor

@ruflin Any reason to deprecate it in 5.6 instead of 6.0.0?

This comment has been minimized.

Copy link
@ruflin

ruflin Jun 26, 2017

Collaborator

I don't like deprecated features in the code base. If don't do it now, we have to wait until 7.0 to remove it. Seems like a good time to do it now. Any disadvantage doing it now?

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Jun 26, 2017

Author Member

Does it make sense to add deprecation warnings in 5.6 when there is no replacement for cpu_ticks in 5.6? I thought there should be some period of overlap where the two config options are available to aid in migration.

This comment has been minimized.

Copy link
@ruflin

ruflin Jun 26, 2017

Collaborator

you mean no replacement in 6.0?

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Jun 26, 2017

Author Member

I meant 5.6. But possibly I misunderstood the original suggestion. You were suggesting adding the deprecation warnings in 5.6 and not having a migration path in 5.6 (e.g. we are telling the user not to use cpu_ticks, but not offering them an alternative in 5.6)?

// Validate validates the cpu config.
func (c Config) Validate() error {
if c.CPUTicks != nil {
logp.Deprecate("6.0.0", "cpu_ticks is deprecated. Add 'ticks' to the cpu.metrics list.")

This comment has been minimized.

Copy link
@ruflin

ruflin Jun 26, 2017

Collaborator

See previous comment

@tsg

This comment has been minimized.

Copy link
Collaborator

tsg commented Jun 26, 2017

Merged #4550, can you rebase this one, please? The flaky container failures in Metricbeat/travis should also be fixed after rebase.

andrewkroh added some commits Jun 23, 2017

Simplify the system.Round arguments
Remove the arguments and use a constant to control the maximum number of decimal places.

@andrewkroh andrewkroh force-pushed the andrewkroh:bugfix/6-0-cpu-normalization branch 2 times, most recently from e86be9d to 84e718f Jun 26, 2017

@andrewkroh

This comment has been minimized.

Copy link
Member Author

andrewkroh commented Jun 26, 2017

I rebased the PR to resolve the conflicts, and squashed the commits where I addressed action items because I think it would be best to "Rebase and merge" this PR.

Add normalized CPU values and number of cores
- Added normalized CPU metrics for core, cpu, and process metricsets that are divided by the number of CPU cores.
- Refactored helper code used by core, cpu, and load metricsets.
- Deprecated use of `cpu_ticks` in favor of per metricset settings.

@andrewkroh andrewkroh force-pushed the andrewkroh:bugfix/6-0-cpu-normalization branch from 84e718f to 979091f Jun 26, 2017

@tsg tsg merged commit d09da0c into elastic:master Jun 26, 2017

3 of 4 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
CLA Commit author has signed the CLA
Details
beats-ci Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@andrewkroh andrewkroh deleted the andrewkroh:bugfix/6-0-cpu-normalization branch Jul 5, 2017

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