Merge pull request #60 from jupp0r/feature/check-labels

Check metric and label names

Author: jupp0r

.travis.yml

@@ -13,6 +13,10 @@ env:
 install:
   - sudo apt-get update
   - sudo apt-get install -y software-properties-common cmake curl python-pip git lcov
+  - sudo add-apt-repository -y ppa:ubuntu-toolchain-r/test
+  - sudo apt-get update
+  - sudo apt-get install -y gcc-5 g++-5
+  - sudo update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-5 1 --slave /usr/bin/g++ g++ /usr/bin/g++-5
   - wget https://github.com/google/protobuf/archive/v3.1.0.tar.gz
   - tar xzf v3.1.0.tar.gz
   - cd protobuf-3.1.0

BUILD

@@ -1,6 +1,7 @@
 cc_library(
     name = "prometheus-cpp",
-    srcs = ["lib/counter.cc",
+    srcs = ["lib/check_names.cc",
+            "lib/counter.cc",
             "lib/gauge.cc",
             "lib/exposer.cc",
             "lib/handler.cc",
@@ -27,6 +28,6 @@ cc_library(
             "@prometheus_client_model//:prometheus_client_model",
             "@civetweb//:civetweb",
            ],
-           linkstatic = 1,
-           copts = ["-I."],
+    linkstatic = 1,
+    copts = ["-I."],
 )

include/prometheus/check_names.h

@@ -0,0 +1,9 @@
+#pragma once
+
+#include <string>
+
+namespace prometheus {
+
+bool CheckMetricName(const std::string& name);
+bool CheckLabelName(const std::string& name);
+}

include/prometheus/family.h

@@ -8,11 +8,12 @@
 #include <string>
 #include <unordered_map>
 
+#include "check_names.h"
 #include "collectable.h"
-#include "metric.h"
 #include "counter_builder.h"
 #include "gauge_builder.h"
 #include "histogram_builder.h"
+#include "metric.h"
 
 namespace prometheus {
 
@@ -51,12 +52,21 @@ class Family : public Collectable {
 template <typename T>
 Family<T>::Family(const std::string& name, const std::string& help,
                   const std::map<std::string, std::string>& constant_labels)
-    : name_(name), help_(help), constant_labels_(constant_labels) {}
+    : name_(name), help_(help), constant_labels_(constant_labels) {
+  assert(CheckMetricName(name_));
+}
 
 template <typename T>
 template <typename... Args>
 T& Family<T>::Add(const std::map<std::string, std::string>& labels,
                   Args&&... args) {
+#ifndef NDEBUG
+  for (auto& label_pair : labels) {
+    auto& label_name = label_pair.first;
+    assert(CheckLabelName(label_name));
+  }
+#endif
+
   std::lock_guard<std::mutex> lock{mutex_};
   auto hash = hash_labels(labels);
   auto metric = new T(std::forward<Args>(args)...);

lib/CMakeLists.txt

@@ -15,6 +15,7 @@ add_custom_command(
   VERBATIM)
 
 add_library(prometheus-cpp
+  check_names.cc
   counter.cc
   counter_builder.cc
   exposer.cc

lib/check_names.cc

@@ -0,0 +1,22 @@
+#include <regex>
+
+#include <prometheus/check_names.h>
+
+namespace prometheus {
+bool CheckMetricName(const std::string& name) {
+  // see https://prometheus.io/docs/concepts/data_model/
+  auto reserved_for_internal_purposes = name.compare(0, 2, "__") == 0;
+  static const std::regex metric_name_regex("[a-zA-Z_:][a-zA-Z0-9_:]*");
+  return std::regex_match(name, metric_name_regex) &&
+         !reserved_for_internal_purposes;
+}
+
+bool CheckLabelName(const std::string& name) {
+  auto reserved_for_internal_purposes = name.compare(0, 2, "__") == 0;
+  // see https://prometheus.io/docs/concepts/data_model/
+  static const std::regex label_name_regex("[a-zA-Z_][a-zA-Z0-9_]*");
+  return std::regex_match(name, label_name_regex) &&
+         !reserved_for_internal_purposes;
+  ;
+}
+}

tests/BUILD

@@ -1,6 +1,7 @@
 cc_test(
     name = "prometheus-test",
     srcs = [
+        "check_names_test.cc",
         "counter_test.cc",
         "family_test.cc",
         "gauge_test.cc",

tests/CMakeLists.txt

@@ -5,6 +5,7 @@ endif()
 add_subdirectory(integration)
 
 add_executable(prometheus_test
+  check_names_test.cc
   counter_test.cc
   family_test.cc
   gauge_test.cc

tests/check_names_test.cc

@@ -0,0 +1,21 @@
+#include <gmock/gmock.h>
+#include <prometheus/check_names.h>
+
+using namespace testing;
+using namespace prometheus;
+
+class CheckNamesTest : public Test {};
+
+TEST_F(CheckNamesTest, empty_metric_name) { EXPECT_FALSE(CheckMetricName("")); }
+TEST_F(CheckNamesTest, good_metric_name) {
+  EXPECT_TRUE(CheckMetricName("prometheus_notifications_total"));
+}
+TEST_F(CheckNamesTest, reserved_metric_name) {
+  EXPECT_FALSE(CheckMetricName("__some_reserved_metric"));
+}
+
+TEST_F(CheckNamesTest, empty_label_name) { EXPECT_FALSE(CheckLabelName("")); }
+TEST_F(CheckNamesTest, good_label_name) { EXPECT_TRUE(CheckLabelName("type")); }
+TEST_F(CheckNamesTest, reserved_label_name) {
+  EXPECT_FALSE(CheckMetricName("__some_reserved_label"));
+}

tests/family_test.cc

@@ -66,11 +66,28 @@ TEST_F(FamilyTest, remove) {
 TEST_F(FamilyTest, Histogram) {
   Family<Histogram> family{"request_latency", "Latency Histogram", {}};
   auto& histogram1 = family.Add({{"name", "histogram1"}},
-                               Histogram::BucketBoundaries{0, 1, 2});
+                                Histogram::BucketBoundaries{0, 1, 2});
   histogram1.Observe(0);
   auto collected = family.Collect();
   ASSERT_EQ(collected.size(), 1);
   ASSERT_GE(collected[0].metric_size(), 1);
   ASSERT_TRUE(collected[0].metric(0).has_histogram());
   EXPECT_THAT(collected[0].metric(0).histogram().sample_count(), Eq(1));
 }
+
+#ifndef NDEBUG
+TEST_F(FamilyTest, should_assert_on_invalid_metric_name) {
+  auto create_family_with_invalid_name = []() {
+    new Family<Counter>("", "empty name", {});
+  };
+  EXPECT_DEATH(create_family_with_invalid_name(), ".*");
+}
+
+TEST_F(FamilyTest, should_assert_on_invalid_labels) {
+  Family<Counter> family{"total_requests", "Counts all requests", {}};
+  auto add_metric_with_invalid_label_name = [&family]() {
+    family.Add({{"__invalid", "counter1"}});
+  };
+  EXPECT_DEATH(add_metric_with_invalid_label_name(), ".*");
+}
+#endif