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

[Dashboard] [Embeddable] Add Ability to Defer Embeddable Loaded State #107227

Merged
merged 11 commits into from
Aug 12, 2021

Conversation

ThomThomson
Copy link
Contributor

@ThomThomson ThomThomson commented Jul 29, 2021

Defer Embeddable Loading

This PR introduces a new opt-in method for embeddables to defer the completion of their loading state until after their output has been completely initialized.

Previously, all embeddables were expected to have their output initialized via the factory create method & the embeddable constructor. This meant that the container could immediately set each embeddable's finished state to true as soon as the constructor returned an embeddable. As part of Time to Visualize, some embeddables were simplified to initialize their output after the constructor, so that their input could be either by value or by reference.

This PR changes Lens and Maps embeddables to defer their loading state in this way.

Dashboard Index Patterns

In the initial setup for Time to Visualize, panels could be fully loaded before updating their index patterns. This meant that dashboard needed to check for new index patterns on every output update on every one of its children. By delaying the loaded state until the embeddable setup is complete, this is no longer necessary. In this PR the dashboard index pattern subscription only listens to output updates from the container itself.

Additionally, a new check has been added to cover the case where:

  • no index patterns have been returned yet
  • There is at least one child on the page
  • There is at least one child loading

If this case is met, the default index pattern will not be loaded. This fixes #105182

The dashboard app has also been changed to no longer require an index pattern in order to render. This can speed up the initial rendering of the dashboard slightly. It will also introduce some mild layout shifting until #107041 is resolved.

Potential Followup

This change can be a major step towards removing the requirement for embeddable factories to have both create and createFromSavedObject methods.

Checklist

Delete any items that are not applicable to this PR.

…ch aren't finished loading after their constructor is finished
@ThomThomson ThomThomson requested a review from a team July 29, 2021 19:07
@ThomThomson ThomThomson requested review from a team as code owners July 29, 2021 19:07
@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label Jul 29, 2021
@ThomThomson ThomThomson marked this pull request as draft July 29, 2021 19:08
@ThomThomson ThomThomson added Feature:Dashboard Dashboard related features Team:AppServices Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.14.1 v7.15.0 v8.0.0 impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:medium Medium Level of Effort impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. and removed impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. labels Jul 29, 2021
@ThomThomson
Copy link
Contributor Author

@elasticmachine merge upstream master

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

looks good, I am in favor of going with such approach and don't have any better alternatives in mind

@ThomThomson ThomThomson marked this pull request as ready for review August 10, 2021 18:19
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

@ThomThomson
Copy link
Contributor Author

@elasticmachine merge upstream

@ThomThomson ThomThomson requested a review from Dosant August 10, 2021 20:07
@nreese nreese mentioned this pull request Aug 10, 2021
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

maps changes LGTM

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

Changes LGTM, I tested different combinations of lenses and tsvbs that have multiple index patterns, didn't notice any regressions in terms of how index patterns and filtering works on a dashboard.
tested that no more wrongly requested default index pattern 👍

Copy link
Contributor

@crob611 crob611 left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Tested on Chrome Linux. It resolves the issue with requesting the default index pattern even if there is no visualization using it. Talked to Devon offline and suggested moving the parent check into the asbtract embeddable to make the usage in the embeddable a bit nicer to read, but so far everything looks fine for me.

@ThomThomson ThomThomson requested a review from a team as a code owner August 12, 2021 16:54
@ThomThomson
Copy link
Contributor Author

Great call @timroes , I've implemented a setInitializationFinished method on the abstract embeddable to make the implementation a little easier. Thanks again!

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

maps changes LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
dashboard 221.9KB 221.4KB -457.0B
lens 1.7MB 1.7MB +80.0B
maps 3.2MB 3.2MB +94.0B
total -283.0B

Page load bundle

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

id before after diff
embeddable 182.6KB 183.0KB +449.0B
Unknown metric groups

API count

id before after diff
embeddable 460 467 +7
maps 213 214 +1
total +8

API count missing comments

id before after diff
embeddable 388 391 +3
maps 212 213 +1
total +4

History

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

@ThomThomson ThomThomson merged commit 33f3933 into elastic:master Aug 12, 2021
ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Aug 12, 2021
…elastic#107227)

Added defer embeddable loading flag to embeddable for embeddables which aren't finished loading after their constructor is finished
ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Aug 12, 2021
…elastic#107227)

Added defer embeddable loading flag to embeddable for embeddables which aren't finished loading after their constructor is finished
ThomThomson added a commit that referenced this pull request Aug 12, 2021
…#107227) (#108460)

Added defer embeddable loading flag to embeddable for embeddables which aren't finished loading after their constructor is finished
ThomThomson added a commit that referenced this pull request Aug 13, 2021
…#107227) (#108459)

Added defer embeddable loading flag to embeddable for embeddables which aren't finished loading after their constructor is finished
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features Feature:Embedding Embedding content via iFrame impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:medium Medium Level of Effort release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.14.1 v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Visualizations request wrongly default index pattern on dashboard
7 participants