[ResponseOps] Update flapping logic order to determine whether an ale…

…rt is flapping after it's returned for notification (#151148) Resolves #151135 ## Summary Moved the logic around to check flapping after an alert is already returned for notification. I also updated the event log test cases to match examples in the rfc. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### To Verify The new test cases capture this example but if you want to test it locally: - Create a connector and make sure to save the connector id ``` curl -X POST -u {username}:{password} "http://localhost:5601/api/actions/connector" -H 'kbn-xsrf: true' -H 'Content-Type: application/json' -d' { "connector_type_id": ".server-log", "name": "server log", "config": {}, "secrets": {} }' ``` - Create the rule and let it run ``` curl -X POST -u {username}:{password} "http://localhost:5601/api/alerting/rule/" -H 'kbn-xsrf: true' -H 'Content-Type: application/json' -d' { "rule_type_id": "example.pattern", "name": "pattern", "schedule": { "interval": "5s" }, "actions": [ { "group": "default", "id": {connector id}, "params": { "message": "{{alert.id}} active on run {{context.runs}} step {{context.patternIndex}} flapping {{alert.flapping}}"}}, { "group": "recovered", "id": {connector id}, "params": { "message": "{{alert.id}} recovered on run flapping {{alert.flapping}}"}} ], "consumer": "alerts", "tags": [], "notify_when": "onActionGroupChange", "params": { "patterns": { "instA": " a - - a - a - a - a - - - - - - - - " } } }' ``` - Verify that you see the following, we want to make sure that aren't missing any notifications ![image](https://user-images.githubusercontent.com/109488926/221615405-48061d10-4b80-4b98-812a-6951da4178da.png)
elastic · Mar 1, 2023 · 5187a6f · 5187a6f
1 parent 647126a
commit 5187a6f
Show file tree

Hide file tree

Showing 7 changed files with 276 additions and 74 deletions.
diff --git a/x-pack/plugins/alerting/server/alerts_client/legacy_alerts_client.test.ts b/x-pack/plugins/alerting/server/alerts_client/legacy_alerts_client.test.ts
@@ -12,7 +12,7 @@ import { createAlertFactory, getPublicAlertFactory } from '../alert/create_alert
 import { Alert } from '../alert/alert';
 import { alertingEventLoggerMock } from '../lib/alerting_event_logger/alerting_event_logger.mock';
 import { ruleRunMetricsStoreMock } from '../lib/rule_run_metrics_store.mock';
-import { getAlertsForNotification, processAlerts, setFlapping } from '../lib';
+import { getAlertsForNotification, processAlerts } from '../lib';
 import { logAlerts } from '../task_runner/log_alerts';
 import { DEFAULT_FLAPPING_SETTINGS } from '../../common/rules_settings';
 
@@ -254,19 +254,6 @@ describe('Legacy Alerts Client', () => {
       flappingSettings: DEFAULT_FLAPPING_SETTINGS,
     });
 
-    expect(setFlapping).toHaveBeenCalledWith(
-      {
-        enabled: true,
-        lookBackWindow: 20,
-        statusChangeThreshold: 4,
-      },
-      {
-        '1': new Alert<AlertInstanceContext, AlertInstanceContext>('1', testAlert1),
-        '2': new Alert<AlertInstanceContext, AlertInstanceContext>('2', testAlert2),
-      },
-      {}
-    );
-
     expect(getAlertsForNotification).toHaveBeenCalledWith(
       {
         enabled: true,

diff --git a/x-pack/plugins/alerting/server/alerts_client/legacy_alerts_client.ts b/x-pack/plugins/alerting/server/alerts_client/legacy_alerts_client.ts
@@ -143,12 +143,6 @@ export class LegacyAlertsClient<
       flappingSettings,
     });
 
-    setFlapping<State, Context, ActionGroupIds, RecoveryActionGroupId>(
-      flappingSettings,
-      processedAlertsActive,
-      processedAlertsRecovered
-    );
-
     const { trimmedAlertsRecovered, earlyRecoveredAlerts } = trimRecoveredAlerts(
       this.options.logger,
       processedAlertsRecovered,
@@ -213,4 +207,12 @@ export class LegacyAlertsClient<
   public getExecutorServices() {
     return getPublicAlertFactory(this.alertFactory!);
   }
+
+  public setFlapping(flappingSettings: RulesSettingsFlappingProperties) {
+    setFlapping<State, Context, ActionGroupIds, RecoveryActionGroupId>(
+      flappingSettings,
+      this.processedAlerts.active,
+      this.processedAlerts.recovered
+    );
+  }
 }
diff --git a/x-pack/plugins/alerting/server/task_runner/task_runner.ts b/x-pack/plugins/alerting/server/task_runner/task_runner.ts
@@ -466,6 +466,8 @@ export class TaskRunner<
       }
     });
 
+    this.legacyAlertsClient.setFlapping(flappingSettings);
+
     let alertsToReturn: Record<string, RawAlertInstance> = {};
     let recoveredAlertsToReturn: Record<string, RawAlertInstance> = {};
     // Only serialize alerts into task state if we're auto-recovering, otherwise

diff --git a/x-pack/plugins/rule_registry/server/utils/create_lifecycle_executor.test.ts b/x-pack/plugins/rule_registry/server/utils/create_lifecycle_executor.test.ts
@@ -1015,7 +1015,7 @@ describe('createLifecycleExecutor', () => {
         return { state };
       });
 
-      await executor(
+      const serializedAlerts = await executor(
         createDefaultAlertExecutorOptions({
           alertId: 'TEST_ALERT_0',
           params: {},
@@ -1061,6 +1061,43 @@ describe('createLifecycleExecutor', () => {
         })
       );
 
+      expect(serializedAlerts.state.trackedAlerts).toEqual({
+        TEST_ALERT_0: {
+          alertId: 'TEST_ALERT_0',
+          alertUuid: 'TEST_ALERT_0_UUID',
+          flapping: true,
+          flappingHistory: flapping.slice(1).concat([false]),
+          pendingRecoveredCount: 0,
+          started: '2020-01-01T12:00:00.000Z',
+        },
+        TEST_ALERT_1: {
+          alertId: 'TEST_ALERT_1',
+          alertUuid: 'TEST_ALERT_1_UUID',
+          flapping: false,
+          flappingHistory: [false, false, false],
+          pendingRecoveredCount: 0,
+          started: '2020-01-02T12:00:00.000Z',
+        },
+        TEST_ALERT_2: {
+          alertId: 'TEST_ALERT_2',
+          alertUuid: 'TEST_ALERT_2_UUID',
+          flapping: true,
+          flappingHistory: flapping.slice(1).concat([false]),
+          pendingRecoveredCount: 0,
+          started: '2020-01-01T12:00:00.000Z',
+        },
+        TEST_ALERT_3: {
+          alertId: 'TEST_ALERT_3',
+          alertUuid: 'TEST_ALERT_3_UUID',
+          flapping: true,
+          flappingHistory: [false, false, false],
+          pendingRecoveredCount: 0,
+          started: '2020-01-02T12:00:00.000Z',
+        },
+      });
+
+      expect(serializedAlerts.state.trackedAlertsRecovered).toEqual({});
+
       expect((await ruleDataClientMock.getWriter()).bulk).toHaveBeenCalledWith(
         expect.objectContaining({
           body: [
@@ -1070,7 +1107,7 @@ describe('createLifecycleExecutor', () => {
               [ALERT_INSTANCE_ID]: 'TEST_ALERT_0',
               [ALERT_WORKFLOW_STATUS]: 'closed',
               [ALERT_STATUS]: ALERT_STATUS_ACTIVE,
-              [ALERT_FLAPPING]: true,
+              [ALERT_FLAPPING]: false,
               [EVENT_ACTION]: 'active',
               [EVENT_KIND]: 'signal',
             }),
@@ -1182,7 +1219,7 @@ describe('createLifecycleExecutor', () => {
         return { state };
       });
 
-      await executor(
+      const serializedAlerts = await executor(
         createDefaultAlertExecutorOptions({
           alertId: 'TEST_ALERT_0',
           params: {},
@@ -1228,17 +1265,55 @@ describe('createLifecycleExecutor', () => {
         })
       );
 
+      expect(serializedAlerts.state.trackedAlerts).toEqual({
+        TEST_ALERT_2: {
+          alertId: 'TEST_ALERT_2',
+          alertUuid: 'TEST_ALERT_2_UUID',
+          flapping: true,
+          flappingHistory: [true, true, true],
+          pendingRecoveredCount: 1,
+          started: '2020-01-02T12:00:00.000Z',
+        },
+      });
+
+      expect(serializedAlerts.state.trackedAlertsRecovered).toEqual({
+        TEST_ALERT_0: {
+          alertId: 'TEST_ALERT_0',
+          alertUuid: 'TEST_ALERT_0_UUID',
+          flapping: true,
+          flappingHistory: [true, true, true, true, true],
+          pendingRecoveredCount: 0,
+          started: '2020-01-01T12:00:00.000Z',
+        },
+        TEST_ALERT_1: {
+          alertId: 'TEST_ALERT_1',
+          alertUuid: 'TEST_ALERT_1_UUID',
+          flapping: false,
+          flappingHistory: notFlapping.slice(0, notFlapping.length - 1).concat([true]),
+          pendingRecoveredCount: 0,
+          started: '2020-01-02T12:00:00.000Z',
+        },
+        TEST_ALERT_3: {
+          alertId: 'TEST_ALERT_3',
+          alertUuid: 'TEST_ALERT_3_UUID',
+          flapping: false,
+          flappingHistory: notFlapping.slice(0, notFlapping.length - 1).concat([true]),
+          pendingRecoveredCount: 0,
+          started: '2020-01-02T12:00:00.000Z',
+        },
+      });
+
       expect((await ruleDataClientMock.getWriter()).bulk).toHaveBeenCalledWith(
         expect.objectContaining({
           body: expect.arrayContaining([
             // alert document
             { index: { _id: 'TEST_ALERT_0_UUID' } },
             expect.objectContaining({
               [ALERT_INSTANCE_ID]: 'TEST_ALERT_0',
-              [ALERT_STATUS]: ALERT_STATUS_ACTIVE,
-              [EVENT_ACTION]: 'active',
+              [ALERT_STATUS]: ALERT_STATUS_RECOVERED,
+              [EVENT_ACTION]: 'close',
               [EVENT_KIND]: 'signal',
-              [ALERT_FLAPPING]: true,
+              [ALERT_FLAPPING]: false,
             }),
             { index: { _id: 'TEST_ALERT_1_UUID' } },
             expect.objectContaining({

diff --git a/x-pack/plugins/rule_registry/server/utils/create_lifecycle_executor.ts b/x-pack/plugins/rule_registry/server/utils/create_lifecycle_executor.ts
@@ -278,12 +278,7 @@ export const createLifecycleExecutor =
           trackedAlertRecoveredIds
         );
 
-        const {
-          alertUuid,
-          started,
-          flapping: isCurrentlyFlapping,
-          pendingRecoveredCount,
-        } = !isNew
+        const { alertUuid, started, flapping, pendingRecoveredCount } = !isNew
           ? state.trackedAlerts[alertId]
           : {
               alertUuid: lifecycleAlertServices.getAlertUuid(alertId),
@@ -294,8 +289,6 @@ export const createLifecycleExecutor =
               pendingRecoveredCount: 0,
             };
 
-        const flapping = isFlapping(flappingSettings, flappingHistory, isCurrentlyFlapping);
-
         const event: ParsedTechnicalFields & ParsedExperimentalFields = {
           ...alertData?.fields,
           ...commonRuleFields,
@@ -368,10 +361,11 @@ export const createLifecycleExecutor =
     const nextTrackedAlerts = Object.fromEntries(
       allEventsToIndex
         .filter(({ event }) => event[ALERT_STATUS] !== ALERT_STATUS_RECOVERED)
-        .map(({ event, flappingHistory, flapping, pendingRecoveredCount }) => {
+        .map(({ event, flappingHistory, flapping: isCurrentlyFlapping, pendingRecoveredCount }) => {
           const alertId = event[ALERT_INSTANCE_ID]!;
           const alertUuid = event[ALERT_UUID]!;
           const started = new Date(event[ALERT_START]!).toISOString();
+          const flapping = isFlapping(flappingSettings, flappingHistory, isCurrentlyFlapping);
           return [
             alertId,
             { alertId, alertUuid, started, flappingHistory, flapping, pendingRecoveredCount },
@@ -389,10 +383,11 @@ export const createLifecycleExecutor =
             event[ALERT_STATUS] === ALERT_STATUS_RECOVERED &&
             (flapping || flappingHistory.filter((f: boolean) => f).length > 0)
         )
-        .map(({ event, flappingHistory, flapping, pendingRecoveredCount }) => {
+        .map(({ event, flappingHistory, flapping: isCurrentlyFlapping, pendingRecoveredCount }) => {
           const alertId = event[ALERT_INSTANCE_ID]!;
           const alertUuid = event[ALERT_UUID]!;
           const started = new Date(event[ALERT_START]!).toISOString();
+          const flapping = isFlapping(flappingSettings, flappingHistory, isCurrentlyFlapping);
           return [
             alertId,
             { alertId, alertUuid, started, flappingHistory, flapping, pendingRecoveredCount },