From d5b2db34d53a9a9a655d974d9c6e7acad11e4533 Mon Sep 17 00:00:00 2001
From: Arthur Silva Sens <arthursens2005@gmail.com>
Date: Tue, 17 Dec 2024 14:36:42 -0300
Subject: [PATCH] Address PR comments.

Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
---
 .../prometheus/metric_name_builder.go          | 18 ++++++++++++------
 .../prometheus/metric_name_builder_test.go     |  2 ++
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/storage/remote/otlptranslator/prometheus/metric_name_builder.go b/storage/remote/otlptranslator/prometheus/metric_name_builder.go
index ee5a222473..7d9d085c55 100644
--- a/storage/remote/otlptranslator/prometheus/metric_name_builder.go
+++ b/storage/remote/otlptranslator/prometheus/metric_name_builder.go
@@ -120,12 +120,17 @@ var (
 
 // Build a normalized name for the specified metric.
 func normalizeName(metric pmetric.Metric, namespace string) string {
-	var nameTokens = strings.FieldsFunc(
+	// Split metric name into "tokens" (of supported metric name runes).
+	// Note that this has the side effect of replacing multiple consecutive underscores with a single underscore.
+	// This is part of the OTel to Prometheus specification: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.38.0/specification/compatibility/prometheus_and_openmetrics.md#otlp-metric-points-to-prometheus.
+	nameTokens := strings.FieldsFunc(
 		metric.Name(),
 		func(r rune) bool { return nonMetricNameCharRE.MatchString(string(r)) },
 	)
 
 	mainUnitSuffix, perUnitSuffix := buildUnitSuffixes(metric.Unit())
+	mainUnitSuffix = cleanUpUnit(mainUnitSuffix)
+	perUnitSuffix = cleanUpUnit(perUnitSuffix)
 	if mainUnitSuffix != "" && !slices.Contains(nameTokens, mainUnitSuffix) {
 		nameTokens = append(nameTokens, mainUnitSuffix)
 	}
@@ -248,11 +253,15 @@ func buildUnitSuffixes(unit string) (mainUnitSuffix, perUnitSuffix string) {
 	unitTokens := strings.SplitN(unit, "/", 2)
 
 	if len(unitTokens) > 0 {
+		// Main unit
+		// Append if not blank and doesn't contain '{}'
 		mainUnitOTel := strings.TrimSpace(unitTokens[0])
 		if mainUnitOTel != "" && !strings.ContainsAny(mainUnitOTel, "{}") {
-			mainUnitSuffix = cleanUpUnit(unitMapGetOrDefault(mainUnitOTel))
+			mainUnitSuffix = unitMapGetOrDefault(mainUnitOTel)
 		}
 
+		// Per unit
+		// Append if not blank and doesn't contain '{}'
 		if len(unitTokens) > 1 && unitTokens[1] != "" {
 			perUnitOTel := strings.TrimSpace(unitTokens[1])
 			if perUnitOTel != "" && !strings.ContainsAny(perUnitOTel, "{}") {
@@ -260,12 +269,9 @@ func buildUnitSuffixes(unit string) (mainUnitSuffix, perUnitSuffix string) {
 			}
 			if perUnitSuffix != "" {
 				perUnitSuffix = "per_" + perUnitSuffix
+				mainUnitSuffix = strings.TrimSuffix(mainUnitSuffix, "_")
 			}
 		}
-
-		if perUnitSuffix != "" {
-			mainUnitSuffix = strings.TrimSuffix(mainUnitSuffix, "_")
-		}
 	}
 
 	return mainUnitSuffix, perUnitSuffix
diff --git a/storage/remote/otlptranslator/prometheus/metric_name_builder_test.go b/storage/remote/otlptranslator/prometheus/metric_name_builder_test.go
index fa8832260b..d5b3d6f7f0 100644
--- a/storage/remote/otlptranslator/prometheus/metric_name_builder_test.go
+++ b/storage/remote/otlptranslator/prometheus/metric_name_builder_test.go
@@ -161,6 +161,7 @@ func TestBuildCompliantMetricNameWithSuffixes(t *testing.T) {
 	// Slashes in units are converted.
 	require.Equal(t, "system_io_foo_per_bar_total", BuildCompliantMetricName(createCounter("system.io", "foo/bar"), "", true))
 	require.Equal(t, "metric_with_foreign_characters_total", BuildCompliantMetricName(createCounter("metric_with_字符_foreign_characters", ""), "", true))
+	require.Equal(t, "temperature_C", BuildCompliantMetricName(createGauge("temperature", "%*()°C"), "", true)) // Removes non aplhanumerical characters from units
 }
 
 func TestBuildCompliantMetricNameWithoutSuffixes(t *testing.T) {
@@ -188,6 +189,7 @@ func TestBuildMetricNameWithSuffixes(t *testing.T) {
 	// Slashes in units are converted.
 	require.Equal(t, "system.io_foo_per_bar_total", BuildMetricName(createCounter("system.io", "foo/bar"), "", true))
 	require.Equal(t, "metric_with_字符_foreign_characters_total", BuildMetricName(createCounter("metric_with_字符_foreign_characters", ""), "", true))
+	require.Equal(t, "temperature_%*()°C", BuildMetricName(createGauge("temperature", "%*()°C"), "", true)) // Keeps the all characters in unit
 
 	// Tests below show weird interactions that users can have with the metric names.
 	// With BuildMetricName we don't check if units/type suffixes are already present in the metric name, we always add them.