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

Add circuit breaker for max number of actions by connector type #128319

Merged
merged 44 commits into from
Apr 26, 2022

Conversation

ersin-erdal
Copy link
Contributor

fixes: #126504

Summary

This PR intends to introduce a new kibana.yml config key that can override the actions.max circuit breaker by connector type(s)

To Verify

Create a rule that has more than one actions of the same type (e.g. Server Log).
Add the below config to your kibana.yml and expect only one of your actions to be triggered.
number_of_triggered_actions value in event log should be 1.
And a warning banner on rule details page should be shown.

xpack.alerting.rules.execution:
  actions:
    connectorTypeOverrides:
      - id: '.server-log'
        max: 1

@ersin-erdal ersin-erdal added backport:skip This commit does not require backporting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) release_note:feature Makes this part of the condensed release notes Feature:Alerting/RuleActions Issues related to the Actions attached to Rules on the Alerting Framework v8.2.0 labels Mar 22, 2022
@ersin-erdal ersin-erdal marked this pull request as ready for review March 23, 2022 11:58
@ersin-erdal ersin-erdal requested review from a team as code owners March 23, 2022 11:58
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

kibana-docker LGTM

@ymao1 ymao1 requested a review from lcawl March 23, 2022 13:14
@ersin-erdal
Copy link
Contributor Author

@elasticmachine merge upstream

docs/settings/alert-action-settings.asciidoc Outdated Show resolved Hide resolved
docs/settings/alert-action-settings.asciidoc Outdated Show resolved Hide resolved
docs/settings/alert-action-settings.asciidoc Outdated Show resolved Hide resolved
docs/settings/alert-action-settings.asciidoc Outdated Show resolved Hide resolved
@ymao1
Copy link
Contributor

ymao1 commented Mar 24, 2022

@ersin-erdal Thank you for making the changes to the config! Those look great. I left a question about whether we're actually capping by connector type in the createExecutionHandler logic

…connector-type-overrides

� Conflicts:
�	x-pack/plugins/alerting/server/plugin.ts
�	x-pack/plugins/alerting/server/task_runner/task_runner.test.ts
�	x-pack/plugins/alerting/server/task_runner/task_runner_factory.ts
@ymao1
Copy link
Contributor

ymao1 commented Mar 24, 2022

I created an Always Firing rule with 20 alerts that has a server log action for active alerts and an action for recovered alerts.

When I set my config to:

xpack.alerting.rules.execution:
  actions:
    max: 28

I see 20 active server log entries and 8 recovered server log entries. This works as expected.

When I set my config to:

xpack.alerting.rules.execution:
  actions:
    max: 28
    connectorTypeOverrides:
      - id: '.server-log'
        max: 5

I would expect to see 5 active server log entries. Instead I see 20 active server log entries and no recovered server log entries.

@ersin-erdal
Copy link
Contributor Author

I would expect to see 5 active server log entries. Instead I see 20 active server log entries and no recovered server log entries.

Fixed :)

…connector-type-overrides

� Conflicts:
�	x-pack/plugins/alerting/server/task_runner/create_execution_handler.test.ts
�	x-pack/plugins/alerting/server/types.ts
@ersin-erdal
Copy link
Contributor Author

@elasticmachine merge upstream

kibanamachine and others added 4 commits April 19, 2022 07:18
…connector-type-overrides

� Conflicts:
�	x-pack/plugins/alerting/server/config.ts
�	x-pack/plugins/alerting/server/lib/rule_execution_status.ts
�	x-pack/plugins/alerting/server/task_runner/create_execution_handler.test.ts
�	x-pack/plugins/alerting/server/task_runner/create_execution_handler.ts
�	x-pack/plugins/alerting/server/task_runner/task_runner.test.ts
�	x-pack/plugins/alerting/server/task_runner/task_runner.ts
�	x-pack/plugins/alerting/server/task_runner/types.ts
ersin-erdal and others added 3 commits April 19, 2022 17:08
Co-authored-by: Mike Côté <mikecote@users.noreply.github.com>
@ersin-erdal ersin-erdal added v8.3.0 and removed v8.2.0 labels Apr 19, 2022
Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM, left a nit comment

};

// Checkers
public hasReachedTheExecutableActionsLimit = (actionsConfigMap: ActionsConfigMap): boolean =>
Copy link
Member

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 the actionsConfigMap() 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 ...

Copy link
Contributor Author

@ersin-erdal ersin-erdal Apr 25, 2022

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 :)

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
alerting 325 324 -1

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
alerting 20 19 -1
Unknown metric groups

API count

id before after diff
alerting 334 333 -1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@ersin-erdal ersin-erdal merged commit f44e198 into elastic:main Apr 26, 2022
@ersin-erdal ersin-erdal deleted the 126504-connector-type-overrides branch April 26, 2022 01:35
kertal pushed a commit to kertal/kibana that referenced this pull request May 24, 2022
…tic#128319)

* connectorTypeOverrides key in kibana.yml can create a connector type specific action config.

* Update docs and docker allowed keys
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Alerting/RuleActions Issues related to the Actions attached to Rules on the Alerting Framework release_note:feature Makes this part of the condensed release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add circuit breaker for max number of actions by connector type a rule can schedule
9 participants