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

Add negative matchers for routing. #2434

Merged
merged 41 commits into from
Jan 15, 2021
Merged

Conversation

asquare14
Copy link
Contributor

@asquare14 asquare14 commented Dec 14, 2020

Signed-off-by: aSquare14 atibhi.a@gmail.com
Related issue : #1023
Note : I am adding negative matchers for routing in this PR. Will add support for inhibition rules in next PR.

Signed-off-by: aSquare14 <atibhi.a@gmail.com>
@asquare14 asquare14 changed the title Add negative matchers for routing and inhibition Add negative matchers for routing and inhibition [WIP] Dec 14, 2020
@beorn7 beorn7 marked this pull request as draft December 14, 2020 14:50
@beorn7 beorn7 self-requested a review December 14, 2020 18:23
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
@asquare14 asquare14 changed the title Add negative matchers for routing and inhibition [WIP] Add negative matchers for routing. Dec 22, 2020
@asquare14 asquare14 marked this pull request as ready for review December 22, 2020 06:37
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

Big picture: In principle, things should work as you have done here, but I think we should go one step further and fully embrace the labels.Matcher for matchers in the config (but not (yet) for matchers in silences, see comments for details). We can talk 1:1 about it if you have questions. (I have found a few issues with the labels.Matcher while looking at the code, which we can also discuss in person.)

"Operational" note: In this PR, you effectively ignore a parsing error twice. It's a good general rule for Go code to only ignore errors if you know exactly why, and then add a comment explaining why it is fine to ignore the error. (Spoiler: In these two cases, it's not fine…)

config/config.go Outdated Show resolved Hide resolved
types/match.go Outdated Show resolved Hide resolved
types/match.go Outdated Show resolved Hide resolved
dispatch/route.go Outdated Show resolved Hide resolved
types/match.go Outdated Show resolved Hide resolved
The alert was just looking at the minimum across integrations. So a
complete failure of one integration would be masked by a still worknig
other integration. With this fix, the `integration` label is retained
(as it was already expected by the `description`), and thus any
failing integration will trigger the alert.

In addition, an `alertmanagerCriticalIntegrationsRegEx` is provided
that allows to mark integrations as critical. Integrations that are
not used to deliver critical alerts, or those that are just there for
auditing and logging purposes can now be configured to only trigger a
warning alert if they fail.

Signed-off-by: beorn7 <beorn@grafana.com>
The doc comments do not describe the current (arguably buggy) state,
but the desired state, as it will be implemented in future commits.

Signed-off-by: beorn7 <beorn@grafana.com>
This addresses a number of issues:

- It was impossible to include a literal '"' or a line break in the value.
- It was impossible to include '=', '~', or '!' in the value.
- It was not validated if the label name is valid.
- It was not validated if the value is valid UTF-8.
- No whitespace was allowed around the operator.

Signed-off-by: beorn7 <beorn@grafana.com>
Previously, escaped double quotes would still be seen as ending the
quoting, potentially leading to wronk tokenization.

Signed-off-by: beorn7 <beorn@grafana.com>
Signed-off-by: beorn7 <beorn@grafana.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

Looks great in general, just minor things to tweak.

Keep in mind that we also need to update the documentation, but we can do that after the next PR that updates the inhibition rules.

config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
types/match.go Outdated Show resolved Hide resolved
dispatch/route.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
dispatch/route_test.go Show resolved Hide resolved
@beorn7 beorn7 marked this pull request as ready for review January 12, 2021 12:50
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

I found an actual issue after all, see comments.

pkg/labels/matcher.go Outdated Show resolved Hide resolved
pkg/labels/matcher.go Outdated Show resolved Hide resolved
pkg/labels/matcher.go Outdated Show resolved Hide resolved
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
@asquare14 asquare14 requested a review from beorn7 January 12, 2021 17:31
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

Almost there, just an issue with marshaling.

config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
dispatch/route_test.go Outdated Show resolved Hide resolved
beorn7 and others added 2 commits January 13, 2021 13:21
Now the string created will correctly parse back.

Signed-off-by: beorn7 <beorn@grafana.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
@asquare14 asquare14 requested a review from beorn7 January 13, 2021 12:53
pkg/labels/matcher.go Outdated Show resolved Hide resolved
@beorn7
Copy link
Member

beorn7 commented Jan 13, 2021

Cool. Besides that minor issue about sorting by Type also, I think we are done here.

@simonpasquier could you give this a final look? Note that ideally, this gets merged together with #2441, where I'd appreciate your review, too.

beorn7 and others added 2 commits January 13, 2021 18:49
Signed-off-by: beorn7 <beorn@grafana.com>
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
pkg/labels/matcher.go Outdated Show resolved Hide resolved
asquare14 and others added 2 commits January 15, 2021 13:01
Signed-off-by: aSquare14 <atibhi.a@gmail.com>
@beorn7
Copy link
Member

beorn7 commented Jan 15, 2021

In the same spirit as #2441, I'll merge this now.

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.

3 participants