Kibana: [APM] Design review: Charts replacement

Created on 24 Nov 2020  ยท  20Comments  ยท  Source: elastic/kibana

Summary

In our current effort to replace our existing charts with Elastic Charts, I've discovered a few issues that we should take a look at before shipping the replacement.

I've divided the issues into blockers and polish in order to help prioritization a bit before FF.

Blocked by Elastic charts team

Clickable buckets regardless of height

Kapture 2020-11-23 at 15 26 43

Aggregate value in legend

โš ๏ธ Moved to issue https://github.com/elastic/kibana/issues/84478

  • [ ] Legends don't show aggregate values. Duration should have an avg. aggregate of the selected time period. ๐Ÿ”ด๐Ÿ”ดโšช๏ธโšช๏ธโšช๏ธ

    ๐Ÿ‘€ View screenshot
    Screenshot 2020-11-23 at 15 25 24

Aggregate value in legend for errors

โš ๏ธ Moved to issue https://github.com/elastic/kibana/issues/84478

  • [ ] The error occurrences chart legend shows average, but it looks like it's the last bucket value. ๐Ÿ”ด๐Ÿ”ดโšช๏ธโšช๏ธโšช๏ธ

    ๐Ÿ‘€ View screenshot
    Screenshot 2020-11-24 at 09 36 39
    Screenshot 2020-11-24 at 09 38 42

Both issues above are already described as part of https://github.com/elastic/kibana/pull/80298 - Charts issue https://github.com/elastic/elastic-charts/issues/561

Tooltip displayed on top of menu

๐Ÿ‘‰ Charts team have opened an enhancement issue for this https://github.com/elastic/elastic-charts/issues/922

  • [x] The chart tooltips show above the Kibana header. Either the z-level should be reduced or we should opt not to show the tooltips if the charts are not in view (not sure if this is feasible)

    ๐Ÿ‘€ View screenshot
    Screenshot 2020-11-25 at 12 50 46

Blockers

  • [x] When browsing around and choosing different time ranges, the charts are still returning empty after initial displaying data. This makes for a very jarring experience ๐Ÿ”ด๐Ÿ”ด๐Ÿ”ด๐Ÿ”ด๐Ÿ”ด

    ๐Ÿ‘€ View screenshot

Kapture 2020-11-24 at 09 41 07

  • [x] ML integration displaying anomaly detection job results are not displaying as expected. Opened its own issue (https://github.com/elastic/kibana/issues/84185) https://github.com/elastic/kibana/issues/84185, but is it considered critical for shipping the charts replacement ๐Ÿ”ด๐Ÿ”ด๐Ÿ”ด๐Ÿ”ดโšช๏ธ
  • [ ] Y-axis doesn't show maxValue in the ticks. Problematic when the graphs go beyond the upper tick. ๐Ÿ”ด๐Ÿ”ด๐Ÿ”ด๐Ÿ”ดโšช๏ธ

โš ๏ธ Moved to https://github.com/elastic/kibana/issues/84482


๐Ÿ‘€ View screenshot
Screenshot 2020-11-23 at 15 19 30

Polish

  • [ ] The amount of ticks in various time ranges selected are not pretty. We previously made an effort in reducing the number of ticks. Not sure if anything here is configurable?

โš ๏ธ Moved to enhancement issue https://github.com/elastic/kibana/issues/84481


๐Ÿ‘€ View screenshot
Screenshot 2020-11-24 at 09 07 55

- [ ] Increase the number of legend items we display in the legend to 4 or 5. We currently reserve too much space for variable content.


๐Ÿ‘€ View screenshot
Screenshot 2020-11-24 at 09 09 22

  • [x] Y-axis ticks don't show maxValue or any other ticks for percentages


    ๐Ÿ‘€ View screenshot
    Screenshot 2020-11-24 at 09 28 59

  • [x] Chart tooltips should be made more legible. Reduce the timestamp to only hh:mm:ss instead of showing the absolute (this is also the case in our existing charts, I would just prefer to change it)


    ๐Ÿ‘€ View screenshot
    Screenshot 2020-11-24 at 09 30 33

apm v7.11.0

Most helpful comment

@formgeist we just released a new version of charts that includes the ability to add a listener to a bucket click: 24.2.0
Please refer to the PR description for a general overview: https://github.com/elastic/elastic-charts/pull/913
and the storybook example for a quick look into that https://elastic.github.io/elastic-charts/?path=/story/interactions--bar-clicks-and-hovers (the github page didn't pick up yet the update, but please check again later)

We also have a PR to update the package on Kibana but it's currently on hold due to an issue with renovatebot.
https://github.com/elastic/kibana/pull/84223 We will update and merge that PR as soon as that issue is solved

All 20 comments

Pinging @elastic/apm-ui (Team:apm)

Chart tooltips should be made more legible. Reduce the timestamp to only hh:mm:ss instead of showing the absolute (this is also the case in our existing charts, I would just prefer to change it)

@formgeist The date format, in this case, is not absolute. It's dynamically calculated based on the difference between the dates. This Function is used to calculate it

And this is the possible output:

* | Difference     |           Format                               |
* | -------------- |------------------------------------------------|
* | >= 5 years     | YYYY - YYYY                                    |
* | >= 5 months    | MMM YYYY - MMM YYYY                            |
* | > 1 day        | MMM D, YYYY - MMM D, YYYY                      |
* | >= 5 hours     | MMM D, YYYY, HH:mm - HH:mm (UTC)               |
* | >= 5 minutes   | MMM D, YYYY, HH:mm:ss - HH:mm:ss (UTC)         |
* | default        | MMM D, YYYY, HH:mm:ss.SSS - HH:mm:ss.SSS (UTC) |

In your case:
Screenshot 2020-11-25 at 11 57 43

The difference is less than 5 minutes, that's why it formates as milliseconds.

I can see if it's possible to increase the width of the tooltip to have it on a single line. WDYT?

@cauemarcondes Thanks for explaining - I'm surprised that we opt to show milliseconds for anything below 5 minutes. That's still a really long time, and the issue I see is that those labels are really hard to process for a user even if we make the tooltip wider to accommodate the longer labels. I would much prefer us to reconsider the formats so we're presenting rounded values instead of what I refer to as "absolute". If we take the common case, using our default range, the user will see this;

Screenshot 2020-11-25 at 12 31 58

I don't think it's necessary to have the milliseconds in there unless the difference is perhaps less than a second. I understand this is pretty aggressive formatting of time, but I think it's for the benefit of improved legibility of the labels. cc @sqren thoughts?

Hey Team, I'm not 100% sure but from the screenshots, it looks like you are not using the EUI Chart theme, that is the default theme to use for charts inside Kibana.
There is a service that you should use to get the correct theme (it also allows you to get the right one if you are in darkmode)
https://github.com/elastic/kibana/blob/master/src/plugins/charts/public/services/theme/README.md

EUI decided design and manage this theme outside the elastic-charts library, so unfortunately at the moment, you have to use it from the services mentioned above and not from the one coming with the library.

Hey Team, I'm not 100% sure but from the screenshots, it looks like you are not using the EUI Chart theme, that is the default theme to use for charts inside Kibana.
There is a service that you should use to get the correct theme (it also allows you to get the right one if you are in darkmode)

hey @markov00 thanks for your comment, I'm going to look into it.

@cauemarcondes I added a new polish issue in the issue description where the chart tooltips are showing on top of the Kibana header.

Screenshot 2020-11-25 at 12 50 46

Hey Team, I'm not 100% sure but from the screenshots, it looks like you are not using the EUI Chart theme, that is the default theme to use for charts inside Kibana.

@markov00 can you tell me how you can tell we are not using the theme correctly based on the images?

@cauemarcondes from the last shared set of images (the one with the tooltips) seems that you are using the EUI theme. I've wrote that comment after seeing the image shared here: https://github.com/elastic/kibana/issues/84178#issuecomment-733652626The tick lines on the axis are not enabled by default on the EUI theme

@cauemarcondes I added a new polish issue in the issue description where the chart tooltips are showing on top of the Kibana header.
@formgeist for this issue you can use an option called boundary in the tooltip configuration that allows you to specify a DOM element to contain tooltip. @nickofthyme can provide more details. You can also check this story for an example usage https://elastic.github.io/elastic-charts/?path=/story/bar-chart--test-tooltip-and-rotation

for this issue you can use an option called boundary in the tooltip configuration that allows you to specify a DOM element to contain tooltip

That's for the tip @markov00

for this issue you can use an option called boundary in the tooltip configuration that allows you to specify a DOM element to contain tooltip. @nickofthyme can provide more details

@markov00 @nickofthyme About the tooltip boundary, I create a simple example here to reproduce the problem, which is the tooltip being shown on top of the menu.

I believe only adding boundary is not enough, because the chart is still visible, but behind the menu, I think the real problem is that the z-index of the Header is set to 1000 and the tooltip is set to 1000000, making it to show on top of all elements.

~@cauemarcondes I don't understand the issue based on the example you posted. The tooltip appears to function as expected.~

Nvm, I see this issue now. Checking...

Screen Recording 2020-11-25 at 08 23 AM

The boundary prop is the element in which the tooltip will _attempt to contain itself within_, this is dependent on the placement and fallbackPlacements. The default value for the boundary is the first parent scroll container on the dom tree. If you pass the string 'chart' it will use the chart element itself as the boundary, but again there is no guarantee that the tooltip will be inside of the chart depending on the tooltip and chart dimensions, the placement and fallbackPlacements.

If you are trying to place a menu element above the chart via some absolute positioning, the boundary will not help.

@nickofthyme This is what I what to avoid:

Screenshot 2020-11-25 at 15 43 42

The tooltip showing on top of the menu. As you mentioned I think it's not possible to fix it with boundary, am I correct?

Oh I see, yeah that's tough I don't think the boundary is not gonna help you when the menu being position: fixed, or absolute for that matter, and the containing chart element is a scroll container. Because the scroll container dimensions are moving with the scroll so it will never block the placement.

Still looking to see if there is some type of hack to make it work.

Update:
So it looks like setting the tooltip z-index is the best option. We can add the option to our theme and set the value to be under the kibana menus.

Update:
So it looks like setting the tooltip z-index is the best option. We can add the option to our theme and set the value to be under the kibana menus.

Thanks @nickofthyme , I see that you've already created the issue. ๐ŸŽ‰

@formgeist we just released a new version of charts that includes the ability to add a listener to a bucket click: 24.2.0
Please refer to the PR description for a general overview: https://github.com/elastic/elastic-charts/pull/913
and the storybook example for a quick look into that https://elastic.github.io/elastic-charts/?path=/story/interactions--bar-clicks-and-hovers (the github page didn't pick up yet the update, but please check again later)

We also have a PR to update the package on Kibana but it's currently on hold due to an issue with renovatebot.
https://github.com/elastic/kibana/pull/84223 We will update and merge that PR as soon as that issue is solved

@markov00 Fantastic! Thanks for the quick turn around on this ๐Ÿ‘

I don't think it's necessary to have the milliseconds in there unless the difference is perhaps less than a second. I understand this is pretty aggressive formatting of time, but I think it's for the benefit of improved legibility of the labels. cc @sqren thoughts?

We could definitely re-think the time bounders of that function. @sqren WDYT?

I agree, having milliseconds is quite noisy and it's better only to show it for small durations (less than 10 seconds or so).

I've opened a number of follow up issues in order to close this one out. There's a list of references available above. Considering most of the critical blockers have been resolved by a first pass PR and help from the charts team I'll close this issue.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

timroes picture timroes  ยท  3Comments

bhavyarm picture bhavyarm  ยท  3Comments

mark54g picture mark54g  ยท  3Comments

timroes picture timroes  ยท  3Comments

MaartenUreel picture MaartenUreel  ยท  3Comments