From 763f11d65bc49a020ca8610c350e6cda7dfdc1b2 Mon Sep 17 00:00:00 2001
From: Ehsan Saei <71217171+esigo@users.noreply.github.com>
Date: Fri, 16 Jun 2023 14:08:18 +0200
Subject: [PATCH] Prometheus exporter sanitizes invalid  characters (#1934)

---
 .../exporters/prometheus/exporter_utils.h     |  3 ++
 exporters/prometheus/src/exporter_utils.cc    | 39 +++++++++++++++++--
 .../prometheus/test/exporter_utils_test.cc    | 32 +++++++++++++--
 3 files changed, 67 insertions(+), 7 deletions(-)

diff --git a/exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h b/exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h
index e602c87737..ce7f0a1191 100644
--- a/exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h
+++ b/exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h
@@ -107,6 +107,9 @@ class PrometheusExporterUtils
                        const std::vector<double> &boundaries,
                        const std::vector<uint64_t> &counts,
                        ::prometheus::ClientMetric *metric);
+
+  // For testing
+  friend class SanitizeNameTester;
 };
 }  // namespace metrics
 }  // namespace exporter
diff --git a/exporters/prometheus/src/exporter_utils.cc b/exporters/prometheus/src/exporter_utils.cc
index 451c34ff45..0a072d3303 100644
--- a/exporters/prometheus/src/exporter_utils.cc
+++ b/exporters/prometheus/src/exporter_utils.cc
@@ -39,9 +39,10 @@ std::vector<prometheus_client::MetricFamily> PrometheusExporterUtils::TranslateT
     for (const auto &metric_data : instrumentation_info.metric_data_)
     {
       auto origin_name = metric_data.instrument_descriptor.name_;
+      auto unit        = metric_data.instrument_descriptor.unit_;
       auto sanitized   = SanitizeNames(origin_name);
       prometheus_client::MetricFamily metric_family;
-      metric_family.name = sanitized;
+      metric_family.name = sanitized + "_" + unit;
       metric_family.help = metric_data.instrument_descriptor.description_;
       auto time          = metric_data.start_ts.time_since_epoch();
       for (const auto &point_data_attr : metric_data.point_data_attr_)
@@ -129,10 +130,40 @@ std::vector<prometheus_client::MetricFamily> PrometheusExporterUtils::TranslateT
  */
 std::string PrometheusExporterUtils::SanitizeNames(std::string name)
 {
-  // replace all '.' and '-' with '_'
-  std::replace(name.begin(), name.end(), '.', '_');
-  std::replace(name.begin(), name.end(), '-', '_');
+  constexpr const auto replacement     = '_';
+  constexpr const auto replacement_dup = '=';
 
+  auto valid = [](int i, char c) {
+    if ((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || c == ':' ||
+        (c >= '0' && c <= '9' && i > 0))
+    {
+      return true;
+    }
+    return false;
+  };
+
+  bool has_dup = false;
+  for (int i = 0; i < (int)name.size(); ++i)
+  {
+    if (valid(i, name[i]))
+    {
+      continue;
+    }
+    if (i > 0 && (name[i - 1] == replacement || name[i - 1] == replacement_dup))
+    {
+      has_dup = true;
+      name[i] = replacement_dup;
+    }
+    else
+    {
+      name[i] = replacement;
+    }
+  }
+  if (has_dup)
+  {
+    auto end = std::remove(name.begin(), name.end(), replacement_dup);
+    return std::string{name.begin(), end};
+  }
   return name;
 }
 
diff --git a/exporters/prometheus/test/exporter_utils_test.cc b/exporters/prometheus/test/exporter_utils_test.cc
index 2b37ab77ad..2eac7a6d8b 100644
--- a/exporters/prometheus/test/exporter_utils_test.cc
+++ b/exporters/prometheus/test/exporter_utils_test.cc
@@ -14,6 +14,22 @@ namespace metric_api        = opentelemetry::metrics;
 namespace prometheus_client = ::prometheus;
 
 OPENTELEMETRY_BEGIN_NAMESPACE
+
+namespace exporter
+{
+namespace metrics
+{
+class SanitizeNameTester
+{
+public:
+  static std::string sanitize(std::string name)
+  {
+    return PrometheusExporterUtils::SanitizeNames(name);
+  }
+};
+}  // namespace metrics
+}  // namespace exporter
+
 template <typename T>
 void assert_basic(prometheus_client::MetricFamily &metric,
                   const std::string &sanitized_name,
@@ -22,9 +38,9 @@ void assert_basic(prometheus_client::MetricFamily &metric,
                   int label_num,
                   std::vector<T> vals)
 {
-  ASSERT_EQ(metric.name, sanitized_name);  // name sanitized
-  ASSERT_EQ(metric.help, description);     // description not changed
-  ASSERT_EQ(metric.type, type);            // type translated
+  ASSERT_EQ(metric.name, sanitized_name + "_unit");  // name sanitized
+  ASSERT_EQ(metric.help, description);               // description not changed
+  ASSERT_EQ(metric.type, type);                      // type translated
 
   auto metric_data = metric.metric[0];
   ASSERT_EQ(metric_data.label.size(), label_num);
@@ -129,4 +145,14 @@ TEST(PrometheusExporterUtils, TranslateToPrometheusHistogramNormal)
   assert_histogram(metric, std::list<double>{10.1, 20.2, 30.2}, {200, 300, 400, 500});
 }
 
+TEST(PrometheusExporterUtils, SanitizeName)
+{
+  ASSERT_EQ(exporter::metrics::SanitizeNameTester::sanitize("name"), "name");
+  ASSERT_EQ(exporter::metrics::SanitizeNameTester::sanitize("name?"), "name_");
+  ASSERT_EQ(exporter::metrics::SanitizeNameTester::sanitize("name???"), "name_");
+  ASSERT_EQ(exporter::metrics::SanitizeNameTester::sanitize("name?__"), "name_");
+  ASSERT_EQ(exporter::metrics::SanitizeNameTester::sanitize("name?__name"), "name_name");
+  ASSERT_EQ(exporter::metrics::SanitizeNameTester::sanitize("name?__name:"), "name_name:");
+}
+
 OPENTELEMETRY_END_NAMESPACE