-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Support UTF-8 label matchers: Add compat package with feature flag and use in amtool #3483
Support UTF-8 label matchers: Add compat package with feature flag and use in amtool #3483
Conversation
fddae71
to
48cefee
Compare
819c322
to
f2c8129
Compare
I was thinking of adding the actual feature flag in a follow up PR to keep this one small and easier to review @gotjosh. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I have a few suggestions, let me know what you think.
No matter what I tried, I couldn't get the message about the Disabled new label matchers
to show up. Can you give me an example command that would make the show up?
I tried:
./amtool --enable-feature="disable-new-label-matchers" --alertmanager.url=http://localhost:9093/ alert query
Ah I was using |
bfed6d8
to
5576e19
Compare
Just rebased
I'll now work on making this test pass, which means adding a check for |
56a2a1f
to
83e4702
Compare
Examples of amtool: Stable matchers parsing Here we are creating two alerts, and their respective silences, in amtool using ./amtool --alertmanager.url=http://127.0.0.1:9093 --enable-feature="stable-matchers-parsing" alert add foo
level=warn msg="Enabled stable matchers parsing"
./amtool --alertmanager.url=http://127.0.0.1:9093 --enable-feature="stable-matchers-parsing" alert query
level=warn msg="Enabled stable matchers parsing"
Alertname Starts At Summary State
foo 2023-09-21 10:05:49 +0100 active
./amtool --alertmanager.url=http://127.0.0.1:9093 --enable-feature="stable-matchers-parsing" alert add "alertname=bar baz"
level=warn msg="Enabled stable matchers parsing"
./amtool --alertmanager.url=http://127.0.0.1:9093 --enable-feature="stable-matchers-parsing" alert query
level=warn msg="Enabled stable matchers parsing"
Alertname Starts At Summary State
bar baz 2023-09-21 10:08:54 +0100 active ./amtool --alertmanager.url=http://127.0.0.1:9093 --enable-feature="stable-matchers-parsing" silence add foo --comment="silence alertname=foo"
level=warn msg="Enabled stable matchers parsing"
93ab0290-cd7a-44f7-8f22-6fcf1cd7c58e
./amtool --alertmanager.url=http://127.0.0.1:9093 --enable-feature="stable-matchers-parsing" silence query
level=warn msg="Enabled stable matchers parsing"
ID Matchers Ends At Created By Comment
93ab0290-cd7a-44f7-8f22-6fcf1cd7c58e alertname="foo" 2023-09-21 10:07:02 UTC grobinson silence alertname=foo
./amtool --alertmanager.url=http://127.0.0.1:9093 --enable-feature="stable-matchers-parsing" silence add "alertname=bar baz" -c="silence alertname=bar baz"
level=warn msg="Enabled stable matchers parsing"
58ae7810-6f5a-428d-be72-d4e3c6da5722
./amtool --alertmanager.url=http://127.0.0.1:9093 --enable-feature="stable-matchers-parsing" silence query
level=warn msg="Enabled stable matchers parsing"
ID Matchers Ends At Created By Comment
58ae7810-6f5a-428d-be72-d4e3c6da5722 alertname="bar baz" 2023-09-21 10:10:09 UTC grobinson =silence alertname=bar baz UTF-8 matchers parsing Here we are creating the same alerts as above but this time using the UTF-8 matchers parsing feature flag. You'll notice that when we use ./amtool --alertmanager.url=http://127.0.0.1:9093 --enable-feature="utf8-matchers-parsing" alert add foo
level=warn msg="Enabled UTF-8 matchers parsing"
./amtool --alertmanager.url=http://127.0.0.1:9093 --enable-feature="utf8-matchers-parsing" alert query
level=warn msg="Enabled UTF-8 matchers parsing"
Alertname Starts At Summary State
foo 2023-09-21 10:11:34 +0100 active
./amtool --alertmanager.url=http://127.0.0.1:9093 --enable-feature="utf8-matchers-parsing" alert add "alertname=bar baz"
level=warn msg="Enabled UTF-8 matchers parsing"
./amtool --alertmanager.url=http://127.0.0.1:9093 --enable-feature="utf8-matchers-parsing" alert query
level=warn msg="Enabled UTF-8 matchers parsing"
Alertname Starts At Summary State
alertname=bar baz 2023-09-21 10:19:14 +0100 active ./amtool --alertmanager.url=http://127.0.0.1:9093 --enable-feature="utf8-matchers-parsing" silence add foo -c="silence alertname=foo"
level=warn msg="Enabled UTF-8 matchers parsing"
7b5a593d-416c-4561-a5d7-a50ca71f0864
./amtool --alertmanager.url=http://127.0.0.1:9093 --enable-feature="utf8-matchers-parsing" silence query
level=warn msg="Enabled UTF-8 matchers parsing"
ID Matchers Ends At Created By Comment
7b5a593d-416c-4561-a5d7-a50ca71f0864 alertname="foo" 2023-09-21 10:20:33 UTC grobinson =silence alertname=foo
./amtool --alertmanager.url=http://127.0.0.1:9093 --enable-feature="utf8-matchers-parsing" silence add "alertname=bar baz" -c="silence alertname=bar baz"
level=warn msg="Enabled UTF-8 matchers parsing"
534cc784-913e-4f5c-90ac-9d05bd127034
./amtool --alertmanager.url=http://127.0.0.1:9093 --enable-feature="utf8-matchers-parsing" silence query
level=warn msg="Enabled UTF-8 matchers parsing"
ID Matchers Ends At Created By Comment
534cc784-913e-4f5c-90ac-9d05bd127034 alertname="alertname=bar baz" 2023-09-21 10:21:11 UTC grobinson =silence alertname=foo Fallback Here we are creating the same alerts as above but this time using the fallback parser (i.e. the default). The error message comes out twice because amtool parses the invalid input twice. I'm not sure if this is an issue or not. ./amtool --alertmanager.url=http://127.0.0.1:9093 alert add foo
./amtool --alertmanager.url=http://127.0.0.1:9093 alert query
Alertname Starts At Summary State
foo 2023-09-21 10:27:11 +0100 active
./amtool --alertmanager.url=http://127.0.0.1:9093 alert add "alertname=bar baz"
level=warn msg="Alertmanager is moving to a new parser for label matchers, and this input is incompatible. Alertmanager has instead parsed the input using the old matchers parser as a fallback. To make this input compatible with the new parser please make sure all regular expressions and values are double-quoted. If you are still seeing this message please open an issue." input="alertname=bar baz" err="14:17: unexpected bar: expected a comma or close brace" suggestion="alertname=\"bar baz\""
level=warn msg="Alertmanager is moving to a new parser for label matchers, and this input is incompatible. Alertmanager has instead parsed the input using the old matchers parser as a fallback. To make this input compatible with the new parser please make sure all regular expressions and values are double-quoted. If you are still seeing this message please open an issue." input="alertname=bar baz" err="14:17: unexpected bar: expected a comma or close brace" suggestion="alertname=\"bar baz\""
./amtool --alertmanager.url=http://127.0.0.1:9093 alert query
Alertname Starts At Summary State
bar baz 2023-09-21 10:27:37 +0100 active ./amtool --alertmanager.url=http://127.0.0.1:9093 silence add foo -c="silence alertname=foo"
cbe74cb0-0c57-4317-9955-a1130b45fb5b
./amtool --alertmanager.url=http://127.0.0.1:9093 silence query
ID Matchers Ends At Created By Comment
cbe74cb0-0c57-4317-9955-a1130b45fb5b alertname="foo" 2023-09-21 10:32:28 UTC grobinson =silence alertname=foo
./amtool --alertmanager.url=http://127.0.0.1:9093 silence add "alertname=bar baz" -c="silence alertname=bar baz"
level=warn msg="Alertmanager is moving to a new parser for label matchers, and this input is incompatible. Alertmanager has instead parsed the input using the old matchers parser as a fallback. To make this input compatible with the new parser please make sure all regular expressions and values are double-quoted. If you are still seeing this message please open an issue." input="alertname=bar baz" err="14:17: unexpected baz: expected a comma or close brace" suggestion="alertname=\"bar baz\""
level=warn msg="Alertmanager is moving to a new parser for label matchers, and this input is incompatible. Alertmanager has instead parsed the input using the old matchers parser as a fallback. To make this input compatible with the new parser please make sure all regular expressions and values are double-quoted. If you are still seeing this message please open an issue." input="alertname=bar baz" err="14:17: unexpected baz: expected a comma or close brace" suggestion="alertname=\"bar baz\""
74f79844-e087-4667-b1f0-8128df37f283
./amtool --alertmanager.url=http://127.0.0.1:9093 silence query
ID Matchers Ends At Created By Comment
74f79844-e087-4667-b1f0-8128df37f283 alertname="bar baz" 2023-09-21 10:33:30 UTC grobinson =silence alertname=bar baz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far, but please take a look at my comments.
@@ -154,6 +175,8 @@ func Execute() { | |||
configureConfigCmd(app) | |||
configureTemplateCmd(app) | |||
|
|||
app.Action(initMatchersCompat) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app.Action(initMatchersCompat) | |
// Matchers parsing initialization happens as a kingpin.Action to guarantee this is run after Kingpin had computed the flags but before running any commands | |
app.Action(initMatchersCompat) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more or less documented in the doc comments for app.Action
:
Action callback to call when all values are populated and parsing is complete, but before any command, flag or argument actions.
If you still want it to be documented here I'm happy to make that change, but just wanted to share this information in case it changes your opinion.
|
||
// Matchers parses one or more matchers in the input string. It returns | ||
// an error if the input is invalid. | ||
func Matchers(s string) (labels.Matchers, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went on a hunt to try and understand why we weren't using this function. On that mission, I concluded that we should address the inconsistency I see between the direct use of the functions in the compat
package and utils.go
functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're still not using this function after that refactor - I believe we should remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be used in Alertmanager server, both in api/v1/api.go
and config/config.go
. I'm surprised it's not used in api/v2/
though, I need to look into that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any thoughts on this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be used in Alertmanager in config/config.go
and api/v1/api.go
. In APIv2 however there is a function called parseFilter
that parses individual matchers.
This commit adds the adapter package that will allow users to switch between the new matchers/parse parser and the old pkg/labels parser. The new matchers/parse parser uses a fallback mechanism where if the input cannot be parsed in the new parser it then attempts to use the old parser. If an input is parsed in the old parser but not the new parser then a warning log is emitted. Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: George Robinson <george.robinson@grafana.com>
5de92f1
to
5f49101
Compare
I had to force push to fix the conflict in |
Signed-off-by: George Robinson <george.robinson@grafana.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, any thoughts on my comment? I'll let this bake for a day or two in case @simonpasquier has any comments.
|
||
// Matchers parses one or more matchers in the input string. It returns | ||
// an error if the input is invalid. | ||
func Matchers(s string) (labels.Matchers, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any thoughts on this comment?
…d use in amtool (prometheus#3483) * Add adapter package for parser feature flag This commit adds the compat package allowing users to switch between the new matchers/parse parser and the old pkg/labels parser. The new matchers/parse parser uses a fallback mechanism where if the input cannot be parsed in the new parser it then attempts to use the old parser. If an input is parsed in the old parser but not the new parser, then a warning log is emitted. --------- Signed-off-by: George Robinson <george.robinson@grafana.com>
* [CHANGE] Deprecate and remove api/v1/ #2970 * [CHANGE] Remove unused feature flags #3676 * [CHANGE] Newlines in smtp password file are now ignored #3681 * [CHANGE] Change compat metrics to counters #3686 * [CHANGE] Do not register compat metrics in amtool #3713 * [CHANGE] Remove metrics from compat package #3714 * [CHANGE] Mark muted alerts #3793 * [FEATURE] Add metric for inhibit rules #3681 * [FEATURE] Support UTF-8 label matchers #3453, #3507, #3523, #3483, #3567, #3568, #3569, #3571, #3595, #3604, #3619, #3658, #3659, #3662, #3668, 3572 * [FEATURE] Add counter to track alerts dropped outside of time_intervals #3565 * [FEATURE] Add date and tz functions to templates #3812 * [FEATURE] Add limits for silences #3852 * [FEATURE] Add time helpers for templates #3863 * [FEATURE] Add auto GOMAXPROCS #3837 * [FEATURE] Add auto GOMEMLIMIT #3895 * [FEATURE] Add Jira receiver integration #3590 * [ENHANCEMENT] Add the receiver name to notification metrics #3045 * [ENHANCEMENT] Add the route ID to uuid #3372 * [ENHANCEMENT] Add duration to the notify success message #3559 * [ENHANCEMENT] Implement webhook_url_file for discord and msteams #3555 * [ENHANCEMENT] Add debug logs for muted alerts #3558 * [ENHANCEMENT] API: Allow the Silences API to use their own 400 response #3610 * [ENHANCEMENT] Add summary to msteams notification #3616 * [ENHANCEMENT] Add context reasons to notifications failed counter #3631 * [ENHANCEMENT] Add optional native histogram support to latency metrics #3737 * [ENHANCEMENT] Enable setting ThreadId for Telegram notifications #3638 * [ENHANCEMENT] Allow webex roomID from template #3801 * [BUGFIX] Add missing integrations to notify metrics #3480 * [BUGFIX] Add missing ttl in pushhover #3474 * [BUGFIX] Fix scheme required for webhook url in amtool #3409 * [BUGFIX] Remove duplicate integration from metrics #3516 * [BUGFIX] Reflect Discord's max length message limits #3597 * [BUGFIX] Fix nil error in warn logs about incompatible matchers #3683 * [BUGFIX] Fix a small number of inconsistencies in compat package logging #3718 * [BUGFIX] Fix log line in featurecontrol #3719 * [BUGFIX] Fix panic in acceptance tests #3592 * [BUGFIX] Fix flaky test TestClusterJoinAndReconnect/TestTLSConnection #3722 * [BUGFIX] Fix crash on errors when url_file is used #3800 * [BUGFIX] Fix race condition in dispatch.go #3826 * [BUGFIX] Fix race conditions in the memory alerts store #3648 * [BUGFIX] Hide config.SecretURL when the URL is incorrect. #3887 * [BUGFIX] Fix invalid silence causes incomplete updates #3898 * [BUGFIX] Fix leaking of Silences matcherCache entries #3930 * [BUGFIX] Close SMTP submission correctly to handle errors #4006 Signed-off-by: SuperQ <superq@gmail.com>
This pull request adds the
compat
package that will allow users to switch between the UTF-8 (matchers/parse
) parser and the classic (pkg/labels
) parser. The default parsing mode uses a fallback mechanism where if the input cannot be parsed in the UTF-8 parser it then attempts to use the classic parser. If an input can be parsed in the classic parser but not the UTF-8 parser then a warning log is emitted encouraging the user to update their configuration/use of amtool.To be reviewed after #3453, #3507 and #3523.