[Security Solution] [DETECTIONS] Set rule status to failure only on large gaps (#71549)

* only display gap error when a gap is too large for the gap mitigation code to cover, general code cleanup, adds some tests for separate function

* removes throwing of errors and log error and return null for maxCatchup, ratio, and gapDiffInUnits properties

* forgot to delete commented out code

* remove math.abs since we fixed this bug by switching around logic when calculating gapDiffInUnits in getGapMaxCatchupRatio fn

* updates tests for when a gap error should be written to rule status

* fix typo

Author: dhurley14

x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.test.ts

@@ -10,7 +10,13 @@ import { getResult, getMlResult } from '../routes/__mocks__/request_responses';
 import { signalRulesAlertType } from './signal_rule_alert_type';
 import { alertsMock, AlertServicesMock } from '../../../../../alerts/server/mocks';
 import { ruleStatusServiceFactory } from './rule_status_service';
-import { getGapBetweenRuns, getListsClient, getExceptions, sortExceptionItems } from './utils';
+import {
+  getGapBetweenRuns,
+  getGapMaxCatchupRatio,
+  getListsClient,
+  getExceptions,
+  sortExceptionItems,
+} from './utils';
 import { RuleExecutorOptions } from './types';
 import { searchAfterAndBulkCreate } from './search_after_bulk_create';
 import { scheduleNotificationActions } from '../notifications/schedule_notification_actions';
@@ -97,6 +103,7 @@ describe('rules_notification_alert_type', () => {
       exceptionsWithValueLists: [],
     });
     (searchAfterAndBulkCreate as jest.Mock).mockClear();
+    (getGapMaxCatchupRatio as jest.Mock).mockClear();
     (searchAfterAndBulkCreate as jest.Mock).mockResolvedValue({
       success: true,
       searchAfterTimes: [],
@@ -126,22 +133,39 @@ describe('rules_notification_alert_type', () => {
   });
 
   describe('executor', () => {
-    it('should warn about the gap between runs', async () => {
-      (getGapBetweenRuns as jest.Mock).mockReturnValue(moment.duration(1000));
+    it('should warn about the gap between runs if gap is very large', async () => {
+      (getGapBetweenRuns as jest.Mock).mockReturnValue(moment.duration(100, 'm'));
+      (getGapMaxCatchupRatio as jest.Mock).mockReturnValue({
+        maxCatchup: 4,
+        ratio: 20,
+        gapDiffInUnits: 95,
+      });
       await alert.executor(payload);
       expect(logger.warn).toHaveBeenCalled();
       expect(logger.warn.mock.calls[0][0]).toContain(
-        'a few seconds (1000ms) has passed since last rule execution, and signals may have been missed.'
+        '2 hours (6000000ms) has passed since last rule execution, and signals may have been missed.'
       );
       expect(ruleStatusService.error).toHaveBeenCalled();
       expect(ruleStatusService.error.mock.calls[0][0]).toContain(
-        'a few seconds (1000ms) has passed since last rule execution, and signals may have been missed.'
+        '2 hours (6000000ms) has passed since last rule execution, and signals may have been missed.'
       );
       expect(ruleStatusService.error.mock.calls[0][1]).toEqual({
-        gap: 'a few seconds',
+        gap: '2 hours',
       });
     });
 
+    it('should NOT warn about the gap between runs if gap small', async () => {
+      (getGapBetweenRuns as jest.Mock).mockReturnValue(moment.duration(1, 'm'));
+      (getGapMaxCatchupRatio as jest.Mock).mockReturnValue({
+        maxCatchup: 1,
+        ratio: 1,
+        gapDiffInUnits: 1,
+      });
+      await alert.executor(payload);
+      expect(logger.warn).toHaveBeenCalledTimes(0);
+      expect(ruleStatusService.error).toHaveBeenCalledTimes(0);
+    });
+
     it("should set refresh to 'wait_for' when actions are present", async () => {
       const ruleAlert = getResult();
       ruleAlert.actions = [

x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.ts

@@ -22,7 +22,14 @@ import {
 } from './search_after_bulk_create';
 import { getFilter } from './get_filter';
 import { SignalRuleAlertTypeDefinition, RuleAlertAttributes } from './types';
-import { getGapBetweenRuns, parseScheduleDates, getListsClient, getExceptions } from './utils';
+import {
+  getGapBetweenRuns,
+  parseScheduleDates,
+  getListsClient,
+  getExceptions,
+  getGapMaxCatchupRatio,
+  MAX_RULE_GAP_RATIO,
+} from './utils';
 import { signalParamsSchema } from './signal_params_schema';
 import { siemRuleActionGroups } from './siem_rule_action_groups';
 import { findMlSignals } from './find_ml_signals';
@@ -130,15 +137,26 @@ export const signalRulesAlertType = ({
 
       const gap = getGapBetweenRuns({ previousStartedAt, interval, from, to });
       if (gap != null && gap.asMilliseconds() > 0) {
-        const gapString = gap.humanize();
-        const gapMessage = buildRuleMessage(
-          `${gapString} (${gap.asMilliseconds()}ms) has passed since last rule execution, and signals may have been missed.`,
-          'Consider increasing your look behind time or adding more Kibana instances.'
-        );
-        logger.warn(gapMessage);
+        const fromUnit = from[from.length - 1];
+        const { ratio } = getGapMaxCatchupRatio({
+          logger,
+          buildRuleMessage,
+          previousStartedAt,
+          ruleParamsFrom: from,
+          interval,
+          unit: fromUnit,
+        });
+        if (ratio && ratio >= MAX_RULE_GAP_RATIO) {
+          const gapString = gap.humanize();
+          const gapMessage = buildRuleMessage(
+            `${gapString} (${gap.asMilliseconds()}ms) has passed since last rule execution, and signals may have been missed.`,
+            'Consider increasing your look behind time or adding more Kibana instances.'
+          );
+          logger.warn(gapMessage);
 
-        hasError = true;
-        await ruleStatusService.error(gapMessage, { gap: gapString });
+          hasError = true;
+          await ruleStatusService.error(gapMessage, { gap: gapString });
+        }
       }
       try {
         const { listClient, exceptionsClient } = await getListsClient({

x-pack/plugins/security_solution/server/lib/detection_engine/signals/types.ts

@@ -11,6 +11,11 @@ import { RuleAlertAction } from '../../../../common/detection_engine/types';
 import { RuleTypeParams } from '../types';
 import { SearchResponse } from '../../types';
 
+// used for gap detection code
+export type unitType = 's' | 'm' | 'h';
+export const isValidUnit = (unitParam: string): unitParam is unitType =>
+  ['s', 'm', 'h'].includes(unitParam);
+
 export interface SignalsParams {
   signalIds: string[] | undefined | null;
   query: object | undefined | null;

x-pack/plugins/security_solution/server/lib/detection_engine/signals/utils.test.ts

@@ -21,6 +21,7 @@ import {
   parseScheduleDates,
   getDriftTolerance,
   getGapBetweenRuns,
+  getGapMaxCatchupRatio,
   errorAggregator,
   getListsClient,
   hasLargeValueList,
@@ -716,6 +717,52 @@ describe('utils', () => {
     });
   });
 
+  describe('getMaxCatchupRatio', () => {
+    test('should return null if rule has never run before', () => {
+      const { maxCatchup, ratio, gapDiffInUnits } = getGapMaxCatchupRatio({
+        logger: mockLogger,
+        previousStartedAt: null,
+        interval: '30s',
+        ruleParamsFrom: 'now-30s',
+        buildRuleMessage,
+        unit: 's',
+      });
+      expect(maxCatchup).toBeNull();
+      expect(ratio).toBeNull();
+      expect(gapDiffInUnits).toBeNull();
+    });
+
+    test('should should have non-null values when gap is present', () => {
+      const { maxCatchup, ratio, gapDiffInUnits } = getGapMaxCatchupRatio({
+        logger: mockLogger,
+        previousStartedAt: moment().subtract(65, 's').toDate(),
+        interval: '50s',
+        ruleParamsFrom: 'now-55s',
+        buildRuleMessage,
+        unit: 's',
+      });
+      expect(maxCatchup).toEqual(0.2);
+      expect(ratio).toEqual(0.2);
+      expect(gapDiffInUnits).toEqual(10);
+    });
+
+    // when a rule runs sooner than expected we don't
+    // consider that a gap as that is a very rare circumstance
+    test('should return null when given a negative gap (rule ran sooner than expected)', () => {
+      const { maxCatchup, ratio, gapDiffInUnits } = getGapMaxCatchupRatio({
+        logger: mockLogger,
+        previousStartedAt: moment().subtract(-15, 's').toDate(),
+        interval: '10s',
+        ruleParamsFrom: 'now-13s',
+        buildRuleMessage,
+        unit: 's',
+      });
+      expect(maxCatchup).toBeNull();
+      expect(ratio).toBeNull();
+      expect(gapDiffInUnits).toBeNull();
+    });
+  });
+
   describe('#getExceptions', () => {
     test('it successfully returns array of exception list items', async () => {
       const client = listMock.getExceptionListClient();