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

Provide indication to rule executor when rule is disabled/re-enabled #163126

Open
nreese opened this issue Aug 3, 2023 · 5 comments
Open

Provide indication to rule executor when rule is disabled/re-enabled #163126

nreese opened this issue Aug 3, 2023 · 5 comments
Labels
Feature:Alerting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)

Comments

@nreese
Copy link
Contributor

nreese commented Aug 3, 2023

Geo containment rule caches containment boundaries between executions. #157934 requests that the boundaries cache be cleared when a rule is disabled/re-enabled. The problem is that the executor is not provided with any state to know when a rule is disabled or re-enabled.

The feature request is to provide the rule executor with state to indicate when a rule has been disabled or re-enabled. One suggested solution is to include enabledAt timestamp in rule state. Another suggestion would be a callback on rule disable. Personally, I like the callback on rule disablement.

@nreese nreese added the Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) label Aug 3, 2023
@elasticmachine
Copy link
Contributor

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

@nreese nreese changed the title Provide indication to rule executor when rule is enabled Provide indication to rule executor when rule is disabled/re-enabled Aug 3, 2023
@pmuellr
Copy link
Member

pmuellr commented Aug 4, 2023

This would have been a behavior change then, when we changed enable/disable to NOT delete it's task document, but mark it as "disabled" instead. The missing behavior is the clearing of rule and instance state. I think we clear instance state, actually (and recover alerts, etc).

I thought we had an issue open to add an explicit "clear rule state" action, to account for this. But not seeing one. Anyone else know of one?

I think that would be good idea as a general "fix it" tool, but would not be great as a UX, forcing the user to click some button "reset rule state" at times.

We have some issues open to coordinate calls to the rule type when lifecycle APIs are called, that seem more relevant:

These all seem so related, we should look at them together when extending to this kind of functionality.

@ymao1
Copy link
Contributor

ymao1 commented Aug 4, 2023

Here's the issue for clearing task state: #140402

@ymao1
Copy link
Contributor

ymao1 commented Aug 4, 2023

Maybe we can set a flag on the rule type to clearStateOnDisable so a rule can choose to clear the state when the rule is disabled?

@pmuellr
Copy link
Member

pmuellr commented Aug 4, 2023

Or perhaps clearStateOnEnable - would someone be interested in the state, while it was disabled, so we'd just clear it when it's re-enabled. Would be good, diagnostic-wise anyway - perhaps confusing though.

The options I mentioned above for "lifecycle hooks for rule types" kinda thing is kinda over-bearing to solve this problem - expect everyone to implement a hook on disable (or enable) to clear task state. And it's not even clear what these "hooks" might allow - I think one issue pointed out a hook should be able to "veto" a creation / update; to "clear state", we'd probably need to provide some special function just for that (like replaceState() in the alert client). So ... having a rule-type option for "task state clearing on disable" seems like the easiest way to do this for rule implementors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Alerting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)
Projects
No open projects
Development

No branches or pull requests

5 participants