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.

Sign up for GitHub

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

Jump to bottom

[data.search.aggs]: Clean up TimeBuckets implementation #62123

Merged
merged 11 commits into from
Apr 9, 2020
Merged

[data.search.aggs]: Clean up TimeBuckets implementation #62123

merged 11 commits into from
Apr 9, 2020

Conversation

VladLasitsa
Copy link
Contributor

@VladLasitsa VladLasitsa commented Apr 1, 2020 •
edited
Loading

Part of #60130

Summary

  • updated config to take explicit ui setting values (using uiSettings object instead of getConfig method)
  • Removed duplicate TimeBuckets interface in the date_histogram agg type and replaced to TimeBuckets
  • Refactored TimeBuckets class (removed unused values from TimeBucketsInterval and removed one type of argument for setInterval method)
  • Added unit tests for time_buckets.ts
  • Removed cached method
  • Removed Bounds interface. Using TimeRangeBounds instead of Bounds. @lukeelmers I checked and found out that all places call setBounds using bounds as TimeRangeBounds. From my point of view, we can use TimeRangeBounds. What do you think about it?

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@VladLasitsa VladLasitsa requested a review from alexwizp April 1, 2020 10:04
@VladLasitsa VladLasitsa requested a review from lukeelmers April 1, 2020 14:11
@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp alexwizp marked this pull request as ready for review April 6, 2020 15:06
@alexwizp alexwizp requested a review from a team as a code owner April 6, 2020 15:06
@VladLasitsa VladLasitsa added Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes v7.8.0 v8.0.0 labels Apr 6, 2020
@VladLasitsa VladLasitsa self-assigned this Apr 6, 2020
@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

merge conflict between base and head

@VladLasitsa
Merge remote-tracking branch 'upstream/master' into clean_up_timebuck…
304f08d 
…ets_implementation

# Conflicts:
#	src/plugins/data/public/search/aggs/buckets/date_histogram.ts
@VladLasitsa
Fixed comment about config
7c6cf77
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

alexwizp
alexwizp approved these changes Apr 8, 2020
View reviewed changes
Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you

@alexwizp alexwizp requested a review from lukeelmers April 8, 2020 07:52
@alexwizp
Copy link
Contributor

alexwizp commented Apr 8, 2020

@lukeelmers could you please have a look?

lukeelmers
lukeelmers approved these changes Apr 9, 2020
View reviewed changes
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Code changes here LGTM -- thanks @VladLasitsa! This is a huge improvement

@lukeelmers
Copy link
Member

I checked and found out that all places call setBounds using bounds as TimeRangeBounds. From my point of view, we can use TimeRangeBounds. What do you think about it?

Yeah I think this probably makes the most sense as it prevents us from having too many almost-identical interfaces floating around

@VladLasitsa VladLasitsa merged commit 7ec6357 into elastic:master Apr 9, 2020
@VladLasitsa VladLasitsa mentioned this pull request Apr 9, 2020
Merged
VladLasitsa added a commit to VladLasitsa/kibana that referenced this pull request Apr 9, 2020
@VladLasitsa @elasticmachine
[data.search.aggs]: Clean up TimeBuckets implementation (elastic#62123)
e8da846 
* Created tests for time_buckets. Clean up code. Removed getConfig method.

* Fixed comments

* Fixed comments (2)

* Fixes

* Removed __cached__

* Fixes for comments

* some refactoring

* Fixed comment about config

Co-authored-by: Elastic Machine <[email protected]>
VladLasitsa added a commit that referenced this pull request Apr 9, 2020
@VladLasitsa @elasticmachine
[data.search.aggs]: Clean up TimeBuckets implementation (#62123) (#63097
00760c4 
)

* Created tests for time_buckets. Clean up code. Removed getConfig method.

* Fixed comments

* Fixed comments (2)

* Fixes

* Removed __cached__

* Fixes for comments

* some refactoring

* Fixed comment about config

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 9, 2020
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into es_ui_shared/…
bf57be0 
…chore/put-all-xjson-together

* 'master' of github.com:elastic/kibana:
  [EPM] Update UI copy to use `integration` (elastic#63077)
  [NP] Inline buildPointSeriesData and buildHierarchicalData dependencies (elastic#61575)
  [Maps] create NOT EXISTS filter for tooltip property with no value (elastic#62849)
  [Endpoint] Add link to Logs UI to the Host Details view (elastic#62852)
  [UI COPY] Fixes typo in max_shingle_size for search_as_you_type (elastic#63071)
  [APM] docs: add alerting examples for APM (elastic#62864)
  [EPM] Change PACKAGES_SAVED_OBJECT_TYPE id (elastic#62818)
  docs: fix rendering of bulleted list (elastic#62855)
  Exposed AddMessageVariables as separate component (elastic#63007)
  Add Data - Adding cloud reset password link to cloud instructions (elastic#62835)
  [ML] DF Analytics:  update memory estimate after adding exclude fields (elastic#62850)
  [Table Vis] Fix visualization overflow (elastic#62630)
  [Endpoint][EPM] Endpoint depending on ingest manager to initialize (elastic#62871)
  [Remote clusters] Fix flaky jest tests (elastic#58768)
  [Discover] Hide time picker when an indexpattern without timefield is selected (elastic#62134)
  Move search source parsing and serializing to data (elastic#59919)
  [ML] Functional tests - stabilize typing in mml input (elastic#63091)
  [data.search.aggs]: Clean up TimeBuckets implementation (elastic#62123)
  [ML] Functional transform tests - stabilize source selection (elastic#63087)
  add embed flag to saved object url as well (elastic#62926)

# Conflicts:
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/es_index.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexwizp alexwizp alexwizp approved these changes

@lukeelmers lukeelmers lukeelmers approved these changes

Assignees

@VladLasitsa VladLasitsa

Labels
Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes v7.8.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@VladLasitsa @elasticmachine @kibanamachine @alexwizp @lukeelmers