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

Remove legacy optimizer #73154

Merged
merged 34 commits into from
Aug 13, 2020
Merged

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Jul 23, 2020

Fixes #7322
Fixes #58369
Closes #19633
Closes #6495
Closes #27274
Closes #45409
Closes #53160

The legacy optimizer is no longer required so this PR removes it along with a handful of related components. I tried to break this out into separate PRs, but the extra cost of all that review was weighing heavy on the effort to just get this removed so I ditched that approach and moved forward with a pretty colossal deletion PR.

Mostly complete change list:

  • deprecated --optimize CLI flag, we now log and exit as soon as we start with this flag.
  • all optimize.* config options have been deprecated with log messages about removal
    • optimize.viewCaching was used by an unused view rendering layer in the legacy server, which I removed so I could remove this config value
  • Support for ui/* imports in KP scss was removed, remaining usage was converted to use ~ css-loader syntax to pull from assets in src/core/public/core_app
  • The @kbn/optimizer max workers count is now Math.max(CPU_COUNT - 1, 3)
  • legacy sass build logic was removed
  • Global themes were moved from src/legacy/ui/public/styles to src/core/public/core_app/styles
  • Customized bootstrap styles from src/legacy/ui/public were converted from less to css and stored in src/core/server/core_app/assets
  • background images which are provided by the global mixins are moved to src/core/public/core_app/images
  • public_path_placeholder logic which stuck around for legacy bundle support was removed, including integration with bundle routes module
  • --no-optimizer can now be passed to yarn start if people want to run the @kbn/optimizer separately (used to be --optimize.enabled, no deprecation since this is likely never used and dev only)
  • uiExports which only pertained to the legacy optimizer were removed
  • the bootstrap script is moved from /bundles/app/{id}/bootstrap.js to just /bootstrap.js as it's not a bundle and it's not app specific
  • UUID syncToFile option was removed as this was only necessary to support --optimize flag
  • legacy status page was removed
  • UiBundles removed
  • bundleRoute is now just responsible for cache/etag logic, good candidate for replacement with more standard logic
  • legacy optimize mixin just sets up bundle routes
  • kbn/storybook, canvas storybook, and canvas sharable runtime now just import src/core/server/core_app/assets/legacy_light_theme.css rather than building sass and importing plugin light themes from build_assets or ui/public/styles

Stats:

This produces a modest 3MB reduction in the side of the default distributable, 2646 fewer files. Server startup time doesn't seem to be impacted at all by this change, which is generally expected since we've moved basically all the files from legacy to the Kibana Platform.

Holy change set!:

FYI, 23310 of the 24043 additions (97%) were caused by the @kbn/pm dist as well as committing the compiled versions of src/core/server/core_app/assets/legacy_light_theme.css and src/core/server/core_app/assets/legacy_dark_theme.css. That leaves 733 lines actually changed. Please don't be overwhelmed by the described changes :)

Release note:

Kibana no longer needs to optimize plugins for use in the browser when a plugin is installed. This means the --optimize flag is now deprecated and does nothing now. It will be removed in 8.0.

@spalger spalger mentioned this pull request Jul 27, 2020
@spalger spalger force-pushed the remove/legacy-optimizer branch 2 times, most recently from 21ecf55 to 7c820df Compare July 27, 2020 19:43
@spalger spalger force-pushed the remove/legacy-optimizer branch 4 times, most recently from f132f77 to 25ca57a Compare July 30, 2020 19:40
@spalger spalger mentioned this pull request Jul 31, 2020
@spalger spalger force-pushed the remove/legacy-optimizer branch 3 times, most recently from 3567e28 to b058ea3 Compare August 4, 2020 22:57
@spalger
Copy link
Contributor Author

spalger commented Aug 5, 2020

@elasticmachine merge upstream

@spalger
Copy link
Contributor Author

spalger commented Aug 5, 2020

mime was updated (includes new definitions), which is included in the canvas bundle causing the increase.

upgradeAssistant includes the package.json file, causing the increase.

@spalger spalger marked this pull request as ready for review August 5, 2020 22:56
@spalger spalger requested review from a team as code owners August 5, 2020 22:56
@spalger
Copy link
Contributor Author

spalger commented Aug 12, 2020

@elasticmachine merge upstream

Copy link
Contributor

@snide snide 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. Checked without base-path and correctly see success/error messaging. Checked that editing in various themes trigger, and that editing the css directly works.

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.

Code review only of Kibana App owned files. Code changes LGTM

@spalger
Copy link
Contributor Author

spalger commented Aug 12, 2020

@elasticmachine merge upstream

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

This PR turned out to be less scary than I expected 😄 I actually have very little to comment on, most of this was just removing.

I think we can remove the /src/legacy/server/sass/ @elastic/kibana-operations line from our CODEOWNERS

Do you agree that all of the bundle serving logic in src/optimize should be moved to src/core/server when we start removing src/legacy/server? Definitely out of scope for this PR, just wanted to gut check my assumptions here.

@@ -55,6 +55,11 @@ export async function bootstrap({
onRootShutdown('Kibana REPL mode can only be run in development mode.');
}

if (cliArgs.optimize) {
// --optimize is deprecated and does nothing now, avoid starting up and just shutdown
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we log a warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried, but the logger is in buffering mode here and I don't think we should write something to stdout or something without considering the other flags... I figured the --help and release notes are sufficient to warn people that they don't need to call this. In 8.0 we'll remove the flag and then it will stop working, I don't think we need to figure out how to make it log. #73154 (comment)

package.json Show resolved Hide resolved
@@ -118,7 +118,6 @@
},
"dependencies": {
"@babel/core": "^7.11.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we still using babel in production? I thought we pre-babel'd everything during the build?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still babel-register, but I plan to remove that soon, once the plugin-helpers take care of babel-ifying things at build time.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
graph 159 -1 160

async chunks size

id value diff baseline
graph 1.4MB -79.0B 1.4MB

page load bundle size

id value diff baseline
canvas 1.3MB +1.4KB 1.3MB
upgradeAssistant 64.6KB -624.0B 65.2KB
total +799.0B

History

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

Copy link
Contributor

@clintandrewhall clintandrewhall left a comment

Choose a reason for hiding this comment

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

I tested Canvas, Storybook (dev and build) and the Shareable Runtime (dev and build) and all is working! Great job, @spalger

@spalger spalger merged commit 4c810be into elastic:master Aug 13, 2020
@spalger spalger deleted the remove/legacy-optimizer branch August 13, 2020 16:08
spalger pushed a commit to spalger/kibana that referenced this pull request Aug 13, 2020
Co-authored-by: spalger <spalger@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
# Conflicts:
#	docs/setup/install/deb.asciidoc
#	docs/setup/install/rpm.asciidoc
#	docs/setup/install/targz.asciidoc
#	docs/setup/install/windows.asciidoc
#	package.json
#	packages/kbn-pm/dist/index.js
#	packages/kbn-test/src/functional_tests/lib/paths.js
#	src/core/server/config/deprecation/core_deprecations.ts
#	src/legacy/server/config/schema.js
#	src/optimize/base_optimizer.js
#	test/scripts/jenkins_xpack.sh
#	x-pack/legacy/plugins/xpack_main/index.js
#	yarn.lock
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 13, 2020
* upstream/master: (45 commits)
  [Metrics UI] Fix inventory footer misalignment (elastic#74707)
  Remove legacy optimizer (elastic#73154)
  Update design-specific GH code-owners (elastic#74877)
  skip test Reporting paginates content elastic#74922
  [Metrics UI] Add Jest tests for alert previews (elastic#74890)
  Fixed tooltip (elastic#74074)
  [Ingest Pipelines] Processor forms for processors A-D (elastic#72849)
  [Observability] change ingest manager link (elastic#74928)
  [Task manager] Prevents edge case where already running tasks are reschedule every polling interval (elastic#74606)
  [Security Solution] Fix the status of timelines' bulk actions (elastic#74560)
  Data plugin: Suggested enhance pattern (elastic#74505)
  Use jest.useFakeTimers instead of hard coded timeout for tooltip tests. (elastic#74642)
  [Security Solution][lists] Adds tests for exception lists and items part 2 (elastic#74815)
  [Security Solution][Resolver] fix presentation role on edgeline (elastic#74869)
  [Security Solution][Detections] Refactor ML calls for newest ML permissions (elastic#74582)
  [bin/kibana-plugin] support KP plugins instead (elastic#74604)
  Reduce number of indexed fields in index pattern saved object (elastic#74817)
  [reporting] Pass along generic parameters in high-order route handler (elastic#74892)
  Migrated last pieces of legacy fixture code (elastic#74470)
  Empty index patterns page re-design  (elastic#68819)
  ...
@epixa
Copy link
Contributor

epixa commented Aug 13, 2020

This PR is good.

spalger added a commit that referenced this pull request Aug 13, 2020
Co-authored-by: spalger <spalger@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 14, 2020
* master: (23 commits)
  Adding API test for custom link transaction example (elastic#74238)
  [Uptime] Singular alert (elastic#74659)
  Revert "attempt excluding a codeowners directory" (elastic#75023)
  [Metrics UI] Remove TSVB dependency from Metrics Explorer APIs (elastic#74804)
  Remove degraded state from ES status service (elastic#75007)
  [Reporting/Functional] unskip pagination test (elastic#74973)
  [Resolver] Stale query string values are removed when resolver's component instance ID changes. (elastic#74979)
  Add public url to Workplace Search plugin (elastic#74991)
  [Reporting] Update more Server Types for TaskManager (elastic#74915)
  [I18n] verify select icu-message options are in english (elastic#74963)
  Make data.search.aggs available on the server. (elastic#74472)
  [Security Solution][Resolver] Graph Control Tests and Update Simulator Selectors (elastic#74680)
  attempt excluding a codeowners directory
  [ML] DF Analytics: allow failed job to be stopped by force via the UI (elastic#74710)
  Add kibana-core-ui-designers team (elastic#74970)
  [Metrics UI] Fix inventory footer misalignment (elastic#74707)
  Remove legacy optimizer (elastic#73154)
  Update design-specific GH code-owners (elastic#74877)
  skip test Reporting paginates content elastic#74922
  [Metrics UI] Add Jest tests for alert previews (elastic#74890)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet