[ML] Adding ability to change data view in advanced job wizard #115191

jgowdyelastic · 2021-10-15T13:36:04Z

Adds a new setting to the Advanced Job Wizard to allow the user to change the selected data view.
Problems may occur if the newly selected data view does not contain the fields are being used in the job, therefore we run a datafeed preview to validate that the current job works with the selected data view before applying it.

We would not expect the user to use this feature when creating a brand new job from scratch, as the data view is selected moments before the wizard opens. However, this feature is useful for users who need to adjust an existing job by allowing them to clone the job and then change the data view.

@szabosteve could you please review the text used in the UI.

Note, screenshots below use the term "Data view", this has now been changed to "Index pattern"

There are 4 validation results

The job does not contain any detectors and so validation is skipped.

Datafeed preview ran successfully and produced results

Datafeed preview ran successfully but no results were produced, this may be due to the index being empty.

The datafeed preview failed to run. Most probably the job contains fields which do not exist in the newly selected data view

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

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

jgowdyelastic · 2021-10-18T07:26:13Z

@elasticmachine merge upstream

…nced-job-wizard

elasticmachine · 2021-10-18T10:05:16Z

Pinging @elastic/ml-ui (:ml)

...cation/jobs/new_job/pages/components/datafeed_step/components/data_view/change_data_view.tsx

...jobs/new_job/pages/components/datafeed_step/components/data_view/change_data_view_button.tsx

peteharverson · 2021-10-18T10:41:30Z

...application/jobs/new_job/pages/components/datafeed_step/components/data_view/description.tsx

+      description={
+        <FormattedMessage
+          id="xpack.ml.newJob.wizard.datafeedStep.dataView.description"
+          defaultMessage="The currently selected Data view being used for this job. Change this text."


data view. Are you looking for a suggestion from @szabosteve for the last part of this message?

Updated in 489f0e3 to remove the change this text part, but yes, all text in this PR needs review please @szabosteve

x-pack/plugins/ml/server/routes/job_validation.ts

peteharverson · 2021-10-18T11:04:42Z

The warning text at the bottom of the JSON editor can probably be changed too, to say something like The datafeed indices can be changed by editing the data view on the first step of the wizard.

@peteharverson updated in c89ca51

Also changes all uses of "data view" to "index pattern"

peteharverson

Tested for the principal use case of editing the index pattern for a job created in the advanced wizard and looks good overall. Just left some suggestions for the text.

szabosteve

I left a couple of suggestions. Please let me know if you have any questions or requests.

szabosteve · 2021-10-18T13:08:26Z

...cation/jobs/new_job/pages/components/datafeed_step/components/data_view/change_data_view.tsx

+      >
+        <FormattedMessage
+          id="xpack.ml.newJob.wizard.datafeedStep.dataView.validation.noDetectors.message"
+          defaultMessage="No detectors have been configured, so changing to this data view should be ok."


Suggested change

defaultMessage="No detectors have been configured, so changing to this data view should be ok."

defaultMessage="No detectors have been configured; this data view can be applied to the job."

Updated in 0336d02

szabosteve · 2021-10-18T13:09:03Z

...cation/jobs/new_job/pages/components/datafeed_step/components/data_view/change_data_view.tsx

+        >
+          <FormattedMessage
+            id="xpack.ml.newJob.wizard.datafeedStep.dataView.validation.valid.message"
+            defaultMessage="This data view appears to be a good match for this job."


Suggested change

defaultMessage="This data view appears to be a good match for this job."

defaultMessage="This data view can be applied to this job."

Updated in 0336d02

szabosteve · 2021-10-18T13:24:19Z

...cation/jobs/new_job/pages/components/datafeed_step/components/data_view/change_data_view.tsx

+        >
+          <FormattedMessage
+            id="xpack.ml.newJob.wizard.datafeedStep.dataView.validation.possiblyInvalid.message"
+            defaultMessage="This data view produced no results when previewing the datafeed. This could be due to there being no documents in {dataViewTitle}."


Suggested change

defaultMessage="This data view produced no results when previewing the datafeed. This could be due to there being no documents in {dataViewTitle}."

defaultMessage="This data view produced no results when previewing the datafeed. There might be no documents in {dataViewTitle}."

Updated in 0336d02

szabosteve · 2021-10-18T13:25:17Z

...cation/jobs/new_job/pages/components/datafeed_step/components/data_view/change_data_view.tsx

+      >
+        <FormattedMessage
+          id="xpack.ml.newJob.wizard.datafeedStep.dataView.validation.invalid.message"
+          defaultMessage="This data view produced an error when attempting to preview the datafeed. This could be due to fields selected for this job not existing in {dataViewTitle}."


Suggested change

defaultMessage="This data view produced an error when attempting to preview the datafeed. This could be due to fields selected for this job not existing in {dataViewTitle}."

defaultMessage="This data view produced an error when attempting to preview the datafeed. The fields selected for this job might not exist in {dataViewTitle}."

Updated in 0336d02

szabosteve · 2021-10-18T13:40:08Z

...cation/jobs/new_job/pages/components/datafeed_step/components/data_view/change_data_view.tsx

+              <EuiCallOut color="warning">
+                <FormattedMessage
+                  id="xpack.ml.newJob.wizard.datafeedStep.dataView.step0.description"
+                  defaultMessage="Changing the current data view may cause problems with the current job configuration as it may not contain the same fields."


I think the it in the second part of the sentence refers to the data view that the user wants to switch to. If my assumption is correct, then I suggest a clarification. Let me know if I misinterpret the sentence, please.

Suggested change

defaultMessage="Changing the current data view may cause problems with the current job configuration as it may not contain the same fields."

defaultMessage="Changing the current data view may cause problems with the corresponding job configuration as the newly selected data view may not contain the same fields."

Updated in 0336d02

szabosteve · 2021-10-18T13:45:23Z

...cation/jobs/new_job/pages/components/datafeed_step/components/data_view/change_data_view.tsx

+            <>
+              <FormattedMessage
+                id="xpack.ml.newJob.wizard.datafeedStep.dataView.step1.title"
+                defaultMessage="Select new data view for job"


Suggested change

defaultMessage="Select new data view for job"

defaultMessage="Select new data view for the job"

Updated in 0336d02

szabosteve · 2021-10-18T13:47:19Z

...cation/jobs/new_job/pages/components/datafeed_step/components/data_view/change_data_view.tsx

+                  <EuiLoadingSpinner />
+                  <FormattedMessage
+                    id="xpack.ml.newJob.wizard.datafeedStep.dataView.step2.validatingText"
+                    defaultMessage=" Checking compatibility of data view with job"


Suggested change

defaultMessage=" Checking compatibility of data view with job"

defaultMessage=" Checking data view and job compatibility"

Updated in 0336d02

szabosteve · 2021-10-18T13:51:27Z

...application/jobs/new_job/pages/components/datafeed_step/components/data_view/description.tsx

+      description={
+        <FormattedMessage
+          id="xpack.ml.newJob.wizard.datafeedStep.dataView.description"
+          defaultMessage="The currently selected data view being used for this job."


Suggested change

defaultMessage="The currently selected data view being used for this job."

defaultMessage="The data view that is currently used for this job."

Updated in 0336d02

szabosteve · 2021-10-18T17:32:49Z

...c/application/jobs/new_job/pages/components/common/json_editor_flyout/json_editor_flyout.tsx

@@ -204,7 +204,7 @@ export const JsonEditorFlyout: FC<Props> = ({ isDisabled, jobEditorMode, datafee
                >
                  <FormattedMessage
                    id="xpack.ml.newJob.wizard.jsonFlyout.indicesChange.calloutText"
-                    defaultMessage="It is not possible to alter the indices being used by the datafeed here. If you wish to select a different index pattern or saved search, please start the job creation again, selecting a different index pattern."
+                    defaultMessage="It is not possible to alter the indices being used by the datafeed here. If you wish to select a different index pattern or saved search, please jump to step 1 of the wizard and select change index pattern."


Somehow I missed this part, I think it was updated while I was reviewing the PR.

Suggested change

defaultMessage="It is not possible to alter the indices being used by the datafeed here. If you wish to select a different index pattern or saved search, please jump to step 1 of the wizard and select change index pattern."

defaultMessage="You cannot alter the indices being used by the datafeed here. To select a different index pattern or saved search, go to step 1 of the wizard and select the Change index pattern option."

Another option if you want it shorter:

You cannot alter ... To select a different index pattern or saved search, go to step 1...

Even better. I updated my suggestion accordingly.

lcawl · 2021-10-19T01:22:44Z

...cation/jobs/new_job/pages/components/datafeed_step/components/data_view/change_data_view.tsx

+              <EuiCallOut color="warning">
+                <FormattedMessage
+                  id="xpack.ml.newJob.wizard.datafeedStep.dataView.step0.description"
+                  defaultMessage="Changing the current index pattern may cause problems with the corresponding job configuration as the newly selected index pattern may not contain the same fields."


Changing the current index pattern may cause problems with the corresponding job configuration as the newly selected index pattern may not contain the same fields.

Drive-by comment that it's unclear what the user is supposed to do with this message. Is it really necessary at this stage? IMO it would be more helpful to qualify only the error/warning checks' messages with something like: "If you apply this change, the mismatch between the index pattern and the job configuration might cause.... the job to fail? the analysis to be incorrect? (whatever the consequences are that we're trying to warn folks about there)".

Yes I agree, this first warning step was added last minute, but I don't think it's needed.
I've removed it in f161ef4

peteharverson

Latest changes LGTM

jgowdyelastic · 2021-10-19T09:12:07Z

@elasticmachine merge upstream

…nced-job-wizard

pheyos

Great to see the tests coming as part of this PR 🎉
Left a few comments.

x-pack/test/api_integration/apis/ml/job_validation/datafeed_preview_validation.ts

darnautov

LGTM

darnautov · 2021-10-19T10:28:27Z

...cation/jobs/new_job/pages/components/datafeed_step/components/data_view/change_data_view.tsx

+      const indices = title.split(',');
+      if (jobCreator.detectors.length) {
+        const datafeed: Datafeed = { ...jobCreator.datafeedConfig, indices };
+        const gg = await validateDatafeedPreview({


shall we rename this variable?

Suggested change

const gg = await validateDatafeedPreview({

const validationResponse = await validateDatafeedPreview({

ha!! gg and hh are my go to names for variables when halfway through writing something.

Updated in 35edeee

...jobs/new_job/pages/components/datafeed_step/components/data_view/change_data_view_button.tsx

pheyos

Functional tests LGTM

kibanamachine · 2021-10-19T14:47:47Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`ml`	1683	1687	+4

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.6MB	3.6MB	+6.4KB

History

💛 Build #162596 was flaky 35edeee
💛 Build #162561 was flaky 886e4da
💔 Build #162532 failed f161ef4
💔 Build #162226 failed 0336d02
💔 Build #162043 failed c89ca51

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

cc @jgowdyelastic

…ic#115191) * [ML] Adding ability to change data view in advanced job wizard * updating translation ids * type and text changes * code clean up * route id change * text changes * text change * changing data view to index pattern * adding api tests * text updates * removing first step * renaming temp variable * adding permission checks Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-10-19T14:54:36Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…-link-to-kibana-app * 'master' of github.com:elastic/kibana: (30 commits) Fix potential error from undefined (elastic#115562) [App Search, Crawler] Fix validation step panel padding/whitespace (elastic#115542) [Cases][Connectors] ServiceNow ITOM: MVP (elastic#114125) Change default session idle timeout to 8 hours. (elastic#115565) Upgrade EUI to v39.1.1 (elastic#114732) [App Search] Wired up organic results on Curation Suggestions view (elastic#114717) [i18n] remove i18n html extractor (elastic#115004) [Logs/Metrics UI] Add deprecated field configuration to Deprecations API (elastic#115103) [Transform] Add alerting rules management to Transform UI (elastic#115363) Update UI links to Fleet and Agent docs (elastic#115295) [ML] Adding ability to change data view in advanced job wizard (elastic#115191) Change deleteByNamespace to include legacy URL aliases (elastic#115459) [Unified Integrations] Remove and cleanup add data views (elastic#115424) [Discover] Show ignored field values (elastic#115040) [ML] Stop reading the ml.max_open_jobs node attribute (elastic#115524) [Discover] Improve doc viewer code in Discover (elastic#114759) [Security Solutions] Adds security detection rule actions as importable and exportable (elastic#115243) [Security Solution] [Platform] Migrate legacy actions whenever user interacts with the rule (elastic#115101) [Fleet] Add telemetry for integration cards (elastic#115413) 🐛 Fix single percentile case when ES is returning no buckets (elastic#115214) ... # Conflicts: # x-pack/plugins/reporting/public/management/__snapshots__/report_listing.test.tsx.snap

…) (#115585) * [ML] Adding ability to change data view in advanced job wizard * updating translation ids * type and text changes * code clean up * route id change * text changes * text change * changing data view to index pattern * adding api tests * text updates * removing first step * renaming temp variable * adding permission checks Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: James Gowdy <jgowdy@elastic.co>

jgowdyelastic added 2 commits October 15, 2021 14:35

[ML] Adding ability to change data view in advanced job wizard

de79d63

updating translation ids

20984de

kibanamachine and others added 4 commits October 18, 2021 03:26

Merge branch 'master' into adding-ability-to-change-data-view-in-adva…

0dbb6ca

…nced-job-wizard

type and text changes

22bccfd

code clean up

496c375

route id change

5626439

jgowdyelastic requested review from darnautov, peteharverson and qn895 October 18, 2021 10:03

jgowdyelastic self-assigned this Oct 18, 2021

jgowdyelastic added :ml auto-backport Deprecated - use backport:version if exact versions are needed Feature:Anomaly Detection ML anomaly detection release_note:feature Makes this part of the condensed release notes review v7.16.0 v8.0.0 labels Oct 18, 2021

jgowdyelastic requested a review from szabosteve October 18, 2021 10:05

jgowdyelastic marked this pull request as ready for review October 18, 2021 10:05

jgowdyelastic requested a review from a team as a code owner October 18, 2021 10:05

peteharverson reviewed Oct 18, 2021

View reviewed changes

x-pack/plugins/ml/server/routes/job_validation.ts Outdated Show resolved Hide resolved

peteharverson reviewed Oct 18, 2021

View reviewed changes

x-pack/plugins/ml/server/routes/job_validation.ts Show resolved Hide resolved

jgowdyelastic added 2 commits October 18, 2021 12:08

text changes

489f0e3

text change

243b4c7

peteharverson reviewed Oct 18, 2021

View reviewed changes

changing data view to index pattern

c89ca51

szabosteve reviewed Oct 18, 2021

View reviewed changes

jgowdyelastic added 2 commits October 18, 2021 17:42

adding api tests

a346964

text updates

0336d02

szabosteve reviewed Oct 18, 2021

View reviewed changes

lcawl reviewed Oct 19, 2021

View reviewed changes

removing first step

f161ef4

jgowdyelastic requested a review from pheyos October 19, 2021 08:26

peteharverson mentioned this pull request Oct 19, 2021

[ML] Increase Test Coverage 7.16 #110949

Closed

16 tasks

peteharverson approved these changes Oct 19, 2021

View reviewed changes

Merge branch 'master' into adding-ability-to-change-data-view-in-adva…

886e4da

…nced-job-wizard

pheyos reviewed Oct 19, 2021

View reviewed changes

darnautov approved these changes Oct 19, 2021

View reviewed changes

jgowdyelastic added 2 commits October 19, 2021 11:44

renaming temp variable

35edeee

adding permission checks

6187135

pheyos approved these changes Oct 19, 2021

View reviewed changes

jgowdyelastic merged commit 92e1cd2 into elastic:master Oct 19, 2021

jgowdyelastic deleted the adding-ability-to-change-data-view-in-advanced-job-wizard branch October 19, 2021 14:51

kibanamachine mentioned this pull request Oct 19, 2021

[7.x] [ML] Adding ability to change data view in advanced job wizard (#115191) #115585

Merged

peteharverson mentioned this pull request Oct 26, 2021

[ML] Renaming index patterns for job wizards #116250

Merged

1 task

	defaultMessage="No detectors have been configured, so changing to this data view should be ok."
	defaultMessage="No detectors have been configured; this data view can be applied to the job."

	defaultMessage="This data view appears to be a good match for this job."
	defaultMessage="This data view can be applied to this job."

	defaultMessage="This data view produced no results when previewing the datafeed. This could be due to there being no documents in {dataViewTitle}."
	defaultMessage="This data view produced no results when previewing the datafeed. There might be no documents in {dataViewTitle}."

	defaultMessage="Changing the current data view may cause problems with the current job configuration as it may not contain the same fields."
	defaultMessage="Changing the current data view may cause problems with the corresponding job configuration as the newly selected data view may not contain the same fields."

	defaultMessage="Select new data view for job"
	defaultMessage="Select new data view for the job"

	defaultMessage=" Checking compatibility of data view with job"
	defaultMessage=" Checking data view and job compatibility"

	defaultMessage="The currently selected data view being used for this job."
	defaultMessage="The data view that is currently used for this job."

	defaultMessage="It is not possible to alter the indices being used by the datafeed here. If you wish to select a different index pattern or saved search, please jump to step 1 of the wizard and select change index pattern."
	defaultMessage="You cannot alter the indices being used by the datafeed here. To select a different index pattern or saved search, go to step 1 of the wizard and select the Change index pattern option."

	const gg = await validateDatafeedPreview({
	const validationResponse = await validateDatafeedPreview({

[ML] Adding ability to change data view in advanced job wizard #115191

[ML] Adding ability to change data view in advanced job wizard #115191

Conversation

jgowdyelastic commented Oct 15, 2021 • edited by peteharverson Loading

Checklist

Risk Matrix

For maintainers

jgowdyelastic commented Oct 18, 2021

elasticmachine commented Oct 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peteharverson commented Oct 18, 2021 • edited by jgowdyelastic Loading

peteharverson left a comment

Choose a reason for hiding this comment

szabosteve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szabosteve Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

jgowdyelastic commented Oct 19, 2021

pheyos left a comment

Choose a reason for hiding this comment

darnautov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pheyos left a comment

Choose a reason for hiding this comment

kibanamachine commented Oct 19, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

kibanamachine commented Oct 19, 2021

💚 Backport successful

jgowdyelastic commented Oct 15, 2021 •

edited by peteharverson

Loading

peteharverson commented Oct 18, 2021 •

edited by jgowdyelastic

Loading

szabosteve Oct 18, 2021 •

edited

Loading