Check for base unit when adjusting unit label #1447

yaacov · 2017-05-28T10:34:00Z

Description

Currently we do not check the base unit when creating the unit label.
For example 1000s will become 1Ks, and 1000ms will become 1Kms.

This is wrong, we want to take into account the base units.
For example 1000s will become 1Ks, and 1000ms will become 1s.

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

Screenshots

Bug

Fix

yaacov · 2017-05-28T10:47:41Z

@miq-bot add_label bug, providers/containers

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

miq-bot · 2017-05-28T10:48:05Z

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

yaacov · 2017-05-28T10:48:42Z

@miq-bot add_label compute/containers

yaacov · 2017-05-29T06:03:00Z

@zeari please review

yaacov · 2017-05-29T11:33:33Z

@zeari (raised original question) 👍 thanks

@simon3z @serenamarie125 @himdel please help:

The problem
We want to chart large float point numbers representing time, for exmaple 107,040,046.064 s

Possible options
a. we can use X years Y months and Z days notation ( e.g. 1 year 3 months and 3 minutes )
b. we can use the number as is ( e.g. 1,030,003,322,033.104 s )
c. we can use the metric system ( e.g. 1.03Ts)
d. ?

Solution used in this PR
Using years and months is problematic because a chart plotting the values[4weeks, 1year, 1months and 3weeks, .. ] is not usable.
Using the numbers as is also problematic :-(
I chose to use the metric system, but it may looks strange to user. Kilosecond is not widely used time unit.

Question
Do we have a convention for charting large numbers representing time in MiQ ?

yaacov · 2017-05-29T12:54:19Z

@simon3z I found this lib:
http://gentooboontoo.github.io/js-quantities/

but I think it's an overkill for just ms, ns, milliseconds ...

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

@simon3z @serenamarie125 @himdel please review

yaacov · 2017-06-04T07:09:33Z

@miq-bot add_label fine/yes

https://bugzilla.redhat.com/show_bug.cgi?id=1456246 target 5.8.2

himdel · 2017-06-06T13:50:37Z

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

+
+      // get base unit for special case units
+      switch (units) {
+        case 'milliseconde':


millisecond ;) Unless you really wanted this in french..?

himdel · 2017-06-06T13:51:02Z

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

+          baseUnit = units;
+      }
+
+      // adjuat to base units and calc exponent


himdel · 2017-06-06T13:51:38Z

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

+
+      // adjuat to base units and calc exponent
+      baseUnitMaxValue = maxValue * baseUnitMultiplier;
+      exp = parseInt(Math.log10(baseUnitMaxValue) / 3, 10) * 3;


parseInt to convert from float to int? Sounds like ~~ might do the trick better..

himdel · 2017-06-06T13:52:17Z

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

+
+      // calc output unit label and multiplier
+      metricPrefixes = {
+        3: {unitLable: 'K' + baseUnit, multiplier: baseUnitMultiplier * Math.pow(10, -3)},


unitLabel? (all over the place)

@himdel Sorry 👍 fixed ( all above notes also ... )

himdel · 2017-06-06T13:58:56Z

@yaacov As for jq-quantities, possibly an overkill, possibly not, but..

we're already using numeral.js for these things in charting, so it may make sense to use here too.. numeral(63846).format('00:00:00') == '17:44:06'
and we're also using moment.js which does support formatting relative time .. moment().subtract(63846 * 1000).fromNow() == '18 hours ago'.

(Also there's a bunch of custom formatters in ./app/assets/javascripts/miq_formatters.js which may or may not be useful here.)

himdel · 2017-06-06T14:02:28Z

@yaacov Is the time formatting something you want to solve in this PR, or do kilosecs, etc. make sense for now?

I'm good with this as it is 👍 (but to be honest I do already have a calendar entry to celebrate reaching 1Gs of age :))

himdel · 2017-06-06T14:05:23Z

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

+
+      // adjust to base units and calc exponent
+      baseUnitMaxValue = maxValue * baseUnitMultiplier;
+      exp = ~~(Math.log10(baseUnitMaxValue) / 3, 10) * 3;


missed the , 10 ;) (Don't want this to always yield 30, I guess..)

miq-bot · 2017-06-06T14:09:06Z

Checked commit yaacov@0786f27 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-06T14:11:31Z

@yaacov Is the time formatting something you want to solve in this PR, or do kilosecs, etc. make sense for now?

@himdel
Just wanted not to have Tns or Gms, this are really not nice ...

himdel · 2017-06-06T14:32:21Z

Agreed, Gms is evil, Ks is definitely ..less so :).

serenamarie125 · 2017-06-07T04:35:04Z

@yaacov what is the time interval representing, and why do we need to be so granular?
I'm also confused as to which number is the time interval versus the CPU usage

yaacov · 2017-06-07T05:34:06Z

@serenamarie125 @simon3z @himdel hi,

This PR is solving the bug of "Kns" and "Tms", killo nano second and tera milli second are simply not metric unit, we must fix that.

The problem of "tera-milli-second" (this is not a unit) may have different solutions.

We choose to convert to base unit and use the metric system, e.g. Km, Ggr, Ts ... this is correct, but "tera second", may sound bad to a none engineer :-)

We wanted to know if MiQ has a UX policy for cases like this, (Using "engineer" vs "human" language) ?

If MiQ policy is to use "human" in this cases, we can do this too, @himdel suggested usind moment.js for this.

what is the time interval representing

Some metrics are measured in time intervals, with unit tag set to "ns", "ms" and "millisecond", we do not currently collect metrics with other time units.

For example: CPU usage is measured in nano seconds since power on, the unit tag is set to "ns"

why do we need to be so granular

The units "ms", "ns" comes with the measurement collected (we get a value and a unit, e.g byte, ms, ns), nano seconds is a common unit to measure CPU utilization. High granularity may show important things, for example, 1day - 1day is different then 86500sec - 86400sec

I'm also confused as to which number is the time interval versus the CPU usage

CPU usage is a time interval, it is the time in ns that the CPU was in use since power on.

yaacov · 2017-06-07T05:57:24Z

p.s.

Looking how others solved this:
OpenShift seem to only show the usage_rate, that never climb to the tera/gega range ( so they do not have this problem :-) )
And Grafana / Prometheus just show very long numbers, e.g. 209,478,882,200,033ns

Check for base unit when adjusting unit label

Check for base unit when adjusting unit label (cherry picked from commit e2865c1) https://bugzilla.redhat.com/show_bug.cgi?id=1478379

simaishi · 2017-08-04T13:18:24Z

Fine backport details:

$ git log -1
commit 820ad5ee40faf5c68281fe6d73aee11a5a312797
Author: Martin Hradil <[email protected]>
Date:   Tue Jun 6 14:32:26 2017 +0000

    Merge pull request #1447 from yaacov/units-ad-hoc
    
    Check for base unit when adjusting unit label
    (cherry picked from commit e2865c1e1f7b770bb49195392c74aa411b498cd3)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1478379

yaacov force-pushed the units-ad-hoc branch from 5e84379 to 2e87e30 Compare May 28, 2017 10:39

miq-bot added the bug label May 28, 2017

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

yaacov force-pushed the units-ad-hoc branch from 2e87e30 to c99ddfc Compare May 28, 2017 11:54

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

himdel reviewed Jun 6, 2017

View reviewed changes

yaacov force-pushed the units-ad-hoc branch from c99ddfc to 13417f6 Compare June 6, 2017 13:57

himdel reviewed Jun 6, 2017

View reviewed changes

Check for base unit when adjusting unit label

0786f27

yaacov force-pushed the units-ad-hoc branch from 13417f6 to 0786f27 Compare June 6, 2017 14:07

himdel self-assigned this Jun 6, 2017

himdel merged commit e2865c1 into ManageIQ:master Jun 6, 2017

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

yaacov pushed a commit to yaacov/manageiq-ui-classic that referenced this pull request Jun 25, 2017

Merge pull request ManageIQ#1447 from yaacov/units-ad-hoc

4757fc1

Check for base unit when adjusting unit label

yaacov mentioned this pull request Jun 25, 2017

[FINE] Fix units bug and add differential #1600

Merged

simaishi pushed a commit that referenced this pull request Aug 4, 2017

Merge pull request #1447 from yaacov/units-ad-hoc

820ad5e

Check for base unit when adjusting unit label (cherry picked from commit e2865c1) https://bugzilla.redhat.com/show_bug.cgi?id=1478379

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

Check for base unit when adjusting unit label #1447

Check for base unit when adjusting unit label #1447

yaacov commented May 28, 2017 •

edited

Loading

yaacov commented May 28, 2017

miq-bot commented May 28, 2017

yaacov commented May 28, 2017

yaacov commented May 29, 2017

yaacov commented May 29, 2017

yaacov commented May 29, 2017

yaacov commented Jun 1, 2017

yaacov commented Jun 4, 2017

himdel Jun 6, 2017

himdel Jun 6, 2017

himdel Jun 6, 2017

himdel Jun 6, 2017

yaacov Jun 6, 2017

himdel commented Jun 6, 2017 •

edited

Loading

himdel commented Jun 6, 2017 •

edited

Loading

himdel Jun 6, 2017 •

edited

Loading

yaacov Jun 6, 2017

miq-bot commented Jun 6, 2017

yaacov commented Jun 6, 2017

himdel commented Jun 6, 2017

serenamarie125 commented Jun 7, 2017

yaacov commented Jun 7, 2017

yaacov commented Jun 7, 2017

simaishi commented Aug 4, 2017

Check for base unit when adjusting unit label #1447

Check for base unit when adjusting unit label #1447

Conversation

yaacov commented May 28, 2017 • edited Loading

yaacov commented May 28, 2017

miq-bot commented May 28, 2017

yaacov commented May 28, 2017

yaacov commented May 29, 2017

yaacov commented May 29, 2017

yaacov commented May 29, 2017

yaacov commented Jun 1, 2017

yaacov commented Jun 4, 2017

himdel Jun 6, 2017

Choose a reason for hiding this comment

himdel Jun 6, 2017

Choose a reason for hiding this comment

himdel Jun 6, 2017

Choose a reason for hiding this comment

himdel Jun 6, 2017

Choose a reason for hiding this comment

yaacov Jun 6, 2017

Choose a reason for hiding this comment

himdel commented Jun 6, 2017 • edited Loading

himdel commented Jun 6, 2017 • edited Loading

himdel Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

yaacov Jun 6, 2017

Choose a reason for hiding this comment

miq-bot commented Jun 6, 2017

yaacov commented Jun 6, 2017

himdel commented Jun 6, 2017

serenamarie125 commented Jun 7, 2017

yaacov commented Jun 7, 2017

yaacov commented Jun 7, 2017

simaishi commented Aug 4, 2017

yaacov commented May 28, 2017 •

edited

Loading

himdel commented Jun 6, 2017 •

edited

Loading

himdel commented Jun 6, 2017 •

edited

Loading

himdel Jun 6, 2017 •

edited

Loading