-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Combines ui/time_buckets into MlTimeBuckets #46227
Conversation
Pinging @elastic/ml-ui |
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good for this PR. Just a question about copying over the code: Does the original time buckets code maybe have more test coverage we could also bring over?
@walterra there wasn't any test coverage for the original time_buckets.js code, and we already have good coverage for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cc0e0d1
to
8fe3dec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested and LGTM
As the original kibana TimeBuckets
is being removed, is it worth just renaming this just to TimeBuckets
rather than MlTimeBuckets
?
💚 Build Succeeded |
8749e8a
to
3999afb
Compare
Still LGTM |
💚 Build Succeeded |
* [ML] Combines ui/time_buckets into MlTimeBuckets * [ML] Add unit tests for ml calc_auto_interval * [ML] Rename MlTimeBuckets to TimeBuckets
#178756) ## Summary Follow up to #46227. Consolidates multiple copies of `time_buckets.js` into `@kbn/ml-time-buckets`. The scope of this PR is just to consolidate the files. In follow ups we still need to: Refactor JS to TS and get rid of the code that uses this using "dependency cache" in the `ml` plugin. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
Summary
ML was making heavy use of the Kibana
TimeBuckets
object (ui/time_buckets
), in calculating the aggregation interval for time series charts. This was both through direct use, and with theMlTimeBuckets
object which inherited fromTimeBuckets
but with added functionality for setting the number of bars to target in the chart.As discussed in #44249, in preparation for the new platform Kibana
TimeBuckets
will become part of the data plugin, and won't be exposed as an API. This PR therefore copies intoml_time_buckets.js
the parts ofTimeBuckets
fromui/time_buckets
which were being inherited, and switches all usage in the ML plugin toMlTimeBuckets
.The code has been copied in unchanged, apart from dropping the cache object which was used in the Kibana TimeBuckets.
Checklist
Fixes #44249