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

Introduce Consumption class to abstract charging from MetricRollups #12792

Merged
merged 8 commits into from
Nov 30, 2016

Conversation

isimluk
Copy link
Member

@isimluk isimluk commented Nov 22, 2016

What

Encapsulate chargeback's manipulation with metric_rollups to a separate class.

Why?

When charging we take into account three main inputs

  • consumption (now was MetricRollup)
  • rates (Chargeback::RatesCache & ChargebackRate*)
  • cb type & report user properties (Chargeback::ReportOptions)

Now, let's investigate, why we need consumption to be polymorphic:

  1. We had assumed that consumption == single metric_rollup.
  2. However, then @lpichler introduced more clever charging for bunch of rollups. So, until now we have assumed consumption equals N metric_rollups`
  3. I want to introduced charging per dynamic disk types. Then, the consumption will equal to metric_rollups and/or vim_performance_states.
  4. Then, @gtanzillo has an idea of charging without metric rollups. Then the consumption will equal to (metric_rollups and/or vim_performance_states) and/or assumed flat consumption.

Other notes

I have found some irregularities in a way we handle @hours_in_interval. (related to starts/ends of interval & rates interval, vs report interval). The Consumption object will make it easier to address these irregularities.

@miq-bot add_label chargeback, refactoring
@miq-bot assign @gtanzillo

@miq-bot
Copy link
Member

miq-bot commented Nov 26, 2016

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

This class represents a consumption of resource in given interval. This
will enable us to introduce charging with missing metric_rollups.
Previously, we had special case to return either HOURS_IN_DAY or
HOURS_IN_WEEK that was because the

    (query_end_time - query_start_time) / 1.hour

without rounding returns stuff like 23.999988 hours.
@miq-bot
Copy link
Member

miq-bot commented Nov 28, 2016

Checked commits isimluk/manageiq@3e09bbc~...de586f0 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
3 files checked, 0 offenses detected
Everything looks good. ⭐

@gtanzillo
Copy link
Member

👍 Looks great!

@gtanzillo gtanzillo added this to the Sprint 50 Ending Dec 5, 2016 milestone Nov 30, 2016
@gtanzillo gtanzillo merged commit f9658d1 into ManageIQ:master Nov 30, 2016
@simaishi
Copy link
Contributor

Backported to Euwe via #13419

Fryguy added a commit to Fryguy/manageiq that referenced this pull request Feb 8, 2017
After the backport of ManageIQ#12792 and ManageIQ#13700, a user can be in a state where
their VimPerformanceOperatingRanges are inconsistent.  Although it
doesn't really harm anything, this data migration makes it consistent
once again.
Fryguy added a commit to Fryguy/manageiq that referenced this pull request Mar 1, 2017
After the backport of ManageIQ#12792 and ManageIQ#13700, a user can be in a state where
their VimPerformanceOperatingRanges are inconsistent.  Although it
doesn't really harm anything, this data migration makes it consistent
once again.
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.

4 participants