diff --git a/x-pack/legacy/plugins/actions/server/action_type_registry.test.ts b/x-pack/legacy/plugins/actions/server/action_type_registry.test.ts index a157e85d2ac2c..0c1643e20d8bf 100644 --- a/x-pack/legacy/plugins/actions/server/action_type_registry.test.ts +++ b/x-pack/legacy/plugins/actions/server/action_type_registry.test.ts @@ -13,6 +13,7 @@ import { encryptedSavedObjectsMock } from '../../encrypted_saved_objects/server/ import { ActionTypeRegistry } from './action_type_registry'; import { ExecutorType } from './types'; import { SavedObjectsClientMock } from '../../../../../src/core/server/mocks'; +import { ExecutorError } from './lib'; const mockTaskManager = taskManagerMock.create(); @@ -50,16 +51,18 @@ describe('register()', () => { expect(actionTypeRegistry.has('my-action-type')).toEqual(true); expect(mockTaskManager.registerTaskDefinitions).toHaveBeenCalledTimes(1); expect(mockTaskManager.registerTaskDefinitions.mock.calls[0]).toMatchInlineSnapshot(` -Array [ - Object { - "actions:my-action-type": Object { - "createTaskRunner": [MockFunction], - "title": "My action type", - "type": "actions:my-action-type", - }, - }, -] -`); + Array [ + Object { + "actions:my-action-type": Object { + "createTaskRunner": [MockFunction], + "getRetry": [Function], + "maxAttempts": 1, + "title": "My action type", + "type": "actions:my-action-type", + }, + }, + ] + `); expect(getCreateTaskRunnerFunction).toHaveBeenCalledTimes(1); const call = getCreateTaskRunnerFunction.mock.calls[0][0]; expect(call.actionTypeRegistry).toBeTruthy(); @@ -86,6 +89,27 @@ Array [ `"Action type \\"my-action-type\\" is already registered."` ); }); + + test('provides a getRetry function that handles ExecutorError', () => { + const actionTypeRegistry = new ActionTypeRegistry(actionTypeRegistryParams); + actionTypeRegistry.register({ + id: 'my-action-type', + name: 'My action type', + unencryptedAttributes: [], + executor, + }); + expect(mockTaskManager.registerTaskDefinitions).toHaveBeenCalledTimes(1); + const registerTaskDefinitionsCall = mockTaskManager.registerTaskDefinitions.mock.calls[0][0]; + const getRetry = registerTaskDefinitionsCall['actions:my-action-type'].getRetry!; + + const retryTime = new Date(); + expect(getRetry(0, new Error())).toEqual(false); + expect(getRetry(0, new ExecutorError('my message', {}, true))).toEqual(true); + expect(getRetry(0, new ExecutorError('my message', {}, false))).toEqual(false); + expect(getRetry(0, new ExecutorError('my message', {}, null))).toEqual(false); + expect(getRetry(0, new ExecutorError('my message', {}, undefined))).toEqual(false); + expect(getRetry(0, new ExecutorError('my message', {}, retryTime))).toEqual(retryTime); + }); }); describe('get()', () => { @@ -99,13 +123,13 @@ describe('get()', () => { }); const actionType = actionTypeRegistry.get('my-action-type'); expect(actionType).toMatchInlineSnapshot(` -Object { - "executor": [Function], - "id": "my-action-type", - "name": "My action type", - "unencryptedAttributes": Array [], -} -`); + Object { + "executor": [Function], + "id": "my-action-type", + "name": "My action type", + "unencryptedAttributes": Array [], + } + `); }); test(`throws an error when action type doesn't exist`, () => { diff --git a/x-pack/legacy/plugins/actions/server/action_type_registry.ts b/x-pack/legacy/plugins/actions/server/action_type_registry.ts index feba76b06b81a..72ba36c402bf9 100644 --- a/x-pack/legacy/plugins/actions/server/action_type_registry.ts +++ b/x-pack/legacy/plugins/actions/server/action_type_registry.ts @@ -8,7 +8,7 @@ import Boom from 'boom'; import { i18n } from '@kbn/i18n'; import { ActionType, GetServicesFunction } from './types'; import { TaskManager, TaskRunCreatorFunction } from '../../task_manager'; -import { getCreateTaskRunnerFunction } from './lib'; +import { getCreateTaskRunnerFunction, ExecutorError } from './lib'; import { EncryptedSavedObjectsPlugin } from '../../encrypted_saved_objects'; interface ConstructorOptions { @@ -61,6 +61,14 @@ export class ActionTypeRegistry { [`actions:${actionType.id}`]: { title: actionType.name, type: `actions:${actionType.id}`, + maxAttempts: actionType.maxAttempts || 1, + getRetry(attempts: number, error: any) { + if (error instanceof ExecutorError) { + return error.retry == null ? false : error.retry; + } + // Don't retry other kinds of errors + return false; + }, createTaskRunner: this.taskRunCreatorFunction, }, }); diff --git a/x-pack/legacy/plugins/actions/server/lib/execute.test.ts b/x-pack/legacy/plugins/actions/server/lib/execute.test.ts index 2d184a36d56ea..ec9ebb2450c7b 100644 --- a/x-pack/legacy/plugins/actions/server/lib/execute.test.ts +++ b/x-pack/legacy/plugins/actions/server/lib/execute.test.ts @@ -132,6 +132,7 @@ test('throws an error when config is invalid', async () => { const result = await execute(executeParams); expect(result).toEqual({ status: 'error', + retry: false, message: `The actionTypeConfig is invalid: [param1]: expected value of type [string] but got [undefined]`, }); }); @@ -163,6 +164,7 @@ test('throws an error when params is invalid', async () => { const result = await execute(executeParams); expect(result).toEqual({ status: 'error', + retry: false, message: `The actionParams is invalid: [param1]: expected value of type [string] but got [undefined]`, }); }); diff --git a/x-pack/legacy/plugins/actions/server/lib/execute.ts b/x-pack/legacy/plugins/actions/server/lib/execute.ts index 899efbaead088..19e683c3386b3 100644 --- a/x-pack/legacy/plugins/actions/server/lib/execute.ts +++ b/x-pack/legacy/plugins/actions/server/lib/execute.ts @@ -43,7 +43,7 @@ export async function execute({ validatedConfig = validateActionTypeConfig(actionType, mergedActionTypeConfig); validatedParams = validateActionTypeParams(actionType, params); } catch (err) { - return { status: 'error', message: err.message }; + return { status: 'error', message: err.message, retry: false }; } let result: ActionTypeExecutorResult | null = null; diff --git a/x-pack/legacy/plugins/actions/server/lib/executor_error.ts b/x-pack/legacy/plugins/actions/server/lib/executor_error.ts new file mode 100644 index 0000000000000..5e0dee3f3cc2d --- /dev/null +++ b/x-pack/legacy/plugins/actions/server/lib/executor_error.ts @@ -0,0 +1,15 @@ +/* + * 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. + */ + +export class ExecutorError extends Error { + readonly data?: any; + readonly retry?: null | boolean | Date; + constructor(message?: string, data?: any, retry?: null | boolean | Date) { + super(message); + this.data = data; + this.retry = retry; + } +} diff --git a/x-pack/legacy/plugins/actions/server/lib/get_create_task_runner_function.test.ts b/x-pack/legacy/plugins/actions/server/lib/get_create_task_runner_function.test.ts index 489325748b4df..d2cc4cb618568 100644 --- a/x-pack/legacy/plugins/actions/server/lib/get_create_task_runner_function.test.ts +++ b/x-pack/legacy/plugins/actions/server/lib/get_create_task_runner_function.test.ts @@ -12,6 +12,7 @@ import { encryptedSavedObjectsMock } from '../../../encrypted_saved_objects/serv import { getCreateTaskRunnerFunction } from './get_create_task_runner_function'; import { SavedObjectsClientMock } from '../../../../../../src/core/server/mocks'; import { actionTypeRegistryMock } from '../action_type_registry.mock'; +import { ExecutorError } from './executor_error'; const actionTypeRegistry = actionTypeRegistryMock.create(); const mockedEncryptedSavedObjectsPlugin = encryptedSavedObjectsMock.create(); @@ -54,6 +55,9 @@ test('executes the task by calling the executor with proper parameters', async ( const { execute: mockExecute } = jest.requireMock('./execute'); const createTaskRunner = getCreateTaskRunnerFunction(getCreateTaskRunnerFunctionParams); const runner = createTaskRunner({ taskInstance: taskInstanceMock }); + + mockExecute.mockResolvedValueOnce({ status: 'ok' }); + const runnerResult = await runner.run(); expect(runnerResult).toBeUndefined(); @@ -66,3 +70,25 @@ test('executes the task by calling the executor with proper parameters', async ( params: { baz: true }, }); }); + +test('throws an error with suggested retry logic when return status is error', async () => { + const { execute: mockExecute } = jest.requireMock('./execute'); + const createTaskRunner = getCreateTaskRunnerFunction(getCreateTaskRunnerFunctionParams); + const runner = createTaskRunner({ taskInstance: taskInstanceMock }); + + mockExecute.mockResolvedValueOnce({ + status: 'error', + message: 'Error message', + data: { foo: true }, + retry: false, + }); + + try { + await runner.run(); + throw new Error('Should have thrown'); + } catch (e) { + expect(e instanceof ExecutorError).toEqual(true); + expect(e.data).toEqual({ foo: true }); + expect(e.retry).toEqual(false); + } +}); diff --git a/x-pack/legacy/plugins/actions/server/lib/get_create_task_runner_function.ts b/x-pack/legacy/plugins/actions/server/lib/get_create_task_runner_function.ts index 04b7345781e3c..eb1c0e8ac8a40 100644 --- a/x-pack/legacy/plugins/actions/server/lib/get_create_task_runner_function.ts +++ b/x-pack/legacy/plugins/actions/server/lib/get_create_task_runner_function.ts @@ -5,6 +5,7 @@ */ import { execute } from './execute'; +import { ExecutorError } from './executor_error'; import { ActionTypeRegistryContract, GetServicesFunction } from '../types'; import { TaskInstance } from '../../../task_manager'; import { EncryptedSavedObjectsPlugin } from '../../../encrypted_saved_objects'; @@ -28,7 +29,7 @@ export function getCreateTaskRunnerFunction({ return { run: async () => { const { namespace, id, actionTypeParams } = taskInstance.params; - await execute({ + const executorResult = await execute({ namespace, actionTypeRegistry, encryptedSavedObjectsPlugin, @@ -36,6 +37,15 @@ export function getCreateTaskRunnerFunction({ services: getServices(taskInstance.params.basePath), params: actionTypeParams, }); + if (executorResult.status === 'error') { + // Task manager error handler only kicks in when an error thrown (at this time) + // So what we have to do is throw when the return status is `error`. + throw new ExecutorError( + executorResult.message, + executorResult.data, + executorResult.retry + ); + } }, }; }; diff --git a/x-pack/legacy/plugins/actions/server/lib/index.ts b/x-pack/legacy/plugins/actions/server/lib/index.ts index 23305f4eba90e..75e4a5595a769 100644 --- a/x-pack/legacy/plugins/actions/server/lib/index.ts +++ b/x-pack/legacy/plugins/actions/server/lib/index.ts @@ -8,3 +8,4 @@ export { execute } from './execute'; export { getCreateTaskRunnerFunction } from './get_create_task_runner_function'; export { validateActionTypeConfig } from './validate_action_type_config'; export { validateActionTypeParams } from './validate_action_type_params'; +export { ExecutorError } from './executor_error'; diff --git a/x-pack/legacy/plugins/actions/server/types.ts b/x-pack/legacy/plugins/actions/server/types.ts index 70f79327ae93e..886c7429e99ca 100644 --- a/x-pack/legacy/plugins/actions/server/types.ts +++ b/x-pack/legacy/plugins/actions/server/types.ts @@ -54,6 +54,7 @@ export interface ActionType { id: string; name: string; unencryptedAttributes: string[]; + maxAttempts?: number; validate?: { params?: { validate: (object: any) => any }; config?: { validate: (object: any) => any }; diff --git a/x-pack/legacy/plugins/alerting/README.md b/x-pack/legacy/plugins/alerting/README.md index 091d5a9da0115..18034807b7ac7 100644 --- a/x-pack/legacy/plugins/alerting/README.md +++ b/x-pack/legacy/plugins/alerting/README.md @@ -52,8 +52,8 @@ This is the primary function for an alert type. Whenever the alert needs to exec |services.callCluster(path, opts)|Use this to do Elasticsearch queries on the cluster Kibana connects to. This function is the same as any other `callCluster` in Kibana.

**NOTE**: This currently authenticates as the Kibana internal user, but will change in a future PR.| |services.savedObjectsClient|This is an instance of the saved objects client. This provides the ability to do CRUD on any saved objects within the same space the alert lives in.

**NOTE**: This currently only works when security is disabled. A future PR will add support for enabled security using Elasticsearch API tokens.| |services.log(tags, [data], [timestamp])|Use this to create server logs. (This is the same function as server.log)| -|scheduledRunAt|The date and time the alert type execution was scheduled to be called.| -|previousScheduledRunAt|The previous date and time the alert type was scheduled to be called.| +|startedAt|The date and time the alert type started execution.| +|previousStartedAt|The previous date and time the alert type started a successful execution.| |params|Parameters for the execution. This is where the parameters you require will be passed in. (example threshold). Use alert type validation to ensure values are set before execution.| |state|State returned from previous execution. This is the alert level state. What the executor returns will be serialized and provided here at the next execution.| @@ -74,8 +74,8 @@ server.plugins.alerting.registerType({ }), }, async executor({ - scheduledRunAt, - previousScheduledRunAt, + startedAt, + previousStartedAt, services, params, state, @@ -131,8 +131,8 @@ server.plugins.alerting.registerType({ }), }, async executor({ - scheduledRunAt, - previousScheduledRunAt, + startedAt, + previousStartedAt, services, params, state, diff --git a/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts b/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts index 4421ea7435c98..8a6ffe7ed4506 100644 --- a/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts +++ b/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts @@ -119,99 +119,98 @@ describe('create()', () => { }); const result = await alertsClient.create({ data }); expect(result).toMatchInlineSnapshot(` - Object { - "actions": Array [ - Object { - "group": "default", - "id": "1", - "params": Object { - "foo": true, - }, - }, - ], - "alertTypeId": "123", - "alertTypeParams": Object { - "bar": true, - }, - "id": "1", - "interval": "10s", - "scheduledTaskId": "task-123", - } - `); + Object { + "actions": Array [ + Object { + "group": "default", + "id": "1", + "params": Object { + "foo": true, + }, + }, + ], + "alertTypeId": "123", + "alertTypeParams": Object { + "bar": true, + }, + "id": "1", + "interval": "10s", + "scheduledTaskId": "task-123", + } + `); expect(savedObjectsClient.create).toHaveBeenCalledTimes(1); expect(savedObjectsClient.create.mock.calls[0]).toHaveLength(3); expect(savedObjectsClient.create.mock.calls[0][0]).toEqual('alert'); expect(savedObjectsClient.create.mock.calls[0][1]).toMatchInlineSnapshot(` - Object { - "actions": Array [ - Object { - "actionRef": "action_0", - "group": "default", - "params": Object { - "foo": true, - }, - }, - ], - "alertTypeId": "123", - "alertTypeParams": Object { - "bar": true, - }, - "enabled": true, - "interval": "10s", - } - `); + Object { + "actions": Array [ + Object { + "actionRef": "action_0", + "group": "default", + "params": Object { + "foo": true, + }, + }, + ], + "alertTypeId": "123", + "alertTypeParams": Object { + "bar": true, + }, + "enabled": true, + "interval": "10s", + } + `); expect(savedObjectsClient.create.mock.calls[0][2]).toMatchInlineSnapshot(` - Object { - "references": Array [ - Object { - "id": "1", - "name": "action_0", - "type": "action", - }, - ], - } - `); + Object { + "references": Array [ + Object { + "id": "1", + "name": "action_0", + "type": "action", + }, + ], + } + `); expect(taskManager.schedule).toHaveBeenCalledTimes(1); expect(taskManager.schedule.mock.calls[0]).toMatchInlineSnapshot(` - Array [ - Object { - "params": Object { - "alertId": "1", - "basePath": "/s/default", - }, - "scope": Array [ - "alerting", - ], - "state": Object { - "alertInstances": Object {}, - "alertTypeState": Object {}, - "previousScheduledRunAt": null, - "scheduledRunAt": 2019-02-12T21:01:22.479Z, - }, - "taskType": "alerting:123", - }, - ] - `); + Array [ + Object { + "params": Object { + "alertId": "1", + "basePath": "/s/default", + }, + "scope": Array [ + "alerting", + ], + "state": Object { + "alertInstances": Object {}, + "alertTypeState": Object {}, + "previousStartedAt": null, + }, + "taskType": "alerting:123", + }, + ] + `); expect(savedObjectsClient.update).toHaveBeenCalledTimes(1); expect(savedObjectsClient.update.mock.calls[0]).toHaveLength(4); expect(savedObjectsClient.update.mock.calls[0][0]).toEqual('alert'); expect(savedObjectsClient.update.mock.calls[0][1]).toEqual('1'); expect(savedObjectsClient.update.mock.calls[0][2]).toMatchInlineSnapshot(` - Object { - "scheduledTaskId": "task-123", - } - `); + Object { + "scheduledTaskId": "task-123", + } + `); expect(savedObjectsClient.update.mock.calls[0][3]).toMatchInlineSnapshot(` - Object { - "references": Array [ - Object { - "id": "1", - "name": "action_0", - "type": "action", - }, - ], - } - `); + Object { + "references": Array [ + Object { + "id": "1", + "name": "action_0", + "type": "action", + }, + ], + } + `); }); test('creates a disabled alert', async () => { @@ -252,25 +251,25 @@ describe('create()', () => { }); const result = await alertsClient.create({ data }); expect(result).toMatchInlineSnapshot(` - Object { - "actions": Array [ - Object { - "group": "default", - "id": "1", - "params": Object { - "foo": true, - }, - }, - ], - "alertTypeId": "123", - "alertTypeParams": Object { - "bar": true, - }, - "enabled": false, - "id": "1", - "interval": 10000, - } - `); + Object { + "actions": Array [ + Object { + "group": "default", + "id": "1", + "params": Object { + "foo": true, + }, + }, + ], + "alertTypeId": "123", + "alertTypeParams": Object { + "bar": true, + }, + "enabled": false, + "id": "1", + "interval": 10000, + } + `); expect(savedObjectsClient.create).toHaveBeenCalledTimes(1); expect(taskManager.schedule).toHaveBeenCalledTimes(0); }); @@ -351,11 +350,11 @@ describe('create()', () => { ); expect(savedObjectsClient.delete).toHaveBeenCalledTimes(1); expect(savedObjectsClient.delete.mock.calls[0]).toMatchInlineSnapshot(` - Array [ - "alert", - "1", - ] - `); + Array [ + "alert", + "1", + ] + `); }); test('returns task manager error if cleanup fails, logs to console', async () => { @@ -400,14 +399,14 @@ describe('create()', () => { ); expect(alertsClientParams.log).toHaveBeenCalledTimes(1); expect(alertsClientParams.log.mock.calls[0]).toMatchInlineSnapshot(` - Array [ - Array [ - "alerting", - "error", - ], - "Failed to cleanup alert \\"1\\" after scheduling task failed. Error: Saved object delete error", - ] - `); + Array [ + Array [ + "alerting", + "error", + ], + "Failed to cleanup alert \\"1\\" after scheduling task failed. Error: Saved object delete error", + ] + `); }); test('throws an error if alert type not registerd', async () => { @@ -469,8 +468,7 @@ describe('enable()', () => { state: { alertInstances: {}, alertTypeState: {}, - previousScheduledRunAt: null, - scheduledRunAt: mockedDate, + previousStartedAt: null, }, scope: ['alerting'], }); @@ -577,31 +575,31 @@ describe('get()', () => { }); const result = await alertsClient.get({ id: '1' }); expect(result).toMatchInlineSnapshot(` - Object { - "actions": Array [ - Object { - "group": "default", - "id": "1", - "params": Object { - "foo": true, - }, - }, - ], - "alertTypeId": "123", - "alertTypeParams": Object { - "bar": true, - }, - "id": "1", - "interval": "10s", - } - `); + Object { + "actions": Array [ + Object { + "group": "default", + "id": "1", + "params": Object { + "foo": true, + }, + }, + ], + "alertTypeId": "123", + "alertTypeParams": Object { + "bar": true, + }, + "id": "1", + "interval": "10s", + } + `); expect(savedObjectsClient.get).toHaveBeenCalledTimes(1); expect(savedObjectsClient.get.mock.calls[0]).toMatchInlineSnapshot(` - Array [ - "alert", - "1", - ] - `); + Array [ + "alert", + "1", + ] + `); }); test(`throws an error when references aren't found`, async () => { @@ -672,34 +670,34 @@ describe('find()', () => { }); const result = await alertsClient.find(); expect(result).toMatchInlineSnapshot(` - Array [ - Object { - "actions": Array [ - Object { - "group": "default", - "id": "1", - "params": Object { - "foo": true, + Array [ + Object { + "actions": Array [ + Object { + "group": "default", + "id": "1", + "params": Object { + "foo": true, + }, + }, + ], + "alertTypeId": "123", + "alertTypeParams": Object { + "bar": true, + }, + "id": "1", + "interval": "10s", }, - }, - ], - "alertTypeId": "123", - "alertTypeParams": Object { - "bar": true, - }, - "id": "1", - "interval": "10s", - }, - ] - `); + ] + `); expect(savedObjectsClient.find).toHaveBeenCalledTimes(1); expect(savedObjectsClient.find.mock.calls[0]).toMatchInlineSnapshot(` - Array [ - Object { - "type": "alert", - }, - ] - `); + Array [ + Object { + "type": "alert", + }, + ] + `); }); }); @@ -741,17 +739,17 @@ describe('delete()', () => { expect(result).toEqual({ success: true }); expect(savedObjectsClient.delete).toHaveBeenCalledTimes(1); expect(savedObjectsClient.delete.mock.calls[0]).toMatchInlineSnapshot(` - Array [ - "alert", - "1", - ] - `); + Array [ + "alert", + "1", + ] + `); expect(taskManager.remove).toHaveBeenCalledTimes(1); expect(taskManager.remove.mock.calls[0]).toMatchInlineSnapshot(` - Array [ - "task-123", - ] - `); + Array [ + "task-123", + ] + `); }); }); @@ -823,58 +821,58 @@ describe('update()', () => { }, }); expect(result).toMatchInlineSnapshot(` - Object { - "actions": Array [ - Object { - "group": "default", - "id": "1", - "params": Object { - "foo": true, - }, - }, - ], - "alertTypeParams": Object { - "bar": true, - }, - "enabled": true, - "id": "1", - "interval": "10s", - "scheduledTaskId": "task-123", - } - `); + Object { + "actions": Array [ + Object { + "group": "default", + "id": "1", + "params": Object { + "foo": true, + }, + }, + ], + "alertTypeParams": Object { + "bar": true, + }, + "enabled": true, + "id": "1", + "interval": "10s", + "scheduledTaskId": "task-123", + } + `); expect(savedObjectsClient.update).toHaveBeenCalledTimes(1); expect(savedObjectsClient.update.mock.calls[0]).toHaveLength(4); expect(savedObjectsClient.update.mock.calls[0][0]).toEqual('alert'); expect(savedObjectsClient.update.mock.calls[0][1]).toEqual('1'); expect(savedObjectsClient.update.mock.calls[0][2]).toMatchInlineSnapshot(` - Object { - "actions": Array [ - Object { - "actionRef": "action_0", - "group": "default", - "params": Object { - "foo": true, - }, - }, - ], - "alertTypeParams": Object { - "bar": true, - }, - "interval": "10s", - } - `); + Object { + "actions": Array [ + Object { + "actionRef": "action_0", + "group": "default", + "params": Object { + "foo": true, + }, + }, + ], + "alertTypeParams": Object { + "bar": true, + }, + "interval": "10s", + } + `); expect(savedObjectsClient.update.mock.calls[0][3]).toMatchInlineSnapshot(` - Object { - "references": Array [ - Object { - "id": "1", - "name": "action_0", - "type": "action", - }, - ], - "version": "123", - } - `); + Object { + "references": Array [ + Object { + "id": "1", + "name": "action_0", + "type": "action", + }, + ], + "version": "123", + } + `); }); it('should validate alertTypeParams', async () => { diff --git a/x-pack/legacy/plugins/alerting/server/alerts_client.ts b/x-pack/legacy/plugins/alerting/server/alerts_client.ts index c4f87270d5b7e..4cfee6f4ab1f5 100644 --- a/x-pack/legacy/plugins/alerting/server/alerts_client.ts +++ b/x-pack/legacy/plugins/alerting/server/alerts_client.ts @@ -8,7 +8,7 @@ import { omit } from 'lodash'; import { SavedObjectsClientContract, SavedObjectReference } from 'src/core/server'; import { Alert, RawAlert, AlertTypeRegistry, AlertAction, Log } from './types'; import { TaskManager } from '../../task_manager'; -import { validateAlertTypeParams, parseDuration } from './lib'; +import { validateAlertTypeParams } from './lib'; interface ConstructorOptions { log: Log; @@ -213,10 +213,7 @@ export class AlertsClient { basePath, }, state: { - // This is here because we can't rely on the task manager's internal runAt. - // It changes it for timeout, etc when a task is running. - scheduledRunAt: new Date(Date.now() + parseDuration(interval)), - previousScheduledRunAt: null, + previousStartedAt: null, alertTypeState: {}, alertInstances: {}, }, diff --git a/x-pack/legacy/plugins/alerting/server/lib/get_create_task_runner_function.test.ts b/x-pack/legacy/plugins/alerting/server/lib/get_create_task_runner_function.test.ts index c4a992773e2e5..f12751e2288dd 100644 --- a/x-pack/legacy/plugins/alerting/server/lib/get_create_task_runner_function.test.ts +++ b/x-pack/legacy/plugins/alerting/server/lib/get_create_task_runner_function.test.ts @@ -4,18 +4,38 @@ * you may not use this file except in compliance with the Elastic License. */ +import sinon from 'sinon'; import { schema } from '@kbn/config-schema'; import { AlertExecutorOptions } from '../types'; +import { ConcreteTaskInstance } from '../../../task_manager'; import { SavedObjectsClientMock } from '../../../../../../src/core/server/mocks'; import { getCreateTaskRunnerFunction } from './get_create_task_runner_function'; -const mockedNow = new Date('2019-06-03T18:55:25.982Z'); -const mockedLastRunAt = new Date('2019-06-03T18:55:20.982Z'); -(global as any).Date = class Date extends global.Date { - static now() { - return mockedNow.getTime(); - } -}; +let fakeTimer: sinon.SinonFakeTimers; +let mockedTaskInstance: ConcreteTaskInstance; + +beforeAll(() => { + fakeTimer = sinon.useFakeTimers(); + mockedTaskInstance = { + id: '', + attempts: 0, + status: 'running', + version: '123', + runAt: new Date(), + scheduledAt: new Date(), + startedAt: new Date(), + retryAt: new Date(Date.now() + 5 * 60 * 1000), + state: { + startedAt: new Date(Date.now() - 5 * 60 * 1000), + }, + taskType: 'alerting:test', + params: { + alertId: '1', + }, + }; +}); + +afterAll(() => fakeTimer.restore()); const savedObjectsClient = SavedObjectsClientMock.create(); @@ -36,17 +56,6 @@ const getCreateTaskRunnerFunctionParams = { internalSavedObjectsRepository: savedObjectsClient, }; -const mockedTaskInstance = { - runAt: mockedLastRunAt, - state: { - scheduledRunAt: mockedLastRunAt, - }, - taskType: 'alerting:test', - params: { - alertId: '1', - }, -}; - const mockedAlertTypeSavedObject = { id: '1', type: 'alert', @@ -85,23 +94,22 @@ test('successfully executes the task', async () => { const runnerResult = await runner.run(); expect(runnerResult).toMatchInlineSnapshot(` Object { - "runAt": 2019-06-03T18:55:30.982Z, + "runAt": 1970-01-01T00:00:10.000Z, "state": Object { "alertInstances": Object {}, "alertTypeState": undefined, - "previousScheduledRunAt": 2019-06-03T18:55:20.982Z, - "scheduledRunAt": 2019-06-03T18:55:30.982Z, + "previousStartedAt": 1970-01-01T00:00:00.000Z, }, } `); expect(getCreateTaskRunnerFunctionParams.alertType.executor).toHaveBeenCalledTimes(1); const call = getCreateTaskRunnerFunctionParams.alertType.executor.mock.calls[0][0]; expect(call.params).toMatchInlineSnapshot(` - Object { - "bar": true, - } - `); - expect(call.scheduledRunAt).toMatchInlineSnapshot(`2019-06-03T18:55:20.982Z`); + Object { + "bar": true, + } + `); + expect(call.startedAt).toMatchInlineSnapshot(`1970-01-01T00:00:00.000Z`); expect(call.state).toMatchInlineSnapshot(`Object {}`); expect(call.services.alertInstanceFactory).toBeTruthy(); expect(call.services.callCluster).toBeTruthy(); @@ -120,16 +128,16 @@ test('fireAction is called per alert instance that fired', async () => { await runner.run(); expect(getCreateTaskRunnerFunctionParams.fireAction).toHaveBeenCalledTimes(1); expect(getCreateTaskRunnerFunctionParams.fireAction.mock.calls[0]).toMatchInlineSnapshot(` - Array [ - Object { - "basePath": undefined, - "id": "1", - "params": Object { - "foo": true, - }, - }, - ] - `); + Array [ + Object { + "basePath": undefined, + "id": "1", + "params": Object { + "foo": true, + }, + }, + ] + `); }); test('persists alertInstances passed in from state, only if they fire', async () => { @@ -157,7 +165,7 @@ test('persists alertInstances passed in from state, only if they fire', async () Object { "1": Object { "meta": Object { - "lastFired": 1559588125982, + "lastFired": 0, }, "state": Object { "bar": false, diff --git a/x-pack/legacy/plugins/alerting/server/lib/get_create_task_runner_function.ts b/x-pack/legacy/plugins/alerting/server/lib/get_create_task_runner_function.ts index c21ddbe7ed986..5591b63188b35 100644 --- a/x-pack/legacy/plugins/alerting/server/lib/get_create_task_runner_function.ts +++ b/x-pack/legacy/plugins/alerting/server/lib/get_create_task_runner_function.ts @@ -7,7 +7,7 @@ import { SavedObjectsClientContract } from 'src/core/server'; import { ActionsPlugin } from '../../../actions'; import { AlertType, Services, AlertServices } from '../types'; -import { TaskInstance } from '../../../task_manager'; +import { ConcreteTaskInstance } from '../../../task_manager'; import { createFireHandler } from './create_fire_handler'; import { createAlertInstanceFactory } from './create_alert_instance_factory'; import { AlertInstance } from './alert_instance'; @@ -22,7 +22,7 @@ interface CreateTaskRunnerFunctionOptions { } interface TaskRunnerOptions { - taskInstance: TaskInstance; + taskInstance: ConcreteTaskInstance; } export function getCreateTaskRunnerFunction({ @@ -66,8 +66,8 @@ export function getCreateTaskRunnerFunction({ services: alertTypeServices, params: validatedAlertTypeParams, state: taskInstance.state.alertTypeState || {}, - scheduledRunAt: taskInstance.state.scheduledRunAt, - previousScheduledRunAt: taskInstance.state.previousScheduledRunAt, + startedAt: taskInstance.startedAt!, + previousStartedAt: taskInstance.state.previousStartedAt, }); await Promise.all( @@ -88,7 +88,7 @@ export function getCreateTaskRunnerFunction({ ); const nextRunAt = getNextRunAt( - new Date(taskInstance.state.scheduledRunAt), + new Date(taskInstance.startedAt!), alertSavedObject.attributes.interval ); @@ -96,9 +96,7 @@ export function getCreateTaskRunnerFunction({ state: { alertTypeState, alertInstances, - // We store nextRunAt ourselves since task manager changes runAt when executing a task - scheduledRunAt: nextRunAt, - previousScheduledRunAt: taskInstance.state.scheduledRunAt, + previousStartedAt: taskInstance.startedAt!, }, runAt: nextRunAt, }; diff --git a/x-pack/legacy/plugins/alerting/server/types.ts b/x-pack/legacy/plugins/alerting/server/types.ts index 50fbba498226f..b1e268431e40e 100644 --- a/x-pack/legacy/plugins/alerting/server/types.ts +++ b/x-pack/legacy/plugins/alerting/server/types.ts @@ -29,8 +29,8 @@ export interface AlertServices extends Services { } export interface AlertExecutorOptions { - scheduledRunAt: Date; - previousScheduledRunAt?: Date; + startedAt: Date; + previousStartedAt?: Date; services: AlertServices; params: Record; state: State; diff --git a/x-pack/test/api_integration/apis/actions/fire.ts b/x-pack/test/api_integration/apis/actions/fire.ts index a2397c2ce737b..da92f6c2d7f64 100644 --- a/x-pack/test/api_integration/apis/actions/fire.ts +++ b/x-pack/test/api_integration/apis/actions/fire.ts @@ -68,7 +68,7 @@ export default function({ getService }: KibanaFunctionalTestDefaultProviders) { .then((resp: any) => { expect(resp.body).to.be.an('object'); }); - const indexedRecord = await retry.tryForTime(5000, async () => { + const indexedRecord = await retry.tryForTime(15000, async () => { const searchResult = await es.search({ index: esTestIndexName, body: { @@ -139,7 +139,7 @@ export default function({ getService }: KibanaFunctionalTestDefaultProviders) { .then((resp: any) => { expect(resp.body).to.be.an('object'); }); - const indexedRecord = await retry.tryForTime(5000, async () => { + const indexedRecord = await retry.tryForTime(15000, async () => { const searchResult = await es.search({ index: esTestIndexName, body: { diff --git a/x-pack/test/api_integration/apis/alerting/alerts.ts b/x-pack/test/api_integration/apis/alerting/alerts.ts index 27fabdc3208cc..2e62602970736 100644 --- a/x-pack/test/api_integration/apis/alerting/alerts.ts +++ b/x-pack/test/api_integration/apis/alerting/alerts.ts @@ -68,7 +68,7 @@ export default function alertTests({ getService }: KibanaFunctionalTestDefaultPr .then((resp: any) => { createdAlertIds.push(resp.body.id); }); - const alertTestRecord = await retry.tryForTime(5000, async () => { + const alertTestRecord = await retry.tryForTime(15000, async () => { const searchResult = await es.search({ index: esTestIndexName, body: { @@ -102,7 +102,7 @@ export default function alertTests({ getService }: KibanaFunctionalTestDefaultPr reference: 'create-test-1', }, }); - const actionTestRecord = await retry.tryForTime(5000, async () => { + const actionTestRecord = await retry.tryForTime(15000, async () => { const searchResult = await es.search({ index: esTestIndexName, body: { @@ -141,5 +141,78 @@ export default function alertTests({ getService }: KibanaFunctionalTestDefaultPr source: 'action:test.index-record', }); }); + + it('should handle custom retry logic', async () => { + // We'll use this start time to query tasks created after this point + const testStart = new Date(); + // We have to provide the test.rate-limit the next runAt, for testing purposes + const retryDate = new Date(Date.now() + 60000); + + const { body: createdAlert } = await supertest + .post('/api/alert') + .set('kbn-xsrf', 'foo') + .send( + getTestAlertData({ + interval: '1m', + alertTypeId: 'test.always-firing', + alertTypeParams: { + index: esTestIndexName, + reference: 'create-test-2', + }, + actions: [ + { + group: 'default', + id: 'ce37997f-0fb6-460a-8baf-f81ac5d38348', + params: { + index: esTestIndexName, + reference: 'create-test-1', + retryAt: retryDate.getTime(), + }, + }, + ], + }) + ) + .expect(200); + createdAlertIds.push(createdAlert.id); + + const scheduledActionTask = await retry.tryForTime(15000, async () => { + const searchResult = await es.search({ + index: '.kibana_task_manager', + body: { + query: { + bool: { + must: [ + { + term: { + 'task.status': 'idle', + }, + }, + { + term: { + 'task.attempts': 1, + }, + }, + { + term: { + 'task.taskType': 'actions:test.rate-limit', + }, + }, + { + range: { + 'task.scheduledAt': { + gte: testStart, + }, + }, + }, + ], + }, + }, + }, + }); + expect(searchResult.hits.total.value).to.eql(1); + return searchResult.hits.hits[0]; + }); + expect(scheduledActionTask._source.task.runAt).to.eql(retryDate.toISOString()); + }); }); } diff --git a/x-pack/test/api_integration/fixtures/plugins/alerts/index.ts b/x-pack/test/api_integration/fixtures/plugins/alerts/index.ts index 03548caeb6e9b..4bd6894c7d508 100644 --- a/x-pack/test/api_integration/fixtures/plugins/alerts/index.ts +++ b/x-pack/test/api_integration/fixtures/plugins/alerts/index.ts @@ -67,8 +67,38 @@ export default function(kibana: any) { throw new Error('Failed to execute action type'); }, }; + const rateLimitedActionType: ActionType = { + id: 'test.rate-limit', + name: 'Test: Rate Limit', + unencryptedAttributes: [], + maxAttempts: 2, + validate: { + params: schema.object({ + index: schema.string(), + reference: schema.string(), + retryAt: schema.number(), + }), + }, + async executor({ config, params, services }: ActionTypeExecutorOptions) { + await services.callCluster('index', { + index: params.index, + refresh: 'wait_for', + body: { + params, + config, + reference: params.reference, + source: 'action:test.rate-limit', + }, + }); + return { + status: 'error', + retry: new Date(params.retryAt), + }; + }, + }; server.plugins.actions.registerType(indexRecordActionType); server.plugins.actions.registerType(failingActionType); + server.plugins.actions.registerType(rateLimitedActionType); // Alert types const alwaysFiringAlertType: AlertType = { diff --git a/x-pack/test/functional/es_archives/actions/basic/data.json b/x-pack/test/functional/es_archives/actions/basic/data.json index e490e2b89aa7d..4509fcc038576 100644 --- a/x-pack/test/functional/es_archives/actions/basic/data.json +++ b/x-pack/test/functional/es_archives/actions/basic/data.json @@ -32,3 +32,19 @@ } } } + +{ + "value": { + "id": "action:ce37997f-0fb6-460a-8baf-f81ac5d38348", + "index": ".kibana", + "source": { + "type": "action", + "migrationVersion": {}, + "action": { + "description": "My rate limited action", + "actionTypeId": "test.rate-limit", + "actionTypeConfig": {} + } + } + } +}