Add triggered actions list in task state #146183

ersin-erdal · 2022-11-23T16:27:28Z

This PR adds actions.date field to the alertInstances in the Task context, and TaskRunner uses it (and the new notifyWhen and throttle fields in the actions too) to decide if an action is throttled or not.

To verify:
Create a rule with an action and expect it to run as usual,
Then modify the action data in backend to mimic an action that has the new notifyWhen and throttle fields and expect it to run as expected as well.

I usually replace the following code with the below code snippet to mimic an action with the new fields:

kibana/x-pack/plugins/alerting/server/routes/create_rule.ts

Line 129 in 8f790d7

data: rewriteBodyReq({

data: rewriteBodyReq({
   ...rule,
   actions: [
            {
                   group: rule.actions[0].group,
                   id: rule.actions[0].id,
                   params: rule.actions[0].params,
                   frequency: {
                         summary: false,
                          notifyWhen: 'onThrottleInterval',
                          throttle: '5m',
                   },
              },
   ],
   notify_when: undefined,
   throttle: undefined,
}),

… 143376-alerts-summary-exec # Conflicts: # x-pack/plugins/alerting/server/task_runner/execution_handler.test.ts # x-pack/plugins/alerting/server/task_runner/execution_handler.ts # x-pack/plugins/alerting/server/task_runner/task_runner.ts

ersin-erdal · 2022-12-05T16:45:31Z

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

@@ -470,8 +470,7 @@ export class TaskRunner<
        );
        this.countUsageOfActionExecutionAfterRuleCancellation();
      } else {
-        await executionHandler.run(activeAlerts);
-        await executionHandler.run(currentRecoveredAlerts, true);
+        await executionHandler.run({ ...activeAlerts, ...currentRecoveredAlerts });


I merged these to avoid triggering a summary action twice

pmuellr · 2022-12-05T17:47:19Z

I'm curious about this:

kibana/x-pack/plugins/alerting/common/alert_instance.ts

Lines 15 to 27 in 89baad3

    
           export const actionsSchema = t.record(t.string, actionSchema); 
        
           export type AlertActions = t.TypeOf<typeof actionsSchema>; 
        
           const lastScheduledActionsSchema = t.intersection([ 
        
             t.partial({ 
        
               subgroup: t.string, 
        
             }), 
        
             t.type({ 
        
               group: t.string, 
        
               date: DateFromString, 
        
             }), 
        
             t.partial({ actions: actionsSchema }), 
        
           ]);

It looks to me like actions is an object of where the keys are actionId (connector id), and the value is {date: ...}. But it's possible to use the same connector more than once within an action group. Two server log actions, for example. So it doesn't actually look like this is specific enough - it really needs to identify a SPECIFIC action, not any action that uses that connector id.

ersin-erdal · 2022-12-05T18:23:34Z

I'm curious about this:

kibana/x-pack/plugins/alerting/common/alert_instance.ts

Lines 15 to 27 in 89baad3

export const actionsSchema = t.record(t.string, actionSchema);

export type AlertActions = t.TypeOf<typeof actionsSchema>;

const lastScheduledActionsSchema = t.intersection([

t.partial({

subgroup: t.string,

}),

t.type({

group: t.string,

date: DateFromString,

}),

t.partial({ actions: actionsSchema }),

]);

It looks to me like actions is an object of where the keys are actionId (connector id), and the value is {date: ...}. But it's possible to use the same connector more than once within an action group. Two server log actions, for example. So it doesn't actually look like this is specific enough - it really needs to identify a SPECIFIC action, not any action that uses that connector id.

Ok i thought actionId is not connectorId, it's a uuid that we generate and it sticks with the action. If it's not, i should replace it with the actionREf.

… 143376-alerts-summary-exec

This reverts commit 77b51b0.

# Conflicts: # x-pack/plugins/alerting/server/rules_client/rules_client.ts

ersin-erdal · 2022-12-07T02:17:09Z

x-pack/plugins/alerting/server/routes/create_rule.ts

@@ -33,13 +33,13 @@ export const bodySchema = schema.object({
  enabled: schema.boolean({ defaultValue: true }),
  consumer: schema.string(),
  tags: schema.arrayOf(schema.string(), { defaultValue: [] }),
-  throttle: schema.nullable(schema.maybe(schema.string({ validate: validateDurationSchema }))),
+  throttle: schema.maybe(schema.nullable(schema.string({ validate: validateDurationSchema }))),


This order matters. schema.nullable() converts undefined to null

mikecote

Finished reviewing the code, will do some testing tomorrow 👍

mikecote · 2022-12-07T14:26:49Z

x-pack/plugins/alerting/server/rules_client/lib/validate_actions.ts

-  if (hasNotifyWhen && hasThrottle) usesRuleLevelFreqParams = true;
+  // I removed the below ` && hasThrottle` check temporarily.
+  // Currently the UI sends "throttle" as undefined but schema converts it to null, so they never become both undefined
+  // I changed the schema too, but as the UI (and tests) sends "notifyWhen" as string and "throttle" as undefined, they never become both defined.
+  // We should add it back when the UI is changed (https://github.com/elastic/kibana/issues/143369)
+  if (hasNotifyWhen) usesRuleLevelFreqParams = true;


I'm not sure I follow why this needs to be done temporarily until the UI changes. Is there an API breaking change introduced in the summarization process?

No there isn't any breaking change. But it's a bit confusing, took me some time to figure out as well...

I want to test the new frequency fields by creating a new rule with notifyWhen and throttle in action level.
But as per the above code, both rule level notifyWhen and throttle should be undefined.
As the current schema converts undefined throttle field value to null (schema.nullable -> schema.maybe order) it was impossible, so i changed it.
Then functional tests started to fail because the current UI sends notifyWhen as string and throttle as undefined! So the following if case never returns true if (hasNotifyWhen && hasThrottle) usesRuleLevelFreqParams = true;

Therefore i removed the hasThrottle part...

Actually, as per our design, when there are notifyWhen and throttle on action level, rule level ones should be null, not undefined... but we can tackle that when we implement the UI part of the meta...

mikecote

Tested locally and LGTM!

afharo

Core changes LGTM

kibana-ci · 2022-12-12T14:32:11Z

💚 Build Succeeded

Buildkite Build
Commit: f15a6cb

Metrics [docs]

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
`alerting`	408	411	+3

Page load bundle

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

id	before	after	diff
`alerting`	39.3KB	39.4KB	+118.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/saved-objects-service.html#_mappings

id	before	after	diff
`alert`	95	99	+4

Unknown metric groups

API count

id	before	after	diff
`alerting`	417	420	+3

ESLint disabled in files

id	before	after	diff
`osquery`	1	2	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`fleet`	60	66	+6
`osquery`	109	115	+6
`securitySolution`	445	451	+6
total			+20

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	69	75	+6
`osquery`	110	117	+7
`securitySolution`	521	527	+6
total			+21

History

💚 Build #93267 succeeded 5f6f96c
💔 Build #93200 failed 1829f44
💚 Build #93108 succeeded 10f2aa8
💚 Build #92992 succeeded 7810568
💔 Build #92874 failed 75ee6a6
💔 Build #92823 failed 8095917

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

rudolf · 2022-12-12T14:44:40Z

x-pack/plugins/alerting/server/saved_objects/mappings.ts

@@ -56,6 +56,22 @@ export const alertMappings: SavedObjectsTypeMappingDefinition = {
          enabled: false,
          type: 'object',
        },
+        frequency: {


If we're not querying these fields (index: false) why are we specifying these fields in the mappings?

We use those fields but not query by them.
I can't create a rule that has those fields without adding them in the mapping, or am i wrong?

i get:
mapping set to strict, dynamic introduction of [frequency] within [alert.actions] is not allowed: strict_dynamic_mapping_exception: [strict_dynamic_mapping_exception] Reason: mapping set to strict, dynamic introduction of [frequency] within [alert.actions] is not allowed

if they are not in the mapping.

Fixes: elastic#146049 This PR adds `actions.date` field to the alertInstances in the Task context, and TaskRunner uses it (and the new `notifyWhen` and `throttle` fields in the actions too) to decide if an action is throttled or not.

ersin-erdal added 4 commits November 18, 2022 17:59

WIP

4313fe0

WIP

75e3ba5

Add lastTriggerDate to action

5226374

ersin-erdal added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) release_note:feature Makes this part of the condensed release notes v8.7.0 labels Nov 23, 2022

fix type check

a44fd53

ersin-erdal changed the title ~~143376 alerts summary exec~~ Add lastTriggerDate field to the rule actions Nov 23, 2022

ersin-erdal added 20 commits November 23, 2022 18:55

fix type check

9cdeea7

fix SO hash

6fe76e3

fix integration tests

31737b1

fix RuleActions and Rule types for UI

8cdd598

fix ResolvedRule type for UI

0007b70

make lastTriggerDate optional

51f3df5

getLastTriggerData

860e486

revert integration tests

b3c4ab6

fix functional test

848b0d1

fix functional test

84862f0

fix functional test

689634e

move lastTriggerDate to alertInstances in task object

cd3dd0d

remove updatedActions

25a580f

change SO hash

06ef8f9

revert actionRef

6e8941d

revert actionRef

048033b

revert actionRef

0a92d22

add integration test

dbeb620

change validation

c8252a7

FIx wording

04306e9

ersin-erdal changed the title ~~Add lastTriggerDate field to the rule actions~~ Add triggered actions list in task state Nov 26, 2022

ersin-erdal added 3 commits December 5, 2022 15:52

resolve merge conflicts

7516307

fix alert hash

3f5e9a6

fix merge mistakes

89baad3

ersin-erdal commented Dec 5, 2022

View reviewed changes

ersin-erdal closed this Dec 5, 2022

ersin-erdal reopened this Dec 5, 2022

ersin-erdal and others added 9 commits December 5, 2022 19:24

Merge branch 'main' into 143376-alerts-summary-exec

8095917

use actionRef rather than actionId

77b51b0

Merge remote-tracking branch 'origin/143376-alerts-summary-exec' into…

f03fe0d

… 143376-alerts-summary-exec

new alert hash

75ee6a6

Revert "use actionRef rather than actionId"

7810568

This reverts commit 77b51b0.

use action hash instead of actionId

5da5dca

use action hash instead of actionId

10f2aa8

Merge branch 'main' into 143376-alerts-summary-exec

dfa56cd

# Conflicts: # x-pack/plugins/alerting/server/rules_client/rules_client.ts

resolve merge conflict

1829f44

ersin-erdal commented Dec 7, 2022

View reviewed changes

Merge branch 'main' into 143376-alerts-summary-exec

5f6f96c

mikecote self-requested a review December 7, 2022 14:06

mikecote reviewed Dec 7, 2022

View reviewed changes

mikecote approved these changes Dec 8, 2022

View reviewed changes

Merge branch 'main' into 143376-alerts-summary-exec

f15a6cb

afharo approved these changes Dec 12, 2022

View reviewed changes

ersin-erdal merged commit d152e0a into elastic:main Dec 12, 2022

kibanamachine added the backport:skip This commit does not require backporting label Dec 12, 2022

ersin-erdal deleted the 143376-alerts-summary-exec branch December 12, 2022 14:42

rudolf reviewed Dec 12, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add triggered actions list in task state #146183

Add triggered actions list in task state #146183

ersin-erdal commented Nov 23, 2022 •

edited

ersin-erdal Dec 5, 2022

pmuellr commented Dec 5, 2022

ersin-erdal commented Dec 5, 2022 •

edited

ersin-erdal Dec 7, 2022

mikecote left a comment

mikecote Dec 7, 2022

ersin-erdal Dec 7, 2022 •

edited

mikecote left a comment

afharo left a comment

kibana-ci commented Dec 12, 2022

API count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

rudolf Dec 12, 2022

ersin-erdal Dec 13, 2022

Add triggered actions list in task state #146183

Add triggered actions list in task state #146183

Conversation

ersin-erdal commented Nov 23, 2022 • edited

ersin-erdal Dec 5, 2022

Choose a reason for hiding this comment

pmuellr commented Dec 5, 2022

ersin-erdal commented Dec 5, 2022 • edited

ersin-erdal Dec 7, 2022

Choose a reason for hiding this comment

mikecote left a comment

Choose a reason for hiding this comment

mikecote Dec 7, 2022

Choose a reason for hiding this comment

ersin-erdal Dec 7, 2022 • edited

Choose a reason for hiding this comment

mikecote left a comment

Choose a reason for hiding this comment

afharo left a comment

Choose a reason for hiding this comment

kibana-ci commented Dec 12, 2022

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Page load bundle

Saved Objects .kibana field count

API count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

rudolf Dec 12, 2022

Choose a reason for hiding this comment

ersin-erdal Dec 13, 2022

Choose a reason for hiding this comment

ersin-erdal commented Nov 23, 2022 •

edited

ersin-erdal commented Dec 5, 2022 •

edited

ersin-erdal Dec 7, 2022 •

edited