Allow UTF-8 characters in metric and label names as opt-in feature by ArthurSens · Pull Request #15258 · prometheus/prometheus

ArthurSens · 2024-11-01T14:02:55Z

Work originally comes from #14974

This PR aims to unblock OTel users who want to keep their metric names unchanged when sending it to Prometheus. The decision is backed by a survey, and by dev-summits where this topic was discussed.

To keep the current behavior, we're adding a new configuration option to the config.file, so people will need to opt-in

config/config.go

aknuds1 · 2024-11-02T15:00:13Z

Taking a look.

aknuds1 · 2024-11-03T12:28:34Z

Could you fix conflicts/tests?

aknuds1

Please see comments.

storage/remote/otlptranslator/prometheus/normalize_label.go

storage/remote/otlptranslator/prometheus/normalize_label_test.go

ArthurSens · 2024-11-06T20:47:13Z

Friendly ping @aknuds1, could we merge this now? :)

aknuds1 · 2024-11-07T07:07:17Z

Friendly ping @aknuds1, could we merge this now? :)

@ArthurSens I'll have to review it first, getting around to it today.

aknuds1

I found some more issues.

config/config.go

config/config_test.go

storage/remote/write_handler.go

config/config.go

Signed-off-by: Arthur Silva Sens <[email protected]>

documentation/examples/prometheus-otlp.yml

aknuds1 · 2024-11-07T14:06:23Z

storage/remote/otlptranslator/prometheus/normalize_name.go


-	// Metric name starts with a digit? Prefix it with an underscore.
-	if metricName != "" && unicode.IsDigit(rune(metricName[0])) {
+	// Metric name starts with a digit and utf8 not allowed? Prefix it with an underscore.


@jesusvazquez do you know if this change is correct?

The mode only says it will "keep UTF-8 characters as they are". Maybe it makes sense to also drop the rule of a metric not starting a metric with a digit, not sure.

I have no clue 😱

Signed-off-by: Arthur Silva Sens <[email protected]>

aknuds1 · 2024-11-07T14:25:50Z

storage/remote/otlptranslator/prometheus/normalize_name_test.go

+func TestAllowUTF8(t *testing.T) {
+	t.Run("allow UTF8", func(t *testing.T) {
+		require.Equal(t, "unsupported.metric.temperature_°F", normalizeName(createGauge("unsupported.metric.temperature", "°F"), "", true))
+		require.Equal(t, "unsupported.metric.weird_+=.:,!* & #", normalizeName(createGauge("unsupported.metric.weird", "+=.:,!* & #"), "", true))


If you try applying the following, it's going to fail. There's a fundamental bug in the UTF-8 handling.

Suggested change

require.Equal(t, "unsupported.metric.weird_+=.:,!* & #", normalizeName(createGauge("unsupported.metric.weird", "+=.:,!* & #"), "", true))

require.Equal(t, "unsupported.metric.+weird_+=.:,!* & #", normalizeName(createGauge("unsupported.metric.+weird", "+=.:,!* & #"), "", true))

Yes you're correct.

Just wanted to mention that although OTel allows full UTF8, the most common ones are dots and foreign characters, and this PR is focused to unblock OTel

I don't think we should approve a PR with fundamentally broken logic. I think @jesusvazquez is also checking this out now. It can't be that difficult to fix FWIW.

@jesusvazquez

Leaving to @jesusvazquez to review from here

Signed-off-by: Arthur Silva Sens <[email protected]>

ArthurSens · 2024-11-07T19:12:45Z

storage/remote/otlptranslator/prometheus/normalize_name_test.go

+func TestAllowUTF8KnownBugs(t *testing.T) {
+	// Due to historical reasons, the translator code was copied from OpenTelemetry collector codebase.
+	// Over there, they tried to provide means to translate metric names following Prometheus conventions that are documented here:
+	// https://prometheus.io/docs/practices/naming/
+	//
+	// Althogh not explicitly said, it was implied that words should be separated by a single underscore and the codebase was written
+	// with that in mind.
+	//
+	// Now that we're allowing OTel users to have their original names store in prometheus without any transformation, we're facing problems
+	// where two (or more) UTF-8 characters are being used to separate words.
+	// TODO(arthursens): Fix it!
+
+	// We're asserting on 'NotEqual', which proves the bug.
+	require.NotEqual(t, "metric....split_=+by_//utf8characters", normalizeName(createGauge("metric....split_=+by_//utf8characters", ""), "", true))
+	// Here we're asserting on 'Equal', showing the current behavior
+	require.Equal(t, "metric.split_by_utf8characters", normalizeName(createGauge("metric....split_=+by_//utf8characters", ""), "", true))
 }


I've added a test showcasing the known bug, along with the explanation to why we've got to this situation.

This is a super edge case since appliations using the OTel SDK follow semantic convention, and those conventions do not have metrics with repeated dots or things like that.

Could we tackle this bug in a separate PR since it's not mission critical?

Signed-off-by: Arthur Silva Sens <[email protected]>

jesusvazquez

LGTM lets do a close followup on that bug in case it happens in unknown cases!

ArthurSens · 2024-11-08T13:04:19Z

Issue opened, it's on my TODO list!

…15258) * Allow UTF-8 characters in metric and label names as opt-in feature --------- Signed-off-by: Arthur Silva Sens <[email protected]>

bwplotka · 2024-11-11T21:48:31Z

FYI, porting to 3.0 #15384

In non-UTF-8 mode, use strings.FieldsFunc to split string into tokens, as it was before PR prometheus#15258. This makes the code more robust and readable. Signed-off-by: Arve Knudsen <[email protected]>

…#15573) In non-UTF-8 mode, use strings.FieldsFunc to split string into tokens, as it was before PR #15258. This makes the code more robust and readable. Signed-off-by: Arve Knudsen <[email protected]>

ArthurSens requested review from aknuds1, bwplotka, cstyan, jesusvazquez and tomwilkie as code owners November 1, 2024 14:02

ArthurSens force-pushed the otlp-utf8-allowed branch from 429758e to 5056345 Compare November 1, 2024 14:06

ArthurSens commented Nov 1, 2024

View reviewed changes

config/config.go Outdated Show resolved Hide resolved

ArthurSens force-pushed the otlp-utf8-allowed branch from 5056345 to 5293e89 Compare November 1, 2024 14:09

ArthurSens mentioned this pull request Nov 2, 2024

bugfix: Fix otlp translator for foreign characters #15249

Merged

aknuds1 reviewed Nov 3, 2024

View reviewed changes

storage/remote/otlptranslator/prometheus/normalize_label.go Show resolved Hide resolved

storage/remote/otlptranslator/prometheus/normalize_label_test.go Outdated Show resolved Hide resolved

ArthurSens force-pushed the otlp-utf8-allowed branch 3 times, most recently from fab4d8b to b81eaef Compare November 4, 2024 14:27

ArthurSens mentioned this pull request Nov 4, 2024

chore: Remove dead code #15328

Merged

aknuds1 previously requested changes Nov 7, 2024

View reviewed changes

config/config.go Outdated Show resolved Hide resolved

config/config_test.go Outdated Show resolved Hide resolved

storage/remote/write_handler.go Outdated Show resolved Hide resolved

config/config.go Outdated Show resolved Hide resolved

ArthurSens force-pushed the otlp-utf8-allowed branch from b81eaef to 5743d83 Compare November 7, 2024 12:26

Allow UTF-8 characters in metric and label names as opt-in feature

58215fe

Signed-off-by: Arthur Silva Sens <[email protected]>

ArthurSens force-pushed the otlp-utf8-allowed branch from 5743d83 to 58215fe Compare November 7, 2024 13:08

jesusvazquez reviewed Nov 7, 2024

View reviewed changes

documentation/examples/prometheus-otlp.yml Outdated Show resolved Hide resolved

aknuds1 reviewed Nov 7, 2024

View reviewed changes

Fix bug where validation scheme was not validated

2eec687

Signed-off-by: Arthur Silva Sens <[email protected]>

aknuds1 reviewed Nov 7, 2024

View reviewed changes

Fix config tests

18abcd8

Signed-off-by: Arthur Silva Sens <[email protected]>

ArthurSens commented Nov 7, 2024

View reviewed changes

Add test showcasing known bug

6c79f5c

Signed-off-by: Arthur Silva Sens <[email protected]>

ArthurSens force-pushed the otlp-utf8-allowed branch from 8c8154f to 6c79f5c Compare November 7, 2024 19:14

ArthurSens mentioned this pull request Nov 8, 2024

otlp/translator: Metrics with words split by multiple UTF8 character lose characters during translation #15362

Closed

jesusvazquez approved these changes Nov 8, 2024

View reviewed changes

ArthurSens merged commit 0399577 into main Nov 8, 2024

ArthurSens deleted the otlp-utf8-allowed branch November 8, 2024 13:02

bwplotka mentioned this pull request Nov 11, 2024

3.0 Port: Allow UTF-8 characters in metric and label names as opt-in feature (plus config entry) #15384

Merged

aknuds1 mentioned this pull request Nov 25, 2024

Fix: OTLP translator now keeps multiple separators in metric names #15440

Closed

charleskorn mentioned this pull request Dec 1, 2024

Merge upstream at cd1f8ac grafana/mimir-prometheus#775

Closed

This was referenced Dec 3, 2024

merge upstream at f9057544cb69261acd59628c2e81369adeb584c9 grafana/mimir-prometheus#781

Merged

Vendor update mimir-prometheus at 320e9aa3985d0f997a125b1a58a4ff8d1ae… grafana/mimir#10102

Merged

aknuds1 mentioned this pull request Dec 10, 2024

OTLP receiver: Return to using stdlib for splitting metric name in classical translation mode #15573

Merged

julienduchesne mentioned this pull request Dec 14, 2024

Sync upstream to current HEAD at e2f037e554a8a336ca4487324dd4ceee893fda0a grafana/mimir-prometheus#792

Merged

ywwg mentioned this pull request Jan 8, 2025

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

Closed

40 tasks

	require.Equal(t, "unsupported.metric.weird_+=.:,!* & #", normalizeName(createGauge("unsupported.metric.weird", "+=.:,!* & #"), "", true))
	require.Equal(t, "unsupported.metric.+weird_+=.:,!* & #", normalizeName(createGauge("unsupported.metric.+weird", "+=.:,!* & #"), "", true))

Conversation

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

Uh oh!

Uh oh!

Uh oh!

aknuds1 commented Nov 2, 2024

Uh oh!

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

Uh oh!

Uh oh!

aknuds1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ArthurSens commented Nov 6, 2024

Uh oh!

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

Uh oh!

Uh oh!

aknuds1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aknuds1 Nov 7, 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!

jesusvazquez Nov 7, 2024

Choose a reason for hiding this comment

Uh oh!

aknuds1 Nov 7, 2024

Choose a reason for hiding this comment

Uh oh!

ArthurSens Nov 7, 2024

Choose a reason for hiding this comment

Uh oh!

aknuds1 Nov 7, 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!

ArthurSens Nov 7, 2024

Choose a reason for hiding this comment

Uh oh!

jesusvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

ArthurSens commented Nov 8, 2024

Uh oh!

bwplotka commented Nov 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ArthurSens commented Nov 1, 2024 •

edited

Loading

aknuds1 commented Nov 3, 2024 •

edited

Loading

aknuds1 commented Nov 7, 2024 •

edited

Loading

aknuds1 Nov 7, 2024 •

edited

Loading

aknuds1 Nov 7, 2024 •

edited

Loading