[Pie] Loads the no results screen if all slices have zero value #111931

stratoula · 2021-09-13T10:02:30Z

Summary

This PR fixes a problem that occurs in the legacy pie (es-charts implementation). What happens here is that when we have data with zero values it fallbacks to Number.EPSILON. This is an actual value and es-charts renders it causing this weird behavior. I changed it to fallback to zero. When the slice value is zero nothing is rendered from the es-charts side. In order to not render a blank component I added a no results component. So if I find that all the metric values are zero then I render this component.

I also investigated how Lens behaves. I can't break it. Lens works correctly for zero values. But as I think that this fallback to epsilon doesn't make sense I also changed a bit the getSliceValue to return zero as a fallback. It seems that nothing breaks with this change.

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

src/plugins/vis_types/pie/public/pie_component.tsx

x-pack/plugins/lens/public/pie_visualization/render_helpers.test.ts

x-pack/plugins/lens/public/pie_visualization/render_helpers.ts

monfera · 2021-09-13T12:12:10Z

Just wanted to mention here too the result of our chat with @stratoula: while there's no record of the driver for adding an epsilon in the very 1st commit of the original Lens PR, it may have been motivated by a desire to show zero valued legend items, which, in the absence of other arguments, is not the best default (there can be a high number of zero-valued items, all with color assignment, which don't show up as slices, so it's generally not valuable or sensible for the user).

So I think the better baseline behavior is to not have legends for zero valued items, unless a real driver becomes known. Product needs to know about this facet, and if there's significant user demand, it's feasible for Kibana to add a toggle for showing zero values, as well as Charts to optionally enable zero-valued items in the legend without a Kibana-side EPSILON hack.

monfera · 2021-09-13T12:18:09Z

This PR is actually two things:

adding a "no data" overlay (please see the related Charts discuss issue here)
removing the EPSILON

Why to make this distinction, when both aspects are linked to the referred issue #111743? Because that issue only talks about a use case of all-zero slices, and neither the issue nor the PR discusses the more general circumstance of having some zero slices, which was the likely target of the EPSILON hack (and the special case of all zeros may not have been considered, or a check regressed since)

stratoula · 2021-09-13T12:29:42Z

Thanx @monfera for your comments :)

Another thing that I decided to do in this PR is something that Lens already does. I try to find out if the metric has negative values and display the corresponding message

src/plugins/vis_types/pie/public/components/visualization_noresults.tsx

src/plugins/vis_types/pie/public/pie_component.tsx

elasticmachine · 2021-09-14T06:06:14Z

Pinging @elastic/kibana-vis-editors (Team:VisEditors)

dej611

Did a first code review. Left some minor comments.

src/plugins/vis_types/pie/public/components/visualization_noresults.tsx

dej611 · 2021-09-14T08:48:26Z

src/plugins/vis_types/pie/public/pie_component.tsx

-    }
-    return Number.EPSILON;
+    const value = d[metricColumn.id];
+    return Number.isFinite(value) && value >= 0 ? value : 0;


Can it be Infinity/-Infinity?

Suggested change

return Number.isFinite(value) && value >= 0 ? value : 0;

return Math.max(0, value);

@monfera as you asked about this change wdyt?

With the formula usage, it can be, right? count()/0

I think in formula that is handled by the expression system. Need to check that.

src/plugins/vis_types/pie/public/pie_component.tsx

dej611 · 2021-09-14T08:55:08Z

x-pack/plugins/lens/public/pie_visualization/render_helpers.ts

-  }
-  return Number.EPSILON;
+  const value = d[metricColumn.id];
+  return Number.isFinite(value) && value >= 0 ? value : 0;


dej611

Some more feedback from testing it:

this more an edge case, but when sizing pie slices by Top Hit + Aggregate with any function for negative or all zero values it shows an empty workspace:

Underneath the server is returning undefined values for each row value:

The legacy version didn't work well either, showing a 0% label:

In Lens the negative message is way too long for the suggestions, perhaps it can be improved also for the main workspace:

stratoula · 2021-09-14T11:12:10Z

@elasticmachine merge upstream

stratoula · 2021-09-14T11:32:21Z

Discussed it offline with @dej611. In the example above he used a runtime field which doesn't work currently with top_hit. Here is the PR that fixes it #109531

About the Lens message, I agree, let's make it smaller. Any suggestions?

mbondyra · 2021-09-14T12:54:59Z

For the error messages, I'd just remove 'try a different visualization type'. Suggestions kind of show it anyway. CC @MichaelMarcialis

I tested Lens only so far with different configurations and all work fine.

‘0’ formula
Count()/0 formula
Filters that filter data to 0 (either one of them or all of them)
Dataset with empty fields only in considered time span
Cases from above on 2 or three“ slice by”
Intervals (I actually found unrelated issue here, will submit a separate PR)
Combinations of top values and filters, timestamp and intervals

src/plugins/vis_types/pie/public/pie_component.test.tsx

Co-authored-by: Marta Bondyra <marta.bondyra@gmail.com>

mbondyra

Tested Lens thoroughly and Pie chart a bit less, but still trying to think about all the edgecases. Code LGTM 👌🏼

kibanamachine · 2021-09-15T11:43:18Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`visTypePie`	51	52	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`lens`	1.6MB	1.6MB	-57.0B
`visTypePie`	75.9KB	77.0KB	+1.1KB
total			+1.0KB

History

💚 Build #153438 succeeded 99d4020
💚 Build #153150 succeeded c3535a2
💚 Build #153128 succeeded daf5628
💚 Build #152903 succeeded 84ebcf7
💚 Build #152814 succeeded c7e0cf9

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…tic#111931) * [Pie] Loads the no results screen if all slices have zero value * Add a functional test * Apply PR changes * Display no results component if the chart metric has negative values * Nits * Apply some of the PR comments * Change the negative values text * Update src/plugins/vis_types/pie/public/pie_component.test.tsx Co-authored-by: Marta Bondyra <marta.bondyra@gmail.com> * Update src/plugins/vis_types/pie/public/pie_component.test.tsx Co-authored-by: Marta Bondyra <marta.bondyra@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Marta Bondyra <marta.bondyra@gmail.com>

kibanamachine · 2021-09-15T12:14:28Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

) (#112250) * [Pie] Loads the no results screen if all slices have zero value * Add a functional test * Apply PR changes * Display no results component if the chart metric has negative values * Nits * Apply some of the PR comments * Change the negative values text * Update src/plugins/vis_types/pie/public/pie_component.test.tsx Co-authored-by: Marta Bondyra <marta.bondyra@gmail.com> * Update src/plugins/vis_types/pie/public/pie_component.test.tsx Co-authored-by: Marta Bondyra <marta.bondyra@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Marta Bondyra <marta.bondyra@gmail.com> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> Co-authored-by: Marta Bondyra <marta.bondyra@gmail.com>

stratoula added 2 commits September 13, 2021 13:01

[Pie] Loads the no results screen if all slices have zero value

da7de0e

Add a functional test

b4b523d