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

use cache busting for KP bundles #64414

Merged
merged 29 commits into from
Apr 29, 2020
Merged

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Apr 24, 2020

Summary

This PR changes the cache invalidation policy for the platform + plugins bundles in distributable version of Kibana.
Instead of etag usage which leads to a round-trip per an asset, we use cache busting pattern, with a build number included in URL. (it looks like http://localhost:5601/bundles/8467/plugin/dashboard/dashboard.plugin.js)
Kibana server always uses the build number in an asset path to have similar URLs in development and distributable version. The difference exists in cache-control headers:

  • Cache-control: max-age=31536000 for distributable version
  • etag: ... & Cache-control: must-revalidate for non-distributable version

Some numbers:

  • for http://localhost:5601/app/kibana#/home loaded locally
    Loading time for a page with assets cached via etag: 8.61 sec.
    Loading time for a page with assets cached via cache-busting: 8.08 sec. (-8%)

  • NP app http://localhost:5601/app/logs/stream loaded locally
    Loading time for a page with assets cached via etag: 5.77 sec.
    Loading time for a page with assets cached via cache-busting: 5.25 sec. (-10%)

In the case of real-world client-server communication with the growing number of migrated plugins, that difference would be more significant.

Checklist

For maintainers

Dev Docs

Kibana static assets from now on served under a release-specific URL with long-term caching headers Cache-Control: max-age=31536000.
Before:
http://localhost:5601/bundles/plugin/dashboard/dashboard.plugin.js
After:
http://localhost:5601/bundles/8467/plugin/dashboard/dashboard.plugin.js

@mshustov mshustov added chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform performance v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.8.0 labels Apr 24, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@mshustov mshustov changed the title Js cachable assets use cache busting for KP bundles Apr 24, 2020

const uiPluginIds = [...kbnServer.newPlatform.__internals.uiPlugins.public.keys()];
...['kibanaUtils', 'esUiShared', 'kibanaReact', ...uiPluginIds].map(
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 moved plugins loading logic to consolidate it in the one place.

@spalger spalger marked this pull request as ready for review April 27, 2020 19:34
@spalger spalger requested review from a team as code owners April 27, 2020 19:34
@jbudz
Copy link
Member

jbudz commented Apr 27, 2020

in case anyone has issues testing - i've seen a few cases of strange behaviour recently more at the browser level than the configuration.

in the context of firefox with default settings, there's a memory cache and a disk cache.

disk caches have a max file size of ~50mb or 1/8 of the disk cache size. the disk cache size is dynamic based on free space, not sure on the math but probably maxes around 1gb. we're at a decent fraction of this for some files, but not there.

memory cache defaults to 5mb. most of our bundles won't fit there. you'll see this in private browsing

@jbudz jbudz mentioned this pull request Apr 27, 2020
2 tasks
Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

Great piece of work here!
I just added one NIT in case we want to use (in addition) the stale-while-revalidate Cache-Control policy. The tests and reasoning is explained in the PR #64518 (superseded by the way more detailed piece of work done in here) :)

@spalger
Copy link
Contributor

spalger commented Apr 28, 2020

@elasticmachine merge upstream

Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

LGTM

I had a mini thought in between viewing the paths. There's an assumption that we always have a build hash or else routes //. And then I was wondering if there's any utility to adding overrides. It was definitely a stretch - "i want to manually rewrite all the paths bottom up to double make sure there's no vulnerabilities", "i want to turn off caching because we have our own cdn". But noting just in case it springboards some other ideas.

# Conflicts:
#	src/core/public/plugins/plugin_reader.ts
#	src/legacy/ui/ui_render/ui_render_mixin.js
# Conflicts:
#	src/optimize/bundles_route/bundles_route.ts
#	src/optimize/bundles_route/dynamic_asset_response.ts
#	src/optimize/bundles_route/proxy_bundles_route.ts
#	src/optimize/index.ts
#	src/optimize/np_ui_plugin_public_dirs.ts
@mshustov
Copy link
Contributor Author

@elasticmachine merge upstream

@spalger spalger merged commit 5082ff3 into elastic:master Apr 29, 2020
spalger added a commit to spalger/kibana that referenced this pull request Apr 29, 2020
* convert into TS

* load plugin scripts in html body

* use buildNum as a unique Id for cache busting

* add tests for caching

* fix tests

* remove the last TODO. url should be inlined with assetss server

* this logic handled by publicPathMap on the client

* cache kbn-shared-deps as well

* attempt to fix karma tests

* always run file through replace stream

* place buildHash at begining of path, include all static files

* update bundles_route tests to inject buildNum everywhere

* fix karma config to point to right prefix

* use isDist naming throughout

* explain magic number with variables

* restore replacePublicPath option from elastic#64226

* replace one more instance of replacePublicPath

* use promisify instead of bluebird + non-null assertions

* remove one more magic number

Co-authored-by: spalger <spalger@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@mshustov mshustov added release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. and removed release_note:skip Skip the PR/issue when compiling release notes labels May 12, 2020
@kibanamachine
Copy link
Contributor

💔 Build Failed

Failed CI Steps


Test Failures

Kibana Pipeline / kibana-intake-agent / Jest Integration Tests.src/core/public/application/integration_tests.AppContainer can navigate between two applications with custom appRoutes

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has failed 11 times on tracked branches: https://github.com/elastic/kibana/issues/64381


Stack Trace

Error: Caught error after test environment was torn down

Http server is not setup up yet
    at HttpServer.start (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/src/core/server/http/http_server.ts:117:13)
    at HttpService.start (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/src/core/server/http/http_service.ts:110:29)

Kibana Pipeline / kibana-intake-agent / Jest Integration Tests.src/core/public/application/integration_tests.AppContainer should not mount when partial route path has higher specificity

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has failed 11 times on tracked branches: https://github.com/elastic/kibana/issues/64382


Stack Trace

Error: expect(jest.fn()).toHaveBeenCalled()

Expected number of calls: >= 1
Received number of calls:    0
    at Object.it (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/src/core/public/application/integration_tests/router.test.tsx:174:35)

Kibana Pipeline / kibana-intake-agent / Jest Integration Tests.src/core/server/http/integration_tests.http service elasticsearch passes request authorization header to Elasticsearch if registerAuth was not set

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has failed 33 times on tracked branches: https://github.com/elastic/kibana/issues/64310


Stack Trace

Error: unknown error
    at respond (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/elasticsearch/src/lib/transport.js:351:15)
    at checkRespForFailure (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/elasticsearch/src/lib/transport.js:306:7)
    at HttpConnector.<anonymous> (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/elasticsearch/src/lib/connectors/http.js:173:7)
    at ClientRequest.wrapper (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/elasticsearch/node_modules/lodash/lodash.js:4929:19)
    at ClientRequest.emit (events.js:198:13)
    at Socket.socketErrorListener (_http_client.js:401:9)
    at Socket.emit (events.js:198:13)
    at emitErrorNT (internal/streams/destroy.js:91:8)
    at emitErrorAndCloseNT (internal/streams/destroy.js:59:3)
    at process._tickCallback (internal/process/next_tick.js:63:19)

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Feature:New Platform performance release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants