inhibit: update inhibition cache when alerts resolve#1309
inhibit: update inhibition cache when alerts resolve#1309simonpasquier wants to merge 4 commits intoprometheus:masterfrom
Conversation
|
/cc @fabxc |
stuartnelson3
left a comment
stuartnelson3
left a comment
There was a problem hiding this comment.
The test definitely reliably fails when removing the code you added, which is great. However, the only code change in this PR that has an effect on the test (from running locally on my machine) is the check for a.Resolved(). All of the other code changes can be removed, and the acceptance tests still pass.
I'll admit, I'm confused. Reading the minimum working code from above that passes the test, I'm not sure what would really be happening by only removing the continue if a.Resolved() == true in the run() method.
| // Update the inhibition rules' cache. | ||
| for _, r := range ih.rules { | ||
| if r.SourceMatchers.Match(a.Labels) { | ||
| if r.exists(a) || r.SourceMatchers.Match(a.Labels) { |
There was a problem hiding this comment.
what is the purpose of r.exists(a)? It seems like from the code, if it exists, then it gets set (which doesn't accomplish anything, since it already exists?)
Running the tests without this code still passes.
There was a problem hiding this comment.
IIUC the only case where r.exists(a) would be false is when AlertManager processes a resolved alert that hasn't been seen as firing previously (eg after a restart). Not sure it is worth to keep it then.
inhibit/inhibit.go
Outdated
| // Only inhibit if target matchers match but source matchers don't. | ||
| if inhibitedByFP, eq := r.hasEqual(lset); !r.SourceMatchers.Match(lset) && r.TargetMatchers.Match(lset) && eq { | ||
| ih.marker.SetInhibited(fp, fmt.Sprintf("%d", inhibitedByFP)) | ||
| ih.marker.SetInhibited(fp, fmt.Sprintf("%s", inhibitedByFP.String())) |
There was a problem hiding this comment.
this can just be ih.marker.SetInhibited(fp, inhibitedByFP.String())
There was a problem hiding this comment.
It also appears that the original code works as well?
There was a problem hiding this comment.
Done. Yes the original code works but the API returns different fingerprint formats between silenced and inhibited alerts:
...snip...
"status": {
"state": "suppressed",
"silencedBy": [
"95c9789c-f7f6-4d57-99d3-66c9eb25a4b8"
],
"inhibitedBy": []
},
...snip..
...snip...
"status": {
"state": "suppressed",
"silencedBy": [],
"inhibitedBy": [
"12124264373735012735"
]
},
...snip...
simonpasquier
left a comment
simonpasquier
left a comment
There was a problem hiding this comment.
@stuartnelson3 thanks for the review! I've replied to your remarks.
I'm also extending the unit tests for the inhibit package so that we don't solely rely on acceptance testing for this code.
inhibit/inhibit.go
Outdated
| // Only inhibit if target matchers match but source matchers don't. | ||
| if inhibitedByFP, eq := r.hasEqual(lset); !r.SourceMatchers.Match(lset) && r.TargetMatchers.Match(lset) && eq { | ||
| ih.marker.SetInhibited(fp, fmt.Sprintf("%d", inhibitedByFP)) | ||
| ih.marker.SetInhibited(fp, fmt.Sprintf("%s", inhibitedByFP.String())) |
There was a problem hiding this comment.
Done. Yes the original code works but the API returns different fingerprint formats between silenced and inhibited alerts:
...snip...
"status": {
"state": "suppressed",
"silencedBy": [
"95c9789c-f7f6-4d57-99d3-66c9eb25a4b8"
],
"inhibitedBy": []
},
...snip..
...snip...
"status": {
"state": "suppressed",
"silencedBy": [],
"inhibitedBy": [
"12124264373735012735"
]
},
...snip...
| // Update the inhibition rules' cache. | ||
| for _, r := range ih.rules { | ||
| if r.SourceMatchers.Match(a.Labels) { | ||
| if r.exists(a) || r.SourceMatchers.Match(a.Labels) { |
There was a problem hiding this comment.
IIUC the only case where r.exists(a) would be false is when AlertManager processes a resolved alert that hasn't been seen as firing previously (eg after a restart). Not sure it is worth to keep it then.
inhibit/inhibit.go
Outdated
|
|
||
| // NewInhibitor returns a new Inhibitor. | ||
| func NewInhibitor(ap provider.Alerts, rs []*config.InhibitRule, mk types.Marker, logger log.Logger) *Inhibitor { | ||
| if logger == nil { |
There was a problem hiding this comment.
@simonpasquier As far as I can tell, logger is only nil for tests, right? I would prefer to create a proper logger in the test code and pass it to NewInhibitor instead of bloating the real code here. What do you think?
There was a problem hiding this comment.
logger is only nil for tests
yes. Agree with your recommendation.
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
simonpasquier
force-pushed
the
fix-inhibition-cache
branch
from
61e27c0 to
727cc14
Compare
|
fixed in #1331 |
* Add nvme_metrics.sh text collector example Signed-off-by: Henk <henk@wearespindle.com>
4 participants
@stuartnelson3 this is the fix for #1153.
I've added another acceptance scenario on inhibition where inhibiting and inhibited alerts fire and resolve together.