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

grobinson-grafana · 2023-08-24T06:06:46Z

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 · 2023-09-07T07:48:57Z

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

matchers/adapter/parse.go

cli/root.go

featurecontrol/featurecontrol.go

gotjosh

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

cli/root.go

featurecontrol/featurecontrol.go

cli/root.go

matchers/adapter/parse.go

grobinson-grafana · 2023-09-11T10:32:13Z

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

featurecontrol/featurecontrol.go

grobinson-grafana · 2023-09-12T15:36:29Z

@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 · 2023-09-12T16:33:57Z

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.

cli/alert_add.go

cli/alert_query.go

cli/root.go

featurecontrol/featurecontrol.go

matchers/compat/parse.go

grobinson-grafana · 2023-09-21T09:35:57Z

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

featurecontrol/featurecontrol.go

gotjosh

Looks good so far, but please take a look at my comments.

gotjosh · 2023-09-22T08:15:43Z

cli/root.go

@@ -154,6 +175,8 @@ func Execute() {
 	configureConfigCmd(app)
 	configureTemplateCmd(app)

+	app.Action(initMatchersCompat)


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)

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

gotjosh · 2023-09-22T10:39:48Z

matchers/compat/parse.go

+
+// 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) {


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

If we're still not using this function after that refactor - I believe we should remove it.

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.

Any thoughts on this comment?

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

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>

grobinson-grafana · 2023-09-25T11:25:03Z

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

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

gotjosh

LGTM, any thoughts on my comment? I'll let this bake for a day or two in case @simonpasquier has any comments.

gotjosh · 2023-10-17T11:32:07Z

matchers/compat/parse.go

+
+// 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) {


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>

grobinson-grafana force-pushed the grobinson/add-adapter-for-feature-flag branch from fddae71 to 48cefee Compare August 24, 2023 06:07

grobinson-grafana mentioned this pull request Aug 24, 2023

Support UTF-8 names and values in label matchers #3486

Closed

4 tasks

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 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 commented Sep 7, 2023

View reviewed changes

matchers/adapter/parse.go Outdated Show resolved Hide resolved

grobinson-grafana commented Sep 8, 2023

View reviewed changes

cli/root.go Outdated Show resolved Hide resolved

featurecontrol/featurecontrol.go Outdated Show resolved Hide resolved

gotjosh requested changes Sep 8, 2023

View reviewed changes

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 force-pushed the grobinson/add-adapter-for-feature-flag branch from bfed6d8 to 5576e19 Compare September 12, 2023 10:16

grobinson-grafana commented Sep 12, 2023

View reviewed changes

featurecontrol/featurecontrol.go Outdated Show resolved Hide resolved

grobinson-grafana mentioned this pull request Sep 12, 2023

Support UTF-8 label matchers: Add test for PromQL braces when parsing lists of matchers #3507

Merged

grobinson-grafana force-pushed the grobinson/add-adapter-for-feature-flag branch from 56a2a1f to 83e4702 Compare September 12, 2023 16:34

grobinson-grafana mentioned this pull request Sep 20, 2023

Support UTF-8 label matchers: Add more acceptance tests for braces when using amtool #3523

Merged

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

grobinson-grafana commented Sep 20, 2023

View reviewed changes

grobinson-grafana commented Sep 21, 2023

View reviewed changes

featurecontrol/featurecontrol.go Outdated Show resolved Hide resolved

gotjosh reviewed Sep 22, 2023

View reviewed changes

grobinson-grafana mentioned this pull request Sep 23, 2023

Remove unused function GetAlertmanagerURL #3535

Merged

grobinson-grafana added 6 commits September 25, 2023 12:24

Fix lint

2c311db

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

Add tests

5fde3a0

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

Fix missing license

7516580

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

Fix lint

075bd06

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

Use adapter in amtool

f958675

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

grobinson-grafana added 19 commits September 25, 2023 12:24

Create feature flag to disable new label matchers

9282da4

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

Fix missing feature in AllowedFlags

8744682

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

Rename adapter to compat

cdab2c4

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

Fix lint

4ec38da

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

Add two option flag

86f0851

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

Fix lint

2278d78

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

Update warning log on fallback

846a933

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

Use same error when parsing matchers plural

6e76633

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

Fix TestQuerySilence test

d2e0b07

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

Add check for braces to parseLabels

b1f8e9c

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

Remove extra : in error message

0fa923e

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

Add verbose logging and suggestions to warning logs

c8691b4

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

Fix check for braces in wrong place

794361d

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

Fix incorrect parser used

5371e4a

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

Put enabled at the end of the sentence

9dbd07d

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

Use single line when emitting logs

9543033

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

Move comment inside if

40d3e53

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

Update log line to include both labels and matchers

f85a75e

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

Rename stable to classic

1efe79b

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

grobinson-grafana force-pushed the grobinson/add-adapter-for-feature-flag branch from 5de92f1 to 5f49101 Compare September 25, 2023 11:24

grobinson-grafana added 2 commits September 25, 2023 12:25

Remove functions from utils.go

5f49101

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

Fix tests

ff1c279

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

grobinson-grafana requested a review from gotjosh October 16, 2023 10:38

gotjosh approved these changes Oct 17, 2023

View reviewed changes

gotjosh merged commit 16aa996 into prometheus:main Oct 19, 2023
8 checks passed

alanprot mentioned this pull request Apr 2, 2024

Chore: Update Prometheus cortexproject/cortex#5841

Merged

grobinson-grafana deleted the grobinson/add-adapter-for-feature-flag branch April 16, 2024 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

grobinson-grafana commented Aug 24, 2023 •

edited

grobinson-grafana commented Sep 7, 2023

gotjosh left a comment

grobinson-grafana commented Sep 11, 2023

grobinson-grafana commented Sep 12, 2023

grobinson-grafana commented Sep 12, 2023

grobinson-grafana commented Sep 21, 2023

gotjosh left a comment

gotjosh Sep 22, 2023

grobinson-grafana Sep 23, 2023

gotjosh Sep 22, 2023

gotjosh Sep 22, 2023

grobinson-grafana Sep 22, 2023

gotjosh Oct 17, 2023

grobinson-grafana Oct 17, 2023

grobinson-grafana commented Sep 25, 2023

gotjosh left a comment

gotjosh Oct 17, 2023

	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)

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

grobinson-grafana commented Aug 24, 2023 • edited

grobinson-grafana commented Sep 7, 2023

gotjosh left a comment

Choose a reason for hiding this comment

grobinson-grafana commented Sep 11, 2023

grobinson-grafana commented Sep 12, 2023

grobinson-grafana commented Sep 12, 2023

grobinson-grafana commented Sep 21, 2023

gotjosh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grobinson-grafana commented Sep 25, 2023

gotjosh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grobinson-grafana commented Aug 24, 2023 •

edited