Change metric accuracy in container projects dashboard

[zeari]

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1509956
Depends on: #2677

Before:

After:

@moolitayer @himdel
@miq-bot add_label compute/containers, gaprindashvili/yes

[zeari]

@himdel Before you say anything, its in my backlog to merge code from ContainerServiceMixin and EmsInfraDashboardService. Hopefully ill get to that and #2499 together 😰 . Projects dashboard is still new and im figuring out the code as we go.

[moolitayer]

EDIT: 2/0?

[moolitayer]

Just a thought, would this read clearer if we would not use variables?
cc @cben

[cben]

At quick glance (didn't read whole PR) I think the consts help. However I'm not sure what "graph usage" means.

[moolitayer]

OK so vars it is. for GRAPH_USAGE_PRECISION I suggested GRAPH_PRECISION below because it makes sense to have one precision for graphs (be it usage or others)

[moolitayer]

@zeari what should be the value of DISPLAY_PRECISION here?

[zeari]

0 by default

[moolitayer]

This feels like it can be more generic. This is the precision we would want for all graph values so maybe this can be GRAPH_PRECISION?

[zeari]

👍

[moolitayer]

Explicit argument for round here too?

[zeari]

Yeah, thats total cpu cores. it cant be a fraction.

[himdel]

Something's weird :).

I understand that you're trying to introduce display precision for displaying data, and graph precision for charts.

But.. why then is ContainerServiceMixin using a different value for DISPLAY_PRECISION? (Or is that the bug, since the value is 0 but the comment says 2? ;))

But either way.. constants are weird in ruby, with respect to inheritance... (and mixins)

So.. if the idea is to have them defined in ContainerServiceMixin and used everywhere, no issue there, but please remove those DISPLAY_PRECISION = 2 from the other services.

If the idea is to have one value for display precision and one for graph precision, in all the code, with support for individual dashboards overiding this, then make those into methods please, so that we get consistent behaviour.

(Right now, that DISPLAY_PRECISION=2 does absolutely nothing in ContainerDashboardService, because for methods defined in ContainerServiceMixin, the value of 0 is used anyway.)

[zeari]

(Right now, that DISPLAY_PRECISION=2 does absolutely nothing in ContainerDashboardService, because for methods defined in ContainerServiceMixin, the value of 0 is used anyway.)

Well then by some ruby magic it works as expected 😄 🎩 . The point is having different accuracy for different parts of dashboards. For projects dashboard I set higher precision since some projects have very low usage and it sucks seeing zeros. For providers we want to have lower accuracy since the patternfly chart does an available = total - used calculation and with high accuracy we get ugly values like 5.1500000000007 . We would like the same chart accuracy for both.
I wanted to avoid dealing with floats on the JS side and couldnt find the right way to override the precision on ruby side with these mixins in place. Any suggestions welcome...

[zeari]

Overriding here instead....

[himdel]

..which brings me back to the original point .. please don't use constants when you want to override.

Use methods.

Because this is worse now .. not only do I have to look which constant is defined where with those multiple definiitons. I also have to look at an extra instance variable.

EDIT: actually, sorry, not worse, I didn't notice you removed those extra definitions..
But still... could be clearer :)

[himdel]

(But if for some reason we need to stay with constants - then please add some specs?
Because I can no longer really tell which what should be rounded how much, with the code as is.)

[himdel]

... OK, went through it once more ...

ContainerDashboardService - only uses GRAPH_PRECISION (=2) (in heatmap_data)
ContainerProjectDashboardService - doesn't use any, overrides custom display precision
ContainerServiceMixin - uses both GRAPH_PRECISION (=2) and @custom_display_precision || DISPLAY_PRECISION (in utilization_data).

• => ContainerDashboardService uses utilization_data with display precision = 0
• => ContainerProjectDashboardService uses utilization_data with display precision = 2

(not affected by these changes)
EmsInfraDashboardService - ems_utilization uses round(0) for both kinds

[himdel]

I would still love to see a solution which brings the services closer together, not further apart.

---

container heatmap
  cpu|mem total - 0
  cpu|mem percent - GRAPH =2

infra heatmaps
  cpu|mem total - 0
  cpu|mem percent - CPU =2

container utilization
  cpu|mem used - DISPLAY =0
  cpu total - 0
  mem total - DISPLAY  =0
  cpu|mem xData/yData - GRAPH  =2

project utiliziation
  cpu|mem used - DISPLAY  =2
  cpu total - 0
  mem total - DISPLAY  =2
  cpu|mem xData/yData - GRAPH  =2

infra utilization
  cpu|mem used - 0
  cpu|mem total - 0
  cpu|mem xData/yData - 0

conainer|project trend data
  xData/yData - 0

[himdel]

Shouldn't this use the same precision?

(Otherwise this is the only place where used is calculated with different precision than total.)

[zeari]

Total cpu should always be a round number. There usually isnt a total of 1.444455 cpu cores for a node\cluster(and i dont think we want to show it if it does happen 😓 ).

[himdel]

Ah, got it.. so total is always precision=0.

We can still fit that in :)

[himdel]

Assuming #2762 (comment) is a bug and not intentional, this should get us to a state where utilization data is rendered in one place, while still being able to override the precisions:

commit 28b0d5a794f396e72b831798feff93bedc2b4850
Author: Martin Hradil <mhradil@redhat.com>
Date:   Mon Nov 27 14:28:41 2017 +0000

    *DashboardService - common formatter for utilization data
    
    with overridable precisions

diff --git a/app/services/container_dashboard_service.rb b/app/services/container_dashboard_service.rb
index 7790c07fcb..d4bf6ac6c2 100644
--- a/app/services/container_dashboard_service.rb
+++ b/app/services/container_dashboard_service.rb
@@ -1,7 +1,7 @@
-class ContainerDashboardService
+class ContainerDashboardService < DashboardService
   include UiServiceMixin
   include ContainerServiceMixin
-
+  CPU_USAGE_PRECISION = 2 # 2 decimal points
 
   def initialize(provider_id, controller)
     @provider_id = provider_id
@@ -185,7 +185,7 @@ class ContainerDashboardService
         :node     => m.resource.name,
         :provider => provider_name,
         :total    => m.derived_vm_numvcpus.present? ? m.derived_vm_numvcpus.round : nil,
-        :percent  => m.cpu_usage_rate_average.present? ? (m.cpu_usage_rate_average / 100.0).round(GRAPH_PRECISION) : nil # pf accepts fractions 90% = 0.90
+        :percent  => m.cpu_usage_rate_average.present? ? (m.cpu_usage_rate_average / 100.0).round(CPU_USAGE_PRECISION) : nil # pf accepts fractions 90% = 0.90
       }
 
       node_memory_usage << {
@@ -193,7 +193,7 @@ class ContainerDashboardService
         :node     => m.resource.name,
         :provider => m.resource.ext_management_system.name,
         :total    => m.derived_memory_available.present? ? m.derived_memory_available.round : nil,
-        :percent  => m.mem_usage_absolute_average.present? ? (m.mem_usage_absolute_average / 100.0).round(GRAPH_PRECISION) : nil # pf accepts fractions 90% = 0.90
+        :percent  => m.mem_usage_absolute_average.present? ? (m.mem_usage_absolute_average / 100.0).round(CPU_USAGE_PRECISION) : nil # pf accepts fractions 90% = 0.90
       }
     end
 
diff --git a/app/services/container_project_dashboard_service.rb b/app/services/container_project_dashboard_service.rb
index 3182db2418..15949df73c 100644
--- a/app/services/container_project_dashboard_service.rb
+++ b/app/services/container_project_dashboard_service.rb
@@ -1,4 +1,4 @@
-class ContainerProjectDashboardService
+class ContainerProjectDashboardService < DashboardService
   include UiServiceMixin
   include ContainerServiceMixin
 
@@ -7,7 +7,10 @@ class ContainerProjectDashboardService
     @project = ContainerProject.find(@project_id)
     @resource = @project
     @controller = controller
-    @custom_display_precision = 2 # 2 decimal points
+  end
+
+  def display_precision
+    2
   end
 
   def all_data
diff --git a/app/services/container_service_mixin.rb b/app/services/container_service_mixin.rb
index bfb84bb203..5bbb9760a6 100644
--- a/app/services/container_service_mixin.rb
+++ b/app/services/container_service_mixin.rb
@@ -1,6 +1,8 @@
 module ContainerServiceMixin
-  GRAPH_PRECISION = 2 # 2 decimal points
-  DISPLAY_PRECISION = 0 # 0 decimal points
+  def graph_precision
+    2
+  end
+
   REALTIME_TIME_RANGE = 10 # 10 minutes
 
   def daily_pod_metrics
@@ -61,22 +63,7 @@ module ContainerServiceMixin
   end
 
   def utilization_data(used_cpu, total_cpu, used_mem, total_mem)
-    if used_cpu.any?
-      {
-        :cpu => {
-          :used  => (used_cpu.values.last || 0).round(@custom_display_precision || DISPLAY_PRECISION),
-          :total => (total_cpu.values.last || 0).round,
-          :xData => used_cpu.keys,
-          :yData => used_cpu.values.map { |m| m.round(GRAPH_PRECISION) }
-        },
-        :mem => {
-          :used  => ((used_mem.values.last || 0) / 1024.0).round(@custom_display_precision || DISPLAY_PRECISION),
-          :total => ((total_mem.values.last || 0) / 1024.0).round(@custom_display_precision || DISPLAY_PRECISION),
-          :xData => used_mem.keys,
-          :yData => used_mem.values.map { |m| ((m || 0) / 1024.0).round(GRAPH_PRECISION) }
-        }
-      }
-    end
+    format_utilization_data(used_cpu, used_mem, total_cpu, total_mem)
   end
 
   def trend_data(trend)
diff --git a/app/services/dashboard_service.rb b/app/services/dashboard_service.rb
new file mode 100644
index 0000000000..fb1535ec72
--- /dev/null
+++ b/app/services/dashboard_service.rb
@@ -0,0 +1,50 @@
+module DashboardService
+  def display_precision
+    0
+  end
+
+  def graph_precision
+    0
+  end
+
+  def format_utilization_data(used_cpu, used_mem, total_cpu, total_mem)
+    {
+      :cpu => used_cpu.any? ? format_cpu(used_cpu, total_cpu) : nil,
+      :memory => used_mem.any? ? format_memory(used_mem, total_mem) : nil,
+    }
+  end
+
+  def format_cpu(used, total)
+    {
+      :used  => cpu_round(used.values.last),
+      :total => cpu_round(total.values.last),
+      :xData => used.keys,
+      :yData => used.values.map { |v| cpu_graph_round(v) },
+    }
+  end
+
+  def cpu_round(val)
+    (val || 0).round(display_precision)
+  end
+
+  def cpu_graph_round(val)
+    (val || 0).round(graph_precision)
+  end
+
+  def format_memory(used, total)
+    {
+      :used  => mem_round(used.values.last),
+      :total => mem_round(total.values.last),
+      :xData => used.keys,
+      :yData => used.values.map { |v| mem_graph_round(v) },
+    }
+  end
+
+  def mem_round(val)
+    ((val || 0) / 1024.0).round(display_precision)
+  end
+
+  def mem_graph_round(val)
+    ((val || 0) / 1024.0).round(graph_precision)
+  end
+end
diff --git a/app/services/ems_infra_dashboard_service.rb b/app/services/ems_infra_dashboard_service.rb
index a467b255e9..81ed3e450b 100644
--- a/app/services/ems_infra_dashboard_service.rb
+++ b/app/services/ems_infra_dashboard_service.rb
@@ -1,4 +1,4 @@
-class EmsInfraDashboardService
+class EmsInfraDashboardService < DashboardService
   include UiServiceMixin
   CPU_USAGE_PRECISION = 2 # 2 decimal points
 
@@ -129,27 +129,7 @@ class EmsInfraDashboardService
       total_mem[date] += metric.derived_memory_available if metric.derived_memory_available.present?
     end
 
-    if used_cpu.any?
-      {
-        :cpu => {
-          :used  => used_cpu.values.last.round,
-          :total => total_cpu.values.last.round,
-          :xData => used_cpu.keys,
-          :yData => used_cpu.values.map(&:round)
-        },
-        :memory => {
-          :used  => (used_mem.values.last / 1024.0).round,
-          :total => (total_mem.values.last / 1024.0).round,
-          :xData => used_mem.keys,
-          :yData => used_mem.values.map { |m| (m / 1024.0).round }
-        }
-      }
-    else
-      {
-        :cpu => nil,
-        :memory => nil
-      }
-    end
+    format_utilization_data(used_cpu, used_mem, total_cpu, total_mem)
   end
 
   def daily_provider_metrics

(If we really need different precision for total cpu here, you'd need to add a case.)
(Also undid the CPU_USAGE_PRECISION change, since that one seems to be a different number - the value of 2 is used in all the code - so another candidate for merging.)

[himdel]

Hoping this fixes it so that total has always precision=0.

diff --git a/app/services/dashboard_service.rb b/app/services/dashboard_service.rb
index 8b260943c1..50c5416d48 100644
--- a/app/services/dashboard_service.rb
+++ b/app/services/dashboard_service.rb
@@ -16,35 +16,27 @@ module DashboardService
 
   def format_cpu(used, total)
     {
-      :used  => cpu_round(used.values.last),
-      :total => cpu_round(total.values.last),
+      :used  => cpu_num(used.values.last).round(display_precision),
+      :total => cpu_num(total.values.last).round(0),
       :xData => used.keys,
-      :yData => used.values.map { |v| cpu_graph_round(v) },
+      :yData => used.values.map { |v| cpu_num(v).round(graph_precision) },
     }
   end
 
-  def cpu_round(val)
-    (val || 0).round(display_precision)
-  end
-
-  def cpu_graph_round(val)
-    (val || 0).round(graph_precision)
+  def cpu_num(val)
+    (val || 0)
   end
 
   def format_memory(used, total)
     {
-      :used  => mem_round(used.values.last),
-      :total => mem_round(total.values.last),
+      :used  => mem_num(used.values.last).round(display_precision),
+      :total => mem_num(total.values.last).round(0),
       :xData => used.keys,
-      :yData => used.values.map { |v| mem_graph_round(v) },
+      :yData => used.values.map { |v| mem_num(v).round(graph_precision) },
     }
   end
 
-  def mem_round(val)
-    ((val || 0) / 1024.0).round(display_precision)
-  end
-
-  def mem_graph_round(val)
-    ((val || 0) / 1024.0).round(graph_precision)
+  def mem_num(val)
+    ((val || 0) / 1024.0)
   end
 end

[zeari]

@himdel I added little fixes on top of your work.

[bazulay]

@himdel PTAL

[Loicavenel]

@himdel Martin, we need to close this one .. thanks for your review

[himdel]

@Loicavenel I'll guess by close you mean merge? :) Anyway, on it..

[himdel]

@zeari Almost there.. looks like you renamed mem to memory but didn't quite catch all the references:

spec/services/container_dashboard_service_spec.rb
109:      expect(node_utilization_all_providers).to eq(:cpu => nil, :mem => nil)
110:      expect(node_utilization_single_provider).to eq(:cpu => nil, :mem => nil)

app/services/container_service_mixin.rb
111:      :xy_data       => {:cpu => nil, :mem => nil}

(or was it me? sorry :))

[zeari]

@zeari Almost there.. looks like you renamed mem to memory but didn't quite catch all the references:

@himdel Then how did the specs pass? :O

[himdel]

Symmetry :D the bit that's broken in the service is the bit that gets tested by the broken spec :)

[miq-bot]

Checked commits zeari/manageiq-ui-classic@73a5e51~...c38f8f7 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 0 offenses detected
Everything looks fine. 👍

[himdel]

LGTM when green 👍, not seeing any breakage in other dashboards ..

[simaishi]

Gaprindashvili backport details:

$ git log -1
commit 6d12604226a31cf948dfcd4b89fc479744fe3d02
Author: Martin Hradil <himdel@seznam.cz>
Date:   Mon Dec 4 14:26:29 2017 +0000

    Merge pull request #2762 from zeari/change_accuracy
    
    Change metric accuracy in container projects dashboard
    (cherry picked from commit 414def53d0e1501bfbcd7e505d57afa0cf017b5e)