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

[Actions] Notify only on action group change #82969

Merged
merged 60 commits into from
Dec 10, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
01c7c7d
plugged Task Manager lifecycle into status reactively
gmmorris Nov 3, 2020
e9f2cd0
fixed tests
gmmorris Nov 5, 2020
42395e9
Revert "fixed tests"
gmmorris Nov 5, 2020
6d00ce8
made action group fields optional
gmmorris Nov 5, 2020
2730301
Merge branch 'master' into alerts/assign-action-groups
gmmorris Nov 5, 2020
89deeca
revert deletion
gmmorris Nov 5, 2020
73fe0a8
again
gmmorris Nov 5, 2020
401d99f
Merge branch 'master' into alerts/assign-action-groups
gmmorris Nov 5, 2020
78873a1
extracted action type for mto its own component
gmmorris Nov 5, 2020
85a1068
extracted more sections of the action form to their own components
gmmorris Nov 5, 2020
44eb670
updated icon
gmmorris Nov 5, 2020
4b36391
added docs
gmmorris Nov 5, 2020
afc6e81
fixed always firing alert
gmmorris Nov 5, 2020
38fb659
fixed export of components
gmmorris Nov 5, 2020
6fe4ae4
fixed react warning
gmmorris Nov 6, 2020
f68763e
Merge branch 'master' of https://github.com/elastic/kibana into pr/82472
ymao1 Nov 6, 2020
9d1f594
Adding flag for notifying on state change
ymao1 Nov 6, 2020
9005c1e
Updating logic in task runner
ymao1 Nov 6, 2020
1422911
Starting to update tests
ymao1 Nov 6, 2020
7b7f3c8
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 Nov 9, 2020
a0e3ac5
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 Nov 9, 2020
d37288c
Adding tests
ymao1 Nov 9, 2020
e0fa427
Merging in master
ymao1 Nov 9, 2020
22b7a00
Fixing types check
ymao1 Nov 9, 2020
a075514
Tests and types
ymao1 Nov 9, 2020
8311ab8
Tests
ymao1 Nov 9, 2020
e21ea91
Tests
ymao1 Nov 9, 2020
5e0ee21
Tests
ymao1 Nov 9, 2020
f81b436
Tests
ymao1 Nov 9, 2020
df7d7af
Tests
ymao1 Nov 9, 2020
6fa36a2
Merging in master
ymao1 Nov 23, 2020
4896a35
Renaming field to a more descriptive name. Adding migrations
ymao1 Nov 23, 2020
13fea7b
Renaming field to a more descriptive name. Adding migrations
ymao1 Nov 23, 2020
8f94818
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 Nov 23, 2020
2932b4e
Fixing tests
ymao1 Nov 24, 2020
4e65be8
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 Dec 7, 2020
7b57ff8
Type check and tests
ymao1 Dec 8, 2020
f062f95
Moving schedule and notify interval to bottom of flyout. Implementing…
ymao1 Dec 8, 2020
a3eac70
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 Dec 8, 2020
ff0ef3b
Changing boolean flag to enum type and updating in triggers_actions_ui
ymao1 Dec 8, 2020
e331498
Changing boolean flag to enum type and updating in alerts plugin
ymao1 Dec 8, 2020
d778639
Fixing types check
ymao1 Dec 8, 2020
e31ee56
Fixing monitoring jest tests
ymao1 Dec 8, 2020
01dbeb4
Changing last references to old variable names
ymao1 Dec 8, 2020
2e26609
Merging in master
ymao1 Dec 8, 2020
35f60eb
Moving form inputs back to the top
ymao1 Dec 8, 2020
819d959
Renaming to alert_notify_when
ymao1 Dec 8, 2020
f835ef1
Updating functional tests
ymao1 Dec 8, 2020
2b5c53c
Adding new functional test for notifyWhen onActionGroupChange
ymao1 Dec 8, 2020
9a4b5d4
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 Dec 8, 2020
9697efd
Merging in master
ymao1 Dec 9, 2020
1adf049
Updating wording
ymao1 Dec 9, 2020
3b1813a
Merging in master
ymao1 Dec 9, 2020
38bd887
Merging in master
ymao1 Dec 10, 2020
b2f6610
Incorporating action subgroups into logic
ymao1 Dec 10, 2020
6765bab
PR fixes
ymao1 Dec 10, 2020
86deabd
Updating functional test
ymao1 Dec 10, 2020
85ed885
Fixing types check
ymao1 Dec 10, 2020
dd3b29a
Changing default throttle interval to hour
ymao1 Dec 10, 2020
46e8025
Fixing types check
ymao1 Dec 10, 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
2 changes: 2 additions & 0 deletions x-pack/plugins/alerts/common/alert.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/

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>;
Expand Down Expand Up @@ -68,6 +69,7 @@ export interface Alert {
apiKey: string | null;
apiKeyOwner: string | null;
throttle: string | null;
notifyWhen: AlertNotifyWhenType | null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the field from a boolean to a string type with 3 options. This aligns better. with the dropdown selector with 3 options. Allowing this to be null to provide backwards compatibility.

Copy link
Member

Choose a reason for hiding this comment

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

I see we are setting a value for this in the migration, so in theory I don't think we have to allow a null value. I think. We do however have some folks internally who do upgrade deployments mid-version (security and o11y test deployments), and having a null value would be useful for them, since the migrations don't actually run in those cases. But I kinda hate to have to allow null for only that reason - if that is the only reason. @mikecote ?

Copy link
Contributor

@mikecote mikecote Dec 10, 2020

Choose a reason for hiding this comment

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

@pmuellr I was thinking more in the context of stack monitoring, SIEM and whoever else using our APIs today. Adding a required field would break their usage (of alerts client / HTTP API) and I was thinking we could apply the same logic as the migrations for now to determine what value to set for these calls. Later on, 8.0, make this a mandatory field (follow up).

muteAll: boolean;
mutedInstanceIds: string[];
executionStatus: AlertExecutionStatus;
Expand Down
18 changes: 18 additions & 0 deletions x-pack/plugins/alerts/common/alert_notify_when_type.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/*
* 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.
*/

import { validateNotifyWhenType } from './alert_notify_when_type';

test('validates valid notify when type', () => {
expect(validateNotifyWhenType('onActionGroupChange')).toBeUndefined();
expect(validateNotifyWhenType('onActiveAlert')).toBeUndefined();
expect(validateNotifyWhenType('onThrottleInterval')).toBeUndefined();
});
test('returns error string if input is not valid notify when type', () => {
expect(validateNotifyWhenType('randomString')).toEqual(
`string is not a valid AlertNotifyWhenType: randomString`
);
});
19 changes: 19 additions & 0 deletions x-pack/plugins/alerts/common/alert_notify_when_type.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/*
* 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.
*/

const AlertNotifyWhenTypeValues = [
'onActionGroupChange',
'onActiveAlert',
'onThrottleInterval',
] as const;
Copy link
Member

Choose a reason for hiding this comment

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

@gmmorris noted the needed as const on here - we are using this pattern in one other place, and had the same issue which Gidi also caught when it was introduced. I wonder whether we should stop using this pattern, despite it being soooo useful.

export type AlertNotifyWhenType = typeof AlertNotifyWhenTypeValues[number];

export function validateNotifyWhenType(notifyWhen: string) {
if (AlertNotifyWhenTypeValues.includes(notifyWhen as AlertNotifyWhenType)) {
return;
}
return `string is not a valid AlertNotifyWhenType: ${notifyWhen}`;
}
1 change: 1 addition & 0 deletions x-pack/plugins/alerts/common/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export * from './alert_navigation';
export * from './alert_instance_summary';
export * from './builtin_action_groups';
export * from './disabled_action_groups';
export * from './alert_notify_when_type';

export interface AlertingFrameworkHealth {
isSufficientlySecure: boolean;
Expand Down
108 changes: 108 additions & 0 deletions x-pack/plugins/alerts/server/alert_instance/alert_instance.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,114 @@ describe('isThrottled', () => {
});
});

describe('scheduledActionGroupOrSubgroupHasChanged()', () => {
test('should be false if no last scheduled and nothing scheduled', () => {
const alertInstance = new AlertInstance();
expect(alertInstance.scheduledActionGroupOrSubgroupHasChanged()).toEqual(false);
});

test('should be false if group does not change', () => {
const alertInstance = new AlertInstance({
meta: {
lastScheduledActions: {
date: new Date(),
group: 'default',
},
},
});
alertInstance.scheduleActions('default');
expect(alertInstance.scheduledActionGroupOrSubgroupHasChanged()).toEqual(false);
});

test('should be false if group and subgroup does not change', () => {
const alertInstance = new AlertInstance({
meta: {
lastScheduledActions: {
date: new Date(),
group: 'default',
subgroup: 'subgroup',
},
},
});
alertInstance.scheduleActionsWithSubGroup('default', 'subgroup');
expect(alertInstance.scheduledActionGroupOrSubgroupHasChanged()).toEqual(false);
});

test('should be false if group does not change and subgroup goes from undefined to defined', () => {
const alertInstance = new AlertInstance({
meta: {
lastScheduledActions: {
date: new Date(),
group: 'default',
},
},
});
alertInstance.scheduleActionsWithSubGroup('default', 'subgroup');
expect(alertInstance.scheduledActionGroupOrSubgroupHasChanged()).toEqual(false);
});

test('should be false if group does not change and subgroup goes from defined to undefined', () => {
const alertInstance = new AlertInstance({
meta: {
lastScheduledActions: {
date: new Date(),
group: 'default',
subgroup: 'subgroup',
},
},
});
alertInstance.scheduleActions('default');
expect(alertInstance.scheduledActionGroupOrSubgroupHasChanged()).toEqual(false);
});

test('should be true if no last scheduled and has scheduled action', () => {
const alertInstance = new AlertInstance();
alertInstance.scheduleActions('default');
expect(alertInstance.scheduledActionGroupOrSubgroupHasChanged()).toEqual(true);
});

test('should be true if group does change', () => {
const alertInstance = new AlertInstance({
meta: {
lastScheduledActions: {
date: new Date(),
group: 'default',
},
},
});
alertInstance.scheduleActions('penguin');
expect(alertInstance.scheduledActionGroupOrSubgroupHasChanged()).toEqual(true);
});

test('should be true if group does change and subgroup does change', () => {
const alertInstance = new AlertInstance({
meta: {
lastScheduledActions: {
date: new Date(),
group: 'default',
subgroup: 'subgroup',
},
},
});
alertInstance.scheduleActionsWithSubGroup('penguin', 'fish');
expect(alertInstance.scheduledActionGroupOrSubgroupHasChanged()).toEqual(true);
});

test('should be true if group does not change and subgroup does change', () => {
const alertInstance = new AlertInstance({
meta: {
lastScheduledActions: {
date: new Date(),
group: 'default',
subgroup: 'subgroup',
},
},
});
alertInstance.scheduleActionsWithSubGroup('default', 'fish');
expect(alertInstance.scheduledActionGroupOrSubgroupHasChanged()).toEqual(true);
});
});

describe('getScheduledActionOptions()', () => {
test('defaults to undefined', () => {
const alertInstance = new AlertInstance();
Expand Down
25 changes: 25 additions & 0 deletions x-pack/plugins/alerts/server/alert_instance/alert_instance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,31 @@ export class AlertInstance<
return false;
}

scheduledActionGroupOrSubgroupHasChanged(): boolean {
if (!this.meta.lastScheduledActions && this.scheduledExecutionOptions) {
// it is considered a change when there are no previous scheduled actions
// and new scheduled actions
return true;
}

if (this.meta.lastScheduledActions && this.scheduledExecutionOptions) {
// compare previous and new scheduled actions if both exist
return (
!this.scheduledActionGroupIsUnchanged(
this.meta.lastScheduledActions,
this.scheduledExecutionOptions
) ||
!this.scheduledActionSubgroupIsUnchanged(
this.meta.lastScheduledActions,
this.scheduledExecutionOptions
)
);
}

// no previous and no new scheduled actions
return false;
}

private scheduledActionGroupIsUnchanged(
lastScheduledActions: NonNullable<AlertInstanceMeta['lastScheduledActions']>,
scheduledExecutionOptions: ScheduledExecutionOptions<State, Context>
Expand Down
16 changes: 14 additions & 2 deletions x-pack/plugins/alerts/server/alerts_client/alerts_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,13 @@ import {
AlertTaskState,
AlertInstanceSummary,
AlertExecutionStatusValues,
AlertNotifyWhenType,
} from '../types';
import { validateAlertTypeParams, alertExecutionStatusFromRaw } from '../lib';
import {
validateAlertTypeParams,
alertExecutionStatusFromRaw,
getAlertNotifyWhenType,
} from '../lib';
import {
GrantAPIKeyResult as SecurityPluginGrantAPIKeyResult,
InvalidateAPIKeyResult as SecurityPluginInvalidateAPIKeyResult,
Expand Down Expand Up @@ -157,6 +162,7 @@ interface UpdateOptions {
actions: NormalizedAlertAction[];
params: Record<string, unknown>;
throttle: string | null;
notifyWhen: AlertNotifyWhenType | null;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to accept a null here, but for backward compatibility we could make this property optional, and then calculate the value with getAlertNotifyWhenType(). The same is true with the create call, but the typing on that is a little more complicated ...

};
}

Expand Down Expand Up @@ -251,6 +257,8 @@ export class AlertsClient {
const createTime = Date.now();
const { references, actions } = await this.denormalizeActions(data.actions);

const notifyWhen = getAlertNotifyWhenType(data.notifyWhen, data.throttle);

const rawAlert: RawAlert = {
...data,
...this.apiKeyAsAlertAttributes(createdAPIKey, username),
Expand All @@ -262,6 +270,7 @@ export class AlertsClient {
params: validatedAlertTypeParams as RawAlert['params'],
muteAll: false,
mutedInstanceIds: [],
notifyWhen,
executionStatus: {
status: 'pending',
lastExecutionDate: new Date().toISOString(),
Expand Down Expand Up @@ -694,6 +703,7 @@ export class AlertsClient {
? await this.createAPIKey(this.generateAPIKeyName(alertType.id, data.name))
: null;
const apiKeyAttributes = this.apiKeyAsAlertAttributes(createdAPIKey, username);
const notifyWhen = getAlertNotifyWhenType(data.notifyWhen, data.throttle);

let updatedObject: SavedObject<RawAlert>;
const createAttributes = this.updateMeta({
Expand All @@ -702,6 +712,7 @@ export class AlertsClient {
...apiKeyAttributes,
params: validatedAlertTypeParams as RawAlert['params'],
actions,
notifyWhen,
updatedBy: username,
updatedAt: new Date().toISOString(),
});
Expand Down Expand Up @@ -1326,7 +1337,7 @@ export class AlertsClient {

private getPartialAlertFromRaw(
id: string,
{ createdAt, updatedAt, meta, scheduledTaskId, ...rawAlert }: Partial<RawAlert>,
{ createdAt, updatedAt, meta, notifyWhen, scheduledTaskId, ...rawAlert }: Partial<RawAlert>,
references: SavedObjectReference[] | undefined
): PartialAlert {
// Not the prettiest code here, but if we want to use most of the
Expand All @@ -1341,6 +1352,7 @@ export class AlertsClient {
const executionStatus = alertExecutionStatusFromRaw(this.logger, id, rawAlert.executionStatus);
return {
id,
notifyWhen,
...rawAlertWithoutExecutionStatus,
// we currently only support the Interval Schedule type
// Once we support additional types, this type signature will likely change
Expand Down
Loading