Fix queueing of historical metrics collection #14695

gtanzillo · 2017-04-07T19:11:05Z

Set end_time to the end of the current day before queueing historical collection to allow put match and update existing queue item
Set last_perf_capture_on to end time when no metrics were available to prevent constantly queueing the same historical range.

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

/cc @Fryguy @blomquisg @kbrock @jrafanie

jrafanie · 2017-04-07T19:36:53Z

app/models/metric/ci_mixin/capture.rb

@@ -177,6 +177,8 @@ def perf_capture(interval_name, start_time = nil, end_time = nil)

    if start_range.nil?
      _log.info "#{log_header} Skipping processing for #{log_target} as no metrics were captured."
+      # Set the last capture on to end_time to prevent forever queueing up the same collection range
+      update_attribute(:last_perf_capture_on, end_time) if last_perf_capture_on.nil?


Looks good, although I think we want update or update_attributes so it does validations I believe

jrafanie · 2017-04-07T19:39:20Z

app/models/metric/ci_mixin/capture.rb

@@ -49,7 +49,7 @@ def perf_capture_queue(interval_name, options = {})
    cb = nil
    if interval_name == 'historical'
      start_time = Metric::Capture.historical_start_time if start_time.nil?
-      end_time = Time.now.utc if end_time.nil?
+      end_time = Time.now.utc.end_of_day # Ensure no more than one historical collection is queue up in the same day


minor typo: queued

I think it should be Time.tomorrow.beginning_of_day

Fryguy · 2017-04-07T19:42:06Z

app/models/metric/ci_mixin/capture.rb

@@ -49,7 +49,7 @@ def perf_capture_queue(interval_name, options = {})
    cb = nil
    if interval_name == 'historical'
      start_time = Metric::Capture.historical_start_time if start_time.nil?
-      end_time = Time.now.utc if end_time.nil?
+      end_time = Time.now.utc.end_of_day # Ensure no more than one historical collection is queue up in the same day


I'm curious why this happens... I would think the thing that queues up the historicals would put start/end pairs for every record.

Also, should this be an ||=. If an end_time is passed in, why wouldn't we honor it?

Fryguy · 2017-04-07T19:44:05Z

@gtanzillo @blomquisg Here's what I was talking about on the call, when I said that captures should never be open-ended for the start_time: https://github.com/ManageIQ/manageiq/pull/14695/files#diff-98e67a56bb0805013a9953cc93e47389R138 (seems it's only for hourly captures)

- Set end_time to the end of the current day before queueing historical collection to allow put match and update existing queue item - Set last_perf_capture_on to end time when no metrics were available to prevent constantly queueing the same historical range. https://bugzilla.redhat.com/show_bug.cgi?id=1436034

Fryguy · 2017-04-07T22:29:02Z

app/models/metric/ci_mixin/capture.rb

+        items = split_capture_intervals("historical", last_perf_capture_on, realtime_cut_off)
+      end
+      # push realtime item to the front of the array
+      items.unshift([interval_name, realtime_cut_off])


I think we only want to do this when the time is outside the realtime_cut_off

that is, I think something like

realtime_cut_off = 4.hours.ago.utc.beginning_of_day if last_perf_capture_on && last_perf_capture_on < realtime_cut_off items = [[interval_name, realtime_cut_off]] + split_capture_intervals("historical", last_perf_capture_on, realtime_cut_off) else items = [interval_name] end

In the case of VMware this will be fine, I think. Since the max we can get from the provider is 4.hours of data. However, for Hawkular, I think this breaks, since the max we can get from the provider is 7.days.

So, without a start date, I think it will try to get the maximum amount of data available. But, I'm not positive.

Edit

And, that leads us back to the "we should fix this in perf_capture instead of perf_capture_queue" discussion...

Fryguy · 2017-04-07T22:34:22Z

app/models/metric/ci_mixin/capture.rb

@@ -78,15 +86,17 @@ def perf_capture_queue(interval_name, options = {})
        if msg.nil?
          qi[:priority] = priority
          qi.delete(:state)
-          qi[:miq_callback] = cb if cb
+          if cb and item_interval == "realtime"


Prefer && over and ;)

Fryguy · 2017-04-07T22:34:54Z

app/models/metric/ci_mixin/capture.rb

@@ -177,6 +187,8 @@ def perf_capture(interval_name, start_time = nil, end_time = nil)

    if start_range.nil?
      _log.info "#{log_header} Skipping processing for #{log_target} as no metrics were captured."
+      # Set the last capture on to end_time to prevent forever queueing up the same collection range
+      update_attribute(:last_perf_capture_on, end_time) if interval_name == 'realtime'


This needs to be update_attributes instead of update_attribute

gtanzillo · 2017-04-07T22:43:57Z

I think we still need to make the intervals here https://github.com/gtanzillo/manageiq/blob/8b5bf7441789c5c1c829ff40edea1b3728440acf/app/models/metric/ci_mixin/capture.rb#L28 in split_capture_intervals land on day boundaries so that the queue args will match the next time we try to queue them.

The specs should test * a single day (where the realtime queue item should have no end time) * realtime over multiple days (where the older queue items are historical) * multiple attempts to queue realtime metrics for a single object in the same 20-minute window

blomquisg · 2017-04-08T01:43:05Z

The Time.now rubocops for the spec are for code that's frozen by Timecop. If you think it's necessary, I can update based on rubocop warnings.

…gle realtime capture.

miq-bot · 2017-04-10T15:02:36Z

Checked commits gtanzillo/manageiq@155ddd6~...004569f with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 5 offenses detected

spec/models/metric/ci_mixin/capture_spec.rb

❗ - Line 103, Col 29 - Rails/TimeZone - Do not use Time.now without zone. Use one of Time.zone.now, Time.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.to_i, Time.now.to_f instead.
❗ - Line 113, Col 29 - Rails/TimeZone - Do not use Time.now without zone. Use one of Time.zone.now, Time.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.to_i, Time.now.to_f instead.
❗ - Line 116, Col 29 - Rails/TimeZone - Do not use Time.now without zone. Use one of Time.zone.now, Time.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.to_i, Time.now.to_f instead.
❗ - Line 87, Col 29 - Rails/TimeZone - Do not use Time.now without zone. Use one of Time.zone.now, Time.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.to_i, Time.now.to_f instead.
❗ - Line 95, Col 29 - Rails/TimeZone - Do not use Time.now without zone. Use one of Time.zone.now, Time.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.to_i, Time.now.to_f instead.

Fix queueing of historical metrics collection (cherry picked from commit 1f13f1b) https://bugzilla.redhat.com/show_bug.cgi?id=1441204

simaishi · 2017-04-11T13:04:15Z

Euwe backport details:

 $ git log -1
commit 634ca989d05291825797c3c2ecc317d49135b025
Author: Jason Frey <fryguy9@gmail.com>
Date:   Mon Apr 10 14:21:37 2017 -0400

    Merge pull request #14695 from gtanzillo/fix-perf-capture_queue
    
    Fix queueing of historical metrics collection
    (cherry picked from commit 1f13f1b0aac6487d76e0885db1cbe43234e394f0)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1441204

Fix queueing of historical metrics collection (cherry picked from commit 1f13f1b) https://bugzilla.redhat.com/show_bug.cgi?id=1441203

simaishi · 2017-04-11T13:48:59Z

Fine backport details:

$ git log -1
commit 2f06bc215e7c19387e55482ac3b240455ea53e52
Author: Jason Frey <fryguy9@gmail.com>
Date:   Mon Apr 10 14:21:37 2017 -0400

    Merge pull request #14695 from gtanzillo/fix-perf-capture_queue
    
    Fix queueing of historical metrics collection
    (cherry picked from commit 1f13f1b0aac6487d76e0885db1cbe43234e394f0)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1441203

kbrock · 2017-04-07T22:40:42Z

app/models/metric/ci_mixin/capture.rb

    if interval_name == 'historical'
      start_time = Metric::Capture.historical_start_time if start_time.nil?
      end_time ||= 1.day.from_now.utc.beginning_of_day # Ensure no more than one historical collection is queue up in the same day
+      items = split_capture_intervals(interval_name, start_time, end_time)


kbrock · 2017-04-11T21:27:55Z

app/models/metric/ci_mixin/capture.rb

@@ -78,15 +89,17 @@ def perf_capture_queue(interval_name, options = {})
        if msg.nil?
          qi[:priority] = priority
          qi.delete(:state)
-          qi[:miq_callback] = cb if cb
+          if cb && item_interval == "realtime"


I didn't see this before. very nice

kbrock · 2017-04-11T21:30:01Z

app/models/metric/ci_mixin/capture.rb

          qi
        elsif msg.state == "ready" && (task_id || MiqQueue.higher_priority?(priority, msg.priority))
          qi[:priority] = priority
          # rerun the job (either with new task or higher priority)
          qi.delete(:state)
          if task_id
            existing_tasks = (((msg.miq_callback || {})[:args] || []).first) || []
-            qi[:miq_callback] = cb.merge(:args => [existing_tasks + [task_id]])
+            qi[:miq_callback] = cb.merge(:args => [existing_tasks + [task_id]]) if item_interval == "realtime"


If we can get rid of the check on upgrading priority, then it will be easier to go over to put_unless_exists.

Real-time will potentially still need this

Fix queueing of historical metrics collection (cherry picked from commit 1f13f1b) https://bugzilla.redhat.com/show_bug.cgi?id=1441204

gtanzillo added bug providers/containers wip labels Apr 7, 2017

gtanzillo requested review from kbrock, blomquisg, jrafanie and Fryguy April 7, 2017 19:11

gtanzillo force-pushed the fix-perf-capture_queue branch from 088e8d3 to fc88644 Compare April 7, 2017 19:11

jrafanie reviewed Apr 7, 2017

View reviewed changes

Fryguy self-assigned this Apr 7, 2017

Fryguy reviewed Apr 7, 2017

View reviewed changes

gtanzillo force-pushed the fix-perf-capture_queue branch from fc88644 to 155ddd6 Compare April 7, 2017 20:39

Splitting realtime collection into historical

8b5bf74

Fryguy reviewed Apr 7, 2017

View reviewed changes

gtanzillo added 2 commits April 7, 2017 22:40

Fix RuboCop - Use update_attributes instead of update_attribute

9bf613e

Handle case where last_perf_capture_on is nil and just queue up a sin…

004569f

…gle realtime capture.

gtanzillo changed the title ~~[WIP] Fix queueing of historical metrics collection~~ Fix queueing of historical metrics collection Apr 10, 2017

gtanzillo removed the wip label Apr 10, 2017

Fryguy merged commit 1f13f1b into ManageIQ:master Apr 10, 2017

Fryguy added this to the Sprint 58 Ending Apr 10, 2017 milestone Apr 10, 2017

gtanzillo added the euwe/yes label Apr 10, 2017

gtanzillo mentioned this pull request Apr 11, 2017

Additional tests for queueing of collection of historical metrics #14723

Merged

simaishi pushed a commit that referenced this pull request Apr 11, 2017

Merge pull request #14695 from gtanzillo/fix-perf-capture_queue

634ca98

Fix queueing of historical metrics collection (cherry picked from commit 1f13f1b) https://bugzilla.redhat.com/show_bug.cgi?id=1441204

simaishi added euwe/backported fine/yes and removed euwe/yes labels Apr 11, 2017

simaishi pushed a commit that referenced this pull request Apr 11, 2017

Merge pull request #14695 from gtanzillo/fix-perf-capture_queue

2f06bc2

Fix queueing of historical metrics collection (cherry picked from commit 1f13f1b) https://bugzilla.redhat.com/show_bug.cgi?id=1441203

simaishi added fine/backported and removed fine/yes labels Apr 11, 2017

kbrock reviewed Apr 11, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix queueing of historical metrics collection #14695

Fix queueing of historical metrics collection #14695

gtanzillo commented Apr 7, 2017

jrafanie Apr 7, 2017

jrafanie Apr 7, 2017

kbrock Apr 7, 2017

Fryguy Apr 7, 2017 •

edited

Loading

Fryguy Apr 7, 2017 •

edited

Loading

Fryguy commented Apr 7, 2017 •

edited

Loading

Fryguy Apr 7, 2017

blomquisg Apr 8, 2017 •

edited

Loading

Fryguy Apr 7, 2017

Fryguy Apr 7, 2017

gtanzillo commented Apr 7, 2017

blomquisg commented Apr 8, 2017

miq-bot commented Apr 10, 2017

simaishi commented Apr 11, 2017

simaishi commented Apr 11, 2017

kbrock Apr 7, 2017

kbrock Apr 11, 2017

kbrock Apr 11, 2017

Fix queueing of historical metrics collection #14695

Fix queueing of historical metrics collection #14695

Conversation

gtanzillo commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Fryguy Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Fryguy commented Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

blomquisg Apr 8, 2017 • edited Loading

Choose a reason for hiding this comment

Edit

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtanzillo commented Apr 7, 2017

blomquisg commented Apr 8, 2017

miq-bot commented Apr 10, 2017

simaishi commented Apr 11, 2017

simaishi commented Apr 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy Apr 7, 2017 •

edited

Loading

Fryguy Apr 7, 2017 •

edited

Loading

Fryguy commented Apr 7, 2017 •

edited

Loading

blomquisg Apr 8, 2017 •

edited

Loading