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.

Sign up for GitHub

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

Jump to bottom

[Alerting] Remove defaultRuleTaskTimeout and set ruleType specific timeout from kibana.yml #128294

Merged

ersin-erdal merged 8 commits into elastic:main from ersin-erdal:124807-rule-task-timeout

Mar 28, 2022

Contributor

ersin-erdal commented Mar 22, 2022 •

edited by pmuellr

Loading

fixes: #124807

doc preview: https://kibana_128294.docs-preview.app.elstc.co/guide/en/kibana/master/alert-action-settings-kb.html#alert-settings

Summary

This PR intends to remove defaultRuleTaskTimeout config and use config.execution.timeout key instead of it.
The new key config.execution.timeout can be modified by the users globally or per rule type.

To Verify

Add a sleep function into the task runner (just before this.ruleType.executor) code such as: await new Promise(r => setTimeout(r, 360000));
That would delay your rule execution for 6 minutes.
Add the below config to your kibana.yml and run kibana
Create two rules (e.g. Index Threshold and a Security Rule) and wait your rules to be executed.

Index Threshold should timeout in 10s and the Security Rule in 5m.
You can see the error message in the rule detail page with the applied timeout config.

xpack.alerting.rules.execution:
    timeout: 30s
    ruleTypeOverrides:
      - id: '.index-threshold'
         timeout: 10s

To try global timeout config remove ruleTypeOverrides from your kibana.yml and run the kibana again...
You should see 30s in the error message on the rule details page.

xpack.alerting.rules.execution:
    timeout: 30s

Checklist

Delete any items that are not applicable to this PR.

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list


          ruleTaskTimeout consumes the timeout value in the following order:

283b3ba

1- Rule Type specific timeout from kibana.yml
xpack.alerting.rules.execution.ruleTypeOverrides

2- Timeout that applies to all rule types, from kibana.yml
xpack.alerting.rules.execution.timeout

3- ruleTaskTimeout from the source plugin
4- default timeout '5m'

ersin-erdal added backport:skip Team:ResponseOps release_note:feature Feature:Alerting/RulesFramework v8.2.0 labels

ersin-erdal commented

View reviewed changes

x-pack/plugins/alerting/server/plugin.ts

@@ @@ -286,18 +285,21 @@ export class AlertingPlugin { @@
                         ActionGroupIds,
                         RecoveryActionGroupId
                       >
-                    ) {
+                    ) => {

Contributor Author

ersin-erdal Mar 22, 2022

Converted to an arrow function, so we can use this in the function.

ersin-erdal marked this pull request as ready for review

March 22, 2022 19:30

ersin-erdal requested review from a team as code owners

March 22, 2022 19:30

Contributor

elasticmachine commented Mar 22, 2022

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

jbudz approved these changes

View reviewed changes

Member

jbudz left a comment

kibana-docker LGTM

ymao1 requested a review from lcawl

March 23, 2022 12:27

ymao1 approved these changes

View reviewed changes

Contributor

ymao1 left a comment

LGTM! Verified the following:

When no value is set for xpack.alerting.rules.execution.timeout, rule type timeouts are 5m for all rules except security rules.
When value is set for xpack.alerting.rules.execution.timeout, value is applied to all rules
When value is set for specific rule type using xpack.alerting.rules.execution.ruleTypeOverrides, value is applied to that specific rule type.

Nice job! Tagging @lcawl to review copy and @jbudz for a question about the docker allowlist

x-pack/plugins/alerting/server/lib/get_rule_task_timeout.ts Show resolved Hide resolved

src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker Show resolved Hide resolved

src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker Show resolved Hide resolved

x-pack/plugins/alerting/server/plugin.ts Show resolved Hide resolved

ersin-erdal and others added 3 commits

March 23, 2022 20:30


          Merge branch 'main' into 124807-rule-task-timeout

04eb88d


          Revert removed defaultRuleTaskTimeout from docker allowed list,

9af17f5

left a comment about timeout config priority


          Merge remote-tracking branch 'origin/124807-rule-task-timeout' into 1…

17bbb99

…24807-rule-task-timeout

lcawl reviewed

View reviewed changes

docs/settings/alert-action-settings.asciidoc

               `xpack.alerting.rules.execution.actions.max`::
               Specifies the maximum number of actions that a rule can trigger each time detection checks run.
+              `xpack.alerting.rules.execution.timeout`::

Contributor

lcawl Mar 23, 2022

xpack.alerting.rules.execution.timeout

I know that it currently appears in our Rules UI, but "execution" is one of those words with violent connotations that we generally try to avoid. Is there some other term we can use that's less problematic? For example, "xpack.alerting.rules.tasks.timeout" or "xpack.alerting.rules.invocation.timeout", or just "xpack.alerting.rules.timeout"?

docs/settings/alert-action-settings.asciidoc Outdated Show resolved Hide resolved

docs/settings/alert-action-settings.asciidoc

+              +
+              For example, `20m`, `24h`, `7d`, `1w`. Default: `5m`.
+              `xpack.alerting.rules.execution.ruleTypeOverrides`::

Contributor

lcawl Mar 23, 2022

xpack.alerting.rules.execution.ruleTypeOverrides::

Likewise, is it possible to omit "execution" from this setting name too? For example, just use "xpack.alerting.rules.typeOverrides" instead?

docs/settings/alert-action-settings.asciidoc Outdated

+              For example, `20m`, `24h`, `7d`, `1w`. Default: `5m`.
+              `xpack.alerting.rules.execution.ruleTypeOverrides`::
+              Overrides the configs under `xpack.alerting.rules.execution` for the rule type with the given ID. The value is array of objects consists of the key(s) that are wanted to be overridden under execution object and the id of the specific rule type that is wanted to be modified.

Contributor

lcawl Mar 23, 2022

Overrides the configs under xpack.alerting.rules.execution for the rule type with the given ID.

This makes me wonder if the configs referred to here are in some other file or UI. If not, this might be clearer:

"Overrides xpack.alerting.rules.* settings for a specific rule identifier.

docs/settings/alert-action-settings.asciidoc Outdated Show resolved Hide resolved


          Correct wording

3064b3c

mikecote approved these changes

View reviewed changes

Contributor

mikecote left a comment

Changes LGTM! I tested the different mechanisms locally and saw it work! Great work

x-pack/plugins/alerting/server/lib/get_rule_task_timeout.ts

+              export const getRuleTaskTimeout = ({
+                config,
+                ruleTaskTimeout,

Contributor

mikecote Mar 24, 2022

optional nit: to make the naming of this variable clearer, what if it was called something like ruleTypeProposedTimeout?

ersin-erdal and others added 3 commits

March 27, 2022 20:48


          Merge branch 'main' into 124807-rule-task-timeout

7ae3a6b


          fix numberOfTriggeredActions calculation bug

0942b3e


          Merge remote-tracking branch 'origin/124807-rule-task-timeout' into 1…

2cfd02a

…24807-rule-task-timeout

Collaborator

kibana-ci commented Mar 27, 2022

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #33467 succeeded 3064b3c
💚 Build #33220 succeeded 17bbb99
💚 Build #32726 succeeded 283b3ba

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

ersin-erdal merged commit c9aad65 into elastic:main

ersin-erdal deleted the 124807-rule-task-timeout branch

March 28, 2022 06:36

ymao1 mentioned this pull request

[Discover] Cancel long running requests in Discover alert #130077

Merged

1 task

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip Feature:Alerting/RulesFramework release_note:feature Team:ResponseOps v8.2.0