Add muting support for alerts #43712
Conversation
mikecote
added
Feature:Alerting
v8.0.0
release_note:skip
|
Pinging @elastic/kibana-stack-services |
This comment has been minimized.
This comment has been minimized.
💚 Build Succeeded |
💚 Build Succeeded |
💚 Build Succeeded |
| ); | ||
| }); | ||
|
|
||
| test('skips muting when alert already muted', async () => { |
There was a problem hiding this comment.
Is there room to consider not skipping but rather making it explicit that the action failed and why?
Obviously if in other places we skip then we should keep the behaviour uniform, but I'm not a fan of No-Ops tha appear to be successful as they can often obscure an underlying problem (such as UI not updating relative to retrieved data etc.)
There was a problem hiding this comment.
Yeah this is to keep it uniform with the design of enable / disable which also skips if ever the alert is already in that state. Returning error isn't a bad idea but I guess it comes down to what behaviour we prefer our APIs to have.
There was a problem hiding this comment.
Fair.
I guess, broadly, I tend to bias for explicitness and would prefer that we did return an error, but definitely only if it was done that way in all cases, and definitely isn't a blocker.
There was a problem hiding this comment.
Just moving @bmcconaghy's comment (#43712 (comment)) here:
To me if I call an API to put something a state, and it's already in that state, that's not an error, so it makes sense to just say "OK" in that case. What value would returning an error response give? The client would then have to figure out what state things are in, and oh yeah, they're in the state the client wanted anyway. Seems like a distinction without a difference to me.
| groupsToScheduleActionsInSeries: ['default', null, 'default'], | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| switch (scenario.id) { |
There was a problem hiding this comment.
I realise this isn't a new pattern, as there was already a preexisting switch like this above, but this pattern feels a little dangerous to me.
It feels like we're packing multiple tests into one and I can't help but wonder if we're stretching the scenario pattern here too much.
As a person new to this test suite, following this is hard and confusing and defeats one of the purposes of testing which is providing a clear documentation of how an API is meant to be used and what the expected behaviour is.
I wonder if perhaps we can have the tests with the Switch case explicit for each scenario, and only use the for loop for tests that are the same for each scenario and don't require a switch?
This isn't a blocker for me, but I can't shale the feeling this should be rethought.
There was a problem hiding this comment.
To me if I call an API to put something a state, and it's already in that state, that's not an error, so it makes sense to just say "OK" in that case. What value would returning an error response give? The client would then have to figure out what state things are in, and oh yeah, they're in the state the client wanted anyway. Seems like a distinction without a difference to me.
There was a problem hiding this comment.
To provide a bit of background on the tests. These were inspired from https://github.com/elastic/kibana/tree/master/x-pack/test/ui_capabilities as suggested by the security team. We need some coverage when security is enabled / disabled as well as spaces enabled / disabled. In order to just have coverage we opted to follow the approach they did and defer until later a look at how we want our test suite to work (https://github.com/elastic/kibana/projects/26#card-26655072).
There will be room to propose different approaches but yeah I would say lets talk then.
There was a problem hiding this comment.
@bmcconaghy From my perspective that would be an issue in how the error response is communicated, not with whether an error response is valid. 🤷♂️
I dislike No-ops, it's subjective, but they often cause invalid states to be missed so I try to avoid them when possible.
But like I said, by no means a blocker.
x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/disable.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Made a few comments, but none are blockers, mostly just subjective preferences, so feel free to skip them if you disagree. :)
My one caveat is that I don't know enough about SavedObjects to catch something that's invalid in this PR, so hopefully someone with a bit more context can review too.
💚 Build Succeeded |
💚 Build Succeeded |
|
@gmmorris I'm going to merge this PR and handle the API behaviour as a separate request. If we want to keep the conversation alive, lets create a card within the Make it Action project https://github.com/elastic/kibana/projects/26. We'll be able to discuss when everyone is around. |
* Create supporting API * Rename mute terminology to mute instance to allow alert level muting * Add alert mute and unmute APIs * Add logic to handle alert muting * Add integration tests + fix AAD breaking the object * Fix failing jest tests * Fix test failures * Clear out mutedInstanceIds when muting / unmuting an alert * Skip muting / unmuting instances when alert is muted * Rename interface for alert instance * Rename functional tests to alert instance terminology * Add API integration tests for alert muting / unmuting * Apply PR feedback pt1 * Create single index record action * Function to create always firing alerts and function to generate reference * Make tests use alert utils * Rename mute / unmute alert routes * Make alerts.ts integration test use alertUtils for both spaces_only and security_and_spaces * Re-use alert utils where possible * Change muted in mapping to muteAll * Rename alert client methods to muteAll and unmuteAll * Rename files * Rename alert utils function muteAll and unmuteAll * Rename variable in task runner * Cleanup * Destructure instead of using existingObject variable
💚 Build Succeeded |
* Create supporting API * Rename mute terminology to mute instance to allow alert level muting * Add alert mute and unmute APIs * Add logic to handle alert muting * Add integration tests + fix AAD breaking the object * Fix failing jest tests * Fix test failures * Clear out mutedInstanceIds when muting / unmuting an alert * Skip muting / unmuting instances when alert is muted * Rename interface for alert instance * Rename functional tests to alert instance terminology * Add API integration tests for alert muting / unmuting * Apply PR feedback pt1 * Create single index record action * Function to create always firing alerts and function to generate reference * Make tests use alert utils * Rename mute / unmute alert routes * Make alerts.ts integration test use alertUtils for both spaces_only and security_and_spaces * Re-use alert utils where possible * Change muted in mapping to muteAll * Rename alert client methods to muteAll and unmuteAll * Rename files * Rename alert utils function muteAll and unmuteAll * Rename variable in task runner * Cleanup * Destructure instead of using existingObject variable
Resolves #46034.
In this PR, I'm adding muting support for alerts and alert instances.
Four new API endpoints have been created:
/api/alert/{id}/_mute_all/api/alert/{id}/_unmute_all/api/alert/{alertId}/alert_instance/{alertInstanceId}/_mute/api/alert/{alertId}/alert_instance/{alertInstanceId}/_unmuteThis will stop scheduling actions for a given alert or alert instance when muted.
Other changes include:
savedObjectsClient.update(...)calls when changing the apiKey (to avoid breaking AAD)