Skip to content

[ML] Adding index pattern runtime fields to anomaly detection wizards#91168

Merged
jgowdyelastic merged 11 commits intoelastic:masterfrom
jgowdyelastic:runtime-fields-from-indexpattern-in-anomaly-detection-wizards
Feb 16, 2021
Merged

[ML] Adding index pattern runtime fields to anomaly detection wizards#91168
jgowdyelastic merged 11 commits intoelastic:masterfrom
jgowdyelastic:runtime-fields-from-indexpattern-in-anomaly-detection-wizards

Conversation

@jgowdyelastic
Copy link
Copy Markdown
Member

@jgowdyelastic jgowdyelastic commented Feb 11, 2021 •
edited
Loading

Runtime fields which are stored in the index pattern are now automatically added to the datafeed when creating a new anomaly detection job in the wizards.

Currently all of the automatically added fields are left in the datafeed, a follow up PR will add the ability to remove all unused fields from the runtime_mappings

Checklist

Delete any items that are not applicable to this PR.

@jgowdyelastic
Copy link
Copy Markdown
Member Author

jgowdyelastic commented Feb 11, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Feb 11, 2021

merge conflict between base and head

@jgowdyelastic
Copy link
Copy Markdown
Member Author

jgowdyelastic commented Feb 11, 2021

@elasticmachine merge upstream

@jgowdyelastic
Copy link
Copy Markdown
Member Author

jgowdyelastic commented Feb 15, 2021

@elasticmachine merge upstream

@jgowdyelastic jgowdyelastic self-assigned this Feb 15, 2021
@jgowdyelastic jgowdyelastic marked this pull request as ready for review February 15, 2021 11:39
@jgowdyelastic jgowdyelastic requested a review from a team as a code owner February 15, 2021 11:39
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Feb 15, 2021

Pinging @elastic/ml-ui (:ml)

peteharverson
peteharverson reviewed Feb 15, 2021
View reviewed changes
peteharverson
peteharverson approved these changes Feb 15, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Latest edits LGTM

walterra
walterra reviewed Feb 15, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Just noticed one of my comments related to code that has just been moved, so changing anything totally optional :)

x-pack/plugins/ml/common/util/fields_utils.ts
const isRollup = Object.keys(rollupFields).length > 0;
const mix = mixFactory(isRollup, rollupFields);

aggs.forEach((a) => {
Copy link
Copy Markdown
Contributor

@walterra walterra Feb 15, 2021

Choose a reason for hiding this comment

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

All other code in this file is creating new instances of arrays so I wonder if the code here would be more clear if it was using .map() and reassigned to a new aggs instances, what do you think? For me it was a bit hard to follow the code because it's mutating the aggregations. As an alternative maybe add a comment here that this is mutating aggs with the inner forEachs?

Copy link
Copy Markdown
Member Author

@jgowdyelastic jgowdyelastic Feb 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

i'm hesitant to change this code at all in this PR as it is not new, I have just moved it from the server side into common so it can be used on the client side.
I should have made this clear in the PR description.

.../new_job/pages/components/pick_fields_step/components/multi_metric_view/metric_selection.tsx Outdated
const jobCreator = jc as MultiMetricJobCreator;

const { fields } = newJobCapsService;
const [fields] = useState(sortFields([...newJobCapsService.fields, ...jobCreator.runtimeFields]));
Copy link
Copy Markdown
Contributor

@walterra walterra Feb 15, 2021

Choose a reason for hiding this comment

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

Does this need useState if a setFields is never needed? Maybe it doesn't need to be wrapped in useState at all or can make use of useMemo?

Copy link
Copy Markdown
Member Author

@jgowdyelastic jgowdyelastic Feb 15, 2021

Choose a reason for hiding this comment

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

updated in 3e19ad9

...bs/new_job/pages/components/pick_fields_step/components/population_view/metric_selection.tsx Outdated
const jobCreator = jc as PopulationJobCreator;

const { fields } = newJobCapsService;
const [fields] = useState(sortFields([...newJobCapsService.fields, ...jobCreator.runtimeFields]));
Copy link
Copy Markdown
Contributor

@walterra walterra Feb 15, 2021

Choose a reason for hiding this comment

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

Does this need useState if a setFields is never needed? Maybe it doesn't need to be wrapped in useState at all or can make use of useMemo?

Copy link
Copy Markdown
Member Author

@jgowdyelastic jgowdyelastic Feb 15, 2021

Choose a reason for hiding this comment

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

updated in 3e19ad9

...new_job/pages/components/pick_fields_step/components/single_metric_view/metric_selection.tsx Outdated
const jobCreator = jc as SingleMetricJobCreator;

const { fields } = newJobCapsService;
const [fields] = useState(sortFields([...newJobCapsService.fields, ...jobCreator.runtimeFields]));
Copy link
Copy Markdown
Contributor

@walterra walterra Feb 15, 2021

Choose a reason for hiding this comment

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

Does this need useState if a setFields is never needed? Maybe it doesn't need to be wrapped in useState at all or can make use of useMemo?

Copy link
Copy Markdown
Member Author

@jgowdyelastic jgowdyelastic Feb 15, 2021

Choose a reason for hiding this comment

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

updated in 3e19ad9

...plication/jobs/new_job/pages/components/pick_fields_step/components/split_field/by_field.tsx Outdated
Comment on lines +28 to +32
const [runtimeCategoryFields] = useState(filterCategoryFields(jobCreator.runtimeFields));
const [allCategoryFields] = useState([
...newJobCapsService.categoryFields,
...runtimeCategoryFields,
]);
Copy link
Copy Markdown
Contributor

@walterra walterra Feb 15, 2021

Choose a reason for hiding this comment

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

Does this need useState if a setFields is never needed? Maybe it doesn't need to be wrapped in useState at all or can make use of useMemo?

Copy link
Copy Markdown
Member Author

@jgowdyelastic jgowdyelastic Feb 15, 2021

Choose a reason for hiding this comment

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

updated in 3e19ad9

...cation/jobs/new_job/pages/components/pick_fields_step/components/split_field/split_field.tsx Outdated
Comment on lines +29 to +33
const [runtimeCategoryFields] = useState(filterCategoryFields(jobCreator.runtimeFields));
const [categoryFields] = useState([
...newJobCapsService.categoryFields,
...runtimeCategoryFields,
]);
Copy link
Copy Markdown
Contributor

@walterra walterra Feb 15, 2021

Choose a reason for hiding this comment

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

Does this need useState if a setFields is never needed? Maybe it doesn't need to be wrapped in useState at all or can make use of useMemo?

Copy link
Copy Markdown
Member Author

@jgowdyelastic jgowdyelastic Feb 15, 2021

Choose a reason for hiding this comment

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

updated in 3e19ad9

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Feb 15, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
ml 1735 1736 +1

Async chunks

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

id before after diff
ml 6.3MB 6.4MB +13.0KB

History

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

@jgowdyelastic jgowdyelastic added the auto-backport Deprecated - use backport:version if exact versions are needed label Feb 16, 2021
walterra
walterra approved these changes Feb 16, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Latest changes LGTM

@jgowdyelastic jgowdyelastic merged commit bfba507 into elastic:master Feb 16, 2021
@jgowdyelastic jgowdyelastic deleted the runtime-fields-from-indexpattern-in-anomaly-detection-wizards branch February 16, 2021 13:24
@kibanamachine kibanamachine mentioned this pull request Feb 16, 2021
Merged
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Feb 16, 2021

Backport result

{"level":"info","message":"POST https://api.github.com/graphql (status: 200)"}
{"level":"info","message":"POST https://api.github.com/graphql (status: 200)"}
{"meta":{"labels":[":ml","Feature:Anomaly Detection","auto-backport","release_note:enhancement","v7.12.0","v8.0.0"],"branchLabelMapping":{"^v8.0.0$":"master","^v7.12.0$":"7.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"},"existingTargetPullRequests":[]},"level":"info","message":"Inputs when calculating target branches:"}
{"meta":["7.x"],"level":"info","message":"Target branches inferred from labels:"}
{"meta":{"killed":false,"code":2,"signal":null,"cmd":"git remote rm kibanamachine","stdout":"","stderr":"error: No such remote: 'kibanamachine'\n"},"level":"info","message":"exec error 'git remote rm kibanamachine':"}
{"meta":{"killed":false,"code":2,"signal":null,"cmd":"git remote rm elastic","stdout":"","stderr":"error: No such remote: 'elastic'\n"},"level":"info","message":"exec error 'git remote rm elastic':"}
{"level":"info","message":"Backporting [{\"sourceBranch\":\"master\",\"targetBranchesFromLabels\":[\"7.x\"],\"sha\":\"bfba5070a55a32197677b7364e0e913db7426619\",\"formattedMessage\":\"[ML] Adding index pattern runtime fields to anomaly detection wizards (#91168)\",\"originalMessage\":\"[ML] Adding index pattern runtime fields to anomaly detection wizards (#91168)\\n\\n* [ML] Adding index pattern runtime fields to anomaly detection wizards\\r\\n\\r\\n* hook refactor\\r\\n\\r\\n* small refactor of search json\\r\\n\\r\\n* fixing mml estimation error\\r\\n\\r\\n* changes based on review\\r\\n\\r\\n* sorting fields in metric selection\\r\\n\\r\\n* using useMemo rather than useState\\r\\n\\r\\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>\",\"pullNumber\":91168,\"existingTargetPullRequests\":[]}] to 7.x"}

Backporting to 7.x:
{"level":"info","message":"Backporting via filesystem"}
{"level":"info","message":"Creating PR with title: \"[7.x] [ML] Adding index pattern runtime fields to anomaly detection wizards (#91168)\". kibanamachine:backport/7.x/pr-91168 -> 7.x"}
{"level":"info","message":"POST /repos/elastic/kibana/pulls - 201 in 1212ms"}
{"level":"info","message":"Adding assignees to #91480: jgowdyelastic"}
{"level":"info","message":"POST /repos/elastic/kibana/issues/91480/assignees - 201 in 456ms"}
{"level":"info","message":"Adding labels: backport"}
{"level":"info","message":"POST /repos/elastic/kibana/issues/91480/labels - 200 in 333ms"}
View pull request: https://github.com/elastic/kibana/pull/91480

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Feb 16, 2021
@jgowdyelastic @kibanamachine
[ML] Adding index pattern runtime fields to anomaly detection wizards (…
54d21a1 
…elastic#91168)

* [ML] Adding index pattern runtime fields to anomaly detection wizards

* hook refactor

* small refactor of search json

* fixing mml estimation error

* changes based on review

* sorting fields in metric selection

* using useMemo rather than useState

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
jgowdyelastic added a commit that referenced this pull request Feb 16, 2021
@kibanamachine @jgowdyelastic
[ML] Adding index pattern runtime fields to anomaly detection wizards (…
57fbcd1 
…#91168) (#91480)

* [ML] Adding index pattern runtime fields to anomaly detection wizards

* hook refactor

* small refactor of search json

* fixing mml estimation error

* changes based on review

* sorting fields in metric selection

* using useMemo rather than useState

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: James Gowdy <jgowdy@elastic.co>
@jgowdyelastic jgowdyelastic mentioned this pull request Feb 17, 2021
Merged
1 task
@jgowdyelastic jgowdyelastic mentioned this pull request Mar 18, 2021
Merged
2 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

@peteharverson peteharverson peteharverson approved these changes

@qn895 qn895 Awaiting requested review from qn895

Assignees

@jgowdyelastic jgowdyelastic

Labels

auto-backport Deprecated - use backport:version if exact versions are needed Feature:Anomaly Detection ML anomaly detection :ml release_note:enhancement v7.12.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jgowdyelastic @kibanamachine @elasticmachine @walterra @peteharverson