#7446: Total Metric Count inside donut pie charts #16820

varunsharma27 · 2018-02-20T11:16:36Z

#7446 I believe the name of the metric would be implicit and not required to be mentioned in the inner text or perhaps we could add a tooltip?. How do you feel about the logic for 'font-size' at LOC 346?
Preview:-

elasticmachine · 2018-02-20T11:16:38Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

tsullivan

I think the donut descriptor should be controlled by a setting in the "Pie Settings" panel, src/core_plugins/kbn_vislib_vis_types/public/editors/pie.html. There may be times when it's not desirable to show this

tsullivan · 2018-02-21T19:57:50Z

@Bargs (commented on original issue) and @elastic/kibana-visualizations would you mind doing a review on this?

varunsharma27 · 2018-02-21T20:00:29Z

Having an empty space doesn't actually give any benefit per se, moreover would you be okay with the addition of another setting (Clutter)? If yes then I can do that.

trevan · 2018-02-21T20:12:22Z

I would prefer seeing the first label in the middle. Or maybe the value for the highest result (kind of like a gauge).

thomasneirynck · 2018-02-21T20:21:35Z

thanks @varundbest!

jenkins, test this.

elasticmachine · 2018-02-21T21:08:34Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

varunsharma27 · 2018-02-21T21:11:59Z

Fixing the issues.

varunsharma27 · 2018-02-21T21:30:15Z

Weird, should've been caught by the linter. No more issues (In my local).

timroes · 2018-02-22T09:15:35Z

I second @tsullivan here. We try to keep the charts as configurable as possible, since there is no such thing as a universal desire people have. Even though you say: you don't need whitespace in your pie charts, I can see a lot of people who say, they don't want the label inside their pie charts.

So I would kindly ask you to add an option to show the label.

I also think there should be a label for this. Without looking at the sources I wouldn't have been able to figure out what the number means exactly. When you use Count as a metric aggregation I would be rather confident, that it's the Count of all documents drawn in the inner circle. If I use an Sum as a metric, will this be the count of all documents, or the sum of all "sum values" in the inner circle?

I think a tooltip at least could help solve that confusion for users.

varunsharma27 · 2018-02-22T09:20:57Z

Okay, will add an option and label.

timroes · 2018-04-12T09:44:29Z

Hi @varundbest. Do you currently still have time to work on this? Otherwise I would like to close it, if you can't spend the time in it at the moment, but of course we could reopen it as soon as you want to continue working on this.

Cheers,
Tim

varunsharma27 · 2018-04-12T09:50:24Z

Hi @timroes, I've been a little busy lately. Will add an Option (Enabled by default) and an onHover label over the weekend.

timroes · 2018-04-12T09:59:59Z

Thanks for the quick response. Glad you will do that. Also feel free to close PRs yourself if you feel you can't spend time in the near future working on them again, and feel free to reopen them or open a new one for the same feature. That way we better have an overview what PRs are actually still active and need our attention :-)

varunsharma27 · 2018-04-25T09:26:05Z

If I use an Sum as a metric, will this be the count of all documents, or the sum of all "sum values" in the inner circle?

It will be the total of whatever metric the Pie is being split on.

@timroes Pushed the desired changes (an Option (Enabled by default) and an onHover label). How do you feel about it now?

timroes · 2018-05-04T11:05:36Z

src/ui/public/vislib/visualizations/pie_chart.js

@@ -32,7 +33,7 @@ export function VislibVisualizationsPieChartProvider(Private) {

      const charts = this.handler.data.getVisData();
      this._validatePieData(charts);
-
+      this.ABBREVIATIONS = ['', 'k', 'Mil', 'Bn', 'Tn'];


I think we should rather use the common unit prefixes as in other places, i.e. k, M, G, T, P.

These are imho better understand and less prone to misunderstanding due to cultural/language differences.

Example time: In German we have a bit different system to naming millions and above:

Million [EN] -> Million [DE]

Billion [EN] -> Milliarde [DE]

Trillion [EN] -> Billion [DE]

Quadrillion [EN] -> Billiarde [DE]

Quintillion [EN] -> Trillion [DE]

...

Even while speaking English most of the time, I have a hard time, thinking when seeing a billion or Bn about an English billion rather than a German billion. Also Tn could be easily misunderstood as the abbreviation of "Tausend" (the German word for thousand).

In general using the well acknowledged prefixes, would imho be a better solution and better aligned with the rest of Kibana.

timroes · 2018-05-04T11:07:40Z

src/ui/public/vislib/visualizations/pie_chart.js

+    readableNumber(number) {
+      let tier = Math.log10(Math.abs(number)) / 3 | 0;
+      if(tier === 0) return number;
+      if(tier > 4) tier = 4;


Please don't hardcode 4 here, but rather use this.ABBREVIATIONS.length

timroes · 2018-05-04T11:09:28Z

src/ui/public/vislib/visualizations/pie_chart.js

+      if(tier === 0) return number;
+      if(tier > 4) tier = 4;
+      const postfix = this.ABBREVIATIONS[tier];
+      const scale = Math.pow(10, tier * 3);


We try to use modern JS where possible: 10 ** (tier * 3)

timroes · 2018-05-04T11:13:15Z

src/ui/public/vislib/visualizations/pie_chart.js

+      const postfix = this.ABBREVIATIONS[tier];
+      const scale = Math.pow(10, tier * 3);
+      const scaled = number / scale;
+      let formatted = String(scaled.toFixed(1));


The String is unnecessary, since toFixed already returns a string.

timroes · 2018-05-04T11:17:48Z

src/ui/public/vislib/visualizations/pie_chart.js

+      const scale = Math.pow(10, tier * 3);
+      const scaled = number / scale;
+      let formatted = String(scaled.toFixed(1));
+      if (/\.0$/.test(formatted)) {


We can get rid of the whole (expensive) regex operations, string cutting and even using let, by doing the following instead:

// Converting a string to a numeric value will cut off any unnecessary trailing zeroes const formatted = Number(scaled.toFixed(1)); return `${formatted}${postfix}`;

timroes · 2018-05-04T11:20:29Z

src/ui/public/vislib/visualizations/pie_chart.js

+    }
+
+    addDonutDescriptor(svg, width, height) {
+      const self = this;


Please don't. Just use arrow functions, which will preserve the this context.

For new code we don't try to fall back to hacky ES5 solutions anymore.

timroes · 2018-05-04T11:32:25Z

src/core_plugins/kbn_vislib_vis_types/public/pie.js

@@ -21,6 +21,7 @@ export default function HistogramVisType(Private) {
        addLegend: true,
        legendPosition: 'right',
        isDonut: true,
+        showTotal: true,


Please make the default false, so we don't change behavior on existing charts.

timroes · 2018-05-04T11:36:32Z

src/ui/public/vislib/visualizations/pie_chart.js

+        .text(function (chartData) {
+          return self.readableNumber(chartData.slices.sumOfChildren);
+        });
+      let tooltip;


In my opinion we could get rid of the tooltip altogether. The user already enabled "Show total", so I would assume they would be aware, that it's the total of the chart. If we want to keep the tooltip, we need to add a better styling for it, because currently it's having no padding at all. A solution could be adding a table into it with a "Total {label}" and a "{value}" column in it.

In general I don't like that we mess around with the tooltip like that. We don't try to manually modify the tooltip in any other chart like that, so I would really prefer getting rid of it for now.

Generally, the person creating the dashboards isn't the one using them. It would aid dashboard users. Perhaps I should add a table? Or we have a better way to add such a tooltip?

timroes · 2018-05-04T11:37:12Z

src/ui/public/vislib/visualizations/pie_chart.js

+        .style('text-anchor', 'middle')
+        .attr('font-size', function (chartData) {
+          const dividend = Math.min(width, height);
+          let length = 0;


const length = chartData.raw ? chartData.raw.columns.length : 0;

And one let less .. :-)

timroes · 2018-05-04T11:39:55Z

src/ui/public/vislib/visualizations/pie_chart.js

+          if(chartData.raw) {
+            length = chartData.raw.columns.length;
+          }
+          const divisor = Math.max(length, 4);


Could you please explain (and ideally add a comment) what this part does exactly? Why are we using 4 here as a minimum?

This code determines the size of font for descriptor. As we add sub-buckets, it increases divisor to accommodate more pie layers. 4 is the minimum number derived by trial and error, the divisor should be set to. Would you have any suggestion for a better logic to determine font size (It has to be dynamic, based on size of the panel and number of sub-buckets)?

…er label.

…izations.

varunsharma27 · 2018-06-15T18:38:29Z

Any other pressing issue you can see in this PR?

ppisljar · 2018-06-18T07:06:17Z

ping @timroes

ppisljar

thanks a lot for this contribution @varunsharma27
i left some comments, let me know what you think.

ppisljar · 2018-06-18T08:32:23Z

src/ui/public/vislib/visualizations/pie_chart.js

@@ -32,7 +33,7 @@ export function VislibVisualizationsPieChartProvider(Private) {

      const charts = this.handler.data.getVisData();
      this._validatePieData(charts);
-
+      this.ABBREVIATIONS = ['', 'k', 'M', 'G', 'T', 'P'];


Usually we rely on field formatters to do this kind of formatting. The reason is that this can be field dependent, for example if you used sum of bytes for your pie chart 1k will mean 1024, but if you used sum of price 1k will mean 1000 ...

at the moment this would be the only chart handling this on its own, what do you think @timroes ?

if we decide to keep this logic here i would suggest using numeral.js which is already used in other parts of kibana to handle this.

"1k will mean 1024", we are denoting it in standard convention of naming large numbers but to clarify it further, we might as well show the full number in the tooltip :- "Sum of Bytes: 1000"
Would it make sense for you?

ppisljar · 2018-06-18T08:35:37Z

src/ui/public/vislib/visualizations/pie_chart.js

@@ -327,6 +328,54 @@ export function VislibVisualizationsPieChartProvider(Private) {
      }
    }

+    readableNumber(number) {
+      let tier = Math.log10(Math.abs(number)) / 3 | 0;


where does the 3 come from ?

Our tiers step up by 10^3 :- Thousand (10^3), Million (10^6), Billion (10^9). So 3 signifies a step up.

ppisljar · 2018-06-18T09:04:17Z

src/ui/public/vislib/visualizations/pie_chart.js

+        .text((chartData) => {
+          return this.readableNumber(chartData.slices.sumOfChildren);
+        });
+      let tooltip;


i suggest to replace the whole section below with:

descriptor.call(this.tooltip.render(`<table><tr><td>Total ${metricLabel}</td></tr></table>`));

this way we let our tooltip service take care of showing/hiding and positioning of tooltips.

this will require some changes to the ui/vis/components/tooltip/tooltip.js:

on line 132 you should add a parameter to the render method: defaultHtml

on line 186 you could then use this to render the tooltip:

if (defaultHtml) { return render(defaultHtml); }

Makes Sense, Will be on to it.

rashmivkulkarni · 2018-10-10T15:20:22Z

hi @varunsharma27 - can you address the failures and pull master into your branch and re- run the tests?

tsullivan · 2018-10-10T15:25:18Z

Hi again @varunsharma27, nice to see so much improvement coming into our pie charts here!

Another requirement for us to be able to merge this is a license agreement you'll need to look at and sign: http://www.elasticsearch.org/contributor-agreement/

elasticmachine · 2018-10-10T15:54:20Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

tsullivan · 2019-02-25T17:42:10Z

Hi @varunsharma27, it seems we lost touch on this PR. Are you still interested in taking care of the feedback and merging master?

elasticmachine · 2019-02-25T17:42:32Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

varunsharma27 · 2019-02-27T12:58:25Z

@tsullivan Yes I'd like to do so! Maybe start with rebasing my code to kibana 7 master. However I've been seeing PRs getting lost in kibana repository. For example : #29086
Would you still be interested in this pr if I implement it (Maybe using EUI now)?

tsullivan · 2019-03-04T18:55:43Z

I've been seeing PRs getting lost in kibana repository.

Hi, sorry about that. It's getting hard to triage newcoming PRs. I', not saying this is acceptable, but you will probably get more traction on PRs if there is an issue that the PR closes, as issues get more attention and triaging.

varunsharma27 · 2019-03-04T18:57:07Z

Thanks! I'll rebase and redo this implementation in accordance with 7.x and suggestions from this pr.

elasticmachine · 2019-03-22T17:51:19Z

Pinging @elastic/kibana-app

timroes · 2019-07-05T08:05:19Z

It seems this was never rebased back on master since February. I would close this PR for now, but if you want to pick this up again, please feel free to reopen. @markov00 could we also keep a feature like that in mind for elastic-charts pie chart implementation?

tsullivan reviewed Feb 21, 2018

View reviewed changes

thomasneirynck requested review from thomasneirynck and ppisljar February 21, 2018 20:19

thomasneirynck added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) Feature:Pie Chart Pie chart visualization feature enhancement New value added to drive a business result labels Feb 21, 2018

varunsharma27 added a commit to varunsharma27/kibana that referenced this pull request Feb 21, 2018

Issue: elastic#7446 PR: elastic#16820 Build issue fix.

c12c200

timroes mentioned this pull request Apr 4, 2018

Ability to superimpose a visualization on top of another #17318

Closed

thomasneirynck requested review from timroes and removed request for thomasneirynck April 11, 2018 21:32

timroes suggested changes May 4, 2018

View reviewed changes

varunsharma27 added 5 commits May 8, 2018 16:18

elastic#7446: Total Metric Count inside donut pie charts

ba73ff9

Issue: elastic#7446 PR: elastic#16820 Build issue fix.

6e5dbf9

Adds an Option (Enabled by default) for Donut Descriptor and an onHov…

47a5993

…er label.

Bugfix: Hover Crash in case of Split Series.

12419e3

Defaults showTotal as false, applies suggested Code Changes and Optim…

ec0bb69

…izations.

varunsharma27 force-pushed the 7446-test-inside-donut branch from a94cae2 to ec0bb69 Compare May 8, 2018 12:33

ppisljar requested changes Jun 18, 2018

View reviewed changes

rayafratkina added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Mar 22, 2019

timroes closed this Jul 5, 2019

markov00 added the Feature:ElasticCharts Issues related to the elastic-charts library label Jul 5, 2019

markov00 mentioned this pull request Jul 5, 2019

Pie chart elastic/elastic-charts#84

Closed

4 tasks

#7446: Total Metric Count inside donut pie charts #16820

#7446: Total Metric Count inside donut pie charts #16820

Conversation

varunsharma27 commented Feb 20, 2018 • edited

elasticmachine commented Feb 20, 2018

tsullivan left a comment

Choose a reason for hiding this comment

tsullivan commented Feb 21, 2018

varunsharma27 commented Feb 21, 2018

trevan commented Feb 21, 2018

thomasneirynck commented Feb 21, 2018

elasticmachine commented Feb 21, 2018

💔 Build Failed

varunsharma27 commented Feb 21, 2018

varunsharma27 commented Feb 21, 2018 • edited

timroes commented Feb 22, 2018

varunsharma27 commented Feb 22, 2018

timroes commented Apr 12, 2018

varunsharma27 commented Apr 12, 2018

timroes commented Apr 12, 2018 • edited

varunsharma27 commented Apr 25, 2018

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

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

varunsharma27 commented Jun 15, 2018

ppisljar commented Jun 18, 2018

ppisljar left a 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

Choose a reason for hiding this comment

ppisljar Jun 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rashmivkulkarni commented Oct 10, 2018

tsullivan commented Oct 10, 2018

elasticmachine commented Oct 10, 2018

💔 Build Failed

tsullivan commented Feb 25, 2019

elasticmachine commented Feb 25, 2019

💚 Build Succeeded

varunsharma27 commented Feb 27, 2019

tsullivan commented Mar 4, 2019

varunsharma27 commented Mar 4, 2019

elasticmachine commented Mar 22, 2019

timroes commented Jul 5, 2019

varunsharma27 commented Feb 20, 2018 •

edited

varunsharma27 commented Feb 21, 2018 •

edited

timroes commented Apr 12, 2018 •

edited

ppisljar Jun 18, 2018 •

edited