re-add alert groups endpoint #1791

stuartnelson3 · 2019-03-11T20:50:39Z

requested in #868

this contains a lot that is open to discussion, including:

what the definition of the returned struct/struct members for the alert groups endpoint should be
the implementation of finding the groups, particularly the weird bit with the giving the EnrichedAlert a reference to a slice of its receivers. staticcheck isn't happy about how I went about doing this, either.
all the naming (like EnhancedAlert), and the path name /alerts/groups

I decided to wait on writing any tests until we can figure out what it is we want to have in this pr

stuartnelson3 · 2019-03-11T20:51:56Z

PR that removed alert groups originally: #1525

simonpasquier

Just a few notes but I haven't look into the details yet.

simonpasquier · 2019-03-12T13:40:28Z

api/v2/api.go

+		matchers       = []*labels.Matcher{}
+	)
+
+	if params.Filter != nil {


This can be removed.

api/v2/openapi.yaml

mxinden

Only looked at the API code for now. I will take a look at the dispatcher logic in a bit. This week is a bit packed, I am sorry for the delay.

api/v2/openapi.yaml

mxinden · 2019-03-12T16:22:23Z

api/v2/openapi.yaml

+      labels:
+        $ref: '#/definitions/labelSet'
+      receiver:
+        type: string


What do you think of reusing the receiver definition below?

Why is receiver an object? I thought we might want to expose extra info about a single receiver in the future.

Hm, any idea what we might expose? A receiver is mostly connection information that we don't want to expose.

Not aware of anything specific. I am fine with either way, I would just prefer to be consistent.

Unfortunately, I'm fine either way too :) if you want to add the extra info let me know, otherwise i'll leave it as-is

Then I would prefer reusing the receiver definition below. (We might want to expose the receiver type in the future.)

mxinden · 2019-03-12T16:24:42Z

api/v2/api.go

+			Alerts:   make([]*open_api_models.GettableAlert, 0, len(alertGroup.Alerts)),
+		}
+
+		for _, ea := range alertGroup.Alerts {


What does ea stand for?

enriched alert. happy to change it, it's fairly cryptic.

That would be helpful, thanks.

api/v2/api.go

stuartnelson3 · 2019-03-12T17:23:24Z

Aw, if I accept your suggestions they fail because they have no DCO

dispatch/dispatch.go

simonpasquier

My main feedback would be to keep the dispatch package mostly dumb and move most of the logic to the API v2.

dispatch/dispatch.go

stuartnelson3 · 2019-03-25T17:26:26Z

Made a lot of changes based on @simonpasquier 's suggestions, when you have a chance take a look. Did some rough manual testing and it seems to be working.

mxinden

Logic in dispatch looks very clean! Just small comments, nothing major.

Given that this could fix the compatibility issues with karma, @prymitive do you have thoughts on this as well?

mxinden · 2019-03-29T11:04:05Z

api/v2/api.go

@@ -53,6 +54,7 @@ type API struct {
 	peer           *cluster.Peer
 	silences       *silence.Silences
 	alerts         provider.Alerts
+	groups         groupsFn


For me this is ambiguous, does it return groups of alerts, silences, receivers, apples, ...? How about alertGroups?

Apples would be nice. Will update :)

mxinden · 2019-03-29T11:05:54Z

api/v2/api.go

+	if params.Receiver != nil {
+		receiverFilter, err = regexp.Compile("^(?:" + *params.Receiver + ")$")
+		if err != nil {
+			return alertgroup_ops.


In the above for loop we log the error, here we don't. Can we be consistent with either way?

mxinden · 2019-03-29T11:09:18Z

api/v2/openapi.yaml

+      labels:
+        $ref: '#/definitions/labelSet'
+      receiver:
+        type: string


Then I would prefer reusing the receiver definition below. (We might want to expose the receiver type in the future.)

stuartnelson3 · 2019-03-29T14:44:47Z

ptal when you two have a chance

api/v2/api.go

dispatch/dispatch.go

prymitive · 2019-03-29T16:18:54Z

Logic in dispatch looks very clean! Just small comments, nothing major.

Given that this could fix the compatibility issues with karma, @prymitive do you have thoughts on this as well?

In general this seem to make it easier for me, for example karma already has a concept of alert group, I don't see anything that could be problematic for me in any obvious ways. That being said I'm currently traveling so I'm not gonna pretend that I've put a lot of effort into reviewing this or trying to use this code.

stuartnelson3 · 2019-04-02T16:18:27Z

Added a test

prymitive · 2019-04-05T21:10:02Z

FYI I'll be back home next week and I'll be able to do some integration tests agains this PR for karma, if that's helpful

stuartnelson3 · 2019-04-06T09:48:09Z

Definitely! would love to hear if this is useful for you, or if there's anything you would like to add/change

mohsen0 · 2019-04-10T14:11:30Z

Can this PR be progressed?
Thanks

stuartnelson3 · 2019-04-10T14:54:15Z

Can this PR be progressed?

Waiting for some feedback from @prymitive, and if there are any more questions/comments from @mxinden and @simonpasquier

prymitive · 2019-04-10T14:55:38Z

I hope to get some work done today evening

prymitive · 2019-04-10T18:25:22Z

I was able to get it working easily with those changes and it seems to be working perfectly 🎉
I used the alert generator I have on http://karma-demo.herokuapp.com and I got exact same set of alerts via the v2 API 🎉
It also looks like I could kill some of the ugly code I have in the backend if I would drop support for non openapi alertmanager versions.
There's really not much feedback I can provide other than "it works" since all karma is doing is pulling all silences and alerts, so it's a very basic use of the API provided.
Thanks! Great work!

stuartnelson3 · 2019-04-11T08:30:32Z

great to hear! once I get a final 👍 from @mxinden and @simonpasquier this will be merged

mxinden

Other than #1791 (comment) this looks good to me. Thanks @stuartnelson3!

masalawarrior · 2019-04-15T15:24:11Z

@stuartnelson3 You got a chance to look at Max's comment ? Would be great to have this merged in soon. Thanks for your work on this :)

stuartnelson3 · 2019-04-15T15:59:21Z

@stuartnelson3 You got a chance to look at Max's comment ? Would be great to have this merged in soon. Thanks for your work on this :)

thanks for the ping, i missed his message!

Other than #1791 (comment) this looks good to me. Thanks @stuartnelson3!

sorry for missing that log line, I added it to the alert groups endpoint but not the normal alerts endpoint. hopefully now this PR is good to go

mohsen0 · 2019-04-16T13:26:48Z

@mxinden Are you happy with the latest changes?

mxinden

This looks good to me. @stuartnelson3 would you mind reducing the amount of commits in the pull request? Otherwise we can also squash them to a single one. As you wish.

xocasdashdash · 2019-04-17T09:17:27Z

Once this PR is merged, when do you think the next alertmanager version is going to be released?

Signed-off-by: stuart nelson <stuartnelson3@gmail.com>

stuartnelson3 · 2019-04-17T09:58:39Z

Once this PR is merged, when do you think the next alertmanager version is going to be released?

Not sure. I would like to update the ui to use this groups endpoint by default when displaying alerts, which could hopefully be done next week.

stuartnelson3 · 2019-04-24T21:27:13Z

I plan on releasing 0.17.0 next week, which will include this PR.

stuartnelson3 requested review from simonpasquier and mxinden March 11, 2019 20:50

simonpasquier reviewed Mar 12, 2019

View reviewed changes

mxinden reviewed Mar 12, 2019

View reviewed changes

stuartnelson3 force-pushed the stn+gs/alert-groups-endpoint branch from aa088c4 to 7dbed51 Compare March 12, 2019 17:23

stuartnelson3 commented Mar 13, 2019

View reviewed changes

dispatch/dispatch.go Outdated Show resolved Hide resolved

ScrumpyJack mentioned this pull request Mar 19, 2019

Don't work with the latest version api (v2) alertmanager prymitive/karma#115

Closed

simonpasquier reviewed Mar 20, 2019

View reviewed changes

dispatch/dispatch.go Outdated Show resolved Hide resolved

dispatch/dispatch.go Outdated Show resolved Hide resolved

dispatch/dispatch.go Outdated Show resolved Hide resolved

mxinden reviewed Mar 29, 2019

View reviewed changes

simonpasquier reviewed Mar 29, 2019

View reviewed changes

api/v2/api.go Outdated Show resolved Hide resolved

api/v2/api.go Outdated Show resolved Hide resolved

dispatch/dispatch.go Outdated Show resolved Hide resolved

mxinden approved these changes Apr 11, 2019

View reviewed changes

mxinden approved these changes Apr 17, 2019

View reviewed changes

stuartnelson3 force-pushed the stn+gs/alert-groups-endpoint branch from c4a0c5b to 71c6bd0 Compare April 17, 2019 09:21

add groups endpoint to v2 api

2fa210d

Signed-off-by: stuart nelson <stuartnelson3@gmail.com>

stuartnelson3 force-pushed the stn+gs/alert-groups-endpoint branch from 71c6bd0 to 2fa210d Compare April 17, 2019 09:32

stuartnelson3 merged commit b694eef into master Apr 17, 2019

stuartnelson3 deleted the stn+gs/alert-groups-endpoint branch April 17, 2019 10:24

Spaceman1701 mentioned this pull request Aug 7, 2023

Re-add group key to alert groups endpoint #3446

Open

re-add alert groups endpoint #1791

re-add alert groups endpoint #1791

Conversation

stuartnelson3 commented Mar 11, 2019

stuartnelson3 commented Mar 11, 2019 • edited Loading

simonpasquier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartnelson3 commented Mar 12, 2019

simonpasquier left a comment

Choose a reason for hiding this comment

stuartnelson3 commented Mar 25, 2019

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartnelson3 commented Mar 29, 2019

prymitive commented Mar 29, 2019

stuartnelson3 commented Apr 2, 2019

prymitive commented Apr 5, 2019

stuartnelson3 commented Apr 6, 2019

mohsen0 commented Apr 10, 2019

stuartnelson3 commented Apr 10, 2019

prymitive commented Apr 10, 2019

prymitive commented Apr 10, 2019

stuartnelson3 commented Apr 11, 2019

mxinden left a comment

Choose a reason for hiding this comment

masalawarrior commented Apr 15, 2019

stuartnelson3 commented Apr 15, 2019

mohsen0 commented Apr 16, 2019

mxinden left a comment

Choose a reason for hiding this comment

xocasdashdash commented Apr 17, 2019

stuartnelson3 commented Apr 17, 2019

stuartnelson3 commented Apr 24, 2019

stuartnelson3 commented Mar 11, 2019 •

edited

Loading