[ML] Add switch to enable model plot annotations independently #70678

qn895 · 2020-07-02T23:58:44Z

Summary

This P adds an additional switch toggle to enable model plot annotations independently from the model plot. Part of #69538 and is related to #68232

It will have both options on by default for Single Metric jobs, and off by default for the rest

2) If user does enable model plot without annotations, a prompt will be given to suggest that they turn it on to take advantage of the feature if the allows for it

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
This was checked for keyboard-only and screenreader accessibility

elasticmachine · 2020-07-02T23:58:47Z

Pinging @elastic/ml-ui (:ml)

jgowdyelastic · 2020-07-03T07:45:54Z

...s/job_details_step/components/advanced_section/components/annotations/annotations_switch.tsx

+
+  useEffect(() => {
+    if (jobCreator.modelPlot === undefined) {
+      jobCreator.modelPlot = false;


I don't think this is needed, modelPlot is a boolean so this should never be true.

Removed in 1b54be9

jgowdyelastic · 2020-07-03T08:01:13Z

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

+        this._job_config.model_plot_config.annotations_enabled = true;
+      } else {
+        this._job_config.model_plot_config = {
+          enabled: false,


i believe this isn't needed, enabled should default to false

both this and public set modelPlot above should have a check to see if all items inside are false, if so, delete model_plot_config.
line 235 above (thanks github for not allowing me to add a comment there) should not now delete model_plot_config if modelPlot is set to false as it will wipe out annotations_enabled

I tested trying to create a job where model plot was disabled, but model change annotations were enabled, and if I check in the JSON for the created job, this block which should be there is missing:

"model_plot_config": { "annotations_enabled": true },

I think I had to do enabled: false there because ModelPlotConfig interface has enabled as required. And you're right James, deleting that was causing the bug Pete ran into. I have updated to delete the model_plot_config object only if both of the enabled and annotations_enabled are false in 1b54be9.

droberts195 · 2020-07-03T10:55:29Z

I wonder if we should call these "model change annotations" instead of "model plot annotations"? The event for these annotations is model_change, and they get created when there's a significant change in the model.

Switching from "Enable model plot annotations" to "Enable model change annotations" would also avoid doubling from 3 to 6 the number of occurrences of "Enable model plot" that @lcawl complained about in #68232 (comment).

jgowdyelastic · 2020-07-03T13:56:16Z

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

+        const body: Partial<Job> = request.body;
+
+        // if annotations is enabled but model plot is not
+        if (


we shouldn't have additional logic in this endpoint. this should be a simple proxy to the es endpoint and should behave in the exact same way.

Updated in 1b54be9

jgowdyelastic · 2020-07-03T14:02:41Z

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

@@ -243,6 +243,25 @@ export class JobCreator {
    );
  }

+  public set annotations(enable: boolean) {


this would be better named modelPlotAnnotations

Changed it to modelChangeAnnotations in 1b54be9

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

...s/job_details_step/components/advanced_section/components/annotations/annotations_switch.tsx

...mponents/job_details_step/components/advanced_section/components/annotations/description.tsx

- Rename to model change annotations - Delete model_plot_config if all fields are false - Always explicitly mark if either field are t or f - Remove API side logic

…tions-create-wizard # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

jgowdyelastic · 2020-07-08T19:39:29Z

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

-      this._job_config.model_plot_config = {
-        enabled: true,
-      };
+      this._job_config.model_plot_config.enabled = true;


this can now be this._job_config.model_plot_config.enabled = enable

Updated here fcb1bab

jgowdyelastic · 2020-07-08T19:42:41Z

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

@@ -175,9 +176,11 @@ export function jobRoutes({ router, mlLicense }: RouteInitialization) {
    mlLicense.fullLicenseAPIGuard(async (context, request, response) => {
      try {
        const { jobId } = request.params;
+        const body: Partial<Job> = request.body;


i don't think this is needed

Removed here fcb1bab

jgowdyelastic · 2020-07-08T19:43:22Z

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

@@ -125,6 +125,7 @@ export const Page: FC<PageProps> = ({ existingJobsAndGroups, jobType }) => {

    if (jobCreator.type === JOB_TYPE.SINGLE_METRIC) {
      jobCreator.modelPlot = true;
+      jobCreator.modelChangeAnnotations = true;


do we definitely want to enabling this for all single metric jobs?

I think just enabling modelPlot (and omitting modelChangeAnnotations) will actually enable modelChangeAnnotations as well by default on elastic's side so I thought it would be clearer to state modelChangeAnnotations as true on the UI side for consistency.

As for why we wanted both on for SM jobs, the discussion was that we expect annotations to be fairly high volume for high cardinality jobs, but nowhere near as high volume as model plot. So if we can tolerate the volume of model plot, we would probably also want annotations.

I think just enabling modelPlot (and omitting modelChangeAnnotations) will actually enable modelChangeAnnotations as well by default on elastic's side

This is no longer possible with the changes you added in 405c2a7. So if we do want to only enable modelPlot this line could be omitted.

I was asking more from a product design point of view, whether this had been discussed and it sounds like it has.
I was mainly concerned about the noise of the new annotations that will be generated.

kibanamachine · 2020-07-09T01:02:01Z

💛 Build succeeded, but was flaky

continuous-integration/kibana-ci/pull-request
Commit: fcb1bab
Flaky suites:
- xpack-kibana-ciGroup1

Build metrics

✅ unchanged

History

💔 Build #59844 failed 405c2a7
💔 Build #59792 failed b49baf1
💚 Build #59232 succeeded 327c3e2
💛 Build #58569 was flaky e7411f7

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

peteharverson

Tested and LGTM

jgowdyelastic

LGTM

…ic#70678)

…70678) (#71251)

[ML] Add switch to enable model plot annotations independently

e7411f7

qn895 added release_note:enhancement :ml Feature:Anomaly Detection ML anomaly detection v8.0.0 v7.9.0 labels Jul 2, 2020

qn895 requested review from alvarezmelissa87, peteharverson and jgowdyelastic July 2, 2020 23:58

qn895 requested a review from a team as a code owner July 2, 2020 23:58

qn895 self-assigned this Jul 2, 2020

jgowdyelastic reviewed Jul 3, 2020

View reviewed changes

peteharverson reviewed Jul 3, 2020

View reviewed changes

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

peteharverson reviewed Jul 3, 2020

View reviewed changes

...s/job_details_step/components/advanced_section/components/annotations/annotations_switch.tsx Outdated Show resolved Hide resolved

...mponents/job_details_step/components/advanced_section/components/annotations/description.tsx Outdated Show resolved Hide resolved

qn895 added 5 commits July 6, 2020 17:36

[ML] Refactor model change annotations

1b54be9

- Rename to model change annotations - Delete model_plot_config if all fields are false - Always explicitly mark if either field are t or f - Remove API side logic

[ML] Update default behavior when page /new_job is loaded

42d9697

[ML] Use _initModelPlotConfig

405c2a7

jgowdyelastic reviewed Jul 8, 2020

View reviewed changes

[ML] Fix type error, remove Job, shorten set logic

fcb1bab

peteharverson approved these changes Jul 9, 2020

View reviewed changes

jgowdyelastic approved these changes Jul 9, 2020

View reviewed changes

qn895 merged commit cf3133a into elastic:master Jul 9, 2020

qn895 deleted the model-plot-annotations-create-wizard branch July 9, 2020 14:16

qn895 mentioned this pull request Jul 9, 2020

[7.x] [ML] Add switch to enable model plot annotations independently (#70678) #71251

Merged

qn895 added a commit to qn895/kibana that referenced this pull request Jul 9, 2020

[ML] Add switch to enable model plot annotations independently (elast…

dd24e46

…ic#70678)

qn895 added a commit that referenced this pull request Jul 9, 2020

[7.x] [ML] Add switch to enable model plot annotations independently (#…

571ca8f

…70678) (#71251)

peteharverson mentioned this pull request Jul 14, 2020

[ML] Add control to job wizards for enabling model change annotations #68232

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Add switch to enable model plot annotations independently #70678

[ML] Add switch to enable model plot annotations independently #70678

qn895 commented Jul 2, 2020

elasticmachine commented Jul 2, 2020

jgowdyelastic Jul 3, 2020

qn895 Jul 7, 2020

jgowdyelastic Jul 3, 2020

jgowdyelastic Jul 3, 2020 •

edited

Loading

peteharverson Jul 3, 2020

qn895 Jul 7, 2020

droberts195 commented Jul 3, 2020

jgowdyelastic Jul 3, 2020

qn895 Jul 7, 2020

jgowdyelastic Jul 3, 2020

qn895 Jul 7, 2020

jgowdyelastic Jul 8, 2020

qn895 Jul 8, 2020

jgowdyelastic Jul 8, 2020

qn895 Jul 8, 2020

jgowdyelastic Jul 8, 2020

qn895 Jul 8, 2020 •

edited

Loading

jgowdyelastic Jul 9, 2020 •

edited

Loading

kibanamachine commented Jul 9, 2020

peteharverson left a comment

jgowdyelastic left a comment

[ML] Add switch to enable model plot annotations independently #70678

[ML] Add switch to enable model plot annotations independently #70678

Conversation

qn895 commented Jul 2, 2020

Summary

Checklist

elasticmachine commented Jul 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgowdyelastic Jul 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droberts195 commented Jul 3, 2020

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

qn895 Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

jgowdyelastic Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

kibanamachine commented Jul 9, 2020

💛 Build succeeded, but was flaky

Build metrics

History

peteharverson left a comment

Choose a reason for hiding this comment

jgowdyelastic left a comment

Choose a reason for hiding this comment

jgowdyelastic Jul 3, 2020 •

edited

Loading

qn895 Jul 8, 2020 •

edited

Loading

jgowdyelastic Jul 9, 2020 •

edited

Loading