[exporters/prometheus] Handle attribute key collisions from sanitation

Fixes #2290
open-telemetry · Sep 23, 2023 · aa43a2a · aa43a2a
1 parent 41a7875
commit aa43a2a
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 8 deletions.
diff --git 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
@@ -163,4 +163,49 @@ 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)
+  {
+    metric_sdk::MetricData metric_data;
+    metric_data.instrument_descriptor = instrument_descriptor_;
+    metric_data.point_data_attr_.push_back({attrs, metric_sdk::SumPointData{10.0}});
+
+    metric_sdk::ResourceMetrics data;
+    data.resource_ = &resource_;
+    data.scope_metric_data_.push_back({instrumentation_scope_.get()});
+    data.scope_metric_data_.back().metric_data_.push_back({metric_data});
+
+    std::vector<prometheus::ClientMetric::Label> result =
+        PrometheusExporterUtils::TranslateToPrometheus(data).begin()->metric.begin()->label;
+    ASSERT_EQ(result, 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