Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the alerts plugin support generics #72716

Merged
merged 22 commits into from
Aug 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import { i18n } from '@kbn/i18n';
import { Params } from './alert_type_params';
import { AlertExecutorOptions } from '../../../../alerts/server';
import { AlertExecutorOptions, AlertInstanceContext } from '../../../../alerts/server';

// alert type context provided to actions

Expand All @@ -19,7 +19,7 @@ export interface ActionContext extends BaseActionContext {
message: string;
}

export interface BaseActionContext {
export interface BaseActionContext extends AlertInstanceContext {
// the aggType used in the alert
// the value of the aggField, if used, otherwise 'all documents'
group: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import { i18n } from '@kbn/i18n';
import { AlertType, AlertExecutorOptions } from '../../types';
import { Params, ParamsSchema } from './alert_type_params';
import { BaseActionContext, addMessages } from './action_context';
import { ActionContext, BaseActionContext, addMessages } from './action_context';
import { TimeSeriesQuery } from './lib/time_series_query';
import { Service } from '../../types';
import { BUILT_IN_ALERTS_FEATURE_ID } from '../../../common';
Expand All @@ -19,7 +19,7 @@ const ActionGroupId = 'threshold met';
const ComparatorFns = getComparatorFns();
export const ComparatorFnNames = new Set(ComparatorFns.keys());

export function getAlertType(service: Service): AlertType {
export function getAlertType(service: Service): AlertType<Params, {}, {}, ActionContext> {
const { logger } = service;

const alertTypeName = i18n.translate('xpack.alertingBuiltins.indexThreshold.alertTypeTitle', {
Expand Down Expand Up @@ -118,9 +118,8 @@ export function getAlertType(service: Service): AlertType {
producer: BUILT_IN_ALERTS_FEATURE_ID,
};

async function executor(options: AlertExecutorOptions) {
const { alertId, name, services } = options;
const params: Params = options.params as Params;
async function executor(options: AlertExecutorOptions<Params, {}, {}, ActionContext>) {
const { alertId, name, services, params } = options;

const compareFn = ComparatorFns.get(params.thresholdComparator);
if (compareFn == null) {
Expand Down
9 changes: 6 additions & 3 deletions x-pack/plugins/alerts/common/alert.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@

import { SavedObjectAttributes } from 'kibana/server';

// eslint-disable-next-line @typescript-eslint/no-explicit-any
export type AlertTypeState = Record<string, any>;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export type AlertTypeParams = Record<string, any>;

Comment on lines +9 to +13
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't this be a generic type with a default of unknown instead of any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Converting these types to Record<string, unknown> caused over 130 type check failures. Most of these were outside of the alerting team's code and would cause us to change a lot of external code. I'm thinking we enforce this at some other time and have created #74897 to track this.

export interface IntervalSchedule extends SavedObjectAttributes {
interval: string;
}
Expand All @@ -28,9 +33,7 @@ export interface Alert {
consumer: string;
schedule: IntervalSchedule;
actions: AlertAction[];
// This will have to remain `any` until we can extend Alert Executors with generics
// eslint-disable-next-line @typescript-eslint/no-explicit-any
params: Record<string, any>;
params: AlertTypeParams;
scheduledTaskId?: string;
createdBy: string | null;
updatedBy: string | null;
Expand Down
3 changes: 3 additions & 0 deletions x-pack/plugins/alerts/common/alert_instance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ export type AlertInstanceMeta = t.TypeOf<typeof metaSchema>;
const stateSchema = t.record(t.string, t.unknown);
export type AlertInstanceState = t.TypeOf<typeof stateSchema>;

const contextSchema = t.record(t.string, t.unknown);
export type AlertInstanceContext = t.TypeOf<typeof contextSchema>;

export const rawAlertInstance = t.partial({
state: stateSchema,
meta: metaSchema,
Expand Down
32 changes: 19 additions & 13 deletions x-pack/plugins/alerts/server/alert_instance/alert_instance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,24 +8,26 @@ import {
AlertInstanceState,
RawAlertInstance,
rawAlertInstance,
AlertInstanceContext,
} from '../../common';

import { State, Context } from '../types';
import { parseDuration } from '../lib';

interface ScheduledExecutionOptions {
Copy link
Member

Choose a reason for hiding this comment

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

+1 on an interface removal - that's neat!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure, for this one here I had to move it within the AlertInstance class in order to use the generics passed to the class.

actionGroup: string;
context: Context;
state: State;
}
export type AlertInstances = Record<string, AlertInstance>;
export class AlertInstance {
private scheduledExecutionOptions?: ScheduledExecutionOptions;
export class AlertInstance<
State extends AlertInstanceState = AlertInstanceState,
Context extends AlertInstanceContext = AlertInstanceContext
> {
private scheduledExecutionOptions?: {
actionGroup: string;
context: Context;
state: State;
};
private meta: AlertInstanceMeta;
private state: AlertInstanceState;
private state: State;

constructor({ state = {}, meta = {} }: RawAlertInstance = {}) {
this.state = state;
constructor({ state, meta = {} }: RawAlertInstance = {}) {
this.state = (state || {}) as State;
this.meta = meta;
}

Expand Down Expand Up @@ -62,11 +64,15 @@ export class AlertInstance {
return this.state;
}

scheduleActions(actionGroup: string, context: Context = {}) {
scheduleActions(actionGroup: string, context?: Context) {
if (this.hasScheduledActions()) {
throw new Error('Alert instance execution has already been scheduled, cannot schedule twice');
}
this.scheduledExecutionOptions = { actionGroup, context, state: this.state };
this.scheduledExecutionOptions = {
actionGroup,
context: (context || {}) as Context,
state: this.state,
};
return this;
}

Expand Down
28 changes: 22 additions & 6 deletions x-pack/plugins/alerts/server/alert_type_registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,13 @@ import { schema } from '@kbn/config-schema';
import typeDetect from 'type-detect';
import { RunContext, TaskManagerSetupContract } from '../../task_manager/server';
import { TaskRunnerFactory } from './task_runner';
import { AlertType } from './types';
import {
AlertType,
AlertTypeParams,
AlertTypeState,
AlertInstanceState,
AlertInstanceContext,
} from './types';

interface ConstructorOptions {
taskManager: TaskManagerSetupContract;
Expand Down Expand Up @@ -59,7 +65,12 @@ export class AlertTypeRegistry {
return this.alertTypes.has(id);
}

public register(alertType: AlertType) {
public register<
Params extends AlertTypeParams = AlertTypeParams,
State extends AlertTypeState = AlertTypeState,
InstanceState extends AlertInstanceState = AlertInstanceState,
InstanceContext extends AlertInstanceContext = AlertInstanceContext
>(alertType: AlertType<Params, State, InstanceState, InstanceContext>) {
if (this.has(alertType.id)) {
throw new Error(
i18n.translate('xpack.alerts.alertTypeRegistry.register.duplicateAlertTypeError', {
Expand All @@ -71,18 +82,23 @@ export class AlertTypeRegistry {
);
}
alertType.actionVariables = normalizedActionVariables(alertType.actionVariables);
this.alertTypes.set(alertIdSchema.validate(alertType.id), { ...alertType });
this.alertTypes.set(alertIdSchema.validate(alertType.id), { ...alertType } as AlertType);
this.taskManager.registerTaskDefinitions({
[`alerting:${alertType.id}`]: {
title: alertType.name,
type: `alerting:${alertType.id}`,
createTaskRunner: (context: RunContext) =>
this.taskRunnerFactory.create(alertType, context),
this.taskRunnerFactory.create({ ...alertType } as AlertType, context),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

{ ...alertType } is a "while I'm here" for shallow clones that was missed in #65309.

},
});
}

public get(id: string): AlertType {
public get<
Params extends AlertTypeParams = AlertTypeParams,
State extends AlertTypeState = AlertTypeState,
InstanceState extends AlertInstanceState = AlertInstanceState,
InstanceContext extends AlertInstanceContext = AlertInstanceContext
>(id: string): AlertType<Params, State, InstanceState, InstanceContext> {
if (!this.has(id)) {
throw Boom.badRequest(
i18n.translate('xpack.alerts.alertTypeRegistry.get.missingAlertTypeError', {
Expand All @@ -93,7 +109,7 @@ export class AlertTypeRegistry {
})
);
}
return this.alertTypes.get(id)!;
return this.alertTypes.get(id)! as AlertType<Params, State, InstanceState, InstanceContext>;
}

public list(): Set<RegistryAlertType> {
Expand Down
5 changes: 4 additions & 1 deletion x-pack/plugins/alerts/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,11 @@ export {
AlertExecutorOptions,
AlertActionParams,
AlertServices,
State,
AlertTypeState,
AlertTypeParams,
PartialAlert,
AlertInstanceState,
AlertInstanceContext,
} from './types';
export { PluginSetupContract, PluginStartContract } from './plugin';
export { FindResult } from './alerts_client';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,18 @@
*/

import { map } from 'lodash';
import { AlertAction, State, Context, AlertType, AlertParams } from '../types';
import { Logger, KibanaRequest } from '../../../../../src/core/server';
import { transformActionParams } from './transform_action_params';
import { PluginStartContract as ActionsPluginStartContract } from '../../../actions/server';
import { IEventLogger, IEvent, SAVED_OBJECT_REL_PRIMARY } from '../../../event_log/server';
import { EVENT_LOG_ACTIONS } from '../plugin';
import {
AlertAction,
AlertInstanceState,
AlertInstanceContext,
AlertType,
AlertTypeParams,
} from '../types';

interface CreateExecutionHandlerOptions {
alertId: string;
Expand All @@ -24,14 +30,14 @@ interface CreateExecutionHandlerOptions {
logger: Logger;
eventLogger: IEventLogger;
request: KibanaRequest;
alertParams: AlertParams;
alertParams: AlertTypeParams;
}

interface ExecutionHandlerOptions {
actionGroup: string;
alertInstanceId: string;
context: Context;
state: State;
context: AlertInstanceContext;
state: AlertInstanceState;
}

export function createExecutionHandler({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@

import Mustache from 'mustache';
import { isString, cloneDeepWith } from 'lodash';
import { AlertActionParams, State, Context, AlertParams } from '../types';
import {
AlertActionParams,
AlertInstanceState,
AlertInstanceContext,
AlertTypeParams,
} from '../types';

interface TransformActionParamsOptions {
alertId: string;
Expand All @@ -15,9 +20,9 @@ interface TransformActionParamsOptions {
tags?: string[];
alertInstanceId: string;
actionParams: AlertActionParams;
state: State;
context: Context;
alertParams: AlertParams;
alertParams: AlertTypeParams;
state: AlertInstanceState;
context: AlertInstanceContext;
}

export function transformActionParams({
Expand Down
51 changes: 34 additions & 17 deletions x-pack/plugins/alerts/server/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import { AlertInstance } from './alert_instance';
import { AlertTypeRegistry as OrigAlertTypeRegistry } from './alert_type_registry';
import { PluginSetupContract, PluginStartContract } from './plugin';
import { Alert, AlertActionParams, ActionGroup } from '../common';
import { AlertsClient } from './alerts_client';
export * from '../common';
import {
Expand All @@ -17,13 +16,16 @@ import {
SavedObjectAttributes,
SavedObjectsClientContract,
} from '../../../../src/core/server';
import {
Alert,
AlertActionParams,
ActionGroup,
AlertTypeParams,
AlertTypeState,
AlertInstanceContext,
AlertInstanceState,
} from '../common';

// This will have to remain `any` until we can extend Alert Executors with generics
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export type State = Record<string, any>;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export type Context = Record<string, any>;
export type AlertParams = Record<string, unknown>;
export type WithoutQueryAndParams<T> = Pick<T, Exclude<keyof T, 'query' | 'params'>>;
export type GetServicesFunction = (request: KibanaRequest) => Services;
export type GetBasePathFunction = (spaceId?: string) => string;
Expand All @@ -44,18 +46,24 @@ export interface Services {
getLegacyScopedClusterClient(clusterClient: ILegacyClusterClient): ILegacyScopedClusterClient;
}

export interface AlertServices extends Services {
alertInstanceFactory: (id: string) => AlertInstance;
export interface AlertServices<
InstanceState extends AlertInstanceState = AlertInstanceState,
InstanceContext extends AlertInstanceContext = AlertInstanceContext
> extends Services {
alertInstanceFactory: (id: string) => AlertInstance<InstanceState, InstanceContext>;
}

export interface AlertExecutorOptions {
export interface AlertExecutorOptions<
Params extends AlertTypeParams = AlertTypeParams,
State extends AlertTypeState = AlertTypeState,
InstanceState extends AlertInstanceState = AlertInstanceState,
InstanceContext extends AlertInstanceContext = AlertInstanceContext
> {
alertId: string;
startedAt: Date;
previousStartedAt: Date | null;
services: AlertServices;
// This will have to remain `any` until we can extend Alert Executors with generics
// eslint-disable-next-line @typescript-eslint/no-explicit-any
params: Record<string, any>;
services: AlertServices<InstanceState, InstanceContext>;
params: Params;
state: State;
spaceId: string;
namespace?: string;
Expand All @@ -70,15 +78,24 @@ export interface ActionVariable {
description: string;
}

export interface AlertType {
export interface AlertType<
Params extends AlertTypeParams = AlertTypeParams,
State extends AlertTypeState = AlertTypeState,
InstanceState extends AlertInstanceState = AlertInstanceState,
InstanceContext extends AlertInstanceContext = AlertInstanceContext
> {
id: string;
name: string;
validate?: {
params?: { validate: (object: unknown) => AlertExecutorOptions['params'] };
params?: { validate: (object: unknown) => Params };
};
actionGroups: ActionGroup[];
defaultActionGroupId: ActionGroup['id'];
executor: ({ services, params, state }: AlertExecutorOptions) => Promise<State | void>;
executor: ({
services,
params,
state,
}: AlertExecutorOptions<Params, State, InstanceState, InstanceContext>) => Promise<State | void>;
producer: string;
actionVariables?: {
context?: ActionVariable[];
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/monitoring/server/alerts/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@
* you may not use this file except in compliance with the Elastic License.
*/
import { AlertMessageTokenType, AlertSeverity } from '../../common/enums';
import { AlertInstanceState as BaseAlertInstanceState } from '../../../alerts/server';

export interface AlertEnableAction {
id: string;
config: { [key: string]: any };
}

export interface AlertInstanceState {
export interface AlertInstanceState extends BaseAlertInstanceState {
alertStates: AlertState[];
}

Expand Down
Loading