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

Charge since the first MetricRollup #14666

Merged
merged 2 commits into from
Apr 10, 2017
Merged

Conversation

isimluk
Copy link
Member

@isimluk isimluk commented Apr 6, 2017

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

On a fresh install, we start with inventory collection at time X. The resources are then created_on time X. However, the C&U collection can go few day back (by default we go back to beginning_of_day, but this is
subject to configuration. (See Settings.performance.history.initial_capture_days)

When charging, we cannot rely on created_on time, we can charge from the beginning of the C&U for the given resource.

if born_at && born_at > @start_time
# run this extra SELECT only when useful -- if resource was born in the current interval
# metrics can be older than resource (first capture may go few days back)
born_at = [born_at, resource.first_capture].compact.min
Copy link
Contributor

Choose a reason for hiding this comment

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

@isimluk can we somehow reuse first_metric_rollup_record what we already have (instead of first_capture) ?

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

On a fresh install, we start with inventory collection at time X. The
resources are then created_on time X. However, the C&U collection can go
few day back (by default we go back to beginning_of_day, but this is
subject to configuration.

(See Settings.performance.history.initial_capture_days)

When charging, we cannot rely on created_on time, we can charge from
the beginning of the C&U for the given resource.
@miq-bot
Copy link
Member

miq-bot commented Apr 7, 2017

Checked commits isimluk/manageiq@cc3f6c1~...cf82d67 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 🍪

Copy link
Contributor

@lpichler lpichler left a comment

Choose a reason for hiding this comment

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

Nice 👍

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.

LGTM 👍

@gtanzillo gtanzillo merged commit e43ccab into ManageIQ:master Apr 10, 2017
@isimluk isimluk deleted the metrics-time branch April 10, 2017 13:37
simaishi pushed a commit that referenced this pull request Apr 11, 2017
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit 6c6e5c10dd77c598b0c58e1bda3a7ff830746f71
Author: Gregg Tanzillo <gtanzill@redhat.com>
Date:   Mon Apr 10 09:25:22 2017 -0400

    Merge pull request #14666 from isimluk/metrics-time
    
    Charge since the first MetricRollup
    (cherry picked from commit e43ccabaddc886bdeff4d0e8f91bc557ad3ac3fc)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1441244

@kbrock kbrock added this to the Sprint 59 Ending Apr 24, 2017 milestone Apr 11, 2017
@simaishi
Copy link
Contributor

@isimluk @gtanzillo backporting this to Euwe branch is requested on the BZ. Would that be a problem?

@gtanzillo
Copy link
Member

@simaishi I don't see any problem with backporting this to Euwe.

simaishi pushed a commit that referenced this pull request Jun 30, 2017
@simaishi
Copy link
Contributor

Euwe backport details:

$ git log -1
commit 3780cdc7486ba866580d58e2bc3b4476e13f8dc4
Author: Gregg Tanzillo <gtanzill@redhat.com>
Date:   Mon Apr 10 09:25:22 2017 -0400

    Merge pull request #14666 from isimluk/metrics-time
    
    Charge since the first MetricRollup
    (cherry picked from commit e43ccabaddc886bdeff4d0e8f91bc557ad3ac3fc)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1465077

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.

6 participants