-
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
Add circuit breaker for max number of actions by connector type #128319
Merged
ersin-erdal
merged 44 commits into
elastic:main
from
ersin-erdal:126504-connector-type-overrides
Apr 26, 2022
Merged
Changes from 43 commits
Commits
Show all changes
44 commits
Select commit
Hold shift + click to select a range
0cc3fa6
connectorTypeOverrides key in kibana.yml can create a connector type …
ersin-erdal 04582f2
Update docs and docker allowed keys
ersin-erdal 3e293b0
Fix connectorTypeSchema max schema
ersin-erdal 65af35b
Remove config from rule type.
ersin-erdal 4ddd789
Merge branch 'main' into 126504-connector-type-overrides
ersin-erdal eab40ad
Merge remote-tracking branch 'origin/126504-connector-type-overrides'…
ersin-erdal bce5239
Remove the redundant RuleTypeConfig interface
ersin-erdal e0cc496
Merge branch 'main' into 126504-connector-type-overrides
kibanamachine d32c813
Set and check numberOfTriggeredActions by connector type
ersin-erdal 2634526
Merge remote-tracking branch 'origin/126504-connector-type-overrides'…
ersin-erdal 22efc8e
Merge branch 'main' of https://github.com/elastic/kibana into 126504-…
ersin-erdal ef8f42c
Calculate numberOfTriggeredAction in the same execution cycle
ersin-erdal 1f03025
Merge branch 'main' into 126504-connector-type-overrides
ersin-erdal c4d37bb
fix test
ersin-erdal 6c27c1d
Merge remote-tracking branch 'origin/126504-connector-type-overrides'…
ersin-erdal 50390c8
Merge branch 'main' of https://github.com/elastic/kibana into 126504-…
ersin-erdal 88d0fa4
Merge branch 'main' of https://github.com/elastic/kibana into 126504-…
ersin-erdal afbd3f4
Refactor AlertExecutionStore
ersin-erdal 7c2139c
Functional test for capped action/connector type
ersin-erdal 709054d
Functional test withoutAuth
ersin-erdal 490207a
fix 403 problem
ersin-erdal a98770d
Merge branch 'main' into 126504-connector-type-overrides
ersin-erdal 17efd86
Fix as per the reviews
ersin-erdal f0cd050
Merge remote-tracking branch 'origin/126504-connector-type-overrides'…
ersin-erdal 13fd3e2
Merge branch 'main' of https://github.com/elastic/kibana into 126504-…
ersin-erdal 50e8791
merge
ersin-erdal 007ceb9
Log capped connector type once, keep number of sheduled actions by co…
ersin-erdal 46bc049
Merge branch 'main' of https://github.com/elastic/kibana into 126504-…
ersin-erdal b34b1ef
merge conflicts
ersin-erdal 914a5a0
Fix docs bug
ersin-erdal b325d2c
rename overlooked "execution" key in test config
ersin-erdal 2945da6
Merge branch 'main' into 126504-connector-type-overrides
kibanamachine d6ee25f
Merge branch 'main' of https://github.com/elastic/kibana into 126504-…
ersin-erdal eae4998
Resolve Merge Conflicts and rename ScheduledActions to GeneratedActions
ersin-erdal 9bfb570
Merge remote-tracking branch 'origin/126504-connector-type-overrides'…
ersin-erdal ad6a1b3
Update x-pack/plugins/alerting/server/config.ts
ersin-erdal 71acc21
fix number_of_generated_actions
ersin-erdal 9b8657f
Merge remote-tracking branch 'origin/126504-connector-type-overrides'…
ersin-erdal 5715a96
Merge branch 'main' into 126504-connector-type-overrides
ersin-erdal 2970bca
Fix overlooked renaming in docker
ersin-erdal 005fe0a
Fix failing test
ersin-erdal 1fb7e2c
Add objectRemover after rule creation
ersin-erdal 0c2aa07
Merge Conflicts
ersin-erdal f0433b7
Merge branch 'main' into 126504-connector-type-overrides
ersin-erdal 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
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
115 changes: 115 additions & 0 deletions
115
x-pack/plugins/alerting/server/lib/alert_execution_store.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,115 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
import { AlertExecutionStore } from './alert_execution_store'; | ||
import { ActionsCompletion } from '../task_runner/types'; | ||
|
||
describe('AlertExecutionStore', () => { | ||
const alertExecutionStore = new AlertExecutionStore(); | ||
const testConnectorId = 'test-connector-id'; | ||
|
||
// Getter Setter | ||
test('returns the default values if there is no change', () => { | ||
expect(alertExecutionStore.getTriggeredActionsStatus()).toBe(ActionsCompletion.COMPLETE); | ||
expect(alertExecutionStore.getNumberOfTriggeredActions()).toBe(0); | ||
expect(alertExecutionStore.getNumberOfGeneratedActions()).toBe(0); | ||
expect(alertExecutionStore.getStatusByConnectorType('any')).toBe(undefined); | ||
}); | ||
|
||
test('sets and returns numberOfTriggeredActions', () => { | ||
alertExecutionStore.setNumberOfTriggeredActions(5); | ||
expect(alertExecutionStore.getNumberOfTriggeredActions()).toBe(5); | ||
}); | ||
|
||
test('sets and returns numberOfGeneratedActions', () => { | ||
alertExecutionStore.setNumberOfGeneratedActions(15); | ||
expect(alertExecutionStore.getNumberOfGeneratedActions()).toBe(15); | ||
}); | ||
|
||
test('sets and returns triggeredActionsStatusByConnectorType', () => { | ||
alertExecutionStore.setTriggeredActionsStatusByConnectorType({ | ||
actionTypeId: testConnectorId, | ||
status: ActionsCompletion.PARTIAL, | ||
}); | ||
expect( | ||
alertExecutionStore.getStatusByConnectorType(testConnectorId).triggeredActionsStatus | ||
).toBe(ActionsCompletion.PARTIAL); | ||
expect(alertExecutionStore.getTriggeredActionsStatus()).toBe(ActionsCompletion.PARTIAL); | ||
}); | ||
|
||
// increment | ||
test('increments numberOfTriggeredActions by 1', () => { | ||
alertExecutionStore.incrementNumberOfTriggeredActions(); | ||
expect(alertExecutionStore.getNumberOfTriggeredActions()).toBe(6); | ||
}); | ||
|
||
test('increments incrementNumberOfGeneratedActions by x', () => { | ||
alertExecutionStore.incrementNumberOfGeneratedActions(2); | ||
expect(alertExecutionStore.getNumberOfGeneratedActions()).toBe(17); | ||
}); | ||
|
||
test('increments numberOfTriggeredActionsByConnectorType by 1', () => { | ||
alertExecutionStore.incrementNumberOfTriggeredActionsByConnectorType(testConnectorId); | ||
expect( | ||
alertExecutionStore.getStatusByConnectorType(testConnectorId).numberOfTriggeredActions | ||
).toBe(1); | ||
}); | ||
|
||
test('increments NumberOfGeneratedActionsByConnectorType by 1', () => { | ||
alertExecutionStore.incrementNumberOfGeneratedActionsByConnectorType(testConnectorId); | ||
expect( | ||
alertExecutionStore.getStatusByConnectorType(testConnectorId).numberOfGeneratedActions | ||
).toBe(1); | ||
}); | ||
|
||
// Checker | ||
test('checks if it has reached the executable actions limit', () => { | ||
expect(alertExecutionStore.hasReachedTheExecutableActionsLimit({ default: { max: 10 } })).toBe( | ||
false | ||
); | ||
|
||
expect(alertExecutionStore.hasReachedTheExecutableActionsLimit({ default: { max: 5 } })).toBe( | ||
true | ||
); | ||
}); | ||
|
||
test('checks if it has reached the executable actions limit by connector type', () => { | ||
alertExecutionStore.incrementNumberOfTriggeredActionsByConnectorType(testConnectorId); | ||
alertExecutionStore.incrementNumberOfTriggeredActionsByConnectorType(testConnectorId); | ||
alertExecutionStore.incrementNumberOfTriggeredActionsByConnectorType(testConnectorId); | ||
alertExecutionStore.incrementNumberOfTriggeredActionsByConnectorType(testConnectorId); | ||
alertExecutionStore.incrementNumberOfTriggeredActionsByConnectorType(testConnectorId); | ||
|
||
expect( | ||
alertExecutionStore.hasReachedTheExecutableActionsLimitByConnectorType({ | ||
actionsConfigMap: { | ||
default: { max: 20 }, | ||
[testConnectorId]: { | ||
max: 5, | ||
}, | ||
}, | ||
actionTypeId: testConnectorId, | ||
}) | ||
).toBe(true); | ||
|
||
expect( | ||
alertExecutionStore.hasReachedTheExecutableActionsLimitByConnectorType({ | ||
actionsConfigMap: { | ||
default: { max: 20 }, | ||
[testConnectorId]: { | ||
max: 8, | ||
}, | ||
}, | ||
actionTypeId: testConnectorId, | ||
}) | ||
).toBe(false); | ||
}); | ||
|
||
test('checks if a connector type it has already reached the executable actions limit', () => { | ||
expect(alertExecutionStore.hasConnectorTypeReachedTheLimit(testConnectorId)).toBe(true); | ||
}); | ||
}); |
106 changes: 106 additions & 0 deletions
106
x-pack/plugins/alerting/server/lib/alert_execution_store.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,106 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
import { set } from 'lodash'; | ||
import { ActionsConfigMap } from './get_actions_config_map'; | ||
import { ActionsCompletion } from '../task_runner/types'; | ||
|
||
interface State { | ||
numberOfTriggeredActions: number; | ||
numberOfGeneratedActions: number; | ||
connectorTypes: { | ||
[key: string]: { | ||
triggeredActionsStatus: ActionsCompletion; | ||
numberOfTriggeredActions: number; | ||
numberOfGeneratedActions: number; | ||
}; | ||
}; | ||
} | ||
|
||
export class AlertExecutionStore { | ||
private state: State = { | ||
numberOfTriggeredActions: 0, | ||
numberOfGeneratedActions: 0, | ||
connectorTypes: {}, | ||
}; | ||
|
||
// Getters | ||
public getTriggeredActionsStatus = () => { | ||
const hasPartial = Object.values(this.state.connectorTypes).some( | ||
(connectorType) => connectorType?.triggeredActionsStatus === ActionsCompletion.PARTIAL | ||
); | ||
return hasPartial ? ActionsCompletion.PARTIAL : ActionsCompletion.COMPLETE; | ||
}; | ||
public getNumberOfTriggeredActions = () => { | ||
return this.state.numberOfTriggeredActions; | ||
}; | ||
public getNumberOfGeneratedActions = () => { | ||
return this.state.numberOfGeneratedActions; | ||
}; | ||
public getStatusByConnectorType = (actionTypeId: string) => { | ||
return this.state.connectorTypes[actionTypeId]; | ||
}; | ||
|
||
// Setters | ||
public setNumberOfTriggeredActions = (numberOfTriggeredActions: number) => { | ||
this.state.numberOfTriggeredActions = numberOfTriggeredActions; | ||
}; | ||
|
||
public setNumberOfGeneratedActions = (numberOfGeneratedActions: number) => { | ||
this.state.numberOfGeneratedActions = numberOfGeneratedActions; | ||
}; | ||
|
||
public setTriggeredActionsStatusByConnectorType = ({ | ||
actionTypeId, | ||
status, | ||
}: { | ||
actionTypeId: string; | ||
status: ActionsCompletion; | ||
}) => { | ||
set(this.state, `connectorTypes["${actionTypeId}"].triggeredActionsStatus`, status); | ||
}; | ||
|
||
// Checkers | ||
public hasReachedTheExecutableActionsLimit = (actionsConfigMap: ActionsConfigMap): boolean => | ||
this.state.numberOfTriggeredActions >= actionsConfigMap.default.max; | ||
|
||
public hasReachedTheExecutableActionsLimitByConnectorType = ({ | ||
actionsConfigMap, | ||
actionTypeId, | ||
}: { | ||
actionsConfigMap: ActionsConfigMap; | ||
actionTypeId: string; | ||
}): boolean => { | ||
const numberOfTriggeredActionsByConnectorType = | ||
this.state.connectorTypes[actionTypeId]?.numberOfTriggeredActions || 0; | ||
const executableActionsLimitByConnectorType = | ||
actionsConfigMap[actionTypeId]?.max || actionsConfigMap.default.max; | ||
|
||
return numberOfTriggeredActionsByConnectorType >= executableActionsLimitByConnectorType; | ||
}; | ||
|
||
public hasConnectorTypeReachedTheLimit = (actionTypeId: string) => | ||
this.state.connectorTypes[actionTypeId]?.triggeredActionsStatus === ActionsCompletion.PARTIAL; | ||
|
||
// Incrementer | ||
public incrementNumberOfTriggeredActions = () => { | ||
this.state.numberOfTriggeredActions++; | ||
}; | ||
|
||
public incrementNumberOfGeneratedActions = (incrementBy: number) => { | ||
this.state.numberOfGeneratedActions += incrementBy; | ||
}; | ||
|
||
public incrementNumberOfTriggeredActionsByConnectorType = (actionTypeId: string) => { | ||
const currentVal = this.state.connectorTypes[actionTypeId]?.numberOfTriggeredActions || 0; | ||
set(this.state, `connectorTypes["${actionTypeId}"].numberOfTriggeredActions`, currentVal + 1); | ||
}; | ||
public incrementNumberOfGeneratedActionsByConnectorType = (actionTypeId: string) => { | ||
const currentVal = this.state.connectorTypes[actionTypeId]?.numberOfGeneratedActions || 0; | ||
set(this.state, `connectorTypes["${actionTypeId}"].numberOfGeneratedActions`, currentVal + 1); | ||
}; | ||
} |
44 changes: 44 additions & 0 deletions
44
x-pack/plugins/alerting/server/lib/get_actions_config_map.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,44 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
import { getActionsConfigMap } from './get_actions_config_map'; | ||
|
||
const connectorTypeId = 'test-connector-type-id'; | ||
const actionsConfig = { | ||
max: 1000, | ||
}; | ||
|
||
const actionsConfigWithConnectorType = { | ||
...actionsConfig, | ||
connectorTypeOverrides: [ | ||
{ | ||
id: connectorTypeId, | ||
max: 20, | ||
}, | ||
], | ||
}; | ||
|
||
describe('get actions config map', () => { | ||
test('returns the default actions config', () => { | ||
expect(getActionsConfigMap(actionsConfig)).toEqual({ | ||
default: { | ||
max: 1000, | ||
}, | ||
}); | ||
}); | ||
|
||
test('applies the connector type specific config', () => { | ||
expect(getActionsConfigMap(actionsConfigWithConnectorType)).toEqual({ | ||
default: { | ||
max: 1000, | ||
}, | ||
[connectorTypeId]: { | ||
max: 20, | ||
}, | ||
}); | ||
}); | ||
}); |
27 changes: 27 additions & 0 deletions
27
x-pack/plugins/alerting/server/lib/get_actions_config_map.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,27 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
import { omit } from 'lodash'; | ||
import { ActionsConfig, ActionTypeConfig } from '../config'; | ||
|
||
export interface ActionsConfigMap { | ||
default: ActionTypeConfig; | ||
[key: string]: ActionTypeConfig; | ||
} | ||
|
||
export const getActionsConfigMap = (actionsConfig: ActionsConfig): ActionsConfigMap => { | ||
const configsByConnectorType = actionsConfig.connectorTypeOverrides?.reduce( | ||
(config, configByConnectorType) => { | ||
return { ...config, [configByConnectorType.id]: omit(configByConnectorType, 'id') }; | ||
}, | ||
{} | ||
); | ||
return { | ||
default: omit(actionsConfig, 'connectorTypeOverrides'), | ||
...configsByConnectorType, | ||
}; | ||
}; |
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.
nit: seems like for this and the next
hasReached...()
method, we could pass theactionsConfigMap()
into the constructor, so we wouldn't have to pass it into these methods directly. Haven't seen where it's used, but it could save having to pass the configMap object around with this object ... OTOH, perhaps there is a reason we do this, or we already have it available and the current structure makes it easier to test ...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.
it could be moved in to this class but they are completely different objects.
This class is to hold ruleRunMetrics while actionsConfigMap is the config object from kibana.yml...
Maybe later we can do this refactoring, lets discuss :)