Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Visualize] New heatmap implementation with elastic-charts #118338

Merged
merged 53 commits into from
Nov 30, 2021

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented Nov 11, 2021

Summary

Part of #118516
Part of #103633
Partially fixes #30681 (adds a grid)
Closes #13926
Closes #10648
Closes #12666

PR's output

  • Creates a new visTypeHeatmap plugin with the new implementation of the visualize heatmap chart. The new implementation is using the elastic-charts library and is NOT the default. In order to enable it, you have to switch off the corresponding switch from the advanced settings.
    image
  • Creates a new expressionHeatmap plugin which has the expression function and renderer of the heatmap chart. These are used by both Lens and Visualize editors 🎉

Vislib heatmap vs New implementation

  • Color scale (is not working correctly in vislib) is not supported
  • Rotate and overwrite color values are not supported
  • We always scale to data bounds. This is super important and the main reason that the users might encounter some differences between the charts (the charts that have the Scale to data bounds switch off

For the above, we will expect the user's feedback and decide if we would like to support them in the future

The above are blocked from EC side and we would like to support them in order to make the new implementation the default

Screenshots

dynamic ranges
image

percentage mode
image

custom ranges and overwrite color
image

Checklist

@elastic elastic deleted a comment from kibanamachine Nov 12, 2021
@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula
Copy link
Contributor Author

Hey, @stratoula. This is looking great so far! I'm leaving you a few comments and questions below for your review. Thanks!

  • Is it typical for Kibana to have deprecated advanced settings that are enabled by default? I was just curious, as it felt odd to not default to the new elastic charts heatmap (if the previous implementation has been officially deprecated). Alternatively, we could also flip it so that the new heatmap implementation is a beta/experimental feature that is off by default (that makes more sense in my head). Thoughts?

You are right! I removed the deprecated message. This should be deprecated when the new implementation is the default.

  • I believe we also have this issue in Lens, but I noticed that lengthy y-axis labels can cause the available horizontal space for the visualization to be extremely small. Do we have any ability to truncate or break lines for these labels?

Yes this also exists in Lens. @markov00 is this possible? I would suggest if this is possible to address it in different PR though.

image

  • There doesn't appear to be any whitespace around the edges of the visualization. This has the potential to cause collisions between the visualization and the bottom-left legend toggle button. Is it possible to bake in some whitespace into the visualization to avoid such collisions?

image

I added more padding and I think it looks better now. A note here: One thing missing are the axis titles. When they will be supported it will give more space to the axis and we won't have this kind of problems.

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retested thoroughly and couldn't find anything wrong! Code LGTM 👌🏼

Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested in Safari and all works fine 👍

@stratoula
Copy link
Contributor Author

@MichaelMarcialis are you ok with the current status? I need your approval to merge :D

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
charts 65 66 +1
expressionHeatmap - 29 +29
lens 683 672 -11
visTypeHeatmap - 16 +16
visTypeVislib 186 184 -2
visualize 61 62 +1
total +34

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
charts 278 281 +3
expressionHeatmap - 109 +109
visTypeHeatmap - 3 +3
total +115

Async chunks

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

id before after diff
lens 967.9KB 961.3KB -6.7KB
visTypeHeatmap - 7.1KB +7.1KB
visTypeVislib 359.4KB 354.7KB -4.7KB
visualize 50.1KB 51.5KB +1.3KB
total -2.9KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
charts 3 4 +1
expressionHeatmap - 3 +3
visTypeHeatmap - 2 +2
total +6

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
charts 56.2KB 56.6KB +412.0B
expressionHeatmap - 26.5KB +26.5KB
lens 44.2KB 40.3KB -3.9KB
visDefaultEditor 19.3KB 19.4KB +48.0B
visTypeHeatmap - 10.1KB +10.1KB
visTypeVislib 20.1KB 18.6KB -1.5KB
total +31.7KB
Unknown metric groups

API count

id before after diff
charts 310 313 +3
expressionHeatmap - 113 +113
visTypeHeatmap - 3 +3
total +119

async chunk count

id before after diff
visTypeHeatmap - 1 +1
visTypeVislib 4 3 -1
total -0

History

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

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making those changes, @stratoula. I have one last question below, but nothing worth holding you up over. Approving.

The question I have is regarding the legend toggle button. With the new padding you've added to the visualization container, should we remove the padding to the .visLegend__toggle button in order to reduce the chance of overlapping with the visualization and to keep it's position consistent with other legend toggle buttons (such as those in XY charts)?

@stratoula
Copy link
Contributor Author

Discussed offline with Michael, just adding it here for visibility. The .visLegend__toggle is used by the vislib heatmap, vislib pie and gauge/goal. I have made no changes to the vislib component so we could remove the padding from the toggle legend button but I suggest to do it in another PR as is irrelevant with the changes we made here

@stratoula stratoula merged commit fea4d2a into elastic:main Nov 30, 2021
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Nov 30, 2021
@MichaelMarcialis
Copy link
Contributor

@stratoula: Ignore my question above. I had forgotten to disable the legacy heatmaps in advanced settings. What you have here is good. Thanks again!

TinLe pushed a commit to TinLe/kibana that referenced this pull request Dec 22, 2021
…18338)

* [WIP][Heatmap] Creates implementation with elastic-charts

* Fix types and connection with vislib

* Add coloring options

* Brush, click events, coloring etc

* Cleaning up the expression function

* Add legend picker, fix sorting and other fixes

* Further fixes

* Use the shared expression to Lens and cleanup

* PrepareLogTables for new expression function

* Use common renderer and expression function with lens

* Fix i18n

* Small tweaks

* Add unit tests

* Adds a unit test to the heatmap component

* update plugin list

* Fix types

* Fix types

* update limits

* Change to the expression function

* Cleanup translations

* Refactor to use vis

* Fix types

* further cleanup of the translations

* register new setting

* Fix sorting for histogram

* Adds functional tests for the new nisualize heatmap

* Cleanup

* Fix

* Apply PR comments

* Address PR comments

* Fix i18n

* Fix i18n

* Makes the <Heatmap /> id dynamic

* reverse

* fix translation file

* Apply design PR comments

* Fix package

* More fixes

* Fix brush problem

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Heatmap Heatmap visualization release_note:feature Makes this part of the condensed release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.1.0
Projects
None yet
9 participants