Implement wanted error handling for alerting and actions (elastic#41917

)

* Implement wanted error handling

* Cleanup

* Add retry logic to actions

* Leverage startedAt from task manager

* Fix broken jest tests

* Add missing unit test

* Add unit tests for getRetry

* Add test for rate limit

* Remove fake timers

* Don't retry errors by default for actions unless the proper result format is returned with status: error

* Don't retry unless attribute specified

* Fix tests

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`, () => {

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,
       },
     });

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]`,
   });
 });

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;

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;
+  }
+}

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);
+  }
+});

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,14 +29,23 @@ export function getCreateTaskRunnerFunction({
     return {
       run: async () => {
         const { namespace, id, actionTypeParams } = taskInstance.params;
-        await execute({
+        const executorResult = await execute({
           namespace,
           actionTypeRegistry,
           encryptedSavedObjectsPlugin,
           actionId: id,
           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
+          );
+        }
       },
     };
   };

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';

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 };

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.<br><br>**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.<br><br>**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,