From bdd9adfb606d36277085cf66d9ad255afddba1f4 Mon Sep 17 00:00:00 2001
From: Punya Biswal <punya@google.com>
Date: Sat, 23 Sep 2023 07:48:33 -0400
Subject: [PATCH] [exporters/prometheus] Handle attribute key collisions from
 sanitation

Fixes #2290
---
 CHANGELOG.md                                  |  2 +
 exporters/prometheus/src/exporter_utils.cc    | 37 ++++++++++++++----
 .../prometheus/test/exporter_utils_test.cc    | 38 +++++++++++++++++++
 3 files changed, 69 insertions(+), 8 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5b2e80326e..38a077e7d8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -19,6 +19,8 @@ Increment the:
   [#2291](https://github.com/open-telemetry/opentelemetry-cpp/pull/2291)
 * [EXPORTER] Remove explicit timestamps from metric points exported by Prometheus
   [#2324](https://github.com/open-telemetry/opentelemetry-cpp/pull/2324)
+* [EXPORTER] Handle attribute key collisions caused by sanitation
+  [#2324](https://github.com/open-telemetry/opentelemetry-cpp/pull/2326)
 
 ## [1.11.0] 2023-08-21
 
diff --git a/exporters/prometheus/src/exporter_utils.cc b/exporters/prometheus/src/exporter_utils.cc
index e912256e4f..149d93290b 100644
--- a/exporters/prometheus/src/exporter_utils.cc
+++ b/exporters/prometheus/src/exporter_utils.cc
@@ -258,16 +258,37 @@ void PrometheusExporterUtils::SetData(std::vector<T> values,
 void PrometheusExporterUtils::SetMetricBasic(prometheus_client::ClientMetric &metric,
                                              const metric_sdk::PointAttributes &labels)
 {
-  // auto label_pairs = ParseLabel(labels);
-  if (!labels.empty())
+  if (labels.empty())
   {
-    metric.label.resize(labels.size());
-    size_t i = 0;
-    for (auto const &label : labels)
+    return;
+  }
+
+  // Concatenate values for keys that collide after sanitation.
+  // Note that attribute keys are sorted, but sanitized keys can be out-of-order.
+  // We could sort the sanitized keys again, but this seems too expensive to do
+  // in this hot code path. Instead, we ignore out-of-order keys and emit a warning.
+  metric.label.reserve(labels.size());
+  std::string previous_key;
+  for (auto const &label : labels)
+  {
+    auto sanitized = SanitizeNames(label.first);
+    int comparison = previous_key.compare(sanitized);
+    if (metric.label.empty() || comparison < 0)  // new key
+    {
+      previous_key = sanitized;
+      metric.label.push_back({sanitized, AttributeValueToString(label.second)});
+    }
+    else if (comparison == 0)  // key collision after sanitation
+    {
+      metric.label.back().value += ";" + AttributeValueToString(label.second);
+    }
+    else  // order inversion introduced by sanitation
     {
-      auto sanitized          = SanitizeNames(label.first);
-      metric.label[i].name    = sanitized;
-      metric.label[i++].value = AttributeValueToString(label.second);
+      OTEL_INTERNAL_LOG_WARN(
+          "[Prometheus Exporter] SetMetricBase - "
+          "the sort order of labels has changed because of sanitization: '"
+          << label.first << "' became '" << sanitized << "' which is less than '" << previous_key
+          << "'. Ignoring this label.");
     }
   }
 }
diff --git a/exporters/prometheus/test/exporter_utils_test.cc b/exporters/prometheus/test/exporter_utils_test.cc
index ac9ed4545f..0c08e2ffa7 100644
--- a/exporters/prometheus/test/exporter_utils_test.cc
+++ b/exporters/prometheus/test/exporter_utils_test.cc
@@ -163,4 +163,42 @@ TEST(PrometheusExporterUtils, SanitizeName)
   ASSERT_EQ(exporter::metrics::SanitizeNameTester::sanitize("name?__name:"), "name_name:");
 }
 
+class AttributeCollisionTest : public ::testing::Test
+{
+  Resource resource_ = Resource::Create(ResourceAttributes{});
+  nostd::unique_ptr<InstrumentationScope> instrumentation_scope_ =
+      InstrumentationScope::Create("library_name", "1.2.0");
+  metric_sdk::InstrumentDescriptor instrument_descriptor_{"library_name", "description", "unit",
+                                                          metric_sdk::InstrumentType::kCounter,
+                                                          metric_sdk::InstrumentValueType::kDouble};
+
+protected:
+  void CheckTranslation(const metric_sdk::PointAttributes &attrs,
+                        const std::vector<prometheus::ClientMetric::Label> &expected)
+  {
+    std::vector<prometheus::MetricFamily> result = PrometheusExporterUtils::TranslateToPrometheus(
+        {&resource_,
+         {{instrumentation_scope_.get(), {{instrument_descriptor_, {}, {}, {}, {{attrs, {}}}}}}}});
+    EXPECT_EQ(result.begin()->metric.begin()->label, expected);
+  }
+};
+
+TEST_F(AttributeCollisionTest, SeparatesDistinctKeys)
+{
+  CheckTranslation({{"foo.a", "value1"}, {"foo.b", "value2"}},
+                   {{"foo_a", "value1"}, {"foo_b", "value2"}});
+}
+
+TEST_F(AttributeCollisionTest, JoinsCollidingKeys)
+{
+  CheckTranslation({{"foo.a", "value1"}, {"foo_a", "value2"}},  //
+                   {{"foo_a", "value1;value2"}});
+}
+
+TEST_F(AttributeCollisionTest, DropsInvertedKeys)
+{
+  CheckTranslation({{"foo.a", "value1"}, {"foo.b", "value2"}, {"foo__a", "value3"}},
+                   {{"foo_a", "value1"}, {"foo_b", "value2"}});
+}
+
 OPENTELEMETRY_END_NAMESPACE