-
Notifications
You must be signed in to change notification settings - Fork 898
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
Use parents from resource when there are not exist in MetricRollup #12164
Use parents from resource when there are not exist in MetricRollup #12164
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM
perf.parent_storage || perf.resource.storage, | ||
perf.parent_ems || perf.resource.ext_management_system, | ||
@enterprise, | ||
perf.resource.try(:tenant)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notes
- This list is later passed to the method that finds applicable rate/s (in
get_rates
). - We cache rates (probably for performance reasons).
- We need to update caching mechanism, when amending rate selection. -> please add
resource_id
intohash_features_affecting_rate
Another idea:
- Given New chargeback calculations #11648 💛 brings beautiful performance boost 🎆 (less top-level cycles), perhaps we don't need the caching anymore. ❓ dunno. 💡
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 comment
@miq-bot add_label wip |
and move compact to the get_rate_parents
d72dd2b
to
725878d
Compare
@miq-bot remove_label wip |
After disscussion with @isimluk we realized that these changes should be done also for determination of key in the method so before it I moved determination of resource parents (host, ems_cluster, storage and tenant) to the one method in first two commits and then I did described changes on the one place. thanks @isimluk and please review |
In this PR ManageIQ#11648 we introduced selecting rates according to first MetricRollup record of group. We can do it because group of these records belong to one resource. Selection is based on MetricRollup#parent_host or parent_ems_cluster or parent_storage or parent_ems but sometimes this fields in MetricRollup are nil and it produces empty cells on the report - it is because proper rate was not selected. Fix provides that we are selecting parents from the resource direcly when there are not exists in MetricRollup.
725878d
to
4e2086a
Compare
Checked commits lpichler/manageiq@3f48492~...4e2086a with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 |
|
||
expect(parents).to match_array(expected_array) | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woot! 🌳
parent_ems || resource.try(:ext_management_system), | ||
resource.try(:tenant) | ||
].compact | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@gtanzillo I think this is step in the right direction. |
The question remains, why the metric rollup records without values had existed. |
…_metric_rollup Use parents from resource when there are not exist in MetricRollup (cherry picked from commit 7061718)
Euwe Backport details: $ git log -1
commit 9910726a39da3ebd18159b90f3a10998f74e71d6
Author: Gregg Tanzillo <gtanzill@redhat.com>
Date: Tue Nov 1 09:08:04 2016 -0400
Merge pull request #12164 from lpichler/select_parent_when_missing_in_metric_rollup
Use parents from resource when there are not exist in MetricRollup
(cherry picked from commit 70617181fdcfa9ba625dcef95989d3a48a182f32) |
Select parents from resource when there are nils in MetricRollups records
In this PR #11648 we
introduced selecting rates according to first MetricRollup
record of group. We can do it because group of these records
belong to one resource.
Selection is based on
MetricRollup#parent_host or parent_ems_cluster or
parent_storage or parent_ems but sometimes this fields in
MetricRollup are nil and it produces empty cells on the
report - it is because proper rate was not selected.
Fix provides that we are selecting parents from
the resource direcly when there are not exists in
MetricRollup.
Screenshot of issue: MetricRollup#parent_storage was nil in May records.
before
after
@miq-bot add_label chargeback, bug, reporting
@miq-bot assign @gtanzillo