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

Support UTF-8 label matchers: Add compat package with feature flag and use in amtool #3483

Merged

Conversation

grobinson-grafana
Copy link
Contributor

@grobinson-grafana grobinson-grafana commented Aug 24, 2023

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.

@grobinson-grafana grobinson-grafana force-pushed the grobinson/add-adapter-for-feature-flag branch from fddae71 to 48cefee Compare August 24, 2023 06:07
@grobinson-grafana grobinson-grafana changed the title Add adapter package for parser feature flag Support UTF-8 label matchers: Add adapter package for parser feature flag Sep 5, 2023
@grobinson-grafana grobinson-grafana force-pushed the grobinson/add-adapter-for-feature-flag branch 2 times, most recently from 819c322 to f2c8129 Compare September 6, 2023 20:18
@grobinson-grafana
Copy link
Contributor Author

I was thinking of adding the actual feature flag in a follow up PR to keep this one small and easier to review @gotjosh.

cli/root.go Outdated Show resolved Hide resolved
featurecontrol/featurecontrol.go Outdated Show resolved Hide resolved
Copy link
Member

@gotjosh gotjosh left a 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

matchers/adapter/parse.go Outdated Show resolved Hide resolved
cli/root.go Show resolved Hide resolved
cli/root.go Outdated Show resolved Hide resolved
featurecontrol/featurecontrol.go Outdated Show resolved Hide resolved
cli/root.go Outdated Show resolved Hide resolved
matchers/adapter/parse.go Outdated Show resolved Hide resolved
@grobinson-grafana
Copy link
Contributor Author

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 log.NewNopLogger()! 😄

@grobinson-grafana grobinson-grafana changed the title Support UTF-8 label matchers: Add adapter package for parser feature flag Support UTF-8 label matchers: Add compat package for parser feature flag Sep 12, 2023
@grobinson-grafana grobinson-grafana force-pushed the grobinson/add-adapter-for-feature-flag branch from bfed6d8 to 5576e19 Compare September 12, 2023 10:16
@grobinson-grafana
Copy link
Contributor Author

@gotjosh I forgot to mention that #3507 must be merged before this PR, as the test I'm introducing in #3507 is supposed to fail on this branch! I will make this clear in both PRs.

@grobinson-grafana
Copy link
Contributor Author

Just rebased main to include #3507 and make test fails as expected 🥳 :

--- FAIL: TestQuerySilence (0.14s)
    cli_test.go:187:
        	Error Trace:	/Users/grobinson/go/src/github.com/prometheus/alertmanager/test/cli/acceptance/cli_test.go:187
        	Error:      	Not equal:
        	            	expected: []string{"alertname=\"{alertname=test3}\"", "severity=\"warn\""}
        	            	actual  : []string{"alertname=\"test3\"", "severity=\"warn\""}

        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,3 +1,3 @@
        	            	 ([]string) (len=2) {
        	            	- (string) (len=29) "alertname=\"{alertname=test3}\"",
        	            	+ (string) (len=17) "alertname=\"test3\"",
        	            	  (string) (len=15) "severity=\"warn\""
        	Test:       	TestQuerySilence
FAIL

I'll now work on making this test pass, which means adding a check for { and } to the relevant commands.

@grobinson-grafana grobinson-grafana force-pushed the grobinson/add-adapter-for-feature-flag branch from 56a2a1f to 83e4702 Compare September 12, 2023 16:34
@grobinson-grafana grobinson-grafana changed the title Support UTF-8 label matchers: Add compat package for parser feature flag Support UTF-8 label matchers: Add compat package with feature flag and use in amtool Sep 20, 2023
cli/alert_add.go Show resolved Hide resolved
cli/alert_query.go Show resolved Hide resolved
cli/root.go Show resolved Hide resolved
featurecontrol/featurecontrol.go Outdated Show resolved Hide resolved
featurecontrol/featurecontrol.go Outdated Show resolved Hide resolved
featurecontrol/featurecontrol.go Outdated Show resolved Hide resolved
matchers/compat/parse.go Outdated Show resolved Hide resolved
@grobinson-grafana
Copy link
Contributor Author

Examples of amtool:

Stable matchers parsing

Here we are creating two alerts, and their respective silences, in amtool using foo and alertname=bar baz using the stable matchers parsing feature flag (pkg/labels):

./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 "alertname=bar baz" the alerts and silences come out as alertname="alertname=bar baz" because of the heuristic that attempts to detect the first label as the alertname. This is different for the strict and fallback parsers which create the alert and silence as alertname="bar baz" instead of alertname="alertname=bar baz".

./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

Copy link
Member

@gotjosh gotjosh left a 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)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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)

Copy link
Contributor Author

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.

cli/utils.go Show resolved Hide resolved
cli/utils.go Show resolved Hide resolved
cli/utils.go Outdated Show resolved Hide resolved

// 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) {
Copy link
Member

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

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

matchers/compat/parse.go Outdated Show resolved Hide resolved
matchers/compat/parse.go Outdated Show resolved Hide resolved
matchers/compat/parse.go Outdated Show resolved Hide resolved
matchers/compat/parse.go Outdated Show resolved Hide resolved
matchers/compat/parse.go Outdated Show resolved Hide resolved
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>
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>
@grobinson-grafana grobinson-grafana force-pushed the grobinson/add-adapter-for-feature-flag branch from 5de92f1 to 5f49101 Compare September 25, 2023 11:24
@grobinson-grafana
Copy link
Contributor Author

I had to force push to fix the conflict in cli/utils.go.

Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: George Robinson <george.robinson@grafana.com>
Copy link
Member

@gotjosh gotjosh left a 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) {
Copy link
Member

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?

@gotjosh gotjosh merged commit 16aa996 into prometheus:main Oct 19, 2023
4 checks passed
alexweav pushed a commit to grafana/prometheus-alertmanager that referenced this pull request Oct 27, 2023
…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>
@grobinson-grafana grobinson-grafana deleted the grobinson/add-adapter-for-feature-flag branch April 16, 2024 14:44
SuperQ added a commit that referenced this pull request Oct 16, 2024
* [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>
@SuperQ SuperQ mentioned this pull request Oct 16, 2024
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.

2 participants