replace invalid characters in ids (#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

@@ -2,4 +2,5 @@
 .idea/
 cmake-build-debug
 cmake-build/
+spectator/valid_chars.inc
 venv/

CMakeLists.txt

@@ -39,6 +39,17 @@ add_library(spectator
     "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
+)

build.sh

@@ -10,8 +10,11 @@ NC="\033[0m"
 if [[ "$1" == "clean" ]]; then
   echo -e "${BLUE}==== clean ====${NC}"
   rm -rf $BUILD_DIR
-  # remove all packages and binaries from the local cache, to allow swapping between Debug/Release builds
-  conan remove '*' --force
+  rm -f spectator/*.inc
+  if [[ "$2" == "--force" ]]; then
+    # remove all packages and binaries from the local cache, to allow swapping between Debug/Release builds
+    conan remove '*' --force
+  fi
 fi
 
 if [[ "$OSTYPE" == "linux-gnu"* ]]; then

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

@@ -17,6 +17,7 @@ TEST(Id, Create) {
   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

@@ -22,4 +22,13 @@ TEST(MonotonicCounter, Set) {
   std::vector<std::string> expected = {"C:ctr:42.1", "C:ctr2,key=val:2", "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

@@ -22,4 +22,13 @@ TEST(MonotonicCounterUint, Set) {
   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

tools/gen_valid_chars.cc

@@ -0,0 +1,48 @@
+// generate the atlas valid charsets
+
+#include <array>
+#include <fstream>
+
+void dump_array(std::ostream& os, const std::string& name, const std::array<bool, 256>& chars) {
+  os << "static constexpr std::array<bool, 256> " << name << " = {{";
+
+  os << chars[0];
+  for (auto i = 1u; i < chars.size(); ++i) {
+    os << ", " << chars[i];
+  }
+
+  os << "}};\n";
+}
+
+int main(int argc, char* argv[]) {
+  std::ofstream of;
+  if (argc > 1) {
+    of.open(argv[1]);
+  } else {
+    of.open("/dev/stdout");
+  }
+
+  // default false
+  std::array<bool, 256> charsAllowed{};
+  for (int i = 0; i < 256; ++i) {
+    charsAllowed[i] = false;
+  }
+
+  // configure allowed characters
+  charsAllowed['.'] = true;
+  charsAllowed['-'] = true;
+
+  for (auto ch = '0'; ch <= '9'; ++ch) {
+    charsAllowed[ch] = true;
+  }
+  for (auto ch = 'a'; ch <= 'z'; ++ch) {
+    charsAllowed[ch] = true;
+  }
+  for (auto ch = 'A'; ch <= 'Z'; ++ch) {
+    charsAllowed[ch] = true;
+  }
+  charsAllowed['~'] = true;
+  charsAllowed['^'] = true;
+
+  dump_array(of, "kAtlasChars", charsAllowed);
+}