From 396cc23544c838df9d03dd07295c0ebec82ea7bf Mon Sep 17 00:00:00 2001
From: Gregor Jasny <gjasny@googlemail.com>
Date: Wed, 6 Apr 2022 22:57:24 +0200
Subject: [PATCH] fix(core): ignore labels with empty value

The Data Model spec states:
> A label with an empty label value is considered
> equivalent to a label that does not exist.
---
 core/include/prometheus/family.h |  8 +++---
 core/src/family.cc               | 46 ++++++++++++++++++++++++++++----
 core/tests/family_test.cc        | 39 +++++++++++++++++++++++++++
 3 files changed, 84 insertions(+), 9 deletions(-)

diff --git a/core/include/prometheus/family.h b/core/include/prometheus/family.h
index c5d725e4..55d5b07e 100644
--- a/core/include/prometheus/family.h
+++ b/core/include/prometheus/family.h
@@ -88,7 +88,7 @@ class PROMETHEUS_CPP_CORE_EXPORT Family : public Collectable {
   /// metric.
   /// \throw std::runtime_exception on invalid metric or label names.
   Family(const std::string& name, const std::string& help,
-         const Labels& constant_labels);
+         Labels constant_labels);
 
   /// \brief Add a new dimensional data.
   ///
@@ -108,7 +108,7 @@ class PROMETHEUS_CPP_CORE_EXPORT Family : public Collectable {
   /// labels already exists - the already existing dimensional data.
   /// \throw std::runtime_exception on invalid label names.
   template <typename... Args>
-  T& Add(const Labels& labels, Args&&... args) {
+  T& Add(Labels labels, Args&&... args) {
     return Add(labels, std::make_unique<T>(args...));
   }
 
@@ -121,7 +121,7 @@ class PROMETHEUS_CPP_CORE_EXPORT Family : public Collectable {
   /// \brief Returns true if the dimensional data with the given labels exist
   ///
   /// \param labels A set of key-value pairs (= labels) of the dimensional data.
-  bool Has(const Labels& labels) const;
+  bool Has(Labels labels) const;
 
   /// \brief Returns the name for this family.
   ///
@@ -149,7 +149,7 @@ class PROMETHEUS_CPP_CORE_EXPORT Family : public Collectable {
   mutable std::mutex mutex_;
 
   ClientMetric CollectMetric(const Labels& labels, T* metric) const;
-  T& Add(const Labels& labels, std::unique_ptr<T> object);
+  T& Add(Labels labels, std::unique_ptr<T> object);
 };
 
 }  // namespace prometheus
diff --git a/core/src/family.cc b/core/src/family.cc
index 46781a62..1b9c25c9 100644
--- a/core/src/family.cc
+++ b/core/src/family.cc
@@ -14,11 +14,42 @@
 #include "prometheus/summary.h"
 
 namespace prometheus {
+namespace {
+
+template <class Key, class T, class Compare, class Alloc, class Pred>
+void erase_if(std::map<Key, T, Compare, Alloc>& c, Pred pred) {
+  for (auto i = c.begin(), last = c.end(); i != last;)
+    if (pred(*i)) {
+      i = c.erase(i);
+    } else {
+      ++i;
+    }
+}
+
+auto empty_label_value = [](const Labels::value_type& label) {
+  return label.second.empty();
+};
+
+// A label with an empty label value is considered equivalent to a label that
+// does not exist.
+void filter_labels(Labels& labels) {
+  // with C++20 use std::erase_if
+  erase_if(labels, empty_label_value);
+}
+
+Labels filter_and_return_labels(Labels labels) {
+  filter_labels(labels);
+  return labels;
+}
+
+}  // namespace
 
 template <typename T>
 Family<T>::Family(const std::string& name, const std::string& help,
-                  const Labels& constant_labels)
-    : name_(name), help_(help), constant_labels_(constant_labels) {
+                  Labels constant_labels)
+    : name_(name),
+      help_(help),
+      constant_labels_(filter_and_return_labels(std::move(constant_labels))) {
   if (!CheckMetricName(name_)) {
     throw std::invalid_argument("Invalid metric name");
   }
@@ -31,7 +62,9 @@ Family<T>::Family(const std::string& name, const std::string& help,
 }
 
 template <typename T>
-T& Family<T>::Add(const Labels& labels, std::unique_ptr<T> object) {
+T& Family<T>::Add(Labels labels, std::unique_ptr<T> object) {
+  filter_labels(labels);
+
   std::lock_guard<std::mutex> lock{mutex_};
 
   auto insert_result =
@@ -70,9 +103,12 @@ void Family<T>::Remove(T* metric) {
 }
 
 template <typename T>
-bool Family<T>::Has(const Labels& labels) const {
+bool Family<T>::Has(Labels labels) const {
+  filter_labels(labels);
+
   std::lock_guard<std::mutex> lock{mutex_};
-  return metrics_.count(labels) != 0u;
+  auto count = metrics_.count(labels);
+  return count != 0u;
 }
 
 template <typename T>
diff --git a/core/tests/family_test.cc b/core/tests/family_test.cc
index f25b19a4..e1058b8c 100644
--- a/core/tests/family_test.cc
+++ b/core/tests/family_test.cc
@@ -139,5 +139,44 @@ TEST(FamilyTest, reject_summary_with_quantile_label) {
   EXPECT_ANY_THROW(family.Add(labels, quantiles));
 }
 
+TEST(FamilyTest, ignore_constant_labels_with_empty_value) {
+  auto labels = Labels{{"foo", "bar"}, {"label", ""}};
+  auto expected_labels = Labels{{"foo", "bar"}};
+
+  Family<Counter> family{"total_requests", "All Requests", labels};
+  EXPECT_EQ(expected_labels, family.GetConstantLabels());
+}
+
+TEST(FamilyTest, ignore_labels_with_empty_value_on_add) {
+  auto labels = Labels{{"foo", "bar"}, {"label", ""}};
+  auto expected_labels = Labels{{"foo", "bar"}};
+
+  Family<Counter> family{"total_requests", "All Requests", {}};
+  family.Add(labels);
+
+  EXPECT_TRUE(family.Has(expected_labels));
+}
+
+TEST(FamilyTest, ignore_labels_with_empty_value_for_has) {
+  auto labels = Labels{{"foo", "bar"}};
+  auto accepted_labels = Labels{{"foo", "bar"}, {"label", ""}};
+
+  Family<Counter> family{"total_requests", "All Requests", {}};
+  family.Add(labels);
+
+  EXPECT_TRUE(family.Has(accepted_labels));
+}
+
+TEST(FamilyTest, ignore_labels_with_empty_value_and_merge) {
+  auto labelsA = Labels{{"foo", "bar"}};
+  auto labelsB = Labels{{"foo", "bar"}, {"label", ""}};
+
+  Family<Counter> family{"total_requests", "All Requests", {}};
+  auto& a = family.Add(labelsA);
+  auto& b = family.Add(labelsB);
+
+  EXPECT_EQ(&a, &b);
+}
+
 }  // namespace
 }  // namespace prometheus