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.

Sign up for GitHub

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

Filter relevant fields also according to chargeback class in Chargeback #17414

Merged
merged 1 commit into from
May 21, 2018
Merged

Filter relevant fields also according to chargeback class in Chargeback #17414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions app/models/chargeable_field.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,13 @@ def metric_key(sub_metric = nil)
"#{rate_name}_#{sub_metric ? sub_metric + '_' : ''}metric" # metric value (e.g. Storage [Used|Allocated|Fixed])
end

# Fixed metric has _1 or _2 in name but column
# fixed_compute_metric is used in report and calculations
# TODO: remove and unify with metric_key
def metric_column_key
fixed? ? metric_key.gsub(/\_1|\_2/, '') : metric_key
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what this is doing

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 is hack,

we have chargeback_rate_details and each detail is representing metric column na cost column.

For example:

�CPU Allocated Cost has metric column: cpu_allocated_metric and cost column cpu_allocated_cost

so when we are checking if cpu_allocated_cost is allowed to report, we are asking here https://github.com/ManageIQ/manageiq/pull/17414/files/213289cd034ea8054bbb9a30c80d229cd776c049#diff-7c998acd3d11c70ed830e216754f4b6bR42
if string cpu_allocated_cost (from selected fields) is related to metric_key which is based from ChargeableField#group and Chargeback#source.

But we have to add exception for fixed rate detail because there are two columns
fixed_compute_1_cost and fixed_compute_1_cost but related metric field is only one: fixed_compute_metric and then method ChargeableField#metric_key is not enough (it would produce fixed_compute_1_metric ) but we need to base check in fixed_compute_metric (some for storage)

so input of this method is fixed_compute_1_metric and it should remove _1 (etc.)

end

def cost_keys(sub_metric = nil)
keys = ["#{rate_name}_#{sub_metric ? sub_metric + '_' : ''}cost", # cost associated with metric (e.g. Storage [Used|Allocated|Fixed] Cost)
'total_cost']
Expand Down
4 changes: 2 additions & 2 deletions app/models/chargeback.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ def new_chargeback_calculate_costs(consumption, rates)
:resource => MiqEnterprise.first)

data = {}
rate.rate_details_relevant_to(relevant_fields).each do |r|
rate.rate_details_relevant_to(relevant_fields, self.class.attribute_names).each do |r|
r.populate_showback_rate(plan, r, showback_category)
measure = r.chargeable_field.showback_measure
dimension, _, _ = r.chargeable_field.showback_dimension
Expand Down Expand Up @@ -152,7 +152,7 @@ def calculate_costs(consumption, rates)
self.class.try(:refresh_dynamic_metric_columns)

rates.each do |rate|
rate.rate_details_relevant_to(relevant_fields).each do |r|
rate.rate_details_relevant_to(relevant_fields, self.class.attribute_names).each do |r|
Copy link
Member

Choose a reason for hiding this comment

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

It feels like this should be a method... I believe you're doing a set intersection of relevant_fields and self.class.attribute_names) in rate_details_relevant_to below, right?

def calculate_costs(consumption, rates)
  self.fixed_compute_metric = consumption.chargeback_fields_present if consumption.chargeback_fields_present
  self.class.try(:refresh_dynamic_metric_columns)
	 
  rates.each do |rate|
    rate.rate_details_relevant_to(relevant_fields & self.class.attribute_names).each do |r|

Either that or move the set intersection into the method below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately , it is not same.

Originally,
method rate_details_relevant_to is going thru all rate chargeback details and it is basically asking each rate detail chargeback - Does your cost calculation affect a selected report fields(report_cols) ?

And it will meet also rate detail chargeback with derived_vm_numvcpu_cores.
But if you have only total_cost on report - this total value are affecting all columns - derived_vm_numvcpu_cores included.

But we need to refuse derived_vm_numvcpu_cores for chargeback vm.

Hopefully, you understand better.

And sure we need to do some refactoring here: for example similar condition is also in ChargebackRateDetail#charge:97 method, I will do it in folluw up PR

r.charge(relevant_fields, consumption, @options).each do |field, value|
next unless self.class.attribute_names.include?(field)
self[field] = self[field].kind_of?(Numeric) ? (self[field] || 0) + value : value
Expand Down
10 changes: 7 additions & 3 deletions app/models/chargeback_rate.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,13 @@ def self.tag_class(klass)
super(klass)
end

def rate_details_relevant_to(report_cols)
# we can memoize, as we get the same report_cols thrrough the life of the object
@relevant ||= chargeback_rate_details.select { |r| r.affects_report_fields(report_cols) }
def rate_details_relevant_to(report_cols, allowed_cols)
Copy link
Member

Choose a reason for hiding this comment

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

We probably don't want to change this method signature if we don't have to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes we don't want but it is necessary here. But we can improve with some unification because similar condition is also in ChargebackRateDetail#charge:97 method,as I mentioned above.

# we can memoize, as we get the same report_cols through the life of the object
@relevant ||= begin
chargeback_rate_details.select do |r|
r.affects_report_fields(report_cols) && allowed_cols.include?(r.metric_column_key)
end
end
end

def self.validate_rate_type(type)
Expand Down
2 changes: 1 addition & 1 deletion app/models/chargeback_rate_detail.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ class ChargebackRateDetail < ApplicationRecord

delegate :rate_type, :to => :chargeback_rate, :allow_nil => true

delegate :metric_key, :cost_keys, :rate_key, :to => :chargeable_field
delegate :metric_column_key, :metric_key, :cost_keys, :rate_key, :to => :chargeable_field
Copy link
Member

Choose a reason for hiding this comment

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

Wait, we still want to delegate metric_key, don't we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we need it in ChargebackRateDetail#charge method


FORM_ATTRIBUTES = %i(description per_time per_unit metric group source metric chargeable_field_id sub_metric).freeze
PER_TIME_TYPES = {
Expand Down
12 changes: 12 additions & 0 deletions spec/models/chargeback_rate_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,16 @@
describe ChargebackRate do
describe "#rate_details_relevant_to" do
let(:count_hourly_variable_tier_rate) { {:variable_rate => '10'} }

let!(:chargeback_rate) do
FactoryGirl.create(:chargeback_rate, :detail_params => {:chargeback_rate_detail_cpu_cores_allocated => {:tiers => [count_hourly_variable_tier_rate]}})
end

it "skips cpu_cores_allocated column" do
expect(chargeback_rate.rate_details_relevant_to(['total_cost'].to_set, ChargebackVm.attribute_names)).to be_empty
end
end

context ".validate_rate_type" do
it "handles valid types" do
[:compute, :storage, 'compute', 'storage', 'Compute', 'Storage'].each do |type|
Expand Down
10 changes: 10 additions & 0 deletions spec/models/chargeback_vm_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,6 +627,16 @@ def result_row_for_vm(vm)

subject { ChargebackVm.build_results_for_report_ChargebackVm(options).first.first }

context "chargeback rate contains rate unrelated to chargeback vm" do
let!(:chargeback_rate) do
FactoryGirl.create(:chargeback_rate, :detail_params => detail_params.merge(:chargeback_rate_detail_cpu_cores_allocated => {:tiers => [count_hourly_variable_tier_rate]}))
end

it "skips unrelated columns and calculate related columns" do
expect(subject.cpu_allocated_metric).to eq(cpu_count)
end
end

it "cpu" do
expect(subject.cpu_allocated_metric).to eq(cpu_count)
used_metric = used_average_for(:cpu_usagemhz_rate_average, hours_in_month, @vm1)
Expand Down