-
Notifications
You must be signed in to change notification settings - Fork 189
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
Benchmarks to compare autohisto with date histogram #40
Conversation
new date autohisto aggregation.
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.
The changes look fine to me. I just have a few comments:
Using the full NYC taxi data set on my local laptop gave a few errors that I was unable to reproduce using a subset of the dataset, adjusting the number of buckets to correspond to date_histogram to ensure we're doing an apples to apples comparison.
Does this mean that with the full data set there will be errors? I read the discussion in elastic/elasticsearch#28993 and as far as I understand it, there will be errors with the full data set. In that case I think it might make sense to combine it e.g. with a query to reduce the amount of data and avoid errors?
My understanding is also that we need to wait merging this until the Elasticsearch PR is available on master.
@pcsanwald I'm interested what the errors were with the full dataset? Were they the bucket limit errors we are discussing in elastic/elasticsearch#28993? Otherwise, could we add a second pair of operations that run a sub-aggregation under the histogram? Something like a terms agg on a low cardinality field with an avg fare under it? The auto_date_histogram runs in breadth first mode so the evaluation of the sub aggregations is delayed until after collection is finished so I am interested in what affect this has on the performance. @danielmitterdorfer you are right, this will need to be merged once elastic/elasticsearch#28993 is in master otherwise your nightly benchmarks will start failing |
@colings86 @danielmitterdorfer I was struggling to debug the error in rally (couldn't find the actual error messages), and wasn't able to load the full dataset on a vanilla ES server running locally (which I was also struggling to debug), which led me to suspect that perhaps the problem was with running a local setup. I like the idea of limiting the query further and will update the PR and re-test. |
One tip @pcsanwald: By default, Rally will just continue on errors and you will only see that something bad has happened if you look at the "error rate". However, you can enforce a more strict mode by specifying |
thanks @danielmitterdorfer, the part I was struggling with was how to get a more specific stacktrace for runs where the error rate for a query was 100%, I was looking in the logs, and couldn't find the errors. I'll use |
Pushed changes to limit by query as @danielmitterdorfer @colings86 suggested, re-ran successfully with full NYC taxi dataset, results here:
|
Changes look fine to me. One remark: Your target throughput seems a bit too high for your hardware but we can adjust this for our nightly hardware once you have merged (more background info on target throughput and latency, and also my ElasticON talk starting at around 11:30 into the video / slide 15). |
@pcsanwald is there still interest in merging this PR? |
@danielmitterdorfer - yes, especially now that the auto-interval histogram stuff is merged. let me "dust this off" and re-test, etc. |
Great! Thank you Paul. |
@danielmitterdorfer I re-ran this locally and am happy with the results for this dataset; I might also add a query over geonames in order to benchmark cases where we have sparse fields (and thus lots of 0 buckets in the aggregation result), but I will open a separate PR for that. |
@pcsanwald you're free to merge this at any time. It will automatically be run in our nightly benchmarks as soon as it is merged but we need to create a new chart for it (I'll take care of that). |
I've added charts now for our nightly and release benchmarks. Thanks for your PR @pcsanwald! |
This PR pairs with this autohisto PR and adds two aggregations for comparison, which @colings86 suggested.
To test, I ran rally locally using
esrally --track-path=/Users/paulsanwald/Code/rally-tracks/nyc_taxis
. Using the full NYC taxi data set on my local laptop gave a few errors that I was unable to reproduce using a subset of the dataset, adjusting the number of buckets to correspond to date_histogram to ensure we're doing an apples to apples comparison. Here's the results from a local run: