otlp/translator: Use separate function for metric names with UTF8 characters by ArthurSens · Pull Request #15664 · prometheus/prometheus

ArthurSens · 2024-12-11T23:21:52Z

After many discussions with the team, it seems that our previous approach of using the method BuildCompliantName to translate all kinds of translation strategies stretched its usefulness too much.

This is an alternative for #15440. Here, concerns are split into two separate functions. BuildCompliantName was renamed to BuildCompliantMetricName and focuses only on building metric names that follow Prometheus naming conventions.

The new function BuildMetricName is responsible for building metric names that keep characters as they come. The only thing it does is append prefixes and suffixes, if asked.

For the reviewers, I recommend reviewing commits individually :)

Fixes #15534
Fixes #15362

ArthurSens · 2024-12-11T23:26:19Z

@aknuds1 I did not add a test specifically for the problem described in #15534, but I can add it if you think it adds value :)

storage/remote/otlptranslator/prometheus/metric_name_builder_test.go

aknuds1 · 2024-12-12T13:37:39Z

Can you change the name of this PR? Refactoring per definition doesn't fix any bugs. Refactoring just means changing code for maintainability reasons.

ArthurSens · 2024-12-16T21:37:28Z

merge conflict resolved :)

aknuds1 · 2024-12-17T07:38:17Z

Thanks, going to take a look.

aknuds1

Please see comments for bugs I could spot.

storage/remote/otlptranslator/prometheus/metric_name_builder.go

aknuds1

I forgot to include one of the bugs I spotted in the previous review.

storage/remote/otlptranslator/prometheus/metric_name_builder.go

aknuds1

I've defined a test that catches a bug in this PR. If you try that with this PR, you'll see that the suffix comes out as _km_per_, instead of just _km.

It's probably a good idea to copy said test to this PR, to guard against the regression in question.

ArthurSens · 2024-12-23T13:10:54Z

I've defined a test that catches a bug in this PR. If you try that with this PR, you'll see that the suffix comes out as _km_per_, instead of just _km.

It's probably a good idea to copy said test to this PR, to guard against the regression in question.

Sounds good, I'm merging that PR and rebasing this one

ArthurSens · 2024-12-23T13:35:18Z

Hmmm, I'm not really sure about what should be the expected behavior in the failing test 😵

require.Equal(t, "broken_metric_speed_km", normalizeName(createGauge("broken.metric.speed", "km/°"), ""))
// Not equal: 
// expected: "broken_metric_speed_km"
// actual  : "broken_metric_speed_km_per_"

We see a /, so per OTel conventions it's a something per something kind of translation, but the second unit is invalid for the Prometheus Compliant naming convention. What should we do here? Drop the per completely? Or keep the current behavior that translates to per_?

aknuds1 · 2024-12-23T20:41:22Z

Hmmm, I'm not really sure about what should be the expected behavior in the failing test 😵
We see a /, so per OTel conventions it's a something per something kind of translation, but the second unit is invalid for the Prometheus Compliant naming convention. What should we do here? Drop the per completely? Or keep the current behavior that translates to per_?

@ArthurSens It's the other way around. The current behaviour is what's in main, i.e. before the changes in this PR, and since the test in question passes there (on the main branch), the current behaviour is to drop the "per" part. The expectation of the test represents the current, desired, behaviour. This PR is not supposed to change the classical translation mode, only the UTF-8 translation mode, so it follows that the implementation should merely pass the test.

BuildCompliantName no longer takes UTF8 support into consideration and fully focus on building a metric name that follows Prometheus conventions. Signed-off-by: Arthur Silva Sens <[email protected]>

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

By making it return only the appropriate suffixes without modifying them. Character transformation is now done by the caller if necessary. Signed-off-by: Arthur Silva Sens <[email protected]>

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

ArthurSens · 2025-01-03T15:01:06Z

Finally coming back to this after the holidays 👋

aknuds1

Looks much better now, but please see my suggestion for cleaning up a hairy code block that I think has a bug:

storage/remote/otlptranslator/prometheus/metric_name_builder.go

storage/remote/otlptranslator/prometheus/metric_name_builder_test.go

Co-authored-by: Arve Knudsen <[email protected]> Signed-off-by: Arthur Silva Sens <[email protected]>

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

aknuds1

LGTM, but I think there's a misleading test case comment. Should we remove the test case?

storage/remote/otlptranslator/prometheus/metric_name_builder_test.go

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

aknuds1

LGTM, thanks

ArthurSens requested review from aknuds1 and jesusvazquez as code owners December 11, 2024 23:21

ArthurSens commented Dec 12, 2024

View reviewed changes

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

ArthurSens force-pushed the otlp-noncompliant-name branch from 4863399 to 90ce391 Compare December 12, 2024 12:59

ArthurSens changed the title ~~otlp/translator: Refactor how OTLP metric names are translated~~ otlp/translator: Use separate function for metric names with UTF8 characters Dec 12, 2024

ArthurSens force-pushed the otlp-noncompliant-name branch from 90ce391 to d1934e8 Compare December 16, 2024 21:37

aknuds1 requested changes Dec 17, 2024

View reviewed changes

aknuds1 mentioned this pull request Dec 17, 2024

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

Closed

aknuds1 requested changes Dec 17, 2024

View reviewed changes

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

aknuds1 requested changes Dec 21, 2024

View reviewed changes

ArthurSens force-pushed the otlp-noncompliant-name branch from bf1c8ff to 960edd8 Compare December 23, 2024 13:30

ArthurSens added 8 commits January 3, 2025 11:57

Revert changes made to BuildCompliantName.

3e14a76

BuildCompliantName no longer takes UTF8 support into consideration and fully focus on building a metric name that follows Prometheus conventions. Signed-off-by: Arthur Silva Sens <[email protected]>

Add option for metric names that don't follow Prometheus conventions

030cc13

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

Remove unused file

f0108c8

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

lint

13ea4db

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

Address PR comments.

d5b2db3

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

Split concerns for buildUnitSuffixes.

3a99b92

By making it return only the appropriate suffixes without modifying them. Character transformation is now done by the caller if necessary. Signed-off-by: Arthur Silva Sens <[email protected]>

Include colon in BuildCompliantMetricName test

f8bc738

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

Clean up perUnits ending with underscores

2026c5b

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

ArthurSens force-pushed the otlp-noncompliant-name branch from 8a30da4 to 2026c5b Compare January 3, 2025 14:58

lint

bca87fc

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

aknuds1 reviewed Jan 3, 2025

View reviewed changes

ArthurSens and others added 3 commits January 3, 2025 15:16

Apply suggestions from code review

cd111bd

Co-authored-by: Arve Knudsen <[email protected]> Signed-off-by: Arthur Silva Sens <[email protected]>

Fix order of trim and contains

96f7583

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

Remove empty lines from tests

c32694a

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

ArthurSens mentioned this pull request Jan 3, 2025

[pkg/translator/prometheus] Add option to keep UTF-8 characters open-telemetry/opentelemetry-collector-contrib#35904

Closed

aknuds1 previously approved these changes Jan 6, 2025

View reviewed changes

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

aknuds1 added kind/bug area/opentelemetry labels Jan 6, 2025

remove misleading test case

dee6149

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

ArthurSens dismissed aknuds1’s stale review via dee6149 January 6, 2025 13:33

aknuds1 approved these changes Jan 6, 2025

View reviewed changes

ArthurSens enabled auto-merge (squash) January 6, 2025 14:19

ArthurSens merged commit 5fdec31 into main Jan 6, 2025

ArthurSens deleted the otlp-noncompliant-name branch January 6, 2025 14:30

julienduchesne mentioned this pull request Jan 8, 2025

Sync upstream up to 5fdec3140188808bb9c2bf48e94b8533dbf75ac5 grafana/mimir-prometheus#818

Merged

ArthurSens mentioned this pull request Jan 15, 2025

Update OTLP guide with what's new in 3.2 prometheus/docs#2574

Merged

Conversation

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

Uh oh!

Uh oh!

ArthurSens commented Dec 11, 2024

Uh oh!

Uh oh!

aknuds1 commented Dec 12, 2024

Uh oh!

ArthurSens commented Dec 16, 2024

Uh oh!

aknuds1 commented Dec 17, 2024

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!

Uh oh!

Uh oh!

aknuds1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

ArthurSens commented Dec 23, 2024

Uh oh!

ArthurSens commented Dec 23, 2024

Uh oh!

aknuds1 commented Dec 23, 2024

Uh oh!

ArthurSens commented Jan 3, 2025

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aknuds1 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ArthurSens commented Dec 11, 2024 •

edited

Loading

aknuds1 left a comment •

edited

Loading