[Security Solution] Improve find rule and find rule status route perf…

…ormance (#99678) (#100912) * Fetch rule statuses using single aggregation instead of N separate requests * Optimize _find API and _find_statuses * Merge alerting framework errors into rule statuses * Add sortSchema for top hits agg, update terms.order schema Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Marshall Main <55718608+marshallmain@users.noreply.github.com>
elastic · May 28, 2021 · 3fa7834 · 3fa7834
1 parent a3da48d
commit 3fa7834
Show file tree

Hide file tree

Showing 19 changed files with 321 additions and 154 deletions.
diff --git a/src/core/server/saved_objects/service/lib/aggregations/aggs_types/bucket_aggs.ts b/src/core/server/saved_objects/service/lib/aggregations/aggs_types/bucket_aggs.ts
@@ -7,6 +7,7 @@
  */
 
 import { schema as s, ObjectType } from '@kbn/config-schema';
+import { sortOrderSchema } from './common_schemas';
 
 /**
  * Schemas for the Bucket aggregations.
@@ -85,6 +86,12 @@ export const bucketAggsSchemas: Record<string, ObjectType> = {
     min_doc_count: s.maybe(s.number({ min: 1 })),
     size: s.maybe(s.number()),
     show_term_doc_count_error: s.maybe(s.boolean()),
-    order: s.maybe(s.oneOf([s.literal('asc'), s.literal('desc')])),
+    order: s.maybe(
+      s.oneOf([
+        sortOrderSchema,
+        s.recordOf(s.string(), sortOrderSchema),
+        s.arrayOf(s.recordOf(s.string(), sortOrderSchema)),
+      ])
+    ),
   }),
 };
diff --git a/src/core/server/saved_objects/service/lib/aggregations/aggs_types/common_schemas.ts b/src/core/server/saved_objects/service/lib/aggregations/aggs_types/common_schemas.ts
@@ -0,0 +1,29 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+import { schema as s } from '@kbn/config-schema';
+
+// note: these schemas are not exhaustive. See the `Sort` type of `@elastic/elasticsearch` if you need to enhance it.
+const fieldSchema = s.string();
+export const sortOrderSchema = s.oneOf([s.literal('asc'), s.literal('desc'), s.literal('_doc')]);
+const sortModeSchema = s.oneOf([
+  s.literal('min'),
+  s.literal('max'),
+  s.literal('sum'),
+  s.literal('avg'),
+  s.literal('median'),
+]);
+const fieldSortSchema = s.object({
+  missing: s.maybe(s.oneOf([s.string(), s.number(), s.boolean()])),
+  mode: s.maybe(sortModeSchema),
+  order: s.maybe(sortOrderSchema),
+  // nested and unmapped_type not implemented yet
+});
+const sortContainerSchema = s.recordOf(s.string(), s.oneOf([sortOrderSchema, fieldSortSchema]));
+const sortCombinationsSchema = s.oneOf([fieldSchema, sortContainerSchema]);
+export const sortSchema = s.oneOf([sortCombinationsSchema, s.arrayOf(sortCombinationsSchema)]);
diff --git a/src/core/server/saved_objects/service/lib/aggregations/aggs_types/metrics_aggs.ts b/src/core/server/saved_objects/service/lib/aggregations/aggs_types/metrics_aggs.ts
@@ -7,6 +7,7 @@
  */
 
 import { schema as s, ObjectType } from '@kbn/config-schema';
+import { sortSchema } from './common_schemas';
 
 /**
  * Schemas for the metrics Aggregations
@@ -68,7 +69,7 @@ export const metricsAggsSchemas: Record<string, ObjectType> = {
     stored_fields: s.maybe(s.oneOf([s.string(), s.arrayOf(s.string())])),
     from: s.maybe(s.number()),
     size: s.maybe(s.number()),
-    sort: s.maybe(s.oneOf([s.literal('asc'), s.literal('desc')])),
+    sort: s.maybe(sortSchema),
     seq_no_primary_term: s.maybe(s.boolean()),
     version: s.maybe(s.boolean()),
     track_scores: s.maybe(s.boolean()),

diff --git a/...ugins/security_solution/server/lib/detection_engine/routes/__mocks__/request_responses.ts b/...ugins/security_solution/server/lib/detection_engine/routes/__mocks__/request_responses.ts
@@ -491,6 +491,72 @@ export const getFindResultStatus = (): SavedObjectsFindResponse<IRuleSavedAttrib
   ],
 });
 
+export const getFindBulkResultStatus = (): SavedObjectsFindResponse<IRuleSavedAttributesSavedObjectAttributes> => ({
+  page: 1,
+  per_page: 6,
+  total: 2,
+  saved_objects: [],
+  aggregations: {
+    alertIds: {
+      buckets: [
+        {
+          key: '04128c15-0d1b-4716-a4c5-46997ac7f3bd',
+          most_recent_statuses: {
+            hits: {
+              hits: [
+                {
+                  _source: {
+                    'siem-detection-engine-rule-status': {
+                      alertId: '04128c15-0d1b-4716-a4c5-46997ac7f3bd',
+                      statusDate: '2020-02-18T15:26:49.783Z',
+                      status: 'succeeded',
+                      lastFailureAt: undefined,
+                      lastSuccessAt: '2020-02-18T15:26:49.783Z',
+                      lastFailureMessage: undefined,
+                      lastSuccessMessage: 'succeeded',
+                      lastLookBackDate: new Date('2020-02-18T15:14:58.806Z').toISOString(),
+                      gap: '500.32',
+                      searchAfterTimeDurations: ['200.00'],
+                      bulkCreateTimeDurations: ['800.43'],
+                    },
+                  },
+                },
+              ],
+            },
+          },
+        },
+        {
+          key: '1ea5a820-4da1-4e82-92a1-2b43a7bece08',
+          most_recent_statuses: {
+            hits: {
+              hits: [
+                {
+                  _source: {
+                    'siem-detection-engine-rule-status': {
+                      alertId: '1ea5a820-4da1-4e82-92a1-2b43a7bece08',
+                      statusDate: '2020-02-18T15:15:58.806Z',
+                      status: 'failed',
+                      lastFailureAt: '2020-02-18T15:15:58.806Z',
+                      lastSuccessAt: '2020-02-13T20:31:59.855Z',
+                      lastFailureMessage:
+                        'Signal rule name: "Query with a rule id Number 1", id: "1ea5a820-4da1-4e82-92a1-2b43a7bece08", rule_id: "query-rule-id-1" has a time gap of 5 days (412682928ms), and could be missing signals within that time. Consider increasing your look behind time or adding more Kibana instances.',
+                      lastSuccessMessage: 'succeeded',
+                      lastLookBackDate: new Date('2020-02-18T15:14:58.806Z').toISOString(),
+                      gap: '500.32',
+                      searchAfterTimeDurations: ['200.00'],
+                      bulkCreateTimeDurations: ['800.43'],
+                    },
+                  },
+                },
+              ],
+            },
+          },
+        },
+      ],
+    },
+  },
+});
+
 export const getEmptySignalsResponse = (): SignalSearchResponse => ({
   took: 1,
   timed_out: false,

diff --git a/...ugins/security_solution/server/lib/detection_engine/routes/rules/find_rules_route.test.ts b/...ugins/security_solution/server/lib/detection_engine/routes/rules/find_rules_route.test.ts
@@ -10,7 +10,7 @@ import {
   getAlertMock,
   getFindRequest,
   getFindResultWithSingleHit,
-  getFindResultStatus,
+  getFindBulkResultStatus,
 } from '../__mocks__/request_responses';
 import { requestContextMock, serverMock, requestMock } from '../__mocks__';
 import { findRulesRoute } from './find_rules_route';
@@ -27,7 +27,7 @@ describe('find_rules', () => {
 
     clients.alertsClient.find.mockResolvedValue(getFindResultWithSingleHit());
     clients.alertsClient.get.mockResolvedValue(getAlertMock(getQueryRuleParams()));
-    clients.savedObjectsClient.find.mockResolvedValue(getFindResultStatus());
+    clients.savedObjectsClient.find.mockResolvedValue(getFindBulkResultStatus());
 
     findRulesRoute(server.router);
   });

diff --git a/...ck/plugins/security_solution/server/lib/detection_engine/routes/rules/find_rules_route.ts b/...ck/plugins/security_solution/server/lib/detection_engine/routes/rules/find_rules_route.ts
@@ -15,11 +15,10 @@ import type { SecuritySolutionPluginRouter } from '../../../../types';
 import { DETECTION_ENGINE_RULES_URL } from '../../../../../common/constants';
 import { findRules } from '../../rules/find_rules';
 import { buildSiemResponse } from '../utils';
-
-import { getRuleActionsSavedObject } from '../../rule_actions/get_rule_actions_saved_object';
 import { ruleStatusSavedObjectsClientFactory } from '../../signals/rule_status_saved_objects_client';
 import { buildRouteValidation } from '../../../../utils/build_validation/route_validation';
 import { transformFindAlerts } from './utils';
+import { getBulkRuleActionsSavedObject } from '../../rule_actions/get_bulk_rule_actions_saved_object';
 
 export const findRulesRoute = (router: SecuritySolutionPluginRouter) => {
   router.get(
@@ -60,44 +59,11 @@ export const findRulesRoute = (router: SecuritySolutionPluginRouter) => {
           filter: query.filter,
           fields: query.fields,
         });
-
-        // if any rules attempted to execute but failed before the rule executor is called,
-        // an execution status will be written directly onto the rule via the kibana alerting framework,
-        // which we are filtering on and will write a failure status
-        // for any rules found to be in a failing state into our rule status saved objects
-        const failingRules = rules.data.filter(
-          (rule) => rule.executionStatus != null && rule.executionStatus.status === 'error'
-        );
-
-        const ruleStatuses = await Promise.all(
-          rules.data.map(async (rule) => {
-            const results = await ruleStatusClient.find({
-              perPage: 1,
-              sortField: 'statusDate',
-              sortOrder: 'desc',
-              search: rule.id,
-              searchFields: ['alertId'],
-            });
-            const failingRule = failingRules.find((badRule) => badRule.id === rule.id);
-            if (failingRule != null) {
-              if (results.saved_objects.length > 0) {
-                results.saved_objects[0].attributes.status = 'failed';
-                results.saved_objects[0].attributes.lastFailureAt = failingRule.executionStatus.lastExecutionDate.toISOString();
-              }
-            }
-            return results;
-          })
-        );
-        const ruleActions = await Promise.all(
-          rules.data.map(async (rule) => {
-            const results = await getRuleActionsSavedObject({
-              savedObjectsClient,
-              ruleAlertId: rule.id,
-            });
-
-            return results;
-          })
-        );
+        const alertIds = rules.data.map((rule) => rule.id);
+        const [ruleStatuses, ruleActions] = await Promise.all([
+          ruleStatusClient.findBulk(alertIds, 1),
+          getBulkRuleActionsSavedObject({ alertIds, savedObjectsClient }),
+        ]);
         const transformed = transformFindAlerts(rules, ruleActions, ruleStatuses);
         if (transformed == null) {
           return siemResponse.error({ statusCode: 500, body: 'Internal error transforming' });

diff --git a/...ecurity_solution/server/lib/detection_engine/routes/rules/find_rules_status_route.test.ts b/...ecurity_solution/server/lib/detection_engine/routes/rules/find_rules_status_route.test.ts
@@ -7,9 +7,9 @@
 
 import { DETECTION_ENGINE_RULES_URL } from '../../../../../common/constants';
 import {
-  getFindResultStatus,
   ruleStatusRequest,
   getAlertMock,
+  getFindBulkResultStatus,
 } from '../__mocks__/request_responses';
 import { serverMock, requestContextMock, requestMock } from '../__mocks__';
 import { findRulesStatusesRoute } from './find_rules_status_route';
@@ -26,7 +26,7 @@ describe('find_statuses', () => {
   beforeEach(async () => {
     server = serverMock.create();
     ({ clients, context } = requestContextMock.createTools());
-    clients.savedObjectsClient.find.mockResolvedValue(getFindResultStatus()); // successful status search
+    clients.savedObjectsClient.find.mockResolvedValue(getFindBulkResultStatus()); // successful status search
     clients.alertsClient.get.mockResolvedValue(getAlertMock(getQueryRuleParams()));
     findRulesStatusesRoute(server.router);
   });

diff --git a/...ins/security_solution/server/lib/detection_engine/routes/rules/find_rules_status_route.ts b/...ins/security_solution/server/lib/detection_engine/routes/rules/find_rules_status_route.ts
@@ -9,14 +9,13 @@ import { transformError } from '@kbn/securitysolution-es-utils';
 import { buildRouteValidation } from '../../../../utils/build_validation/route_validation';
 import type { SecuritySolutionPluginRouter } from '../../../../types';
 import { DETECTION_ENGINE_RULES_URL } from '../../../../../common/constants';
-import { RuleStatusResponse } from '../../rules/types';
 import { buildSiemResponse, mergeStatuses, getFailingRules } from '../utils';
-
 import { ruleStatusSavedObjectsClientFactory } from '../../signals/rule_status_saved_objects_client';
 import {
   findRulesStatusesSchema,
   FindRulesStatusesSchemaDecoded,
 } from '../../../../../common/detection_engine/schemas/request/find_rule_statuses_schema';
+import { mergeAlertWithSidecarStatus } from '../../schemas/rule_converters';
 
 /**
  * Given a list of rule ids, return the current status and
@@ -51,45 +50,27 @@ export const findRulesStatusesRoute = (router: SecuritySolutionPluginRouter) =>
       const ids = body.ids;
       try {
         const ruleStatusClient = ruleStatusSavedObjectsClientFactory(savedObjectsClient);
-        const failingRules = await getFailingRules(ids, alertsClient);
+        const [statusesById, failingRules] = await Promise.all([
+          ruleStatusClient.findBulk(ids, 6),
+          getFailingRules(ids, alertsClient),
+        ]);
 
-        const statuses = await ids.reduce(async (acc, id) => {
-          const accumulated = await acc;
-          const lastFiveErrorsForId = await ruleStatusClient.find({
-            perPage: 6,
-            sortField: 'statusDate',
-            sortOrder: 'desc',
-            search: id,
-            searchFields: ['alertId'],
-          });
+        const statuses = ids.reduce((acc, id) => {
+          const lastFiveErrorsForId = statusesById[id];
 
-          if (lastFiveErrorsForId.saved_objects.length === 0) {
-            return accumulated;
+          if (lastFiveErrorsForId == null || lastFiveErrorsForId.length === 0) {
+            return acc;
           }
 
           const failingRule = failingRules[id];
-          const lastFailureAt = lastFiveErrorsForId.saved_objects[0].attributes.lastFailureAt;
-
-          if (
-            failingRule != null &&
-            (lastFailureAt == null ||
-              new Date(failingRule.executionStatus.lastExecutionDate) > new Date(lastFailureAt))
-          ) {
-            const currentStatus = lastFiveErrorsForId.saved_objects[0];
-            currentStatus.attributes.lastFailureMessage = `Reason: ${failingRule.executionStatus.error?.reason} Message: ${failingRule.executionStatus.error?.message}`;
-            currentStatus.attributes.lastFailureAt = failingRule.executionStatus.lastExecutionDate.toISOString();
-            currentStatus.attributes.statusDate = failingRule.executionStatus.lastExecutionDate.toISOString();
-            currentStatus.attributes.status = 'failed';
-            const updatedLastFiveErrorsSO = [
-              currentStatus,
-              ...lastFiveErrorsForId.saved_objects.slice(1),
-            ];
 
-            return mergeStatuses(id, updatedLastFiveErrorsSO, accumulated);
+          if (failingRule != null) {
+            const currentStatus = mergeAlertWithSidecarStatus(failingRule, lastFiveErrorsForId[0]);
+            const updatedLastFiveErrorsSO = [currentStatus, ...lastFiveErrorsForId.slice(1)];
+            return mergeStatuses(id, updatedLastFiveErrorsSO, acc);
           }
-          return mergeStatuses(id, [...lastFiveErrorsForId.saved_objects], accumulated);
-        }, Promise.resolve<RuleStatusResponse>({}));
-
+          return mergeStatuses(id, [...lastFiveErrorsForId], acc);
+        }, {});
         return response.ok({ body: statuses });
       } catch (err) {
         const error = transformError(err);

diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/utils.test.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/utils.test.ts
@@ -27,7 +27,6 @@ import { PartialFilter } from '../../types';
 import { BulkError, ImportSuccessError } from '../utils';
 import { getOutputRuleAlertForRest } from '../__mocks__/utils';
 import { PartialAlert } from '../../../../../../alerting/server';
-import { SanitizedAlert } from '../../../../../../alerting/server/types';
 import { createRulesStreamFromNdJson } from '../../rules/create_rules_stream_from_ndjson';
 import { RuleAlertType } from '../../rules/types';
 import { ImportRulesSchemaDecoded } from '../../../../../common/detection_engine/schemas/request/import_rules_schema';
@@ -256,7 +255,7 @@ describe('utils', () => {
 
   describe('transformFindAlerts', () => {
     test('outputs empty data set when data set is empty correct', () => {
-      const output = transformFindAlerts({ data: [], page: 1, perPage: 0, total: 0 }, []);
+      const output = transformFindAlerts({ data: [], page: 1, perPage: 0, total: 0 }, {}, {});
       expect(output).toEqual({ data: [], page: 1, perPage: 0, total: 0 });
     });
 
@@ -268,7 +267,8 @@ describe('utils', () => {
           total: 0,
           data: [getAlertMock(getQueryRuleParams())],
         },
-        []
+        {},
+        {}
       );
       const expected = getOutputRuleAlertForRest();
       expect(output).toEqual({
@@ -278,20 +278,6 @@ describe('utils', () => {
         data: [expected],
       });
     });
-
-    test('returns 500 if the data is not of type siem alert', () => {
-      const unsafeCast = ([{ name: 'something else' }] as unknown) as SanitizedAlert[];
-      const output = transformFindAlerts(
-        {
-          data: unsafeCast,
-          page: 1,
-          perPage: 1,
-          total: 1,
-        },
-        []
-      );
-      expect(output).toBeNull();
-    });
   });
 
   describe('transform', () => {