-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[ML] Limit exposing shared static code through ml/public/index.ts. #77745
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
Conversation
b72faee to
99a78f5
Compare
|
Pinging @elastic/ml-ui (:ml) |
| status: AppStatus.inaccessible, | ||
| })); | ||
| if (managementApp) { | ||
| managementApp.disable(); |
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.
is this no longer needed? or was it never actually needed?
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 ran into issues with a race condition with loading related code asynchronously for the management section when the section was first registered and only later a decision was made wether to enabled/disable it. I was able to solve it by flipping the checks around and both register and enable the ML part for management only inside the check where the decision is made to enable it here: https://github.com/elastic/kibana/pull/77745/files/98223ebfe878487f69933d3c7a142a0e42df46ef#diff-c9362f58636a69dc517555a6f0c7f28aR139
peteharverson
left a comment
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.
Tested and LGTM
jgowdyelastic
left a comment
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.
LGTM
|
@elasticmachine merge upstream |
darnautov
left a comment
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.
LGTM! Great job 👍
💚 Build SucceededMetrics [docs]@kbn/optimizer bundle module count
async chunks size
distributable file count
page load bundle size
History
To update your PR or re-run it, just comment with: |
|
backport is waiting for #78667 |
…-to-timeline * 'master' of github.com:elastic/kibana: (22 commits) update apm index pattern (elastic#78732) 78024: move transform out of dataset (elastic#78216) [QA][Code Coverage] Upload the coverage static site before ingestion (elastic#78695) [Discover] Make _source field not clickable (elastic#78698) [Fleet] Rename Ingest Manager => Fleet, Fleet => Agents in the UI (elastic#78685) [APM] Review feedback from distribution + transaction metrics (elastic#78752) [Ingest pipelines] Add ability to stop pipeline simulation (elastic#78183) [CSM] Fix core vital legend background (elastic#78273) [Usage Collection] [schema] Support spreads + `canvas` definition (elastic#78481) fix lodash imports (elastic#78456) [Maps] Add layer type preview icons (elastic#78650) [APM] Use transaction metrics for distribution charts (elastic#78484) [Uptime] Ml anomaly alert edit (elastic#76909) [ML] Limit exposing shared static code through ml/public/index.ts. (elastic#77745) making expression debug info serializable (elastic#78727) fix lodahs imports in app-arch code (elastic#78582) Make Field a React.lazy export (elastic#78483) [Security Solution] Improves detections tests (elastic#77295) [TSVB] Different field format on different series is ignored (elastic#78138) RFC: Improve saved object migrations (elastic#66056) ...
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
…lastic#77745) - Clean up lodash imports. - Import types only where applicable. - Reduce page load bundle size by fetching more code asynchronously. # Conflicts: # x-pack/plugins/transform/public/app/sections/transform_management/components/transform_list/expanded_row.test.tsx
Summary
Fixes #77041
This PR is inspired by the fixes for the APM/Logs/Metrics to reduce the page load bundle size (see #71843, #67654).
The bundle size of the page load bundle gets reduced from 873.6KB to 40.2KB.
lodashimport methods, now all imports are done with justlodashinstead of individual imports. Note this PR intentionally doesn't get rid of lodash imports we might be able to migrate away from to reduce code churn for this PR.lodashrelated tree shaking was fixed recently so it's fine again to import from the mainlodashimport (see Adding comment about importing lodash library #78156).plugins/ml/public. This increased the bundle size even if it wasn't needed at runtime. To avoid that, this PR exposes the async functiongetMlSharedImports(). This allows sharing public static code without making it part of themlpage load bundle. This is now used by the transforms plugin. I kept the exportsgetSeverityColor, getSeverityType, ANOMALY_SEVERITYas is to avoid having to refactor other consuming plugins. I changed a bit how this functions get exported though so the impact on bundle size is minimal. The platform team is exploring ways to improve export linting rules so hopefully all of this is just a temporary solution and at some point we'll be able to just do plain exports again.import typeto avoid importing runtime code.<OverviewPage />and<EmbeddableSwimLaneContainer />useReact.lazy()with React'sSuspenseto load code for an inner React tree on demand. I didn't do this for all routes since the other changes in this PR satisfy the reduction of the page load bundle, but it might be something we want to explore further in a follow up.public/plugin.tsnow get loaded on demand to avoid making their code part of the page load bundle.Checklist