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

[Lens] Heatmap / Swim lane integration #97978

Merged
merged 83 commits into from
Jun 4, 2021

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented Apr 22, 2021

Summary

Adds Heatmap/Swim lane chart by https:/elastic/elastic-charts to the Lens editor.

If you encountered any rendering-related issues please check a list of known issues and create a bug report if necessary.

Known issues/limitations

elastic-charts related

require changes on the Lens side

Heatmap / Swim lane chart behavior

Depending on the data type of the X-axis, the chart looks and behaves slightly differently.

TIme-series data

Chart example:
image

Brushing action applies a selected time range similar to the XY chart.
Jun-03-2021 13-36-10

Categorical data

Chart example:
image

The brushing action invokes filters based on selected X and Y values.
image

Checklist

@flash1293
Copy link
Contributor

flash1293 commented May 6, 2021

List of things I noticed:

  • Suggestions - when on broken down xy chart a lot of suggestions are rendered. I think we can solve this by this by emitting suggestions for reduced tables, but make them hidden (only showing it for unchanged tables)
  • As discussed, emit hidden suggestions for tables with just a single bucket dimension (mapping it to x axis and rendering without y axis)
  • Just in general, render the heatmap if there is no vertical axis as it still makes sense
  • Do not emit visible suggestions if heatmap is already the current visualization
  • Legend has two entries for "> 0"

Screenshot 2021-05-06 at 15 12 30

  • The top legend often looks broken - should we default to right legend?
  • Clicking a suggestion breaks Lens
  • Do not generate an expression in toExpression if metric is missing - right now it's just rendering an empty screen, but you can simply return null as an expression in this case
  • Why not allowing date histograms on the vertical axis? I think it makes sense to default it to the horizontal axis but IMHO we shouldn't completely prevent it
  • Filters by clicking not working yet

Let me know once the code is ready for a more detailed loo

@darnautov darnautov self-assigned this May 11, 2021
@flash1293
Copy link
Contributor

I started reviewing again, but it seems like most things from the list above are not addressed yet. Let me know once I can take another look. A few other things I noticed:

  • If y axis is missing, an error is shown now. This should render with a single "row" instead so the user sees what they configured so far (similar to )
  • There are a lot of timezone errors because your are directly using it from uiSettings - however the default setting is Browser which means the actual time zone has to be inferred from the browser. This is how xy chart is doing it:
    function getTimeZone(uiSettings: IUiSettingsClient) {
  • Starting from a broken down xy chart, there are two heatmap suggestions (because there are two tables with different nesting of the dimensions):

Screenshot 2021-05-12 at 10 46 37

* I don't think it makes sense to show both, we should probably hide suggestions based on the `reorder` change type

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

I found one other thing - if "Last value" is used as metric, it's possible array values are returned (like for products.price in the ecommerce sample data). In this case the chart is rendering "No results found".

This is fine for me, but on xy chart we show a warning in this case to make the user aware:
Screenshot 2021-06-03 at 16 52 19

Heatmap should do the same thing - it's this piece of code ( I guess it makes sense to :

getWarningMessages(state, frame) {

Otherwise this looks fine to me - going to approve after that.

@wylieconlon I think it's worth taking another look at that from your side if you have the time.

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Yes, I found the same issue as @flash1293 with the "Last value" function. It looks like it's actually a bug because the heatmap fails to render when using last value. You can reproduce this using the kibana_sample_data_ecommerce dataset using any products.* fields.

I also found another bug, the heatmap legend is not formatted. You can test this by using the Value formatter option on the metric, or by adding a default formatter to your index pattern:

Screen Shot 2021-06-03 at 10 42 51 AM

Other than these new issues, I think my previously found issues were all fixed.

@flash1293
Copy link
Contributor

It looks like it's actually a bug because the heatmap fails to render when using last value

It looks like right now it's excluding all array values, but rendering single values if there are any. IMHO this is an acceptable behavior for the beta version as long as we have the warning

@wylieconlon
Copy link
Contributor

@flash1293 The case I was running into is when every value is an array, the chart is completely blank, instead of showing an empty state or warning message.

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.

Left one additional small comment on re-review. Also responded to my previous comment on the EUI class targeting and styles with a possible solution.

Otherwise, this looks good from my perspective. Will approve once these changes are made. Thanks!

@darnautov
Copy link
Contributor Author

@wylieconlon I've created a PR in the elastic-charts repo to address an issue with the legend items formatting

@darnautov
Copy link
Contributor Author

Thanks for taking another look @MichaelMarcialis, I resolved your comments about the chart switcher in d699e9e

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM

Left two nits I would like to get addressed, but I don't think I have to review again for them.

Not sure whether we noted that down somewhere, but the legend seems to miss entries sometimes:
Screenshot 2021-06-04 at 13 39 03

No entry for the dark shade of red. Seems to be an elastic-charts issue

const valueFormatter = formatFactory(valueColumn.meta.params);

// Enable xDomain when https:/elastic/elastic-charts/issues/1165 is resolved.
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: We usually don't keep these thins in code, can you remove it from here and move it into an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in 9bb03df


// @ts-ignore
const onElementClick: ElementClickListener = (e: HeatmapElementEvent[]) => {
const cell = e[0][0];
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Please cast the (e: HeatmapElementEvent[]) => void to ElementClickListener instead of ignoring the ts error. That's retains at least some type safety.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in ef95471

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

LGTM, this is working very well in all the cases I could test!

I did find a new charts bug in my latest round of testing: elastic/elastic-charts#1192

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. I left two last nitpicky comments, but nothing worth holding you up for. Approving.

@darnautov darnautov enabled auto-merge (squash) June 4, 2021 17:45
@darnautov darnautov added the auto-backport Deprecated - use backport:version if exact versions are needed label Jun 4, 2021
@darnautov darnautov merged commit 0793753 into elastic:master Jun 4, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jun 4, 2021
* heatmap wip

* format values on X axis

* format values for cells

* show labels

* support legend configuration

* render preview

* add icon

* [ML] update visualization

* [ML] init suggestions

* [ML] fix preview

* [ML] fix groupPosition for the legend control

* [ML] add formatter for Y-axis

* [ML] filterOperations for cell value

* [ML] fill all available height

* [ML] delete unused file

* [ML] fix suggestion state

* [ML] update suggestion, add hiding logic

* [ML] suggestions unit tests

* [ML] rename legend interface

* [ML] unit tests for visualization, add error messages

* [ML] fix typos in xy visualization tests

* [ML] support click event

* [ML] add xDomain for time series data

* [ML] support empty Y axis

* [ML] change legend default position

* [ML] getTimeZone util

* [ML] hide suggestions for reorder

* [ML] support brush event

* [ML] update unit tests

* [ML] render grid lines in preview

* [ML] don't display errors on init

* [ML] utilize chartsThemeService

* [ML] support histogram for the vertical axis

* [ML] fix clearLayer

* [ML] show empty placeholder on no data

* [ML] fix X domain min

* [ML] reject suggestions for 3 or more  buckets

* [ML] suggestions for histograms and histogram for Y-axis

* [ML] fix unit tests

* [ML] update suggestions for active heatmap

* [ML] chart data test for heatmap

* [ML] test for transitioning from heatmap to barchart

* [ML] disable xDomain

* [ML] support intervals in axes configurations

* [ML] hide label on the vertical axis when there is only a horizontal dimension

* [ML] set min cell height for better suggestions preview rendering

* [ML] fix tooltip for empty vertical axis config

* [ML] fix click and brushing for empty Y axis

* [ML] update functional test

* [ML] show beta label

* [ML] fix legend control

* [ML] dataIndex sort by default for the X axis

* [ML] use euiPaletteForTemperature with quantize color scale

* [ML] hide all suggestions

* [ML] fix chart data extension issue

* [ML] fix the caret symbol positioning

* [ML] update unit tests for the heatmap suggestions

* [ML] replace EuiBetaBadge with EuiBadge

* [ML] update functional test

* [ML] fix chart switch styles

* [ML] fix functional test

* [ML] return null instead of expression with a missing value accessor

* [ML] use table id as a chart id

* [ML] fix scale type for a single row of data

* [ML] filter out undefined values

* [ML] fix isXAxisLabelVisible

* [ML] update chart_switch styles

* show warning message for the array values

* remove unused code

* replace ts-ignore with manual type casting

* add unit tests for error and warning messages

* add css class for append, conditional flex group
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jun 4, 2021
* heatmap wip

* format values on X axis

* format values for cells

* show labels

* support legend configuration

* render preview

* add icon

* [ML] update visualization

* [ML] init suggestions

* [ML] fix preview

* [ML] fix groupPosition for the legend control

* [ML] add formatter for Y-axis

* [ML] filterOperations for cell value

* [ML] fill all available height

* [ML] delete unused file

* [ML] fix suggestion state

* [ML] update suggestion, add hiding logic

* [ML] suggestions unit tests

* [ML] rename legend interface

* [ML] unit tests for visualization, add error messages

* [ML] fix typos in xy visualization tests

* [ML] support click event

* [ML] add xDomain for time series data

* [ML] support empty Y axis

* [ML] change legend default position

* [ML] getTimeZone util

* [ML] hide suggestions for reorder

* [ML] support brush event

* [ML] update unit tests

* [ML] render grid lines in preview

* [ML] don't display errors on init

* [ML] utilize chartsThemeService

* [ML] support histogram for the vertical axis

* [ML] fix clearLayer

* [ML] show empty placeholder on no data

* [ML] fix X domain min

* [ML] reject suggestions for 3 or more  buckets

* [ML] suggestions for histograms and histogram for Y-axis

* [ML] fix unit tests

* [ML] update suggestions for active heatmap

* [ML] chart data test for heatmap

* [ML] test for transitioning from heatmap to barchart

* [ML] disable xDomain

* [ML] support intervals in axes configurations

* [ML] hide label on the vertical axis when there is only a horizontal dimension

* [ML] set min cell height for better suggestions preview rendering

* [ML] fix tooltip for empty vertical axis config

* [ML] fix click and brushing for empty Y axis

* [ML] update functional test

* [ML] show beta label

* [ML] fix legend control

* [ML] dataIndex sort by default for the X axis

* [ML] use euiPaletteForTemperature with quantize color scale

* [ML] hide all suggestions

* [ML] fix chart data extension issue

* [ML] fix the caret symbol positioning

* [ML] update unit tests for the heatmap suggestions

* [ML] replace EuiBetaBadge with EuiBadge

* [ML] update functional test

* [ML] fix chart switch styles

* [ML] fix functional test

* [ML] return null instead of expression with a missing value accessor

* [ML] use table id as a chart id

* [ML] fix scale type for a single row of data

* [ML] filter out undefined values

* [ML] fix isXAxisLabelVisible

* [ML] update chart_switch styles

* show warning message for the array values

* remove unused code

* replace ts-ignore with manual type casting

* add unit tests for error and warning messages

* add css class for append, conditional flex group

Co-authored-by: Dima Arnautov <[email protected]>
@darnautov darnautov deleted the lens-heatmap-chart branch June 4, 2021 21:27
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 651 670 +19

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.3MB 1.3MB +45.6KB

Page load bundle

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

id before after diff
lens 34.6KB 38.1KB +3.4KB

History

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

cc @darnautov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Heatmap Heatmap visualization Feature:Lens release_note:enhancement v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants