Update status impl by stuartnelson3 · Pull Request #717 · prometheus/alertmanager

stuartnelson3 · 2017-04-18T17:11:41Z

Adding on to #711

stuartnelson3 · 2017-04-18T17:15:24Z

inhibit/inhibit.go

 		}
 	}
-	ih.marker.SetInhibited(fp, false)
+	ih.marker.SetStatus(fp, types.Active)


I'm not sure if this is semantically correct. It appears that alerts will be marked in the notify pipeline, so setting them to active here would be temporary until they get checked as part of the pipeline. Can someone confirm? @brancz

If we don't know the real status until notify can re-check it again then maybe we should reset it into types.Unprocessed, wouldn't that make more sense?

Yeah, it has to go through both the inhibit and silence exec stages, so it'll be set correctly.

prymitive · 2017-04-18T23:33:50Z

types/types.go

+type state uint8
+
+const (
+	Unprocessed state = iota


I'm not sure if Unprocessed is the best name for it, I was thinking about calling it New but that on the other hand might be mistaken for an alert that just fired rather than indicate what we really mean here.

Yeah I'm not sure what name to provide. I don't feel strongly one way or the other.

prymitive · 2017-04-18T23:37:56Z

types/types.go

+		status = "unknown"
+	}
+
+	// TODO: Do we want to return all values?


This would make sense for Silenced status as there might be multiple silences that match our alert. But this means that notify would need to check all, which I see that this PR is adding, so I would say yes, we should return everything.

stuartnelson3 · 2017-04-21T12:45:35Z

Does this seem sane to people? @fabxc @brancz @mxinden

fabxc · 2017-04-21T14:00:34Z

types/types.go

+type State uint8
+
+const (
+	Unprocessed State = iota


Always prefix enums with the original type.

The type should be AlertState to know what it's even about. Thereby the values would be AlertStateUnprocessed etc.

fabxc · 2017-04-21T14:01:19Z

provider/mem/mem_test.go

 }

+func TestAlertsGC(t *testing.T) {
+	dir, err := ioutil.TempDir("", "alerts_test")


defer os.RemoveAll(dir)

fabxc · 2017-04-21T14:06:48Z

types/types.go

+	Unprocessed: "unprocessed",
+	Active:      "active",
+	Silenced:    "silenced",
+	Inhibited:   "inhibited",


An alert can be silenced and inhibited. More so once we move towards a model where this is shown immediately and not just after initial processing.

So "unprocessed", "active", and "suppressed" might be more accurate? @brian-brazil

Sounds understandable to me

I think there's a difference between silenced and inhibited from the user perspective and Alertmanager should distinguish between these two states. Inhibition and silencing is used for different purposes. One is "oh it's silenced, someone is working on it" the other is "oh it's caused by some other issue" (btw. there's no way to ACK an alert, so silences might be used as ACKs).
If alert is inhibited than it's clear that it's suppressed by another alert, so when that other alert is resolved our inhibited alert will fire. Silenced alert on the other hand will never fire while the silence is active. If you just see "suppressed" you won't know what that really means and whenever this alert will fire if there's an action to take in the near future (it won't if it's silenced).

True, but then from AlertStatus we could infer what sort of suppressed it is: if it has silenceIDs in the silencedBy field, it's silenced, if it has alertIDs in the inhibitedBy field it's inhibited. I suppose I'm not a big fan of having to infer the state, though.

fabxc · 2017-04-21T14:10:13Z

types/types.go

+// AlertStatus stores the state and values associated with an Alert.
+type AlertStatus struct {
+	status State
+	values []string


Just having silence IDs here as a bunch of strings might fit with the UI currently offers, but it has semantically no real meaning from an API perspective.
What if we also want to show which alerts inhibited the given alert? How would we distinguish its ID from that of a silence.

I think the status should rather have "status", "silencedBy" (list of silence IDs), and "inhibitedBy" (list of alert IDs).

Also the attributes are both unexported so they shouldn't be marshalled by the JSON decoder, no?

AlertStatus implements the marshaler interface so we can convert the enum into a string representation, so that handles marshaling the unexported fields.

I flatten the AlertStatus inside APIAlert in dispatch.go:139, but I'm not sure if that's the "right" way to do it.

stuartnelson3 · 2017-04-24T09:47:33Z

inhibit/inhibit.go

+			// distinguish AlertStateSuppressed between silenced
+			// and/or inhibited. In the future there will be unique
+			// identifiers for alerts.
+			ih.marker.SetInhibited(fp, "123")


I put these semantics in place now even though we won't use them. This does lead to potential confusing behavior if we attempt to SetInhibited elsewhere before unique identifiers for alerts exist, but the symmetry with SetSilenced made sense to me.

This will be used to track and expose the status of an alert, it will replace dedicated fields in the Marker

Serialize and return all values to clients.

The inhibited stage is the first in the notify pipeline. If the alert here isn't inhibited and the pipeline continues, the alert's state will correctly be changed to Active or Silenced in the silence stage.

Differentiate based on whether the state stores a list of silenceIDs or inhibitIDs

stuartnelson3 · 2017-04-25T10:34:02Z

inhibit/inhibit.go

-			// identifiers for alerts.
-			ih.marker.SetInhibited(fp, "123")
+		if inhibitedByFP, eq := r.hasEqual(lset); r.TargetMatchers.Match(lset) && eq {
+			ih.marker.SetInhibited(fp, fmt.Sprintf("%d", inhibitedByFP))


@fabxc Does this look correct?

fabxc · 2017-04-27T12:18:22Z

👍

stuartnelson3 · 2017-04-27T12:21:08Z

thanks for getting this started @prymitive !!!

prymitive · 2017-04-27T16:08:50Z

Is there any current estimate when would the next release be out? One that would contain this and possibly #748

brancz · 2017-04-27T16:19:19Z

A patch release should go out very soon to release #750

stuartnelson3 commented Apr 18, 2017

View reviewed changes

prymitive reviewed Apr 18, 2017

View reviewed changes

prymitive approved these changes Apr 18, 2017

View reviewed changes

prymitive mentioned this pull request Apr 18, 2017

Issue 609 2 #711

Closed

stuartnelson3 force-pushed the update-status-impl branch from 8181492 to ac6f8eb Compare April 21, 2017 09:57

fabxc reviewed Apr 21, 2017

View reviewed changes

stuartnelson3 commented Apr 24, 2017

View reviewed changes

mattbostock mentioned this pull request Apr 24, 2017

Fix resolve notifications #703

Merged

prymitive and others added 19 commits April 25, 2017 09:50

Add alertStatus with custom JSON serializer

dc1f5e6

This will be used to track and expose the status of an alert, it will replace dedicated fields in the Marker

Use status field for tracking inhibited alerts

69bd013

Use status field for tracking silenced alerts

f18c7a3

Set alerts as active once they are processed by notify

cb34c44

Make alertStatus public and use it in API responses

961c9a0

Update alert partial to use the status field

c5d120f

Update app.js to use the status field

ddf1489

Rebuild binary assets data

fdb3d3a

Update status implementation

6c01e19

Add GC for Marker map

4e01ef1

Change struct member value->values

97be726

Serialize and return all values to clients.

Add comments to exported functions

d62bad5

Set status to Unprocessed if not inhibited.

d191fab

The inhibited stage is the first in the notify pipeline. If the alert here isn't inhibited and the pipeline continues, the alert's state will correctly be changed to Active or Silenced in the silence stage.

Update for cli

6e46ccc

Fix signature on Marshaler impl for State

913e286

Update assets

4e9765a

Use AlertStateSuppressed for silenced/inhibited

cf35a6c

Differentiate based on whether the state stores a list of silenceIDs or inhibitIDs

Fix serialization test

fde0283

Update SetSilenced and SetInhibited internals

5d1464b

stuartnelson3 force-pushed the update-status-impl branch from 5d03da7 to 5d1464b Compare April 25, 2017 07:50

Update assets

b1c6c67

stuartnelson3 force-pushed the update-status-impl branch from 885c14f to b1c6c67 Compare April 25, 2017 08:01

Attach inhibiting alert FP to alert status

9dc1805

stuartnelson3 commented Apr 25, 2017

View reviewed changes

stuartnelson3 mentioned this pull request Apr 26, 2017

Alertpage silenced / inhibited checkbox #688

Closed

fabxc merged commit 6a909ab into master Apr 27, 2017

fabxc deleted the update-status-impl branch April 27, 2017 12:18

This was referenced Apr 29, 2017

API shows briefly reports alerts as not being silenced until silences are applied #609

Closed

Refactor alert status attrs cloudflare/unsee#86

Merged

Comments

Conversation

stuartnelson3 commented Apr 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartnelson3 commented Apr 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabxc Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabxc commented Apr 27, 2017

Uh oh!

stuartnelson3 commented Apr 27, 2017

Uh oh!

prymitive commented Apr 27, 2017

Uh oh!

brancz commented Apr 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fabxc Apr 21, 2017 •

edited

Loading