Throw error when alert instance already fired

[mikecote]

In this PR, we're throwing an error when an alert is trying to fire twice the same instance.

[elasticmachine]

Pinging @elastic/kibana-stack-services

[bmcconaghy]

Won't this wind up triggering retries in task manager?

[mikecote]

@bmcconaghy it will, similar to how an invalid configuration or parameters would. But this is mostly to tell developers firing twice isn't a feature and to be caught at development time.

[mikecote]

retest

[pmuellr]

Won't this wind up triggering retries in task manager?

I suspect non-stop retries, as the retries will fail :-)

Not sure if we can provide some kind of guard for this - eg, we check the thrown error and if it says "don't retry", then ... don't retry, and we would add that in this particular case.

@bmcconaghy has brought up a few times having some way of NOT triggering retries, if the alertType / actionType author knows an error shouldn't be retried. This smells like a case of that, but for our own purposes.

[pmuellr]

Perhaps this should just log a message, instead of throwing an error, to prevent infinite error retries. For now.

[mikecote]

Yeah, I think that's where the error handling phase will come in. The same problem will occur if there's a bug in the alert type executor and it throws an error (cannot call x of undefined). But all these errors should be caught at development / QA time.

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[bmcconaghy]

I was seeing a similar infinite immediate retry thing with a validation error in the action config:

server log [13:11:39.971] [error][task_manager] Task actions:smtp "nBi0cGsBPLu1qZWiYs1Q" failed: Error: params invalid: child "to" fails because ["to" must be an array]
server log [13:11:40.010] [error][task_manager] Task actions:smtp "BBi4cGsBPLu1qZWi-8_T" failed: Error: params invalid: child "to" fails because ["to" must be an array]

This seems bad because it will just chew up task manager slots that could be doing real work. Definitely seems like we want to make the system smart enough not to do something like this.

[pmuellr]

Even though it seems like throwing an error here could be problematic (infinite retries), hard to tell without seeing it in practice. How would we test this? Seems like we might want a set of "poorly behaved alerts and actions" that we could run as manual tests, or something, probably with specialized action and alert types.

I'm not terribly concerned about it, just wondering, I'm sure there's other throws in the mix here as well. We'll figure it out when we can see what it does.

[mikecote]

This seems bad because it will just chew up task manager slots that could be doing real work. Definitely seems like we want to make the system smart enough not to do something like this.

I agree, I think that's where we can tweak the behaviour when we do the error handling phase. I think there's a few scenarios like you described that will cause infinite retries that we don't want to do.

In regards to this change, I still think throwing an error makes sense? And we take care of the infinite retries separately?

@bmcconaghy @pmuellr thoughts?

[pmuellr]

We'll have to figure out a story for "retryable" errors thrown during processing, I'm sure there's plenty more lurking in the implementation.

In the end, I think we would want to display an error indication in the alert UI for this; it's quite possible a developer would never see this error during testing, but end up seeing it in production, and we'll want to provide as much feedback as we can in the notification UI. If we didn't throw an error, what would we do instead? (hint, write more code!) . And we'll throw errors in other cases anyway, so I think throwing here will have to be fine, eventually.

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request