Add support for utf8 names on /v1/label/:name/values endpoint by ywwg · Pull Request #15399 · prometheus/prometheus

ywwg · 2024-11-14T15:53:27Z

Follow-on from #14943

Previously, the api was evaluating this regex to determine if the label
name was valid or not:

https://github.com/prometheus/common/blob/14bac55a992f7b83ab9d147a041e274606bdb607/model/labels.go#L94

However, I believe that the IsValid() function is what ought to be
used in the brave new utf8 era.

Before

$ curl localhost:9090/api/v1/label/host.name/values
{"status":"error","errorType":"bad_data","error":"invalid label name: \"host.name\""}

After

$ curl localhost:9090/api/v1/label/host.name/values
{"status":"success","data":["localhost"]}

It's very likely that I'm missing something here or you were already
planning to do this at some point but I just encountered this issue and
figured I'd give it a go.

ywwg · 2024-11-14T18:02:19Z

windows error seems to be spurious

bwplotka

Nice, LGTM!

Should we do it in release-3.0 branch first (if yes, rebase on that branch), so we do 3.0.1? cc @jan--f

We need to put cap on how much we put into 3.0.x here. We will likely have multiple UTF-8 gaps.. I don't think it must go to 3.0.1, but I think it could (:

ywwg · 2024-11-18T15:13:13Z

@jan--f said to commit to main and then backport as necessary. I can set up that PR

beorn7

Looks good code-wise, but shouldn't there be an update to the documentation in https://github.com/prometheus/prometheus/blob/main/docs/querying/api.md ?

beorn7 · 2024-11-21T12:35:00Z

FTR: I'm not sure if this should really be in v3.0.1. I would see it more as a missing feature than a real bug. (And I would like to get back to a regular release cadence ASAP, so that v3.1.0 is not far down the road.)
But if we want to release this in v3.0.1, then let's first rebase this PR on top of the v3.0 release branch.

beorn7

Sorry for more comments. Documenting is hard…

In any case, before merging this, let's decide if this should go into v3.0.1 or v3.1.0, see other comment. (If need be, we need to rebase this on top of the release branch.)

beorn7 · 2024-11-21T12:40:47Z

 The `data` section of the JSON response is a list of string label values.

-This example queries for all label values for the `job` label:
+This example queries for all label values for the `job_name` label:


Sorry for nitpicking again, but let's not use a "weird" label name in the first example. job_name is a weird because it's so common to have a job label. Why would somebody have job_name? If your intention is to have a label name with an underscore here, let' use one that is actually common like http_status_code (which could then be http.status_code in the example below to even allude to the different meaning of . and _ in OTel).

beorn7 · 2024-11-21T12:43:26Z

+For label names that contain characters that are not valid in the legacy
+Prometheus character set (letters, numbers, underscores, and colons), they


Strictly speaking, you can use non-legacy characters here as long as they are valid as URL components, can't we?

So I guess http://localhost:9090/api/v1/label/job.name/values would actually work?

If that's true, we only need the U__ encoding for characters that don't work as URL components (in particular /).

I was thinking about that, but that might lead people to use HTML encoding: "job%20name" which is not supported. So while job.name would work, I'd prefer to make the reasoning simpler and just ask that people use encoding when the name is non-legacy-compatible.

By HTML encoding, you mean percent encoding, also known as URL encoding, don't you? I.e. https://en.wikipedia.org/wiki/Percent-encoding

I would expect that URL encoding is in fact supported, because that should just be dealt with by the Go HTTP libraries and the Prometheus code wouldn't even notice.

It might very well possible that we only need the U__ encoding for a / in the label name. (At least that was the case long ago for label values as they show up in URLs for pushing to the PGW. Of course, back then there was no U__ encoding, so we elected for base64.)

Sorry for beating this point for so long, but it feels confusing to refer to the legacy Prometheus character set here when there is no technical reason for it.

Assuming that my assumptions above are all correct, I would use the following flow of explanation:

Don't mention URL encoding at all, because it is (IMHO) implied that that always works (it's part of the HTTP standard, and tools like curl and browsers will use it implicitly anyway).

Just state that label names starting with U__ are interpreted as encoded according to the Values Escaping method, which can be used at will, but also is the only way to specify label names that contain a /.

I think I have rewritten it this way.

beorn7 · 2024-11-21T12:44:18Z

+
+For label names that contain characters that are not valid in the legacy
+Prometheus character set (letters, numbers, underscores, and colons), they
+should be escaped using the Values Escaping method:


While it's handy to have a short description here, do we have a comprehensive explanation of this encoding method that we can link to?

the prometheus proposal is the closest thing

@beorn7 I can file an issue for the doc issue, is that enough to unblock this PR?

Linking to the Prometheus proposal is fine. I.e. link to https://github.com/prometheus/proposals/blob/main/proposals/2023-08-21-utf8.md#text-escaping (as we did for PGW, just checked that out). No issue required.

jan--f · 2024-11-25T15:55:45Z

Like beorn7 mentioned let's target release-3.0 for a bug fix. Apologies if I added confusion and extra work with my previous statement to target main.

Previously, the api was evaluating this regex to determine if the label name was valid or not: https://github.com/prometheus/common/blob/14bac55a992f7b83ab9d147a041e274606bdb607/model/labels.go#L94 However, I believe that the `IsValid()` function is what ought to be used in the brave new utf8 era. **Before** ``` $ curl localhost:9090/api/v1/label/host.name/values {"status":"error","errorType":"bad_data","error":"invalid label name: \"host.name\""} ``` **After** ``` $ curl localhost:9090/api/v1/label/host.name/values {"status":"success","data":["localhost"]} ``` It's very likely that I'm missing something here or you were already planning to do this at some point but I just encountered this issue and figured I'd give it a go. Signed-off-by: Owen Williams <[email protected]>

ywwg · 2024-11-25T16:49:22Z

rebased onto release-3.0

ywwg · 2024-11-27T18:10:06Z

This should not block 3.0.1, but I believe it's ready to go in (pending getting the documentation right!)

beorn7

It's actually good to have this in v3.0.1 because it is technically a breaking change (legacy label names could have the form U__...). I don't think it's relevant enough to call it out in the migration guide, though.

jan--f

Could use a follow up, but shouldn't block this fix.

Signed-off-by: Owen Williams <[email protected]>

ywwg requested a review from roidelapluie as a code owner November 14, 2024 15:53

ywwg mentioned this pull request Nov 14, 2024

Add support for utf8 names on /v1/label/:name/values endpoint #14943

Closed

ywwg force-pushed the labels-utf8-fix branch from 461b63f to 609a49b Compare November 14, 2024 15:54

ywwg requested a review from beorn7 November 14, 2024 19:01

itsmylife mentioned this pull request Nov 15, 2024

Prometheus: Support utf-8 labels and metric names grafana/grafana#96244

Closed

17 tasks

bwplotka previously approved these changes Nov 18, 2024

View reviewed changes

beorn7 reviewed Nov 19, 2024

View reviewed changes

ywwg dismissed bwplotka’s stale review via 90b9b4a November 20, 2024 15:31

ywwg force-pushed the labels-utf8-fix branch from 90b9b4a to 23517a2 Compare November 20, 2024 15:32

ywwg requested a review from beorn7 November 20, 2024 15:32

beorn7 requested changes Nov 20, 2024

View reviewed changes

Comment thread docs/querying/api.md Outdated

Comment thread docs/querying/api.md Outdated

ywwg force-pushed the labels-utf8-fix branch from 23517a2 to 919cb80 Compare November 20, 2024 18:29

jan--f added the backport-3.0 Fixes that need to be backported to release 3.0 label Nov 21, 2024

beorn7 requested changes Nov 21, 2024

View reviewed changes

ywwg force-pushed the labels-utf8-fix branch from 919cb80 to 528ecf6 Compare November 25, 2024 16:48

ywwg changed the base branch from main to release-3.0 November 25, 2024 16:49

ywwg force-pushed the labels-utf8-fix branch 2 times, most recently from 25085ee to 07b9c49 Compare November 27, 2024 18:09

ywwg requested a review from beorn7 November 27, 2024 18:09

beorn7 approved these changes Nov 27, 2024

View reviewed changes

jan--f approved these changes Nov 27, 2024

View reviewed changes

Comment thread docs/querying/api.md Outdated

Add support for values unescaping on /v1/label/:name/values endpoint

12577e3

Signed-off-by: Owen Williams <[email protected]>

ywwg force-pushed the labels-utf8-fix branch from 07b9c49 to 12577e3 Compare November 27, 2024 19:03

jan--f enabled auto-merge November 27, 2024 19:05

jan--f merged commit c5c222e into release-3.0 Nov 27, 2024

jan--f deleted the labels-utf8-fix branch November 27, 2024 19:13

charleskorn mentioned this pull request Dec 1, 2024

Merge upstream at cd1f8ac grafana/mimir-prometheus#775

Closed

krajorama mentioned this pull request Dec 9, 2024

Sync upstream at current HEAD af2a1cb10c89de496cd4309ac532624b34112d74 grafana/mimir-prometheus#787

Merged

tlgenius mentioned this pull request Dec 19, 2024

Error scraping target: received unsupported Content-Type "text/html; charset=utf-8" dalance/prosafe_exporter#228

Closed

ywwg mentioned this pull request Jan 7, 2025

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

Closed

40 tasks

yeya24 mentioned this pull request Mar 29, 2025

e2e: basic utf8 test thanos-io/thanos#8171

Open

2 tasks

		For label names that contain characters that are not valid in the legacy
		Prometheus character set (letters, numbers, underscores, and colons), they

Conversation

ywwg commented Nov 14, 2024

Uh oh!

ywwg commented Nov 14, 2024

Uh oh!

bwplotka left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywwg commented Nov 18, 2024

Uh oh!

beorn7 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

beorn7 commented Nov 21, 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!

beorn7 Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

jan--f commented Nov 25, 2024

Uh oh!

ywwg commented Nov 25, 2024

Uh oh!

ywwg commented Nov 27, 2024

Uh oh!

beorn7 left a comment

Choose a reason for hiding this comment

Uh oh!

jan--f left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bwplotka left a comment •

edited

Loading

beorn7 Nov 26, 2024 •

edited

Loading