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

[Lens] optimize percentiles fetching #131875

Merged
merged 22 commits into from
Jun 21, 2022
Merged

[Lens] optimize percentiles fetching #131875

merged 22 commits into from
Jun 21, 2022

Conversation

drewdaemon
Copy link
Contributor

@drewdaemon drewdaemon commented May 9, 2022 •
edited
Loading

Summary

Resolve #126941

This PR optimizes the multiple percentile dimensions scenario by condensing them into fewer percentile aggregations where possible.

It also adds an optional optimizeEsAggs method the the Operation interface so that more of these expression optimizations can be made in the future.

Checklist

drewdaemon added 3 commits May 9, 2022 14:09
@drewdaemon
first attempt at grouping percentage aggs
9c2a41d
@drewdaemon
include only columns with corresponding esAggs fns in idMap
7b264fe 
rename id map and restoration function for greater clarity
@drewdaemon
make restore column id algo independent of agg order
6fa190a 
(introduces infinite loop that need troubleshooting)
@drewdaemon
Copy link
Contributor Author

drewdaemon commented May 11, 2022 •
edited
Loading

Known issues

  • "Inspect" data table doesn't work
  • Multiple identical percentile dimensions cause an Elasticsearch error. Need to add a way to map one esAggs column ID to multiple Lens column IDs. May have to extend the current esAggsIdMap to enable one-to-many relationships.
  • No tests 👎

@drewdaemon
Copy link
Contributor Author

@elasticmachine merge upstream

@drewdaemon drewdaemon force-pushed the 126941/optimize-percentiles-fetching branch from bcf565b to 4ea17ed Compare June 13, 2022 16:59
@drewdaemon drewdaemon added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens v8.4.0 labels Jun 14, 2022
drewdaemon
drewdaemon commented Jun 14, 2022
View reviewed changes
x-pack/plugins/lens/common/expressions/rename_columns/rename_columns.ts Outdated
export const renameColumns: RenameColumnsExpressionFunction = {
name: 'lens_rename_columns',
export const renameColumns: MapToOriginalColumnsExpressionFunction = {
name: 'lens_map_to_original_columns',
Copy link
Contributor Author

@drewdaemon drewdaemon Jun 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What do we think about this expression function name change? I understand that lens_rename_columns was meant to be general wording, but it isn't used anywhere else and this name more clearly communicates how the function is actually used IMO.

If it seems better to others as well, I can rename the files to be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dej611 any feelings on this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

No strong opinion here.
Just noticed there's a mention of lens_rename_columns into x-pack/plugins/lens/server/migrations/saved_object_migrations.test.ts to be migrated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch on the mentions. But, I'm not sure we should change those expressions since they're in tests for previous Kibana versions. WDYT?

@drewdaemon drewdaemon marked this pull request as ready for review June 14, 2022 01:10
@drewdaemon drewdaemon requested a review from a team as a code owner June 14, 2022 01:10
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

@drewdaemon
Copy link
Contributor Author

@elasticmachine merge upstream

@drewdaemon drewdaemon requested a review from flash1293 June 14, 2022 13:03
dej611
dej611 reviewed Jun 16, 2022
View reviewed changes
Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Tested thoroughly with time shifts and filters and works great 👍

Left some minor + nitpick comments on the code.

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/percentile.tsx Outdated Show resolved Hide resolved
x-pack/plugins/lens/public/indexpattern_datasource/to_expression.ts Show resolved Hide resolved
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/percentile.tsx Show resolved Hide resolved
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/percentile.tsx Outdated Show resolved Hide resolved
x-pack/plugins/lens/public/indexpattern_datasource/to_expression.ts Outdated Show resolved Hide resolved
x-pack/plugins/lens/public/indexpattern_datasource/to_expression.ts Outdated Show resolved Hide resolved
@drewdaemon drewdaemon requested a review from dej611 June 16, 2022 14:02
@drewdaemon
Copy link
Contributor Author

@elasticmachine merge upstream

dej611
dej611 approved these changes Jun 16, 2022
View reviewed changes
Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Can approve right now, while waiting for some more feedback on the naming issue.

@mbondyra
Copy link
Contributor

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lens 1.2MB 1.2MB +2.0KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
lens 32.8KB 32.9KB +77.0B

History

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

@drewdaemon drewdaemon merged commit ebe331e into elastic:main Jun 21, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 21, 2022
@drewdaemon drewdaemon mentioned this pull request Jun 27, 2022
Open
@stratoula stratoula mentioned this pull request Aug 4, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dej611 dej611 dej611 approved these changes

@flash1293 flash1293 Awaiting requested review from flash1293

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting Feature:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.4.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Lens] Optimize percentiles fetching
7 participants
@drewdaemon @elasticmachine @mbondyra @kibana-ci @dej611 @flash1293 @kibanamachine