Alerting: Fix alert flapping in the internal alertmanager

[gotjosh]

fixes a bug that caused Alerts that are evaluated at low intervals (sub 1 minute), to flap in the Alertmanager.
Mostly due to a combination of EndsAt and resend delay.

The Alertmanager uses EndsAt as a heuristic to know whenever it should resolve a firing alert, in the case that it hasn't heard
back from the alert generation system.

Because Grafana sent the alert with an EndsAt which is equal to the For of the alert itself,
and we had a hard-coded 1-minute re-send delay (only applicable to firing alerts) this meant that a firing alert would resolve in the Alertmanager before we re-notify that is still firing.

This commit, increases the EndsAt by 3x the for duration in the case where the For is > interval e.g. For 30s each 10s, but also lowers the resend delay - 1 minute seemed too high when we support any For`.

Fixes #38419 and fixes #37461

[gotjosh]

I have added a task to https://github.com/grafana/alerting-squad/issues/59 to make sure we make this configurable via the admin options so that users can change this at runtime.

[grobinson-grafana]

This change doesn't mean that notifications will be sent once per elapsed 10 seconds instead of once per minute right? This is just the interval at which we resend the alert to Alertmanager which will then itself choose whether to re-send the notification or not?

[kylebrandt]

@gerobinson Correct, it is like a keep-alive msg from the state manager to alert manager (since in AM, if endsAt is passed, the alert instance is resolved).

[gotjosh]

I think 3 the evaluation interval seems like a good enough trade-off between flapping and quickly resolving the alert once it has cleared but please do let me know if you have any other opinions.

[kylebrandt]

I'm wondering this should be >= since if For = Interval it only gets 2x below.

[kylebrandt]

One comment on when For == eval interval, but LGTM.

I was able to repro the firinng/resolve with the reported "bad run/for timings" of 10s/1m and 10s/30s on main (as long as I set the 1s wait timings on the policy), and it stops with this branch.

[gotjosh]

@kylebrandt I have opted for changing how this works. I believe that taking into account the resend delay or the interval of the alert is what really matters when it comes to determining when it should be considered as resolved in the Alertmanager.

Therefore, I have simplified the calculation for it. In the end, the outcome should be the same and fix any floppiness we do have.

I have one last question regarding this area and is that I don't see how we would always send a resolve alert (an alert going from alerting -> normal with an EndsAt of now) irrespective of whether we've notified recently for this alert because NeedsSending does not take this into account but there might be some time play with dates to make sure it always matches the last conditional - I need to investigate.

func (a *State) NeedsSending(resendDelay time.Duration) bool {
	if a.State != eval.Alerting && a.State != eval.Normal {
		return false
	}

	if a.State == eval.Normal && !a.Resolved {
		return false
	}
	// if LastSentAt is before or equal to LastEvaluationTime + resendDelay, send again
	return a.LastSentAt.Add(resendDelay).Before(a.LastEvaluationTime) ||
		a.LastSentAt.Add(resendDelay).Equal(a.LastEvaluationTime)
}

[kylebrandt]

Retried the two "bad timings" I referenced in my previous comment on the updated branch, things still behaving well with the updates 🎉

[gotjosh]

To answer my last comment, this works as I expected it to be as well - no further concerns from my side. The dates matches and at most, we'll get a delay of resendDelay on sending the resolve notifications in the worst case which is not that bad.