Skip to content

Commit

Permalink
[Alerting] Encourage type safe usage of Alerting (elastic#86623)
Browse files Browse the repository at this point in the history
This PR encourages type safe usage of the Alerting framework by replacing the current default Params/State/InstanceState/InstanceContext types (which are `AlertTypeParams`/`AlertTypeState`/etc.) with `never`.
This means that code can continue to omit the specific types for these fields, as long as they aren't referenced.
Once an alert developer wishes to actually reference the parameters (or state/context), then they have to specify the type.

This PR also changed the typing of the `AlertTypeParams` and `AlertTypeState` from `Record<string, any>` to `Record<string, unknown>`, to ensure that where these catch-all types are used they will at least enforce `unknown` rather than `any`.
This change broke some usage in both @elastic/kibana-alerting-services  plugins, but also other plugins in the Stack/Solutions. I tried to fix these where I could, but some of these require new types and refactoring in other teams' code, which I decided is best done by the team who own and maintain that code - I've added explicit `TODO` comments in all of these places, describing the required fix.

This PR also introduced a Generics based typing for the `Alert` type so that the `params` field can be typed as something other than `AlertTypeParams`.
  • Loading branch information
gmmorris committed Dec 21, 2020
1 parent 7c7dc5f commit e547549
Show file tree
Hide file tree
Showing 85 changed files with 1,044 additions and 523 deletions.
4 changes: 3 additions & 1 deletion x-pack/examples/alerting_example/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { AlertTypeParams } from '../../../plugins/alerts/common';

export const ALERTING_EXAMPLE_APP_ID = 'AlertingExample';

// always firing
export const DEFAULT_INSTANCES_TO_GENERATE = 5;
export interface AlwaysFiringParams {
export interface AlwaysFiringParams extends AlertTypeParams {
instances?: number;
thresholds?: {
small?: number;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { withRouter, RouteComponentProps } from 'react-router-dom';
import { CoreStart } from 'kibana/public';
import { isEmpty } from 'lodash';
import { Alert, AlertTaskState, BASE_ALERT_API_PATH } from '../../../../plugins/alerts/common';
import { ALERTING_EXAMPLE_APP_ID } from '../../common/constants';
import { ALERTING_EXAMPLE_APP_ID, AlwaysFiringParams } from '../../common/constants';

type Props = RouteComponentProps & {
http: CoreStart['http'];
Expand All @@ -34,7 +34,7 @@ function hasCraft(state: any): state is { craft: string } {
return state && state.craft;
}
export const ViewPeopleInSpaceAlertPage = withRouter(({ http, id }: Props) => {
const [alert, setAlert] = useState<Alert | null>(null);
const [alert, setAlert] = useState<Alert<AlwaysFiringParams> | null>(null);
const [alertState, setAlertState] = useState<AlertTaskState | null>(null);

useEffect(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,11 @@ function getCraftFilter(craft: string) {
craft === Craft.OuterSpace ? true : craft === person.craft;
}

export const alertType: AlertType = {
export const alertType: AlertType<
{ outerSpaceCapacity: number; craft: string; op: string },
{ peopleInSpace: number },
{ craft: string }
> = {
id: 'example.people-in-space',
name: 'People In Space Right Now',
actionGroups: [{ id: 'default', name: 'default' }],
Expand Down
12 changes: 5 additions & 7 deletions x-pack/plugins/alerts/common/alert.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,8 @@
import { SavedObjectAttribute, SavedObjectAttributes } from 'kibana/server';
import { AlertNotifyWhenType } from './alert_notify_when_type';

// 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>;
export type AlertTypeState = Record<string, unknown>;
export type AlertTypeParams = Record<string, unknown>;

export interface IntervalSchedule extends SavedObjectAttributes {
interval: string;
Expand Down Expand Up @@ -52,7 +50,7 @@ export interface AlertAggregations {
alertExecutionStatus: { [status: string]: number };
}

export interface Alert {
export interface Alert<Params extends AlertTypeParams = never> {
id: string;
enabled: boolean;
name: string;
Expand All @@ -61,7 +59,7 @@ export interface Alert {
consumer: string;
schedule: IntervalSchedule;
actions: AlertAction[];
params: AlertTypeParams;
params: Params;
scheduledTaskId?: string;
createdBy: string | null;
updatedBy: string | null;
Expand All @@ -76,7 +74,7 @@ export interface Alert {
executionStatus: AlertExecutionStatus;
}

export type SanitizedAlert = Omit<Alert, 'apiKey'>;
export type SanitizedAlert<Params extends AlertTypeParams = never> = Omit<Alert<Params>, 'apiKey'>;

export enum HealthStatus {
OK = 'ok',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,16 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { AlertInstanceContext, AlertInstanceState } from '../types';
import { AlertInstance } from './alert_instance';

export function createAlertInstanceFactory(alertInstances: Record<string, AlertInstance>) {
return (id: string): AlertInstance => {
export function createAlertInstanceFactory<
InstanceState extends AlertInstanceState,
InstanceContext extends AlertInstanceContext
>(alertInstances: Record<string, AlertInstance<InstanceState, InstanceContext>>) {
return (id: string): AlertInstance<InstanceState, InstanceContext> => {
if (!alertInstances[id]) {
alertInstances[id] = new AlertInstance();
alertInstances[id] = new AlertInstance<InstanceState, InstanceContext>();
}

return alertInstances[id];
Expand Down
43 changes: 29 additions & 14 deletions x-pack/plugins/alerts/server/alert_type_registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export interface ConstructorOptions {

export interface RegistryAlertType
extends Pick<
NormalizedAlertType,
UntypedNormalizedAlertType,
| 'name'
| 'actionGroups'
| 'recoveryActionGroup'
Expand Down Expand Up @@ -66,16 +66,23 @@ const alertIdSchema = schema.string({
});

export type NormalizedAlertType<
Params extends AlertTypeParams = AlertTypeParams,
State extends AlertTypeState = AlertTypeState,
InstanceState extends AlertInstanceState = AlertInstanceState,
InstanceContext extends AlertInstanceContext = AlertInstanceContext
Params extends AlertTypeParams,
State extends AlertTypeState,
InstanceState extends AlertInstanceState,
InstanceContext extends AlertInstanceContext
> = Omit<AlertType<Params, State, InstanceState, InstanceContext>, 'recoveryActionGroup'> &
Pick<Required<AlertType<Params, State, InstanceState, InstanceContext>>, 'recoveryActionGroup'>;

export type UntypedNormalizedAlertType = NormalizedAlertType<
AlertTypeParams,
AlertTypeState,
AlertInstanceState,
AlertInstanceContext
>;

export class AlertTypeRegistry {
private readonly taskManager: TaskManagerSetupContract;
private readonly alertTypes: Map<string, NormalizedAlertType> = new Map();
private readonly alertTypes: Map<string, UntypedNormalizedAlertType> = new Map();
private readonly taskRunnerFactory: TaskRunnerFactory;
private readonly licenseState: ILicenseState;
private readonly licensing: LicensingPluginSetup;
Expand All @@ -96,10 +103,10 @@ export class AlertTypeRegistry {
}

public register<
Params extends AlertTypeParams = AlertTypeParams,
State extends AlertTypeState = AlertTypeState,
InstanceState extends AlertInstanceState = AlertInstanceState,
InstanceContext extends AlertInstanceContext = AlertInstanceContext
Params extends AlertTypeParams,
State extends AlertTypeState,
InstanceState extends AlertInstanceState,
InstanceContext extends AlertInstanceContext
>(alertType: AlertType<Params, State, InstanceState, InstanceContext>) {
if (this.has(alertType.id)) {
throw new Error(
Expand All @@ -113,14 +120,22 @@ export class AlertTypeRegistry {
}
alertType.actionVariables = normalizedActionVariables(alertType.actionVariables);

const normalizedAlertType = augmentActionGroupsWithReserved(alertType as AlertType);
const normalizedAlertType = augmentActionGroupsWithReserved<
Params,
State,
InstanceState,
InstanceContext
>(alertType);

this.alertTypes.set(alertIdSchema.validate(alertType.id), normalizedAlertType);
this.alertTypes.set(
alertIdSchema.validate(alertType.id),
normalizedAlertType as UntypedNormalizedAlertType
);
this.taskManager.registerTaskDefinitions({
[`alerting:${alertType.id}`]: {
title: alertType.name,
createTaskRunner: (context: RunContext) =>
this.taskRunnerFactory.create(normalizedAlertType, context),
this.taskRunnerFactory.create(normalizedAlertType as UntypedNormalizedAlertType, context),
},
});
// No need to notify usage on basic alert types
Expand Down Expand Up @@ -170,7 +185,7 @@ export class AlertTypeRegistry {
producer,
minimumLicenseRequired,
},
]: [string, NormalizedAlertType]) => ({
]: [string, UntypedNormalizedAlertType]) => ({
id,
name,
actionGroups,
Expand Down
Loading

0 comments on commit e547549

Please sign in to comment.