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

[ML] Transforms: Filter aggregation support #67591

Merged
merged 28 commits into from
Jun 4, 2020
Merged

[ML] Transforms: Filter aggregation support #67591

merged 28 commits into from
Jun 4, 2020

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented May 28, 2020 •
edited
Loading

Summary

Resolves #60015. Adds support for the filter aggregations:

Jun-02-2020 12-14-09

The following filter aggs are presented:

  • term
  • number range
  • exists
  • bool

All the other filter aggs use the plain code editor for now.

Please pay attention to filter aggs field support mapping, this dictionary has to be completed. I haven't found the documentation for kibana field types I can look up to resolve it.

Checklist

Delete any items that are not applicable to this PR.

@darnautov darnautov force-pushed the ML-60015-transforms-filter-agg branch from 77ab02f to 116a5e4 Compare May 31, 2020 12:25
@darnautov darnautov force-pushed the ML-60015-transforms-filter-agg branch from a47d189 to d74e5a2 Compare June 2, 2020 09:49
@elastic elastic deleted a comment from kibanamachine Jun 2, 2020
@darnautov darnautov self-assigned this Jun 2, 2020
@darnautov darnautov marked this pull request as ready for review June 2, 2020 10:19
@darnautov darnautov requested a review from a team as a code owner June 2, 2020 10:19
@darnautov darnautov requested a review from qn895 June 2, 2020 11:11
@darnautov
Copy link
Contributor Author

@qn895 Just pushed some enhancements for the term selector 66308fa to allow custom options

@darnautov
Copy link
Contributor Author

Found the following error when trying to switch to and from the JSON editor when editing a range filter agg:

range_agg

thanks for spotting it @peteharverson , just pushed the fix 57b75c4

@qn895
Copy link
Member

qn895 commented Jun 3, 2020

Tested and LGTM

@darnautov
Copy link
Contributor Author

@elasticmachine merge upstream

pheyos
pheyos approved these changes Jun 4, 2020
View reviewed changes
Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

Functional test changes LGTM, just left a data-test-subj naming suggestion, but feel free to merge without it.

x-pack/test/functional/services/transform/wizard.ts Outdated Show resolved Hide resolved
x-pack/test/functional/services/transform/wizard.ts Outdated Show resolved Hide resolved
x-pack/test/functional/services/transform/wizard.ts Outdated Show resolved Hide resolved
@walterra
Copy link
Contributor

walterra commented Jun 4, 2020

So far we followed the principle that each option available in the groupby/agg-dropdown adds a valid configuration with reasonable defaults without a requirement to edit some form. This was a deliberate choice to allow users to quickly add different aggs by just using the keyboard (search in the dropdown, hit enter, search again, hit enter ...).

Can we try to achieve something similar with filter? For example, what if we offer an option for each agg within the dropdown so we can add a valid default config right away? Something like this:

image

Another thing is that for items with additional configuration options we try to provide a summary or hint at the custom settings, for example the interval for histogram or date_histogram group-by configuration. We should aim for something similar with filter aggs, otherwise there's no way of knowing about the settings without opening the edit-popover. For example:

image

@darnautov
Copy link
Contributor Author

So far we followed the principle that each option available in the groupby/agg-dropdown adds a valid configuration with reasonable defaults without a requirement to edit some form. This was a deliberate choice to allow users to quickly add different aggs by just using the keyboard (search in the dropdown, hit enter, search again, hit enter ...).

Can we try to achieve something similar with filter? For example, what if we offer an option for each agg within the dropdown so we can add a valid default config right away? Something like this:

image

Another thing is that for items with additional configuration options we try to provide a summary or hint at the custom settings, for example the interval for histogram or date_histogram group-by configuration. We should aim for something similar with filter aggs, otherwise there's no way of knowing about the settings without opening the edit-popover. For example:

image

@walterra I believe it is not possible to achieve such behavior with filter agg. There are so many different variations and configs, so we can't predict what will be suitable for the user. Even the basic setup with term agg requires a value and we can't just add any value from the data set.

Regarding naming - @peteharverson proposed to have term value as an agg name, and I think it's quite a good idea because it clearly indicates the filed in result index. Maybe it could be something like value.count, not sure

@walterra
Copy link
Contributor

walterra commented Jun 4, 2020

I tested the terms filter and this looks good as you described. I was referring to number based filters like range though, maybe we can add the range to the item summary like the histogram interval information.

walterra
walterra approved these changes Jun 4, 2020
View reviewed changes
Copy link
Contributor

@walterra walterra 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, great work with how we can now compose forms!

@darnautov
Copy link
Contributor Author

@walterra just pushed 7cf1833 support for helper text in the aggregation list. How it looks for the filter range
image

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

peteharverson
peteharverson approved these changes Jun 4, 2020
View reviewed changes
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested latest edits and LGTM!

@darnautov darnautov merged commit 63bb072 into elastic:master Jun 4, 2020
@darnautov darnautov deleted the ML-60015-transforms-filter-agg branch June 4, 2020 12:21
@darnautov darnautov mentioned this pull request Jun 4, 2020
Merged
darnautov added a commit to darnautov/kibana that referenced this pull request Jun 4, 2020
@darnautov
[ML] Transforms: Filter aggregation support (elastic#67591)
9a818de 
* [ML] WIP filter support

* [ML] value selector

* [ML] only supported filter aggs as options

* [ML] WIP apply config

* [ML] fix form persistence

* [ML] refactor

* [ML] support clone

* [ML] validation, get es config

* [ML] support "exists", fixes for the term form, validation

* [ML] fix ts issues

* [ML] don't perform request on adding incomplete agg

* [ML] basic range number support

* [ML] filter bool agg support

* [ML] functional tests

* [ML] getAggConfigFromEsAgg tests

* [ML] fix unit tests

* [ML] agg name update on config change, add unit tests

* [ML] update snapshot

* [ML] range selector enhancements

* [ML] improve types

* [ML] update step for range selector to support float numbers

* [ML] range validation

* [ML] term selector improvements

* [ML] fix switch between advanced editor

* [ML] prefix test ids

* [ML] support helper text for aggs item
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 4, 2020
@gmmorris
Merge branch 'master' into alerting/consumer-based-rbac
541a871 
* master: (26 commits)
  [Console]remove completion for type for filter queries and aggs (elastic#68103)
  [ML] Transforms: Filter aggregation support (elastic#67591)
  [ES UI Shared] Monaco XJSON (elastic#67485)
  [Index Management] Add data streams functionality to indices tab (elastic#67940)
  [Discover] Fix renaming of saved search not displayed in breadcrumb (elastic#67577)
  [SECURITY] Rename siem plugin to security_solution (elastic#67902)
  [Uptime] Fix Telemetry Api flaky test (elastic#67358)
  [Data plugin] Add configuration property to enable / disable autocomplete (elastic#67847)
  remove scripts. prettire update has been done (elastic#68130)
  Closes elastic#68055 by detecting the local Kibana version and using that as (elastic#68198)
  [apm] docs: add deployment annotation example (elastic#67408)
  [ML] Extend population preview chart to show actual and typical value (elastic#67569)
  Refactor index management client integration tests for scalability (elastic#67917)
  Add generator function that creates multiple alerts (elastic#67713)
  chore(NA): remove config arg from os packages (elastic#67871)
  [Reporting] Move code out of Legacy (elastic#67904)
  [Metrics UI] Add overrides to Snapshot API to support alert previews (elastic#68125)
  [Security] [Cases] Manage timeline UI API (elastic#67719)
  [ENDPOINT][INGEST]Task/endpoint ingest update (elastic#67234)
  Fix code coverage for jest, upload merged reports (elastic#68149)
  ...
darnautov added a commit that referenced this pull request Jun 4, 2020
@darnautov
[ML] Transforms: Filter aggregation support (#67591) (#68239)
1b41ea0 
* [ML] WIP filter support

* [ML] value selector

* [ML] only supported filter aggs as options

* [ML] WIP apply config

* [ML] fix form persistence

* [ML] refactor

* [ML] support clone

* [ML] validation, get es config

* [ML] support "exists", fixes for the term form, validation

* [ML] fix ts issues

* [ML] don't perform request on adding incomplete agg

* [ML] basic range number support

* [ML] filter bool agg support

* [ML] functional tests

* [ML] getAggConfigFromEsAgg tests

* [ML] fix unit tests

* [ML] agg name update on config change, add unit tests

* [ML] update snapshot

* [ML] range selector enhancements

* [ML] improve types

* [ML] update step for range selector to support float numbers

* [ML] range validation

* [ML] term selector improvements

* [ML] fix switch between advanced editor

* [ML] prefix test ids

* [ML] support helper text for aggs item
@darnautov darnautov mentioned this pull request Jun 8, 2020
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@walterra walterra walterra approved these changes

@pheyos pheyos pheyos approved these changes

@peteharverson peteharverson peteharverson approved these changes

@qn895 qn895 qn895 approved these changes

Assignees

@darnautov darnautov

Labels
Feature:Transforms ML transforms :ml release_note:enhancement v7.9.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ML] Transforms: add support for filter aggregation
7 participants
@darnautov @elasticmachine @peteharverson @qn895 @walterra @kibanamachine @pheyos