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

Cleanup unnecessary params for date_histogram #20710

Closed
wants to merge 3 commits into from

Conversation

timroes
Copy link
Contributor

@timroes timroes commented Jul 12, 2018

This PR tests if we can safely remove the extended_bounds and min_doc_count of the date_histogram.

The extended_bounds seems never to be set so we should safely be able to remove it, since it was never be part of the query.

Without min_doc_count (which was always 1 as far as I can see) we will get empty buckets within the available data range, which should just increase the response slightly but not change anything else.

Since both of those params won't be available on the new auto date histogram aggregation in ES (and @colings86 is thinking about removing at least min_doc_count completely also from date histogram) we need to check out if we actually still need them in Kibana.

@timroes timroes added WIP Work in progress Feature:Visualizations Generic visualization features (in case no more specific feature label is available) Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) labels Jul 12, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@timroes timroes requested a review from bhavyarm July 12, 2018 13:31
@timroes
Copy link
Contributor Author

timroes commented Jul 17, 2018

Jenkins, test this

@timroes timroes requested review from markov00 and ppisljar July 17, 2018 07:09
@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes
Copy link
Contributor Author

timroes commented Jul 17, 2018

Jenkins, test this - flaky tag cloud test

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

I think this is not enough because our current visualisation depends on the output created by the min_doc_count:1. 'm attaching two area chart examples

This the current version
screen shot 2018-07-17 at 10 41 35

This is the PR version
screen shot 2018-07-17 at 10 43 42

Or we need to change the vis lib to correct this behaviour, or it's better to wait for EUICharts for remove these query params

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@bhavyarm
Copy link
Contributor

Same as @markov00 here.

@ppisljar
Copy link
Member

hmm, have a hard time finding it, but there is an enchantment request asking for this option :)

we should make the zero filling configurable. two examples:

  • i am looking at the temperature sensor. i take a measurment every hour. few data points are missing as the sensor was offline at that time. i don't want zero values here.
  • i am looking at crime numbers. if on some day there was no crime events, then i want to zero fill them

the zero filling should in my opinion happen on kibana side, so we have full control over it and its something that shouldn't require many resources.

@markov00
Copy link
Member

Hey @ppisljar we already have a ticket for tracking this feature: #17717
This will be possibly supported with the new chart implementation

@timroes
Copy link
Contributor Author

timroes commented Sep 6, 2018

I will close this, since we cannot simply remove this parameters. That will then also be a trouble switching over to date auto histogram unless those parameters are enabled there.

@timroes timroes closed this Sep 6, 2018
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, ...) Feature:Visualizations Generic visualization features (in case no more specific feature label is available) WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants