Skip to content

[ML] Fix incorrect behaviors for Anomaly Detection jobs when resetting or converting to advanced job#90078

Merged
qn895 merged 10 commits intoelastic:masterfrom
qn895:ml-fix-cloning-adv-reset
Feb 5, 2021
Merged

[ML] Fix incorrect behaviors for Anomaly Detection jobs when resetting or converting to advanced job#90078
qn895 merged 10 commits intoelastic:masterfrom
qn895:ml-fix-cloning-adv-reset

Conversation

@qn895
Copy link
Member

@qn895 qn895 commented Feb 2, 2021 •
edited
Loading

Summary

This PR fixes a bug introduced by #88898 which causes datafeed_config to be appended back to the job when resetting or converting to advanced job.

Before

  • Converting a job to an advanced job and then create the job will give an error

Screen Shot 2021-02-02 at 12 29 47

  • Resetting the job after running the wizard

Screen Shot 2021-02-02 at 13 51 56

@qn895 qn895 added bug Fixes for quality problems that affect the customer experience :ml Feature:Anomaly Detection ML anomaly detection v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Feb 2, 2021
@qn895 qn895 self-assigned this Feb 2, 2021
@qn895 qn895 requested a review from a team as a code owner February 2, 2021 19:45
@elasticmachine
Copy link
Contributor

elasticmachine commented Feb 2, 2021

Pinging @elastic/ml-ui (:ml)

jgowdyelastic
jgowdyelastic reviewed Feb 2, 2021
View reviewed changes
x-pack/plugins/ml/public/application/jobs/jobs_list/components/utils.js
const hasDatafeed =
typeof job.datafeed_config === 'object' && Object.keys(job.datafeed_config).length > 0;
const datafeedId = hasDatafeed ? job.datafeed_config.datafeed_id : '';
const hasDatafeed = typeof datafeed === 'object' && Object.keys(datafeed).length > 0;
Copy link
Member

@jgowdyelastic jgowdyelastic Feb 2, 2021

Choose a reason for hiding this comment

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

datafeed should be set to undefined like job is on line 377.
This should also happen here:

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

Line 90 in 43faa76

mlJobService.tempJobCloningObjects.job = undefined;

Sorry, I missed this on the previous PR.

Copy link
Member Author

@qn895 qn895 Feb 3, 2021

Choose a reason for hiding this comment

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

Updated here d867b46

@spalger
Copy link
Contributor

spalger commented Feb 3, 2021

jenkins test this

@peteharverson
Copy link
Contributor

peteharverson commented Feb 4, 2021 •
edited
Loading

I'm seeing some strange behavior when resetting a multi metric, or population job, after it has run. Sometimes it goes into the Single metric wizard, and sometimes it's gone into the Advanced wizard.

For example, with this config on cloudwatch-* it went into the Advanced wizard when hitting Reset:

  "custom_settings": {
    "created_by": "multi-metric-wizard"
  },
  "description": "",
  "analysis_config": {
    "bucket_span": "1h",
    "detectors": [
      {
        "detector_description": "mean(CPUUtilization) partitionfield=instance",
        "function": "mean",
        "field_name": "CPUUtilization",
        "partition_field_name": "instance",
        "detector_index": 0
      }
    ],
    "influencers": [
      "instance",
      "region"
    ]
  },

Update: I no longer see this issue after the edits in b8de989

peteharverson
peteharverson reviewed Feb 4, 2021
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.

I no longer see the issues with Reset job after the changes in b8de989.

peteharverson
peteharverson approved these changes Feb 4, 2021
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 edit and LGTM

jgowdyelastic
jgowdyelastic reviewed Feb 5, 2021
View reviewed changes
x-pack/plugins/ml/public/application/services/job_service.d.ts Outdated
saveNewJob(job: any): Promise<any>;
cloneDatafeed(datafeed: any): Datafeed;
saveNewJob(job: Job): Promise<any>;
cloneDatafeed(Datafeed: Datafeed | undefined): Datafeed;
Copy link
Member

@jgowdyelastic jgowdyelastic Feb 5, 2021

Choose a reason for hiding this comment

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

this function shouldn't take undefined as it doesn't know how to deal with it.
we need a check here to ensure datafeed is not undefined:

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

Line 75 in 3a388c6

if (mlJobService.tempJobCloningObjects.job !== undefined) {

Copy link
Member Author

@qn895 qn895 Feb 5, 2021

Choose a reason for hiding this comment

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

Updated here 0fa0b09

jgowdyelastic
jgowdyelastic approved these changes Feb 5, 2021
View reviewed changes
Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM

@kibanamachine
Copy link
Contributor

kibanamachine commented Feb 5, 2021

💚 Build Succeeded

Metrics [docs]

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.6MB 6.6MB +718.0B

History

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

@qn895 qn895 merged commit eff9d43 into elastic:master Feb 5, 2021
@qn895 qn895 deleted the ml-fix-cloning-adv-reset branch February 5, 2021 19:48
@qn895 qn895 mentioned this pull request Feb 5, 2021
Merged
qn895 added a commit that referenced this pull request Feb 9, 2021
@qn895 @kibanamachine
[ML] Fix incorrect behaviors for Anomaly Detection jobs when resettin…
d2eb2d8 
…g or converting to advanced job (#90078) (#90503)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@peteharverson peteharverson peteharverson approved these changes

@jgowdyelastic jgowdyelastic jgowdyelastic approved these changes

Assignees

@qn895 qn895

Labels

bug Fixes for quality problems that affect the customer experience Feature:Anomaly Detection ML anomaly detection :ml release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@qn895 @elasticmachine @spalger @peteharverson @kibanamachine @jgowdyelastic