[Response Ops][Alerting] Update FAAD `AlertsClient` to support AAD payload #158404

ymao1 · 2023-05-24T17:32:52Z

Summary

Updates AlertsClient with create API that allows rule executors to report alerts with AAD payload, along with the values for actionGroup, context and state. This proxies the LegacyAlertsClient to create alerts via the alerts factory but also saves the reported payload
When the alert doc is bulk written at the end of rule execution, the AAD payload (if specified) is included in the alert document
Deprecates the alert factory that is passed into the rule executors, but this PR does not remove or replace usages of the alert factory
Expose AlertsClient services to the rule executors. Note that this PR does not migrate any rule type to use this service.

This PR does not opt any rule types into writing the AAD payload or using the AlertsClient API but updates the AAD functional test to do so. To test it out with the ES query rule type, use the following commit: 1b1e139

Followup issues

This PR does not add a recovery API to the FAAD AlertsClient so alerts reported via the new alerts client currently do not have a way of specifying recovered payload.

To Verify

Verify that rule registry rule types still work as expected
Verify that non rule-registry rule types still work as expected
Check out this commit which onboards the ES query rule type onto FAAD. Create an ES query rule that alerts and then recovers and verify that the alert documents look as expected. Alternatively, you can modify your own rule type to register with FAAD and write alerts and verify that the alert documents look as expected.

Checklist

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

ymao1 · 2023-06-02T16:14:58Z

x-pack/plugins/alerting/server/alerts_client/lib/strip_framework_fields.ts

+import { alertFieldMap } from '@kbn/alerts-as-data-utils';
+import { RuleAlertData } from '../../types';
+
+const allowedFrameworkFields: string[] = [ALERT_REASON];


kibana.alert.reason is defined in the alertFieldMap but the framework does not set it, it is returned by rule types. We may want to set it at a framework level in the future but for now, we add it to this list to allow rule types to override this value.

Is this just for fields defined by the framework but expected to be set by rule - and we don't have any other fields like that yet?

It should probably be a set instead of array, optimize the filter.

@pmuellr Yes, that's correct. There are a few more fields that I'm unsure about: kibana.alert.last_detected and kibana.alert.url come to mind but we can add those as needed I think

Updated to set in this commit: ce316fd

ymao1 · 2023-06-02T16:16:20Z

x-pack/plugins/alerting/server/lib/process_alerts.ts

@@ -124,11 +124,12 @@ function processAlertsHelper<
          // this alert did exist in previous run
          // calculate duration to date for active alerts
          const state = existingAlerts[id].getState();
+          const currentState = activeAlerts[id].getState();


This fixes a bug where an ongoing alert that uses replaceState will have its updated state value overriden.

This reverts commit 1b1e139.

elasticmachine · 2023-06-05T13:08:32Z

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

mikecote

Changes LGTM! Reviewed the code and tested locally with the sample ES Query snippet you shared. I left a bunch of questions but nothing blocking from my perspective 👍

mikecote · 2023-06-06T14:07:14Z

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

+      } catch (err) {
+        this.options.logger.error(
+          `Error writing ${alertsToIndex.length} alerts to ${this.indexTemplateAndPattern.alias} - ${err.message}`
+        );
+      }


question: Do we need to process the bulk response on success as well (when no errors are thrown)? If an error happens for a specific document, it would return as a success and have the error information in the payload (https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-bulk.html#bulk-api-response-body).

Do you think we should retry the individual documents that fail or just log?

I think logging for now is ok and if we think it's worth retrying or partially failing the rule execution, we can create a follow up issue to discuss / implement.

Updated in 633dbb1

x-pack/plugins/alerting/server/alerts_client/lib/build_new_alert.ts

x-pack/plugins/alerting/server/alerts_client/lib/strip_framework_fields.ts

x-pack/plugins/alerting/server/alerts_client/lib/build_ongoing_recovered_alert.ts

…pi-rule-type-payload

ymao1 · 2023-06-06T20:11:04Z

x-pack/plugins/alerting/server/alerts_client/lib/strip_framework_fields.ts

+type AlertRecord = Record<string, AlertTypes>;
+type AlertFields = AlertTypes | AlertRecord;
+
+const removeEmptyObjects = (data: AlertFields): AlertFields => {


@mikecote Do you see any gotchas in this function? Using lodash omit leaves empty objects so we'd be left with fields like

kibana: { alert: { duration: {} } }

if we had to strip kibana.alert.duration.us from the payload (for example), so I added this function but maybe it is overkill?

Function looks good to me. To see if the function is necessary, I just did a check on the behaviours when calling the Elasticsearch index and update APIs to update an existing alert document. In both scenarios with empty objects, the call mutated the alert in the exact way as if it didn't have an empty object, so it seems not doing removeEmptyObjects will work the same.

When creating alerts, the created alert would just contain an empty object but I don't see any issues doing so either. So it seems safe to remove the removeEmptyObjects function and save an processing step.

Removed in b385e5f

…pi-rule-type-payload

pmuellr

LGTM, noted one optimization

pmuellr · 2023-06-05T21:14:59Z

x-pack/plugins/alerting/server/alerts_client/lib/strip_framework_fields.ts

+import { alertFieldMap } from '@kbn/alerts-as-data-utils';
+import { RuleAlertData } from '../../types';
+
+const allowedFrameworkFields: string[] = [ALERT_REASON];


Is this just for fields defined by the framework but expected to be set by rule - and we don't have any other fields like that yet?

It should probably be a set instead of array, optimize the filter.

e40pud

Detection engine changes LGTM

maryam-saeidi

AO changes LGTM!

…pi-rule-type-payload

kibana-ci · 2023-06-07T16:03:17Z

💚 Build Succeeded

Buildkite Build
Commit: c6524fa

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`	595	594	-1

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`	44	45	+1

Unknown metric groups

API count

id	before	after	diff
`alerting`	617	618	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	413	417	+4
total			+6

References to deprecated APIs

id	before	after	diff
`infra`	59	65	+6
`ml`	153	157	+4
`monitoring`	4	5	+1
`observability`	3	5	+2
`ruleRegistry`	0	4	+4
`securitySolution`	603	605	+2
`stackAlerts`	58	64	+6
`synthetics`	42	49	+7
`transform`	29	30	+1
total			+33

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	497	501	+4
total			+6

History

💚 Build #133270 succeeded b385e5f
💔 Build #133101 failed 1621088
💛 Build #132550 was flaky ffb32fb
💚 Build #132278 succeeded d11c667f6871e4e412942915cfc878bd3dca664f
💔 Build #132228 failed 0ef3711d60ac57ffc0319b35b06765b61bca0aeb
💔 Build #132210 failed 704013a0d7cd58dc5edc243606fc5425cab5696a

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

cc @ymao1

ymao1 force-pushed the alerting/faad-api-rule-type-payload branch 2 times, most recently from b133daf to bcf57ab Compare May 24, 2023 19:53

ymao1 changed the title ~~Adding ability to create alert in new alerts client~~ [Response Ops][Alerting] Update FAAD AlertsClient to support AAD payload Jun 1, 2023

ymao1 force-pushed the alerting/faad-api-rule-type-payload branch 3 times, most recently from e31e3b7 to 7045165 Compare June 2, 2023 15:45

ymao1 commented Jun 2, 2023

View reviewed changes

ymao1 force-pushed the alerting/faad-api-rule-type-payload branch 4 times, most recently from 0ef3711 to d11c667 Compare June 2, 2023 19:27

ymao1 self-assigned this Jun 2, 2023

ymao1 added Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Alerting/Alerts-as-Data Issues related to Alerts-as-data and RuleRegistry v8.9.0 labels Jun 2, 2023

ymao1 added 3 commits June 5, 2023 08:03

Updating AlertsClient to accept rule type AAD payload

ce992d2

Onboarding ES query rule type to write AAD

1b1e139

Revert "Onboarding ES query rule type to write AAD"

3b6d2e6

This reverts commit 1b1e139.

ymao1 force-pushed the alerting/faad-api-rule-type-payload branch from d11c667 to 3b6d2e6 Compare June 5, 2023 12:05

Fallback to legacy alerts client if error initializing

ffb32fb

ymao1 marked this pull request as ready for review June 5, 2023 13:08

ymao1 requested review from a team as code owners June 5, 2023 13:08

mikecote self-requested a review June 5, 2023 19:03

mikecote approved these changes Jun 6, 2023

View reviewed changes

ymao1 added 3 commits June 6, 2023 14:30

Merge branch 'main' of github.com:elastic/kibana into alerting/faad-a…

b6d82b5

…pi-rule-type-payload

PR feedback

474f3e9

PR feedback

1621088

ymao1 commented Jun 6, 2023

View reviewed changes

ymao1 added 3 commits June 7, 2023 07:07

Logging individual bulk response errors if they exist

633dbb1

Merge branch 'main' of github.com:elastic/kibana into alerting/faad-a…

1831cb7

…pi-rule-type-payload

Removing unneeded fn

b385e5f

pmuellr approved these changes Jun 7, 2023

View reviewed changes

e40pud approved these changes Jun 7, 2023

View reviewed changes

maryam-saeidi approved these changes Jun 7, 2023

View reviewed changes

ymao1 added 2 commits June 7, 2023 10:57

PR feedback

ce316fd

Merge branch 'main' of github.com:elastic/kibana into alerting/faad-a…

c6524fa

…pi-rule-type-payload

ymao1 merged commit 3aa3f04 into elastic:main Jun 7, 2023

kibanamachine added the backport:skip This commit does not require backporting label Jun 7, 2023

ymao1 deleted the alerting/faad-api-rule-type-payload branch June 7, 2023 16:08

ymao1 mentioned this pull request Jun 7, 2023

[Response Ops][Alerting] Deprecate alertFactory in favor of AlertsClient #156445

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Response Ops][Alerting] Update FAAD `AlertsClient` to support AAD payload #158404

[Response Ops][Alerting] Update FAAD `AlertsClient` to support AAD payload #158404

ymao1 commented May 24, 2023 •

edited

Loading

ymao1 Jun 2, 2023

pmuellr Jun 5, 2023

ymao1 Jun 7, 2023

ymao1 Jun 2, 2023

elasticmachine commented Jun 5, 2023

mikecote left a comment

mikecote Jun 6, 2023

ymao1 Jun 6, 2023

mikecote Jun 7, 2023

ymao1 Jun 7, 2023

ymao1 Jun 6, 2023

mikecote Jun 7, 2023

ymao1 Jun 7, 2023

pmuellr left a comment

pmuellr Jun 5, 2023

e40pud left a comment

maryam-saeidi left a comment

kibana-ci commented Jun 7, 2023

API count

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

[Response Ops][Alerting] Update FAAD AlertsClient to support AAD payload #158404

[Response Ops][Alerting] Update FAAD AlertsClient to support AAD payload #158404

Conversation

ymao1 commented May 24, 2023 • edited Loading

Summary

Followup issues

To Verify

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jun 5, 2023

mikecote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmuellr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e40pud left a comment

Choose a reason for hiding this comment

maryam-saeidi left a comment

Choose a reason for hiding this comment

kibana-ci commented Jun 7, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Public APIs missing exports

API count

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

History

[Response Ops][Alerting] Update FAAD `AlertsClient` to support AAD payload #158404

[Response Ops][Alerting] Update FAAD `AlertsClient` to support AAD payload #158404

ymao1 commented May 24, 2023 •

edited

Loading