From 802f821a546c406930d75b5b32b077ee4e3c9b2f Mon Sep 17 00:00:00 2001 From: Zacqary Xeper Date: Thu, 9 Jul 2020 16:11:34 -0500 Subject: [PATCH 1/5] [Metrics UI] Use alertId instead of uuid for alertInstanceIds --- .../inventory_metric_threshold_executor.ts | 9 +++++---- .../register_inventory_metric_threshold_alert_type.ts | 4 +--- .../metric_threshold_executor.test.ts | 11 ++++++++++- .../metric_threshold/metric_threshold_executor.ts | 4 ++-- .../register_metric_threshold_alert_type.ts | 4 +--- 5 files changed, 19 insertions(+), 13 deletions(-) diff --git a/x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/inventory_metric_threshold_executor.ts b/x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/inventory_metric_threshold_executor.ts index 1ef86d9e7eac4..8788ab9b3d5aa 100644 --- a/x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/inventory_metric_threshold_executor.ts +++ b/x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/inventory_metric_threshold_executor.ts @@ -29,10 +29,11 @@ interface InventoryMetricThresholdParams { alertOnNoData?: boolean; } -export const createInventoryMetricThresholdExecutor = ( - libs: InfraBackendLibs, - alertId: string -) => async ({ services, params }: AlertExecutorOptions) => { +export const createInventoryMetricThresholdExecutor = (libs: InfraBackendLibs) => async ({ + services, + params, + alertId, +}: AlertExecutorOptions) => { const { criteria, filterQuery, diff --git a/x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/register_inventory_metric_threshold_alert_type.ts b/x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/register_inventory_metric_threshold_alert_type.ts index d7c4165d5a870..85b38f48d9f22 100644 --- a/x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/register_inventory_metric_threshold_alert_type.ts +++ b/x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/register_inventory_metric_threshold_alert_type.ts @@ -5,8 +5,6 @@ */ import { i18n } from '@kbn/i18n'; import { schema } from '@kbn/config-schema'; -import { curry } from 'lodash'; -import uuid from 'uuid'; import { createInventoryMetricThresholdExecutor, FIRED_ACTIONS, @@ -43,7 +41,7 @@ export const registerMetricInventoryThresholdAlertType = (libs: InfraBackendLibs defaultActionGroupId: FIRED_ACTIONS.id, actionGroups: [FIRED_ACTIONS], producer: 'metrics', - executor: curry(createInventoryMetricThresholdExecutor)(libs, uuid.v4()), + executor: createInventoryMetricThresholdExecutor(libs), actionVariables: { context: [ { diff --git a/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.test.ts b/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.test.ts index 24f4bc2c678b4..126a942a1b5e7 100644 --- a/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.test.ts +++ b/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.test.ts @@ -27,6 +27,7 @@ describe('The metric threshold alert type', () => { const instanceID = '*::test'; const execute = (comparator: Comparator, threshold: number[], sourceId: string = 'default') => executor({ + alertId: 'test', services, params: { sourceId, @@ -106,6 +107,7 @@ describe('The metric threshold alert type', () => { describe('querying with a groupBy parameter', () => { const execute = (comparator: Comparator, threshold: number[]) => executor({ + alertId: 'test', services, params: { groupBy: 'something', @@ -156,6 +158,7 @@ describe('The metric threshold alert type', () => { groupBy: string = '' ) => executor({ + alertId: 'test', services, params: { groupBy, @@ -213,6 +216,7 @@ describe('The metric threshold alert type', () => { const instanceID = '*::test'; const execute = (comparator: Comparator, threshold: number[]) => executor({ + alertId: 'test', services, params: { criteria: [ @@ -239,6 +243,7 @@ describe('The metric threshold alert type', () => { const instanceID = '*::test'; const execute = (comparator: Comparator, threshold: number[]) => executor({ + alertId: 'test', services, params: { criteria: [ @@ -265,6 +270,7 @@ describe('The metric threshold alert type', () => { const instanceID = '*::test'; const execute = (comparator: Comparator, threshold: number[]) => executor({ + alertId: 'test', services, params: { criteria: [ @@ -291,6 +297,7 @@ describe('The metric threshold alert type', () => { const instanceID = '*::test'; const execute = (alertOnNoData: boolean) => executor({ + alertId: 'test', services, params: { criteria: [ @@ -320,6 +327,7 @@ describe('The metric threshold alert type', () => { // const instanceID = '*::test'; // const execute = (threshold: number[]) => // executor({ + // alertId: 'test', // services, // params: { // criteria: [ @@ -377,9 +385,10 @@ const mockLibs: any = { configuration: createMockStaticConfiguration({}), }; -const executor = createMetricThresholdExecutor(mockLibs, 'test') as (opts: { +const executor = createMetricThresholdExecutor(mockLibs) as (opts: { params: AlertExecutorOptions['params']; services: { callCluster: AlertExecutorOptions['params']['callCluster'] }; + alertId: AlertExecutorOptions['alertId']; }) => Promise; const services: AlertServicesMock = alertsMock.createAlertServices(); diff --git a/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.ts b/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.ts index 4c02593dd0095..0de3c553d2779 100644 --- a/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.ts +++ b/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.ts @@ -17,9 +17,9 @@ import { import { AlertStates } from './types'; import { evaluateAlert } from './lib/evaluate_alert'; -export const createMetricThresholdExecutor = (libs: InfraBackendLibs, alertId: string) => +export const createMetricThresholdExecutor = (libs: InfraBackendLibs) => async function (options: AlertExecutorOptions) { - const { services, params } = options; + const { services, params, alertId } = options; const { criteria } = params; const { sourceId, alertOnNoData } = params as { sourceId?: string; diff --git a/x-pack/plugins/infra/server/lib/alerting/metric_threshold/register_metric_threshold_alert_type.ts b/x-pack/plugins/infra/server/lib/alerting/metric_threshold/register_metric_threshold_alert_type.ts index 02d9ca3e5f0c9..529a1d176c437 100644 --- a/x-pack/plugins/infra/server/lib/alerting/metric_threshold/register_metric_threshold_alert_type.ts +++ b/x-pack/plugins/infra/server/lib/alerting/metric_threshold/register_metric_threshold_alert_type.ts @@ -4,9 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ import { i18n } from '@kbn/i18n'; -import uuid from 'uuid'; import { schema } from '@kbn/config-schema'; -import { curry } from 'lodash'; import { METRIC_EXPLORER_AGGREGATIONS } from '../../../../common/http_api/metrics_explorer'; import { createMetricThresholdExecutor, FIRED_ACTIONS } from './metric_threshold_executor'; import { METRIC_THRESHOLD_ALERT_TYPE_ID, Comparator } from './types'; @@ -107,7 +105,7 @@ export function registerMetricThresholdAlertType(libs: InfraBackendLibs) { }, defaultActionGroupId: FIRED_ACTIONS.id, actionGroups: [FIRED_ACTIONS], - executor: curry(createMetricThresholdExecutor)(libs, uuid.v4()), + executor: createMetricThresholdExecutor(libs), actionVariables: { context: [ { name: 'group', description: groupActionVariableDescription }, From f08702aa5b4232b618540d9bcd5196669aa650c8 Mon Sep 17 00:00:00 2001 From: Zacqary Xeper Date: Thu, 9 Jul 2020 16:42:19 -0500 Subject: [PATCH 2/5] Remove alertID from alertInstanceId entirely --- .../inventory_metric_threshold_executor.ts | 3 +- .../metric_threshold_executor.test.ts | 35 ++++++++----------- .../metric_threshold_executor.ts | 4 +-- 3 files changed, 17 insertions(+), 25 deletions(-) diff --git a/x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/inventory_metric_threshold_executor.ts b/x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/inventory_metric_threshold_executor.ts index 8788ab9b3d5aa..0a3910f2c5d7c 100644 --- a/x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/inventory_metric_threshold_executor.ts +++ b/x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/inventory_metric_threshold_executor.ts @@ -32,7 +32,6 @@ interface InventoryMetricThresholdParams { export const createInventoryMetricThresholdExecutor = (libs: InfraBackendLibs) => async ({ services, params, - alertId, }: AlertExecutorOptions) => { const { criteria, @@ -55,7 +54,7 @@ export const createInventoryMetricThresholdExecutor = (libs: InfraBackendLibs) = const inventoryItems = Object.keys(first(results) as any); for (const item of inventoryItems) { - const alertInstance = services.alertInstanceFactory(`${item}::${alertId}`); + const alertInstance = services.alertInstanceFactory(`${item}`); // AND logic; all criteria must be across the threshold const shouldAlertFire = results.every((result) => result[item].shouldFire); diff --git a/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.test.ts b/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.test.ts index 126a942a1b5e7..c9a0167f88691 100644 --- a/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.test.ts +++ b/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.test.ts @@ -24,10 +24,9 @@ let persistAlertInstances = false; // eslint-disable-line describe('The metric threshold alert type', () => { describe('querying the entire infrastructure', () => { - const instanceID = '*::test'; + const instanceID = '*'; const execute = (comparator: Comparator, threshold: number[], sourceId: string = 'default') => executor({ - alertId: 'test', services, params: { sourceId, @@ -107,7 +106,6 @@ describe('The metric threshold alert type', () => { describe('querying with a groupBy parameter', () => { const execute = (comparator: Comparator, threshold: number[]) => executor({ - alertId: 'test', services, params: { groupBy: 'something', @@ -120,8 +118,8 @@ describe('The metric threshold alert type', () => { ], }, }); - const instanceIdA = 'a::test'; - const instanceIdB = 'b::test'; + const instanceIdA = 'a'; + const instanceIdB = 'b'; test('sends an alert when all groups pass the threshold', async () => { await execute(Comparator.GT, [0.75]); expect(mostRecentAction(instanceIdA).id).toBe(FIRED_ACTIONS.id); @@ -158,7 +156,6 @@ describe('The metric threshold alert type', () => { groupBy: string = '' ) => executor({ - alertId: 'test', services, params: { groupBy, @@ -178,20 +175,20 @@ describe('The metric threshold alert type', () => { }, }); test('sends an alert when all criteria cross the threshold', async () => { - const instanceID = '*::test'; + const instanceID = '*'; await execute(Comparator.GT_OR_EQ, [1.0], [3.0]); expect(mostRecentAction(instanceID).id).toBe(FIRED_ACTIONS.id); expect(getState(instanceID).alertState).toBe(AlertStates.ALERT); }); test('sends no alert when some, but not all, criteria cross the threshold', async () => { - const instanceID = '*::test'; + const instanceID = '*'; await execute(Comparator.LT_OR_EQ, [1.0], [3.0]); expect(mostRecentAction(instanceID)).toBe(undefined); expect(getState(instanceID).alertState).toBe(AlertStates.OK); }); test('alerts only on groups that meet all criteria when querying with a groupBy parameter', async () => { - const instanceIdA = 'a::test'; - const instanceIdB = 'b::test'; + const instanceIdA = 'a'; + const instanceIdB = 'b'; await execute(Comparator.GT_OR_EQ, [1.0], [3.0], 'something'); expect(mostRecentAction(instanceIdA).id).toBe(FIRED_ACTIONS.id); expect(getState(instanceIdA).alertState).toBe(AlertStates.ALERT); @@ -199,7 +196,7 @@ describe('The metric threshold alert type', () => { expect(getState(instanceIdB).alertState).toBe(AlertStates.OK); }); test('sends all criteria to the action context', async () => { - const instanceID = '*::test'; + const instanceID = '*'; await execute(Comparator.GT_OR_EQ, [1.0], [3.0]); const { action } = mostRecentAction(instanceID); const reasons = action.reason.split('\n'); @@ -213,10 +210,9 @@ describe('The metric threshold alert type', () => { }); }); describe('querying with the count aggregator', () => { - const instanceID = '*::test'; + const instanceID = '*'; const execute = (comparator: Comparator, threshold: number[]) => executor({ - alertId: 'test', services, params: { criteria: [ @@ -240,10 +236,9 @@ describe('The metric threshold alert type', () => { }); }); describe('querying with the p99 aggregator', () => { - const instanceID = '*::test'; + const instanceID = '*'; const execute = (comparator: Comparator, threshold: number[]) => executor({ - alertId: 'test', services, params: { criteria: [ @@ -267,10 +262,9 @@ describe('The metric threshold alert type', () => { }); }); describe('querying with the p95 aggregator', () => { - const instanceID = '*::test'; + const instanceID = '*'; const execute = (comparator: Comparator, threshold: number[]) => executor({ - alertId: 'test', services, params: { criteria: [ @@ -294,10 +288,9 @@ describe('The metric threshold alert type', () => { }); }); describe("querying a metric that hasn't reported data", () => { - const instanceID = '*::test'; + const instanceID = '*'; const execute = (alertOnNoData: boolean) => executor({ - alertId: 'test', services, params: { criteria: [ @@ -324,10 +317,10 @@ describe('The metric threshold alert type', () => { }); // describe('querying a metric that later recovers', () => { - // const instanceID = '*::test'; + // const instanceID = '*'; // const execute = (threshold: number[]) => // executor({ - // alertId: 'test', + // // services, // params: { // criteria: [ diff --git a/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.ts b/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.ts index 0de3c553d2779..149b0711fbae9 100644 --- a/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.ts +++ b/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.ts @@ -19,7 +19,7 @@ import { evaluateAlert } from './lib/evaluate_alert'; export const createMetricThresholdExecutor = (libs: InfraBackendLibs) => async function (options: AlertExecutorOptions) { - const { services, params, alertId } = options; + const { services, params } = options; const { criteria } = params; const { sourceId, alertOnNoData } = params as { sourceId?: string; @@ -36,7 +36,7 @@ export const createMetricThresholdExecutor = (libs: InfraBackendLibs) => // Because each alert result has the same group definitions, just grap the groups from the first one. const groups = Object.keys(first(alertResults) as any); for (const group of groups) { - const alertInstance = services.alertInstanceFactory(`${group}::${alertId}`); + const alertInstance = services.alertInstanceFactory(`${group}`); // AND logic; all criteria must be across the threshold const shouldAlertFire = alertResults.every((result) => From 08d109fa23f4481cf27ea478712c24605e053cff Mon Sep 17 00:00:00 2001 From: Zacqary Xeper Date: Thu, 9 Jul 2020 17:00:31 -0500 Subject: [PATCH 3/5] Update alerting readme to clarify alertInstance ID uniqueness --- x-pack/plugins/alerts/README.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/x-pack/plugins/alerts/README.md b/x-pack/plugins/alerts/README.md index 811478426a8d3..631e029e4f87a 100644 --- a/x-pack/plugins/alerts/README.md +++ b/x-pack/plugins/alerts/README.md @@ -476,19 +476,21 @@ Params: |---|---|---| |id|The id of the alert you're trying to update the API key for. System will use user in request context to generate an API key for.|string| -## Schedule Formats +## Schedule Format A schedule is structured such that the key specifies the format you wish to use and its value specifies the schedule. We currently support the _Interval format_ which specifies the interval in seconds, minutes, hours or days at which the alert should execute. Example: `{ interval: "10s" }`, `{ interval: "5m" }`, `{ interval: "1h" }`, `{ interval: "1d" }`. -There are plans to support multiple other schedule formats in the near fuiture. +There are plans to support multiple other schedule formats in the near future. ## Alert instance factory **alertInstanceFactory(id)** -One service passed in to alert types is an alert instance factory. This factory creates instances of alerts and must be used in order to execute actions. The id you give to the alert instance factory is a unique identifier to the alert instance (ex: server identifier if the instance is about the server). The instance factory will use this identifier to retrieve the state of previous instances with the same id. These instances support state persisting between alert type execution, but will clear out once the alert instance stops executing. +One service passed in to alert types is an alert instance factory. This factory creates instances of alerts and must be used in order to execute actions. The `id` you give to the alert instance factory is a unique identifier to the alert instance (ex: server identifier if the instance is about the server). The instance factory will use this identifier to retrieve the state of previous instances with the same `id`. These instances support state persisting between alert type execution, but will clear out once the alert instance stops executing. + +Note that the `id` only needs to be unique **within the scope of a specific alert**, not unique across all alerts or alert types. For example, Alert 1 and Alert 2 can both create an alert instance with an `id` of `"a"` without conflicting with one another. But if Alert 1 creates 2 alert instances, then they must be differentiated with `id`s of `"a"` and `"b"`. This factory returns an instance of `AlertInstance`. The alert instance class has the following methods, note that we have removed the methods that you shouldn't touch. From 7adcd9d82388fd459aff1847cc56c0d98ec386c7 Mon Sep 17 00:00:00 2001 From: Zacqary Xeper Date: Thu, 9 Jul 2020 17:05:03 -0500 Subject: [PATCH 4/5] Fix test --- .../alerting/metric_threshold/metric_threshold_executor.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.test.ts b/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.test.ts index c9a0167f88691..914a09355d554 100644 --- a/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.test.ts +++ b/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.test.ts @@ -381,7 +381,6 @@ const mockLibs: any = { const executor = createMetricThresholdExecutor(mockLibs) as (opts: { params: AlertExecutorOptions['params']; services: { callCluster: AlertExecutorOptions['params']['callCluster'] }; - alertId: AlertExecutorOptions['alertId']; }) => Promise; const services: AlertServicesMock = alertsMock.createAlertServices(); From 70de35fdb6ef6b2e11f42e2932874b1a9bb00c82 Mon Sep 17 00:00:00 2001 From: Zacqary Xeper Date: Thu, 9 Jul 2020 17:20:03 -0500 Subject: [PATCH 5/5] Revert accidental readme change --- x-pack/plugins/alerts/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/alerts/README.md b/x-pack/plugins/alerts/README.md index 631e029e4f87a..2f2ffb52e7e90 100644 --- a/x-pack/plugins/alerts/README.md +++ b/x-pack/plugins/alerts/README.md @@ -476,7 +476,7 @@ Params: |---|---|---| |id|The id of the alert you're trying to update the API key for. System will use user in request context to generate an API key for.|string| -## Schedule Format +## Schedule Formats A schedule is structured such that the key specifies the format you wish to use and its value specifies the schedule. We currently support the _Interval format_ which specifies the interval in seconds, minutes, hours or days at which the alert should execute.