Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip scheduling actions for the alerts without scheduledActions #195948

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions x-pack/plugins/alerting/server/alerts_client/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,11 @@ export interface IAlertsClient<
processAlerts(opts: ProcessAlertsOpts): void;
logAlerts(opts: LogAlertsOpts): void;
getProcessedAlerts(
type: 'new' | 'active' | 'activeCurrent' | 'recovered' | 'recoveredCurrent'
): Record<string, LegacyAlert<State, Context, ActionGroupIds | RecoveryActionGroupId>>;
type: 'new' | 'active' | 'activeCurrent'
): Record<string, LegacyAlert<State, Context, ActionGroupIds>>;
getProcessedAlerts(
type: 'recovered' | 'recoveredCurrent'
): Record<string, LegacyAlert<State, Context, RecoveryActionGroupId>>;
persistAlerts(): Promise<{ alertIds: string[]; maintenanceWindowIds: string[] } | null>;
isTrackedAlert(id: string): boolean;
getSummarizedAlerts?(params: GetSummarizedAlertsParams): Promise<SummarizedAlerts>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,13 @@ export class ActionScheduler<
this.schedulers.sort((a, b) => a.priority - b.priority);
}

public async run(
alerts: Record<string, Alert<State, Context, ActionGroupIds | RecoveryActionGroupId>>
): Promise<RunResult> {
public async run({
activeCurrentAlerts,
recoveredCurrentAlerts,
}: {
activeCurrentAlerts: Record<string, Alert<State, Context, ActionGroupIds>>;
recoveredCurrentAlerts: Record<string, Alert<State, Context, RecoveryActionGroupId>>;
}): Promise<RunResult> {
const throttledSummaryActions: ThrottledActions = getSummaryActionsFromTaskState({
actions: this.context.rule.actions,
summaryActions: this.context.taskInstance.state?.summaryActions,
Expand All @@ -85,7 +89,11 @@ export class ActionScheduler<
const allActionsToScheduleResult: ActionsToSchedule[] = [];
for (const scheduler of this.schedulers) {
allActionsToScheduleResult.push(
...(await scheduler.getActionsToSchedule({ alerts, throttledSummaryActions }))
...(await scheduler.getActionsToSchedule({
activeCurrentAlerts,
recoveredCurrentAlerts,
throttledSummaryActions,
}))
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,10 @@ describe('Per-Alert Action Scheduler', () => {
test('should create action to schedule for each alert and each action', async () => {
// 2 per-alert actions * 2 alerts = 4 actions to schedule
const scheduler = new PerAlertActionScheduler(getSchedulerContext());
const results = await scheduler.getActionsToSchedule({ alerts });
const results = await scheduler.getActionsToSchedule({
activeCurrentAlerts: alerts,
recoveredCurrentAlerts: {},
});

expect(alertsClient.getSummarizedAlerts).not.toHaveBeenCalled();
expect(logger.debug).not.toHaveBeenCalled();
Expand Down Expand Up @@ -243,7 +246,10 @@ describe('Per-Alert Action Scheduler', () => {
maintenanceWindowIds: ['mw-1'],
});
const alertsWithMaintenanceWindow = { ...newAlertWithMaintenanceWindow, ...newAlert2 };
const results = await scheduler.getActionsToSchedule({ alerts: alertsWithMaintenanceWindow });
const results = await scheduler.getActionsToSchedule({
activeCurrentAlerts: alertsWithMaintenanceWindow,
recoveredCurrentAlerts: {},
});

expect(alertsClient.getSummarizedAlerts).not.toHaveBeenCalled();
expect(logger.debug).toHaveBeenCalledTimes(2);
Expand Down Expand Up @@ -281,7 +287,8 @@ describe('Per-Alert Action Scheduler', () => {
});
const alertsWithInvalidActionGroup = { ...newAlertInvalidActionGroup, ...newAlert2 };
const results = await scheduler.getActionsToSchedule({
alerts: alertsWithInvalidActionGroup,
activeCurrentAlerts: alertsWithInvalidActionGroup,
recoveredCurrentAlerts: {},
});

expect(alertsClient.getSummarizedAlerts).not.toHaveBeenCalled();
Expand Down Expand Up @@ -309,6 +316,36 @@ describe('Per-Alert Action Scheduler', () => {
]);
});

test('should skip creating actions to schedule when alert has no scheduled actions', async () => {
// 2 per-alert actions * 2 alerts = 4 actions to schedule
// but alert 1 has has no scheduled actions, so only actions for alert 2 should be scheduled
const scheduler = new PerAlertActionScheduler(getSchedulerContext());
const newAlertInvalidActionGroup = generateAlert({
id: 1,
scheduleActions: false,
});
const alertsWithInvalidActionGroup = { ...newAlertInvalidActionGroup, ...newAlert2 };
const results = await scheduler.getActionsToSchedule({
activeCurrentAlerts: alertsWithInvalidActionGroup,
recoveredCurrentAlerts: {},
});

expect(alertsClient.getSummarizedAlerts).not.toHaveBeenCalled();

expect(ruleRunMetricsStore.getNumberOfGeneratedActions()).toEqual(2);
expect(ruleRunMetricsStore.getNumberOfTriggeredActions()).toEqual(2);
expect(ruleRunMetricsStore.getStatusByConnectorType('test')).toEqual({
numberOfGeneratedActions: 2,
numberOfTriggeredActions: 2,
});

expect(results).toHaveLength(2);
expect(results).toEqual([
getResult('action-1', '2', '111-111'),
getResult('action-2', '2', '222-222'),
]);
});

test('should skip creating actions to schedule when alert has pending recovered count greater than 0 and notifyWhen is onActiveAlert', async () => {
// 2 per-alert actions * 2 alerts = 4 actions to schedule
// but alert 1 has a pending recovered count > 0 & notifyWhen is onActiveAlert, so only actions for alert 2 should be scheduled
Expand All @@ -322,7 +359,8 @@ describe('Per-Alert Action Scheduler', () => {
...newAlert2,
};
const results = await scheduler.getActionsToSchedule({
alerts: alertsWithPendingRecoveredCount,
activeCurrentAlerts: alertsWithPendingRecoveredCount,
recoveredCurrentAlerts: {},
});

expect(alertsClient.getSummarizedAlerts).not.toHaveBeenCalled();
Expand Down Expand Up @@ -368,7 +406,8 @@ describe('Per-Alert Action Scheduler', () => {
...newAlert2,
};
const results = await scheduler.getActionsToSchedule({
alerts: alertsWithPendingRecoveredCount,
activeCurrentAlerts: alertsWithPendingRecoveredCount,
recoveredCurrentAlerts: {},
});

expect(alertsClient.getSummarizedAlerts).not.toHaveBeenCalled();
Expand All @@ -394,7 +433,10 @@ describe('Per-Alert Action Scheduler', () => {
...getSchedulerContext(),
rule: { ...rule, mutedInstanceIds: ['2'] },
});
const results = await scheduler.getActionsToSchedule({ alerts });
const results = await scheduler.getActionsToSchedule({
activeCurrentAlerts: alerts,
recoveredCurrentAlerts: {},
});

expect(alertsClient.getSummarizedAlerts).not.toHaveBeenCalled();
expect(logger.debug).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -453,7 +495,10 @@ describe('Per-Alert Action Scheduler', () => {
rule: { ...rule, actions: [rule.actions[0], onActionGroupChangeAction] },
});

const results = await scheduler.getActionsToSchedule({ alerts: alertsWithOngoingAlert });
const results = await scheduler.getActionsToSchedule({
activeCurrentAlerts: alertsWithOngoingAlert,
recoveredCurrentAlerts: {},
});

expect(alertsClient.getSummarizedAlerts).not.toHaveBeenCalled();
expect(logger.debug).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -508,7 +553,10 @@ describe('Per-Alert Action Scheduler', () => {
rule: { ...rule, actions: [rule.actions[0], onThrottleIntervalAction] },
});

const results = await scheduler.getActionsToSchedule({ alerts: alertsWithOngoingAlert });
const results = await scheduler.getActionsToSchedule({
activeCurrentAlerts: alertsWithOngoingAlert,
recoveredCurrentAlerts: {},
});

expect(alertsClient.getSummarizedAlerts).not.toHaveBeenCalled();
expect(logger.debug).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -563,7 +611,10 @@ describe('Per-Alert Action Scheduler', () => {
rule: { ...rule, actions: [rule.actions[0], onThrottleIntervalAction] },
});

const results = await scheduler.getActionsToSchedule({ alerts: alertsWithOngoingAlert });
const results = await scheduler.getActionsToSchedule({
activeCurrentAlerts: alertsWithOngoingAlert,
recoveredCurrentAlerts: {},
});

expect(alertsClient.getSummarizedAlerts).not.toHaveBeenCalled();
expect(logger.debug).not.toHaveBeenCalled();
Expand Down Expand Up @@ -620,7 +671,10 @@ describe('Per-Alert Action Scheduler', () => {
...getSchedulerContext(),
rule: { ...rule, actions: [rule.actions[0], actionWithUseAlertDataForTemplate] },
});
const results = await scheduler.getActionsToSchedule({ alerts });
const results = await scheduler.getActionsToSchedule({
activeCurrentAlerts: alerts,
recoveredCurrentAlerts: {},
});

expect(alertsClient.getSummarizedAlerts).toHaveBeenCalledTimes(1);
expect(alertsClient.getSummarizedAlerts).toHaveBeenCalledWith({
Expand Down Expand Up @@ -679,7 +733,10 @@ describe('Per-Alert Action Scheduler', () => {
...getSchedulerContext(),
rule: { ...rule, actions: [rule.actions[0], actionWithUseAlertDataForTemplate] },
});
const results = await scheduler.getActionsToSchedule({ alerts });
const results = await scheduler.getActionsToSchedule({
activeCurrentAlerts: alerts,
recoveredCurrentAlerts: {},
});

expect(alertsClient.getSummarizedAlerts).toHaveBeenCalledTimes(1);
expect(alertsClient.getSummarizedAlerts).toHaveBeenCalledWith({
Expand Down Expand Up @@ -739,7 +796,10 @@ describe('Per-Alert Action Scheduler', () => {
...getSchedulerContext(),
rule: { ...rule, actions: [rule.actions[0], actionWithAlertsFilter] },
});
const results = await scheduler.getActionsToSchedule({ alerts });
const results = await scheduler.getActionsToSchedule({
activeCurrentAlerts: alerts,
recoveredCurrentAlerts: {},
});

expect(alertsClient.getSummarizedAlerts).toHaveBeenCalledTimes(1);
expect(alertsClient.getSummarizedAlerts).toHaveBeenCalledWith({
Expand Down Expand Up @@ -799,7 +859,10 @@ describe('Per-Alert Action Scheduler', () => {
...getSchedulerContext(),
rule: { ...rule, actions: [rule.actions[0], actionWithAlertsFilter] },
});
const results = await scheduler.getActionsToSchedule({ alerts });
const results = await scheduler.getActionsToSchedule({
activeCurrentAlerts: alerts,
recoveredCurrentAlerts: {},
});

expect(alertsClient.getSummarizedAlerts).toHaveBeenCalledTimes(1);
expect(alertsClient.getSummarizedAlerts).toHaveBeenCalledWith({
Expand Down Expand Up @@ -860,7 +923,10 @@ describe('Per-Alert Action Scheduler', () => {
...getSchedulerContext(),
rule: { ...rule, actions: [rule.actions[0], actionWithAlertsFilter] },
});
const results = await scheduler.getActionsToSchedule({ alerts });
const results = await scheduler.getActionsToSchedule({
activeCurrentAlerts: alerts,
recoveredCurrentAlerts: {},
});

expect(alertsClient.getSummarizedAlerts).toHaveBeenCalledTimes(1);
expect(alertsClient.getSummarizedAlerts).toHaveBeenCalledWith({
Expand Down Expand Up @@ -919,7 +985,10 @@ describe('Per-Alert Action Scheduler', () => {
...getSchedulerContext(),
rule: { ...rule, actions: [rule.actions[0], actionWithAlertsFilter] },
});
const results = await scheduler.getActionsToSchedule({ alerts });
const results = await scheduler.getActionsToSchedule({
activeCurrentAlerts: alerts,
recoveredCurrentAlerts: {},
});

expect(alertsClient.getSummarizedAlerts).toHaveBeenCalledTimes(1);
expect(alertsClient.getSummarizedAlerts).toHaveBeenCalledWith({
Expand Down Expand Up @@ -960,7 +1029,10 @@ describe('Per-Alert Action Scheduler', () => {
},
},
});
const results = await scheduler.getActionsToSchedule({ alerts });
const results = await scheduler.getActionsToSchedule({
activeCurrentAlerts: alerts,
recoveredCurrentAlerts: {},
});

expect(alertsClient.getSummarizedAlerts).not.toHaveBeenCalled();

Expand Down Expand Up @@ -996,7 +1068,10 @@ describe('Per-Alert Action Scheduler', () => {
},
},
});
const results = await scheduler.getActionsToSchedule({ alerts });
const results = await scheduler.getActionsToSchedule({
activeCurrentAlerts: alerts,
recoveredCurrentAlerts: {},
});

expect(alertsClient.getSummarizedAlerts).not.toHaveBeenCalled();

Expand Down Expand Up @@ -1029,7 +1104,10 @@ describe('Per-Alert Action Scheduler', () => {

expect(alert.getLastScheduledActions()).toBeUndefined();
expect(alert.hasScheduledActions()).toBe(true);
await scheduler.getActionsToSchedule({ alerts: { '1': alert } });
await scheduler.getActionsToSchedule({
activeCurrentAlerts: { '1': alert },
recoveredCurrentAlerts: {},
});

expect(alert.getLastScheduledActions()).toEqual({
date: '1970-01-01T00:00:00.000Z',
Expand Down Expand Up @@ -1066,7 +1144,10 @@ describe('Per-Alert Action Scheduler', () => {
rule: { ...rule, actions: [onThrottleIntervalAction] },
});

await scheduler.getActionsToSchedule({ alerts: { '1': alert } });
await scheduler.getActionsToSchedule({
activeCurrentAlerts: { '1': alert },
recoveredCurrentAlerts: {},
});

expect(alert.getLastScheduledActions()).toEqual({
date: '1970-01-01T00:00:00.000Z',
Expand Down
Loading