UTF-8: Add partial support for parsing UTF-8 metric and label names by ywwg · Pull Request #13271 · prometheus/prometheus

ywwg · 2023-12-08T15:44:36Z

This PR adds support for the new grammar of {"metric_name", "l1"="val"} to promql and some of the exposition formats. This grammar will also be valid for non-UTF-8 names. UTF-8 names will not be considered valid unless model.NameValidationScheme is changed.

This PR does not update the go expfmt parser in text_parse.go, which will be addressed by prometheus/common#554.

Correction, this PR does update the promql parser.

Part of #13095

beorn7

Generally looks good. Mostly nitpicking in the comment and requesting more test cases.

I don't really feel I'm the right person to review this because I am not familiar at all with the generated parser. I might also ask dumb questions in my comments.

Let's try to find a reviewer that knows those things better.

beorn7 · 2024-02-08T14:33:28Z

model/textparse/openmetricslex.l

Warning: I'm not familiar with the lexer syntax. I'll just rubberstamp this one if the results look OK to me. Experts, please chime in in time to stop me.

beorn7 · 2024-02-08T14:45:27Z

model/textparse/openmetricsparse_test.go

+{"http.status",q="0.9",a="b"} 8.3835e-05
+{"http.status",q="0.9",a="b"} 8.3835e-05
+{q="0.9","http.status",a="b"} 8.3835e-05
+{"go.gc_duration_seconds_sum"} 0.004304266`


Maybe add a few test cases with "weirder" characters:

Multibyte characters like "ö" or "€".

Whitespace in the name.

An escaped " in the name.

{ and } as part of the name.

Also, what about quoted label names? Including label names with all the weird characters above.

oo, found some bugs, thanks :)

model/textparse/promparse.go

beorn7 · 2024-02-08T14:47:40Z

model/textparse/promlex.l

Same flag here as for the OpenMetrics lexer file: I don't really know the lexer syntax.

model/textparse/promparse.go

beorn7 · 2024-02-08T14:55:29Z

model/textparse/openmetricsparse.go

 	return val, nil
 }
+
+func (p *OpenMetricsParser) verifyMetricName(start, end int) error {


Same question here as below for the corresponding method in promparse.go.

Apparently you removed verifyMetricName from promparse.go, but left it in here. But this should go, too, shouldn't it?

model/textparse/promparse_test.go

scrape/scrape_test.go

beorn7 · 2024-02-08T15:21:09Z

I believe the Go tests should pass if you run go mod tidy.

beorn7 · 2024-02-08T15:28:58Z

This PR does not update PromQL parsing

It does now, doesn't it?

However, the frontend cod isn't updated yet, so while things work, the UI is complaining passive-aggressively with squiggly lines:

ywwg · 2024-02-08T18:14:51Z

It does now, doesn't it?

fixed

ywwg · 2024-02-08T18:17:35Z

yes, another parser that will have to be fixed is the typescript one -- there is a task for that but I will create a proper issue to raise visibilitiy

darshanime · 2024-02-10T05:10:46Z

promql/parser/generated_parser.y

 label_matcher   : IDENTIFIER match_op STRING
-                        { $$ = yylex.(*parser).newLabelMatcher($1, $2, $3);  }
+                        { $$ = yylex.(*parser).newLabelMatcher($1, $2, $3); }
+                | string_identifier match_op STRING


do we need string_identifier match_op error rule? or is the parser error useful without it already?

would that be like
{"x"=} ?

ok yes, adding that makes the error a little better

ywwg · 2024-02-13T13:38:16Z

test failure is I think unrelated but I'll dig in

ywwg · 2024-02-13T16:25:08Z

Hm this test failure is a bad sign -- there have been multiple places where people are testing the value of the negotiated format against the string constants. This is Bad because there are now many different ways the formats can look and be equivalent. How do we prevent people from doing these bad string comparisons? make the string constants non-exported?

ywwg · 2024-02-13T18:39:32Z

fuzz error looks concerning, I'm trying to figure out how to run that test myself

ywwg · 2024-02-14T19:12:13Z

I was able to debug the fuzz error, fixing

ywwg · 2024-02-14T19:33:15Z

got it

beorn7

Looks good now with fixed fuzzing and extended tests, as far as I can say.

See comments for minor things, otherwise I'd say this is good to go. Last chance for others to chime in!

beorn7 · 2024-02-15T14:04:46Z

go.mod

 	github.com/prometheus/client_golang v1.18.0
 	github.com/prometheus/client_model v0.5.0
-	github.com/prometheus/common v0.46.0
+	github.com/prometheus/common v0.46.1-0.20240206181200-773d5664eb8d


Let's not pin to specific commits here. I have just tagged 0.47.0 in common, so you can use it now.

I don't see the tag on github, do you need to push it?

I have pushed it an hour ago, and even created a nice release.

Maybe Github is once more very eventually consistent?
https://github.com/prometheus/common/releases/tag/0.47.0

beorn7 · 2024-02-15T14:09:48Z

model/textparse/openmetricsparse.go

 	return val, nil
 }
+
+func (p *OpenMetricsParser) verifyMetricName(start, end int) error {


Apparently you removed verifyMetricName from promparse.go, but left it in here. But this should go, too, shouldn't it?

ywwg · 2024-02-15T14:19:24Z

Apparently you removed verifyMetricName from promparse.go, but left it in here. But this should go, too, shouldn't it?

ah yes sorry

beorn7 · 2024-02-15T15:40:56Z

Thank you. I think you can do the squashing and DCO'ing now, and then we can merge (barring any new objections from others).

ywwg · 2024-02-15T19:34:19Z

promql/promql_test.go

-func TestEvaluations(t *testing.T) {
-	RunBuiltinTests(t, newTestEngine())
-}
+// func TestEvaluations(t *testing.T) {


This adds support for the new grammar of `{"metric_name", "l1"="val"}` to promql and some of the exposition formats. This grammar will also be valid for non-UTF-8 names. UTF-8 names will not be considered valid unless model.NameValidationScheme is changed. This does not update the go expfmt parser in text_parse.go, which will be addressed by prometheus/common#554. Part of prometheus#13095 Signed-off-by: Owen Williams <[email protected]>

beorn7 · 2024-02-16T10:43:06Z

🎉

ywwg force-pushed the owilliams/utf8 branch from 9d23ef3 to 64b3665 Compare December 8, 2023 15:45

ywwg added this to the OTEL Support milestone Dec 8, 2023

ywwg mentioned this pull request Feb 6, 2024

Epic: Implement UTF-8 character support for label and metric names #13095

Closed

40 tasks

ywwg changed the title ~~UTF-8: Add support for parsing UTF8 metric and label names~~ UTF-8: Add partial support for parsing UTF8 metric and label names Feb 6, 2024

ywwg marked this pull request as ready for review February 6, 2024 19:30

ywwg requested a review from roidelapluie as a code owner February 6, 2024 19:30

ywwg requested a review from beorn7 February 6, 2024 19:34

beorn7 requested changes Feb 8, 2024

View reviewed changes

ywwg changed the title ~~UTF-8: Add partial support for parsing UTF8 metric and label names~~ UTF-8: Add partial support for parsing UTF-8 metric and label names Feb 8, 2024

darshanime reviewed Feb 10, 2024

View reviewed changes

ywwg requested review from bwplotka, cstyan and tomwilkie as code owners February 14, 2024 19:35

beorn7 requested changes Feb 15, 2024

View reviewed changes

ywwg force-pushed the owilliams/utf8 branch from bb9fbcb to b6c5ac3 Compare February 15, 2024 19:27

ywwg commented Feb 15, 2024

View reviewed changes

ywwg force-pushed the owilliams/utf8 branch from b6c5ac3 to a28d786 Compare February 15, 2024 19:35

beorn7 approved these changes Feb 16, 2024

View reviewed changes

beorn7 merged commit ac10cd4 into prometheus:main Feb 16, 2024

beorn7 mentioned this pull request Feb 20, 2024

UTF-8: updates UI parser to support UTF-8 characters #13590

Merged

This was referenced Feb 23, 2024

Sync with latest upstream grafana/mimir-prometheus#601

Merged

Upgrade to latest mimir-prometheus@main grafana/mimir#7475

Merged

beorn7 mentioned this pull request Feb 28, 2024

promql: setting multiple regexp's for __name__ doesn't work anymore #13596

Closed

Conversation

ywwg commented Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beorn7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

beorn7 commented Feb 8, 2024

Uh oh!

beorn7 commented Feb 8, 2024

Uh oh!

ywwg commented Feb 8, 2024

Uh oh!

ywwg commented Feb 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywwg commented Feb 13, 2024

Uh oh!

ywwg commented Feb 13, 2024

Uh oh!

ywwg commented Feb 13, 2024

Uh oh!

ywwg commented Feb 14, 2024

Uh oh!

ywwg commented Feb 14, 2024

Uh oh!

beorn7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywwg commented Feb 15, 2024

Uh oh!

beorn7 commented Feb 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beorn7 commented Feb 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ywwg commented Dec 8, 2023 •

edited

Loading