[ResponseOps] Flapping recovered alerts do not show up as active in t…

…he alerts table (#184255) Resolves #183735 ## Summary This bug is caused by event log docs not being written for "pending recovered" alerts on rules with "notify on every run" and "throttle" or notify when set to null. In this PR I remove the logic causing this in `xpack/plugins/alerting/server/lib/get_alerts_for_notification.ts` so now these alerts will be logged in the event log and show up in the ui. ### Checklist - [ ] [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 - Create an ES Query rule without actions, or with `notifyWhen` set to "notify on every run" or "throttle". Get it to start flapping. - Let the flapping alert start to recover and verify it's still reported as active in the UI until it's been recovered for X executions (the default is 4 if you don't mess with your flapping settings). Ex. of what to expect <img width="1461" alt="Screen Shot 2024-05-29 at 10 53 57 AM" src="https://github.com/elastic/kibana/assets/109488926/144dc22b-a1c7-460f-b430-a6d49771c9bd">
elastic · Jun 3, 2024 · 1c56c51 · 1c56c51
1 parent 3a217fd
commit 1c56c51
Show file tree

Hide file tree

Showing 10 changed files with 9 additions and 156 deletions.
diff --git a/x-pack/plugins/alerting/server/alerts_client/alerts_client.test.ts b/x-pack/plugins/alerting/server/alerts_client/alerts_client.test.ts
@@ -313,7 +313,6 @@ describe('Alerts Client', () => {
           ruleRunMetricsStore,
           shouldLogAlerts: false,
           flappingSettings: DEFAULT_FLAPPING_SETTINGS,
-          notifyOnActionGroupChange: true,
           maintenanceWindowIds: [],
           alertDelay: 0,
         };

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
@@ -243,7 +243,6 @@ describe('Legacy Alerts Client', () => {
       ruleRunMetricsStore,
       shouldLogAlerts: true,
       flappingSettings: DEFAULT_FLAPPING_SETTINGS,
-      notifyOnActionGroupChange: true,
       maintenanceWindowIds: ['window-id1', 'window-id2'],
       alertDelay: 5,
     });
@@ -274,7 +273,6 @@ describe('Legacy Alerts Client', () => {
         lookBackWindow: 20,
         statusChangeThreshold: 4,
       },
-      true,
       'default',
       5,
       {},

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
@@ -142,7 +142,6 @@ export class LegacyAlertsClient<
   }
 
   public processAlerts({
-    notifyOnActionGroupChange,
     flappingSettings,
     maintenanceWindowIds,
     alertDelay,
@@ -173,7 +172,6 @@ export class LegacyAlertsClient<
 
     const alerts = getAlertsForNotification<State, Context, ActionGroupIds, RecoveryActionGroupId>(
       flappingSettings,
-      notifyOnActionGroupChange,
       this.options.ruleType.defaultActionGroupId,
       alertDelay,
       processedAlertsNew,
@@ -211,12 +209,10 @@ export class LegacyAlertsClient<
     ruleRunMetricsStore,
     shouldLogAlerts,
     flappingSettings,
-    notifyOnActionGroupChange,
     maintenanceWindowIds,
     alertDelay,
   }: ProcessAndLogAlertsOpts) {
     this.processAlerts({
-      notifyOnActionGroupChange,
       flappingSettings,
       maintenanceWindowIds,
       alertDelay,

diff --git a/x-pack/plugins/alerting/server/alerts_client/types.ts b/x-pack/plugins/alerting/server/alerts_client/types.ts
@@ -108,14 +108,12 @@ export interface ProcessAndLogAlertsOpts {
   shouldLogAlerts: boolean;
   ruleRunMetricsStore: RuleRunMetricsStore;
   flappingSettings: RulesSettingsFlappingProperties;
-  notifyOnActionGroupChange: boolean;
   maintenanceWindowIds: string[];
   alertDelay: number;
 }
 
 export interface ProcessAlertsOpts {
   flappingSettings: RulesSettingsFlappingProperties;
-  notifyOnActionGroupChange: boolean;
   maintenanceWindowIds: string[];
   alertDelay: number;
   ruleRunMetricsStore: RuleRunMetricsStore;

diff --git a/x-pack/plugins/alerting/server/lib/get_alerts_for_notification.test.ts b/x-pack/plugins/alerting/server/lib/get_alerts_for_notification.test.ts
@@ -19,7 +19,6 @@ describe('getAlertsForNotification', () => {
 
     const { newAlerts, activeAlerts } = getAlertsForNotification(
       DEFAULT_FLAPPING_SETTINGS,
-      true,
       'default',
       0,
       {
@@ -90,7 +89,6 @@ describe('getAlertsForNotification', () => {
       currentRecoveredAlerts,
     } = getAlertsForNotification(
       DEFAULT_FLAPPING_SETTINGS,
-      true,
       'default',
       0,
       {},
@@ -226,7 +224,6 @@ describe('getAlertsForNotification', () => {
     const { newAlerts, activeAlerts, recoveredAlerts, currentRecoveredAlerts } =
       getAlertsForNotification(
         DISABLE_FLAPPING_SETTINGS,
-        true,
         'default',
         0,
         {},
@@ -347,120 +344,6 @@ describe('getAlertsForNotification', () => {
     `);
   });
 
-  test('should return flapping pending recovered alerts as active alerts only when notifyWhen is onActionGroupChange', () => {
-    const alert1 = new Alert('1', { meta: { flapping: true, pendingRecoveredCount: 3 } });
-    const alert2 = new Alert('2', { meta: { flapping: false } });
-    const alert3 = new Alert('3', { meta: { flapping: true } });
-
-    const {
-      newAlerts,
-      activeAlerts,
-      currentActiveAlerts,
-      recoveredAlerts,
-      currentRecoveredAlerts,
-    } = getAlertsForNotification(
-      DEFAULT_FLAPPING_SETTINGS,
-      false,
-      'default',
-      0,
-      {},
-      {},
-      {
-        // recovered alerts
-        '1': alert1,
-        '2': alert2,
-        '3': alert3,
-      },
-      {
-        // current recovered alerts
-        '1': alert1,
-        '2': alert2,
-        '3': alert3,
-      }
-    );
-
-    expect(newAlerts).toMatchInlineSnapshot(`Object {}`);
-    expect(alertsWithAnyUUID(activeAlerts)).toMatchInlineSnapshot(`
-      Object {
-        "3": Object {
-          "meta": Object {
-            "activeCount": 0,
-            "flapping": true,
-            "flappingHistory": Array [],
-            "maintenanceWindowIds": Array [],
-            "pendingRecoveredCount": 1,
-            "uuid": Any<String>,
-          },
-          "state": Object {},
-        },
-      }
-    `);
-    expect(Object.values(activeAlerts).map((a) => a.getScheduledActionOptions()))
-      .toMatchInlineSnapshot(`
-      Array [
-        Object {
-          "actionGroup": "default",
-          "context": Object {},
-          "state": Object {},
-        },
-      ]
-    `);
-    expect(currentActiveAlerts).toMatchInlineSnapshot(`Object {}`);
-    expect(
-      Object.values(currentActiveAlerts).map((a) => a.getScheduledActionOptions())
-    ).toMatchInlineSnapshot(`Array []`);
-    expect(alertsWithAnyUUID(recoveredAlerts)).toMatchInlineSnapshot(`
-      Object {
-        "1": Object {
-          "meta": Object {
-            "activeCount": 0,
-            "flapping": true,
-            "flappingHistory": Array [],
-            "maintenanceWindowIds": Array [],
-            "pendingRecoveredCount": 0,
-            "uuid": Any<String>,
-          },
-          "state": Object {},
-        },
-        "2": Object {
-          "meta": Object {
-            "activeCount": 0,
-            "flapping": false,
-            "flappingHistory": Array [],
-            "maintenanceWindowIds": Array [],
-            "uuid": Any<String>,
-          },
-          "state": Object {},
-        },
-      }
-    `);
-    expect(alertsWithAnyUUID(currentRecoveredAlerts)).toMatchInlineSnapshot(`
-      Object {
-        "1": Object {
-          "meta": Object {
-            "activeCount": 0,
-            "flapping": true,
-            "flappingHistory": Array [],
-            "maintenanceWindowIds": Array [],
-            "pendingRecoveredCount": 0,
-            "uuid": Any<String>,
-          },
-          "state": Object {},
-        },
-        "2": Object {
-          "meta": Object {
-            "activeCount": 0,
-            "flapping": false,
-            "flappingHistory": Array [],
-            "maintenanceWindowIds": Array [],
-            "uuid": Any<String>,
-          },
-          "state": Object {},
-        },
-      }
-    `);
-  });
-
   test('should increment activeCount for all active alerts', () => {
     const alert1 = new Alert('1', {
       meta: { activeCount: 1, uuid: 'uuid-1' },
@@ -470,7 +353,6 @@ describe('getAlertsForNotification', () => {
     const { newAlerts, activeAlerts, currentActiveAlerts, delayedAlertsCount } =
       getAlertsForNotification(
         DEFAULT_FLAPPING_SETTINGS,
-        true,
         'default',
         0,
         {
@@ -557,7 +439,6 @@ describe('getAlertsForNotification', () => {
     const { recoveredAlerts, currentRecoveredAlerts, delayedAlertsCount } =
       getAlertsForNotification(
         DEFAULT_FLAPPING_SETTINGS,
-        true,
         'default',
         0,
         {},
@@ -630,7 +511,6 @@ describe('getAlertsForNotification', () => {
     const { newAlerts, activeAlerts, currentActiveAlerts, delayedAlertsCount } =
       getAlertsForNotification(
         DEFAULT_FLAPPING_SETTINGS,
-        true,
         'default',
         5,
         {
@@ -683,7 +563,6 @@ describe('getAlertsForNotification', () => {
     const { recoveredAlerts, currentRecoveredAlerts, delayedAlertsCount } =
       getAlertsForNotification(
         DEFAULT_FLAPPING_SETTINGS,
-        true,
         'default',
         5,
         {},
@@ -710,7 +589,6 @@ describe('getAlertsForNotification', () => {
     const { newAlerts, activeAlerts, currentActiveAlerts, delayedAlertsCount } =
       getAlertsForNotification(
         DEFAULT_FLAPPING_SETTINGS,
-        true,
         'default',
         1,
         {},

diff --git a/x-pack/plugins/alerting/server/lib/get_alerts_for_notification.ts b/x-pack/plugins/alerting/server/lib/get_alerts_for_notification.ts
@@ -17,7 +17,6 @@ export function getAlertsForNotification<
   RecoveryActionGroupId extends string
 >(
   flappingSettings: RulesSettingsFlappingProperties,
-  notifyOnActionGroupChange: boolean,
   actionGroupId: string,
   alertDelay: number,
   newAlerts: Record<string, Alert<State, Context, ActionGroupIds>> = {},
@@ -83,12 +82,7 @@ export function getAlertsForNotification<
             context
           );
           activeAlerts[id] = newAlert;
-
-          // rule with "on status change" or rule with at least one
-          // action with "on status change" should return notifications
-          if (notifyOnActionGroupChange) {
-            currentActiveAlerts[id] = newAlert;
-          }
+          currentActiveAlerts[id] = newAlert;
 
           // remove from recovered alerts
           delete recoveredAlerts[id];

diff --git a/x-pack/plugins/alerting/server/task_runner/rule_type_runner.test.ts b/x-pack/plugins/alerting/server/task_runner/rule_type_runner.test.ts
@@ -247,7 +247,6 @@ describe('RuleTypeRunner', () => {
       expect(ruleRunMetricsStore.setSearchMetrics).toHaveBeenCalled();
       expect(alertsClient.processAlerts).toHaveBeenCalledWith({
         flappingSettings: DEFAULT_FLAPPING_SETTINGS,
-        notifyOnActionGroupChange: false,
         maintenanceWindowIds: [],
         alertDelay: 0,
         ruleRunMetricsStore,
@@ -353,7 +352,6 @@ describe('RuleTypeRunner', () => {
       expect(ruleRunMetricsStore.setSearchMetrics).toHaveBeenCalled();
       expect(alertsClient.processAlerts).toHaveBeenCalledWith({
         flappingSettings: DEFAULT_FLAPPING_SETTINGS,
-        notifyOnActionGroupChange: false,
         maintenanceWindowIds: [],
         alertDelay: 0,
         ruleRunMetricsStore,
@@ -415,7 +413,6 @@ describe('RuleTypeRunner', () => {
       expect(ruleRunMetricsStore.setSearchMetrics).toHaveBeenCalled();
       expect(alertsClient.processAlerts).toHaveBeenCalledWith({
         flappingSettings: DEFAULT_FLAPPING_SETTINGS,
-        notifyOnActionGroupChange: false,
         maintenanceWindowIds: [],
         alertDelay: 0,
         ruleRunMetricsStore,
@@ -761,7 +758,6 @@ describe('RuleTypeRunner', () => {
       expect(ruleRunMetricsStore.setSearchMetrics).toHaveBeenCalled();
       expect(alertsClient.processAlerts).toHaveBeenCalledWith({
         flappingSettings: DEFAULT_FLAPPING_SETTINGS,
-        notifyOnActionGroupChange: false,
         maintenanceWindowIds: [],
         alertDelay: 0,
         ruleRunMetricsStore,
@@ -874,7 +870,6 @@ describe('RuleTypeRunner', () => {
       expect(ruleRunMetricsStore.setSearchMetrics).toHaveBeenCalled();
       expect(alertsClient.processAlerts).toHaveBeenCalledWith({
         flappingSettings: DEFAULT_FLAPPING_SETTINGS,
-        notifyOnActionGroupChange: false,
         maintenanceWindowIds: [],
         alertDelay: 0,
         ruleRunMetricsStore,
@@ -981,7 +976,6 @@ describe('RuleTypeRunner', () => {
       expect(ruleRunMetricsStore.setSearchMetrics).toHaveBeenCalled();
       expect(alertsClient.processAlerts).toHaveBeenCalledWith({
         flappingSettings: DEFAULT_FLAPPING_SETTINGS,
-        notifyOnActionGroupChange: false,
         maintenanceWindowIds: [],
         alertDelay: 0,
         ruleRunMetricsStore,
@@ -1084,7 +1078,6 @@ describe('RuleTypeRunner', () => {
       expect(ruleRunMetricsStore.setSearchMetrics).toHaveBeenCalled();
       expect(alertsClient.processAlerts).toHaveBeenCalledWith({
         flappingSettings: DEFAULT_FLAPPING_SETTINGS,
-        notifyOnActionGroupChange: false,
         maintenanceWindowIds: [],
         alertDelay: 0,
         ruleRunMetricsStore,
@@ -1187,7 +1180,6 @@ describe('RuleTypeRunner', () => {
       expect(ruleRunMetricsStore.setSearchMetrics).toHaveBeenCalled();
       expect(alertsClient.processAlerts).toHaveBeenCalledWith({
         flappingSettings: DEFAULT_FLAPPING_SETTINGS,
-        notifyOnActionGroupChange: false,
         maintenanceWindowIds: [],
         alertDelay: 0,
         ruleRunMetricsStore,

diff --git a/x-pack/plugins/alerting/server/task_runner/rule_type_runner.ts b/x-pack/plugins/alerting/server/task_runner/rule_type_runner.ts
@@ -13,7 +13,6 @@ import {
   createTaskRunError,
   TaskErrorSource,
 } from '@kbn/task-manager-plugin/server';
-import { some } from 'lodash';
 import { getErrorSource } from '@kbn/task-manager-plugin/server/task_running';
 import { IAlertsClient } from '../alerts_client/types';
 import { MaintenanceWindow } from '../application/maintenance_window/types';
@@ -24,7 +23,6 @@ import {
   DEFAULT_FLAPPING_SETTINGS,
   RuleAlertData,
   RuleExecutionStatusErrorReasons,
-  RuleNotifyWhen,
   RuleTypeParams,
   RuleTypeState,
   SanitizedRule,
@@ -323,9 +321,6 @@ export class RuleTypeRunner<
     await this.options.timer.runWithTimer(TaskRunnerTimerSpan.ProcessAlerts, async () => {
       alertsClient.processAlerts({
         flappingSettings: context.flappingSettings ?? DEFAULT_FLAPPING_SETTINGS,
-        notifyOnActionGroupChange:
-          notifyWhen === RuleNotifyWhen.CHANGE ||
-          some(actions, (action) => action.frequency?.notifyWhen === RuleNotifyWhen.CHANGE),
         maintenanceWindowIds: maintenanceWindowsWithoutScopedQueryIds,
         alertDelay: alertDelay?.active ?? 0,
         ruleRunMetricsStore: context.ruleRunMetricsStore,

diff --git a/x-pack/plugins/alerting/server/task_runner/task_runner_alerts_client.test.ts b/x-pack/plugins/alerting/server/task_runner/task_runner_alerts_client.test.ts
@@ -815,7 +815,6 @@ describe('Task Runner', () => {
       expect(alertsClientNotToUse.checkLimitUsage).not.toHaveBeenCalled();
 
       expect(alertsClientToUse.processAlerts).toHaveBeenCalledWith({
-        notifyOnActionGroupChange: false,
         alertDelay: 0,
         flappingSettings: {
           enabled: true,