Bugfix: quote label name in matchers when needed #14068
Conversation
When the label name of a matcher contains non-standard characters, like a dot, or starts with a digit, it should be quoted. If it's not quoted, then `VectorSelector.String()` isn't a valid PromQL. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
model/labels/matcher.go
Outdated
| const quote = 1 | ||
| const matcher = 2 |
There was a problem hiding this comment.
Maybe call these lenQuote and lenMatcher or something? I first thought this might be keys to different types of characters.
There was a problem hiding this comment.
It's a valid point, I'll do it in a follow up PR. Right now I removed pre-growing at all, as I don't have a benchmark for it and there's now a 1KB preallocated stack for building the matcher which covers all the benchmarks.
Let's merge this as is, and I can add a 4KB long matcher to the benchmarks in a follow up PR, and see the impact of growing.
model/labels/matcher.go
Outdated
| return fmt.Sprintf("%s%s%q", m.Name, m.Type, m.Value) | ||
| const quote = 1 | ||
| const matcher = 2 | ||
| // As we're not on go1.22 yet and we don't have the new fancy AvailableBuffer method on strings.Builder, |
There was a problem hiding this comment.
Given that v2.52 is already separated from the main branch, and Go is already at 1.22.3, I don't see a problem with requiring Go1.22 from now on. So If it helps with this PR, feel free to bump the requirement.
There was a problem hiding this comment.
In fact, the RC for v2.52 is already built with Go1.22.
There was a problem hiding this comment.
Turns out, it's the bytes.Buffer, not strings.Builder that has the AvailBuffer method. I copied the implementation from Labels.String() that uses it, with the preallocated array on stack. No need to bump the go version after all.
Also removed the growing until there's a benchmark for that. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
colega
force-pushed
the
quote-label-name-in-matchers-when-needed
branch
from
b0318da
to
b7b4355
Compare
|
@beorn7 this is ready to take another look. Thank you. Edit: I also updated the benchmarks in the description of the PR. |
grafana/mimir-prometheus@cf682ab Relevant PRs: prometheus/prometheus#14096 prometheus/prometheus#14068 prometheus/prometheus#13669 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
* Update mimir-prometheus to e5b85c151ba8 grafana/mimir-prometheus@e5b85c1 prometheus/prometheus#14103 prometheus/prometheus#14096 prometheus/prometheus#14068 prometheus/prometheus#13669 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
* Update mimir-prometheus to e5b85c151ba8 grafana/mimir-prometheus@e5b85c1 prometheus/prometheus#14103 prometheus/prometheus#14096 prometheus/prometheus#14068 prometheus/prometheus#13669 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> (cherry picked from commit 307567a)
* Update mimir-prometheus to e5b85c151ba8 grafana/mimir-prometheus@e5b85c1 prometheus/prometheus#14103 prometheus/prometheus#14096 prometheus/prometheus#14068 prometheus/prometheus#13669 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> (cherry picked from commit 307567a) Co-authored-by: George Krajcsovits <krajorama@users.noreply.github.com>
Reverting prometheus/prometheus#14068 because it is causing unforseen complications with LBAC in GEM. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Reverting prometheus/prometheus#14068 because it is causing unforseen complications with LBAC in GEM. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Yet another issue I discovered fuzzying.
{"0"="0"}is now a valid parseable vector selector, butparser.VectorSelector.String()of that is{0="0"}, which is not.This fixes the
Matcher.String()method to quote the name when needed.Obviously, when needed came at a cost, so I also optimized
Match.String()getting rid of the expensivefmt.Sprintf()and making the net cost of the method cheaper in most cases.(Updated benchmarks)