[Alerting] Change logger level to debug when delete a rule without alerts produce an error or warn when untracking alerts

[cnasikas]

Summary

Fixes: #182754

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[elasticmachine]

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

[cnasikas]

@elasticmachine merge upstream

[cnasikas]

@elasticmachine merge upstream

[ymao1]

LGTM

[pmuellr]

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

	if (total === 0 && response.total === 0) {
	throw new Error('No active alerts matched the query');
	}

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.

[cnasikas]

Good point @pmuellr! I missed that. I reverted the log levels and stopped throwing an error when alerts were not found in e0c65fe (#183207). I decided to return an empty array if there are no results. I checked where the function is being called and it seems safe to do so. The only place where the response is used is at

kibana/x-pack/plugins/alerting/server/application/rule/methods/bulk_untrack/bulk_untrack_alerts.ts

Line 67 in e76c5ed

const taskIds = [...new Set(result.map((doc) => doc[ALERT_RULE_UUID]))];

. Let me know if that's ok.

[cnasikas]

We need to mock the response of updateByQuery for the test to test the correct behavior of the code.

[pmuellr]

I reverted the log levels and stopped throwing an error when alerts were not found in e0c65fe (#183207). I decided to return an empty array if there are no results. I checked where the function is being called and it seems safe to do so. The only place where the response is used is at

kibana/x-pack/plugins/alerting/server/application/rule/methods/bulk_untrack/bulk_untrack_alerts.ts

Line 67 in e76c5ed

const taskIds = [...new Set(result.map((doc) => doc[ALERT_RULE_UUID]))];

. Let me know if that's ok.

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

	const taskIds = [...new Set(result.map((doc) => doc[ALERT_RULE_UUID]))];
	await context.taskManager.bulkUpdateState(taskIds, (state, id) => {
	try {
	const uuidsToClear = result
	.filter((doc) => doc[ALERT_RULE_UUID] === id)
	.map((doc) => doc[ALERT_UUID]);

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 ...

[cnasikas]

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

 if (taskIds.length === 0) { <---- should we context.auditLogger?.log here???
      return;
    }
    
    await context.taskManager.bulkUpdateState(taskIds, (state, id) => { //// })
    
    context.auditLogger?.log(
      ruleAuditEvent({
        action: RuleAuditAction.UNTRACK_ALERT,
        outcome: 'success',
      })
    );

should we also audit log when we return without calling context.taskManager.bulkUpdateState?

[pmuellr]

should we also audit log when we return without calling context.taskManager.bulkUpdateState?

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?

[mikecote]

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

[cnasikas]

@pmuellr @mikecote Done in 45bef8e (#183207)

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: c94b453

Metrics [docs]

✅ unchanged

History

• 💚 Build #209858 succeeded da2e343
• 💚 Build #209423 succeeded 45adbe3
• 💔 Build #209394 failed 38d3c87
• 💔 Build #209360 failed 9674a0f
• 💔 Build #209345 failed 3d8c53b

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

cc @cnasikas

[pmuellr]

LGTM!