Add differential chart option to charts #1367

yaacov · 2017-05-17T12:55:19Z

Description

Add rate / differential chart option to charts

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

Motive

Some metrics only come in counter/totals form (especially in Prometheus), we need a rate view to better understand/compare them.

Screenshot

yaacov · 2017-05-17T12:56:03Z

@miq-bot add_label providers/containers

@simon3z @zakiva @moolitayer @cben @himdel please review

miq-bot · 2017-05-17T12:56:13Z

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

yaacov · 2017-05-17T12:57:14Z

@miq-bot add_label compute/containers

simon3z · 2017-05-17T13:11:55Z

app/assets/javascripts/controllers/live_metrics/util/metrics-utils-factory.js

@@ -2,6 +2,16 @@ angular.module('miq.util').factory('metricsUtilsFactory', function() {
  return function (dash) {
    var UNKNOWN_ERROR_STR = __('Something is wrong, try reloading the page');

+    function dataDiff(data) {
+      data.forEach(function(v, i) {
+        if (data[i] && data[i + 1]) {


@yaacov what if either data[i] or data[i + 1] is really 0?

👍 thanks, replaced with explicit !== null

simon3z · 2017-05-17T13:12:33Z

@yaacov at least dataDiff needs tests.

simon3z · 2017-05-17T13:12:59Z

app/assets/javascripts/controllers/live_metrics/util/metrics-utils-factory.js

@@ -2,6 +2,16 @@ angular.module('miq.util').factory('metricsUtilsFactory', function() {
  return function (dash) {
    var UNKNOWN_ERROR_STR = __('Something is wrong, try reloading the page');

+    function dataDiff(data) {


@yaacov let's use a less generic name.

👍 Thanks, replaced with inplaceCalcDataDifferentials :-)

simon3z · 2017-05-17T15:16:02Z

app/assets/javascripts/controllers/live_metrics/util/metrics-utils-factory.js

+    function inplaceCalcDataDifferentials(data) {
+      data.forEach(function(v, i) {
+        if ((data[i] !== null) && (data[i + 1] !== null)) {
+          data[i] = data[i + 1] - data[i];


@yaacov I am not sure if we want to do this inplace... wouldn't this function always result in an array where the last item is null?

@simon3z

I am not sure if we want to do this inplace

inplace saves memory and pushing data between functions ...

wouldn't this function always result in an array where the last item is null?

yes, each consecutive pair becomes one item, the null is left in the end.

yaacov · 2017-05-18T13:13:19Z

Following meeting today, change "Differentials" to "Rate"

cc// @simon3z

simon3z · 2017-05-22T08:14:36Z

@yaacov a couple of comments:

doing an in-place differential seems premature optimization (how many data-points do you expect at max? Arrays of base types should be cheap)
if you keep the original datapoints vs rate in two different variables you can switch back and forth without any interaction with the backend (seems a major advantage for connection that are not fast)

yaacov · 2017-05-22T13:49:23Z

@simon3z
a. Changed the inplace implementation to regular one :-)
b. Thinking how to do it without reloading ...

miq-bot · 2017-05-25T21:19:17Z

This pull request is not mergeable. Please rebase and repush.

yaacov · 2017-05-26T08:58:32Z

@simon3z @himdel please re-review

Now redraw does not require a new ajax request.

yaacov · 2017-06-01T10:15:10Z

@simon3z @himdel please re-review

simon3z · 2017-06-01T11:58:39Z

LGTM 👍

@miq-bot assign himdel
cc @himdel

simon3z · 2017-06-01T11:59:12Z

@yaacov do we have a BZ for this? If not we should (open one, thanks).

yaacov · 2017-06-01T13:22:20Z

do we have a BZ for this? If not we should (open one, thanks).

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

yaacov · 2017-06-04T07:11:28Z

@miq-bot add_label fine/yes

https://bugzilla.redhat.com/show_bug.cgi?id=1457893 target 5.8.1

yaacov · 2017-06-06T06:48:37Z

@himdel please re-review

miq-bot · 2017-06-06T14:33:43Z

This pull request is not mergeable. Please rebase and repush.

himdel · 2017-06-06T15:59:28Z

LGTM 👍 would merge, but seems the JS specs are failing, looks relevant.

yaacov · 2017-06-06T16:14:04Z

but seems the JS specs are failing, looks relevant.

@himdel
yes, I wrote unitLable instead of unitLabel in the spec :-)

miq-bot · 2017-06-07T03:34:01Z

Checked commit yaacov@0131b20 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks fine. 🍪

yaacov · 2017-06-07T08:18:26Z

@himdel all green now

simaishi · 2017-06-19T16:09:40Z

@yaacov there are a bit of conflicts backporting to Fine. Would you mind creating a Fine PR?

$ git status
On branch fine
Your branch is up-to-date with 'origin/fine'.
You are currently cherry-picking commit cc24e00.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:

	modified:   app/assets/javascripts/controllers/live_metrics/util/metrics-http-factory.js
	modified:   app/views/ems_container/ad_hoc/_chart_view_form.html.haml

Unmerged paths:
  (use "git add <file>..." to mark resolution)

	both modified:   app/assets/javascripts/controllers/live_metrics/ad_hoc_metrics_controller.js
	both modified:   app/assets/javascripts/controllers/live_metrics/util/metrics-utils-factory.js
	both modified:   app/assets/stylesheets/metrics.scss
	both modified:   spec/javascripts/controllers/container_live_dashboard/container_live_dashboard_controller_spec.js

yaacov · 2017-06-20T11:55:34Z

@yaacov there are a bit of conflicts backporting to Fine. Would you mind creating a Fine PR?

👍

A Fine PR: #1571

simaishi · 2017-08-04T16:02:17Z

Backported to Fine via #1600

miq-bot added the compute/containers label May 17, 2017

simon3z reviewed May 17, 2017

View reviewed changes

yaacov force-pushed the add-diff-option-to-charts branch 2 times, most recently from 15ff186 to 0f0af69 Compare May 17, 2017 15:15

simon3z reviewed May 17, 2017

View reviewed changes

yaacov force-pushed the add-diff-option-to-charts branch 5 times, most recently from 2858622 to a31762c Compare May 17, 2017 16:09

chessbyte assigned simon3z May 19, 2017

yaacov force-pushed the add-diff-option-to-charts branch from 66d706d to 427f90f Compare May 22, 2017 13:47

miq-bot added the unmergeable label May 25, 2017

yaacov force-pushed the add-diff-option-to-charts branch from 427f90f to 1e88372 Compare May 26, 2017 08:40

miq-bot removed the unmergeable label May 26, 2017

yaacov force-pushed the add-diff-option-to-charts branch from 1e88372 to 36cf08e Compare May 26, 2017 08:49

yaacov closed this May 26, 2017

yaacov reopened this May 26, 2017

miq-bot assigned himdel and unassigned simon3z Jun 1, 2017

miq-bot added the fine/yes label Jun 4, 2017

miq-bot added the unmergeable label Jun 6, 2017

yaacov force-pushed the add-diff-option-to-charts branch from 36cf08e to 7926d7f Compare June 6, 2017 14:48

miq-bot removed the unmergeable label Jun 6, 2017

yaacov force-pushed the add-diff-option-to-charts branch from 7926d7f to 8029d14 Compare June 6, 2017 16:11

Add differential chart option to charts

0131b20

yaacov force-pushed the add-diff-option-to-charts branch from 8029d14 to 0131b20 Compare June 7, 2017 03:29

himdel merged commit cc24e00 into ManageIQ:master Jun 7, 2017

himdel added this to the Sprint 63 Ending Jun 19, 2017 milestone Jun 7, 2017

himdel added the enhancement label Jun 7, 2017

simaishi added the fine/conflict label Jun 19, 2017

yaacov mentioned this pull request Jun 20, 2017

[FINE] Add differential chart option to charts #1571

Closed

simaishi removed the fine/conflict label Jun 20, 2017

yaacov mentioned this pull request Jun 25, 2017

[FINE] Fix units bug and add differential #1600

Merged

simaishi added fine/backported and removed fine/yes labels Aug 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add differential chart option to charts #1367

Add differential chart option to charts #1367

yaacov commented May 17, 2017 •

edited

Loading

yaacov commented May 17, 2017

miq-bot commented May 17, 2017

yaacov commented May 17, 2017

simon3z May 17, 2017

yaacov May 17, 2017

simon3z commented May 17, 2017

simon3z May 17, 2017

yaacov May 17, 2017

simon3z May 17, 2017

yaacov May 17, 2017

yaacov commented May 18, 2017

simon3z commented May 22, 2017

yaacov commented May 22, 2017

miq-bot commented May 25, 2017

yaacov commented May 26, 2017

yaacov commented Jun 1, 2017

simon3z commented Jun 1, 2017

simon3z commented Jun 1, 2017

yaacov commented Jun 1, 2017

yaacov commented Jun 4, 2017

yaacov commented Jun 6, 2017

miq-bot commented Jun 6, 2017

himdel commented Jun 6, 2017

yaacov commented Jun 6, 2017

miq-bot commented Jun 7, 2017

yaacov commented Jun 7, 2017

simaishi commented Jun 19, 2017

yaacov commented Jun 20, 2017

simaishi commented Aug 4, 2017

Add differential chart option to charts #1367

Add differential chart option to charts #1367

Conversation

yaacov commented May 17, 2017 • edited Loading

yaacov commented May 17, 2017

miq-bot commented May 17, 2017

yaacov commented May 17, 2017

simon3z May 17, 2017

Choose a reason for hiding this comment

yaacov May 17, 2017

Choose a reason for hiding this comment

simon3z commented May 17, 2017

simon3z May 17, 2017

Choose a reason for hiding this comment

yaacov May 17, 2017

Choose a reason for hiding this comment

simon3z May 17, 2017

Choose a reason for hiding this comment

yaacov May 17, 2017

Choose a reason for hiding this comment

yaacov commented May 18, 2017

simon3z commented May 22, 2017

yaacov commented May 22, 2017

miq-bot commented May 25, 2017

yaacov commented May 26, 2017

yaacov commented Jun 1, 2017

simon3z commented Jun 1, 2017

simon3z commented Jun 1, 2017

yaacov commented Jun 1, 2017

yaacov commented Jun 4, 2017

yaacov commented Jun 6, 2017

miq-bot commented Jun 6, 2017

himdel commented Jun 6, 2017

yaacov commented Jun 6, 2017

miq-bot commented Jun 7, 2017

yaacov commented Jun 7, 2017

simaishi commented Jun 19, 2017

yaacov commented Jun 20, 2017

simaishi commented Aug 4, 2017

yaacov commented May 17, 2017 •

edited

Loading