Add "Sum of series in legend" option #7970

ppisljar · 2016-08-10T09:26:48Z

fixes #4599 - Add "Sum of series in legend" option

counts can be enabled by checking Show Sum of Series option

ycombinator · 2016-08-10T10:24:25Z

@ppisljar Which Kibana version is this PR targeting? Can you add the corresponding labels? Thanks!

ycombinator · 2016-08-10T10:36:22Z

src/ui/public/visualize/visualize_legend.js

@@ -66,6 +66,18 @@ uiModules.get('kibana')
        return filters.length;
      };

+      $scope.showLabelCount = legendData => {


Consider renaming this to showLegendCount for consistency with vis.params.addLegendCount.

ycombinator · 2016-08-10T10:43:31Z

Functionality looks great. I added a few comments about the code.

ycombinator · 2016-08-10T10:45:30Z

Also, I suggest getting a 2nd reviewer for this PR who is familiar with the visualization code (Spencer?) as I'm not too familiar with it myself at the moment.

ycombinator · 2016-08-10T10:57:22Z

src/core_plugins/kbn_vislib_vis_types/public/histogram.js

@@ -29,6 +29,7 @@ export default function HistogramVisType(Private) {
      modes: ['stacked', 'percentage', 'grouped'],
      editor: histogramTemplate
    },
+    isLegendCountSupported: true,


While this is definitely the right file for this line to go in, I'm not 100% sure this is the right place in the object being passed to the VislibVisType constructor. Maybe isLegendCountSupported needs to go in params or params.defaults? It seems a bit hanging out on its own when other parameters are either in params or in params.defaults. Maybe try out either of those places and also check with Spencer as a 2nd reviewer?

In fact, I just tested this PR and I'm pretty sure this is not the right place for this parameter. $scope.vis.type.isLegendCountSupported is evaluating to undefined.

ycombinator · 2016-08-10T11:11:58Z

src/ui/public/visualize/visualize_legend.js

@@ -68,7 +68,7 @@ uiModules.get('kibana')

      $scope.showLegendCount = legendData => {
        if (!$scope.vis.params.addLegendCount) return false;
-        if (!$scope.vis.type.isLegendCountSupported) return false;
+        if (!$scope.vis.type.params.isLegendCountSupported) return false;
        return  $scope.getLegendCount(legendData);


Based on the name of this function (showLegendCount) and its usage, I expect it to return a boolean value. So consider changing the final return statement to return !!$scope.getLegendCount(legendData); or return Boolean($scope.getLegendCount(legendData)); as $scope.getLegendCount itself does not return a boolean value.

ycombinator · 2016-08-10T11:27:56Z

LGTM! Nice work @ppisljar!

On to the next reviewer...

spalger · 2016-08-10T20:31:00Z

I'm able to choose the "Show Legend Counts" option on the Line chart

spalger · 2016-08-10T20:31:49Z

Why does the legend count work with the bar chart but not the line and area charts?

ppisljar · 2016-08-11T10:31:03Z

@spalger thats what people were mentioning in the issue ticket. Do you think it would be useful there as well ?

i added the option to line and area charts as well.

spalger · 2016-08-11T22:09:04Z

I imagine it could be useful everywhere, but I don't have any reason why. I just want to make sure that the option is only available when the visualization supports it.

spalger · 2016-08-11T22:52:01Z

Sorry Peter, dug into this a bit more and I'm afraid this will need to change approach a bit.

two issues:

the value in the legend isn't using a field formatter
the value isn't a count unless the first metric aggregation is a count

possible solutions:

Always show count, not the sum of the series values, by reading the doc_count property of the agg response
Move the option into each aggregation, change it to "Summarize in the legend" or something, and allow some/all of the aggregations to be summarized (ie. average summerizes differently than count)
Only show the summary if their is a count aggregation in the metric list

Either way I think we change this to have Kibana send a different legend label label to the vislib, rather than the vislib try to offer this setting. That way, Kibana can use it's awareness of the aggregation types and field formatters to produce a high quality series summaries.

spalger · 2016-08-11T22:56:41Z

I suppose another options might be to change the option to "show sum of each series in legend" and just apply the default number formatter... That might be good enough to ship this feature. @epixa thoughts?

ppisljar · 2016-08-12T09:56:14Z

ok after your live session this is much clearer

tbragin · 2016-09-12T21:59:55Z

@spalger Thanks for tagging me! @ppisljar I'm super excited that you are working on this, I think having this feature will really help folks displaying Kibana dashboards on overhead projectors and distributing them as reports, where user cannot mouse-over to see the counts.

In regards to the things @spalger noted in his Aug 11 comment, I do agree there is room for improvement here. I went head and checked out the current PR in @ppisljar's branch and compared it against Kibana 3.1.3, to dig into specifics.

Interestingly, Kibana 3 has followed approach (1) in @spalger's list and chose to show only document counts, even when the histogram displays max or total. We may or may not agree with this approach for Kibana 4/5, but it's interesting to note that anything above this is an improvement, or a change in functionality, depending on your perspective.

So while I personally find it confusing to look at document counts when the visualization presents something else, and as a result find option (2) attractive conceptually, I wonder why @rashidkpc chose not to go down that path in Kibana 3. Conceptually it should be possible to apply the same logic the Metric vis does to arrive at a single value for all of these aggregations, with the caveat of needing to filter properly for a specific term if required. Is it too complex / computationally expensive to arrive at labels that make sense for all scenarios?

If option (2) ends up being prohibitively complex in the near term, given the Kibana 3 history, if we took the more simple approach suggested in (1) in the first phase of this PR, I'd be ok with it. Though I hope we would find a better way to indicate in the UI what the counts mean, because it's not clear in Kibana 3. I'd also be ok with doing (3), starting with legend data label support for just Counts and Sum, for instance, as long as the Visualize Options UI is really clear about when the data label option is available and not (see below). The value-add of showing counts in the UI for most common scenarios where they make sense would outweigh lack of support for all aggregations for me personally.

Looking at the current state of @ppisljar's PR, it appears that he's sort of headed down the path of (2), with special support for data labels when Sum and Unique Count aggregations are in play, adding up values in all buckets. Some comments:

Option to provide data labels exists in Options even if it has no impact on the selected aggregation (e.g. Bar vis, Metric: Average, Bucket: Date Histogram).
I'm not sure why we call the option "Show Sum of Series" even when we're dealing with other data summaries. Can we just call it "Show data label in legend"?
Label seems to be missing completely, when bar diagram has no terms (see screenshot).
I like the way the Sum aggregation is treated across visualizations, it seems consistent. But I'm not really sure how we are doing the "unique count" labels in date histograms and why the resulting counts exceed the total numbers of unique users in the Metric vis, looking side-by-side at results from the Pie chart, date histogram, and metric vis. I think simply adding up unique counts across buckets is misleading.

spalger · 2016-10-17T17:48:04Z

We talked about this collectively and decided that option 2 would be ideal, but option 1 also works as long as it is opt-in, and labels the count with a unit of some sort ("XYZ docs", or "XYZ hits", etc.)

thomasneirynck · 2017-02-21T17:02:00Z

@ppisljar I'm going to close this, since there's been little movement on this.

Do reopen if you think it's worth continuing from here again.

`v95.7.0` ⏩ `v95.9.0` > [!CAUTION] > This PR contains the final set of Emotion conversions for all EuiForm components. > If your plugin contains any custom CSS/styling to **EuiFormRow, EuiFormControlLayout, EuiCheckbox, EuiRadio, or EuiSwitch**, ⚠️ make sure to QA your UI to ensure no visual regressions have occurred! ⚠️ --- ## [`v95.9.0`](https://github.com/elastic/eui/releases/v95.9.0) - Updated `EuiSearchBar`'s optional `box.schema` prop with a new `recognizedFields` configuration. This allows specifying the phrases that will be parsed as field clauses ([#7960](elastic/eui#7960)) - Updated `EuiIcon` with a new `tokenSemanticText` glyph ([#7971](elastic/eui#7971)) - Added support for TypeScript 5 ([#7980](elastic/eui#7980)) **Bug fixes** - Fixed `EuiSelectableTemplateSitewide` styles when used within a dark-themed `EuiHeader` ([#7977](elastic/eui#7977)) ## [`v95.8.0`](https://github.com/elastic/eui/releases/v95.8.0) - Updated `EuiHeaderLinks`'s mobile menu to set a slight popover padding by default ([#7961](elastic/eui#7961)) - This can be overridden via `popoverProps.panelPaddingSize` if needed. - Updated `EuiHeaderLink` to default to a size of `s` (down from `m`) ([#7961](elastic/eui#7961)) **Accessibility** - Updated the `aria-label` attribute for the `EuiFieldSearch` clear button ([#7970](elastic/eui#7970)) **Bug fixes** - Fixed a visual bug with `<EuiDualRange showInput="inputWithPopover" />` form controls ([#7957](elastic/eui#7957)) **Deprecations** - Deprecated `EuiFormRow`'s `columnCompressedSwitch` display prop. Use `columnCompressed` instead, which will automatically account for child `EuiSwitch`es ([#7968](elastic/eui#7968)) - Deprecated `EuiFormRow`'s `rowCompressed` display prop. Use `row` instead for vertical forms, or `centerCompressed` for inline forms ([#7968](elastic/eui#7968)) - (Styling) Updated `EuiFormRow`'s `hasEmptySpaceLabel` prop to no longer attempt to automatically align its content to a vertical center. Use the `display="center"` prop for that instead ([#7968](elastic/eui#7968)) **CSS-in-JS conversions** - Converted `EuiFormControlLayout` to Emotion ([#7954](elastic/eui#7954)) - Removed `.euiFormControlLayout--*icons` classNames and `--eui-form-control-layout-icons-padding` CSS var. Use `--euiFormControlRightIconsCount` or `--euiFormControlLeftIconsCount` instead - Converted `EuiFormLayoutDelimited` to Emotion ([#7957](elastic/eui#7957)) - Fixed `cloneElementWithCss` throwing an error when used multiple times without a `key` prop ([#7957](elastic/eui#7957)) - Updated `cloneElementWithCss` utility to support a third argument that allows prepending vs. appending the cloned Emotion css className ([#7957](elastic/eui#7957)) - Removed `@euiFormControlLayoutClearIcon` Sass mixin ([#7959](elastic/eui#7959)) - Converted `EuiDescribedFormGroup` to Emotion ([#7964](elastic/eui#7964)) - Converted `EuiForm`, `EuiFormHelpText`, and `EuiFormErrorText` to Emotion ([#7966](elastic/eui#7966)) - Converted `EuiFormLabel` and `EuiFormLegend` to Emotion; Removed `@euiFormLabel` mixin ([#7967](elastic/eui#7967)) - Converted `EuiFormRow` to Emotion ([#7968](elastic/eui#7968)) - Converted `EuiCheckbox` to Emotion ([#7969](elastic/eui#7969)) - Converted `EuiRadio` to Emotion ([#7969](elastic/eui#7969)) - Converted `EuiSwitch` to Emotion ([#7969](elastic/eui#7969)) - Removed the following Sass variables: ([#7969](elastic/eui#7969)) - `$euiFormCustomControlDisabledIconColor` - `$euiFormCustomControlBorderColor` - `$euiRadioSize` - `$euiCheckBoxSize` - `$euiCheckboxBorderRadius` - `$euiSwitchHeight` (and compressed/mini variants) - `$euiSwitchWidth` (and compressed/mini variants) - `$euiSwitchThumbSize` (and compressed/mini variants) - `$euiSwitchIconHeight` - `$euiSwitchOffColor` - Removed the following Sass mixins: ([#7969](elastic/eui#7969)) - `euiIconBackground` - `euiCustomControl` - `euiCustomControlSelected` - `euiCustomControlDisabled` - `euiCustomControlFocused` --------- Co-authored-by: Marta Bondyra <4283304+mbondyra@users.noreply.github.com>

ppisljar added the review label Aug 10, 2016

ppisljar assigned ycombinator and tbragin Aug 10, 2016

ppisljar force-pushed the fix/4599 branch from ad17af5 to db98f32 Compare August 10, 2016 10:23

ppisljar added the v5.0.0-beta1 label Aug 10, 2016

ycombinator added the v5.0.0 label Aug 10, 2016

ycombinator reviewed Aug 10, 2016
View reviewed changes

ppisljar assigned spalger Aug 10, 2016

ycombinator reviewed Aug 10, 2016
View reviewed changes

ycombinator removed their assignment Aug 10, 2016

ppisljar unassigned tbragin Aug 10, 2016

spalger assigned ppisljar and unassigned spalger Aug 10, 2016

ppisljar assigned spalger Aug 11, 2016

ppisljar added 12 commits September 12, 2016 20:21

fixes elastic#4599 - optional counts next to legend labels

e39ced7

fixes elastic#4599 - optional counts next to legend labels

bf14ca0

fixes elastic#4599 - optional counts next to legend labels

e4e24bb

adding option to line and area chart as well.

34f6c4a

changing option label to better reflect what it does

f8cdbac

changing to only show counts for Count metric

2f4f291

...

ee24fb1

adding support to pie charts

3c938b1

fixing tests

291cb5b

showing counts for sum and cardinality metrics as well.

4697859

adding documentation

9e429e1

changing label to show sum of series

3eeed65

ppisljar force-pushed the fix/4599 branch from 0f560b4 to 3eeed65 Compare September 12, 2016 18:22

epixa removed the v5.0.0-beta1 label Sep 13, 2016

ppisljar added v5.1.0 and removed v5.0.0 labels Oct 3, 2016

epixa added the discuss label Oct 8, 2016

spalger removed the discuss label Oct 17, 2016

spalger added v6.0.0 and removed review labels Oct 24, 2016

spalger removed their assignment Oct 27, 2016

tbragin added the Feature:Visualizations Generic visualization features (in case no more specific feature label is available) label Nov 12, 2016

ppisljar removed the v5.1.0 label Nov 14, 2016

ppisljar added the stalled label Dec 9, 2016

thomasneirynck closed this Feb 21, 2017

jimgoodwin added the v6.0.0-rc1 label Sep 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "Sum of series in legend" option #7970

Add "Sum of series in legend" option #7970

ppisljar commented Aug 10, 2016 •

edited

Loading

ycombinator commented Aug 10, 2016

ycombinator Aug 10, 2016

ycombinator commented Aug 10, 2016

ycombinator commented Aug 10, 2016

ycombinator Aug 10, 2016

ycombinator Aug 10, 2016

ycombinator Aug 10, 2016 •

edited

Loading

ycombinator commented Aug 10, 2016

spalger commented Aug 10, 2016

spalger commented Aug 10, 2016 •

edited

Loading

ppisljar commented Aug 11, 2016

spalger commented Aug 11, 2016 •

edited

Loading

spalger commented Aug 11, 2016

spalger commented Aug 11, 2016

ppisljar commented Aug 12, 2016 •

edited

Loading

tbragin commented Sep 12, 2016

spalger commented Oct 17, 2016

thomasneirynck commented Feb 21, 2017

Add "Sum of series in legend" option #7970

Add "Sum of series in legend" option #7970

Conversation

ppisljar commented Aug 10, 2016 • edited Loading

ycombinator commented Aug 10, 2016

ycombinator Aug 10, 2016

Choose a reason for hiding this comment

ycombinator commented Aug 10, 2016

ycombinator commented Aug 10, 2016

ycombinator Aug 10, 2016

Choose a reason for hiding this comment

ycombinator Aug 10, 2016

Choose a reason for hiding this comment

ycombinator Aug 10, 2016 • edited Loading

Choose a reason for hiding this comment

ycombinator commented Aug 10, 2016

spalger commented Aug 10, 2016

spalger commented Aug 10, 2016 • edited Loading

ppisljar commented Aug 11, 2016

spalger commented Aug 11, 2016 • edited Loading

spalger commented Aug 11, 2016

spalger commented Aug 11, 2016

ppisljar commented Aug 12, 2016 • edited Loading

tbragin commented Sep 12, 2016

spalger commented Oct 17, 2016

thomasneirynck commented Feb 21, 2017

ppisljar commented Aug 10, 2016 •

edited

Loading

ycombinator Aug 10, 2016 •

edited

Loading

spalger commented Aug 10, 2016 •

edited

Loading

spalger commented Aug 11, 2016 •

edited

Loading

ppisljar commented Aug 12, 2016 •

edited

Loading