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] Anomaly Detection alert initialisation from the ML app #91283

Merged
merged 12 commits into from
Feb 15, 2021

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented Feb 12, 2021

Summary

Part of #88940

  • Allows initializing the Anomaly Detection alert directly in the Job wizard
    Feb-12-2021 11-36-20

  • Allows initializing the Anomaly Detection alert from the Job list with single and bulk actions
    Feb-12-2021 11-38-46

  • Adds "Create alert" action to the jobs management table
    image
    image

  • Adds control for including/excluding interim results
    image

  • Removes Watcher integration from the ML plugin

Checklist

Delete any items that are not applicable to this PR.

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@darnautov darnautov added the auto-backport Deprecated - use backport:version if exact versions are needed label Feb 15, 2021
@darnautov darnautov requested a review from a team as a code owner February 15, 2021 14:37
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.

Code LGTM

@@ -37,6 +37,7 @@ export const emptyMlCapabilities: MlCapabilitiesResponse = {
canDeleteDataFrameAnalytics: false,
canCreateDataFrameAnalytics: false,
canStartStopDataFrameAnalytics: false,
canCreateMlAlerts: false,
Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify, is there any expectation we check this permission and restrict access within Detections for creating ML Rules? I imagine no since the Detection Engine is managed as a separate application, and this seems specific to the new ML Alert Type, but want to make sure we're respecting permissions as expected from the ML permissions model perspective.

i.e Should we disable the ML Detections Rule type if canCreateMlAlerts:false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hey @spong! 👋 I reckon canCreateMlAlerts should not affect the ML Detections Rule in Security app, as it's dedicated to the ML related alert types only. No actions are required on your side

Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

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

Security Solution changes LGTM! One outstanding question on if we should be respecting the new canCreateMlAlerts ML permission within the Detection Engine, but we can open a dedicated issue to address if necessary. Thanks @darnautov! 🙂

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 and LGTM

@darnautov darnautov enabled auto-merge (squash) February 15, 2021 17:16
@darnautov darnautov disabled auto-merge February 15, 2021 18:39
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
ml 1735 1730 -5

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.3MB 6.3MB -22.4KB
securitySolution 7.5MB 7.5MB +24.0B
total -22.4KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
ml 68.2KB 68.1KB -120.0B

History

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

@darnautov darnautov merged commit a8e1e47 into elastic:master Feb 15, 2021
@darnautov darnautov deleted the ML-88940-alert-from-ml-app branch February 15, 2021 19:47
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 15, 2021
@kibanamachine
Copy link
Contributor

Backport result

{"level":"info","message":"POST https://api.github.com/graphql (status: 200)"}
{"level":"info","message":"POST https://api.github.com/graphql (status: 200)"}
{"meta":{"labels":[":ml","Feature:Alerting","Feature:Anomaly Detection","auto-backport","release_note:enhancement","v7.12.0","v8.0.0"],"branchLabelMapping":{"^v8.0.0$":"master","^v7.12.0$":"7.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"},"existingTargetPullRequests":[]},"level":"info","message":"Inputs when calculating target branches:"}
{"meta":["7.x"],"level":"info","message":"Target branches inferred from labels:"}
{"meta":{"killed":false,"code":2,"signal":null,"cmd":"git remote rm kibanamachine","stdout":"","stderr":"error: No such remote: 'kibanamachine'\n"},"level":"info","message":"exec error 'git remote rm kibanamachine':"}
{"meta":{"killed":false,"code":2,"signal":null,"cmd":"git remote rm elastic","stdout":"","stderr":"error: No such remote: 'elastic'\n"},"level":"info","message":"exec error 'git remote rm elastic':"}
{"level":"info","message":"Backporting [{\"sourceBranch\":\"master\",\"targetBranchesFromLabels\":[\"7.x\"],\"sha\":\"a8e1e47de6ee7b385c17928e91589db630835d6c\",\"formattedMessage\":\"[ML] Anomaly Detection alert initialisation from the ML app  (#91283)\",\"originalMessage\":\"[ML] Anomaly Detection alert initialisation from the ML app  (#91283)\",\"pullNumber\":91283,\"existingTargetPullRequests\":[]}] to 7.x"}

Backporting to 7.x:
{"level":"info","message":"Backporting via filesystem"}
{"level":"info","message":"Creating PR with title: \"[7.x] [ML] Anomaly Detection alert initialisation from the ML app  (#91283)\". kibanamachine:backport/7.x/pr-91283 -> 7.x"}
{"level":"info","message":"POST /repos/elastic/kibana/pulls - 201 in 1172ms"}
{"level":"info","message":"Adding assignees to #91441: darnautov"}
{"level":"info","message":"POST /repos/elastic/kibana/issues/91441/assignees - 201 in 526ms"}
{"level":"info","message":"Adding labels: backport"}
{"level":"info","message":"POST /repos/elastic/kibana/issues/91441/labels - 200 in 365ms"}
View pull request: https://github.com/elastic/kibana/pull/91441

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Alerting Feature:Anomaly Detection ML anomaly detection :ml release_note:enhancement v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants