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: Support sub-aggregations #68306

Merged
merged 21 commits into from
Jun 9, 2020
Merged

[ML] Transforms: Support sub-aggregations #68306

merged 21 commits into from
Jun 9, 2020

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented Jun 4, 2020 •
edited
Loading

Summary

Follow-up on #67591. Adds sub-aggregations support to the transforms wizard.
Jun-08-2020 13-53-26

Please bear in mind data-grid preview does not work properly at the moment because it can't handle keys containing . in the property name. If you change the aggregation name to the one without dots, you should see counters updated in the data-grid table accordingly. This issue will be resolved in another PR.

Checklist

@darnautov darnautov changed the title [ML] Transforms: Support sug-aggregations [ML] Transforms: Support sub-aggregations Jun 4, 2020
darnautov
darnautov commented Jun 8, 2020
View reviewed changes
...s/create_transform/components/step_define/common/filter_agg/components/filter_range_form.tsx
Comment on lines +63 to +64
// @ts-ignore
step="any"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

elastic/eui#3550 EUI related issue

@darnautov darnautov self-assigned this Jun 8, 2020
@darnautov darnautov marked this pull request as ready for review June 8, 2020 12:10
@darnautov darnautov requested a review from a team as a code owner June 8, 2020 12:10
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@elastic elastic deleted a comment from kibanamachine Jun 8, 2020
alvarezmelissa87
alvarezmelissa87 approved these changes Jun 8, 2020
View reviewed changes
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

LGTM ⚡

walterra
walterra reviewed Jun 9, 2020
View reviewed changes
Copy link
Contributor

@walterra walterra left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code LGTM! f triggered this propTypes related error:

image

Looks like we need to cast to a number for the EUI component for the number input.

@darnautov
Copy link
Contributor Author

Code LGTM! f triggered this propTypes related error:

image

Looks like we need to cast to a number for the EUI component for the number input.

@walterra https://github.com/elastic/kibana/pull/68306/files#r436599588 I have left a comment about it, in case you missed 😉 it's a EUI issue, hope it's going to be resolved soon.

walterra
walterra approved these changes Jun 9, 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.

LGTM (error screenshot I posted relates to EUI issue)

@darnautov darnautov requested a review from pheyos June 9, 2020 09:23
@peteharverson
Copy link
Contributor

With the default dot . separators for the nested sub-aggs, the column that gets added to the preview data grid is blank. If we can't fix the grid to work with the dot separators, then I think we need to replace the dot separator used for the default aggregation name to e.g. underscores. Otherwise the user is going to be very confused as to why the column they add to the table is blank.

image

@darnautov
Copy link
Contributor Author

With the default dot . separators for the nested sub-aggs, the column that gets added to the preview data grid is blank. If we can't fix the grid to work with the dot separators, then I think we need to replace the dot separator used for the default aggregation name to e.g. underscores. Otherwise the user is going to be very confused as to why the column they add to the table is blank.

image

This bug took place before this PR, so we decided to fix it in a follow-up. It's a part of the meta issue for the data grid enhancements in DFA and Transforms #66383

peteharverson
peteharverson approved these changes Jun 9, 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 and LGTM.

Noted that the handling of fields with dot separators in the data grid will be addressed in #66383.

@darnautov darnautov requested a review from pheyos June 9, 2020 12:23
pheyos
pheyos approved these changes Jun 9, 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 tests LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@darnautov darnautov merged commit 1160d08 into elastic:master Jun 9, 2020
@darnautov darnautov deleted the ML-transforms-sub-aggs branch June 9, 2020 14:13
@darnautov darnautov mentioned this pull request Jun 9, 2020
Merged
darnautov added a commit to darnautov/kibana that referenced this pull request Jun 9, 2020
@darnautov
[ML] Transforms: Support sub-aggregations (elastic#68306)
1c0000b 
* [ML] WIP UI support for the sug-aggs

* [ML] extract sug-aggs from es config

* [ML] fix for range filter

* [ML] delete sub-aggregation

* [ML] isConfigInvalid

* [ML] fix actions, support parent agg

* [ML] update getAggConfigFromEsAgg

* [ML] unit tests

* [ML] SubAggsSection

* [ML] fix snapshot, add validation

* [ML] fix label capping

* [ML] rename Filter query label

* [ML] fix updateSubAggregation validation

* [ML] fix state update warning

* [ML] increase MAX_NESTING_SUB_AGGS

* [ML] functional tests for sub-aggregations

* [ML] change the tooltip message

* [ML] update functional tests

* [ML] fix typo
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 9, 2020
@gmmorris
Merge branch 'master' into alerting/consumer-based-rbac
168ce21 
* master: (37 commits)
  [DOCS] Adds documentation for drilldowns (elastic#68122)
  [Telemetry] telemetry.sendUsageFrom: "server" by default (elastic#68071)
  [ML] Transforms: Support sub-aggregations (elastic#68306)
  Fixed pre-configured docs link points to the wrong page and functional tests configs (elastic#68606)
  [Ingest Manager] Update queries from `stream.*` to `dataset.*` (elastic#68322)
  Enable Watcher by default to fix bug in which Watcher doesn't render in the side nav (elastic#68602)
  Convert Index Templates API routes to snakecase. (elastic#68463)
  [SECURITY SOLUTION] [Detections] Add / Update e2e tests to ensure initial rule runs are successful (elastic#68441)
  [Ingest] OpenAPI spec file (elastic#68323)
  chore(NA): skip apis Endpoint plugin Endpoint policy api (elastic#68638)
  bumping makelogs version to v6.0.0 (elastic#66163)
  [SIEM] Add create template button (elastic#66613)
  Bump websocket-extensions from 0.1.3 to 0.1.4 (elastic#68414)
  [ML] Sample data modules - use event.dataset instead of index name (elastic#68538)
  [ML] Functional tests - fix job validation API test with maxModelMemoryLimit (elastic#68501)
  [ML] Functional tests - stabilize DFA job creation (elastic#68495)
  [TSVB] Allows the user to change the tooltip mode (elastic#67775)
  chore(NA): skip apis Endpoint plugin Endpoint alert API when data is in elasticsearch (elastic#68613)
  chore(NA): skip endpoint Endpoint Alert Page: when es has data and user has navigated to the page (elastic#68596)
  [SIEM][Detection Engine] Converts from joi to use io-ts and moves the types to common  (elastic#68127)
  ...
darnautov added a commit that referenced this pull request Jun 9, 2020
@darnautov
[ML] Transforms: Support sub-aggregations (#68306) (#68655)
2880008 
* [ML] WIP UI support for the sug-aggs

* [ML] extract sug-aggs from es config

* [ML] fix for range filter

* [ML] delete sub-aggregation

* [ML] isConfigInvalid

* [ML] fix actions, support parent agg

* [ML] update getAggConfigFromEsAgg

* [ML] unit tests

* [ML] SubAggsSection

* [ML] fix snapshot, add validation

* [ML] fix label capping

* [ML] rename Filter query label

* [ML] fix updateSubAggregation validation

* [ML] fix state update warning

* [ML] increase MAX_NESTING_SUB_AGGS

* [ML] functional tests for sub-aggregations

* [ML] change the tooltip message

* [ML] update functional tests

* [ML] fix typo
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

@alvarezmelissa87 alvarezmelissa87 alvarezmelissa87 approved these changes

@peteharverson peteharverson peteharverson approved these changes

@hendrikmuhs hendrikmuhs Awaiting requested review from hendrikmuhs

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.
7 participants
@darnautov @elasticmachine @peteharverson @kibanamachine @walterra @pheyos @alvarezmelissa87