Implement quorum reads and response merging for /v1/alerts. #4126
Conversation
Reading alert listings via /v1/alerts will now read from a quorum of replicas and return a merged response. The merging logic returns a union of the alerts from all responses. When the same alert is returned by multiple replicas, one is just chosen arbitrarily. This should be sufficient as alerts posted to each replica should be identical. The replicas may have some disagreement on the alert status, but this should become consistent over time. For the /v2 API, we can use the "updatedAt" timestamp to choose the most up-to-date status, but this field is not available for the /v1 API. Merging for /v2/alerts and /v2/alerts/groups to follow. Signed-off-by: Steve Simpson <[email protected]>
| @@ -0,0 +1,15 @@ | |||
| package merger | |||
There was a problem hiding this comment.
Super-Nit: Not sure this package carries its own weight. I know there will be v2 implementation too, but it's still looks pretty thin. Same goes for Merger interface.
There was a problem hiding this comment.
I agree and wasn't 100% sure what to do. The pkg/alertmanager package is getting very busy, was my reasoning here.
Would a pkg/alertmanager/distributor perhaps be better?
Also regarding the interface, I'd be equally happy with functions, if that's what you meant? (I was mainly taking inspiration from here
)There was a problem hiding this comment.
Personally I would just use pkg/alertmanager, I think it's still a small package.
Also regarding the interface, I'd be equally happy with functions, if that's what you meant?
Yes, I was thinking about just using functions here. My reasoning is that 1) we're only going to have 2 implementations (v1, v2 api) and 2) client doesn't need to supply its own implementation. But I am first to admit this is just a personal preference for this specific case, and don't mind keeping it as-is.
There was a problem hiding this comment.
I don't have a strong opinion here. I personally did like the merger package, given it's content is self contained but moving to pkg/alertmanager looks good to me as well. Let's just not block on this. V2 merging PR is awaiting for this PR! 😉
Would a pkg/alertmanager/distributor perhaps be better?
I wouldn't name the package distributor because we already have pkg/distributor and in the past (when we tried pkg/alertmanager/distributor at some point) we realised it was causing much confusion when reading the code.
There was a problem hiding this comment.
Leaving as-is to unblock, happy to refactor after the fact.
Signed-off-by: Steve Simpson <[email protected]>
stevesg
force-pushed
the
am-merging-part1
branch
from
9596292 to
d72a357
Compare
What this PR does:
Reading alert listings via /v1/alerts will now read from a quorum of
replicas and return a merged response. The merging logic returns a union
of the alerts from all responses. When the same alert is returned by
multiple replicas, one is just chosen arbitrarily. This should be
sufficient as alerts posted to each replica should be identical.
The replicas may have some disagreement on the alert status, but this
should become consistent over time. For the /v2 API, we can use the
"updatedAt" timestamp to choose the most up-to-date status, but this
field is not available for the /v1 API.
Merging for /v2/alerts and /v2/alerts/groups to follow.
Checklist
Documentation addedCHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]