From 3c1850257164e4312a2a0494855061c0e5c71d66 Mon Sep 17 00:00:00 2001 From: Matthew Johnson Date: Mon, 15 Jul 2024 17:40:15 -0400 Subject: [PATCH] replace invalid characters in ids 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 | 1 + CMakeLists.txt | 11 ++++++ build.sh | 7 +++- spectator/age_gauge_test.cc | 8 ++++ spectator/counter_test.cc | 14 +++++-- spectator/dist_summary_test.cc | 8 ++++ spectator/gauge_test.cc | 8 ++++ spectator/id_test.cc | 1 + spectator/max_gauge_test.cc | 8 ++++ spectator/monotonic_counter_test.cc | 9 +++++ spectator/monotonic_counter_uint_test.cc | 9 +++++ spectator/perc_dist_summary_test.cc | 16 ++++++-- spectator/perc_timer_test.cc | 14 +++++-- spectator/stateless_meters.h | 35 ++++++++++++++++- spectator/timer_test.cc | 15 ++++++-- tools/gen_valid_chars.cc | 48 ++++++++++++++++++++++++ 16 files changed, 193 insertions(+), 19 deletions(-) create mode 100644 tools/gen_valid_chars.cc diff --git a/.gitignore b/.gitignore index 96e66f4..8d9970f 100644 --- a/.gitignore +++ b/.gitignore @@ -2,4 +2,5 @@ .idea/ cmake-build-debug cmake-build/ +spectator/valid_chars.inc venv/ diff --git a/CMakeLists.txt b/CMakeLists.txt index 637d797..3f2c869 100644 --- a/CMakeLists.txt +++ b/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 "$<$:-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 +) diff --git a/build.sh b/build.sh index aed9dfd..aed55f7 100755 --- a/build.sh +++ b/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 diff --git a/spectator/age_gauge_test.cc b/spectator/age_gauge_test.cc index cab82af..844e1f9 100644 --- a/spectator/age_gauge_test.cc +++ b/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("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo", + Tags{{"tag1,:=", "value1,:="}}); + AgeGauge g{id, &publisher}; + EXPECT_EQ("A:test______^____-_~______________.___foo,tag1___=value1___:", g.GetPrefix()); +} } // namespace diff --git a/spectator/counter_test.cc b/spectator/counter_test.cc index 2727d54..91267ce 100644 --- a/spectator/counter_test.cc +++ b/spectator/counter_test.cc @@ -13,8 +13,8 @@ TEST(Counter, Activity) { TestPublisher publisher; auto id = std::make_shared("ctr.name", Tags{}); auto id2 = std::make_shared("c2", Tags{{"key", "val"}}); - Counter c{id, &publisher}; - Counter 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 c{std::make_shared("foo", Tags{{"key", "val"}}), + Counter c{std::make_shared("foo", Tags{{"key", "val"}}), &publisher}; auto id = std::make_shared("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("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo", + Tags{{"tag1,:=", "value1,:="}}); + Counter c{id, &publisher}; + EXPECT_EQ("c:test______^____-_~______________.___foo,tag1___=value1___:", c.GetPrefix()); +} } // namespace diff --git a/spectator/dist_summary_test.cc b/spectator/dist_summary_test.cc index 7202b1b..ed8eec5 100644 --- a/spectator/dist_summary_test.cc +++ b/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("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo", + Tags{{"tag1,:=", "value1,:="}}); + DistributionSummary d{id, &publisher}; + EXPECT_EQ("d:test______^____-_~______________.___foo,tag1___=value1___:", d.GetPrefix()); +} } // namespace diff --git a/spectator/gauge_test.cc b/spectator/gauge_test.cc index 014910e..d92e11a 100644 --- a/spectator/gauge_test.cc +++ b/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("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo", + Tags{{"tag1,:=", "value1,:="}}); + Gauge g{id, &publisher}; + EXPECT_EQ("g:test______^____-_~______________.___foo,tag1___=value1___:", g.GetPrefix()); +} } // namespace diff --git a/spectator/id_test.cc b/spectator/id_test.cc index 35a53ec..5e03976 100644 --- a/spectator/id_test.cc +++ b/spectator/id_test.cc @@ -17,6 +17,7 @@ TEST(Id, Create) { std::shared_ptr 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) { diff --git a/spectator/max_gauge_test.cc b/spectator/max_gauge_test.cc index f769b4d..10c5bac 100644 --- a/spectator/max_gauge_test.cc +++ b/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("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo", + Tags{{"tag1,:=", "value1,:="}}); + MaxGauge g{id, &publisher}; + EXPECT_EQ("m:test______^____-_~______________.___foo,tag1___=value1___:", g.GetPrefix()); +} } // namespace diff --git a/spectator/monotonic_counter_test.cc b/spectator/monotonic_counter_test.cc index ec91a9f..3cf8e39 100644 --- a/spectator/monotonic_counter_test.cc +++ b/spectator/monotonic_counter_test.cc @@ -22,4 +22,13 @@ TEST(MonotonicCounter, Set) { std::vector 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("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo", + Tags{{"tag1,:=", "value1,:="}}); + MonotonicCounter c{id, &publisher}; + EXPECT_EQ("C:test______^____-_~______________.___foo,tag1___=value1___:", c.GetPrefix()); +} } // namespace diff --git a/spectator/monotonic_counter_uint_test.cc b/spectator/monotonic_counter_uint_test.cc index 564f7e4..d3f55f2 100644 --- a/spectator/monotonic_counter_uint_test.cc +++ b/spectator/monotonic_counter_uint_test.cc @@ -22,4 +22,13 @@ TEST(MonotonicCounterUint, Set) { std::vector 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("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo", + Tags{{"tag1,:=", "value1,:="}}); + MonotonicCounterUint c{id, &publisher}; + EXPECT_EQ("U:test______^____-_~______________.___foo,tag1___=value1___:", c.GetPrefix()); +} } // namespace \ No newline at end of file diff --git a/spectator/perc_dist_summary_test.cc b/spectator/perc_dist_summary_test.cc index 5f9df29..86b730e 100644 --- a/spectator/perc_dist_summary_test.cc +++ b/spectator/perc_dist_summary_test.cc @@ -11,13 +11,21 @@ using spectator::TestPublisher; TEST(PercDistSum, Record) { TestPublisher publisher; auto id = std::make_shared("pds", Tags{}); - PercentileDistributionSummary 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 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("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo", + Tags{{"tag1,:=", "value1,:="}}); + PercentileDistributionSummary d{id, &publisher, 0, 1000}; + EXPECT_EQ("D:test______^____-_~______________.___foo,tag1___=value1___:", d.GetPrefix()); +} } // namespace diff --git a/spectator/perc_timer_test.cc b/spectator/perc_timer_test.cc index 9efa7de..6301349 100644 --- a/spectator/perc_timer_test.cc +++ b/spectator/perc_timer_test.cc @@ -11,14 +11,20 @@ using spectator::TestPublisher; TEST(PercentileTimer, Record) { TestPublisher publisher; auto id = std::make_shared("pt", Tags{}); - PercentileTimer 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 expected = {"T:pt:0.042", "T:pt:0.0005", - "T:pt:5"}; + std::vector 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("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo", + Tags{{"tag1,:=", "value1,:="}}); + PercentileTimer t{id, &publisher, absl::ZeroDuration(), absl::Seconds(5)}; + EXPECT_EQ("T:test______^____-_~______________.___foo,tag1___=value1___:", t.GetPrefix()); +} } // namespace diff --git a/spectator/stateless_meters.h b/spectator/stateless_meters.h index 5c344af..df12eed 100644 --- a/spectator/stateless_meters.h +++ b/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; diff --git a/spectator/timer_test.cc b/spectator/timer_test.cc index ed5f92e..0f380e5 100644 --- a/spectator/timer_test.cc +++ b/spectator/timer_test.cc @@ -12,14 +12,21 @@ TEST(Timer, Record) { TestPublisher publisher; auto id = std::make_shared("t.name", Tags{}); auto id2 = std::make_shared("t2", Tags{{"key", "val"}}); - Timer t{id, &publisher}; - Timer 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 expected = { - "t:t.name:0.001", "t:t2,key=val:0.1", "t:t2,key=val:0.0005"}; + std::vector 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("timer`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo", + Tags{{"tag1,:=", "value1,:="}}); + Timer t{id, &publisher}; + EXPECT_EQ("t:timer______^____-_~______________.___foo,tag1___=value1___:", t.GetPrefix()); +} } // namespace diff --git a/tools/gen_valid_chars.cc b/tools/gen_valid_chars.cc new file mode 100644 index 0000000..ab046f0 --- /dev/null +++ b/tools/gen_valid_chars.cc @@ -0,0 +1,48 @@ +// generate the atlas valid charsets + +#include +#include + +void dump_array(std::ostream& os, const std::string& name, const std::array& chars) { + os << "static constexpr std::array " << 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 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); +}