-
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
[Lens] improve percentile agg optimizations #145303
[Lens] improve percentile agg optimizations #145303
Conversation
…percentile-agg-deduplication
@elasticmachine merge upstream |
…percentile-agg-deduplication
…com:andrewctate/kibana into 135265/improve-percentile-agg-deduplication
Pinging @elastic/kibana-visualizations @elastic/kibana-visualizations-external (Team:Visualizations) |
@elasticmachine merge upstream |
@@ -142,7 +142,7 @@ function getExpressionForLayer( | |||
if (def.input !== 'fullReference' && def.input !== 'managedReference') { | |||
const aggId = String(index); | |||
|
|||
const wrapInFilter = Boolean(def.filterable && col.filter); | |||
const wrapInFilter = Boolean(def.filterable && col.filter?.query); |
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.
This change prevents us wrapping this agg in a filter when the query is just an empty string. No difference on the elasticsearch query, but this does make sure that if someone removes a kql filter from a dimension, that agg can be collapsed into other matching aggs instead of being passed over because it looks like it has a query.
💚 Build Succeeded
Metrics [docs]Async chunks
Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
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 locally on Safari 👍
I had a go also with a mixed formula like:
defaults(percentile(bytes, percentile=2, kql='extension.keyword : CSS'), 0) +
defaults(percentile(bytes, percentile=2, kql='extension.keyword : CSS'), 0) +
percentile(bytes, percentile=4) +
percentile(bytes, percentile=4) +
percentile(bytes, percentile=5)
And both 4th
and 2nd
percentile were aggregated into each own bucket.
Adding an extra percentile(bytes, percentile=2)
will add the 2nd
percentile metric into the non-filtered bucket correctly and group the other similar ones into the filtered bucket.
Summary
Part of #135265
Unfiltered single-percentile agg configs were already optimized.
As of this PR, filtered single-percentile aggs that use the same percentile (and other args) are now collapsed into one agg config.
Unfortunately, esaggs doesn't currently support combining filtered single-percentile aggs with different percentiles into a single agg config, so that special optimization is currently only applied on non-filtered single-percentile aggs.
Testing
Filtered percentiles with same value now deduplicated
Add a dimension with the following formula
Check the request. Should only have two aggs, one for each filter.
Unfiltered percentiles with different values still collapsed
Add a dimension with the following formula
Check the request. Agg should look like this