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

Metric::Rollup - simplify column declarations #22608

Merged
merged 2 commits into from
Aug 12, 2023

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Jul 10, 2023

3 commits:

  • Metric::Rollup: dedup common rollup aggregate column lists
  • extract Metric::Rollup#aggregate_column
  • drop VimPerformanceState special case for looking up EmsCluster#vms

@kbrock kbrock requested a review from agrare as a code owner July 10, 2023 15:04
@kbrock kbrock changed the title Metric::Rollup - simplify column declarations [WIP] Metric::Rollup - simplify column declarations Jul 13, 2023
@miq-bot miq-bot added the wip label Jul 13, 2023
@kbrock
Copy link
Member Author

kbrock commented Jul 13, 2023

WIP: not sure that the aggregation is correct for EmsCluster. So the change may not be valid.

TODO: verify that aggregation is working, then verify if the vms are the same thing.

@kbrock kbrock removed the wip label Jul 15, 2023
@kbrock kbrock changed the title [WIP] Metric::Rollup - simplify column declarations Metric::Rollup - simplify column declarations Jul 15, 2023
@kbrock
Copy link
Member Author

kbrock commented Jul 15, 2023

UN-WIP: removed EmsCluster changes.

so this PR is now just the 2 refactors around rollup

@Fryguy Fryguy self-assigned this Jul 19, 2023
Comment on lines 9 to 10
BASIC_VM_CPU_COLS = [:cpu_usage_rate_average, :derived_vm_numvcpus, :derived_memory_used, :net_usage_rate_average].freeze
BASIC_VM_DISK_COLS = [:cpu_usage_rate_average, :derived_memory_used, :disk_usage_rate_average, :net_usage_rate_average].freeze
Copy link
Member

Choose a reason for hiding this comment

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

The first set is really around container usage, not vm usage. Also it includes more than CPU. Also, also the pattern is to use the phrase ROLLUP_COLS (aside from storage), so I'm thinking rename to something like:

Suggested change
BASIC_VM_CPU_COLS = [:cpu_usage_rate_average, :derived_vm_numvcpus, :derived_memory_used, :net_usage_rate_average].freeze
BASIC_VM_DISK_COLS = [:cpu_usage_rate_average, :derived_memory_used, :disk_usage_rate_average, :net_usage_rate_average].freeze
CONTAINER_ROLLUP_COLS = [:cpu_usage_rate_average, :derived_vm_numvcpus, :derived_memory_used, :net_usage_rate_average].freeze
VM_ROLLUP_COLS = [:cpu_usage_rate_average, :derived_memory_used, :disk_usage_rate_average, :net_usage_rate_average].freeze

@kbrock
Copy link
Member Author

kbrock commented Aug 1, 2023

update:

  • rebased (fix merge conflict)
  • renamed constants

@miq-bot
Copy link
Member

miq-bot commented Aug 1, 2023

Checked commits kbrock/manageiq@d10075a~...9f6b517 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 🍪

@kbrock
Copy link
Member Author

kbrock commented Aug 11, 2023

@Fryguy I think this is ready for merge. Probably without backport (since it is low risk, but no need to backport)

@Fryguy Fryguy merged commit 3a1b9be into ManageIQ:master Aug 12, 2023
9 checks passed
@kbrock kbrock deleted the rollup_columns branch August 14, 2023 17:00
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