[ML] Anomaly Detection wizards: adds geo job wizard by alvarezmelissa87 · Pull Request #147043 · elastic/kibana

alvarezmelissa87 · 2022-12-05T19:47:31Z

Summary

Adds geo job wizard for lat_long jobs.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

elasticmachine · 2022-12-06T18:42:43Z

Pinging @elastic/ml-ui (:ml)

alvarezmelissa87 · 2022-12-07T16:17:58Z

Tagged you, @lcawl, for copy stuff 😄 🙏

x-pack/plugins/ml/public/application/jobs/new_job/common/job_creator/geo_job_creator.ts

…reator/geo_job_creator.ts Co-authored-by: James Gowdy <jgowdy@elastic.co>

peteharverson · 2022-12-07T18:07:02Z

.../application/jobs/new_job/pages/components/pick_fields_step/components/geo_view/settings.tsx

+      </EuiFlexGroup>
+      <EuiFlexGroup gutterSize="xl">
+        <EuiFlexItem>
+          <BucketSpan setIsValid={setIsValid} />


Does the bucket span estimator currently work with lat_long jobs, and if not, is it possible to hide the 'Estimate bucket span' button from the wizard?

Thanks for taking a look, @peteharverson. Looks like we also get this for rare jobs so might be worth looking into getting it working for the newer job wizards or removing that portion from all.
Happy to look at it in a follow up 👍

Leave this for a follow-up. Could you raise an issue for it please.

Created an issue for this 👍

x-pack/plugins/ml/public/application/jobs/new_job/pages/job_type/page.tsx

peteharverson · 2022-12-07T18:11:47Z

Not related to the changes here, but noticed the formatting of the values in the tooltip have &quot in them. Can we do anything about the formatting?

Good catch, @peteharverson
Added a fix in 410b861

alvarezmelissa87 · 2022-12-07T20:16:07Z

This has been updated and is ready for another look when you get a chance. 🙏 cc @peteharverson, @jgowdyelastic

alvarezmelissa87 · 2022-12-07T20:16:17Z

@elasticmachine merge upstream

alvarezmelissa87 · 2022-12-07T21:59:44Z

@elasticmachine merge upstream

szabosteve

One suggestion, otherwise UI text LGTM.

szabosteve · 2022-12-08T09:13:00Z

...plication/jobs/new_job/pages/components/pick_fields_step/components/geo_view/description.tsx

+      description={
+        <FormattedMessage
+          id="xpack.ml.newJob.wizard.pickFieldsStep.geoField.description"
+          defaultMessage="Select a geo field. The lat_long function detects anomalies in the geographic location of the input data."


Suggested change

defaultMessage="Select a geo field. The lat_long function detects anomalies in the geographic location of the input data."

defaultMessage="Select a geo field to detect anomalies in the geographic location of the input data."

Updated in 9be34fe

jgowdyelastic · 2022-12-08T12:28:11Z

x-pack/plugins/ml/public/application/jobs/new_job/common/job_creator/geo_job_creator.ts

+    this._geoAgg = geo;
+  }
+
+  public set geoField(field: Field | null) {


looks like this setter isn't needed

Removed in 9be34fe

jgowdyelastic · 2022-12-08T12:31:20Z

x-pack/plugins/ml/public/application/jobs/new_job/common/job_creator/geo_job_creator.ts

+    return dtr;
+  }
+
+  public removeDetector(index: number) {


this function isn't needed

Removed in 9be34fe

jgowdyelastic · 2022-12-08T12:33:22Z

x-pack/plugins/ml/public/application/jobs/new_job/common/job_creator/geo_job_creator.ts

+    return this._splitField;
+  }
+
+  public addDetector(agg: Aggregation, field: Field) {


looks like this function isn't needed.

Removed in 9be34fe

jgowdyelastic · 2022-12-08T12:36:49Z

x-pack/plugins/ml/public/application/jobs/new_job/common/job_creator/geo_job_creator.ts

+      return;
+    }
+    const agg = this._geoAgg!;
+    if (this._detectors.length === 0) {


I think we can remove addDetector and simplify this logic to:

this.removeAllDetectors(); const dtr = this._createDetector(agg, field); this._addDetector(dtr, agg, field);

updated in 9be34fe

jgowdyelastic · 2022-12-08T12:43:36Z

x-pack/plugins/ml/public/application/jobs/new_job/common/map_loader/map_loader.ts

+const eq = (newArgs: any[], lastArgs: any[]) => isEqual(newArgs, lastArgs);
+
+export class MapLoader extends ChartLoader {
+  private _indexPatternId: string | undefined;


I think we should changes the base class to store the whole "indexPattern" naming it the _dataView and remove the _indexPatternTitle and _indexPatternId member variables.

Good call- updated in 9be34fe

jgowdyelastic · 2022-12-08T12:49:02Z

...tion/jobs/new_job/pages/components/pick_fields_step/components/geo_view/metric_selection.tsx

+    }
+    setIsValid(valid);
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [jobCreatorUpdated, jobCreator.geoField]);


if we're watching jobCreatorUpdated we shouldn't need to also watch jobCreator.geoField.
I know this seems counter intuitive, as really we should be watching all dependencies, but these wizards are designed around jobCreatorUpdated being the single notification that changes have happened.
So for consistency with the other components, I think we should only watch jobCreatorUpdated here.

👌 Thanks for the context - updated in 9be34fe

jgowdyelastic · 2022-12-08T12:56:30Z

...application/jobs/new_job/pages/components/pick_fields_step/components/geo_view/geo_field.tsx

@@ -0,0 +1,40 @@
+/*


For consistency with other components, I think description.tsx, geo_field_select.tsx and geo_field.tsx should be moved out one level into a folder called geo_field.
So it is the same as population_field, rare_field, split_field etc

Moved in 9be34fe

peteharverson

Tested latest changes and LGTM

alvarezmelissa87 · 2022-12-08T23:36:58Z

Apologies - this has been updated with latest recommendations and is ready for a final look 🙏 cc @jgowdyelastic, @peteharverson

lcawl · 2022-12-09T00:33:42Z

Just a drive-by comment: Per https://www.elastic.co/guide/en/machine-learning/current/ml-geo-functions.html, "
You cannot create forecasts for anomaly detection jobs that contain geographic functions. You also cannot add rules with conditions to detectors that use geographic functions." It might be nice to include that type of information (e.g. on the last page of the wizard?). Otherwise, I think users only learn about these limitations when they try and fail to accomplish those tasks.

lcawl · 2022-12-09T00:38:26Z

...lication/jobs/new_job/pages/components/pick_fields_step/components/geo_field/description.tsx

+      description={
+        <FormattedMessage
+          id="xpack.ml.newJob.wizard.pickFieldsStep.geoField.description"
+          defaultMessage="Select a geo field to detect anomalies in the geographic location of the input data."


defaultMessage="Select a geo field to detect anomalies in the geographic location of the input data."

Per https://www.elastic.co/guide/en/machine-learning/current/ml-geo-functions.html, "The field_name that you supply must be a single string that contains two comma-separated numbers of the form latitude,longitude, a geo_point field, a geo_shape field that contains point values, or a geo_centroid aggregation. The latitude and longitude must be in the range -180 to 180 and represent a point on the surface of the Earth." I presume this wizard filters the possible field values to only those that meet these requirements. If not, however, it might be helpful to include this type of information in a tip (in case a user thinks they have geo data and it isn't showing up because it doesn't meet these requirements).

The wizard does only show supported fields already - and also the wizard option only shows up if there are fields that are supported by this type of job so the user wouldn't be running into any situations in which there would be no fields in the dropdown.
🤔 We could still add a tooltip with info about the fields just for information sake, though I don't think it's necessary for this PR. Happy to add in follow up

alvarezmelissa87 · 2022-12-09T00:52:30Z

Just a drive-by comment: Per https://www.elastic.co/guide/en/machine-learning/current/ml-geo-functions.html, " You cannot create forecasts for anomaly detection jobs that contain geographic functions. You also cannot add rules with conditions to detectors that use geographic functions." It might be nice to include that type of information (e.g. on the last page of the wizard?). Otherwise, I think users only learn about these limitations when they try and fail to accomplish those tasks.

That's a good point, @lcawl 🤔 I think that's a good idea and would be good to add for the other specific wizards as well (if it's not in there already). Sounds like one for a follow up.

jgowdyelastic

LGTM

kibana-ci · 2022-12-12T12:48:22Z

💚 Build Succeeded

Buildkite Build
Commit: 12319e3

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`ml`	1577	1591	+14

Async chunks

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

id	before	after	diff
`ml`	3.4MB	3.4MB	+9.4KB

Unknown metric groups

ESLint disabled in files

id	before	after	diff
`osquery`	1	2	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`fleet`	60	66	+6
`ml`	558	566	+8
`osquery`	109	115	+6
`securitySolution`	445	451	+6
total			+28

References to deprecated APIs

id	before	after	diff
`ml`	249	244	-5

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	69	75	+6
`ml`	561	569	+8
`osquery`	110	117	+7
`securitySolution`	521	527	+6
total			+29

History

💔 Build #93797 failed 9be34fe
💔 Build #93452 failed f01ed96
💔 Build #93415 failed 5bcbc16
💚 Build #93380 succeeded e1821d3
💚 Build #93116 succeeded 0dbba5a

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

cc @alvarezmelissa87

alvarezmelissa87 added 4 commits December 5, 2022 09:23

add boiler plate code for geo job wizard

d995f0f

wip: add geo_view components

06232f9

wip: show maps on pickfield step

85720e4

show detector description above cards

5134fda

alvarezmelissa87 added :ml Feature:Anomaly Detection ML anomaly detection v8.7.0 labels Dec 5, 2022

alvarezmelissa87 self-assigned this Dec 5, 2022

add mapLoader class

f3e11a6

alvarezmelissa87 requested a review from jgowdyelastic December 5, 2022 23:53

fix localization

0dbba5a

alvarezmelissa87 marked this pull request as ready for review December 6, 2022 18:42

alvarezmelissa87 requested a review from a team as a code owner December 6, 2022 18:42

alvarezmelissa87 added the release_note:enhancement label Dec 6, 2022

alvarezmelissa87 requested a review from lcawl December 7, 2022 16:17

jgowdyelastic reviewed Dec 7, 2022

View reviewed changes

x-pack/plugins/ml/public/application/jobs/new_job/common/job_creator/geo_job_creator.ts Outdated Show resolved Hide resolved

Update x-pack/plugins/ml/public/application/jobs/new_job/common/job_c…

e1821d3

…reator/geo_job_creator.ts Co-authored-by: James Gowdy <jgowdy@elastic.co>

peteharverson reviewed Dec 7, 2022

View reviewed changes

x-pack/plugins/ml/public/application/jobs/new_job/pages/job_type/page.tsx Outdated Show resolved Hide resolved

fix tooltip.only show geo card if supported fields

410b861

Merge branch 'main' into ml-geo-job-wizard

5bcbc16

Merge branch 'main' into ml-geo-job-wizard

f01ed96

szabosteve approved these changes Dec 8, 2022

View reviewed changes

jgowdyelastic reviewed Dec 8, 2022

View reviewed changes

peteharverson approved these changes Dec 8, 2022

View reviewed changes

remove unused methods. reorg files

9be34fe

lcawl reviewed Dec 9, 2022

View reviewed changes

Merge branch 'main' into ml-geo-job-wizard

12319e3

jgowdyelastic approved these changes Dec 12, 2022

View reviewed changes

alvarezmelissa87 merged commit 95902fa into elastic:main Dec 12, 2022

kibanamachine added the backport:skip This PR does not require backporting label Dec 12, 2022

alvarezmelissa87 deleted the ml-geo-job-wizard branch December 12, 2022 21:15

peteharverson added release_note:feature Makes this part of the condensed release notes and removed release_note:enhancement labels Feb 1, 2023

	defaultMessage="Select a geo field. The lat_long function detects anomalies in the geographic location of the input data."
	defaultMessage="Select a geo field to detect anomalies in the geographic location of the input data."

Conversation

alvarezmelissa87 commented Dec 5, 2022 • edited by peteharverson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

elasticmachine commented Dec 6, 2022

Uh oh!

alvarezmelissa87 commented Dec 7, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

peteharverson commented Dec 7, 2022 • edited by alvarezmelissa87 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alvarezmelissa87 commented Dec 7, 2022

Uh oh!

alvarezmelissa87 commented Dec 7, 2022

Uh oh!

alvarezmelissa87 commented Dec 7, 2022

Uh oh!

szabosteve left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peteharverson left a comment

Choose a reason for hiding this comment

Uh oh!

alvarezmelissa87 commented Dec 8, 2022

Uh oh!

lcawl commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lcawl Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alvarezmelissa87 commented Dec 9, 2022

Uh oh!

alvarezmelissa87 commented Dec 5, 2022 •

edited by peteharverson

Loading

peteharverson commented Dec 7, 2022 •

edited by alvarezmelissa87

Loading

lcawl commented Dec 9, 2022 •

edited

Loading

lcawl Dec 9, 2022 •

edited

Loading