[ResponseOps][Alerts] Add alerts grouping aggregations endpoint #186475

umbopepato · 2024-06-19T15:07:47Z

Summary

Adds an endpoint dedicated to fetching alerts group aggregations to avoid adding runtime mappings and client-side controlled scripts to the internal/rac/alerts/find endpoint.
The new endpoint injects a groupByField runtime field used to normalize the values of the field used for grouping, to account for null and multi-element arrays.

#184635 depends on this

Closes #186383

To verify

Review the added tests.
Use the Kibana Dev Console to test various body params and aggregations:

Create any type of rule that fire alerts
Wait for the alerts to be created
Call the _group_aggregations endpoint, using the feature id(s) that cover the type of rules you used:

POST kbn:internal/rac/alerts/_group_aggregations
{
   "featureIds": [...],
  ...
}

See here and here to know the available params and pre-defined aggregations.

Checklist

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

…rouping-aggregations-endpoint

umbopepato · 2024-06-19T15:07:56Z

/ci

umbopepato · 2024-06-19T15:20:01Z

/ci

…rouping-aggregations-endpoint

umbopepato · 2024-06-19T16:09:39Z

/ci

…rouping-aggregations-endpoint

umbopepato · 2024-06-20T06:33:24Z

/ci

elasticmachine · 2024-06-20T08:19:09Z

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

adcoelho · 2024-06-20T09:29:15Z

x-pack/plugins/rule_registry/server/routes/get_alerts_group_aggregations.test.ts

+    });
+  });
+
+  test('returns error status if rac client find fails', async () => {


Could you also add a test where getGroupAggregations resolves but alerts == null? If I understand correctly the message should be different right? Satus 404 and message alerts not found?

As discussed on Slack that case is unreachable without an exception being thrown, I've removed the check altogether

adcoelho · 2024-06-20T09:36:00Z

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts

+  /**
+   * Performs a `find` query to extract aggregations on alert groups
+   */
+  public getGroupAggregations({


How about adding something like x-pack/plugins/rule_registry/server/alert_data_client/tests/getGroupAggregations.test.ts that tests this?

Nothing too elaborate, to ensure that find is called with the correct params and no changes break this function.

I know we do some testing in x-pack/plugins/rule_registry/server/routes/get_alerts_group_aggregations.test.ts but that is like "indirectly" 😛

adcoelho · 2024-06-20T09:37:01Z

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts

+    pageSize?: number;
+  }) {
+    const uniqueValue = uuid();
+    return this.find({


Like you were saying yesterday, QueryDSL gives me headaches 😄

adcoelho · 2024-06-20T09:40:32Z

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts

@@ -5,6 +5,7 @@
 * 2.0.
 */
 import Boom from '@hapi/boom';
+import { v4 as uuid } from 'uuid';


super nit:

Suggested change

import { v4 as uuid } from 'uuid';

import { v4 as uuidv4 } from 'uuid';

adcoelho · 2024-06-20T09:45:13Z

packages/kbn-alerts-ui-shared/src/common/hooks/use_get_alerts_group_aggregations_query.ts

+ *
+ * Applies alerting RBAC through featureIds.
+ */
+export const useGetAlertsGroupAggregationsQuery = <T>({


Could you please add tests for this in packages/kbn-alerts-ui-shared/src/common/hooks/use_get_alerts_group_aggregations_query.test.ts? Stuff like displays error toast if params are not JSON or API gets called with the correct body..

…rouping-aggregations-endpoint

kibana-ci · 2024-06-26T09:19:08Z

💚 Build Succeeded

Buildkite Build
Commit: 0731496

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`observability`	467	468	+1
`triggersActionsUi`	764	765	+1
total			+2

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
`ruleRegistry`	247	249	+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.7MB	1.7MB	+6.0B

Unknown metric groups

API count

id	before	after	diff
`ruleRegistry`	276	286	+10

History

💔 Build #217633 failed 238e803
💛 Build #217164 was flaky de6c865
💔 Build #216941 failed eaa7bd6
💛 Build #216825 was flaky d848db0
💔 Build #216769 failed 6f57233

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

cnasikas

LGTM! I left some comments.

cnasikas · 2024-06-26T11:02:31Z

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts

+    /**
+     * Any sort options to apply to the groupByField aggregations
+     */
+    sort?: object[];


In the useGetAlertsGroupAggregationsQuery you used the SortCombinations type for the sort. Should we use the same here?

cnasikas · 2024-06-26T11:07:32Z

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts

+  /**
+   * Performs a `find` query to extract aggregations on alert groups
+   */
+  public getGroupAggregations({


cnasikas · 2024-06-26T11:22:39Z

x-pack/plugins/rule_registry/server/routes/get_alerts_group_aggregations.ts

+        pageIndex = 0,
+        pageSize = DEFAULT_ALERTS_GROUP_BY_FIELD_SIZE,
+      } = request.body;
+      if (pageIndex < 0 || pageIndex > 100) {


With this validation, it seems that we cap users to not be able to go over the 100th page if the pageSize is 1. What about the following?

if (pageIndex > maxPerPage) { <--- 100 items per page throw Boom.badRequest('The provided pageIndex value is too high. The maximum allowed pageIndex value is ${maxPerPage}.'); } if (Math.max(pageIndex, pageIndex * pageSize) > MAX_DOCS_PER_PAGE) { <-- 10K max documents throw Boom.badRequest('The number of documents is too high. Paginating through more than ${MAX_DOCS_PER_PAGE} documents is not possible.'); }

Also, I am wondering if we should move the validation inside the alertsClient.getGroupAggregations. This way consumers of the alert client will have the same validation. Wdyt?

…rouping-aggregations-endpoint

cnasikas

Thanks for your patience with the review! LGTM 🚀 !

JiaweiWu

LGTM, just some small comments

JiaweiWu · 2024-07-03T17:13:32Z

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts

  index?: string;
-  _source?: string[] | undefined;
+  _source?: string[] | false;


curious about this type, why can it be false?

To disable the _source field (docs)

JiaweiWu · 2024-07-03T17:16:38Z

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts

-        this.logger.error(errorMessage);
-        throw Boom.badData(errorMessage);
-      }
+      if (result?.hits?.hits?.length > 0) {


nitpick: can return early here

if (!result?.hits?.hits?.length) { return result; } ... rest

JiaweiWu · 2024-07-03T17:21:54Z

x-pack/plugins/rule_registry/common/types.ts

+);
+
+export const alertsGroupFilterSchema = t.record(
+  t.union([


Do we have existing enums for these literals? Just so we can have a more centralized source of truth

Would have been nice, but I was only able to find type-level information. I added a type assertion to check that the keys are correctly aligned to the es types though 🙂

JiaweiWu · 2024-07-03T17:36:47Z

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts

+          script: {
+            source:
+              // When size()==0, emits a uniqueValue as the value to represent this group  else join by uniqueValue.
+              "if (doc[params['selectedGroup']].size()==0) { emit(params['uniqueValue']) }" +


nitpick: can use `` to avoid concatenating the strings

Yep, I thought it was worth keeping it as it was originally written by Security though, to comment the two parts separately 🙂

…rouping-aggregations-endpoint

elasticmachine · 2024-07-04T16:17:19Z

💚 Build Succeeded

Buildkite Build
Commit: c962e1f

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`observability`	469	470	+1
`triggersActionsUi`	770	771	+1
total			+2

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
`ruleRegistry`	247	249	+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.7MB	1.7MB	+6.0B

Unknown metric groups

API count

id	before	after	diff
`ruleRegistry`	276	286	+10

History

💛 Build #219345 was flaky cb2e535
💔 Build #219321 failed 6fc8aee
💔 Build #219296 failed 8869584

kibanamachine · 2024-07-04T17:00:00Z

💔 All backports failed

Status	Branch	Result
❌	8.16	The branch "8.16" does not exist

Manual backport

To create the backport manually run:

node scripts/backport --pr 186475

Questions ?

Please refer to the Backport tool documentation

kibanamachine · 2024-08-27T21:49:18Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 186475 locally

umbopepato added 3 commits June 19, 2024 15:02

Add get alerts group aggregations endpoint

ac35282

Add alerts group aggregation endpoint tests

15f9aa7

Merge branch 'main' of github.com:elastic/kibana into 186383-alerts-g…

46fea59

…rouping-aggregations-endpoint

Fix import

a47b66f

umbopepato added 4 commits June 19, 2024 17:57

Automatically add unitsCount subaggregation in groupByField aggregation

e091401

Merge branch 'main' of github.com:elastic/kibana into 186383-alerts-g…

ae8b482

…rouping-aggregations-endpoint

Make aggregations field optional, improve documentation

0529905

Merge branch 'main' of github.com:elastic/kibana into 186383-alerts-g…

6f57233

…rouping-aggregations-endpoint

umbopepato added 2 commits June 20, 2024 08:28

Merge branch 'main' of github.com:elastic/kibana into 186383-alerts-g…

e39bfa5

…rouping-aggregations-endpoint

Fix tests

d848db0

umbopepato added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.15.0 labels Jun 20, 2024

umbopepato marked this pull request as ready for review June 20, 2024 08:19

umbopepato requested review from a team as code owners June 20, 2024 08:19

umbopepato requested review from adcoelho and JiaweiWu June 20, 2024 08:42

adcoelho reviewed Jun 20, 2024

View reviewed changes

umbopepato added 2 commits June 20, 2024 17:18

Add tests and improve AlertsClient.find errors handling

46fb4fd

Merge branch 'main' of github.com:elastic/kibana into 186383-alerts-g…

eaa7bd6

…rouping-aggregations-endpoint

umbopepato added 6 commits June 21, 2024 15:07

Revert error response transformation

e8b0329

Use type instead of interface for validation

de6c865

Merge branch 'main' of github.com:elastic/kibana into 186383-alerts-g…

62194c9

…rouping-aggregations-endpoint

Restrict filters and pagination options validation

238e803

Merge branch 'main' of github.com:elastic/kibana into 186383-alerts-g…

1c1a1fd

…rouping-aggregations-endpoint

Fix test

0731496

cnasikas reviewed Jun 26, 2024

View reviewed changes

umbopepato added 4 commits July 3, 2024 08:57

Merge branch 'main' of github.com:elastic/kibana into 186383-alerts-g…

ca88557

…rouping-aggregations-endpoint

Improve typings and validations

8869584

Fix AlertsClient.find not returning result

11ac1fc

Merge branch 'main' of github.com:elastic/kibana into 186383-alerts-g…

6fc8aee

…rouping-aggregations-endpoint

cnasikas approved these changes Jul 3, 2024

View reviewed changes

This comment was marked as duplicate.

Sign in to view

Move alert group aggregations pagination tests

cb2e535

cnasikas added v8.16.0 v8.15.0 and removed v8.15.0 labels Jul 3, 2024

JiaweiWu approved these changes Jul 3, 2024

View reviewed changes

umbopepato added 3 commits July 4, 2024 15:45

Improve filters validation schema and searchAlerts control flow

90a86d1

Merge branch 'main' of github.com:elastic/kibana into 186383-alerts-g…

39e0ae2

…rouping-aggregations-endpoint

Fix filter keys validation schema type assertion

c962e1f

umbopepato merged commit 52ccd17 into elastic:main Jul 4, 2024
35 of 36 checks passed

cnasikas removed the v8.16.0 label Jul 5, 2024

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 27, 2024

cnasikas added backport:skip This commit does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Sep 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ResponseOps][Alerts] Add alerts grouping aggregations endpoint #186475

[ResponseOps][Alerts] Add alerts grouping aggregations endpoint #186475

umbopepato commented Jun 19, 2024 •

edited by kibanamachine

Loading

umbopepato commented Jun 19, 2024

umbopepato commented Jun 19, 2024

umbopepato commented Jun 19, 2024

umbopepato commented Jun 20, 2024

elasticmachine commented Jun 20, 2024

adcoelho Jun 20, 2024

umbopepato Jun 20, 2024

adcoelho Jun 20, 2024

cnasikas Jun 26, 2024

adcoelho Jun 20, 2024 •

edited

Loading

adcoelho Jun 20, 2024

adcoelho Jun 20, 2024 •

edited

Loading

kibana-ci commented Jun 26, 2024

API count

cnasikas left a comment

cnasikas Jun 26, 2024

cnasikas Jun 26, 2024

cnasikas Jun 26, 2024

cnasikas left a comment

This comment was marked as duplicate.

JiaweiWu left a comment

JiaweiWu Jul 3, 2024

umbopepato Jul 4, 2024

JiaweiWu Jul 3, 2024

JiaweiWu Jul 3, 2024

umbopepato Jul 4, 2024

JiaweiWu Jul 3, 2024

umbopepato Jul 4, 2024 •

edited

Loading

elasticmachine commented Jul 4, 2024

API count

kibanamachine commented Jul 4, 2024

kibanamachine commented Aug 27, 2024

	import { v4 as uuid } from 'uuid';
	import { v4 as uuidv4 } from 'uuid';

[ResponseOps][Alerts] Add alerts grouping aggregations endpoint #186475

[ResponseOps][Alerts] Add alerts grouping aggregations endpoint #186475

Conversation

umbopepato commented Jun 19, 2024 • edited by kibanamachine Loading

Summary

To verify

Checklist

umbopepato commented Jun 19, 2024

umbopepato commented Jun 19, 2024

umbopepato commented Jun 19, 2024

umbopepato commented Jun 20, 2024

elasticmachine commented Jun 20, 2024

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

adcoelho Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adcoelho Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

kibana-ci commented Jun 26, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

API count

History

cnasikas 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

cnasikas left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

JiaweiWu 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

umbopepato Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Jul 4, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

API count

History

kibanamachine commented Jul 4, 2024

💔 All backports failed

Manual backport

Questions ?

kibanamachine commented Aug 27, 2024

umbopepato commented Jun 19, 2024 •

edited by kibanamachine

Loading

adcoelho Jun 20, 2024 •

edited

Loading

adcoelho Jun 20, 2024 •

edited

Loading

umbopepato Jul 4, 2024 •

edited

Loading