Support UTF-8 label matchers in Alertmanager

[grobinson-grafana]

Background

As explained in #3319, Alertmanager constrains label names to the characters ^[a-zA-Z_][a-zA-Z0-9_]*$ - the same as Prometheus.

However, because Alertmanager is such a well known and popular open source project it makes sense to use it for other kinds of alert generators too, and not just Prometheus. An example of this is Grafana, where Alertmanager is used to manage alerts created from both Prometheus and non-Prometheus like datasources, including Graphite, InfluxDB, SQL, Loki and more. The issue with this though is that these other datasources do not share the same constraints as Prometheus when it comes to label names, and so using them with Alertmanager requires some kind of normalization to ensure labels match the set of allowed characters before the alert can be sent to the Alertmanager.

What this pull request does

@yuri-tceretian has proposed adding support for UTF-8 characters in #3321 that updates the existing regular expression to support double quoted UTF-8 sequences as label names, while keeping unquoted label names the same.

In this pull request I wanted to propose a different option to where we would add a "simple" LL(1) parser to Alertmanager to parse matchers instead.

Motivation

The original motivation for writing this parser was to add support for matching label names containing . and spaces to grafana/grafana. However, about the same time I learned that Prometheus maintainers agreed to add support for UTF-8 labels in Alertmanager, and so I decided to further the work to see if it could be upstreamed to Alertmanager instead.

The original source code can be found at grobinson-grafana/matchers.

Supported grammar

This LL(1) parser in its current version is not 100% compatible with the existing regular expression, although it is close and can be modified if required. The grammar can be understood as follows:

<expr>        ::= "{" <sequence> "}" | <sequence>
<sequence>    ::= <matcher> | <sequence> "," <matcher>
<matcher>     ::= <label_name> <operator> <label_value>
<label_name>  ::= <ident> | <quoted>
<operator>    ::= "=" | "=~" | "!=" | "!~"
<label_value> ::= <ident> | <quoted>
<ident>       ::= /^[a-zA-Z_][a-zA-Z0-9_]*$/
<quoted>      ::= "\"" /.*/ "\""

Here are some examples of valid inputs:

{}
foo=bar
{foo=bar}
{foo!=bar}
{foo="bar"}
{foo=~"[a-zA-Z0-9]+"}
{"foo"!~"[0-9]+"}
{ "foo with spaces" = "bar with spaces" }
{foo="bar",bar="foo 🙂","baz"!=qux,qux!="baz 🙂"}

and some examples of invalid inputs:

=
foo
{
{foo
{foo=
{foo=bar
{foo=bar,
{foo=bar,}
{foo=bar 🙂}
{foo=[a-zA-Z0-9]+}
{foo with spaces=bar with spaces}

Breaking changes

#### Expressions must start and end with open and closing braces

All expressions must start and end with { and }, although this can be relaxed if required. For example foo=bar is not valid, it must be {foo=bar}.

#### Trailing commas are not permitted

Trailing commas are not permitted. For example {foo=bar,} is not valid, it must be {foo=bar}.

All non [a-zA-Z_:][a-zA-Z0-9_:]* values must be double quoted

The set of unquoted characters is now the same on both sides of the expression. In other words, both label names and label values without double quotes must match the regular expression [a-zA-Z_:][a-zA-Z0-9_:]*. For example {foo=!bar} is not valid, it must be {foo="!bar"}. In current versions of Alertmanager, unquoted label values can contain all UTF-8 code points with the exception of comma, such as {foo=!bar}.

There are two reasons for this:

1. It's no longer possible to write ambiguous matchers which I feel is something Alertmanager should fix. For example is {foo=~} equivalent to {foo="~"} or {foo=~""}?

2. If we restrict the =, !, ~ characters to double quotes we can keep the grammar LL(1). Without this restriction lookahead/backtrack is required to parse matchers such as {foo==~!=!~bar} which are valid in current versions of Alertmanager.

Errors

One of the goals with this LL(1) parser is to provide better error messages than what is possible using just a regular expression. For example:

{foo
0:4: end of input: expected an operator such as '=', '!=', '=~' or '!~'

{foo=bar
0:8: end of input: expected close paren

foo=bar}
0:8: }: expected opening paren

{foo=bar,,}
9:10: unexpected ,: expected a matcher or close paren after comma

{foo=bar 🙂}
9:13: 🙂: invalid input: expected comma or closing '}'

{foo with spaces=bar with spaces}
5:9: unexpected with: expected an operator such as '=', '!=', '=~' or '!~'

Benchmarks

I've also provided a number of benchmarks of both the LL(1) parser and regex parser that supports UTF-8. These can be found at grobinson-grafana/matchers-benchmarks. However, to run them go.mod must be updated to use the branch https://github.com/grafana/prometheus-alertmanager/tree/yuri-tceretian/utf-8-label-names here.

BenchmarkMatchersSimple, BenchmarkPrometheusSimple
{foo="bar"}

BenchmarkMatchersComplex, BenchmarkPrometheusComplex
{foo="bar",bar="foo 🙂","baz"!=qux,qux!="baz 🙂"}

BenchmarkMatchersRegexSimple, BenchmarkPrometheusRegexSimple
{foo=~"[a-zA-Z_:][a-zA-Z0-9_:]*"}

BenchmarkMatchersRegexComplex, BenchmarkPrometheusRegexComplex
{foo=~"[a-zA-Z_:][a-zA-Z0-9_:]*",bar=~"[a-zA-Z_:]","baz"!~"[a-zA-Z_:][a-zA-Z0-9_:]*",qux!~"[a-zA-Z_:]"}

go test -bench=. -benchmem
goos: darwin
goarch: arm64
pkg: github.com/grobinson-grafana/matchers-benchmarks
BenchmarkMatchersRegexSimple-8      	  488295	      2425 ns/op	    3248 B/op	      49 allocs/op
BenchmarkMatchersRegexComplex-8     	  138081	      9074 ns/op	   11448 B/op	     169 allocs/op
BenchmarkPrometheusRegexSimple-8    	  329244	      3496 ns/op	    3531 B/op	      58 allocs/op
BenchmarkPrometheusRegexComplex-8   	   95188	     12554 ns/op	   12619 B/op	     204 allocs/op
BenchmarkMatchersSimple-8           	 2888340	       414.9 ns/op	      56 B/op	       2 allocs/op
BenchmarkMatchersComplex-8          	  741590	      1628 ns/op	     248 B/op	       7 allocs/op
BenchmarkPrometheusSimple-8         	 1919209	       613.9 ns/op	     233 B/op	       8 allocs/op
BenchmarkPrometheusComplex-8        	  425430	      2803 ns/op	    1015 B/op	      31 allocs/op
PASS
ok  	github.com/grobinson-grafana/matchers-benchmarks	11.766s

[gotjosh]

Thank you @grobinson-grafana for such a thorough description.

Can you please clarify

This LR(1) parser in its current version is not 100% compatible with the existing regular expression, although it is close and can be modified as required.

What are those cases that would cause a breaking change?

[grobinson-grafana]

Thank you @grobinson-grafana for such a thorough description.

Can you please clarify

This LR(1) parser in its current version is not 100% compatible with the existing regular expression, although it is close and can be modified as required.

What are those cases that would cause a breaking change?

Those are in the Restrictions 🙂

The following restrictions contain known differences between this parser and the current regular expression parser. Most of these can be relaxed if required, although I think it would be best to keep them as much possible.

1. All expressions must start and end with { and }, although this could be relaxed if required
2. Trailing commas are not permitted

[grobinson-grafana]

Hi, all! 👋

1. I've renamed Restrictions to Breaking changes and added more information about the breaking changes in the description.
2. I would be interested in having a discussion on the possible consequences of making such backwards incompatible changes to Alertmanager. I think these backwards incompatible changes are positive changes to make to Alertmanager if we look at the big picture, but might be too disruptive in the immediate future without some kind of transition period.

[grobinson-grafana]

To add a little more information about ambiguous/inconsistent matchers, the current version of Alertmanager allows the following:

{foo=}} equivalent to {foo="}"}, but I think this should be an error
{{foo=} is an error because of two {{, unlike the above
{foo=~} could be either {foo=~""} or {foo="~"}, it's interpreted in current versions as {foo=~""}
{foo=,} is equivalent to {foo=""}, but I think should be an error as a comma with no value has a high likelihood of being human error
{foo=,,} is an error, unlike {foo=,} or {foo=}}
{foo= } is equivalent to {foo=""}
{foo= }b is equivalent to {foo="}b"}
{foo= b} and {foo=b } are equivalent to {foo="b"}, but {foo=b b} is equivalent to {foo="b b"}

The changes proposed here would also restrict all of the examples above as 1. trailing commas are rejected and 2. unquoted text must match the regular expression [a-zA-Z_:][a-zA-Z0-9_:]*.

[beorn7]

About the breaking changes:

• Very generally, since we are still on v0.x, we are technically allowed to do those, but we have to consider carefully how common the cases are and what will happen when users run into them. (My Alertmanager foo is rusty, but IIRC we persist all the matchers in a normalised way, so only new input would run into the breakage, which is very easily notable for the user. Worst case is they have some tooling to submit silences, and that suddenly breaks in the wrong moment.)
• More specifically: The current behavior of accepting trailing commas and other quoting characters (single quotes and backticks) comes from PromQL. Even allowing a single trailing comma, but not two trailing commas comes from PromQL. I would prefer to keep this consistency rather than dropping it.

[grobinson-grafana]

I think trailing commas and quoting characters (single quotes and backticks) can be supported without much work. Supporting unquoted =, =~, != and !~ would be problematic because its no longer LL(1) as =, =~, != and !~ can be either a comparison operator or a label value depending on where we are in the input.

[grobinson-grafana]

When I first started working on this about a month ago, I had intended to remove support for unquoted label values, but then later relaxed this to [a-zA-Z_:][a-zA-Z0-9_:]*. However, I've just looked at both Prometheus and the OpenMetrics specification, and it seems to me that in both cases regular expressions and literals must be double quoted:

labels = "{" [label *(COMMA label)] "}"
label = label-name EQ DQUOTE escaped-string DQUOTE

The question I have then is what was the motivation for supporting both double quoted and unquoted regular expressions and literals in Alertmanager when double quoting seems to be a requirement in both Prometheus and OpenMetrics specifications?

Knowing this, I would instead like to restrict the grammar further and require both regular expressions and literals to be double quoted as I had first intended. It's important to highlight that this would be a further breaking change, but it seems like a good one with respect to a.) compliance and b.) simplicity.

[beorn7]

You were looking at the text-based exposition formats (classic Prometheus and OM). I was talking about PromQL. PromQL is actually older than those text-based exposition formats, and while the latter had been inspired by the former, they are not the same. (For the sake of historical completeness: The first two exposition formats were protobuf-based (not needing any quoting) and JSON-based (following JSON quoting rules, but the JSON-based format disappeared early in the history of Prometheus).)

You might ask the question why PromQL decided to be liberal with quoting character, and why the later text-based exposition formats did not follow that lead, but it's fairly academic at this point.

The inspiration for the matchers in AM are the matchers in PromQL. (Note that the exposition formats do not have matchers.) And they followed the quoting rules of PromQL. I wouldn't deviate from that now because it would decrease consistency. (And I would not give the simplicity argument much weight because some people see it the other way, that it's simpler if you can use any quoting character.)

[beorn7]

Supporting unquoted =, =~, != and !~ would be problematic because its no longer LL(1) as =, =~, != and !~ can be either a comparison operator or a label value depending on where we are in the input.

I think that's a valid argument. (Just to clarify that my objection is only about removing the trailing comma and the alternative quote characters.)

[grobinson-grafana]

You were looking at the text-based exposition formats (classic Prometheus and OM). I was talking about PromQL.

I was looking at both PromQL and the exposition format - but while the exposition format requires regular expressions and literals to be double quoted, I see that PromQL also allows for single quotes and backticks. However, I didn't know that single quotes and backticks were also supported in PromQL when writing the original comment, and I apologize if the original comment was confusing! 🙂

PromQL is actually older than those text-based exposition formats, and while the latter had been inspired by the former, they are not the same. (For the sake of historical completeness: The first two exposition formats were protobuf-based (not needing any quoting) and JSON-based (following JSON quoting rules, but the JSON-based format disappeared early in the history of Prometheus).)

This is useful to know, thanks! 😊

You might ask the question why PromQL decided to be liberal with quoting character, and why the later text-based exposition formats did not follow that lead, but it's fairly academic at this point.

I see that PromQL is liberal with quoting in that single quotes, double quotes and backticks are supported. I think I am less concerned about that, but more concerned about supporting unquoted literals and regular expressions, as permitted in matchers for current versions of Alertmanager. For example {foo=bar} and {foo=~[a-zA-Z].*}.

The inspiration for the matchers in AM are the matchers in PromQL. (Note that the exposition formats do not have matchers.) And they followed the quoting rules of PromQL.

Does the exposition format also support single quotes and backticks, as I had understood it was just double quotes?

I wouldn't deviate from that now because it would decrease consistency. (And I would not give the simplicity argument much weight because some people see it the other way, that it's simpler if you can use any quoting character.)

I think we can change it to support both single quotes and backticks, however I don't think those are support in matchers for current versions of Alertmanager either? For example {foo=`bar`} is parsed as {foo="`bar`"} rather than {foo="bar"}.

[grobinson-grafana]

This morning I had a call with @gotjosh! We looked at both amtool and the UI for silences and found quite significant differences between them, and the tests in pkg/labels:

Example	UI	amtool	pkg/labels	This PR
foo=	❌	⚠️ (allowed in queries, but not silences)	✅	❌
foo=""	✅	⚠️ (allowed in queries, but not silences)	✅	✅
foo=bar	❌	✅	✅	✅
foo="bar"	✅	✅	✅	✅
foo="bar",	❌	⚠️ [~] foo="bar,"	✅	❌
foo='bar'	❌	✅	✅	❌
foo=`bar`	❌	❌	⚠️ [~] {foo="`bar`"}	❌
{foo=bar}	❌	⚠️ [~] {alertname="{foo=bar}"})	✅	✅
{foo=bar,}	❌	⚠️ [~] {alertname="{foo=bar,}"})	✅	❌
{foo="bar"}	❌	⚠️ [~] {alertname="{foo=bar}"})	✅	✅
{foo="bar",}	❌	⚠️ [~] {alertname="{foo=bar,}"})	✅	❌
foo=~[a-zA-Z]+	❌	✅	✅	❌
foo=~"[a-zA-Z]+"	✅	✅	✅	✅

[~] means equivalent to

The biggest surprise for me is learning that 1. { and } and 2. unquoted label values such as foo=bar are not permitted in Alertmanager UI when filtering alerts or creating silences. However, both are permitted in amtool. Given that the UI has restrictions on unquoted label values, I think we could do the same in amtool.

I do have pending changes where examples such as {foo=bar,} and {foo="bar",} would be permitted in this PR, but since those are not pushed I have not included them in the table.

[beorn7]

but while the exposition format requires regular expressions and literals to be double quoted...

Note that the exposition formats do not deal with regular expressions at all. The exposition format is just not a good reference for matcher syntax because it (a) does not contain matchers itself and (b) its syntax is inspired by PromQL syntax, but it is not the same and it is also solving a different problem.

I would vote for PromQL as the role model for matchers. That's what's had been done in the past, plus added tolerance around trying to "guess" what the user means (which does have the potential for trouble, see below).

however I don't think those are support in matchers for current versions of Alertmanager either?

Yes, totally possible. That's probably because I wasn't aware that backticks are allowed in PromQL when I coded the AM matchers.

In summary, I think we should definitely support valid PromQL matcher syntax in AM land (implying that we have to add backtick support).

And we may keep (or even add) some tolerance for "not quite correct" syntax, but we should avoid those parts that create trouble. (Which means I'm totally on board to require certain quoting where ambiguities arise, as you have explained above.) And we should have consistency in the level of tolerance between amtool and the AM UI (which isn't the case right now because the UI is less tolerant).

[grobinson-grafana]

I would vote for PromQL as the role model for matchers. That's what's had been done in the past, plus added tolerance around trying to "guess" what the user means (which does have the potential for trouble, see below).

That's good because it's what I was thinking too! It would mean that support for unquoted literals and regular expressions would be dropped, as those are not permitted in PromQL, and instead replaced with support for double quotes, single quotes and backticks.

In summary, I think we should definitely support valid PromQL matcher syntax in AM land (implying that we have to add backtick support).

I think also support for opening and closing parens in both the UI and amtool, as it doesn't work as expected in either of them (see table above).

And we may keep (or even add) some tolerance for "not quite correct" syntax, but we should avoid those parts that create trouble.

I would argue trailing commas are a good example of "not quite correct". The argument I have against trailing commas is that here commas indicate there is another matcher after this one, so either the user has forgotten to include it or appended additional text by mistake (which could be a comma). However, since it's also supported in PromQL we should support it here too! 🙂

[beorn7]

The trailing comma thing (in PromQL and from there propagating into AM matchers) was probably done with the intention of simplifying auto-generation of selectors. (You programmatically iterate through a list of labels and want to create a selector from it. If you now have to special case the last iteration to not render a comma, everything gets clunkier.) Many got burned by the absence of trailing commas in JSON (and jsonnet allowed them for that same reason, I presume).

[beorn7]

I think the tolerance of missing curly braces and quotes was really coming from amtool usage. Since both quotes and curly braces have special meaning in most shells, it's quite a challenge to correctly write {foo="bar"} on the command line, while foo=bar is straight forward. (But the former should still be allowed.)

It's ultimately the call of the AM maintainers, but I would like to see a moderate amount of tolerance being kept. (And I think that's what you propose with this PR.) We shouldn't allow syntax that requires backtracking in parsing, as you said above.

The UI being more strict is probably mostly a result that the shell quoting issue doesn't arrive there. And then the parsing is implemented again in the UI to give you validation even before you submit.

[beorn7]

As if we aren't juggling enough balls already, here's another thought:

We want to allow all of UTF-8 in label names as well, which means we need quoting of the label name in matchers in that case, too. Not sure if that needs to be taken into account here to avoid another invasive change later.

[grobinson-grafana]

This week I've been working on optional opening and closing braces and trailing commas so those will be supported too 🙂

but I would like to see a moderate amount of tolerance being kept. (And I think that's what you propose with this PR.) We shouldn't allow syntax that requires backtracking in parsing, as you said above.

Sounds good to me!

The UI being more strict is probably mostly a result that the shell quoting issue doesn't arrive there. And then the parsing is implemented again in the UI to give you validation even before you submit.

That makes sense to me, but I think at some point the UI and amtool should be made consistent (perhaps using the grammar in the description). However, that's not this PR.

We want to allow all of UTF-8 in label names as well, which means we need quoting of the label name in matchers in that case, too. Not sure if that needs to be taken into account here to avoid another invasive change later.

That's supported 🙂 However, like values, non [a-zA-Z_:][a-zA-Z0-9_:]* must be double quoted. We can add support for single quotes and backticks as both names and values are tokenized as TokenQuoted.

[grobinson-grafana]

I also want to highlight that foo=bar is permitted because both foo and bar match the regular expression [a-zA-Z_:][a-zA-Z0-9_:]*. However, foo=🙂 and foo=Σ are not, and must be written as foo="🙂" and foo="Σ".

[grobinson-grafana]

I've just updated this PR to add support for optional open and close parens and optional trailing commas as discussed with @beorn7.

[grobinson-grafana]

I've put together two utils that will assist the review of this pull request and also create tooling to help migrate incompatible matchers in user's their configuration files.

1. matchers-cli A simple command-line interface for Prometheus-like matchers
2. matchers-translate Translates incompatible Prometheus-like matchers

[grobinson-grafana]

Closing this due to size. Instead, @gotjosh and I are working together to add this work to Alertmanager in smaller iterations rather than one large PR. You can follow the work here.