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

Do not calculate useless group metrics #15260

Merged

Conversation

isimluk
Copy link
Member

@isimluk isimluk commented May 31, 2017

  • cpu is useless as it counts usagemhz and vm_numvcpus together
  • cpu_cores is useless as it only reports cpu_cores
  • memory is useless as counts used and available together
  • net_io is useless as it only reports net_ui_rate_avg
  • disk_io is useless as it only reports disk_usage_rate_avg
  • storage is useless as it counts used and allocated together
  • fixed is useless as it counts all the fixed metrics together (that equals to 4 for each day, not useful)

https://bugzilla.redhat.com/show_bug.cgi?id=1455690
https://bugzilla.redhat.com/show_bug.cgi?id=1454456

Note this only fixes part of the upper mentioned bzs.

https://bugzilla.redhat.com/show_bug.cgi?id=1455690
https://bugzilla.redhat.com/show_bug.cgi?id=1454456

 - cpu is useless as it counts usagemhz and vm_numvcpus together
 - cpu_cores is useless as it only reports cpu_cores
 - memory is useless as counts used and available together
 - net_io is useless as it only reports net_ui_rate_avg
 - disk_io is useless as it only reports disk_usage_rate_avg
 - storage is useless as it counts used and allocated together
 - fixed is useless as it counts all the fixed metrics together
   (that equals to 4 for each day, not useful)
It used to return array with single item, now it just returns that
single item.

Less object creation -> less fat in the code -> performance goes up.
@miq-bot
Copy link
Member

miq-bot commented May 31, 2017

Checked commits isimluk/manageiq@afb10c1~...4b45950 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. ⭐

Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@gtanzillo gtanzillo added this to the Sprint 62 Ending Jun 5, 2017 milestone Jun 6, 2017
@gtanzillo gtanzillo merged commit d2f04e9 into ManageIQ:master Jun 6, 2017
@simaishi
Copy link
Contributor

simaishi commented Jun 6, 2017

@isimluk @gtanzillo this can be fine/yes and euwe/yes?

@isimluk
Copy link
Member Author

isimluk commented Jun 7, 2017

Definitely fine/yes. However, I would prefer euwe/no (needs manual backport), unless there is a real need to have it.

The other pr can be euwe/yes, effectively fixing that bz.

@miq-bot add_label fine/yes, euwe/no

@isimluk isimluk deleted the do-not-report-useles-group-metric branch June 7, 2017 07:08
@simaishi
Copy link
Contributor

simaishi commented Jun 9, 2017

Backported to Euwe via #15337

simaishi pushed a commit that referenced this pull request Jun 12, 2017
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit c7d9736f646427fa90970f2d4b47b96fc85eff41
Author: Gregg Tanzillo <[email protected]>
Date:   Mon Jun 5 23:09:23 2017 -0400

    Merge pull request #15260 from isimluk/do-not-report-useles-group-metric
    
    Do not calculate useless group metrics
    (cherry picked from commit d2f04e9ed311bf796dbb4405c90ccdc833417e53)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1459562

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.

4 participants