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.

Sign up for GitHub

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

Jump to bottom

[Lens] Duplicated datatable available as inspector data for Heatmap chart #126786

Merged
merged 7 commits into from
Mar 16, 2022
Merged

[Lens] Duplicated datatable available as inspector data for Heatmap chart #126786

merged 7 commits into from
Mar 16, 2022

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Mar 3, 2022 •
edited
Loading

Closes: #123176, Closes: #126886

Summary

[Lens] Duplicated datatable available as inspector data for Heatmap chart

Describe the bug:

Since #120239 PR the Heatmap visualization has been unified between Lens, Visualize and Canvas.
The new shared function contains an inspector code to record the datatable with the default id, but because in Lens the inspector registers the data within the mergeTables expression function (executed earlier on) using the layer generated uuid, the result is a duplicated datatable in the Inspector (or in the CSV download).

Screen:

Screen.Recording.2022-03-14.at.10.45.37.AM.mov

[Lens] Datatables are not cleared on layer removal

Describe the bug:

Internal datatables are not cleaned up on layer removal in the configuration, appending any new table to the same multitable data structure.

To reproduce create a Lens visualization, then add a new layer and configure it, then remove it. Open the Inspect panel and see that there are 2 tables available to inspect.
Same issues applies also for reference layers.

Screen:

Screen.Recording.2022-03-14.at.10.44.52.AM.mov

@alexwizp
Copy link
Contributor Author

alexwizp commented Mar 8, 2022

@elasticmachine merge upstream

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp alexwizp requested a review from dej611 March 10, 2022 15:04
@dej611
Copy link
Contributor

dej611 commented Mar 10, 2022

Checked this PR and it fixes also #126886 👍

@alexwizp alexwizp force-pushed the #123176 branch 2 times, most recently from 5f41118 to 1ffec98 Compare March 11, 2022 09:57
@alexwizp alexwizp self-assigned this Mar 14, 2022
@alexwizp alexwizp added v8.2.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens release_note:fix labels Mar 14, 2022
@elastic elastic deleted a comment from kibana-ci Mar 14, 2022
@alexwizp alexwizp marked this pull request as ready for review March 14, 2022 08:48
@alexwizp alexwizp requested review from a team as code owners March 14, 2022 08:48
@elasticmachine
Copy link
Contributor

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

@botelastic botelastic bot added the Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) label Mar 14, 2022
ppisljar
ppisljar approved these changes Mar 14, 2022
View reviewed changes
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

app services code LGTM

@flash1293 flash1293 mentioned this pull request Mar 15, 2022
Closed
@flash1293
Copy link
Contributor

@elasticmachine merge upstream

flash1293
flash1293 approved these changes Mar 15, 2022
View reviewed changes
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.

Tested and didn't manage to break it. Left one thing that should be addressed before merging - approving as I don't think it needs another round of review

...ck/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx

dispatchLens(
onActiveDataChange(
Object.entries(adapters.tables?.tables).reduce<Record<string, Datatable>>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to @andrewctate We need to do something similar on your PR which brings "show underlying data" to the dashboard as a panel action - otherwise it won't have the required active data around in all cases

...ck/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx Outdated
}
},
[dispatchLens]
[datasourceLayers, dispatchLens]
Copy link
Contributor

Choose a reason for hiding this comment

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

This will cause a lot of re-evaluations of this callback as datasourceLayers changes often. Let's move the default layer id calculation (const [defaultLayerId] = Object.keys(datasourceLayers);) into the component itself and pass defaultLayerId into the hook as a dependency instead.

@alexwizp alexwizp enabled auto-merge (squash) March 16, 2022 07:51
@alexwizp alexwizp added the backport:skip This commit does not require backporting label Mar 16, 2022
@alexwizp alexwizp merged commit 11451ad into elastic:main Mar 16, 2022
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 744 745 +1

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
expressions 1696 1698 +2

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.1MB 1.1MB +360.0B

Page load bundle

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

id before after diff
expressions 88.3KB 88.4KB +56.0B
lens 42.8KB 43.0KB +243.0B
total +299.0B
Unknown metric groups

API count

id before after diff
expressions 2140 2142 +2

History

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

cc @alexwizp

@mbondyra mbondyra mentioned this pull request Mar 16, 2022
Merged
maksimkovalev pushed a commit to maksimkovalev/kibana that referenced this pull request Mar 18, 2022
@alexwizp @kibanamachine @maksimkovalev
[Lens] Duplicated datatable available as inspector data for Heatmap c…
327154f 
…hart (elastic#126786)

* [Lens] Duplicated datatable available as inspector data for Heatmap chart

Close: elastic#123176

* update workspace_panel

* revert allowCsvExport

* fix CI

* fix PR comments

* apply pr comments

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
Reviewers

@ppisljar ppisljar ppisljar approved these changes

@flash1293 flash1293 flash1293 approved these changes

@dej611 dej611 Awaiting requested review from dej611

Assignees

@alexwizp alexwizp

Labels
backport:skip This commit does not require backporting Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Feature:Lens release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.2.0
Projects
None yet
Milestone
No milestone
7 participants
@alexwizp @dej611 @elasticmachine @flash1293 @kibana-ci @ppisljar @kibanamachine