-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Logs + Metrics UI] Clean up async plugin initialization #67654
[Logs + Metrics UI] Clean up async plugin initialization #67654
Conversation
dad7aae
to
f4325d6
Compare
f4325d6
to
8d2862a
Compare
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I downloaded the branch and everything works. I'm taking a look at the code now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! I left some comments/questions but maybe you tried those already. If that's the case it is good to go.
x-pack/plugins/infra/public/components/alerting/inventory/validation.tsx
Show resolved
Hide resolved
x-pack/plugins/infra/public/components/alerting/inventory/expression_lazy.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/infra/public/components/alerting/logs/expression_editor/editor_lazy.ts
Outdated
Show resolved
Hide resolved
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
This refactors the browser-side plugin bootstrap code such that the eagerly loaded bundle `infra.plugin.js` is minimal and the rest of the logs and metrics app bundles are loaded only when the apps are visited.
* master: (22 commits) Partial revert of "Sync Kerberos + Anonymous access tests with the latest `security/_authenticate` API (user roles now include roles of anonymous user)." (elastic#68624) adapt some snapshot test (elastic#68489) [APM] Service maps - Fix missing ML status for services with jobs but no anomalies (elastic#68486) [skip test] apis Kerberos security Kerberos authentication finishing SPNEGO should properly set cookie and authenticate user [SIEM][Exceptions] - ExceptionsViewer UI component part 2 (elastic#68294) Surface data streams in Index Management. (elastic#67806) Fix edit datasource not working following changes in elastic#67234 (elastic#68583) [Logs + Metrics UI] Clean up async plugin initialization (elastic#67654) APM Storybook fixes (elastic#68671) Upgrade EUI to v24.1.0 (elastic#68141) [ML] DF Analytics: Creation wizard part 2 (elastic#68462) [Uptime] Fix race on overview page query (elastic#67843) Prefer using npm_execpath when spawning Yarn (elastic#68673) [Security] [Cases] Attach timeline to existing case (elastic#68580) Use Search API in Vega (elastic#68257) [Component templates] Table view (elastic#68031) [Uptime] Added relative date info in cert status column (elastic#67612) [Endpoint] Re-enable Functional test case for Endpoint related pages (elastic#68445) run page_load_metrics tests in visual regresssion jobs (elastic#68570) Enable exhaustive-deps; correct any lint warnings (elastic#68453) ...
Summary
This refactors the browser-side plugin bootstrap code such that the eagerly loaded bundle
infra.plugin.js
is minimal and the rest of the logs and metrics app bundles are loaded only when the apps are visited.closes #62413
closes #66190
Changes
Reaching the goal stated above required a refactoring of the code paths to enable code splitting and asynchronous imports. More specifically, the following changes were made:
There now are singular entry points for the render trees of the metrics and logs apps in
public/apps/metrics_app.tsx
andpublic/apps/logs_app.tsx
respectively. They each mount a set of kibana-core- and infra-specific providers fromcommon_providers.tsx
. The app registrations inpublic/plugin.ts
now feature a single asyncimport()
of the respectiverenderApp
function.This alone didn't have the full intended effect, though, because the import chain of the alert definitions registered during the
setup()
phase pulled in large parts of the UI. Fortunately the the alerting team had the foresight to support lazily loaded React components for the alert condition editing UI. Introducing default exports (expression_lazy.ts
andeditor_lazy.ts
) for the alert condition components and importing them wrapped inReact.lazy
components, though, caused the bundler to split out a large chunk ofinfra
andcore
code for lazy loading.At that point the largest part of the entry point bundle consisted of lodash. Replacing the single used
isNumber
function with a naive local implementation caused the bundle size to drop further once more.Size Comparison
master
Before (in
master
)entry point
all infra bundles
After (in PR)
entry point
all infra bundles
Testing
From the user's perspective everything should work as before. The
infra.plugin.js
bundle loaded eagerly should be significantly smaller.IMHO there are some aspects that should be verified particularly thoroughly:
Task breakdown
Future work
The platform rule of only allowing imports from the index files combined with the lack of tree shaking means that plugins statically import an unnecessarily large portion of other plugins even when they just access a single type definition. There is not much that can be done right now, but the introduction of type imports in TypeScript 3.8 will allow for most of these to disappear. There are platform efforts that work towards that: