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] Lazy DashboardRenderer #192754

Merged
merged 27 commits into from
Sep 17, 2024
Merged

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Sep 12, 2024

Changes

  1. expose DashboardRenderer as lazy loaded component to reduce static page load size
  2. Use onApiAvailable prop to pass DashboardApi to parent instead of forwardRef
  3. Decouple DashboardApi from legacy embeddable system. This changed functions such as updateInput and using redux select to access state.

@nreese
Copy link
Contributor Author

nreese commented Sep 12, 2024

/ci

@nreese nreese changed the title Lazy dashboard renderer [dashboard] Migrate external usage of legacy DashboardContainer embeddable apis to new interface Sep 12, 2024
@nreese
Copy link
Contributor Author

nreese commented Sep 12, 2024

/ci

@nreese
Copy link
Contributor Author

nreese commented Sep 13, 2024

/ci

@nreese nreese changed the title [dashboard] Migrate external usage of legacy DashboardContainer embeddable apis to new interface [dashboard] Lazy DashboardRenderer Sep 13, 2024
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing this example because it uses getInput to get panel state. Instead, example will have to use dashboardApi.serializeState, but that method is not exposed yet. Since its just an example, removing now is the simplest fix.

@nreese
Copy link
Contributor Author

nreese commented Sep 13, 2024

/ci

@nreese
Copy link
Contributor Author

nreese commented Sep 13, 2024

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Sep 13, 2024

/ci

@nreese
Copy link
Contributor Author

nreese commented Sep 13, 2024

/ci

@nreese nreese requested review from a team as code owners September 16, 2024 01:20
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

LGTM

@botelastic botelastic bot added ci:project-deploy-observability Create an Observability project Feature:Embedding Embedding content via iFrame Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team labels Sep 16, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

Copy link
Contributor

@Heenawter Heenawter left a comment

Choose a reason for hiding this comment

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

Left a few questions + comments, but everything else LGTM 👍 Code review + tested locally (Dashboard app, security solution, example plugin).

fullScreenMode$: PublishingSubject<boolean | undefined>;
focusedPanelId$: PublishingSubject<string | undefined>;
forceRefresh: () => void;
getPanelsState: () => DashboardPanelMap;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we end up with a bunch of getters, we might a single getRuntimeState that returns the entire state (currently just explicitInput) rather than having individual getters for every piece of state. That being said, since there is currently only one getter for panels, I think this is fine 🤷

src/plugins/dashboard/public/dashboard_api/types.ts Outdated Show resolved Hide resolved
Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

ML changes LGTM

@nreese
Copy link
Contributor Author

nreese commented Sep 17, 2024

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

merge conflict between base and head

@kibana-ci
Copy link
Collaborator

kibana-ci commented Sep 17, 2024

💚 Build Succeeded

  • Buildkite Build
  • Commit: d83e0ba
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-192754-d83e0bade41f

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dashboard 561 562 +1
securitySolution 5764 5797 +33
total +34

Async chunks

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

id before after diff
apm 3.5MB 3.5MB +17.0B
dashboard 457.3KB 481.2KB +23.9KB
infra 1.6MB 1.6MB -8.0B
securitySolution 20.4MB 20.4MB +315.0B
total +24.3KB

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
dashboard 12 13 +1

Page load bundle

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

id before after diff
dashboard 42.4KB 26.9KB -15.5KB
Unknown metric groups

API count

id before after diff
dashboard 129 128 -1

async chunk count

id before after diff
dashboard 14 15 +1

History

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

@nreese nreese merged commit e2380af into elastic:main Sep 17, 2024
43 checks passed
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.x Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 192754

Questions ?

Please refer to the Backport tool documentation

@nreese
Copy link
Contributor Author

nreese commented Sep 17, 2024

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

@nreese
Copy link
Contributor Author

nreese commented Sep 17, 2024

8.x backport #193219

nreese added a commit that referenced this pull request Sep 18, 2024
# Backport

This will backport the following commits from `main` to `8.x`:
- [[dashboard] Lazy DashboardRenderer
(#192754)](#192754)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Nathan
Reese","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-17T19:15:50Z","message":"[dashboard]
Lazy DashboardRenderer (#192754)\n\nChanges\r\n1. expose
DashboardRenderer as lazy loaded component to reduce static\r\npage load
size\r\n2. Use `onApiAvailable` prop to pass DashboardApi to parent
instead of\r\n`forwardRef`\r\n3. Decouple DashboardApi from legacy
embeddable system. This changed\r\nfunctions such as `updateInput` and
using redux `select` to
access\r\nstate.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"e2380afd7b72912713c2372084463ced489bedb7","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Embedding","Team:Presentation","release_note:skip","v9.0.0","ci:project-deploy-observability","Team:obs-ux-infra_services","apm:review","project:embeddableRebuild","v8.16.0"],"number":192754,"url":"https://github.com/elastic/kibana/pull/192754","mergeCommit":{"message":"[dashboard]
Lazy DashboardRenderer (#192754)\n\nChanges\r\n1. expose
DashboardRenderer as lazy loaded component to reduce static\r\npage load
size\r\n2. Use `onApiAvailable` prop to pass DashboardApi to parent
instead of\r\n`forwardRef`\r\n3. Decouple DashboardApi from legacy
embeddable system. This changed\r\nfunctions such as `updateInput` and
using redux `select` to
access\r\nstate.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"e2380afd7b72912713c2372084463ced489bedb7"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/192754","number":192754,"mergeCommit":{"message":"[dashboard]
Lazy DashboardRenderer (#192754)\n\nChanges\r\n1. expose
DashboardRenderer as lazy loaded component to reduce static\r\npage load
size\r\n2. Use `onApiAvailable` prop to pass DashboardApi to parent
instead of\r\n`forwardRef`\r\n3. Decouple DashboardApi from legacy
embeddable system. This changed\r\nfunctions such as `updateInput` and
using redux `select` to
access\r\nstate.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"e2380afd7b72912713c2372084463ced489bedb7"}},{"branch":"8.x","label":"v8.16.0","labelRegex":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

---------

Co-authored-by: kibanamachine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:review ci:project-deploy-observability Create an Observability project Feature:Embedding Embedding content via iFrame project:embeddableRebuild release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants