[Discover] Optimize Discover plugin page load bundle by davismcphee · Pull Request #208298 · elastic/kibana

davismcphee · 2025-01-25T06:51:10Z

Summary

This PR optimizes the Discover page load bundle by reducing it to only code which is actually required on startup, and dynamically loading other code when it's needed, resulting in a 55% decrease in the bundle size.

Before (44.15 KB):

After (20.12 KB):

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
Flaky Test Runner was used on any tests changed
The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

elasticmachine · 2025-03-03T03:52:40Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

jbudz

limits.yml

elasticmachine · 2025-03-06T23:22:31Z

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #4 / Dataset Quality Dataset Quality Details navigation should go to discover page when the open in discover button is clicked
[job] [logs] FTR Configs #32 / discover/context_awareness extension getAdditionalCellActions data view mode should render additional cell actions for logs data source
[job] [logs] FTR Configs #119 / discover/group1 discover accessibility top nav menu buttons should return focus to the alerts button when dismissing the alerts popover

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`discover`	1186	1195	+9

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
`discover`	161	150	-11

Async chunks

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

id	before	after	diff
`discover`	919.9KB	956.3KB	+36.5KB

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
`discover`	29	27	-2

Page load bundle

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

id	before	after	diff
`discover`	44.3KB	20.3KB	-24.0KB

Unknown metric groups

API count

id	before	after	diff
`discover`	209	198	-11

async chunk count

id	before	after	diff
`discover`	31	32	+1

References to deprecated APIs

id	before	after	diff
`discover`	11	10	-1
`securitySolution`	354	355	+1
total			-0

History

💚 Build #280952 succeeded e7ea53c
💚 Build #280124 succeeded 6669a38
💛 Build #279458 was flaky 4496ad2
💔 Build #279306 failed 2e512cce560e26013960195fa3f6f2ef4a5db430
💔 Build #279300 failed 7a82e0f9c8a62a4a298fc859ad74005d2b613c0d
💚 Build #279290 succeeded fe0464f7c4b91981b9b5ab2a1ce1757d41c4d203

cc @davismcphee

jughosta · 2025-03-07T12:10:04Z

...platform/plugins/shared/discover/public/components/discover_container/discover_container.tsx

   */
  overrideServices: Partial<DiscoverServices>;
-  getDiscoverServices: () => DiscoverServices;
+  getDiscoverServices: () => Promise<DiscoverServices>;


I wonder what are benefits of making services async if the app can't work without them?

I'm not sure if you mean in the specific case of this component, or generally loading services async in the Discover plugin.

In this specific case it's because they're loaded async in the plugin but this component is exposed synchronously on the Discover plugin start contract, so we need to fetch them async when rendering the container. If it seems cleaner, we could instead wrap this in a HOC within the React.lazy call that first loads the services and then passes them synchronously to the container, although the end result will be the same loading wise.

If you mean the benefits of loading services async in the Discover plugin generally, then it's because plugin page load bundles are loaded for every enabled plugin every time Kibana is loaded, regardless of which page. So if a user opens the home page and has no intention of going to Discover in their session, the entire Discover page load bundle still has to be loaded. All plugins should limit their page load bundles to the bare minimum required to minimize that impact, and load the remainder of what's needed to support the app only when interacting with it / navigating to it.

Also the patterns this PR introduces will prevent the page load bundle from growing much whenever we add new services or new code to existing services (e.g. ProfilesManager or DiscoverEBTManager). This is another big benefit since it protects us from changes in the future.

Discover is only a small piece of this, but there's a broader push for it across Kibana plugins right now, and this makes sure we're at least doing our part.

Thanks for the explanation! Was not aware how much Discover plugin loads on other pages. Sounds like it could be something to improve in the plugins system too.

Np, in hindsight I should've included more of these details in the PR summary anyway. And agreed there's probably room for improvement in the plugin system itself, but my understanding is it's tricky due to plugin dependencies. So for the most part I've been trying to follow the guidance in the plugin performance docs.

jughosta · 2025-03-07T17:18:20Z

src/platform/plugins/shared/discover/public/plugin.tsx

    }

+    forwardLegacyUrls(plugins.urlForwarding);
+    registerDiscoverAnalytics(core, this.discoverEbtContext$);


It used to run conditionally and at the moment of initialisation. I can look into it again on Monday.

Yeah this was one of the trickiest parts to figure out, but I think it should be working the same now. We used to call ebtManager.initialize() directly in setup with the conditions hardcoded to true, so the registrations always happened whenever Kibana was loaded. I tried keeping the logic and calling initialize when the Discover app was mounted instead, but it seems core.analytics doesn't accept registrations after setup, so the events weren't working correctly and breaking the functional tests.

Looks like the conditions are redundant now. Also I think it would be better to colocate registerDiscoverAnalytics() with the initialization and call it after ebtManager.initialize().

Good point, I removed the flags here so the props are set whenever initialize is called: 85e126c.

If you mean colocating the code within the same file, I completely agree, but it seems plugins only support tree shaking at the file level. If we put the registration logic in the same file, it'll also bring the manager and all imports into the page load bundle. Not quite the same, but I renamed the registration file to discover_ebt_manager_registrations so they're at least side by side in the file tree (also renamed the function to registerDiscoverEBTManagerAnalytics). Hopefully that makes the relationship a bit clearer for others: c6a17b2.

I also can't call registerDiscoverEBTManagerAnalytics directly after initialize since the registrations need to happen synchronously in setup, and EBT manager code is loaded async, but I put the registration call directly below getEbtManager in the plugin file to also hopefully make it clearer.

jughosta · 2025-03-10T10:41:39Z

There are some merge conflicts now.

… plugin.tsx

…-fix'

jughosta

LGTM 👍
Thanks for these optimizations!

kibanamachine · 2025-03-11T20:30:43Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/13797576181

kibanamachine · 2025-03-11T20:36:10Z

💔 All backports failed

Status	Branch	Result
❌	8.x	Backport failed because of merge conflicts You might need to backport the following PRs to 8.x: - [Discover] Enable `consistent-type-imports` eslint rule (#212293)

Manual backport

To create the backport manually run:

node scripts/backport --pr 208298

Questions ?

Please refer to the Backport tool documentation

kibanamachine · 2025-03-13T20:51:03Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 208298 locally

kibanamachine · 2025-03-14T21:48:41Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 208298 locally

davismcphee · 2025-03-17T19:16:28Z

💚 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

## Summary This PR optimizes the Discover page load bundle by reducing it to only code which is actually required on startup, and dynamically loading other code when it's needed, resulting in a 55% decrease in the bundle size. Before (44.15 KB): ![before](https://github.com/user-attachments/assets/989d1626-4dd7-4710-a9bc-8d80220101eb) After (20.12 KB): ![after](https://github.com/user-attachments/assets/ff68b367-3293-47cf-9d3f-5c35d0aea27a) ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit e1bffa6) # Conflicts: # src/platform/plugins/shared/discover/public/__mocks__/services.ts # src/platform/plugins/shared/discover/public/index.ts # src/platform/plugins/shared/discover/public/plugin.tsx # src/platform/plugins/shared/discover/public/types.ts

kibanamachine · 2025-03-17T21:48:49Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

…214867) # Backport This will backport the following commits from `main` to `8.x`: - [[Discover] Optimize Discover plugin page load bundle (#208298)](#208298)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)

## Summary This PR optimizes the Discover page load bundle by reducing it to only code which is actually required on startup, and dynamically loading other code when it's needed, resulting in a 55% decrease in the bundle size. Before (44.15 KB): ![before](https://github.com/user-attachments/assets/989d1626-4dd7-4710-a9bc-8d80220101eb) After (20.12 KB): ![after](https://github.com/user-attachments/assets/ff68b367-3293-47cf-9d3f-5c35d0aea27a) ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

davismcphee added Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. t// backport:prev-minor labels Jan 25, 2025

davismcphee self-assigned this Jan 25, 2025

davismcphee force-pushed the optimize-plugin-imports branch from ed317eb to 3b9a8c8 Compare February 24, 2025 19:48

davismcphee added backport:version Backport to applied version labels v9.1.0 v8.19.0 and removed backport:prev-minor labels Feb 26, 2025

davismcphee force-pushed the optimize-plugin-imports branch from 0957f74 to 2e512cc Compare February 26, 2025 03:21

davismcphee changed the title ~~[Discover] Optimize plugin imports~~ [Discover] Optimize Discover plugin page load bundle Feb 26, 2025

davismcphee added 18 commits February 26, 2025 10:37

Optimize context awareness plugin imports

f6095c1

Fix test

a877f42

Make DiscoverEBTManager import async

b44a21e

Optimize plugin service imports

ce52049

Optimize more plugin imports and services

6cf1998

Fix types

f600ff1

Another optimization

eae8b11

Discover analytics registrations on startup

b9d6b1c

Reorganize plugin services

9354e11

Group plugin dynamic imports

c8bff97

Move search_provider

f05f46a

Don't export appLocatorGetLocationCommon

7663fd9

Split out forwardLegacyUrls

d6ad865

Split out onAddSearchEmbeddable

97bb258

Fix utils exports

2dd0cd4

Fix importing profiles manager and EBT manager

bc7d308

Remove customizations from load bundle

67dcf88

Remove embeddable code from page load

ffd6ba2

davismcphee requested review from a team as code owners March 3, 2025 03:52

jbudz approved these changes Mar 3, 2025

View reviewed changes

Merge branch 'main' into optimize-plugin-imports

e7ea53c

davismcphee mentioned this pull request Mar 6, 2025

[discover] lazy load actions #211974

Closed

Merge branch 'main' into optimize-plugin-imports

2fc40f5

jughosta reviewed Mar 7, 2025

View reviewed changes

davismcphee and others added 4 commits March 10, 2025 18:11

Merge branch 'main' into optimize-plugin-imports

b2f6dad

Remove EBT manager init flags

85e126c

Rename EBT manager registration file and function, and reorder within…

c6a17b2

… plugin.tsx

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

0a3d405

…-fix'

jughosta approved these changes Mar 11, 2025

View reviewed changes

davismcphee merged commit e1bffa6 into elastic:main Mar 11, 2025
10 checks passed

davismcphee deleted the optimize-plugin-imports branch March 11, 2025 20:30

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 13, 2025

davismcphee mentioned this pull request Mar 17, 2025

[8.x] [Discover] Optimize Discover plugin page load bundle (#208298) #214867

Merged

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 18, 2025

Conversation

davismcphee commented Jan 25, 2025 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

elasticmachine commented Mar 3, 2025

Uh oh!

jbudz left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Mar 6, 2025

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

async chunk count

References to deprecated APIs

History

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davismcphee Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jughosta commented Mar 10, 2025

Uh oh!

jughosta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibanamachine commented Mar 11, 2025

Uh oh!

kibanamachine commented Mar 11, 2025

💔 All backports failed

Manual backport

Questions ?

Uh oh!

kibanamachine commented Mar 13, 2025

Uh oh!

kibanamachine commented Mar 14, 2025

Uh oh!

davismcphee commented Mar 17, 2025

💚 All backports created successfully

Questions ?

Uh oh!

kibanamachine commented Mar 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

davismcphee commented Jan 25, 2025 •

edited by kibanamachine

Loading

davismcphee Mar 8, 2025 •

edited

Loading