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

also hide inhibited alerts #1039

Merged
merged 2 commits into from
Oct 17, 2017
Merged

also hide inhibited alerts #1039

merged 2 commits into from
Oct 17, 2017

Conversation

caarlos0
Copy link
Contributor

refs #1034

I feel like it would be better to rename the form parameter to something else, since it's not only silenced alerts anymore... what do you folks think?

@brancz
Copy link
Member

brancz commented Oct 12, 2017

Internally this is often referred to as "muted".

@caarlos0
Copy link
Contributor Author

@beorn7 @stuartnelson3 what do you guys think about renaming this form param to muted?

@beorn7
Copy link
Member

beorn7 commented Oct 13, 2017

We could do that. Or we create a new parameter showInhibited to give the UI the option to select those separately even if we wouldn't use it for now.

@brancz
Copy link
Member

brancz commented Oct 13, 2017

Yes I think it would be good to be able to view inhibited, silenced, and firing alerts in all combinations.

@stuartnelson3
Copy link
Contributor

+1 to beorn's suggestion. Keep it flexibile in the API even if we don't make it an option from the UI.

@caarlos0
Copy link
Contributor Author

@stuartnelson3 @beorn7 @brancz done!

@beorn7
Copy link
Member

beorn7 commented Oct 16, 2017

@stuartnelson3 I'd better leave the code level review to you. (One day, I'll get familiar with Alertmanager internals, but not today…)

Copy link
Member

@brancz brancz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good so far, but I would expect that I can toggle the front end to show me the inhibited alerts. As far as I can tell from this PR, this only handles the API filtering part. (if that is meant to be done in a subsequent PR, I'm fine with that as well)

@@ -343,6 +359,10 @@ func (api *API) listAlerts(w http.ResponseWriter, r *http.Request) {
continue
}

if !showInhibited && len(status.InhibitedBy) != 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still hoping that we'll one day have a real index around this 🙂

@caarlos0
Copy link
Contributor Author

Generally looks good so far, but I would expect that I can toggle the front end to show me the inhibited alerts. As far as I can tell from this PR, this only handles the API filtering part. (if that is meant to be done in a subsequent PR, I'm fine with that as well)

Yeah, as far as I understood the original issue was about the backend only. I can change the frontend as well if required...

@beorn7
Copy link
Member

beorn7 commented Oct 16, 2017

@w0rm will take care of the frontend part (IIRC).

@brancz
Copy link
Member

brancz commented Oct 17, 2017

In that case, this lgtm

@stuartnelson3 stuartnelson3 merged commit 26489b1 into prometheus:master Oct 17, 2017
@caarlos0 caarlos0 deleted the silences-and-inhibitions branch October 17, 2017 10:34
hh pushed a commit to ii/alertmanager that referenced this pull request Aug 14, 2018
…metheus#1039)

* If NRestarts or NRefused are not available, don't ignore the unit itself
* Don't report systemd metrics (NRestarts/NRefused) that are not available

Signed-off-by: James Hartig <james@getadmiral.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants