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

dynamically generate CPU counts for metrics #23154

Merged
merged 9 commits into from
Dec 18, 2020

Conversation

fearful-symmetry
Copy link
Contributor

@fearful-symmetry fearful-symmetry commented Dec 15, 2020

What does this PR do?

This changes the behavior of the process and CPU libbeat metrics libraries so that we don't "store" CPU values, in the case of users running this inside VMs were CPU counts can change during runtime. I'm a tad worried about any potential performance impacts of hitting the NumCPU call, but unless it's serious, it might be better than coming up with something more sophisticated that refreshes the value every n minutes or something.

Why is it important?

If users are running metricbeat inside a VM where logical CPUs can be changed live, metricbeat can report incorrect metrics.

Checklist

  • My code follows the style guidelines of this project
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

@fearful-symmetry fearful-symmetry added bug Team:Integrations Label for the Integrations team labels Dec 15, 2020
@fearful-symmetry fearful-symmetry requested a review from a team December 15, 2020 20:34
@fearful-symmetry fearful-symmetry self-assigned this Dec 15, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Dec 15, 2020
@elasticmachine
Copy link
Collaborator

elasticmachine commented Dec 15, 2020

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #23154 updated

  • Start Time: 2020-12-17T16:57:31.506+0000

  • Duration: 53 min 39 sec

Test stats 🧪

Test Results
Failed 0
Passed 16974
Skipped 1276
Total 18250

Steps errors 2

Expand to view the steps failures

Terraform Apply on x-pack/metricbeat/module/aws
  • Took 0 min 15 sec . View more details on here
  • Description: terraform apply -auto-approve
Terraform Apply on x-pack/metricbeat/module/aws
  • Took 0 min 15 sec . View more details on here
  • Description: terraform apply -auto-approve

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 16974
Skipped 1276
Total 18250

@@ -52,6 +52,7 @@ The list below covers the major changes between 7.0.0-rc2 and master only.
- Replace `ACKCount`, `ACKEvents`, and `ACKLastEvent` callbacks with `ACKHandler` and interface in `beat.ClientConfig`. {pull}19632[19632]
- Remove global ACK handler support via `SetACKHandler` from publisher pipeline. {pull}19632[19632]
- Make implementing `Close` required for `reader.Reader` interfaces. {pull}20455[20455]
- Remove `NumCPU` as clients should update the CPU count on the fly in case of config changes in a VM. {pull}23154[23154]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we consider this a breaking change or bug fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a publicly exported variable, so technically a breaking fix.

Copy link

Choose a reason for hiding this comment

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

Although it is a change in reporting, most users will not notice. Users that will notice have had wrong data. Let's call it a fix. How about: Fix CPU usage metrics on VMs with dynamic CPU assignment. ...

@fearful-symmetry
Copy link
Contributor Author

jenkins, test this

@fearful-symmetry
Copy link
Contributor Author

/test

@fearful-symmetry fearful-symmetry merged commit 40aeeef into elastic:master Dec 18, 2020
fearful-symmetry added a commit to fearful-symmetry/beats that referenced this pull request Dec 18, 2020
* dynamically generate CPU counts for metrics

* dynamic-numcpu

* fix exported vars

* clean up CPU code

* fix CPU count in system/cpu

* update load

* add changelog

* fix tests

(cherry picked from commit 40aeeef)
fearful-symmetry added a commit that referenced this pull request Dec 18, 2020
* dynamically generate CPU counts for metrics

* dynamic-numcpu

* fix exported vars

* clean up CPU code

* fix CPU count in system/cpu

* update load

* add changelog

* fix tests

(cherry picked from commit 40aeeef)
@urso
Copy link

urso commented Dec 19, 2020

Can you please add an entry to Changelog.next.asciidoc as well? The change is clearly a user-visible fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Team:Integrations Label for the Integrations team v7.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants