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

[HTTP/OAS] Prepare @kbn/config-schema for generating OAS #180184

Merged

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Apr 5, 2024

Summary

Introduces a set of meta fields that will be used to track metadata lost in how we currently use joi inside of @kbn/config-schema.

Notes

@jloleysens
Copy link
Contributor Author

/ci

@jloleysens jloleysens added Feature:http Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes v8.14.0 labels Apr 5, 2024
@jloleysens
Copy link
Contributor Author

/ci

}
})
)
.meta({ [META_FIELD_X_OAS_MIN_LENGTH]: options.minLength });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to our custom handling of string length validation, these values are currently lost. In order to avoid changing behaviour we use the meta function (docs) to attach a new piece of metadata containing string min/max length will propagated to through Joi.schema.describe (docs)

* These fields are used in Joi meta to capture additional information used
* by OpenAPI spec generator.
*/
export const META_FIELD_X_OAS_OPTIONAL = 'x-oas-optional' as const;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the x- prefix is expected by our partner lib https://www.npmjs.com/package/joi-to-json (to be introduced in a follow up PR)

@jloleysens jloleysens marked this pull request as ready for review April 8, 2024 13:25
@jloleysens jloleysens requested review from a team as code owners April 8, 2024 13:25
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

Hard to tell given the PR does not utilize the added meta, but LGTM.

Given how joi schema definitions are in the hot path at startup (due to all routes schemas being defined around that time), we may want to make sure that the addition of those meta fields aren't impacting the schema definition time significantly (I assume it doesn't, but better safe than sorry)

@jloleysens
Copy link
Contributor Author

jloleysens commented Apr 9, 2024

@pgayvallet I tested for performance regression using console.profile and captured the following CPU profiles boot -> after setup. Here are the ones I think are most representative:

Total on main 11280ms
main-CPU-20240409T130705.cpuprofile

Total with meta changes 11203ms
meta-20240409T125652.cpuprofile

I saw variation of +1.5sec on both across multiple runs so I'm not sure how reliable this is. I noticed a fair amount of variance in handleDiscoveredPlugins actually.

Overall this branch did not seem to introduce a clear performance regression. Though I do expect we losegain a few ms overall, just tricky to isolate overall impact to something other than "negligible". Let me know what you think!

How I tested:

diff --git a/packages/core/root/core-root-server-internal/src/bootstrap.ts b/packages/core/root/core-root-server-internal/src/bootstrap.ts
index aea90c81f47..a23bfd1373f 100644
--- a/packages/core/root/core-root-server-internal/src/bootstrap.ts
+++ b/packages/core/root/core-root-server-internal/src/bootstrap.ts
@@ -6,6 +6,8 @@
  * Side Public License, v 1.
  */
 
+console.profile('setup 2');
+
 import chalk from 'chalk';
 import { getPackages } from '@kbn/repo-packages';
 import { CliArgs, Env, RawConfigService } from '@kbn/config';
@@ -117,6 +119,7 @@ export async function bootstrap({ configs, cliArgs, applyConfigOverrides }: Boot
     }
 
     await root.setup();
+    console.profileEnd('setup 2');
     await root.start();
 
     // Notify parent process if we haven't done that yet during preboot stage.

Then started Kibana using node --inspect scripts/kibana --dev --no-base-path and using Chrome's debugger inspected and exported the profiles.

@jloleysens jloleysens enabled auto-merge (squash) April 9, 2024 13:01
Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

Response Ops changes LGTM

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
enterpriseSearch 2264 2265 +1
serverlessSearch 316 317 +1
total +2

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
@kbn/config-schema 137 138 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
enterpriseSearch 2.7MB 2.7MB +231.0B
serverlessSearch 448.8KB 449.1KB +231.0B
total +462.0B
Unknown metric groups

API count

id before after diff
@kbn/config-schema 140 141 +1

History

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

@jloleysens jloleysens merged commit 5336a23 into elastic:main Apr 9, 2024
33 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 9, 2024
@jloleysens jloleysens deleted the http/prepare-config-schema-for-oas branch April 10, 2024 07:54
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 Feature:http release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants