[Actions] Notify only on action group change #82969

ymao1 · 2020-11-09T15:53:09Z

Resolve #50077

Summary

Added dropdown for specifying notification interval

Opening the create alert flyout:

Dropdown selections:

Custom throttle schedule option exposes the throttle schedule:

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
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

This reverts commit e9f2cd0.

* master: (127 commits) [ILM] Fix breadcrumbs (elastic#82594) [UX]Swap env filter with percentile (elastic#82246) Add platform's missing READMEs (elastic#82268) [Discover] Adding uiMetric to track Visualize link click (elastic#82344) [Search] Add used index pattern name to the search agg error field (elastic#82604) improve client-side SO client get pooling (elastic#82603) [Security Solution] Unskips Overview tests (elastic#82459) Embeddables/migrations (elastic#82296) [Enterprise Search] Refactor product server route registrations to their own files/folders (elastic#82663) Moving reinstall function outside of promise.all (elastic#82672) Load choropleth layer correctly (elastic#82628) Master backport elastic#81233 (elastic#82642) [Fleet] Allow snake cased Kibana assets (elastic#77515) Reduce saved objects authorization checks (elastic#82204) [data.search] Add request handler context and asScoped pattern (elastic#80775) [ML] Fixes formatting of fields in index data visualizer (elastic#82593) Usage collector readme (elastic#82548) [Lens] Visualization validation and better error messages (elastic#81439) [ML] Add annotation markers to time series brush area to indicate annotations exist outside of selected range (elastic#81490) chore(NA): install microdnf in UBI docker build only (elastic#82611) ...

* master: Revert "[Fleet] Allow snake cased Kibana assets (elastic#77515)" (elastic#82706)

…ing/notify-on-action-group-change

ymao1 · 2020-12-10T16:56:46Z

My only question is this - we're changing the default selected Notify behavior.
Is that on purpose?

Yes, it's something that we discussed during the 11/24 Design-Dev meeting

pmuellr

LGTM; my main concern is why we allow null to be used for the new notifyWhen field, when it doesn't seem like that's needed. Making it optional on the endpoints - alertsClient and routes, for create and update - seems reasonable to me though. I may have missed why we have to do this though ...

x-pack/plugins/alerts/common/alert_notify_when_type.ts

pmuellr · 2020-12-10T16:09:21Z

x-pack/plugins/alerts/common/alert.ts

@@ -68,6 +69,7 @@ export interface Alert {
  apiKey: string | null;
  apiKeyOwner: string | null;
  throttle: string | null;
+  notifyWhen: AlertNotifyWhenType | null;


I see we are setting a value for this in the migration, so in theory I don't think we have to allow a null value. I think. We do however have some folks internally who do upgrade deployments mid-version (security and o11y test deployments), and having a null value would be useful for them, since the migrations don't actually run in those cases. But I kinda hate to have to allow null for only that reason - if that is the only reason. @mikecote ?

x-pack/test/alerting_api_integration/spaces_only/tests/alerting/notify_when.ts

pmuellr · 2020-12-10T16:52:19Z

x-pack/plugins/alerts/server/alerts_client/alerts_client.ts

@@ -157,6 +158,7 @@ interface UpdateOptions {
    actions: NormalizedAlertAction[];
    params: Record<string, unknown>;
    throttle: string | null;
+    notifyWhen: AlertNotifyWhenType | null;


I don't think we need to accept a null here, but for backward compatibility we could make this property optional, and then calculate the value with getAlertNotifyWhenType(). The same is true with the create call, but the typing on that is a little more complicated ...

pmuellr · 2020-12-10T16:59:21Z

x-pack/plugins/alerts/common/alert_notify_when_type.ts

+  'onActionGroupChange',
+  'onActiveAlert',
+  'onThrottleInterval',
+] as const;


@gmmorris noted the needed as const on here - we are using this pattern in one other place, and had the same issue which Gidi also caught when it was introduced. I wonder whether we should stop using this pattern, despite it being soooo useful.

pmuellr · 2020-12-10T17:02:09Z

x-pack/plugins/alerts/server/routes/create.ts

@@ -38,6 +38,7 @@ export const bodySchema = schema.object({
    }),
    { defaultValue: [] }
  ),
+  notifyWhen: schema.nullable(schema.string({ validate: validateNotifyWhenType })),


per previous comment about making this property optional vs nullable, if we go that route, we can change this to schema.maybe().

ymao1 · 2020-12-10T17:42:00Z

LGTM; my main concern is why we allow null to be used for the new notifyWhen field, when it doesn't seem like that's needed. Making it optional on the endpoints - alertsClient and routes, for create and update - seems reasonable to me though. I may have missed why we have to do this though ...

My reasoning for not making the field optional was that I consider it a field that should be set for the Alert. If we had this selection to begin with, it would always be set. It's only because we don't want this to be a breaking change that we're allowing null for backwards compatibility. Does that make sense? Making it optional would certainly lead to fewer files changed with this PR though :)

ymao1 · 2020-12-10T18:03:26Z

Changes look good! One small nit would be to change the default on the throttle to be (at least) 1 hour, instead of 1 minute. I'm guessing that if I change to set custom interval, then I will be wanting to set something less frequent than the other options. Curious to get others thoughts on what a good default time would be though. 1 hour seemed reasonable but maybe 1 day is also an option.

@mdefazio Updated to default to 1h

mdefazio

Thanks for make the edits!

pmuellr · 2020-12-10T18:54:34Z

My reasoning for not making the field optional was that I consider it a field that should be set for the Alert. If we had this selection to begin with, it would always be set. It's only because we don't want this to be a breaking change that we're allowing null for backwards compatibility. Does that make sense? Making it optional would certainly lead to fewer files changed with this PR though :)

We have the migration for backwards compatibility with the saved objects. Making the property optional on create/update APIs (client and HTTP) is also backwardly compatible with existing API users. And that works today, eg with kbn-alert, since the property is schema'd to schema.nullable() which accepts null or undefined (and thus "optional"). But I think we could constrain that from null | undefined to just undefined / optional ?.

Are there other cases I'm not thinking of?

mikecote · 2020-12-10T19:35:27Z

x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_notify_when.tsx

+    <>
+      <FormattedMessage
+        id="xpack.triggersActionsUI.sections.alertForm.renotifyFieldLabel"
+        defaultMessage="Notify every"


I saw in @mdefazio's screenshots that the "Notify every" label changed to "Action frequency" (I haven't followed the PR to see if this was discussed already).

To me, it seems to read better with the new label: Action frequency: Run only...

I don't want this question to block the PR so this can be a follow up issue / change if it wasn't previously discussed.

kibanamachine · 2020-12-10T20:13:02Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 46e8025

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`alerts`	23	24	+1
`triggersActionsUi`	297	298	+1
total			+2

Async chunks

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

id	before	after	diff
`triggersActionsUi`	1.5MB	1.5MB	+7.2KB

Distributable file count

id	before	after	diff
`default`	47010	47772	+762

Page load bundle

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

id	before	after	diff
`alerts`	67.9KB	68.5KB	+657.0B
`triggersActionsUi`	161.7KB	161.7KB	-2.0B
total			+655.0B

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/development-plugin-saved-objects.html#_mappings

id	before	after	diff
`alert`	31	32	+1

History

💔 Build #93603 failed dd3b29a
💔 Build #93570 failed 6765bab
💚 Build #93562 succeeded b2f6610
💚 Build #93114 succeeded 3b1813a
💚 Build #92975 succeeded 1adf049

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

dhurley14

LGTM! Tested locally with a rule + slack connector and everything was firing correctly.

* plugged Task Manager lifecycle into status reactively * fixed tests * Revert "fixed tests" This reverts commit e9f2cd0. * made action group fields optional * revert deletion * again * extracted action type for mto its own component * extracted more sections of the action form to their own components * updated icon * added docs * fixed always firing alert * fixed export of components * fixed react warning * Adding flag for notifying on state change * Updating logic in task runner * Starting to update tests * Adding tests * Fixing types check * Tests and types * Tests * Tests * Tests * Tests * Tests * Renaming field to a more descriptive name. Adding migrations * Renaming field to a more descriptive name. Adding migrations * Fixing tests * Type check and tests * Moving schedule and notify interval to bottom of flyout. Implementing dropdown from mockup in new component * Changing boolean flag to enum type and updating in triggers_actions_ui * Changing boolean flag to enum type and updating in alerts plugin * Fixing types check * Fixing monitoring jest tests * Changing last references to old variable names * Moving form inputs back to the top * Renaming to alert_notify_when * Updating functional tests * Adding new functional test for notifyWhen onActionGroupChange * Updating wording * Incorporating action subgroups into logic * PR fixes * Updating functional test * Fixing types check * Changing default throttle interval to hour * Fixing types check Co-authored-by: Gidi Meir Morris <github@gidi.io>

* plugged Task Manager lifecycle into status reactively * fixed tests * Revert "fixed tests" This reverts commit e9f2cd0. * made action group fields optional * revert deletion * again * extracted action type for mto its own component * extracted more sections of the action form to their own components * updated icon * added docs * fixed always firing alert * fixed export of components * fixed react warning * Adding flag for notifying on state change * Updating logic in task runner * Starting to update tests * Adding tests * Fixing types check * Tests and types * Tests * Tests * Tests * Tests * Tests * Renaming field to a more descriptive name. Adding migrations * Renaming field to a more descriptive name. Adding migrations * Fixing tests * Type check and tests * Moving schedule and notify interval to bottom of flyout. Implementing dropdown from mockup in new component * Changing boolean flag to enum type and updating in triggers_actions_ui * Changing boolean flag to enum type and updating in alerts plugin * Fixing types check * Fixing monitoring jest tests * Changing last references to old variable names * Moving form inputs back to the top * Renaming to alert_notify_when * Updating functional tests * Adding new functional test for notifyWhen onActionGroupChange * Updating wording * Incorporating action subgroups into logic * PR fixes * Updating functional test * Fixing types check * Changing default throttle interval to hour * Fixing types check Co-authored-by: Gidi Meir Morris <github@gidi.io> Co-authored-by: Gidi Meir Morris <github@gidi.io>

…t and side car notifications (Part 1) (#109722) ## Summary Removes the "side car" actions object and side car notification (Part 1). Part 1 makes it so that newly created rules and editing existing rules will update them to using the new side car notifications. Part 2 in a follow up PR will be the migrations to move the existing data. The saved object side we are removing usages of is: ``` siem-detection-engine-rule-actions ``` The alerting side car notification system we are removing is: ``` siem.notifications ``` * Removes the notification files and types * Adds transform to and from alerting concepts of `notityWhen` and our `throttle` * Adds unit tests for utilities and pure functions created * Updates unit tests to have more needed jest mock * Adds business rules and logic for the different states of `notifyWhen`, and `throttle` on each of the REST routes to determine when we should `muteAll` vs. not muting using secondary API call from client alerting * Adds e2e tests for the throttle conditions and how they are to interact with the kibana-alerting `throttle` and `notifyWhen` A behavioral change under the hood is that we now support the state changes of `muteAll` from the UI/UX of [stack management](https://www.elastic.co/guide/en/kibana/master/create-and-manage-rules.html#controlling-rules). Whenever the `security_solution` ["Perform no actions"](https://www.elastic.co/guide/en/security/current/rules-api-create.html ) is selected we do a `muteAll`. However, we do not change the state if all individual actions are muted within the rule. Instead we only maintain the state of `muteAll`: <img width="2299" alt="ui_state_change" src="https://user-images.githubusercontent.com/1151048/130823045-48a9f34b-db23-44e3-b9ed-cbbb57edc3d6.png"> <img width="1163" alt="no_actions_state_change" src="https://user-images.githubusercontent.com/1151048/130823056-3f8953fa-9433-4973-a2d3-6e11263b9619.png"> Ref: * Issue and PR where notifyWhen was added to kibna-alerting * #82969 * #50077 ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

…t and side car notifications (Part 1) (elastic#109722) ## Summary Removes the "side car" actions object and side car notification (Part 1). Part 1 makes it so that newly created rules and editing existing rules will update them to using the new side car notifications. Part 2 in a follow up PR will be the migrations to move the existing data. The saved object side we are removing usages of is: ``` siem-detection-engine-rule-actions ``` The alerting side car notification system we are removing is: ``` siem.notifications ``` * Removes the notification files and types * Adds transform to and from alerting concepts of `notityWhen` and our `throttle` * Adds unit tests for utilities and pure functions created * Updates unit tests to have more needed jest mock * Adds business rules and logic for the different states of `notifyWhen`, and `throttle` on each of the REST routes to determine when we should `muteAll` vs. not muting using secondary API call from client alerting * Adds e2e tests for the throttle conditions and how they are to interact with the kibana-alerting `throttle` and `notifyWhen` A behavioral change under the hood is that we now support the state changes of `muteAll` from the UI/UX of [stack management](https://www.elastic.co/guide/en/kibana/master/create-and-manage-rules.html#controlling-rules). Whenever the `security_solution` ["Perform no actions"](https://www.elastic.co/guide/en/security/current/rules-api-create.html ) is selected we do a `muteAll`. However, we do not change the state if all individual actions are muted within the rule. Instead we only maintain the state of `muteAll`: <img width="2299" alt="ui_state_change" src="https://user-images.githubusercontent.com/1151048/130823045-48a9f34b-db23-44e3-b9ed-cbbb57edc3d6.png"> <img width="1163" alt="no_actions_state_change" src="https://user-images.githubusercontent.com/1151048/130823056-3f8953fa-9433-4973-a2d3-6e11263b9619.png"> Ref: * Issue and PR where notifyWhen was added to kibna-alerting * elastic#82969 * elastic#50077 ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

…t and side car notifications (Part 1) (#109722) (#110305) ## Summary Removes the "side car" actions object and side car notification (Part 1). Part 1 makes it so that newly created rules and editing existing rules will update them to using the new side car notifications. Part 2 in a follow up PR will be the migrations to move the existing data. The saved object side we are removing usages of is: ``` siem-detection-engine-rule-actions ``` The alerting side car notification system we are removing is: ``` siem.notifications ``` * Removes the notification files and types * Adds transform to and from alerting concepts of `notityWhen` and our `throttle` * Adds unit tests for utilities and pure functions created * Updates unit tests to have more needed jest mock * Adds business rules and logic for the different states of `notifyWhen`, and `throttle` on each of the REST routes to determine when we should `muteAll` vs. not muting using secondary API call from client alerting * Adds e2e tests for the throttle conditions and how they are to interact with the kibana-alerting `throttle` and `notifyWhen` A behavioral change under the hood is that we now support the state changes of `muteAll` from the UI/UX of [stack management](https://www.elastic.co/guide/en/kibana/master/create-and-manage-rules.html#controlling-rules). Whenever the `security_solution` ["Perform no actions"](https://www.elastic.co/guide/en/security/current/rules-api-create.html ) is selected we do a `muteAll`. However, we do not change the state if all individual actions are muted within the rule. Instead we only maintain the state of `muteAll`: <img width="2299" alt="ui_state_change" src="https://user-images.githubusercontent.com/1151048/130823045-48a9f34b-db23-44e3-b9ed-cbbb57edc3d6.png"> <img width="1163" alt="no_actions_state_change" src="https://user-images.githubusercontent.com/1151048/130823056-3f8953fa-9433-4973-a2d3-6e11263b9619.png"> Ref: * Issue and PR where notifyWhen was added to kibna-alerting * #82969 * #50077 ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios Co-authored-by: Frank Hassanabad <frank.hassanabad@elastic.co>

gmmorris and others added 28 commits November 3, 2020 18:47

plugged Task Manager lifecycle into status reactively

01c7c7d

fixed tests

e9f2cd0

Revert "fixed tests"

42395e9

This reverts commit e9f2cd0.

made action group fields optional

6d00ce8

revert deletion

89deeca

again

73fe0a8

Merge branch 'master' into alerts/assign-action-groups

401d99f

* master: Revert "[Fleet] Allow snake cased Kibana assets (elastic#77515)" (elastic#82706)

extracted action type for mto its own component

78873a1

extracted more sections of the action form to their own components

85a1068

updated icon

44eb670

added docs

4b36391

fixed always firing alert

afc6e81

fixed export of components

38fb659

fixed react warning

6fe4ae4

Merge branch 'master' of https://github.com/elastic/kibana into pr/82472

f68763e

Adding flag for notifying on state change

9d1f594

Updating logic in task runner

9005c1e

Starting to update tests

1422911

Merge branch 'master' of https://github.com/elastic/kibana into alert…

7b7f3c8

…ing/notify-on-action-group-change

Merge branch 'master' of https://github.com/elastic/kibana into alert…

a0e3ac5

…ing/notify-on-action-group-change

Adding tests

d37288c

Merging in master

e0fa427

Fixing types check

22b7a00

Tests and types

a075514

Tests

8311ab8

Tests

e21ea91

Tests

5e0ee21

ymao1 added Feature:Actions release_note:skip Skip the PR/issue when compiling release notes labels Nov 9, 2020

PR fixes

6765bab

pmuellr approved these changes Dec 10, 2020

View reviewed changes

ymao1 added 3 commits December 10, 2020 12:43

Updating functional test

86deabd

Fixing types check

85ed885

Changing default throttle interval to hour

dd3b29a

mdefazio approved these changes Dec 10, 2020

View reviewed changes

Fixing types check

46e8025

mikecote reviewed Dec 10, 2020

View reviewed changes

ymao1 mentioned this pull request Dec 10, 2020

[8.0 only] Alert notifyWhen field should not be nullable #85639

Closed

dhurley14 approved these changes Dec 10, 2020

View reviewed changes

ymao1 merged commit ab08264 into elastic:master Dec 10, 2020

ymao1 mentioned this pull request Dec 10, 2020

[7.x] [Actions] Notify only on action group change (#82969) #85642

Merged

ymao1 mentioned this pull request Dec 10, 2020

[Alerting] Improve wording on "Notify every" form input #85644

Closed

spong mentioned this pull request Dec 11, 2020

[Detection Rules] Add 7.11 rules #85506

Merged

1 task

mikecote added release_note:enhancement needs_docs and removed release_note:skip Skip the PR/issue when compiling release notes labels Dec 16, 2020

ymao1 deleted the alerting/notify-on-action-group-change branch February 4, 2021 15:24

ymao1 mentioned this pull request Feb 19, 2021

[Alerting][Docs] Updating docs for notify only on action group change #92084

Merged

FrankHassanabad mentioned this pull request Aug 25, 2021

[Security Solutions][Detection Engine] Removes side car actions object and side car notifications (Part 1) #109722

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Actions] Notify only on action group change #82969

[Actions] Notify only on action group change #82969

ymao1 commented Nov 9, 2020 •

edited

Loading

ymao1 commented Dec 10, 2020

pmuellr left a comment

pmuellr Dec 10, 2020

pmuellr Dec 10, 2020

pmuellr Dec 10, 2020

pmuellr Dec 10, 2020

ymao1 commented Dec 10, 2020

ymao1 commented Dec 10, 2020

mdefazio left a comment

pmuellr commented Dec 10, 2020

mikecote Dec 10, 2020

kibanamachine commented Dec 10, 2020

dhurley14 left a comment

[Actions] Notify only on action group change #82969

[Actions] Notify only on action group change #82969

Conversation

ymao1 commented Nov 9, 2020 • edited Loading

Summary

Checklist

ymao1 commented Dec 10, 2020

pmuellr left a comment

Choose a reason for hiding this comment

pmuellr Dec 10, 2020

Choose a reason for hiding this comment

pmuellr Dec 10, 2020

Choose a reason for hiding this comment

pmuellr Dec 10, 2020

Choose a reason for hiding this comment

pmuellr Dec 10, 2020

Choose a reason for hiding this comment

ymao1 commented Dec 10, 2020

ymao1 commented Dec 10, 2020

mdefazio left a comment

Choose a reason for hiding this comment

pmuellr commented Dec 10, 2020

mikecote Dec 10, 2020

Choose a reason for hiding this comment

kibanamachine commented Dec 10, 2020

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Distributable file count

Page load bundle

Saved Objects .kibana field count

History

dhurley14 left a comment

Choose a reason for hiding this comment

ymao1 commented Nov 9, 2020 •

edited

Loading