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

Change ContextContainer to lazily initialize providers #129896

Merged
merged 102 commits into from
Apr 22, 2022

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Apr 11, 2022

(obligatory comment: this is not a failed rebase)

Summary

Fix #84763
Enhancement of #78957

For performances purposes, we adapted core's request handler context to lazily instantiate its properties on access (#78957). This PR goes a step further, by ensuring that all registered context providers will only be invoked/instantiated when effectively used by the request handler.

This was done by changing the ContextContainer implementation to lazily create the context parts when they are individually accessed instead of eagerly instantiating them all before handler execution.

Due to the allowed asynchronous nature of some context providers, this implies that access to context parts within a handler has now to be done asynchronously.

E.g

const handler = async (ctx, req, res) {
   const esClient = ctx.core.elasticsearch.client;
   // do something async with the client
   return res.ok()
}

Now becomes

const handler = (ctx, req, res) {
   const esClient = (await ctx.core).elasticsearch.client;
   // do something async with the client
   return res.ok()
}

Or

const handler = async (ctx, req, res) {
   const resolved = await ctx.resolve(['core']);
   const esClient = resolved.core.elasticsearch.client;
   // do something async with the client
   return res.ok()
}

This PR:

  • implements the changes withint the ContextContainer
  • update core RequestHandlerContext accordingly
  • expose a new CustomRequestHandlerContext type to perform any potential sync context provider -> async context handler type conversion
  • update all usages of request handler contexts in the code base and associated test files.

Notes to reviewers

The changes, even if impacting a lot of files, were for the most trivial, and given the overall excellent TS type checks around usages of contexts within request handlers (in addition to our FTR test coverage), the potential risks of having missed any usages is evaluated as fairly low.

@pgayvallet pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc performance release_note:skip Skip the PR/issue when compiling release notes labels Apr 12, 2022
Copy link
Member

@joeypoon joeypoon left a comment

Choose a reason for hiding this comment

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

one tiny thing for OLM otherwise 👌 ✅

import { SecuritySolutionRequestHandlerContext } from '../types';
import { parseExperimentalConfigValue } from '../../common/experimental_features';
// A TS error (TS2403) is thrown when attempting to export the mock function below from Cases
Copy link
Member

Choose a reason for hiding this comment

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

I think this comment should stay above line 26?

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

App Services changes LGTM


const builtContext = {} as HandlerContextType<RequestHandler>;
(builtContext as unknown as RequestHandlerContext).resolve = async (keys) => {
const resolved = await Promise.all(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to reject everything on the first rejected promise and/or error?

Copy link
Contributor Author

@pgayvallet pgayvallet Apr 21, 2022

Choose a reason for hiding this comment

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

Yea, I think we do. Managing partially resolved contexts seems too complex and not really necessary given there's no real risk given the underlying providers. This is why I felt a simple Promise.all was good enough.

Also, that was the behavior when the context was implicitly awaited before the route handler execution.

Copy link
Contributor

@ari-aviran ari-aviran left a comment

Choose a reason for hiding this comment

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

Approving for cloud security posture

@@ -6,6 +6,8 @@
*/
/* eslint-disable @typescript-eslint/no-explicit-any */

import * as estypes from '@elastic/elasticsearch/lib/api/typesWithBodyKey';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this import * needed if we only use the IndexRequest ?

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 usually import * for the ES client types in the whole codebase, given this has no real impact (especially for test files), but I can use named imports instead if you prefer.

Copy link
Contributor

@dasansol92 dasansol92 left a comment

Choose a reason for hiding this comment

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

OLM changes LGTM!

x-pack/plugins/security_solution/server/types.ts Outdated Show resolved Hide resolved
@pgayvallet
Copy link
Contributor Author

pgayvallet commented Apr 21, 2022

Some benchmarking:

Kibana instances: GCP n2d-standard-8 (created via https://github.com/elastic/kibana-remote-dev)
Elasticsearch: Cloud's default ES cluster configuration, same region as the GCP instances.
Load test machine: local (not ideal but that's a start until I find out how to reach the remote-dev instance from another VM created manually)
tool: kibana-load-testing - DemoJourney

Given the testing environment wasn't perfect, the results are to be taken with a grain of salt, but each scenario was ran 3 times with fairly consistent results.

Overall summary

Average response times

Response time distribution reference - 200 users PR - 200 users reference - 300 users PR - 300 users
t < 800 ms 41% 58% 27% 33%
800 ms < t < 1200 ms 21% 21% 11% 24%
t > 1200 ms 38% 21% 62% 43%

Observation: not the best indicator, but the PR's results are plain better.

Overall statistics

Scenario Min 50th pct 75th pct 95th pct 99th pct Mean
Reference - 200 users 230 965 1556 3079 4684 1215
PR - 200 users 230 692 1093 2138 3617 903
Reference - 300 users 2 1593 2445 5206 8855 2001
PR - 300 users 3 1082 1671 3408 8729 1449
  • Min response time not affected
  • Mean response time cut by 25%
  • 25th, 50th, 75th, 95th percentile cut by 25% to 33%
  • 99th percentile less impacted

Overall this looks very positive.

Detailed results

Reference - 200 users

Screenshot 2022-04-21 at 17 02 10

PR - 200 users

Screenshot 2022-04-21 at 17 03 01

Reference - 300 users

Screenshot 2022-04-21 at 17 02 33

PR - 300 users

Screenshot 2022-04-21 at 17 02 48

Remarks: the failures on the 300 users scenarios are caused by reaching limits on ssh-forwarding and are not real Kibana failures

@TinaHeiligers TinaHeiligers self-requested a review April 21, 2022 15:13
Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

LGTM

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Security Solution Tests / Timeline search and filters Update kqlMode for timeline "before all" hook for "should be able to update timeline kqlMode with filter"

Metrics [docs]

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
core 970 977 +7
data 2803 2802 -1
screenshotMode 14 13 -1
securitySolution 45 46 +1
total +6

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
kibana 334 335 +1
observability 31 30 -1
total -0
Unknown metric groups

API count

id before after diff
@kbn/utility-types 27 28 +1
core 2515 2524 +9
data 3415 3414 -1
screenshotMode 33 32 -1
securitySolution 45 46 +1
total +9

History

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

@pgayvallet pgayvallet merged commit a02c00b into elastic:main Apr 22, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 22, 2022
kertal pushed a commit to kertal/kibana that referenced this pull request May 24, 2022
* Change ContextContainer to lazily initialize providers

* Introduce CustomRequestHandlerContext, start adapting usages

* adapt IContextProvider's return type

* start fixing violations

* fixing violations - 2

* adapt home routes

* fix remaining core violation

* fix violations on core tests

* fixing more violations

* fixing more violations

* update generated doc...

* fix more violations

* adapt remaining RequestHandlerContext

* fix more violations

* fix non-async method

* more fixes

* fix another await in non async method

* add yet another missing async

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

* add yet yet another missing async

* update fleet's endpoints

* fix telemetry endpoints

* fix event_log endpoints

* fix some security unit tests

* adapt canvas routes

* adapt alerting routes

* adapt more so_tagging routes

* fix data_enhanced routes

* fix license_management routes

* fix file_upload routes

* fix index_management routes

* fix lists routes

* fix snapshot_restore routes

* fix rule_registry routes

* fix ingest_pipelines routes

* fix remote_clusters routes

* fix index_lifecycle_management routes

* improve and fix the lazy implementation

* fix triggers_actions_ui endpoints

* start fixing unit tests

* fix cases routes

* fix transform routes

* fix upgrade_assistant routes

* fix uptime route wrapper

* fix uptime route wrapper bis

* update osquery routes

* update cross_cluster_replication routes

* fix some ML routes / wrappers

* adapt maps routes

* adapt rollup routes

* fix some canvas unit tests

* fix more canvas unit tests

* fix observability wrapper

* fix (?) infra type hell

* start fixing monitoring

* fix a few test plugins

* woups

* fix yet more violations

* fixing UA  tests

* fix logstash handlers

* fix fleet unit tests

* lint?

* one more batch

* update security_solution endpoints

* start fixing security_solution mocks

* start fixing security_solution tests

* fix more security_solution tests

* fix more security_solution tests

* just one more

* fix last (?) security_solution tests

* fix timelion javascript file

* fix more test plugins

* fix transforms context type

* fix ml context type

* fix context tests

* fix securitySolution withEndpointAuthz tests

* fix features unit tests

* fix actions unit tests

* fix imports

* fix duplicate import

* fix some merge problems

* fix new usage

* fix new test

* introduces context.resolve

* down the rabbit hole again

* start fixing test type failures

* more test type failures fixes

* move import comment back to correct place

* more test type failures fixes, bis

* use context.resolve for security solution rules routes

* fix new violations due to master merge

* remove comment

Co-authored-by: kibanamachine <[email protected]>
@tylersmalley tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting ci:cloud-deploy Create or update a Cloud deployment performance release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Fleet Team label for Observability Data Collection Fleet team v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enforce lazy creation in RouteHandlerContext