[RAM] Add bulk action to Untrack selected alerts by Zacqary · Pull Request #167579 · elastic/kibana

Zacqary · 2023-09-28T22:56:01Z

Summary

This PR:

Moves the setAlertStatusToUntracked function from the AlertsClient into the AlertsService. This function doesn't actually need any Rule IDs to do what it's supposed to do, only indices and Alert UUIDs. Therefore, we want to make it possible to use outside of a created AlertsClient, which requires a Rule to initialize.
Creates a versioned internal API to bulk untrack a given set of alertUuids present on indices. Both of these pieces of information are readily available from the ECS fields sent to the alert table component, from where this bulk action will be called.
Switches the setAlertStatusToUntracked query to look for alert UUIDs instead of alert instance IDs. [RAM] Mark disabled alerts as Untracked in both Stack and o11y #164788 dealt with untracking alerts that were bound to a single rule at a time, but this PR could be untracking alerts generated by many different rules at once. Multiple rules may generate the same alert instance ID names with different UUIDs, so using UUID increases the specificity and prevents us from untracking alert instances that the user didn't intend.
Adds a bulkUpdateState method to the task scheduler. [RAM] Mark disabled alerts as Untracked in both Stack and o11y #164788 modified the bulkDisable method to clear untracked alerts from task states, but this new method allows us to untrack a given set of alert instances without disabling the task that generated them.

Why omit rule ID from this API?

The rule ID is technically readily available from the alert table, but it becomes redundant when we already have immediate access to the alert document's index. #164788 used the rule ID to get the ruleTypeId and turn this into a corresponding index, which we don't have to do anymore.

Furthermore, it helps to omit the rule ID from the updateByQuery request, because the user can easily select alerts that were generated by a wide variety of different rules, and untrack them all at once. We could include the rule ID in a separate should query, but this adds needless complexity to the query.

We do need to know the rule ID after performing updateByQuery, because it corresponds to the task state we want to modify, but it's easier to retrieve this using the same query params provided.

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
Unit or functional tests were updated or added to match the most common scenarios

ghost · 2023-09-28T22:56:17Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
/oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

Zacqary · 2023-09-28T23:00:54Z

x-pack/plugins/alerting/server/alerts_service/alerts_service.ts

    }
  }
+
+  public async setAlertsToUntracked(opts: SetAlertsToUntrackedOpts) {


I'm unsure that this method actually belongs in AlertsService rather than AlertsClient, because it interacts with and updates alert data. However, we want to make it available without being connected to a Rule (which is a prerequisite for creating an AlertsClient), and it also doesn't actually need a corresponding Rule to operate.

Not sure if there's a specific design goal re: Service vs. Client that I'm violating here with this, but it was the most expedient way I could think of to get this to work.

This is probably fine for now. The initial intent was for the AlertService to handle all of the framework level resource installation that's required to write alerts for a rule and then AlertsClient to be the way for the alerting task runner to interact with those alerts. As you point out, there are use cases for users interacting directly with alerts. In the future, we may want to consolidate all of those interactions into a separate client but this is fine for now.

…fix'

…on-164059

elasticmachine · 2023-09-29T23:06:41Z

Pinging @elastic/response-ops (Team:ResponseOps)

maryam-saeidi

Tested locally and for happy path it worked fine.

But the Mark as untracked option for recovered alerts:

When I applied it, it showed a successful message:

Can you please fix that?

Another issue was related to AlertSummaryWidget component:

Number of active alerts includes the untracked alerts as well, can it be fixed in this PR? If it is out of scope, would you please create an issue?

Questions:

What is the difference between AlertsClient and AlertsService?
@maciejforcone @katrin-freihofner Should we count untracked alerts in the total number of alerts in the Alert Summary Widget? [Out of scope of this PR]

Alert Summary Widget	Alerts

maryam-saeidi · 2023-10-02T12:57:18Z

x-pack/plugins/observability/public/pages/alerts/components/alert_actions.tsx

        </EuiContextMenuItem>
      ),
    ],
+    ...(ruleSavePermissions


What exactly is this permission? Is this different from the permission for adding an alert to a case?

I think this is the permission for editing rules. I didn't see a permissions requirement spec'd out at all so I wasn't sure which one to use, but I can switch it to the case permission, that probably makes more sense.

@maryam-saeidi How's tying this to the update case permission sound? Or do we need to create an entirely new permission for this?

Talked to @XavierM, turns out we don't need permissions on the client side because the user can only see alerts from rules that they're authorized to interact with. I need to implement some auth checking on the server side though.

Thanks for the update 👍🏻

x-pack/plugins/alerting/server/alerts_service/lib/set_alerts_to_untracked.ts

Zacqary · 2023-10-02T15:24:25Z

@maryam-saeidi

Questions

What is the difference between AlertsClient and AlertsService?

I was wondering that myself. As far as I can tell the AlertsClient is a thing we create specifically to interact with the alerts generated by a particular rule, given that it needs rule properties to be initialized.

The AlertsService on the other hand is, uhhhh, the thing that has the right elasticsearch client stuff already built in and feature freeze is tomorrow.

EDIT Sorry missed @ymao1's explanation: #167579 (comment)

…lkaction-164059

XavierM · 2023-10-03T17:54:06Z

x-pack/plugins/alerting/server/routes/rule/apis/bulk_untrack/bulk_untrack_alert_route.ts

+) => {
+  router.post(
+    {
+      path: `${INTERNAL_BASE_ALERTING_API_PATH}/rules/_bulk_untrack`,


I think we should not use rules but alerts here

Suggested change

path: `${INTERNAL_BASE_ALERTING_API_PATH}/rules/_bulk_untrack`,

path: `${INTERNAL_BASE_ALERTING_API_PATH}/alerts/_bulk_untrack`,

...ugins/triggers_actions_ui/public/application/sections/alerts_table/hooks/use_bulk_actions.ts

XavierM · 2023-10-03T17:59:52Z

x-pack/plugins/alerting/server/alerts_service/lib/set_alerts_to_untracked.ts

+    },
+  ];
+
+  if (ensureAuthorized) {


ensureAuthorized should always be there if we can, we need to enforce this check!

setAlertsToUntracked is sometimes called through untrackRuleAlerts, which is only ever called after a different ensureAuthorized method has run

let's go with the way it is and we can refactor it later on

ymao1

Tried out all the disable/delete/untrack scenarios. Left some comments about unit tests that I'm fine with being addressed as a follow up issue.

Would like to see two things addressed:

try/catch around the callback function that updates the task manager state
deleting a rule did not mark the associated active alerts as untracked when I tested it.

x-pack/plugins/alerting/server/alerts_service/lib/set_alerts_to_untracked.test.ts

x-pack/plugins/alerting/server/rules_client/methods/delete.ts

x-pack/plugins/alerting/server/application/rule/methods/bulk_untrack/bulk_untrack_alerts.ts

x-pack/plugins/task_manager/server/task_scheduling.ts

elasticmachine · 2023-10-03T19:13:58Z

Pinging @elastic/apm-ui (Team:APM)

…into bulkaction-164059

ymao1

Verified with the latest commits that delete works & the requested try/catch has been updated. I see @XavierM left a comment about ensureAuthorized so I'll let him give the final 👍 but LGTM

…into bulkaction-164059

achyutjhunjhunwala

LGTM from APM side

kibana-ci · 2023-10-04T00:47:08Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: b85edd2

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #38 / Alerting alerts_as_data alerts as data "after all" hook for "should write alert docs during rule execution"
[job] [logs] Defend Workflows Cypress Tests #4 / Artifact pages Trusted applications should update Endpoint Policy on Endpoint when adding Trusted application name should update Endpoint Policy on Endpoint when adding Trusted application name
[job] [logs] Security Solution Cypress Tests #9 / Ransomware Detection Alerts Ransomware in Timelines Renders ransomware entries in timelines table Renders ransomware entries in timelines table

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`triggersActionsUi`	586	587	+1

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
`triggersActionsUi`	552	553	+1

Async chunks

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

id	before	after	diff
`observability`	1.0MB	1.0MB	+438.0B
`triggersActionsUi`	1.4MB	1.4MB	+1.2KB
total			+1.6KB

Page load bundle

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

id	before	after	diff
`triggersActionsUi`	92.8KB	93.9KB	+1.1KB

Unknown metric groups

API count

id	before	after	diff
`triggersActionsUi`	578	579	+1

History

💔 Build #165140 failed 25114b3
💔 Build #165107 failed 2a7b1a9
💔 Build #165006 failed ac63b3e
💔 Build #164958 failed 3e6c478
💔 Build #164915 failed c15286b
💔 Build #164765 failed c506384

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

Zacqary added 3 commits September 28, 2023 14:51

Add bulk_untrack API

3d5ff27

Add bulk action for untracking alerts

e5ac9ec

Clear task state after bulk untrack

f5b6a81

Zacqary added Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// release_note:feature Makes this part of the condensed release notes Feature:Alerting/RulesManagement Issues related to the Rules Management UX v8.11.0 labels Sep 28, 2023

Zacqary commented Sep 28, 2023

View reviewed changes

kibanamachine and others added 11 commits September 28, 2023 23:02

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

177f954

…fix'

Fix jest

f793562

Fix typecheck

1d6ad2e

Fix alert table test

b4c691c

Add permissions check for untracked action

b4d6e09

Merge remote-tracking branch 'origin/bulkaction-164059' into bulkacti…

652b96a

…on-164059

Fix typecheck

596f24a

Add test for bulkUntrackAlerts method

bb39962

Remove duplicate bulk untrack hook

1664888

Move setAlertsToUntracked test to own file

834c4cb

Add call snapshots to tests

8391391

Zacqary marked this pull request as ready for review September 29, 2023 23:06

Zacqary requested a review from a team as a code owner September 29, 2023 23:06

Zacqary requested a review from a team September 29, 2023 23:06

Fix i18n

14a4fa2

maryam-saeidi requested changes Oct 2, 2023

View reviewed changes

ymao1 reviewed Oct 2, 2023

View reviewed changes

x-pack/plugins/alerting/server/alerts_service/lib/set_alerts_to_untracked.ts Outdated Show resolved Hide resolved

Zacqary added 2 commits October 2, 2023 10:59

Check for empty ruleIds and uuids in setAlertsToUntracked

32c5207

Add untrack call to delete rule api

f65925d

Merge branch 'bulkaction-164059' of github.com:Zacqary/kibana into bu…

ac63b3e

…lkaction-164059

XavierM reviewed Oct 3, 2023

View reviewed changes

...ugins/triggers_actions_ui/public/application/sections/alerts_table/hooks/use_bulk_actions.ts Show resolved Hide resolved

XavierM reviewed Oct 3, 2023

View reviewed changes

ymao1 reviewed Oct 3, 2023

View reviewed changes

fix FTR

740a8b6

XavierM requested a review from a team October 3, 2023 19:13

botelastic bot added the Team:APM - DEPRECATED Use Team:obs-ux-infra_services. label Oct 3, 2023

Zacqary added 5 commits October 3, 2023 14:15

Fix untracking rules from bulk delete API

c0728bb

Update permissions

36780c3

Update permissions

31d27b2

Merge branch 'bulkaction-164059' of https://github.com/Zacqary/kibana …

50cb9da

…into bulkaction-164059

Add error handling for bulk updater

171b0a6

ymao1 reviewed Oct 3, 2023

View reviewed changes

XavierM approved these changes Oct 3, 2023

View reviewed changes

Zacqary and others added 5 commits October 3, 2023 15:07

Add bulkUpdateState test

cd98921

Merge branch 'main' into bulkaction-164059

2c6d1f2

Add ensureAuthorized tests for setAlertsToUntracked

be2de2b

Merge branch 'bulkaction-164059' of https://github.com/Zacqary/kibana …

e62a1f1

…into bulkaction-164059

rules/_bulk_untrack -> alerts/_bulk_untrack

2a7b1a9

XavierM enabled auto-merge (squash) October 3, 2023 20:26

achyutjhunjhunwala approved these changes Oct 3, 2023

View reviewed changes

Zacqary and others added 2 commits October 3, 2023 17:02

Add conflicts: proceed to failing test

25114b3

add conflicts: "proceed"

b85edd2

XavierM merged commit 5b1ae66 into elastic:main Oct 4, 2023

kibanamachine added the backport:skip This PR does not require backporting label Oct 4, 2023

mikecote mentioned this pull request Nov 22, 2024

Bug Report: Alert behaviour after deleting its rule #152123

Closed

	path: `${INTERNAL_BASE_ALERTING_API_PATH}/rules/_bulk_untrack`,
	path: `${INTERNAL_BASE_ALERTING_API_PATH}/alerts/_bulk_untrack`,

Conversation

Zacqary commented Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why omit rule ID from this API?

Checklist

Uh oh!

ghost commented Sep 28, 2023

🤖 GitHub comments

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 29, 2023

Uh oh!

maryam-saeidi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Zacqary commented Oct 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ymao1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elasticmachine commented Oct 3, 2023

Uh oh!

ymao1 left a comment

Choose a reason for hiding this comment

Uh oh!

achyutjhunjhunwala left a comment

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Oct 4, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Zacqary commented Sep 28, 2023 •

edited

Loading

maryam-saeidi left a comment •

edited

Loading

Zacqary commented Oct 2, 2023 •

edited

Loading