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([

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.h

@@ -17,8 +17,7 @@ class Tags {
  public:
   Tags() = default;
 
-  Tags(
-      std::initializer_list<std::pair<absl::string_view, absl::string_view>> vs) {
+  Tags(std::initializer_list<std::pair<absl::string_view, absl::string_view>> vs) {
     for (auto& pair : vs) {
       add(pair.first, pair.second);
     }

spectator/id_test.cc

@@ -8,9 +8,15 @@ using spectator::Tags;
 TEST(Id, Create) {
   Id id{"foo", Tags{}};
   EXPECT_EQ(id.Name(), "foo");
+  EXPECT_EQ(id.GetTags().size(), 0);
 
   Id id_tags{"name", Tags{{"k", "v"}, {"k1", "v1"}}};
+  EXPECT_EQ(id_tags.Name(), "name");
   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);
 }
 
 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

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);
+}