Add multiple sources for inhibition rules #4712
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Coleen Iona Quadros <coleen.quadros27@gmail.com>
Signed-off-by: Coleen Iona Quadros <coleen.quadros27@gmail.com>
coleenquadros
force-pushed
the
add_multiple_sources
branch
from
068f019 to
4d97c9a
Compare
Contributor
|
This sounds like it may need some documentation changes as well, so people know it exists and to use it? |
Don-Sanvura
approved these changes
Nov 7, 2025
Contributor
|
I closed #4504 since same result could be achieved using regex matching. |
Contributor
|
Could you run the benchmarks and also add new benchmarks with multiple source matchers? |
Author
|
Signed-off-by: Coleen Iona Quadros <coleen.quadros27@gmail.com>
Contributor
|
Could you run the benchmarks before and after, and then submit rather the output of benchstat, please? Otherwise it's a bit hard to compare... |
Author
|
@ultrotter can you help me understand what do you mean by before and after? The multiple sources feature is implemented for the first time. So I am not sure what we need to compare? |
Contributor
@coleenquadros Please follow these steps:
|
Contributor
|
The idea is to compare the benchmarks as ran without the patch applied, with the benchmarks ran after you applied it, to make sure the current use cases/code paths are not negatively affected by the feature as it's implemented. So something like: Thanks! |
siavashs
reviewed
Nov 11, 2025
Contributor
siavashs
left a comment
siavashs
left a comment
There was a problem hiding this comment.
Left some comments for now, will do another review after these are discussed.
config/config.go
Outdated
| SourceMatchRE MatchRegexps `yaml:"source_match_re,omitempty" json:"source_match_re,omitempty"` | ||
| // SourceMatchers defines a set of label matchers that have to be fulfilled for source alerts. | ||
| SourceMatchers Matchers `yaml:"source_matchers,omitempty" json:"source_matchers,omitempty"` | ||
| // Sources defines a set of source matchers and equal labels. |
Contributor
There was a problem hiding this comment.
The docs are incomplete and missing a lot of context, for example:
- it allows an inhibition rule to match multiple source alert
- the fact that it uses an AND operator to match multiple sources
- or that this will override
SourceMatchers - etc.
Also we should think about if this should become the new default and we deprecate SourceMatchers so a rule can have one or more of this new source matcher, if more than one all must match. You get the idea.
I'm interested to know what others think since we have already another deprecated config here, which would add more deprecations.
Maybe it's time for InhibitRule version 2 (or versioned config in general)?
Contributor
There was a problem hiding this comment.
cc @Spaceman1701 and @ultrotter since we discussed this in Slack briefly.
config/config.go
Outdated
| // SourceMatchers defines a set of label matchers that have to be fulfilled for source alerts. | ||
| SourceMatchers Matchers `yaml:"source_matchers,omitempty" json:"source_matchers,omitempty"` | ||
| // Sources defines a set of source matchers and equal labels. | ||
| Sources []Source `yaml:"source,omitempty" json:"source,omitempty"` |
Contributor
There was a problem hiding this comment.
The yaml/json naming is missing an s suffix:
Suggested change
| Sources []Source `yaml:"source,omitempty" json:"source,omitempty"` | |
| // Sources defines a set of source matchers and equal labels. | |
| Sources []Source `yaml:"sources,omitempty" json:"sources,omitempty"` |
I'm also not fully happy with this naming, but still think about alternatives:
AllSourceMatchersMultiSourceMatchers- etc.
Maybe other maintainers have a better idea how to name this thing.
Comment on lines
+93
to
+120
| if len(r.Sources) > 0 { | ||
| cached = 0 | ||
| indexed = 0 | ||
| for _, src := range r.Sources { | ||
| if src.SrcMatchers.Matches(a.Labels) { | ||
| if err := src.scache.Set(a); err != nil { | ||
| ih.logger.Error("error on set alert", "err", err) | ||
| continue | ||
| } | ||
| src.updateIndex(a) | ||
| cached += src.scache.Len() | ||
| indexed += src.sindex.Len() | ||
| break | ||
| } | ||
| } | ||
| r.updateIndex(a) | ||
|
|
||
| } | ||
| cached := r.scache.Len() | ||
| indexed := r.sindex.Len() | ||
| } else { | ||
| if r.SourceMatchers.Matches(a.Labels) { | ||
| if err := r.scache.Set(a); err != nil { | ||
| ih.logger.Error("error on set alert", "err", err) | ||
| continue | ||
| } | ||
| r.updateIndex(a) | ||
| } | ||
| cached = r.scache.Len() | ||
| indexed = r.sindex.Len() | ||
|
|
||
| } |
Contributor
There was a problem hiding this comment.
I think this logic is too big now and should move to Rule level as a method.
Author
There was a problem hiding this comment.
but if this multiple source way becomes the standard way to configure inhibition rules we dont need the else part right?
Contributor
There was a problem hiding this comment.
That is correct, there is another comment I left about versioning inhibit rules, so the structure will change.
We can discuss that first and then update the implementation based on the outcome.
Comment on lines
191
to
227
|
|
||
| if len(r.Sources) > 0 { | ||
| var inhibitorIDs []string | ||
| for _, source := range r.Sources { | ||
| if inhibitedByFP, eq := source.hasEqual(lset, source.SrcMatchers.Matches(lset), ruleStart, r.TargetMatchers); eq && !source.foundMatch { | ||
| inhibitorIDs = append(inhibitorIDs, inhibitedByFP.String()) | ||
| source.foundMatch = true | ||
| } else { | ||
| break | ||
| } | ||
| } | ||
| if allSourcesMatched := r.allSourcesSatisfied(); allSourcesMatched { | ||
| compositeInhibitorID := strings.Join(inhibitorIDs, ",") | ||
| ih.marker.SetInhibited(fp, compositeInhibitorID) | ||
| now := time.Now() | ||
| sinceStart := now.Sub(start) | ||
| sinceRuleStart := now.Sub(ruleStart) | ||
| ih.metrics.mutesDurationMuted.Observe(sinceStart.Seconds()) | ||
| r.metrics.mutesDurationMuted.Observe(sinceRuleStart.Seconds()) | ||
| return true | ||
| } | ||
| // Reset for next use. | ||
| for _, source := range r.Sources { | ||
| source.foundMatch = false | ||
| } | ||
|
|
||
| } else { | ||
| if inhibitedByFP, eq := r.hasEqual(lset, r.SourceMatchers.Matches(lset), ruleStart); eq { | ||
| ih.marker.SetInhibited(fp, inhibitedByFP.String()) | ||
| now := time.Now() | ||
| sinceStart := now.Sub(start) | ||
| sinceRuleStart := now.Sub(ruleStart) | ||
| ih.metrics.mutesDurationMuted.Observe(sinceStart.Seconds()) | ||
| r.metrics.mutesDurationMuted.Observe(sinceRuleStart.Seconds()) | ||
| return true | ||
| } | ||
|
|
| // Overwrites only happen if the new source alert has bigger EndsAt value. | ||
| sindex *index | ||
|
|
||
| foundMatch bool |
Contributor
There was a problem hiding this comment.
Why do we need this, can you elaborate?
Author
There was a problem hiding this comment.
the foundMatch is a safeguard when calling the Mutes function for a label set so that the label set is not matched against every source in a rule everytime if there was already a previous match source.hasEqual(lset, source.SrcMatchers.Matches(lset), ruleStart, r.TargetMatchers); eq && !source.foundMatch
Contributor
There was a problem hiding this comment.
I might not understand the logic fully but how the boolean stored on this a valid result for all the different source and target alerts hitting the rule?
What is "the match" we have found here?
Signed-off-by: Coleen Iona Quadros <coleen.quadros27@gmail.com>
coleenquadros
force-pushed
the
add_multiple_sources
branch
from
9cebf6e to
1a37070
Compare
Author
|
updated benchstat |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#4504
benchmark