Fix Containers dashboard heatmaps #14857

zakiva · 2017-04-24T14:27:17Z

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1439671
UI side and tests in ManageIQ/manageiq-ui-classic#608

@simon3z @zeari @moolitayer Please review

@miq-bot add_label providers/containers, bug, fine/yes

zeari

LGTM. @kbrock can you have a look? its the same as https://github.com/ManageIQ/manageiq/blob/master/app/models/metric_rollup.rb#L48

zakiva · 2017-05-10T10:21:48Z

@kbrock Can you please take a look?

simon3z · 2017-05-18T08:48:09Z

app/models/metric/helper.rb

+    metrics = Metric.where(:resource_type => resource_type)
+    metrics = metrics.where(:resource_id => resource_ids) if resource_ids
+    metrics = metrics.order(:resource_id, :timestamp => :desc)
+    metrics = metrics.where('timestamp > ?', 10.minutes.ago.utc)


@zakiva why 10.minutes.ago.utc? Is it dependent on any other value?

@simon3z That is the time interval we agreed on for "realtime" metrics that we display in the dashboard. Same value is used here:
https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/services/container_dashboard_service.rb#L395
(we can change it easily of course)

@zakiva then it seems to me that this should be a single constant in the code (or a setting?) instead that having 10.minutes.ago.utc hard-coded, so in case we need to tweak it we change the value in a single place.

Right, I will fix that.

simon3z · 2017-05-18T08:49:31Z

app/models/metric/helper.rb

@@ -198,4 +198,12 @@ def self.get_time_interval(obj, timestamp)

    start_time..timestamp
  end
+
+  def self.latest_metrics(resource_type, resource_ids = nil)
+    metrics = Metric.where(:resource_type => resource_type)


@kbrock any way to make the syntax of the whole query (these 5 lines) nicer? Any syntactic sugar?

Yea, not perfect, but it will work.

simon3z · 2017-05-18T08:49:46Z

@kbrock can you review this?

simon3z · 2017-05-18T08:51:21Z

@Fryguy can you take a look? Thanks.

simon3z · 2017-05-22T10:09:26Z

@miq-bot assign kbrock

simon3z · 2017-05-22T10:10:36Z

app/models/metric/helper.rb

@@ -198,4 +198,12 @@ def self.get_time_interval(obj, timestamp)

    start_time..timestamp
  end
+
+  def self.latest_metrics(resource_type, timestamp, resource_ids = nil)


@zakiva is it more clear to use since_timestamp instead of just timestamp?

miq-bot · 2017-05-22T11:13:13Z

Checked commit zakiva@12ad5b5 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

simon3z · 2017-05-22T14:06:18Z

@kbrock can you review/approve/merge?
Thanks.

kbrock · 2017-05-31T20:47:09Z

app/models/metric/helper.rb

+    metrics = metrics.where(:resource_id => resource_ids) if resource_ids
+    metrics = metrics.order(:resource_id, :timestamp => :desc)
+    metrics = metrics.where('timestamp > ?', since_timestamp)
+    metrics.select('DISTINCT ON (metrics.resource_id) metrics.*')


think metrics.distinct(:resource_id) is an alternative

Fix Containers dashboard heatmaps (cherry picked from commit 5d0ec5b) https://bugzilla.redhat.com/show_bug.cgi?id=1460357

simaishi · 2017-06-09T20:15:23Z

Fine backport details:

$ git log -1
commit 5fc3450f3cd5ac77b24c1103fb9cabbaee91c3e3
Author: Keenan Brock <[email protected]>
Date:   Wed May 31 16:47:20 2017 -0400

    Merge pull request #14857 from zakiva/fix_realtime_heatmaps
    
    Fix Containers dashboard heatmaps
    (cherry picked from commit 5d0ec5b2e81f0e7666d08b9cae8c9df39cb60975)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1460357

chrisarcand · 2017-08-03T18:18:19Z

Why the added 'helper' here? This module is already a bucket of random things. Seems like this could have been a few scopes on the Metric model with the helper in ui-classic utilizing them.

simon3z · 2017-08-25T15:03:14Z

Why the added 'helper' here? This module is already a bucket of random things. Seems like this could have been a few scopes on the Metric model with the helper in ui-classic utilizing them.

@zakiva how hard is this refactoring? Is it just moving some code around?
@chrisarcand if you can coordinate a quick merge of the code in the two repos we can refactor this for master.

zakiva · 2017-08-28T13:12:13Z

@chrisarcand I'm not sure I understand you right, can you please explain what change exactly would you like to see here? Back then a similar code was in the ui-classic repo and I was asked to move it here.

chrisarcand · 2017-08-28T15:01:57Z

@zakiva This code is basically just layers of scoping on the metrics model. I'm suggesting actually using ActiveRecord scopes instead of this class method.

Forget about it, it's not really worth correcting at this point. I also understand if it's mostly just a code move that you were asked to do :)

zakiva mentioned this pull request Apr 24, 2017

Containers Dashboard: Show one square per node in realtime heatmaps ManageIQ/manageiq-ui-classic#608

Merged

miq-bot added bug fine/yes providers/containers labels Apr 24, 2017

zakiva force-pushed the fix_realtime_heatmaps branch from 6a8ebf3 to 7b9f2b6 Compare April 24, 2017 16:57

chessbyte assigned simon3z Apr 25, 2017

zeari approved these changes May 1, 2017

View reviewed changes

simon3z reviewed May 18, 2017

View reviewed changes

zakiva force-pushed the fix_realtime_heatmaps branch from 7b9f2b6 to 89867aa Compare May 21, 2017 07:50

miq-bot assigned kbrock and unassigned simon3z May 22, 2017

simon3z reviewed May 22, 2017

View reviewed changes

simon3z approved these changes May 22, 2017

View reviewed changes

zakiva force-pushed the fix_realtime_heatmaps branch from 89867aa to 12ad5b5 Compare May 22, 2017 11:09

Fix Containers dashboard heatmaps

12ad5b5

kbrock reviewed May 31, 2017

View reviewed changes

kbrock merged commit 5d0ec5b into ManageIQ:master May 31, 2017

kbrock added this to the Sprint 62 Ending Jun 5, 2017 milestone May 31, 2017

simaishi pushed a commit that referenced this pull request Jun 9, 2017

Merge pull request #14857 from zakiva/fix_realtime_heatmaps

5fc3450

Fix Containers dashboard heatmaps (cherry picked from commit 5d0ec5b) https://bugzilla.redhat.com/show_bug.cgi?id=1460357

simaishi added fine/backported and removed fine/yes labels Jun 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Containers dashboard heatmaps #14857

Fix Containers dashboard heatmaps #14857

zakiva commented Apr 24, 2017 •

edited

Loading

zeari left a comment

zakiva commented May 10, 2017

simon3z May 18, 2017

zakiva May 18, 2017

simon3z May 18, 2017

zakiva May 18, 2017

simon3z May 18, 2017

kbrock May 18, 2017

simon3z commented May 18, 2017

simon3z commented May 18, 2017

simon3z commented May 22, 2017

simon3z May 22, 2017

miq-bot commented May 22, 2017

simon3z commented May 22, 2017

kbrock May 31, 2017

simaishi commented Jun 9, 2017

chrisarcand commented Aug 3, 2017

simon3z commented Aug 25, 2017

zakiva commented Aug 28, 2017

chrisarcand commented Aug 28, 2017

Fix Containers dashboard heatmaps #14857

Fix Containers dashboard heatmaps #14857

Conversation

zakiva commented Apr 24, 2017 • edited Loading

zeari left a comment

Choose a reason for hiding this comment

zakiva commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simon3z commented May 18, 2017

simon3z commented May 18, 2017

simon3z commented May 22, 2017

Choose a reason for hiding this comment

miq-bot commented May 22, 2017

simon3z commented May 22, 2017

Choose a reason for hiding this comment

simaishi commented Jun 9, 2017

chrisarcand commented Aug 3, 2017

simon3z commented Aug 25, 2017

zakiva commented Aug 28, 2017

chrisarcand commented Aug 28, 2017

zakiva commented Apr 24, 2017 •

edited

Loading