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

[data.search.aggs] Remove date histogram dependency on timefilter #69858

Merged
merged 4 commits into from
Jun 29, 2020

Conversation

lukeelmers
Copy link
Member

Part of #65793

Currently aggs (and therefore the search service) has a dependency on the query service which is exclusively due to date_histogram's usage of timefilter.calculateBounds.

timefilter.calculateBounds cannot be moved to the server in its current state because of its use of the optional forceNow parameter, which will look for a forceNow query string parameter on window.location. The forceNow override is used mainly in functional tests, and also a few places in reporting, so it isn't something that we can get rid of entirely without some more extensive work.

In order to make aggs available on the server, we need to ensure date_histogram is no longer reliant on timefilter in its current state.

To that end, this PR makes the following changes:

  • Moves the calculateBounds and getTime utilities out of public/query/timefilter and into common/query/timefilter
  • Moves the logic used by timefilter.getForceNow into a standalone util in public/query/timefilter/lib
  • Removes the query dependency from search service & aggs, and replaces it with a calculateBounds dependency which can be passed in to the date histogram agg type
  • The calculateBounds function is defined in public/search/search_service, where it uses the getForceNow util from timefilter to parse the querystring from window.location
  • When aggs moves to the server, we will define calculateBounds there without the forceNow option, thus bypassing the need for window

Testing notes
This is a refactor and should introduce no functional changes. Areas to check for possible regressions: date histogram visualizations -- particularly ones where the interval is set to auto -- and reports generated based on such visualizations.

@lukeelmers lukeelmers added review Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels Jun 24, 2020
@lukeelmers lukeelmers requested a review from ppisljar June 24, 2020 22:07
@lukeelmers lukeelmers self-assigned this Jun 24, 2020
@lukeelmers lukeelmers marked this pull request as ready for review June 24, 2020 22:12
@lukeelmers lukeelmers requested a review from a team as a code owner June 24, 2020 22:12
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM

@lukeelmers
Copy link
Member Author

@elasticmachine merge upstream

@lukeelmers lukeelmers force-pushed the fix/aggs-remove-timefilter branch from dfa0b82 to 858a9e2 Compare June 29, 2020 16:57
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
data 200 -9 209

History

  • 💔 Build #57149 failed 2c5d8691b8ceaec0e880256be9388a95a0d5fff9
  • 💚 Build #56107 succeeded 0f0d7e440a885f746264c25573a9c6e92d0b12d2

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) release_note:skip Skip the PR/issue when compiling release notes review v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants