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

[ML] @kbn/ml-agg-utils, @kbn/ml-is-populated-object, @kbn/ml-string-hash packages. #132963

Merged
merged 12 commits into from
Jun 23, 2022

Conversation

walterra
Copy link
Contributor

@walterra walterra commented May 25, 2022

Summary

Part of #136265.

Moves some ML utility code to packages.

  • @kbn/ml-agg-utils contains multiple utilities used in combination related to building aggregations.
  • @kbn/ml-is-populated-object contains the isPopulatedObject() utility function used across several plugins.
  • @kbn/ml-string-hash contains the stringHash() utility function used across several plugins.

This is in preparation for this code also to be consumed by the aiops plugin.

Checklist

@walterra walterra self-assigned this May 25, 2022
@walterra walterra force-pushed the ml-package-data-utils branch from 80abf19 to 3ed02f4 Compare May 25, 2022 18:59
@walterra walterra force-pushed the ml-package-data-utils branch from 3ed02f4 to bafadef Compare June 21, 2022 09:20
@walterra walterra force-pushed the ml-package-data-utils branch from 7ef9146 to 253de5f Compare June 21, 2022 11:27
@walterra walterra changed the title [ML] Create @kbn/ml-data-utils package. [ML] @kbn/ml-data-utils and @kbn/ml-is-populated-object packages. Jun 21, 2022
@walterra walterra marked this pull request as ready for review June 21, 2022 15:54
@walterra walterra requested review from a team as code owners June 21, 2022 15:54
@walterra walterra requested a review from peteharverson June 21, 2022 15:54
@walterra walterra added release_note:skip Skip the PR/issue when compiling release notes v8.4.0 :ml labels Jun 21, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

👍 Nice!


let aggInterval = 1;

if (delta > MAX_CHART_COLUMNS || delta <= 1) {
Copy link
Member

Choose a reason for hiding this comment

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

For future follow ups: Since we are making this utility generic now, I think we should be able to pass in an optional maxChartColumns arg with MAX_CHART_COLUMNS as fallback so it's not so opinionated.

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

file_upload changes LGTM
code review

@walterra walterra changed the title [ML] @kbn/ml-data-utils and @kbn/ml-is-populated-object packages. [ML] @kbn/ml-agg-utils, @kbn/ml-is-populated-object, @kbn/ml-string-hash packages. Jun 22, 2022
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #22 / Kibana Tags Page Accessibility tag assignment panel meets a11y requirements

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dataVisualizer 351 363 +12
fileUpload 177 178 +1
ml 1615 1619 +4
transform 303 307 +4
total +21

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
ml 81 78 -3

Async chunks

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

id before after diff
dataVisualizer 537.1KB 543.5KB +6.4KB
fileUpload 813.2KB 813.5KB +357.0B
ml 3.3MB 3.3MB +1.1KB
transform 371.8KB 373.2KB +1.4KB
total +9.2KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
ml 40.3KB 40.3KB +28.0B
Unknown metric groups

API count

id before after diff
ml 254 251 -3

ESLint disabled line counts

id before after diff
dataVisualizer 38 36 -2
ml 105 103 -2
transform 26 24 -2
total -6

Total ESLint disabled count

id before after diff
dataVisualizer 38 36 -2
ml 108 106 -2
transform 29 27 -2
total -6

History

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

cc @walterra

@walterra
Copy link
Contributor Author

@qn895 This is ready for another look. I moved stringHash to its own package too and renamed data-utils to agg-utils to make it more specific. This further improved bundle sizes.

@qn895
Copy link
Member

qn895 commented Jun 22, 2022

Latest code changes LGTM 🎉

@walterra walterra merged commit adbd6a5 into elastic:main Jun 23, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 23, 2022
@walterra walterra deleted the ml-package-data-utils branch June 23, 2022 08:38
@walterra walterra mentioned this pull request Jun 23, 2022
2 tasks
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 :ml release_note:skip Skip the PR/issue when compiling release notes v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants