Containers Dashboard: Show hourly and realtime trends #33

zakiva · 2016-12-25T12:56:34Z

In case that daily trends are not available, we would like to display hourly trends (last 24 hours) if available, or realtime trends (last 10 minutes) instead.
For Image Usage and Pod Creation and Deletion cards I added only hourly trends as we still don't collect the relevant realtime data in the Metric table. This can be added in a follow-up PR once we have the backend functionality.

zakiva · 2016-12-25T18:44:44Z

@simon3z @zeari @yaacov @dkorn Please review

zakiva · 2016-12-25T18:47:10Z

@miq-bot add_label enhancement, providers/containers

miq-bot · 2016-12-25T18:47:13Z

@zakiva Cannot apply the following label because they are not recognized: providers/containers

simon3z · 2016-12-27T16:10:40Z

@zakiva any way to fix the warnings?

zakiva · 2017-01-01T10:29:57Z

@zakiva any way to fix the warnings?

Fixed most of them.
The long lines used to be longer before this commit and I don't think they can be split.
As for the long methods, I believe they should not be changed: I extracted some of the existing code into helper methods in order to reuse it, so actually they now have less lines.

simon3z · 2017-01-02T16:14:21Z

@zakiva please have this reviewed. cc @zeari @cben @yaacov @moolitayer

simon3z · 2017-01-02T16:14:46Z

@miq-bot assign zakiva

yaacov · 2017-01-02T16:27:17Z

LGTM 👍

p.s.
If you can fix the rubocop things, even if you inherited the warnings from previous code, it will be nicer.

zeari · 2017-01-03T10:00:51Z

@himdel Can you take a look here?

himdel · 2017-01-03T10:09:35Z

app/assets/javascripts/controllers/container_dashboard/container_dashboard_controller.js

-                                                                      "dates",
-                                                                      $scope.memoryUsageConfig.units);
+          if (data.daily_ems_utilization.cpu != undefined) {
+              $scope.cpuUsageData = chartsMixin.processUtilizationData(data.daily_ems_utilization.cpu,


broken formatting, please set your editor to 2 spaces, not 4

(multiple places with the same issue..)

himdel · 2017-01-03T10:10:14Z

app/assets/javascripts/controllers/container_dashboard/container_dashboard_controller.js

+              $scope.cpuUsageData = chartsMixin.processUtilizationData(data.hourly_ems_utilization.cpu,
+                                                                       "dates",
+                                                                       $scope.cpuUsageConfig.units);
+              $scope.cpuUsageConfig.timeFrame = 'Last 24 hours';


missing i18n .. you probably want __('Last 24 hours') (double underscore)

(multiple places with the same issue..)

himdel · 2017-01-03T10:19:49Z

app/views/shared/views/_show_containers_dashboard.html.haml

@@ -78,7 +78,7 @@
                   "pf-utilization-trend-chart" => "",
                   "sparkline-config"           => "cpuUsageSparklineConfig"}
            %span.trend-footer-pf{"ng-class" => "{ 'chart-transparent-text': !cpuUsageData }"}
-              = _("Last 30 Days")
+              = _("{{cpuUsageConfig.timeFrame}}")


No point in translating "{{cpuUsageConfig.timeFrame}}" .. the only thing this can accomplish is that the translators will break your code ;).

If the actual string is defined in the frontend, you can't very well translate in on the backend..

(multiple places with the same issue..)

himdel · 2017-01-03T10:33:13Z

Sorry @zakiva , I have skipped reviewing most of the JS code because of broken formatting, please let me know when this is fixed and I'll give it another spin.

But.. from what I can tell, seems like there's too much code duplication.. it seems like you're uselessly copying all the code into both branches of those conditions, instead of simply picking the right field in the if, and keeping the rest of code outside..

Another thing, != undefined feels confusing. Those values are null, not undefined, aren't they? Or maybe you really wanted !==?

Fryguy · 2017-01-05T04:31:32Z

Sorry @zakiva , but I'm going to spam this thread to test the bot. Apologies for the emails.

Fryguy · 2017-01-05T06:35:29Z

@miq-bot assign zakiva

miq-bot · 2017-01-05T06:36:15Z

@Fryguy zakiva is an invalid user, ignoring...

himdel

broken formatting and i18n, see comments please :)

zakiva · 2017-02-22T11:50:26Z

The containerDashboardController (JS) code has too much duplication and inconsistencies:

hourly is assuming xy_data always exists, whereas realtime is assuming it can be null. No idea what daily does, seems the code is now not there (possibly daily doesn't use xy_data?).
you're doing the same data.ems_utilization.xy_data.cpu.xData = data.ems_utilization.xy_data.cpu.xData.map(function (date) { return dashboardUtilsFactory.parseDate(date) }); for cpu and mem, for ems_utilization and network_metrics and pod_metrics and image_metrics.. yet it's the same code in all those if branches - so why not pull it out and do it just once?
server-side code sometimes has a default value for xy_data, sometimes it doesn't (hence the checks for null, presumably)

@himdel I made some fixes: pulled out the date parsing out of the if branches and tried to make the code more consistent and with less code duplication. The data is not necessarily available, server-side code may return daily/hourly/realtime data or no data at all, so I had to check for null before parsing the dates.
I hope the code looks better now, please let me know if there are still fixes to be done.

zakiva · 2017-02-22T11:52:00Z

@miq-bot remove_label wip

simon3z · 2017-02-27T17:24:20Z

@zakiva can you handle the travis failures?

simon3z · 2017-02-28T23:13:19Z

@zakiva what's the status here?
It seems that Travis is still failing and the review is blocked on a change request by @himdel (still actual?).

zakiva · 2017-03-01T09:31:48Z

@zakiva what's the status here?
It seems that Travis is still failing and the review is blocked on a change request by @himdel (still actual?).

@simon3z I tried to look into the Travis failures but it seems like they not related to this PR, I also rebased but they are still there :\
As commented above, I made some fixes following the review which I hope resolved all the requested changes. @himdel Can you please take another look?

simon3z · 2017-03-01T09:34:43Z

@simon3z I tried to look into the Travis failures but it seems like they not related to this PR, I also rebased but they are still there :\

Recently @cben and @enoodle dealt with unrelated travis failures, maybe they can help here.

enoodle · 2017-03-01T09:37:28Z

@zakiva I had a similar problem - Travis is actually testing a commit not related to the PR (look at the Travis page it writes which commit it tests. it should be a commit merging master and your commit but in this case it is something else entirely). My only solution was to keep rebasing or opening a new PR.

miq-bot · 2017-03-01T09:58:04Z

Checked commit zakiva@6902ba8 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 4 offenses detected

app/services/container_dashboard_service.rb

❕ - Line 129, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for heatmaps_data is too high. [41.55/20]
❕ - Line 129, Col 3 - Metrics/CyclomaticComplexity - Cyclomatic complexity for heatmaps_data is too high. [7/6]
❕ - Line 164, Col 27 - Metrics/ParameterLists - Avoid parameter lists longer than 5 parameters.
❕ - Line 226, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for ems_utilization_data is too high. [23.02/20]

fixed

himdel · 2017-03-01T11:57:58Z

Yup, looks like travis is doing it again :( ...

If this is something we want merged soon, @zakiva can you open a new PR please?

(The code looks much more consistent now, thanks! I'll test in the UI and merge if everything works..)

himdel · 2017-03-01T12:06:06Z

(tried to clean travis' cache for this PR and restart the specs, we'll see if that works..)

EDIT: apparently not :( .. so.. the new pr thing..

himdel · 2017-03-01T12:19:00Z

Also, @zakiva please update the PR description with instructions on how to find the thing in the UI.

EDIT: (although, now that I read the title, "container dashboard" is probably descriptive enough, just not where I expected to find it)

zakiva · 2017-03-01T14:39:39Z

@himdel @enoodle Thanks, I opened a new PR: #519.
Closing.

zakiva force-pushed the dashboard branch from 1ec1b91 to d21615c Compare December 25, 2016 13:13

zakiva mentioned this pull request Dec 25, 2016

[WIP] Containers Dashboard: Support hourly and realtime trends ManageIQ/manageiq#13112

Closed

miq-bot added the enhancement label Dec 25, 2016

zakiva force-pushed the dashboard branch from d21615c to 41fd722 Compare December 27, 2016 12:35

zakiva force-pushed the dashboard branch from 41fd722 to bf3dcab Compare January 1, 2017 10:16

zakiva force-pushed the dashboard branch 2 times, most recently from 9a7a71b to 199fac0 Compare January 2, 2017 11:41

himdel reviewed Jan 3, 2017

View reviewed changes

himdel self-assigned this Jan 3, 2017

Fryguy assigned zakiva and unassigned zakiva Jan 5, 2017

himdel previously requested changes Jan 6, 2017

View reviewed changes

zakiva force-pushed the dashboard branch 3 times, most recently from 204c4c2 to 1bd2bf7 Compare January 9, 2017 17:53

zakiva force-pushed the dashboard branch from 765e1ec to f788390 Compare February 22, 2017 11:17

zakiva changed the title ~~[WIP] Containers Dashboard: Show hourly and realtime trends~~ Containers Dashboard: Show hourly and realtime trends Feb 22, 2017

miq-bot removed the wip label Feb 22, 2017

zakiva force-pushed the dashboard branch 3 times, most recently from 692b81d to 5a75e26 Compare February 23, 2017 13:12

zakiva force-pushed the dashboard branch 2 times, most recently from 4cc3968 to 5e8facf Compare February 28, 2017 15:49

Containers Dashboard: Show hourly and realtime trends

6902ba8

zakiva force-pushed the dashboard branch from 5e8facf to 6902ba8 Compare March 1, 2017 08:31

zakiva closed this Mar 1, 2017

zakiva reopened this Mar 1, 2017

himdel closed this Mar 1, 2017

himdel reopened this Mar 1, 2017

himdel closed this Mar 1, 2017

himdel reopened this Mar 1, 2017

zakiva closed this Mar 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Containers Dashboard: Show hourly and realtime trends #33

Containers Dashboard: Show hourly and realtime trends #33

zakiva commented Dec 25, 2016 •

edited by miq-bot

Loading

zakiva commented Dec 25, 2016 •

edited

Loading

zakiva commented Dec 25, 2016

miq-bot commented Dec 25, 2016

simon3z commented Dec 27, 2016

zakiva commented Jan 1, 2017

simon3z commented Jan 2, 2017

simon3z commented Jan 2, 2017

yaacov commented Jan 2, 2017

zeari commented Jan 3, 2017

himdel Jan 3, 2017 •

edited

Loading

himdel Jan 3, 2017 •

edited

Loading

himdel Jan 3, 2017 •

edited

Loading

himdel commented Jan 3, 2017

Fryguy commented Jan 5, 2017

Fryguy commented Jan 5, 2017

miq-bot commented Jan 5, 2017

himdel left a comment

zakiva commented Feb 22, 2017

zakiva commented Feb 22, 2017

simon3z commented Feb 27, 2017

simon3z commented Feb 28, 2017

zakiva commented Mar 1, 2017

simon3z commented Mar 1, 2017

enoodle commented Mar 1, 2017

miq-bot commented Mar 1, 2017

himdel commented Mar 1, 2017

himdel commented Mar 1, 2017 •

edited

Loading

himdel commented Mar 1, 2017 •

edited

Loading

zakiva commented Mar 1, 2017

Containers Dashboard: Show hourly and realtime trends #33

Containers Dashboard: Show hourly and realtime trends #33

Conversation

zakiva commented Dec 25, 2016 • edited by miq-bot Loading

zakiva commented Dec 25, 2016 • edited Loading

zakiva commented Dec 25, 2016

miq-bot commented Dec 25, 2016

simon3z commented Dec 27, 2016

zakiva commented Jan 1, 2017

simon3z commented Jan 2, 2017

simon3z commented Jan 2, 2017

yaacov commented Jan 2, 2017

zeari commented Jan 3, 2017

himdel Jan 3, 2017 • edited Loading

Choose a reason for hiding this comment

himdel Jan 3, 2017 • edited Loading

Choose a reason for hiding this comment

himdel Jan 3, 2017 • edited Loading

Choose a reason for hiding this comment

himdel commented Jan 3, 2017

Fryguy commented Jan 5, 2017

Fryguy commented Jan 5, 2017

miq-bot commented Jan 5, 2017

himdel left a comment

Choose a reason for hiding this comment

zakiva commented Feb 22, 2017

zakiva commented Feb 22, 2017

simon3z commented Feb 27, 2017

simon3z commented Feb 28, 2017

zakiva commented Mar 1, 2017

simon3z commented Mar 1, 2017

enoodle commented Mar 1, 2017

miq-bot commented Mar 1, 2017

himdel commented Mar 1, 2017

himdel commented Mar 1, 2017 • edited Loading

himdel commented Mar 1, 2017 • edited Loading

zakiva commented Mar 1, 2017

zakiva commented Dec 25, 2016 •

edited by miq-bot

Loading

zakiva commented Dec 25, 2016 •

edited

Loading

himdel Jan 3, 2017 •

edited

Loading

himdel Jan 3, 2017 •

edited

Loading

himdel Jan 3, 2017 •

edited

Loading

himdel commented Mar 1, 2017 •

edited

Loading

himdel commented Mar 1, 2017 •

edited

Loading