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

New chargeback calculations #11648

Merged
merged 7 commits into from
Oct 19, 2016

Conversation

lpichler
Copy link
Contributor

@lpichler lpichler commented Oct 3, 2016

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

Changes taking into account averages and maximums per selected interval in report(monthly, weekly, daily).
In general for allocated metric is used maximum and for used metrics is used average.

@miq-bot miq-bot added the wip label Oct 3, 2016
@chargio
Copy link
Contributor

chargio commented Oct 3, 2016

This starts to look great!

@miq-bot
Copy link
Member

miq-bot commented Oct 3, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@lpichler lpichler force-pushed the new_chargeback_calculations branch 8 times, most recently from 2cd041b to a3bf026 Compare October 11, 2016 12:18
@lpichler lpichler changed the title [WIP] new chargeback calculations New chargeback calculations Oct 11, 2016
@lpichler
Copy link
Contributor Author

@miq-bot remove_label wip

@miq-bot miq-bot removed the wip label Oct 11, 2016
cost = 0
else
metric_value = r.metric_value_by(metric_rollup_records)
cost = r.cost(metric_value) * hours_in_interval
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gtanzillo I move out multiplying of cost method because we don't need it calculate (because we are not using accuntable period) separately fixed and variable part.

@lpichler
Copy link
Contributor Author

@gtanzillo PR updated.

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.

All the calculations look perfect 👍 except for the "Storage Allocated Cost" where it seems there was a bug in the original rate tiers code.

(fixed_rate, variable_rate) = find_rate(value)
hourly(fixed_rate) + hourly(variable_rate) * value

hourly(fixed_rate) + hourly(variable_rate) * value
Copy link
Member

Choose a reason for hiding this comment

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

@lpichler I found a bug in this calculation when I was trying to verify the value for "Storage allocated Cost". It was showing 0 when it should be $29,760.00 => (1.0 fixed cost * 40gb * 744 hours).

It's incorrect because there should be parens around hourly(fixed_rate) + hourly(variable_rate). I made this change locally and got the correct cost:

diff --git a/app/models/chargeback_rate_detail.rb b/app/models/chargeback_rate_detail.rb
index 651249a..f9ac46e 100644
--- a/app/models/chargeback_rate_detail.rb
+++ b/app/models/chargeback_rate_detail.rb
@@ -72,7 +72,7 @@ class ChargebackRateDetail < ApplicationRecord

     (fixed_rate, variable_rate) = find_rate(value)

-    hourly(fixed_rate) +  hourly(variable_rate) * value
+    (hourly(fixed_rate) + hourly(variable_rate)) * value
   end

   def hourly(rate)

It looks like this was introduced with the tiered rates feature.

Here are before and after shot of the report so that you have a frame of reference -
Before
old

After
new

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gtanzillo Nice finding, but I need to confirm these calculation which was used for it:
There are used this calculations:

I guess that was used this rate for Storage Allocate Cost:
Fixed: $1
Variable: $0
Per Time: Hourly
Per Unit: GB

Report setttings: Monthly

I guess fixed part should not be depend on metric value (value of Storage Allocate):
Hourly rate for GB: $1 (we have it already in hours)
Hourly rate for B(bytes, it is unit in MetricRollup table):$1 / (1024)^3 = 9.313225746154785e-10

9.313225746154785e-10 * 744(hours in month) = 6.92903995513916e-07

and in report it was rounded to $0.

Can you confirm it please ?

Copy link
Member

Choose a reason for hiding this comment

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

After working through this with @lpichler and confirming with @sergio-ocon, we came up with a slightly different change -

diff --git a/app/models/chargeback_rate_detail.rb b/app/models/chargeback_rate_detail.rb
index 651249a..0ebbd48 100644
--- a/app/models/chargeback_rate_detail.rb
+++ b/app/models/chargeback_rate_detail.rb
@@ -72,10 +72,10 @@ class ChargebackRateDetail < ApplicationRecord

     (fixed_rate, variable_rate) = find_rate(value)

-    hourly(fixed_rate) +  hourly(variable_rate) * value
+    hourly(fixed_rate, true) + (hourly(variable_rate) * value)
   end

-  def hourly(rate)
+  def hourly(rate, fixed=nil)
     hourly_rate = case per_time
                   when "hourly"  then rate
                   when "daily"   then rate / 24
@@ -85,7 +85,7 @@ class ChargebackRateDetail < ApplicationRecord
                   else raise "rate time unit of '#{per_time}' not supported"
                   end

-    rate_adjustment(hourly_rate)
+    fixed ? hourly_rate : rate_adjustment(hourly_rate)
   end

   # Scale the rate in the unit difine by user to the default unit of the metric

Fixed rates whether in a tier or in fixed compute or storage should not involve the value of any metric. The only exception a fixed rate is in a tier, the metric value is used to find the correct tier, but in the calculation of the fixed cost.

See this screen shot for reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @gtanzillo for finding it - I made change in separated commit
0b6e9d6

I did it in slightly different way to see that only hourly variable part is adjusted regard to default unit metric in MetricRollup table.

@gtanzillo
Copy link
Member

@zeari @bazulay @simon3z - fyi, this is the PR with all the new calculations for rolling up metrics and costs.

as test example is used month with 30 days
@miq-bot
Copy link
Member

miq-bot commented Oct 19, 2016

Checked commits lpichler/manageiq@f3506b7~...bdaad80 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
6 files checked, 1 offense detected

app/models/chargeback_rate_detail.rb

@lpichler lpichler changed the title [WIP] New chargeback calculations New chargeback calculations Oct 19, 2016
@lpichler
Copy link
Contributor Author

@miq-bot remove_label wip

@miq-bot miq-bot removed the wip label Oct 19, 2016
@lpichler
Copy link
Contributor Author

@gtanzillo I just corrected calculation of hourly rate, when rate is monthly. It doesn't reflect real count of days in month before.

@gtanzillo gtanzillo added this to the Sprint 48 Ending Oct 24, 2016 milestone Oct 19, 2016
@gtanzillo
Copy link
Member

👍 This one is ready to go!

@gtanzillo gtanzillo merged commit 63996cc into ManageIQ:master Oct 19, 2016
@lpichler
Copy link
Contributor Author

there is a list PRs which need to be merged first

#11645 (already backported to euwe)
#11647 (not backported to euwe)
#11685 (not backported to euwe)

@chessbyte
Copy link
Member

chessbyte pushed a commit that referenced this pull request Oct 21, 2016
@chessbyte
Copy link
Member

Euwe Backport details:

$ git log -1
commit d1b8be27e9d2208967d52fdb2bcc2326fd0c53a6
Author: Gregg Tanzillo <gtanzill@redhat.com>
Date:   Wed Oct 19 17:37:49 2016 -0400

    Merge pull request #11648 from lpichler/new_chargeback_calculations

    New chargeback calculations
    (cherry picked from commit 63996cc5ca543e61d4083af7554701ee21082130)

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

lpichler added a commit to lpichler/manageiq that referenced this pull request Oct 27, 2016
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.
@chessbyte
Copy link
Member

Backported to Darga via #12093

@lpichler lpichler deleted the new_chargeback_calculations branch March 3, 2017 09:41
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.

5 participants