From 7eb2e1b1e3f7dbd2701cfa32b9acd10274a75edc Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Wed, 24 Jul 2019 14:13:53 -0400 Subject: [PATCH 01/12] Implement wanted error handling --- x-pack/legacy/plugins/actions/server/action_type_registry.ts | 2 ++ x-pack/legacy/plugins/actions/server/types.ts | 3 +++ x-pack/legacy/plugins/alerting/server/alert_type_registry.ts | 1 + x-pack/legacy/plugins/alerting/server/types.ts | 2 ++ x-pack/legacy/plugins/task_manager/index.d.ts | 2 +- 5 files changed, 9 insertions(+), 1 deletion(-) 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 feba76b06b81a0..b47fa0888dd19d 100644 --- a/x-pack/legacy/plugins/actions/server/action_type_registry.ts +++ b/x-pack/legacy/plugins/actions/server/action_type_registry.ts @@ -61,6 +61,8 @@ export class ActionTypeRegistry { [`actions:${actionType.id}`]: { title: actionType.name, type: `actions:${actionType.id}`, + maxAttempts: actionType.maxAttempts || 1, + getRetryDelay: actionType.getRetryDelay, createTaskRunner: this.taskRunCreatorFunction, }, }); diff --git a/x-pack/legacy/plugins/actions/server/types.ts b/x-pack/legacy/plugins/actions/server/types.ts index 70f79327ae93e7..d6f7a1e6a10c32 100644 --- a/x-pack/legacy/plugins/actions/server/types.ts +++ b/x-pack/legacy/plugins/actions/server/types.ts @@ -5,6 +5,7 @@ */ import { SavedObjectsClientContract } from 'src/core/server'; +import { TaskDefinition } from '../../task_manager'; import { ActionTypeRegistry } from './action_type_registry'; export type WithoutQueryAndParams = Pick>; @@ -54,6 +55,8 @@ export interface ActionType { id: string; name: string; unencryptedAttributes: string[]; + maxAttempts?: number; + getRetryDelay?: TaskDefinition['getRetryDelay']; validate?: { params?: { validate: (object: any) => any }; config?: { validate: (object: any) => any }; diff --git a/x-pack/legacy/plugins/alerting/server/alert_type_registry.ts b/x-pack/legacy/plugins/alerting/server/alert_type_registry.ts index 61a358473ef465..89f8e832730dac 100644 --- a/x-pack/legacy/plugins/alerting/server/alert_type_registry.ts +++ b/x-pack/legacy/plugins/alerting/server/alert_type_registry.ts @@ -58,6 +58,7 @@ export class AlertTypeRegistry { [`alerting:${alertType.id}`]: { title: alertType.name, type: `alerting:${alertType.id}`, + getRetryDelay: alertType.getRetryDelay, createTaskRunner: getCreateTaskRunnerFunction({ alertType, getServices: this.getServices, diff --git a/x-pack/legacy/plugins/alerting/server/types.ts b/x-pack/legacy/plugins/alerting/server/types.ts index 50fbba498226f5..9b2f48e56052d6 100644 --- a/x-pack/legacy/plugins/alerting/server/types.ts +++ b/x-pack/legacy/plugins/alerting/server/types.ts @@ -7,6 +7,7 @@ import { SavedObjectAttributes, SavedObjectsClientContract } from 'src/core/server'; import { AlertInstance } from './lib'; import { AlertTypeRegistry } from './alert_type_registry'; +import { TaskDefinition } from '../../task_manager'; export type State = Record; export type Context = Record; @@ -39,6 +40,7 @@ export interface AlertExecutorOptions { export interface AlertType { id: string; name: string; + getRetryDelay?: TaskDefinition['getRetryDelay']; validate?: { params?: { validate: (object: any) => any }; }; diff --git a/x-pack/legacy/plugins/task_manager/index.d.ts b/x-pack/legacy/plugins/task_manager/index.d.ts index ca106b6c2372af..ac1c2c5c0744da 100644 --- a/x-pack/legacy/plugins/task_manager/index.d.ts +++ b/x-pack/legacy/plugins/task_manager/index.d.ts @@ -5,4 +5,4 @@ */ export { TaskManager } from './types'; -export { TaskInstance, ConcreteTaskInstance, TaskRunCreatorFunction } from './task'; +export { TaskDefinition, TaskInstance, ConcreteTaskInstance, TaskRunCreatorFunction } from './task'; From 82a2f57e62688bbe292525773c7df59b9d2e27a2 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Fri, 26 Jul 2019 09:59:40 -0400 Subject: [PATCH 02/12] Cleanup --- x-pack/legacy/plugins/actions/server/action_type_registry.ts | 3 ++- x-pack/legacy/plugins/actions/server/types.ts | 2 -- x-pack/legacy/plugins/alerting/server/alert_type_registry.ts | 1 - x-pack/legacy/plugins/alerting/server/types.ts | 2 -- x-pack/legacy/plugins/task_manager/index.d.ts | 2 +- 5 files changed, 3 insertions(+), 7 deletions(-) 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 b47fa0888dd19d..ecacf631ce6591 100644 --- a/x-pack/legacy/plugins/actions/server/action_type_registry.ts +++ b/x-pack/legacy/plugins/actions/server/action_type_registry.ts @@ -62,7 +62,8 @@ export class ActionTypeRegistry { title: actionType.name, type: `actions:${actionType.id}`, maxAttempts: actionType.maxAttempts || 1, - getRetryDelay: actionType.getRetryDelay, + // TODO: Use generic + // getRetry: actionType.getRetry, createTaskRunner: this.taskRunCreatorFunction, }, }); diff --git a/x-pack/legacy/plugins/actions/server/types.ts b/x-pack/legacy/plugins/actions/server/types.ts index d6f7a1e6a10c32..886c7429e99cae 100644 --- a/x-pack/legacy/plugins/actions/server/types.ts +++ b/x-pack/legacy/plugins/actions/server/types.ts @@ -5,7 +5,6 @@ */ import { SavedObjectsClientContract } from 'src/core/server'; -import { TaskDefinition } from '../../task_manager'; import { ActionTypeRegistry } from './action_type_registry'; export type WithoutQueryAndParams = Pick>; @@ -56,7 +55,6 @@ export interface ActionType { name: string; unencryptedAttributes: string[]; maxAttempts?: number; - getRetryDelay?: TaskDefinition['getRetryDelay']; validate?: { params?: { validate: (object: any) => any }; config?: { validate: (object: any) => any }; diff --git a/x-pack/legacy/plugins/alerting/server/alert_type_registry.ts b/x-pack/legacy/plugins/alerting/server/alert_type_registry.ts index 89f8e832730dac..61a358473ef465 100644 --- a/x-pack/legacy/plugins/alerting/server/alert_type_registry.ts +++ b/x-pack/legacy/plugins/alerting/server/alert_type_registry.ts @@ -58,7 +58,6 @@ export class AlertTypeRegistry { [`alerting:${alertType.id}`]: { title: alertType.name, type: `alerting:${alertType.id}`, - getRetryDelay: alertType.getRetryDelay, createTaskRunner: getCreateTaskRunnerFunction({ alertType, getServices: this.getServices, diff --git a/x-pack/legacy/plugins/alerting/server/types.ts b/x-pack/legacy/plugins/alerting/server/types.ts index 9b2f48e56052d6..50fbba498226f5 100644 --- a/x-pack/legacy/plugins/alerting/server/types.ts +++ b/x-pack/legacy/plugins/alerting/server/types.ts @@ -7,7 +7,6 @@ import { SavedObjectAttributes, SavedObjectsClientContract } from 'src/core/server'; import { AlertInstance } from './lib'; import { AlertTypeRegistry } from './alert_type_registry'; -import { TaskDefinition } from '../../task_manager'; export type State = Record; export type Context = Record; @@ -40,7 +39,6 @@ export interface AlertExecutorOptions { export interface AlertType { id: string; name: string; - getRetryDelay?: TaskDefinition['getRetryDelay']; validate?: { params?: { validate: (object: any) => any }; }; diff --git a/x-pack/legacy/plugins/task_manager/index.d.ts b/x-pack/legacy/plugins/task_manager/index.d.ts index ac1c2c5c0744da..ca106b6c2372af 100644 --- a/x-pack/legacy/plugins/task_manager/index.d.ts +++ b/x-pack/legacy/plugins/task_manager/index.d.ts @@ -5,4 +5,4 @@ */ export { TaskManager } from './types'; -export { TaskDefinition, TaskInstance, ConcreteTaskInstance, TaskRunCreatorFunction } from './task'; +export { TaskInstance, ConcreteTaskInstance, TaskRunCreatorFunction } from './task'; From 78cd509c6f37b821f7a76b2c3f4dae21597429d8 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Fri, 26 Jul 2019 10:10:10 -0400 Subject: [PATCH 03/12] Add retry logic to actions --- .../actions/server/action_type_registry.ts | 11 ++++++++--- .../plugins/actions/server/lib/executor_error.ts | 15 +++++++++++++++ .../server/lib/get_create_task_runner_function.ts | 12 +++++++++++- x-pack/legacy/plugins/actions/server/lib/index.ts | 1 + 4 files changed, 35 insertions(+), 4 deletions(-) create mode 100644 x-pack/legacy/plugins/actions/server/lib/executor_error.ts 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 ecacf631ce6591..1826a5a953d3e6 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 { @@ -62,8 +62,13 @@ export class ActionTypeRegistry { title: actionType.name, type: `actions:${actionType.id}`, maxAttempts: actionType.maxAttempts || 1, - // TODO: Use generic - // getRetry: actionType.getRetry, + getRetry(attempts: number, error: any) { + if (error instanceof ExecutorError) { + return error.retry == null ? true : error.retry; + } + // Retry other kinds of errors + return true; + }, createTaskRunner: this.taskRunCreatorFunction, }, }); 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 00000000000000..5e0dee3f3cc2d8 --- /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.ts b/x-pack/legacy/plugins/actions/server/lib/get_create_task_runner_function.ts index 04b7345781e3c9..eb1c0e8ac8a40b 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 23305f4eba90ea..75e4a5595a7696 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'; From 0459929a6a27d787e987db811224acaa25cb0e96 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Fri, 26 Jul 2019 10:33:13 -0400 Subject: [PATCH 04/12] Leverage startedAt from task manager --- x-pack/legacy/plugins/alerting/README.md | 12 +- .../alerting/server/alerts_client.test.ts | 432 +++++++++--------- .../plugins/alerting/server/alerts_client.ts | 7 +- .../get_create_task_runner_function.test.ts | 82 ++-- .../lib/get_create_task_runner_function.ts | 14 +- .../legacy/plugins/alerting/server/types.ts | 4 +- 6 files changed, 276 insertions(+), 275 deletions(-) diff --git a/x-pack/legacy/plugins/alerting/README.md b/x-pack/legacy/plugins/alerting/README.md index 091d5a9da0115a..18034807b7ac75 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 4421ea7435c982..8a6ffe7ed45064 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 c4f87270d5b7e7..4cfee6f4ab1f56 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 c4a992773e2e51..f12751e2288dd8 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 c21ddbe7ed9863..5591b63188b35b 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 50fbba498226f5..b1e268431e40e9 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; From 13ae86a4ab6c219928938ed72758f8a9f1c87299 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Fri, 26 Jul 2019 13:34:51 -0400 Subject: [PATCH 05/12] Fix broken jest tests --- .../server/action_type_registry.test.ts | 36 ++++++++++--------- .../get_create_task_runner_function.test.ts | 3 ++ 2 files changed, 22 insertions(+), 17 deletions(-) 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 a157e85d2ac2cb..824d00968d7d89 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 @@ -50,16 +50,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(); @@ -99,13 +101,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/lib/get_create_task_runner_function.test.ts b/x-pack/legacy/plugins/actions/server/lib/get_create_task_runner_function.test.ts index 489325748b4dff..1b5c9a3858a52e 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 @@ -54,6 +54,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(); From d9fedaac01c3e56203c47b8c78d49050cb9b97e2 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Mon, 29 Jul 2019 09:14:25 -0400 Subject: [PATCH 06/12] Add missing unit test --- .../get_create_task_runner_function.test.ts | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) 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 1b5c9a3858a52e..d2cc4cb618568d 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(); @@ -69,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); + } +}); From cbb666c9e5653de8e16fce4028a634f65f594aa2 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Mon, 29 Jul 2019 09:25:53 -0400 Subject: [PATCH 07/12] Add unit tests for getRetry --- .../server/action_type_registry.test.ts | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) 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 824d00968d7d89..c1c2e4045e6e27 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 @@ -8,13 +8,16 @@ jest.mock('./lib/get_create_task_runner_function', () => ({ getCreateTaskRunnerFunction: jest.fn(), })); +import sinon from 'sinon'; import { taskManagerMock } from '../../task_manager/task_manager.mock'; import { encryptedSavedObjectsMock } from '../../encrypted_saved_objects/server/plugin.mock'; 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(); +let fakeTimer: sinon.SinonFakeTimers; function getServices() { return { @@ -31,6 +34,12 @@ const actionTypeRegistryParams = { beforeEach(() => jest.resetAllMocks()); +beforeAll(() => { + fakeTimer = sinon.useFakeTimers(); +}); + +afterAll(() => fakeTimer.restore()); + const executor: ExecutorType = async options => { return { status: 'ok' }; }; @@ -88,6 +97,27 @@ describe('register()', () => { `"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(true); + 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(true); + expect(getRetry(0, new ExecutorError('my message', {}, undefined))).toEqual(true); + expect(getRetry(0, new ExecutorError('my message', {}, retryTime))).toEqual(retryTime); + }); }); describe('get()', () => { From 4b751f18ead2aaf60494090d9f502e1f6f34d007 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Mon, 29 Jul 2019 14:45:22 -0400 Subject: [PATCH 08/12] Add test for rate limit --- .../api_integration/apis/alerting/alerts.ts | 73 +++++++++++++++++++ .../fixtures/plugins/alerts/index.ts | 30 ++++++++ .../es_archives/actions/basic/data.json | 16 ++++ 3 files changed, 119 insertions(+) diff --git a/x-pack/test/api_integration/apis/alerting/alerts.ts b/x-pack/test/api_integration/apis/alerting/alerts.ts index 27fabdc3208cc4..181692d787c733 100644 --- a/x-pack/test/api_integration/apis/alerting/alerts.ts +++ b/x-pack/test/api_integration/apis/alerting/alerts.ts @@ -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(5000, 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 03548caeb6e9b9..4bd6894c7d508c 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 e490e2b89aa7d0..4509fcc038576d 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": {} + } + } + } +} From 1ec45719dbd3265a0b83441cf7d88c9bbc6194ad Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Mon, 29 Jul 2019 14:48:38 -0400 Subject: [PATCH 09/12] Remove fake timers --- .../plugins/actions/server/action_type_registry.test.ts | 8 -------- 1 file changed, 8 deletions(-) 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 c1c2e4045e6e27..fd14ac2196f87e 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 @@ -8,7 +8,6 @@ jest.mock('./lib/get_create_task_runner_function', () => ({ getCreateTaskRunnerFunction: jest.fn(), })); -import sinon from 'sinon'; import { taskManagerMock } from '../../task_manager/task_manager.mock'; import { encryptedSavedObjectsMock } from '../../encrypted_saved_objects/server/plugin.mock'; import { ActionTypeRegistry } from './action_type_registry'; @@ -17,7 +16,6 @@ import { SavedObjectsClientMock } from '../../../../../src/core/server/mocks'; import { ExecutorError } from './lib'; const mockTaskManager = taskManagerMock.create(); -let fakeTimer: sinon.SinonFakeTimers; function getServices() { return { @@ -34,12 +32,6 @@ const actionTypeRegistryParams = { beforeEach(() => jest.resetAllMocks()); -beforeAll(() => { - fakeTimer = sinon.useFakeTimers(); -}); - -afterAll(() => fakeTimer.restore()); - const executor: ExecutorType = async options => { return { status: 'ok' }; }; From f6c03536322d03d390e59dba22815db79b802b26 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Mon, 29 Jul 2019 15:07:52 -0400 Subject: [PATCH 10/12] Don't retry errors by default for actions unless the proper result format is returned with status: error --- .../plugins/actions/server/action_type_registry.test.ts | 2 +- x-pack/legacy/plugins/actions/server/action_type_registry.ts | 4 ++-- x-pack/legacy/plugins/actions/server/lib/execute.test.ts | 2 ++ x-pack/legacy/plugins/actions/server/lib/execute.ts | 2 +- 4 files changed, 6 insertions(+), 4 deletions(-) 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 fd14ac2196f87e..fa6d404b74cbc0 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 @@ -103,7 +103,7 @@ describe('register()', () => { const getRetry = registerTaskDefinitionsCall['actions:my-action-type'].getRetry!; const retryTime = new Date(); - expect(getRetry(0, new Error())).toEqual(true); + 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(true); 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 1826a5a953d3e6..b04e249a9d483d 100644 --- a/x-pack/legacy/plugins/actions/server/action_type_registry.ts +++ b/x-pack/legacy/plugins/actions/server/action_type_registry.ts @@ -66,8 +66,8 @@ export class ActionTypeRegistry { if (error instanceof ExecutorError) { return error.retry == null ? true : error.retry; } - // Retry other kinds of errors - return true; + // 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 2d184a36d56ea5..ec9ebb2450c7bf 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 899efbaead0881..19e683c3386b3c 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; From 4bb44c4320f16fc7a98780623cca252f97575abd Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Tue, 30 Jul 2019 09:13:47 -0400 Subject: [PATCH 11/12] Don't retry unless attribute specified --- x-pack/legacy/plugins/actions/server/action_type_registry.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 b04e249a9d483d..72ba36c402bf90 100644 --- a/x-pack/legacy/plugins/actions/server/action_type_registry.ts +++ b/x-pack/legacy/plugins/actions/server/action_type_registry.ts @@ -64,7 +64,7 @@ export class ActionTypeRegistry { maxAttempts: actionType.maxAttempts || 1, getRetry(attempts: number, error: any) { if (error instanceof ExecutorError) { - return error.retry == null ? true : error.retry; + return error.retry == null ? false : error.retry; } // Don't retry other kinds of errors return false; From 2f1f5937b204bc71b90194e67b4b7ca8e7271a75 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Tue, 30 Jul 2019 09:18:21 -0400 Subject: [PATCH 12/12] Fix tests --- .../plugins/actions/server/action_type_registry.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 fa6d404b74cbc0..0c1643e20d8bfd 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 @@ -106,8 +106,8 @@ describe('register()', () => { 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(true); - expect(getRetry(0, new ExecutorError('my message', {}, undefined))).toEqual(true); + 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); }); });