[APM] Design review: Charts replacement

[formgeist]

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

• Fixed in feat: add projection click listener elastic-charts#913 – Transaction duration distribution barchart clickarea for the selection of bucket is only on the bar itself. Quite a big usability issue in this. If this is missing option in the charts, I would propose not to replace the distribution chart for this release. Open charts issue Enable click on the whole bucket elastic-charts#846 🔴🔴🔴🔴🔴
  [Caue]: Draft PR fixing it [APM] Fix Transaction duration distribution barchart clickarea #84394

👀 View screenshot

Aggregate value in legend

⚠️ Moved to issue #84478

• Legends don't show aggregate values. Duration should have an avg. aggregate of the selected time period. 🔴🔴⚪️⚪️⚪️

👀 View screenshot

Aggregate value in legend for errors

⚠️ Moved to issue #84478

• The error occurrences chart legend shows average, but it looks like it's the last bucket value. 🔴🔴⚪️⚪️⚪️

👀 View screenshot

Both issues above are already described as part of #80298 - Charts issue elastic/elastic-charts#561

Tooltip displayed on top of menu

👉 Charts team have opened an enhancement issue for this elastic/elastic-charts#922

• 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)
  [Caue]: A new issue has been created on Elastic charts, they need to expose a new property to fix it. Expose tooltip z-index elastic-charts#922

👀 View screenshot

Blockers

• 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 🔴🔴🔴🔴🔴
  [Caue]: I did a workaround to fix it, and created a new issue Chart disappears when rendering LineAnnotation in another Component elastic-charts#914

👀 View screenshot

• ML integration displaying anomaly detection job results are not displaying as expected. Opened its own issue ([APM] ML anomaly detection integration: Displaying anomaly job results in the Transaction duration chart is not as intended #84185) [APM] ML anomaly detection integration: Displaying anomaly job results in the Transaction duration chart is not as intended #84185, but is it considered critical for shipping the charts replacement 🔴🔴🔴🔴⚪️
  [Caue]: PR fixing it [APM] ML anomaly detection integration: Displaying anomaly job results in the Transaction duration chart is not as intended  #84415

• Y-axis doesn't show maxValue in the ticks. Problematic when the graphs go beyond the upper tick. 🔴🔴🔴🔴⚪️

⚠️ Moved to #84482

[Caue]: Unfortunatelly there's no way to manually set the maxValue, there's an open issue to make it possible: elastic/elastic-charts#397

👀 View screenshot

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 #84481

👀 View screenshot

- [ ] Increase the number of legend items we display in the legend to 4 or 5. We currently reserve too much space for variable content.
[Caue]: Unfortunately there's no way to reduce the space between the legend and its value. There's an open issue to improve the Legend, but it's not clear when it's going to be implemented. elastic/elastic-charts#580

👀 View screenshot

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

👀 View screenshot

• 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)
  [Caue]: PR fixing it: [APM] Adjust time formats based on the difference between start and end #84470

👀 View screenshot

[elasticmachine]

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

[cauemarcondes]

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:

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?

[formgeist]

@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;

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?

[markov00]

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.

[cauemarcondes]

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.

[formgeist]

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

[cauemarcondes]

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?

[markov00]

@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: #84178 (comment) tick lines on the axis are not enabled by default on the EUI theme

[markov00]

@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

[cauemarcondes]

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

[cauemarcondes]

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.

[nickofthyme]

@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...

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.

[cauemarcondes]

@nickofthyme This is what I what to avoid:

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?

[nickofthyme]

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.

[cauemarcondes]

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. 🎉

[markov00]

@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: elastic/elastic-charts#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.
#84223 We will update and merge that PR as soon as that issue is solved

[formgeist]

@markov00 Fantastic! Thanks for the quick turn around on this 👏

[cauemarcondes]

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?

[sorenlouv]

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

[formgeist]

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.