-
Notifications
You must be signed in to change notification settings - Fork 8k
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
[Alerting] Change logger level to debug when delete a rule without alerts produce an error or warn when untracking alerts #183207
Conversation
Pinging @elastic/response-ops (Team:ResponseOps) |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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
Hmmm, perhaps my suggestion of changing the error/warn to debug was not the best :-) It looks like this would end up setting the level for any errors when alerts are updated. My main issue was having messages when there aren't any alerts to update. Peeking at the code, I found this: kibana/x-pack/plugins/alerting/server/alerts_service/lib/set_alerts_to_untracked.ts Lines 226 to 228 in 551e123
Can we just not throw an error here? Or maybe that is indicative of some kind of error, in some case through the loop and with that query? But it is definitely the case that when a rule is deleted / disabled and has no alerts, these logs are generated - which is what I want to avoid. |
Good point @pmuellr! I missed that. I reverted the log levels and stopped throwing an error when alerts were not found in kibana/x-pack/plugins/alerting/server/application/rule/methods/bulk_untrack/bulk_untrack_alerts.ts Line 67 in e76c5ed
|
@@ -426,6 +426,8 @@ describe('setAlertsToUntracked()', () => { | |||
}, | |||
}); | |||
|
|||
clusterClient.updateByQuery.mockResponseOnce({ total: 2, updated: 2 }); |
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.
We need to mock the response of updateByQuery
for the test to test the correct behavior of the code.
That seems like the right thing to do. Following that bulk untrack module, it seems like it might actually try to make some kind of bulk call later (to clear out some alerts in task state docs) with an empty array. Starting here: kibana/x-pack/plugins/alerting/server/application/rule/methods/bulk_untrack/bulk_untrack_alerts.ts Lines 67 to 72 in 71f856b
Not sure how ES will handle a bulk update with no actual updates :-), or perhaps it's short-circuited elsewhere. Seems like deleting a rule which never had an active alert will tell us. I have a feeling we may need to short-circuit that ourselves, if the number of alerts is 0 ... |
I was also skeptical about it but wanted your opinion before doing it 🙂. Thanks! if we return early like
should we also audit log when we return without calling |
Great question - should we log that we did nothing? :-) I guess I'm thinking we should. If we don't, then the pattern of audit logs would look no different than if something went wrong and that log doc didn't get written. So, it might look like an error to someone, to not see that. @mikecote thoughts? |
Yes I think so, it matches patterns like enabling an already enabled rule and such (audit log events are still logged). |
@pmuellr @mikecote Done in |
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @cnasikas |
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!
Summary
Fixes: #182754
For maintainers