Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

re-add alert groups endpoint #1791

Merged
merged 1 commit into from Apr 17, 2019
Merged

re-add alert groups endpoint #1791

merged 1 commit into from Apr 17, 2019

Conversation

@stuartnelson3
Copy link
Member

stuartnelson3 commented Mar 11, 2019

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 stuartnelson3 requested review from simonpasquier and mxinden Mar 11, 2019
@stuartnelson3

This comment has been minimized.

Copy link
Member Author

stuartnelson3 commented Mar 11, 2019

PR that removed alert groups originally: #1525

Copy link
Member

simonpasquier left a comment

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

matchers = []*labels.Matcher{}
)

if params.Filter != nil {

This comment has been minimized.

Copy link
@simonpasquier

simonpasquier Mar 12, 2019

Member

This can be removed.

api/v2/openapi.yaml Show resolved Hide resolved
Copy link
Member

mxinden left a comment

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 Outdated Show resolved Hide resolved
labels:
$ref: '#/definitions/labelSet'
receiver:
type: string

This comment has been minimized.

Copy link
@mxinden

mxinden Mar 12, 2019

Member

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.

This comment has been minimized.

Copy link
@stuartnelson3

stuartnelson3 Mar 12, 2019

Author Member

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

This comment has been minimized.

Copy link
@mxinden

mxinden Mar 18, 2019

Member

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

This comment has been minimized.

Copy link
@stuartnelson3

stuartnelson3 Mar 25, 2019

Author Member

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

This comment has been minimized.

Copy link
@mxinden

mxinden Mar 29, 2019

Member

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

Alerts: make([]*open_api_models.GettableAlert, 0, len(alertGroup.Alerts)),
}

for _, ea := range alertGroup.Alerts {

This comment has been minimized.

Copy link
@mxinden

mxinden Mar 12, 2019

Member

What does ea stand for?

This comment has been minimized.

Copy link
@stuartnelson3

stuartnelson3 Mar 12, 2019

Author Member

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

This comment has been minimized.

Copy link
@mxinden

mxinden Mar 18, 2019

Member

That would be helpful, thanks.

api/v2/api.go Outdated Show resolved Hide resolved
@stuartnelson3 stuartnelson3 force-pushed the stn+gs/alert-groups-endpoint branch from aa088c4 to 7dbed51 Mar 12, 2019
@stuartnelson3

This comment has been minimized.

Copy link
Member Author

stuartnelson3 commented Mar 12, 2019

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

dispatch/dispatch.go Outdated Show resolved Hide resolved
Copy link
Member

simonpasquier left a comment

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 Outdated Show resolved Hide resolved
dispatch/dispatch.go Outdated Show resolved Hide resolved
dispatch/dispatch.go Outdated Show resolved Hide resolved
@stuartnelson3

This comment has been minimized.

Copy link
Member Author

stuartnelson3 commented Mar 25, 2019

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.

Copy link
Member

mxinden left a comment

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?

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

This comment has been minimized.

Copy link
@mxinden

mxinden Mar 29, 2019

Member

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

This comment has been minimized.

Copy link
@stuartnelson3

stuartnelson3 Mar 29, 2019

Author Member

Apples would be nice. Will update :)

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

This comment has been minimized.

Copy link
@mxinden

mxinden Mar 29, 2019

Member

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

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

This comment has been minimized.

Copy link
@mxinden

mxinden Mar 29, 2019

Member

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

@stuartnelson3

This comment has been minimized.

Copy link
Member Author

stuartnelson3 commented Mar 29, 2019

ptal when you two have a chance

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
@prymitive

This comment has been minimized.

Copy link
Contributor

prymitive commented Mar 29, 2019

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

This comment has been minimized.

Copy link
Member Author

stuartnelson3 commented Apr 2, 2019

Added a test

@prymitive

This comment has been minimized.

Copy link
Contributor

prymitive commented Apr 5, 2019

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

This comment has been minimized.

Copy link
Member Author

stuartnelson3 commented Apr 6, 2019

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

@mohsen0

This comment has been minimized.

Copy link

mohsen0 commented Apr 10, 2019

Can this PR be progressed?
Thanks

@stuartnelson3

This comment has been minimized.

Copy link
Member Author

stuartnelson3 commented Apr 10, 2019

Can this PR be progressed?

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

@prymitive

This comment has been minimized.

Copy link
Contributor

prymitive commented Apr 10, 2019

I hope to get some work done today evening

@prymitive

This comment has been minimized.

Copy link
Contributor

prymitive commented Apr 10, 2019

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

This comment has been minimized.

Copy link
Member Author

stuartnelson3 commented Apr 11, 2019

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

Copy link
Member

mxinden left a comment

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

@divishkumar

This comment has been minimized.

Copy link

divishkumar commented Apr 15, 2019

@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

This comment has been minimized.

Copy link
Member Author

stuartnelson3 commented Apr 15, 2019

@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

This comment has been minimized.

Copy link

mohsen0 commented Apr 16, 2019

@mxinden Are you happy with the latest changes?

Copy link
Member

mxinden left a comment

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

This comment has been minimized.

Copy link

xocasdashdash commented Apr 17, 2019

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

@stuartnelson3 stuartnelson3 force-pushed the stn+gs/alert-groups-endpoint branch from c4a0c5b to 71c6bd0 Apr 17, 2019
Signed-off-by: stuart nelson <stuartnelson3@gmail.com>
@stuartnelson3 stuartnelson3 force-pushed the stn+gs/alert-groups-endpoint branch from 71c6bd0 to 2fa210d Apr 17, 2019
@stuartnelson3

This comment has been minimized.

Copy link
Member Author

stuartnelson3 commented Apr 17, 2019

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 stuartnelson3 merged commit b694eef into master Apr 17, 2019
5 checks passed
5 checks passed
DCO DCO
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@stuartnelson3 stuartnelson3 deleted the stn+gs/alert-groups-endpoint branch Apr 17, 2019
@stuartnelson3

This comment has been minimized.

Copy link
Member Author

stuartnelson3 commented Apr 24, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.