-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
ymao1
merged 60 commits into
elastic:master
from
ymao1:alerting/notify-on-action-group-change
Dec 10, 2020
Merged
Changes from 53 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 e9f2cd0
fixed tests
gmmorris 42395e9
Revert "fixed tests"
gmmorris 6d00ce8
made action group fields optional
gmmorris 2730301
Merge branch 'master' into alerts/assign-action-groups
gmmorris 89deeca
revert deletion
gmmorris 73fe0a8
again
gmmorris 401d99f
Merge branch 'master' into alerts/assign-action-groups
gmmorris 78873a1
extracted action type for mto its own component
gmmorris 85a1068
extracted more sections of the action form to their own components
gmmorris 44eb670
updated icon
gmmorris 4b36391
added docs
gmmorris afc6e81
fixed always firing alert
gmmorris 38fb659
fixed export of components
gmmorris 6fe4ae4
fixed react warning
gmmorris f68763e
Merge branch 'master' of https://github.com/elastic/kibana into pr/82472
ymao1 9d1f594
Adding flag for notifying on state change
ymao1 9005c1e
Updating logic in task runner
ymao1 1422911
Starting to update tests
ymao1 7b7f3c8
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 a0e3ac5
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 d37288c
Adding tests
ymao1 e0fa427
Merging in master
ymao1 22b7a00
Fixing types check
ymao1 a075514
Tests and types
ymao1 8311ab8
Tests
ymao1 e21ea91
Tests
ymao1 5e0ee21
Tests
ymao1 f81b436
Tests
ymao1 df7d7af
Tests
ymao1 6fa36a2
Merging in master
ymao1 4896a35
Renaming field to a more descriptive name. Adding migrations
ymao1 13fea7b
Renaming field to a more descriptive name. Adding migrations
ymao1 8f94818
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 2932b4e
Fixing tests
ymao1 4e65be8
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 7b57ff8
Type check and tests
ymao1 f062f95
Moving schedule and notify interval to bottom of flyout. Implementing…
ymao1 a3eac70
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 ff0ef3b
Changing boolean flag to enum type and updating in triggers_actions_ui
ymao1 e331498
Changing boolean flag to enum type and updating in alerts plugin
ymao1 d778639
Fixing types check
ymao1 e31ee56
Fixing monitoring jest tests
ymao1 01dbeb4
Changing last references to old variable names
ymao1 2e26609
Merging in master
ymao1 35f60eb
Moving form inputs back to the top
ymao1 819d959
Renaming to alert_notify_when
ymao1 f835ef1
Updating functional tests
ymao1 2b5c53c
Adding new functional test for notifyWhen onActionGroupChange
ymao1 9a4b5d4
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 9697efd
Merging in master
ymao1 1adf049
Updating wording
ymao1 3b1813a
Merging in master
ymao1 38bd887
Merging in master
ymao1 b2f6610
Incorporating action subgroups into logic
ymao1 6765bab
PR fixes
ymao1 86deabd
Updating functional test
ymao1 85ed885
Fixing types check
ymao1 dd3b29a
Changing default throttle interval to hour
ymao1 46e8025
Fixing types check
ymao1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
18 changes: 18 additions & 0 deletions
18
x-pack/plugins/alerts/common/alert_notify_when_type.test.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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` | ||
); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
/* | ||
* 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']; | ||
ymao1 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
export type AlertNotifyWhenType = typeof AlertNotifyWhenTypeValues[number]; | ||
ymao1 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
export function validateNotifyWhenType(notifyWhen: string) { | ||
if (AlertNotifyWhenTypeValues.includes(notifyWhen)) { | ||
return; | ||
} | ||
return `string is not a valid AlertNotifyWhenType: ${notifyWhen}`; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,7 @@ import { | |
AlertTaskState, | ||
AlertInstanceSummary, | ||
AlertExecutionStatusValues, | ||
AlertNotifyWhenType, | ||
} from '../types'; | ||
import { validateAlertTypeParams, alertExecutionStatusFromRaw } from '../lib'; | ||
import { | ||
|
@@ -157,6 +158,7 @@ interface UpdateOptions { | |
actions: NormalizedAlertAction[]; | ||
params: Record<string, unknown>; | ||
throttle: string | null; | ||
notifyWhen: AlertNotifyWhenType | null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need to accept a |
||
}; | ||
} | ||
|
||
|
@@ -251,6 +253,8 @@ export class AlertsClient { | |
const createTime = Date.now(); | ||
const { references, actions } = await this.denormalizeActions(data.actions); | ||
|
||
const notifyWhen = this.validateAlertNotifyWhenType(data.notifyWhen, data.throttle); | ||
|
||
const rawAlert: RawAlert = { | ||
...data, | ||
...this.apiKeyAsAlertAttributes(createdAPIKey, username), | ||
|
@@ -262,6 +266,7 @@ export class AlertsClient { | |
params: validatedAlertTypeParams as RawAlert['params'], | ||
muteAll: false, | ||
mutedInstanceIds: [], | ||
notifyWhen, | ||
executionStatus: { | ||
status: 'pending', | ||
lastExecutionDate: new Date().toISOString(), | ||
|
@@ -694,6 +699,7 @@ export class AlertsClient { | |
? await this.createAPIKey(this.generateAPIKeyName(alertType.id, data.name)) | ||
: null; | ||
const apiKeyAttributes = this.apiKeyAsAlertAttributes(createdAPIKey, username); | ||
const notifyWhen = this.validateAlertNotifyWhenType(data.notifyWhen, data.throttle); | ||
|
||
let updatedObject: SavedObject<RawAlert>; | ||
const createAttributes = this.updateMeta({ | ||
|
@@ -702,6 +708,7 @@ export class AlertsClient { | |
...apiKeyAttributes, | ||
params: validatedAlertTypeParams as RawAlert['params'], | ||
actions, | ||
notifyWhen, | ||
updatedBy: username, | ||
updatedAt: new Date().toISOString(), | ||
}); | ||
|
@@ -1326,7 +1333,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 | ||
|
@@ -1341,6 +1348,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 | ||
|
@@ -1374,6 +1382,15 @@ export class AlertsClient { | |
} | ||
} | ||
|
||
private validateAlertNotifyWhenType( | ||
ymao1 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
notifyWhen: AlertNotifyWhenType | null, | ||
throttle: string | null | ||
): AlertNotifyWhenType { | ||
// We allow notifyWhen to be null for backwards compatibility. If it is null, determine its | ||
// value based on whether the throttle is set to a value or null | ||
return notifyWhen ? notifyWhen! : throttle ? 'onThrottleInterval' : 'onActiveAlert'; | ||
} | ||
|
||
private async denormalizeActions( | ||
alertActions: NormalizedAlertAction[] | ||
): Promise<{ actions: RawAlert['actions']; references: SavedObjectReference[] }> { | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 anull
value would be useful for them, since the migrations don't actually run in those cases. But I kinda hate to have to allownull
for only that reason - if that is the only reason. @mikecote ?There was a problem hiding this comment.
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).