Expose alert fingerprint in the API #786

prymitive · 2017-05-10T17:34:56Z

Alert fingerprint is already provided as the value of status.inhibitedBy[] attribute that inhibited alerts have, but there's no way to get back to the alert that's inhibiting it as the fingerprint is not exposed.

I'm not sure if using fingerprints is desirable, but that's currently the value of inhibitedBy @stuartnelson3
The goal here is to be able to link back from inhibited to inhibiting alert based on the API response, which isn't currently doable.

stuartnelson3 · 2017-05-10T20:09:01Z

I think fingerprints are the method of identification we're going to be using for now, but with this method you're required to filter through all the alerts being sent from the current api endpoint. I think it would make more sense to have a new endpoint added, like /api/v0/alerts/abc-fingerprint-123, and then maintain a map[Fingerprint]Alert to query against. This map would have to have access coordinated to it for additions and deletions.

@fabxc is this new endpoing something that should be added?

prymitive · 2017-05-10T22:06:08Z

Is there a reason to hide fingerprints in the /api/v1/alerts/groups endpoint?
A dedicated endpoint to get an alert by id seems like a good idea, but you should still be able to get an id of any alert, not just those that are inhibiting others.
Use case example: I have an alert, I want to see if it's inhibiting any other alert - for that I need to know the id. Without the id I need to get all inhibited alerts, fetch each inhibiting alert by id and compare it with the alert that I'm checking.

stuartnelson3 · 2017-05-10T22:32:37Z

Makes sense to me, but waiting on a response from @fabxc

fabxc · 2017-06-06T05:01:44Z

Probably makes sense to add them to our API. We can make individual alerts gettable once there's a use case.

stuartnelson3

Could you also add this to https://github.com/prometheus/alertmanager/blob/master/api/api.go#L254? The frontend is now using the list endpoint instead of the groups endpoint.

prymitive · 2017-06-08T03:43:26Z

Added to the list endpoint

mxinden · 2017-06-08T11:28:21Z

api/api.go

@@ -314,6 +315,7 @@ func (api *API) listAlerts(w http.ResponseWriter, r *http.Request) {
 		apiAlert := &APIAlert{
 			Alert:  &a.Alert,
 			Status: status,
+			ID:     fmt.Sprintf("%d", a.Fingerprint()),


Could we print this not as base10 but as base16 with %x?

Is this to reduce the length of the ID?

No, it's just the base 16, with lower-case letters for a-f. See https://golang.org/pkg/fmt/#hdr-Printing

Right, I'm just wondering why you want base 16 instead of base 10

I think this was a suggestion by @fabxc. @fabxc can you answer this?

I am sorry for the wrong answer here @stuartnelson3. Yes, changing to base16 is due to reducing the length. In addition we prevent someone of interpreting the fingerprint as an increasing integer, and we enforce exposing the fingerprint as a string in a JSON data structure.

mxinden · 2017-06-08T11:29:13Z

dispatch/dispatch.go

@@ -81,6 +81,7 @@ type AlertBlock struct {
 type APIAlert struct {
 	*model.Alert
 	Status types.AlertStatus `json:"status"`
+	ID     string            `json:"id"`


As the fingerprint is not really an id, I would prefer renaming this to HASH. What do you think?

I'm not a fan of hash since that's naming it after its implementation. I'm fine with id for now since the fingerprint is how we're identifying the alerts.

I am not sure if fingerprints are entirely unique, so thereby id might be misleading. @fabxc What do you think?

I do not think id is the right naming. An id would give it the notion of uniqueness and uniqueness over time. An alert can appear/disappear and even alerting rules get changed with or without the labels of the alert changing.

Since we all refer to it as this, why not fingerprint?

fingerprint is fine by me

mxinden

Just some small concerns.

mxinden · 2017-08-14T16:31:18Z

@prymitive Do you have some time to finish this PR? In case you are too busy I can take over.

prymitive · 2017-08-14T16:35:27Z

I should have, I just need to remind myself what needs to be done here, will update soon

Alert fingerprint is already provided as the value of status.inhibitedBy[] attribute that inhibited alerts have, but there's no way to get back to the alert that's inhibiting it as the fingerprint is not exposed.

prymitive · 2017-08-14T17:32:26Z

Renamed ID->Fingerprint and using %x for printing instead of %d. I think that's all that was requested here

mxinden

@prymitive One more comment. I am sorry for the direction changes. Thanks a lot for bearing with me!

mxinden · 2017-08-16T07:06:38Z

api/api.go

+			Alert:       &a.Alert,
+			Status:      status,
+			Receivers:   receivers,
+			Fingerprint: fmt.Sprintf("%x", a.Fingerprint()),


@prymitive When I curl Alertmanager it does not expose the Fingerprint base16 encoded. I looked through the common package and found the String function. Would you mind changing it again to Fingerprint: a.Fingerprint().String(), then we are consistent across projects. I know there are plans to get rid of the commons repo in the future, but as we are using Fingerprint from the repo anyways, we might as well use Fingerprint.String(). Let me know if this sounds reasonable to you.

//cc @fabxc

That makes sense, thanks

"fingerprint": "6f02aa263df152bc"

mxinden · 2017-08-16T07:07:22Z

dispatch/dispatch.go

-					Status: status,
+					Alert:       a,
+					Status:      status,
+					Fingerprint: fmt.Sprintf("%x", a.Fingerprint()),


See comment in api/api.go in regards to using Fingerprint.String().

prymitive · 2017-08-16T21:29:07Z

Anything else left here?

prymitive · 2017-08-16T21:29:29Z

Should I squash those commits?

mxinden · 2017-08-17T21:37:47Z

@prymitive No need to squash, can do so via Github. What is the change in the template/internal/deftmpl/bindata.go in your last commit?

prymitive · 2017-08-17T22:09:49Z

Not sure, probably some dangling changes from building alertmanager locally for testing. Will nuke it.

prymitive · 2017-08-17T22:10:51Z

It's gone now

mxinden · 2017-08-18T17:30:07Z

@stuartnelson3 concern is addressed. I will merge here. Thanks @prymitive for the contribution and for the quick changes.

* Expose alert fingerprint in the API Alert fingerprint is already provided as the value of status.inhibitedBy[] attribute that inhibited alerts have, but there's no way to get back to the alert that's inhibiting it as the fingerprint is not exposed. * Expose alert fingerprint as ID in the list endpoint * Rename ID to Fingerprint * Use Fingerprint().String() in the API

stuartnelson3 approved these changes Jun 6, 2017

View reviewed changes

stuartnelson3 suggested changes Jun 6, 2017

View reviewed changes

mxinden reviewed Jun 8, 2017

View reviewed changes

mxinden requested changes Jun 8, 2017

View reviewed changes

mxinden added area/usability kind/enhancement labels Aug 13, 2017

mxinden mentioned this pull request Aug 13, 2017

Add ID for alerts and add alert ID's to silences #681

Closed

prymitive added 3 commits August 14, 2017 10:13

Expose alert fingerprint in the API

680bd5b

Alert fingerprint is already provided as the value of status.inhibitedBy[] attribute that inhibited alerts have, but there's no way to get back to the alert that's inhibiting it as the fingerprint is not exposed.

Expose alert fingerprint as ID in the list endpoint

d714dcf

Rename ID to Fingerprint

8ab0ea8

mxinden requested changes Aug 16, 2017

View reviewed changes

Use Fingerprint().String() in the API

14f6ce8

mxinden approved these changes Aug 18, 2017

View reviewed changes

mxinden merged commit 8e61ebf into prometheus:master Aug 18, 2017

prymitive deleted the alert-fingerprint-api branch September 29, 2017 02:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose alert fingerprint in the API #786

Expose alert fingerprint in the API #786

prymitive commented May 10, 2017

stuartnelson3 commented May 10, 2017

prymitive commented May 10, 2017 •

edited

Loading

stuartnelson3 commented May 10, 2017

fabxc commented Jun 6, 2017

stuartnelson3 left a comment

prymitive commented Jun 8, 2017

mxinden Jun 8, 2017

stuartnelson3 Jun 13, 2017

mxinden Jun 13, 2017

stuartnelson3 Jun 14, 2017

mxinden Jun 14, 2017

mxinden Aug 14, 2017

mxinden Jun 8, 2017

stuartnelson3 Jun 13, 2017

mxinden Jun 13, 2017

brancz Jun 14, 2017

stuartnelson3 Jun 14, 2017

mxinden left a comment

mxinden commented Aug 14, 2017

prymitive commented Aug 14, 2017

prymitive commented Aug 14, 2017

mxinden left a comment

mxinden Aug 16, 2017

prymitive Aug 16, 2017

mxinden Aug 16, 2017

prymitive commented Aug 16, 2017

prymitive commented Aug 16, 2017

mxinden commented Aug 17, 2017

prymitive commented Aug 17, 2017

prymitive commented Aug 17, 2017

mxinden commented Aug 18, 2017

Expose alert fingerprint in the API #786

Expose alert fingerprint in the API #786

Conversation

prymitive commented May 10, 2017

stuartnelson3 commented May 10, 2017

prymitive commented May 10, 2017 • edited Loading

stuartnelson3 commented May 10, 2017

fabxc commented Jun 6, 2017

stuartnelson3 left a comment

Choose a reason for hiding this comment

prymitive commented Jun 8, 2017

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

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

mxinden commented Aug 14, 2017

prymitive commented Aug 14, 2017

prymitive commented Aug 14, 2017

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

prymitive commented Aug 16, 2017

prymitive commented Aug 16, 2017

mxinden commented Aug 17, 2017

prymitive commented Aug 17, 2017

prymitive commented Aug 17, 2017

mxinden commented Aug 18, 2017

prymitive commented May 10, 2017 •

edited

Loading