-
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
[RAC] turn off observability alerts as data writing in a more granular way #119602
Conversation
16fe39b
to
8df538d
Compare
8df538d
to
e0f87f8
Compare
5a4db77
to
9c3344e
Compare
…a plugin service and not in the resourceInstaller
f04b4b9
to
5aa1d17
Compare
@elasticmachine merge upstream |
@@ -195,7 +200,8 @@ export class RuleDataService implements IRuleDataService { | |||
return new RuleDataClient({ | |||
indexInfo, | |||
resourceInstaller: this.resourceInstaller, | |||
isWriteEnabled: this.isWriteEnabled(), | |||
isWriteEnabled: | |||
this.isWriteEnabled() && !this.isRegistrationContextDisabled(registrationContext), |
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.
Isn't confusing to combine isWriteEnabled
and disabledRegistrationContexts
with the same key in RuleDataClient
? Because it would be hard to determine why the value is true/false.
Besides, from a convention perspective, it seems that the relation here is 1:1 in the RuleDataClient
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.
@fkanout Great question, yep you are right. In the beginning I thought to add a separate key, but that means I would need to change more places in the code (places where isWriteEnabled is called, I should check for disabledEegistrationContexts as well). The way I have it now is more centralized. I do the checks in one place.
@weltenwort is there a benefit of combining isWriteEnabled and disabledRegistrationContexts into one key vs splitting it into separate keys?
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.
Maybe we can try to answer the opposite question: Why would the RuleDataClient
need to know about the reason for being disabled? In the spirit of keeping the coupling as loose as possible it would make sense not to introduce the knowledge about a per-registration-context disablement feature out of the RuleDataClient
if not necessary.
So what would be the benefit of passing them separately?
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.
@weltenwort Ok if we want to keep the coupling as loose as possible, it makes sense to keep the knowledge about per-registration-context disabling out of the RuleDataClient. @fkanout do you have any objections?
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 think the knowledge that RuleDataClient
already has via isWriteEnabled
is similar to isRegistrationContextDisabled
, the difference is only the granularity of that info. (everything/selected things). Unless if RuleDataClient
shouldn't know about isWriteEnabled
in the first place - That, I don't know.
My understanding is once Alert-as-Data is adopted across our products, the general xpack.rule_registry.write.enabled
will be deprecated. While the xpack.ruleRegistry.write.disabledRegistrationContexts
could stay for long. From that point, I would say they should be separated.
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.
@fkanout What if we add even more granularity and disable writing per rule type id? Would we want to pass one more key for example disabledRuleTypeIds? Does the RuleDataClient need to know this level of detail? I don't know what is the correct answer.
For now I would keep it as it is. We could discuss it further if you want and come up with the most appropriate solution.
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.
IMHO, the only thing that RuleDataClient should know is if isWriteEnabled true or false. All the complex logics in it, makes it hard to read/understand.
I would implement all the logic in this.isWriteEnabled()
method at line 118.
Otherwise, what is the reason to create such a method? this.options.isWriteEnabled
could be used in everywhere.
So, it would be:
public isWriteEnabled(): boolean {
return this.options.isWriteEnabled &&
!this.options.disabledRegistrationContexts.includes(registrationContext);
}
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.
@ersin-erdal Sounds good to me. Let me refactor and push the changes.
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 would implement all the logic in this.isWriteEnabled() method at line 118.
It could be more readable, but it is still the same solution. We have a consensus about it, so let's merge it like that 👍🏻
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.
LGTM
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
History
To update your PR or re-run it, just comment with: cc @mgiota |
…r way (elastic#119602) * [RAC] turn off writing to disabled alerts indices * fix error * fix errors * do not install component templates for disabled registration contexts * add resource installer unit tests * refactoring: disable installing index level resources in the rule data plugin service and not in the resourceInstaller * refactor based on review comments * update comment for isWriteEnabled method Co-authored-by: Kibana Machine <[email protected]>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…r way (#119602) (#120126) * [RAC] turn off writing to disabled alerts indices * fix error * fix errors * do not install component templates for disabled registration contexts * add resource installer unit tests * refactoring: disable installing index level resources in the rule data plugin service and not in the resourceInstaller * refactor based on review comments * update comment for isWriteEnabled method Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: mgiota <[email protected]>
I approved the PR, as it fulfills the ACs. However, I shared a couple of questions/scenarios with @mgiota. These could be edge cases or false positives. However, I will follow up here to share them and to get everyone's feedback/ thoughts. Scenario:
Questions I tried to make it as plain as I could and be careful with the terminologies. However, please feel free to ask if something is not clear. |
Thanks @fkanout -- from my perspective, in your scenario, I would expect no writes at all to happen for any rule with "observability.logs" as its registration context after step 7. Note: In all cases with this new flag it should work exactly the same as For your questions:
We shouldn't remove existing indices, so that makes sense. And rule creation should absolutely continue -- this scenario is probably most likely to occur for someone who is using the alerting framework to run rules against their data but doesn't want the alert documents to be written/updated for some reason.
The rule should execute as normal, and schedule actions as normal. It should not create any new alert documents or update any previously created alert documents.
Good question. My understanding is that this flag is checked on every write, but I may be mistaken. We should confirm, because I don't think we should continue to update alerts if this flag is off. However, as I mentioned above, if this is already not the case for the
Not sure I follow this one. The task manager will start up again, find all rules, and start running them again. It will handle passing in the "previous run" state just as it usually does on subsequent executions, as if the restart didn't happen. We do a lookup of the existing alerts to update during the executor phase, so I think we would find any previous alerts and continue to update them, but I'm not sure if we treat the restart as a "resolved" state for any alerts that were active when the system was restarted. I imagine we don't, I'm not sure what we expect here. |
Thank you, @jasonrhodes! |
…r way (elastic#119602) * [RAC] turn off writing to disabled alerts indices * fix error * fix errors * do not install component templates for disabled registration contexts * add resource installer unit tests * refactoring: disable installing index level resources in the rule data plugin service and not in the resourceInstaller * refactor based on review comments * update comment for isWriteEnabled method Co-authored-by: Kibana Machine <[email protected]>
Fixes #119217
xpack.ruleRegistry.write.disabledRegistrationContexts
flag was introduced to disable writing to observability alerts-as-data indices in a more granular way.The registration contexts we use are:
How to test
xpack.ruleRegistry.write.disabledRegistrationContexts : ['observability.logs']
(you could try with other values from the above list)Indices
tab make sure to enable theInclude hidden indices
toggle, search for.internal.alerts
and verify that nothing appears on the list.Index templates
tab, enableView System templates
, search foralerts
and verify that nothing appears on the list.Component templates