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.

Sign up for GitHub

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

[Alerting] Encourage type safe usage of Alerting #86623

Merged
merged 9 commits into from
Dec 21, 2020
Merged

[Alerting] Encourage type safe usage of Alerting #86623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c4863a4
removed defaults from AlertType
gmmorris Dec 18, 2020
bfcab2a
removed `any` usage from AlertTypeState and AlertTypeParams
gmmorris Dec 21, 2020
561ebb1
Merge branch 'master' into alerts/remove-generics-defaults-redux
gmmorris Dec 21, 2020
5ddcf7a
fixed typing in tests
gmmorris Dec 21, 2020
93f10cb
removed dead code
gmmorris Dec 21, 2020
f4c543a
added TODOs
gmmorris Dec 21, 2020
aa95e0e
mixed test
gmmorris Dec 21, 2020
dd833fc
corrected AlertTypeModel typing
gmmorris Dec 21, 2020
1071113
renamed generics
gmmorris Dec 21, 2020
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
4 changes: 3 additions & 1 deletion x-pack/examples/alerting_example/common/constants.ts
View file
Open in desktop
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
4 changes: 2 additions & 2 deletions x-pack/examples/alerting_example/public/components/view_astros_alert.tsx
View file
Open in desktop
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
6 changes: 5 additions & 1 deletion x-pack/examples/alerting_example/server/alert_types/astros.ts
View file
Open in desktop
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
View file
Open in desktop
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
10 changes: 7 additions & 3 deletions x-pack/plugins/alerts/server/alert_instance/create_alert_instance_factory.ts
View file
Open in desktop
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
View file
Open in desktop
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,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't have mixed types in the Map, so we have to string these when we set them, and rely on the user to provide the right type when they get them.

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

Comment on lines +76 to +82
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By Untyped... what we mean is that we down-cast the types to their base type, but it sounded clearer to me than BaseTypeNormalizedAlertType or DownCastTypedNormalizedAlertType

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
Comment on lines +106 to +109
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed defaults so we force the developers to be explicit about the types they expect.
This should encourage them to use real types rather than just Record<string, whatever>

>(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