replace invalid characters in ids (Netflix#106)

It is easy enough to break the spectatord line protocol, if any of the control
characters (`:,=`) are inserted in unexpected places. Since metric names and
tags are often programmatically generated, we want to only construct id strings
which are valid.

.gitignore

@@ -3,3 +3,4 @@
 .vscode/
 bazel-*
 build-*
+spectator/valid_chars.inc

BUILD.bazel

@@ -25,6 +25,20 @@ cc_library(
     ],
 )
 
+cc_binary(
+    name = "gen_valid_chars",
+    srcs = ["tools/gen_valid_chars.cc"],
+    visibility = ["//visibility:public"],
+)
+
+genrule(
+    name = "generate_valid_chars_inc",
+    srcs = [":gen_valid_chars"],  # Dependency on the gen_valid_chars executable
+    outs = ["spectator/valid_chars.inc"],
+    cmd = "$(location :gen_valid_chars) > $(OUTS)",
+    visibility = ["//visibility:public"],
+)
+
 cc_test(
     name = "spectator_test",
     srcs = glob([

CMakeLists.txt

@@ -0,0 +1,55 @@
+cmake_minimum_required(VERSION 3.13)
+
+project(spectator-cpp)
+
+set(CMAKE_CXX_STANDARD 17)
+set(CMAKE_CXX_STANDARD_REQUIRED True)
+
+add_compile_options(-fno-omit-frame-pointer "$<$<CONFIG:Debug>:-fsanitize=address>")
+add_link_options(-fno-omit-frame-pointer "$<$<CONFIG:Debug>:-fsanitize=address>")
+
+include(CTest)
+include(${CMAKE_BINARY_DIR}/conanbuildinfo.cmake)
+conan_basic_setup()
+
+#-- spectator_test test executable
+file(GLOB spectator_test_source_files
+    "spectator/*_test.cc"
+    "spectator/test_*.cc"
+    "spectator/test_*.h"
+)
+add_executable(spectator_test ${spectator_test_source_files})
+target_link_libraries(spectator_test spectator ${CONAN_LIBS})
+add_test(
+    NAME spectator_test
+    COMMAND spectator_test
+    WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
+)
+
+#-- spectator library
+add_library(spectator
+    "spectator/logger.cc"
+    "spectator/publisher.cc"
+    "spectator/config.h"
+    "spectator/id.h"
+    "spectator/logger.h"
+    "spectator/measurement.h"
+    "spectator/meter_type.h"
+    "spectator/publisher.h"
+    "spectator/registry.h"
+    "spectator/stateful_meters.h"
+    "spectator/stateless_meters.h"
+    "spectator/valid_chars.inc"
+)
+target_link_libraries(spectator ${CONAN_LIBS})
+target_link_options(spectator PRIVATE "$<$<CONFIG:Release>:-static-libstdc++>")
+
+#-- generator tools
+add_executable(gen_valid_chars "tools/gen_valid_chars.cc")
+
+#-- file generators, must exist where the outputs are referenced
+add_custom_command(
+    OUTPUT "spectator/valid_chars.inc"
+    COMMAND "${CMAKE_BINARY_DIR}/bin/gen_valid_chars" > "${CMAKE_SOURCE_DIR}/spectator/valid_chars.inc"
+    DEPENDS gen_valid_chars
+)

spectator/age_gauge_test.cc

@@ -25,4 +25,12 @@ TEST(AgeGauge, Set) {
   EXPECT_EQ(publisher.SentMessages(), expected);
 }
 
+TEST(AgeGauge, InvalidTags) {
+  TestPublisher publisher;
+  // test with a single tag, because tags order is not guaranteed in a flat_hash_map
+  auto id = std::make_shared<Id>("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo",
+                                 Tags{{"tag1,:=", "value1,:="}});
+  AgeGauge g{id, &publisher};
+  EXPECT_EQ("A:test______^____-_~______________.___foo,tag1___=value1___:", g.GetPrefix());
+}
 }  // namespace

spectator/counter_test.cc

@@ -13,8 +13,8 @@ TEST(Counter, Activity) {
   TestPublisher publisher;
   auto id = std::make_shared<Id>("ctr.name", Tags{});
   auto id2 = std::make_shared<Id>("c2", Tags{{"key", "val"}});
-  Counter<TestPublisher> c{id, &publisher};
-  Counter<TestPublisher> c2{id2, &publisher};
+  Counter c{id, &publisher};
+  Counter c2{id2, &publisher};
   c.Increment();
   c2.Add(1.2);
   c.Add(0.1);
@@ -25,10 +25,18 @@ TEST(Counter, Activity) {
 
 TEST(Counter, Id) {
   TestPublisher publisher;
-  Counter<TestPublisher> c{std::make_shared<Id>("foo", Tags{{"key", "val"}}),
+  Counter c{std::make_shared<Id>("foo", Tags{{"key", "val"}}),
                            &publisher};
   auto id = std::make_shared<Id>("foo", Tags{{"key", "val"}});
   EXPECT_EQ(*(c.MeterId()), *id);
 }
 
+TEST(Counter, InvalidTags) {
+  TestPublisher publisher;
+  // test with a single tag, because tags order is not guaranteed in a flat_hash_map
+  auto id = std::make_shared<Id>("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo",
+                                 Tags{{"tag1,:=", "value1,:="}});
+  Counter c{id, &publisher};
+  EXPECT_EQ("c:test______^____-_~______________.___foo,tag1___=value1___:", c.GetPrefix());
+}
 }  // namespace

spectator/dist_summary_test.cc

@@ -22,4 +22,12 @@ TEST(DistributionSummary, Record) {
   EXPECT_EQ(publisher.SentMessages(), expected);
 }
 
+TEST(DistributionSummary, InvalidTags) {
+  TestPublisher publisher;
+  // test with a single tag, because tags order is not guaranteed in a flat_hash_map
+  auto id = std::make_shared<Id>("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo",
+                                 Tags{{"tag1,:=", "value1,:="}});
+  DistributionSummary d{id, &publisher};
+  EXPECT_EQ("d:test______^____-_~______________.___foo,tag1___=value1___:", d.GetPrefix());
+}
 }  // namespace

spectator/gauge_test.cc

@@ -24,4 +24,12 @@ TEST(Gauge, Set) {
   EXPECT_EQ(publisher.SentMessages(), expected);
 }
 
+TEST(Gauge, InvalidTags) {
+  TestPublisher publisher;
+  // test with a single tag, because tags order is not guaranteed in a flat_hash_map
+  auto id = std::make_shared<Id>("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo",
+                                 Tags{{"tag1,:=", "value1,:="}});
+  Gauge g{id, &publisher};
+  EXPECT_EQ("g:test______^____-_~______________.___foo,tag1___=value1___:", g.GetPrefix());
+}
 }  // namespace

spectator/id_test.cc

@@ -11,6 +11,11 @@ TEST(Id, Create) {
 
   Id id_tags{"name", Tags{{"k", "v"}, {"k1", "v1"}}};
   EXPECT_EQ(id_tags.GetTags().size(), 2);
+
+  std::shared_ptr<Id> id_of{Id::of("name", Tags{{"k", "v"}, {"k1", "v1"}})};
+  EXPECT_EQ(id_of->Name(), "name");
+  EXPECT_EQ(id_of->GetTags().size(), 2);
+  fmt::format("{}", id);
 }
 
 TEST(Id, Tags) {

spectator/max_gauge_test.cc

@@ -24,4 +24,12 @@ TEST(MaxGauge, Set) {
   EXPECT_EQ(publisher.SentMessages(), expected);
 }
 
+TEST(MaxGauge, InvalidTags) {
+  TestPublisher publisher;
+  // test with a single tag, because tags order is not guaranteed in a flat_hash_map
+  auto id = std::make_shared<Id>("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo",
+                                 Tags{{"tag1,:=", "value1,:="}});
+  MaxGauge g{id, &publisher};
+  EXPECT_EQ("m:test______^____-_~______________.___foo,tag1___=value1___:", g.GetPrefix());
+}
 }  // namespace

spectator/monotonic_counter_test.cc

@@ -23,4 +23,13 @@ TEST(MonotonicCounter, Set) {
                                        "C:ctr:43"};
   EXPECT_EQ(publisher.SentMessages(), expected);
 }
+
+TEST(MonotonicCounter, InvalidTags) {
+  TestPublisher publisher;
+  // test with a single tag, because tags order is not guaranteed in a flat_hash_map
+  auto id = std::make_shared<Id>("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo",
+                                 Tags{{"tag1,:=", "value1,:="}});
+  MonotonicCounter c{id, &publisher};
+  EXPECT_EQ("C:test______^____-_~______________.___foo,tag1___=value1___:", c.GetPrefix());
+}
 }  // namespace

spectator/monotonic_counter_uint_test.cc

@@ -0,0 +1,34 @@
+#include "stateless_meters.h"
+#include "test_publisher.h"
+#include <gtest/gtest.h>
+
+namespace {
+
+using spectator::Id;
+using spectator::MonotonicCounterUint;
+using spectator::Tags;
+using spectator::TestPublisher;
+
+TEST(MonotonicCounterUint, Set) {
+  TestPublisher publisher;
+  auto id = std::make_shared<Id>("ctr", Tags{});
+  auto id2 = std::make_shared<Id>("ctr2", Tags{{"key", "val"}});
+  MonotonicCounterUint<TestPublisher> c{id, &publisher};
+  MonotonicCounterUint<TestPublisher> c2{id2, &publisher};
+
+  c.Set(42);
+  c2.Set(2);
+  c.Set(-1);
+  std::vector<std::string> expected = {"U:ctr:42", "U:ctr2,key=val:2", "U:ctr:18446744073709551615"};
+  EXPECT_EQ(publisher.SentMessages(), expected);
+}
+
+TEST(MonotonicCounterUint, InvalidTags) {
+  TestPublisher publisher;
+  // test with a single tag, because tags order is not guaranteed in a flat_hash_map
+  auto id = std::make_shared<Id>("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo",
+                                 Tags{{"tag1,:=", "value1,:="}});
+  MonotonicCounterUint c{id, &publisher};
+  EXPECT_EQ("U:test______^____-_~______________.___foo,tag1___=value1___:", c.GetPrefix());
+}
+}  // namespace

spectator/perc_dist_summary_test.cc

@@ -11,13 +11,21 @@ using spectator::TestPublisher;
 TEST(PercDistSum, Record) {
   TestPublisher publisher;
   auto id = std::make_shared<Id>("pds", Tags{});
-  PercentileDistributionSummary<TestPublisher> c{id, &publisher, 0, 1000};
-  c.Record(50);
-  c.Record(5000);
-  c.Record(-5000);
+  PercentileDistributionSummary d{id, &publisher, 0, 1000};
+  d.Record(50);
+  d.Record(5000);
+  d.Record(-5000);
   std::vector<std::string> expected = {"D:pds:50", "D:pds:1000",
                                        "D:pds:0"};
   EXPECT_EQ(publisher.SentMessages(), expected);
 }
 
+TEST(PercDistSum, InvalidTags) {
+  TestPublisher publisher;
+  // test with a single tag, because tags order is not guaranteed in a flat_hash_map
+  auto id = std::make_shared<Id>("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo",
+                                 Tags{{"tag1,:=", "value1,:="}});
+  PercentileDistributionSummary d{id, &publisher, 0, 1000};
+  EXPECT_EQ("D:test______^____-_~______________.___foo,tag1___=value1___:", d.GetPrefix());
+}
 }  // namespace

spectator/perc_timer_test.cc

@@ -11,14 +11,20 @@ using spectator::TestPublisher;
 TEST(PercentileTimer, Record) {
   TestPublisher publisher;
   auto id = std::make_shared<Id>("pt", Tags{});
-  PercentileTimer<TestPublisher> c{id, &publisher, absl::ZeroDuration(),
-                                   absl::Seconds(5)};
+  PercentileTimer c{id, &publisher, absl::ZeroDuration(), absl::Seconds(5)};
   c.Record(absl::Milliseconds(42));
   c.Record(std::chrono::microseconds(500));
   c.Record(absl::Seconds(10));
-  std::vector<std::string> expected = {"T:pt:0.042", "T:pt:0.0005",
-                                       "T:pt:5"};
+  std::vector<std::string> expected = {"T:pt:0.042", "T:pt:0.0005", "T:pt:5"};
   EXPECT_EQ(publisher.SentMessages(), expected);
 }
 
+TEST(PercentileTimer, InvalidTags) {
+  TestPublisher publisher;
+  // test with a single tag, because tags order is not guaranteed in a flat_hash_map
+  auto id = std::make_shared<Id>("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo",
+                                 Tags{{"tag1,:=", "value1,:="}});
+  PercentileTimer t{id, &publisher, absl::ZeroDuration(), absl::Seconds(5)};
+  EXPECT_EQ("T:test______^____-_~______________.___foo,tag1___=value1___:", t.GetPrefix());
+}
 }  // namespace

spectator/stateless_meters.h

@@ -6,14 +6,39 @@
 namespace spectator {
 
 namespace detail {
+
+#include "valid_chars.inc"
+
 inline std::string as_string(std::string_view v) { 
     return {v.data(), v.size()}; 
 }
+
+inline bool contains_non_atlas_char(const std::string& input) {
+  return std::any_of(input.begin(), input.end(), [](char c) { return !kAtlasChars[c]; });
+}
+
+inline std::string replace_invalid_characters(const std::string& input) {
+  if (contains_non_atlas_char(input)) {
+    std::string result{input};
+    for (char &c : result) {
+      if (!kAtlasChars[c]) {
+        c = '_';
+      }
+    }
+    return result;
+  } else {
+    return input;
+  }
+}
+
 inline std::string create_prefix(const Id& id, std::string_view type_name) {
-  std::string res = as_string(type_name) + ":" + id.Name();
+  std::string res = as_string(type_name) + ":" + replace_invalid_characters(id.Name());
   for (const auto& tags : id.GetTags()) {
-    absl::StrAppend(&res, ",", tags.first, "=", tags.second);
+    auto first = replace_invalid_characters(tags.first);
+    auto second = replace_invalid_characters(tags.second);
+    absl::StrAppend(&res, ",", first, "=", second);
   }
+
   absl::StrAppend(&res, ":");
   return res;
 }
@@ -38,6 +63,12 @@ class StatelessMeter {
     assert(publisher_ != nullptr);
   }
   virtual ~StatelessMeter() = default;
+  std::string GetPrefix() {
+    if (value_prefix_.empty()) {
+      value_prefix_ = detail::create_prefix(*id_, Type());
+    }
+    return value_prefix_;
+  }
   [[nodiscard]] IdPtr MeterId() const noexcept { return id_; }
   [[nodiscard]] virtual std::string_view Type() = 0;
 

spectator/timer_test.cc

@@ -12,14 +12,21 @@ TEST(Timer, Record) {
   TestPublisher publisher;
   auto id = std::make_shared<Id>("t.name", Tags{});
   auto id2 = std::make_shared<Id>("t2", Tags{{"key", "val"}});
-  Timer<TestPublisher> t{id, &publisher};
-  Timer<TestPublisher> t2{id2, &publisher};
+  Timer t{id, &publisher};
+  Timer t2{id2, &publisher};
   t.Record(std::chrono::milliseconds(1));
   t2.Record(absl::Seconds(0.1));
   t2.Record(absl::Microseconds(500));
-  std::vector<std::string> expected = {
-      "t:t.name:0.001", "t:t2,key=val:0.1", "t:t2,key=val:0.0005"};
+  std::vector<std::string> expected = {"t:t.name:0.001", "t:t2,key=val:0.1", "t:t2,key=val:0.0005"};
   EXPECT_EQ(publisher.SentMessages(), expected);
 }
 
+TEST(Timer, InvalidTags) {
+  TestPublisher publisher;
+  // test with a single tag, because tags order is not guaranteed in a flat_hash_map
+  auto id = std::make_shared<Id>("timer`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo",
+                                 Tags{{"tag1,:=", "value1,:="}});
+  Timer t{id, &publisher};
+  EXPECT_EQ("t:timer______^____-_~______________.___foo,tag1___=value1___:", t.GetPrefix());
+}
 }  // namespace