[Actions] Connector Adapters MVP by cnasikas · Pull Request #166101 · elastic/kibana

cnasikas · 2023-09-08T16:17:53Z

Summary

This PR implements Connector Adapters. Integrations tests will follow on this PR #161726 as we cannot create system actions through the API at the moment.

Issue: #160367
POC: #159866

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2023-09-08T16:18:00Z

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

cnasikas · 2023-09-08T16:23:04Z

x-pack/plugins/alerting/common/rule.ts

  params: RuleActionParams;
  frequency?: RuleActionFrequency;
  alertsFilter?: AlertsFilter;
+  type?: typeof RuleActionTypes.DEFAULT;


The system action will be

export interface RuleSystemAction { uuid: string; id: string; actionTypeId: string; params: RuleActionParams; type: typeof RuleActionTypes.SYSTEM; }

cnasikas · 2023-09-08T16:24:20Z

x-pack/plugins/alerting/server/connector_adapters/types.ts

+
+type ActionTypeParams = Record<string, unknown>;
+
+type Rule = Pick<SanitizedRule<RuleTypeParams>, 'id' | 'name' | 'tags'>;


For the MVP this should be enough. If a connector adapter needs more attributes from the rule we can extend it in the future.

question: do we need to extend or can we pass-through SanitizedRule<RuleTypeParams>?

cnasikas · 2023-09-08T16:27:00Z

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

-          params: { spaceId, alertId: ruleId },
-        },
-      } = this;
+    if (executables.length === 0) {


Removed the outer if and returned early. Better to view it with "Hide white spaces" enabled.

cnasikas · 2023-09-08T16:27:21Z

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

+      ruleRunMetricsStore.incrementNumberOfTriggeredActionsByConnectorType(actionTypeId);
+
+      if (summarizedAlerts && !isSystemAction(action)) {
+        const { actionsToEnqueueForExecution, actionsToLog } = await this.runSummarizedAction({


Small refactor. I create one run*Action method for each possible execution and I move the logic inside the dedicated functions. The logic is the same as before.

cnasikas · 2023-09-08T16:29:11Z

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

I moved the logic of running each action in a separate function. Each runner returns the actions to be bulk executed and the logged messages. Also, I moved the logic of bulk executing and logging into separate functions.

ymao1

Left a few questions. Also wondering if we should be merging into a feature branch instead of main?

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

x-pack/plugins/alerting/server/connector_adapters/connector_adapter_registry.ts

cnasikas · 2023-09-11T13:19:13Z

Left a few questions. Also wondering if we should be merging into a feature branch instead of main?

If fine with both. Whatever you think is best 🙂

ymao1 · 2023-09-12T11:46:43Z

Left a few questions. Also wondering if we should be merging into a feature branch instead of main?

If fine with both. Whatever you think is best 🙂

I think since we're treating main as prod now, this probably makes more sense to merge into a feature branch and only merge the feature branch when it is feature complete.

ymao1

LGTM. Mostly reviewed code changes in alerting execution handler.

kibana-ci · 2023-09-12T13:07:59Z

💚 Build Succeeded

Buildkite Build
Commit: 36c1602

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`	762	766	+4
`triggersActionsUi`	550	551	+1
total			+5

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`alerting`	49	50	+1

Page load bundle

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

id	before	after	diff
`alerting`	21.7KB	21.8KB	+99.0B

Unknown metric groups

API count

id	before	after	diff
`alerting`	793	798	+5
`triggersActionsUi`	576	577	+1
total			+6

History

💔 Build #157726 failed 753eae3

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

cc @cnasikas

mikecote

I managed to go through this PR as discussed. Below are the questions I had and nitpicks.

mikecote · 2023-09-20T14:48:40Z

x-pack/plugins/alerting/common/rule.ts

+export const RuleActionTypes = {
+  DEFAULT: 'default' as const,
+  SYSTEM: 'system' as const,
+} as const;


question: Do we have a follow up issue / task for the feature branch to split DEFAULT into two? (something that associates with each alert and something that associates with summaries).

mikecote · 2023-09-20T14:51:57Z

x-pack/plugins/alerting/server/connector_adapters/types.ts

+
+type ActionTypeParams = Record<string, unknown>;
+
+type Rule = Pick<SanitizedRule<RuleTypeParams>, 'id' | 'name' | 'tags'>;


question: do we need to extend or can we pass-through SanitizedRule<RuleTypeParams>?

mikecote · 2023-09-20T15:02:00Z

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


 export const generateActionHash = (action?: RuleAction) => {
+  if (action != null && isSystemAction(action)) {
+    return `system-action:${action?.actionTypeId || 'no-action-type-id'}:summary`;


nit: No need for action?.actionTypeId || 'no-action-type-id' as it can be action.actionTypeId.

mikecote · 2023-09-20T15:08:17Z

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

          this.logger.warn(
-            `Rule "${this.taskInstance.params.alertId}" skipped scheduling action "${action.id}" because it is disabled`
+            `Rule "${this.taskInstance.params.alertId}" skipped scheduling system action "${action.id}" because no connector adapter is configured`
          );


question: should we throw an error instead similar to when a connector doesn't exist?

cnasikas added 5 commits September 8, 2023 18:16

Create adapter connector registry

e59eab0

Introduce rule action types

aca0ac7

Execute system actions

620751d

Fix types

2e7a0ec

Fix tests

5195504

cnasikas added release_note:skip Skip the PR/issue when compiling release notes Feature:Actions Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// Feature:Actions/Framework Issues related to the Actions Framework v8.11.0 labels Sep 8, 2023

cnasikas requested a review from a team as a code owner September 8, 2023 16:17

cnasikas self-assigned this Sep 8, 2023

cnasikas commented Sep 8, 2023

View reviewed changes

Remove unecessary files

753eae3

cnasikas commented Sep 8, 2023

View reviewed changes

ymao1 reviewed Sep 11, 2023

View reviewed changes

x-pack/plugins/alerting/server/task_runner/execution_handler.ts Show resolved Hide resolved

x-pack/plugins/alerting/server/connector_adapters/connector_adapter_registry.ts Show resolved Hide resolved

cnasikas mentioned this pull request Sep 12, 2023

[Actions] System actions #160367

Closed

15 tasks

cnasikas changed the base branch from main to system_actions_mvp September 12, 2023 12:04

Merge branch 'system_actions_mvp' into connector_adapters_mvp

36c1602

ymao1 approved these changes Sep 12, 2023

View reviewed changes

cnasikas merged commit 29973e2 into elastic:system_actions_mvp Sep 12, 2023

cnasikas deleted the connector_adapters_mvp branch September 12, 2023 14:07

mikecote reviewed Sep 20, 2023

View reviewed changes


		type ActionTypeParams = Record<string, unknown>;

		type Rule = Pick<SanitizedRule<RuleTypeParams>, 'id' \| 'name' \| 'tags'>;

Conversation

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

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Sep 8, 2023

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!

cnasikas Sep 8, 2023 • 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!

cnasikas Sep 8, 2023 • 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!

ymao1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

ymao1 commented Sep 12, 2023

Uh oh!

ymao1 left a comment

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Sep 12, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Public APIs missing exports

Page load bundle

API count

History

Uh oh!

mikecote left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cnasikas commented Sep 8, 2023 •

edited

Loading

cnasikas Sep 8, 2023 •

edited

Loading

cnasikas Sep 8, 2023 •

edited

Loading

cnasikas commented Sep 11, 2023 •

edited

Loading