-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Changes from all commits
01c7c7d
e9f2cd0
42395e9
6d00ce8
2730301
89deeca
73fe0a8
401d99f
78873a1
85a1068
44eb670
4b36391
afc6e81
38fb659
6fe4ae4
f68763e
9d1f594
9005c1e
1422911
7b7f3c8
a0e3ac5
d37288c
e0fa427
22b7a00
a075514
8311ab8
e21ea91
5e0ee21
f81b436
df7d7af
6fa36a2
4896a35
13fea7b
8f94818
2932b4e
4e65be8
7b57ff8
f062f95
a3eac70
ff0ef3b
e331498
d778639
e31ee56
01dbeb4
2e26609
35f60eb
819d959
f835ef1
2b5c53c
9a4b5d4
9697efd
1adf049
3b1813a
38bd887
b2f6610
6765bab
86deabd
85ed885
dd3b29a
46e8025
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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` | ||
); | ||
}); |
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; | ||
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. @gmmorris noted the needed |
||
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}`; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -157,6 +162,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 +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), | ||
|
@@ -262,6 +270,7 @@ export class AlertsClient { | |
params: validatedAlertTypeParams as RawAlert['params'], | ||
muteAll: false, | ||
mutedInstanceIds: [], | ||
notifyWhen, | ||
executionStatus: { | ||
status: 'pending', | ||
lastExecutionDate: new Date().toISOString(), | ||
|
@@ -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({ | ||
|
@@ -702,6 +712,7 @@ export class AlertsClient { | |
...apiKeyAttributes, | ||
params: validatedAlertTypeParams as RawAlert['params'], | ||
actions, | ||
notifyWhen, | ||
updatedBy: username, | ||
updatedAt: new Date().toISOString(), | ||
}); | ||
|
@@ -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 | ||
|
@@ -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 | ||
|
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).