Refactor Chargeback data to instance of Chargeback #11925
Conversation
|
@miq-bot assign @gtanzillo |
isimluk
changed the title
|
|
||
| if defined_column_for_report | ||
| [metric_key, metric_group_key].each { |col| col_hash[col] = metric } | ||
| [cost_key, cost_group_key, 'total_cost'].each { |col| col_hash[col] = cost } | ||
| end | ||
|
|
||
| col_hash | ||
| col_hash.each do |k, val| |
| @@ -33,7 +35,7 @@ def self.build_results_for_report_chargeback(options) | |||
| base_rollup = base_rollup.select(*rate_cols) | |||
|
|
|||
| timerange = get_report_time_range(options, interval, tz) | |||
| data = {} | |||
| cb.data = {} | |||
There was a problem hiding this comment.
@isimluk Where does cb get instantiated?
There was a problem hiding this comment.
Near the method start. It was already there.
isimluk
force-pushed
the
refactor-cb-bind-data-to-instance
branch
from
b5208cd
to
f4d8614
Compare
|
@lpichler @gtanzillo I have improved this pr based on what I learned about Chargebacks yesterday. Also, I've tried to make respective commit messages useful for reviewer. |
|
<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush. |
isimluk
force-pushed
the
refactor-cb-bind-data-to-instance
branch
2 times, most recently
from
1b88b32
to
2109ad3
Compare
|
Reworked on top of the @lpichler's new calculations. Ready to be merged! |
| calculated_costs['fixed_compute_metric'] = chargeback_fields_present if chargeback_fields_present | ||
| def calculate_costs(metric_rollup_records, rates, hours_in_interval) | ||
| chargeback_fields_present = metric_rollup_records.count(&:chargeback_fields_present?) | ||
| self.fixed_compute_metric = chargeback_fields_present if chargeback_fields_present |
There was a problem hiding this comment.
This feels a bit weird or fragile because fixed_compute_metric is only defined in the descendant classes and in in the base class.
There was a problem hiding this comment.
Nod.
Although, I am not bringing the fragility here. I only make changes, that make this easier to spot.
Previously, we have build a hash (was stored in data[key] or calculated_costs), this hash had various keys not defined in Chargeback parent class. Later we passed this hash to new() and hoped for the best. We then modified the code to assume some of the keys are always present, like fixed_compute_metric. Perhaps we can later move cols that are always needed to the Chargeback parent class?
I have noticed, there are other parts of Chargeback class that rely on its descendants to define certain things. For example, get_keys_and_extra_fields, get_rate_parents, report_col_options, report_static_cols, where_clause, and perhaps more. Thus, this pattern is common in this part of the code.
Yep, I feel confident in this regard. This pr does not bring a change of behavior (output for a given input). It only re-structures some of the code and data during the processing. For such change, I think, tests need to go through all the methods I touched. That is already achieved for each of |
|
<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush. |
|
👍 I'm good with this. Will merge once you rebase and eliminate the conflicts. |
isimluk
force-pushed
the
refactor-cb-bind-data-to-instance
branch
from
2109ad3
to
ec03a76
Compare
|
Thanks!! Conflicts resolved. |
isimluk
force-pushed
the
refactor-cb-bind-data-to-instance
branch
from
ec03a76
to
bc575d3
Compare
Do instanciate early in the calculation process. This will allows us to encapsulate code to these instances.
It only manipulates Chargeback instance based on perf record.
This lowers the indirection and complexity.
As we see, it takes a very long name to describe what it does. Perhaps we can lower the complexity in future.
|
Checked commits isimluk/manageiq@527712d~...bc575d3 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 |
|
Backported to Euwe via #13419 |
Chargeback calculation is now set of class methods. Sometimes using class variables. If we convert this to instance methods, we may achieve
This is first step to achieve this. More to came. Many thanks to @lpichler for many consultations!
@miq-bot add_label refactoring, chargeback