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

Added ability to fire actions when an alert instance is resolved #82799

Merged
merged 11 commits into from
Nov 14, 2020
14 changes: 14 additions & 0 deletions x-pack/plugins/alerts/common/builtin_action_groups.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import { i18n } from '@kbn/i18n';
import { ActionGroup } from './alert_type';

export const ResolvedActionGroup: ActionGroup = {
id: 'resolved',
name: i18n.translate('xpack.alerts.builtinActionGroups.resolved', {
defaultMessage: 'Resolved',
}),
};
6 changes: 1 addition & 5 deletions x-pack/plugins/alerts/common/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,7 @@ export * from './alert_instance';
export * from './alert_task_instance';
export * from './alert_navigation';
export * from './alert_instance_summary';

export interface ActionGroup {
id: string;
name: string;
}
YulNaumenko marked this conversation as resolved.
Show resolved Hide resolved
export * from './builtin_action_groups';

export interface AlertingFrameworkHealth {
isSufficientlySecure: boolean;
Expand Down
35 changes: 35 additions & 0 deletions x-pack/plugins/alerts/server/alert_type_registry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,33 @@ describe('register()', () => {
);
});

test('throws if AlertType action groups contains reserved group id', () => {
const alertType = {
id: 'test',
name: 'Test',
actionGroups: [
{
id: 'default',
name: 'Default',
},
{
id: 'resolved',
name: 'Resolved',
},
],
defaultActionGroupId: 'default',
executor: jest.fn(),
producer: 'alerts',
};
const registry = new AlertTypeRegistry(alertTypeRegistryParams);

expect(() => registry.register(alertType)).toThrowError(
new Error(
`Alert type by id ${alertType.id} cannot be registered. Action groups [resolved] is reserved by framework.`
)
);
});

test('registers the executor with the task manager', () => {
const alertType = {
id: 'test',
Expand Down Expand Up @@ -201,6 +228,10 @@ describe('get()', () => {
"id": "default",
"name": "Default",
},
Object {
"id": "resolved",
"name": "Resolved",
},
],
"actionVariables": Object {
"context": Array [],
Expand Down Expand Up @@ -255,6 +286,10 @@ describe('list()', () => {
"id": "testActionGroup",
"name": "Test Action Group",
},
Object {
"id": "resolved",
"name": "Resolved",
},
],
"actionVariables": Object {
"context": Array [],
Expand Down
28 changes: 28 additions & 0 deletions x-pack/plugins/alerts/server/alert_type_registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import Boom from '@hapi/boom';
import { i18n } from '@kbn/i18n';
import { schema } from '@kbn/config-schema';
import typeDetect from 'type-detect';
import { intersection } from 'lodash';
import { RunContext, TaskManagerSetupContract } from '../../task_manager/server';
import { TaskRunnerFactory } from './task_runner';
import {
Expand All @@ -16,7 +17,9 @@ import {
AlertTypeState,
AlertInstanceState,
AlertInstanceContext,
ActionGroup,
} from './types';
import { ResolvedActionGroup } from '../common';

interface ConstructorOptions {
taskManager: TaskManagerSetupContract;
Expand Down Expand Up @@ -82,6 +85,8 @@ export class AlertTypeRegistry {
);
}
alertType.actionVariables = normalizedActionVariables(alertType.actionVariables);
validateActionGroups(alertType.id, alertType.actionGroups);
alertType.actionGroups = [...alertType.actionGroups, ...getBuiltinActionGroups()];
YulNaumenko marked this conversation as resolved.
Show resolved Hide resolved
this.alertTypes.set(alertIdSchema.validate(alertType.id), { ...alertType } as AlertType);
this.taskManager.registerTaskDefinitions({
[`alerting:${alertType.id}`]: {
Expand Down Expand Up @@ -137,3 +142,26 @@ function normalizedActionVariables(actionVariables: AlertType['actionVariables']
params: actionVariables?.params ?? [],
};
}

function validateActionGroups(alertTypeId: string, actionGroups: ActionGroup[]) {
const reservedActionGroups = intersection(
actionGroups.map((item) => item.id),
getBuiltinActionGroups().map((item) => item.id)
);
if (reservedActionGroups.length > 0) {
throw Boom.badRequest(
YulNaumenko marked this conversation as resolved.
Show resolved Hide resolved
i18n.translate('xpack.alerts.alertTypeRegistry.register.reservedActionGroupUsageError', {
defaultMessage:
'Alert type by id {alertTypeId} cannot be registered. Action groups [{actionGroups}] is reserved by framework.',
YulNaumenko marked this conversation as resolved.
Show resolved Hide resolved
values: {
actionGroups: reservedActionGroups.join(', '),
alertTypeId,
},
})
);
}
}

function getBuiltinActionGroups(): ActionGroup[] {
YulNaumenko marked this conversation as resolved.
Show resolved Hide resolved
return [ResolvedActionGroup];
}
87 changes: 84 additions & 3 deletions x-pack/plugins/alerts/server/task_runner/task_runner.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,12 @@ import { alertsMock, alertsClientMock } from '../mocks';
import { eventLoggerMock } from '../../../event_log/server/event_logger.mock';
import { IEventLogger } from '../../../event_log/server';
import { SavedObjectsErrorHelpers } from '../../../../../src/core/server';
import { Alert } from '../../common';
import { Alert, ResolvedActionGroup } from '../../common';
import { omit } from 'lodash';
const alertType = {
id: 'test',
name: 'My test alert',
actionGroups: [{ id: 'default', name: 'Default' }],
actionGroups: [{ id: 'default', name: 'Default' }, ResolvedActionGroup],
defaultActionGroupId: 'default',
executor: jest.fn(),
producer: 'alerts',
Expand Down Expand Up @@ -91,7 +91,7 @@ describe('Task Runner', () => {
throttle: null,
muteAll: false,
enabled: true,
alertTypeId: '123',
alertTypeId: alertType.id,
apiKey: '',
apiKeyOwner: 'elastic',
schedule: { interval: '10s' },
Expand All @@ -112,6 +112,14 @@ describe('Task Runner', () => {
foo: true,
},
},
{
group: ResolvedActionGroup.id,
id: '2',
actionTypeId: 'action',
params: {
isResolved: true,
},
},
],
executionStatus: {
status: 'unknown',
Expand Down Expand Up @@ -489,6 +497,79 @@ describe('Task Runner', () => {
`);
});

test('fire resolved actions for execution for the alertInstances which is in the resolved state', async () => {
taskRunnerFactoryInitializerParams.actionsPlugin.isActionTypeEnabled.mockReturnValue(true);
taskRunnerFactoryInitializerParams.actionsPlugin.isActionExecutable.mockReturnValue(true);

alertType.executor.mockImplementation(
({ services: executorServices }: AlertExecutorOptions) => {
executorServices.alertInstanceFactory('1').scheduleActions('default');
}
);
const taskRunner = new TaskRunner(
alertType,
{
...mockedTaskInstance,
state: {
...mockedTaskInstance.state,
alertInstances: {
'1': { meta: {}, state: { bar: false } },
'2': { meta: {}, state: { bar: false } },
},
},
},
taskRunnerFactoryInitializerParams
);
alertsClient.get.mockResolvedValue(mockedAlertTypeSavedObject);
encryptedSavedObjectsClient.getDecryptedAsInternalUser.mockResolvedValue({
id: '1',
type: 'alert',
attributes: {
apiKey: Buffer.from('123:abc').toString('base64'),
},
references: [],
});
const runnerResult = await taskRunner.run();
expect(runnerResult.state.alertInstances).toMatchInlineSnapshot(`
Object {
"1": Object {
"meta": Object {
"lastScheduledActions": Object {
"date": 1970-01-01T00:00:00.000Z,
"group": "default",
},
},
"state": Object {
"bar": false,
},
},
}
`);

const eventLogger = taskRunnerFactoryInitializerParams.eventLogger;
expect(eventLogger.logEvent).toHaveBeenCalledTimes(5);
expect(actionsClient.enqueueExecution).toHaveBeenCalledTimes(2);
expect(actionsClient.enqueueExecution.mock.calls[0]).toMatchInlineSnapshot(`
Array [
Object {
"apiKey": "MTIzOmFiYw==",
"id": "2",
"params": Object {
"isResolved": true,
},
"source": Object {
"source": Object {
"id": "1",
"type": "alert",
},
"type": "SAVED_OBJECT",
},
"spaceId": undefined,
},
]
`);
});

test('persists alertInstances passed in from state, only if they are scheduled for execution', async () => {
alertType.executor.mockImplementation(
({ services: executorServices }: AlertExecutorOptions) => {
Expand Down
38 changes: 38 additions & 0 deletions x-pack/plugins/alerts/server/task_runner/task_runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import { IEvent, IEventLogger, SAVED_OBJECT_REL_PRIMARY } from '../../../event_l
import { isAlertSavedObjectNotFoundError } from '../lib/is_alert_not_found_error';
import { AlertsClient } from '../alerts_client';
import { partiallyUpdateAlert } from '../saved_objects';
import { ResolvedActionGroup } from '../../common';

const FALLBACK_RETRY_INTERVAL = '5m';

Expand Down Expand Up @@ -225,6 +226,14 @@ export class TaskRunner {
alertInstance.hasScheduledActions()
);
const currentAlertInstanceIds = Object.keys(instancesWithScheduledActions);

scheduleActionsForResolvedInstances(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this will send a notification for a resolved instance event if the alert instance is muted (or if all instances are muted). Is that the expected behavior?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, we shouldn't be scheduling actions if the instance or alert is throttled/muted. We may want to make that check check a reusable function somewhere, wherever it's done today, so other call sites (like this one), can use it. Either that, or figure out how to include these new actions to be scheduled in the same place they are done today - sorry I haven't looked at where these places might be yet, but can poke around a bit more if you want - LMK.

Copy link
Contributor

@mikecote mikecote Nov 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, we shouldn't be scheduling actions if the instance or alert is throttled/muted

I believe only muted since throttle resets when action group changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch! By the way, I'm not sure about the reusable function here - we need to identify if the alert is muted to skip the whole scheduleActionsForResolvedInstances call and passing down the list of mutedInstanceIds to skip it in the schedule function as well. I think we already have this information on the current function level about the specific alert. Or we should get here the most fresh by calling alertsClient get by alertId?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested it with the UI PR part using PagerDuty incidents and it works fine now with handling the mute state. Will provide the api integration test as well

alertInstances,
executionHandler,
originalAlertInstanceIds,
currentAlertInstanceIds
);

generateNewAndResolvedInstanceEvents({
eventLogger,
originalAlertInstanceIds,
Expand Down Expand Up @@ -439,6 +448,35 @@ function generateNewAndResolvedInstanceEvents(params: GenerateNewAndResolvedInst
}
}

function scheduleActionsForResolvedInstances(
alertInstancesMap: {
[x: string]: AlertInstance<
{
[x: string]: unknown;
},
{
[x: string]: unknown;
}
>;
},
YulNaumenko marked this conversation as resolved.
Show resolved Hide resolved
executionHandler: ReturnType<typeof createExecutionHandler>,
originalAlertInstanceIds: string[],
currentAlertInstanceIds: string[]
) {
const resolvedIds = without(originalAlertInstanceIds, ...currentAlertInstanceIds);
for (const id of resolvedIds) {
alertInstancesMap[id].updateLastScheduledActions(ResolvedActionGroup.id);
alertInstancesMap[id].unscheduleActions();
executionHandler({
actionGroup: ResolvedActionGroup.id,
context: {},
state: {},
Comment on lines +512 to +513
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if we're going to find the empty context and state to be a problem 🤔
Should we perhaps remove the values expected in context and stat from the template variables in the UI?
Otherwise users might try to use them in actions when attaching to Resolved, no?

Copy link
Contributor Author

@YulNaumenko YulNaumenko Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it in the UI PR

alertInstanceId: id,
});
alertInstancesMap[id].scheduleActions(ResolvedActionGroup.id);
}
}

/**
* If an error is thrown, wrap it in an AlertTaskRunResult
* so that we can treat each field independantly
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,9 @@ export async function createAlert({
'createdBy' | 'updatedBy' | 'muteAll' | 'mutedInstanceIds' | 'executionStatus'
>;
}): Promise<Alert> {
if (alert.actions.length === 2) {
alert.actions[1].group = 'resolved';
}
YulNaumenko marked this conversation as resolved.
Show resolved Hide resolved
return await http.post(`${BASE_ALERT_API_PATH}/alert`, {
body: JSON.stringify(alert),
});
Expand Down
Loading