Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ML] Add warnings for actions for managed Anomaly detection jobs and Transforms #122305

Merged
merged 34 commits into from
Jan 19, 2022

Conversation

qn895
Copy link
Member

@qn895 qn895 commented Jan 5, 2022

Summary

This PR addresses #120631 adds warning modals when users modify or delete one or more Anomaly detection jobs or Transforms that are managed by other solutions. If the job is not managed, these modals will not be visible.

Anomaly detection

Delete
Screen Shot 2022-01-10 at 13 23 42
Screen Shot 2022-01-10 at 13 23 52

Edit

Screen Shot 2022-01-10 at 13 23 26

Stop

Screen Shot 2022-01-10 at 13 25 10

Screen Shot 2022-01-10 at 13 25 23

Start

Screen Shot 2022-01-10 at 13 24 06

Screen Shot 2022-01-10 at 13 24 27

Close
Screen Shot 2022-01-10 at 13 25 48
Screen Shot 2022-01-10 at 13 26 02

Reset
Screen Shot 2022-01-10 at 13 29 36
Screen Shot 2022-01-10 at 13 29 50

Transforms

Delete
Screen Shot 2022-01-10 at 13 58 16
Screen Shot 2022-01-10 at 13 58 29

Start
Screen Shot 2022-01-10 at 13 57 14
Screen Shot 2022-01-10 at 13 57 27

Stop
Screen Shot 2022-01-10 at 13 56 00
Screen Shot 2022-01-10 at 13 56 15

Checklist

@qn895 qn895 marked this pull request as ready for review January 6, 2022 15:00
@qn895 qn895 requested a review from a team as a code owner January 6, 2022 15:00
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@jgowdyelastic
Copy link
Member

jgowdyelastic commented Jan 6, 2022

A special formatter is needed to correctly show the managed boolean value in the jobs list.
image


Actually, there are other booleans that are missing. This can be done in a follow up.

@jgowdyelastic
Copy link
Member

jgowdyelastic commented Jan 6, 2022

We probably want to add the managed warning to the delete modal when the user doesn't have access to all of the spaces the job belongs in.

image

And when the user has access to all spaces
image

@jgowdyelastic
Copy link
Member

jgowdyelastic commented Jan 6, 2022

I'm not sure about having these warnings in new modals.
They could be added as callouts to existing modals or flyouts and would be less intrusive.
Plus from a code point of view, the callout could just be a single component that takes a job and decides whether or not to display itself.

E.g. at the top of the edit jobs flyout
image

or in the delete modal

image

For the stop datafeed option we would need a new modal, but it could contain the same callout.

Thoughts @peteharverson @qn895 ?

@jgowdyelastic
Copy link
Member

It would be helpful to have a tooltip on the managed label on the jobs list, to explain what a managed job is.
image

@jgowdyelastic
Copy link
Member

Do we want to strip out the managed flag when cloning a job? Currently it's left in.
My initial thought was to remove it, as it won't be the exact job that the non-ml plugin is looking for.
However some plugins (security) look for groups and list all jobs in a known group. They may want the ability to clone a job and it be treated exactly like the original.

@sophiec20
Copy link
Contributor

For the examples we have, the managed job has to have a specific job_id .. therefore I do not believe we should clone the managed tag.

jobId: jobIds[0],
}}
/>
{
Copy link
Member

Choose a reason for hiding this comment

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

nit, these braces aren't needed

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed here 352559c

@@ -324,6 +327,8 @@ export class EditJobFlyoutUI extends Component {
jobClosed,
} = this.state;

console.log('job', job, isManagedJob(job));
Copy link
Member

Choose a reason for hiding this comment

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

this console.log should probably be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed here 352559c

import { i18n } from '@kbn/i18n';
import { MlSummaryJob } from '../../../../../../common/types/anomaly_detection_jobs';
import { isManagedJob } from '../../../jobs_utils';
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

we have a utils.d.ts that stopDatafeeds could be added to to avoid this @ts-ignore

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated here af963d1

// 'Managed' means job is preconfigured and deployed by other solutions
// we should not clone the setting
if (job.custom_settings?.managed === true) {
job.custom_settings.managed = false;
Copy link
Member

Choose a reason for hiding this comment

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

My draft PR has been closed, so the change to remove managed will need to be added to this PR

@@ -42,6 +43,8 @@ export const useActions = ({
modals: (
<>
{startAction.isModalVisible && <StartActionModal {...startAction} />}
{stopAction.isModalVisible && <StopActionModal {...stopAction} />}

{editAction.config && editAction.isFlyoutVisible && (
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also add the warning if the user tries to edit the transform.

Copy link
Member Author

@qn895 qn895 Jan 18, 2022

Choose a reason for hiding this comment

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

Updated here a7f844b

Screen Shot 2022-01-18 at 08 59 10

@peteharverson peteharverson self-requested a review January 17, 2022 16:17
@qn895 qn895 requested a review from jgowdyelastic January 18, 2022 15:00
export const overrideTransformForCloning = (originalConfig: TransformConfigUnion) => {
// 'Managed' means job is preconfigured and deployed by other solutions
// we should not clone this setting
return { ...originalConfig, _meta: { ...originalConfig._meta, managed: false } };
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit - should we be adding

  "_meta": {
    "managed": false
  }

when cloning any transform (including ones which aren't managed), or not adding the managed field at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think managed:false is more explicit but to match with what we have in ML, I removed it altogether here a13f502

Copy link
Member

@jgowdyelastic jgowdyelastic Jan 19, 2022

Choose a reason for hiding this comment

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

The problem with setting it to false is that we'll only get transforms (or AD jobs) with "managed": false in them if they are cloned.
The user may wonder why "managed": false is only appearing for their cloned transforms and not for other other transforms they've created.

If we want to always have a managed property in transforms and AD jobs we'll have to add this to the wizards.

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.

Added one comment about cloning transforms, but otherwise tested latest edits and LGTM.

@qn895 qn895 enabled auto-merge (squash) January 18, 2022 17:24
@qn895 qn895 disabled auto-merge January 18, 2022 17:25
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

@qn895
Copy link
Member Author

qn895 commented Jan 19, 2022

@elasticmachine merge upstream

@qn895 qn895 added the auto-backport Deprecated - use backport:version if exact versions are needed label Jan 19, 2022
@qn895 qn895 enabled auto-merge (squash) January 19, 2022 14:48
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
ml 1592 1596 +4
transform 290 293 +3
total +7

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
ml 287 289 +2

Any counts in public APIs

Total count of every any typed public API. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats any for more detailed information.

id before after diff
ml 8 10 +2

Async chunks

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

id before after diff
infra 997.0KB 997.1KB +90.0B
ml 3.5MB 3.5MB +8.1KB
transform 360.3KB 363.3KB +3.0KB
total +11.2KB
Unknown metric groups

API count

id before after diff
ml 291 293 +2

History

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

cc @qn895

@qn895 qn895 merged commit 57ce437 into elastic:main Jan 19, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 19, 2022
@kibanamachine
Copy link
Contributor

💔 Backport failed

The pull request could not be backported due to the following error:
There are no branches to backport to. Aborting.

How to fix

Re-run the backport manually:

node scripts/backport --pr 122305

Questions ?

Please refer to the Backport tool documentation

@qn895 qn895 removed the auto-backport Deprecated - use backport:version if exact versions are needed label Jan 19, 2022
@qn895 qn895 deleted the ml-ad-managed-badges branch February 9, 2022 16:32
ajosh0504 added a commit that referenced this pull request Apr 24, 2023
## Summary
This PR makes the following updates to the pre-built Security ML jobs:
- Making the `security-packetbeat` compatible with Agent
- Removing superfluous fields from the job configurations to make them
consistent
- Updating the `detector_description` field for almost all jobs
- Adding influencers where missing and/or relevant
- Adding a `job_revision` custom setting similar to the Logs
[jobs](https://github.com/elastic/kibana/blob/main/x-pack/plugins/ml/server/models/data_recognizer/modules/logs_ui_analysis/ml/log_entry_rate.json#L29).
Moving forward, this number will be updated each time a job is updated.
We are starting with 4 since the `linux` and `windows` jobs are at v3
right now
- Adding a `managed`: `true` tag to indicate that these jobs are
pre-configured by Elastic and so users will see the warnings added in
[this](#122305) PR if users choose
to delete, or modify these jobs

---------

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
Labels
backport:skip This commit does not require backporting Feature:Anomaly Detection ML anomaly detection Feature:Transforms ML transforms :ml release_note:enhancement v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants