From 038f0bd70bb00d6bee559b4893254654f2676dfc Mon Sep 17 00:00:00 2001 From: Everett Badeaux Date: Mon, 19 Oct 2026 15:09:00 -0500 Subject: [PATCH] Update Tests --- libs/meter/meter_id/CMakeLists.txt | 6 +- libs/meter/meter_id/meter_id.cpp | 73 +++++++++++++++++------ libs/meter/meter_id/test_meter_id.cpp | 83 ++++++++++++++++++++++++++- 3 files changed, 143 insertions(+), 19 deletions(-) diff --git a/libs/meter/meter_id/CMakeLists.txt b/libs/meter/meter_id/CMakeLists.txt index c296453..a87df52 100644 --- a/libs/meter/meter_id/CMakeLists.txt +++ b/libs/meter/meter_id/CMakeLists.txt @@ -17,7 +17,9 @@ add_executable(MeterID-test ) target_link_libraries(MeterID-test PRIVATE - GTest::gtest + GTest::gtest GTest::gtest_main spectator-meter-id -) \ No newline at end of file +) + +add_test(NAME MeterID-test COMMAND MeterID-test) \ No newline at end of file diff --git a/libs/meter/meter_id/meter_id.cpp b/libs/meter/meter_id/meter_id.cpp index b266556..3a3c1a8 100644 --- a/libs/meter/meter_id/meter_id.cpp +++ b/libs/meter/meter_id/meter_id.cpp @@ -3,6 +3,8 @@ #include #include +#include +#include #include namespace spectator { @@ -35,13 +37,20 @@ void AppendSanitized(std::string& dest, const std::string& src) } } +// A tag is published only if neither its key nor its value is empty or whitespace-only. ValidateTags (GetTags()) and +// ToSpectatorId (the wire id) both use this, so the two always agree. +bool IsValidTag(const std::string& key, const std::string& value) +{ + return IsEmptyOrWhitespace(key) == false && IsEmptyOrWhitespace(value) == false; +} + std::unordered_map ValidateTags(const std::unordered_map& tags) { std::unordered_map validTags{}; for (const auto& [key, value] : tags) { - if (IsEmptyOrWhitespace(key) == false && IsEmptyOrWhitespace(value) == false) + if (IsValidTag(key, value)) { validTags[key] = value; } @@ -49,8 +58,12 @@ std::unordered_map ValidateTags(const std::unordered_m return validTags; } -std::string ToSpectatorId(const std::string& name, const std::unordered_map& tags) +// all_tags_valid is true when ValidateTags dropped nothing, so the common case renders the map without re-checking +// every tag. Otherwise the tags ValidateTags dropped are skipped here too. +std::string ToSpectatorId(const std::string& name, const std::unordered_map& tags, + bool all_tags_valid) { + // Upper bound: it also counts tags skipped below, which only over-reserves when a tag is invalid. size_t size = name.size(); for (const auto& [key, value] : tags) { @@ -62,6 +75,10 @@ std::string ToSpectatorId(const std::string& name, const std::unordered_map& tags) : m_name(name), m_tags(ValidateTags(tags)) { - m_spectatord_id = ToSpectatorId(name, tags); + // The id must not contain the tags ValidateTags dropped: rendering every raw tag would publish e.g. "k=" or "_=v" + // while GetTags() reports neither. It still walks the caller's map, not m_tags, so the tag order on the wire is + // unchanged for valid input. Keys are unique, so equal sizes mean nothing was dropped. + m_spectatord_id = ToSpectatorId(m_name, tags, m_tags.size() == tags.size()); } MeterId MeterId::WithTag(const std::string& key, const std::string& value) const @@ -104,10 +124,13 @@ bool MeterId::operator==(const MeterId& other) const { return m_name == other.m_ std::string MeterId::to_string() const { + // m_tags is unordered; sort so the output does not depend on the standard library's iteration order. + const std::map sorted_tags(m_tags.begin(), m_tags.end()); + std::ostringstream ss; ss << "MeterId(name=" << m_name << ", tags={"; bool first = true; - for (const auto& [fst, snd] : m_tags) + for (const auto& [fst, snd] : sorted_tags) { if (!first) { @@ -122,22 +145,40 @@ std::string MeterId::to_string() const } // namespace spectator -// Implementation of the hash function for MeterId +namespace { + +// splitmix64 finalizer. Spreads every input bit over the whole word so that summing many pair hashes below does not +// cancel or cluster structure from the individual std::hash values. +constexpr uint64_t Mix(uint64_t x) +{ + x ^= x >> 30; + x *= 0xbf58476d1ce4e5b9ULL; + x ^= x >> 27; + x *= 0x94d049bb133111ebULL; + x ^= x >> 31; + return x; +} + +constexpr uint64_t kGoldenRatio = 0x9e3779b97f4a7c15ULL; + +} // namespace + +// Implementation of the hash function for MeterId. +// operator== treats the tags as an unordered map, so equal ids can iterate their tags in different orders (different +// insertion order or bucket count). The hash must not depend on that order: each tag pair is hashed on its own and the +// results are summed, and addition is commutative. size_t std::hash::operator()(const spectator::MeterId& id) const { - // Hash the name first - const size_t name_hash = std::hash{}(id.GetName()); + const std::hash hash_string; - // Hash the tags - size_t tags_hash = 0; - for (const auto& [fst, snd] : id.GetTags()) + uint64_t tags_sum = 0; + for (const auto& [key, value] : id.GetTags()) { - // Combine key and value hashes - const size_t pair_hash = std::hash{}(fst) ^ std::hash{}(snd) << 1; - // Combine with the accumulated tags hash - tags_hash ^= pair_hash + 0x9e3779b9 + (tags_hash << 6) + (tags_hash >> 2); + // The multiply makes the pair hash differ for {a: b} and {b: a}. Mixing each pair before the sum keeps a key + // associated with its own value; summing raw key and value hashes would make {a: 1, b: 2} and {a: 2, b: 1} + // collide. + tags_sum += Mix(hash_string(key) + hash_string(value) * kGoldenRatio); } - // Combine name hash and tags hash - return name_hash ^ tags_hash << 1; + return static_cast(Mix(hash_string(id.GetName()) + kGoldenRatio + tags_sum)); } diff --git a/libs/meter/meter_id/test_meter_id.cpp b/libs/meter/meter_id/test_meter_id.cpp index 8bc9110..9f183d7 100644 --- a/libs/meter/meter_id/test_meter_id.cpp +++ b/libs/meter/meter_id/test_meter_id.cpp @@ -2,6 +2,10 @@ #include +#include +#include +#include + using namespace spectator; TEST(MeterIdTest, EqualsSameName) @@ -25,6 +29,56 @@ TEST(MeterIdTest, HashSameTags) EXPECT_EQ(std::hash{}(id1), std::hash{}(id2)); } +TEST(MeterIdTest, HashIgnoresTagOrderFromDifferentConstructionPaths) +{ + const MeterId id1 = MeterId("foo", {{"a", "1"}}).WithTag("b", "2"); + const MeterId id2 = MeterId("foo", {{"b", "2"}}).WithTag("a", "1"); + EXPECT_EQ(id1, id2); + EXPECT_EQ(std::hash{}(id1), std::hash{}(id2)); + + const std::unordered_set ids = {id1, id2}; + EXPECT_EQ(1u, ids.size()); +} + +TEST(MeterIdTest, HashMatchesEqualityForAnyInsertionOrder) +{ + // The same tags inserted in opposite order into maps with different bucket counts, so the maps iterate differently. + std::unordered_map forward; + forward.reserve(1); + forward.emplace("a", "1"); + forward.emplace("b", "2"); + forward.emplace("c", "3"); + forward.emplace("d", "4"); + forward.emplace("e", "5"); + forward.emplace("f", "6"); + + std::unordered_map backward; + backward.reserve(256); + backward.emplace("f", "6"); + backward.emplace("e", "5"); + backward.emplace("d", "4"); + backward.emplace("c", "3"); + backward.emplace("b", "2"); + backward.emplace("a", "1"); + + const MeterId id1("name", forward); + const MeterId id2("name", backward); + EXPECT_EQ(id1, id2); + EXPECT_EQ(std::hash{}(id1), std::hash{}(id2)); +} + +TEST(MeterIdTest, HashDistinguishesDifferentIds) +{ + const std::hash hash; + EXPECT_NE(hash(MeterId("foo")), hash(MeterId("bar"))); + EXPECT_NE(hash(MeterId("foo", {{"a", "1"}})), hash(MeterId("bar", {{"a", "1"}}))); + EXPECT_NE(hash(MeterId("foo", {{"a", "1"}})), hash(MeterId("foo", {{"a", "2"}}))); + EXPECT_NE(hash(MeterId("foo", {{"a", "1"}})), hash(MeterId("foo", {{"a", "1"}, {"b", "2"}}))); + // Key and value swapped, and values swapped between keys: a plain sum of key and value hashes would collide. + EXPECT_NE(hash(MeterId("foo", {{"a", "b"}})), hash(MeterId("foo", {{"b", "a"}}))); + EXPECT_NE(hash(MeterId("foo", {{"a", "1"}, {"b", "2"}})), hash(MeterId("foo", {{"a", "2"}, {"b", "1"}}))); +} + TEST(MeterIdTest, IllegalCharsAreReplaced) { const MeterId id("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo"); @@ -72,8 +126,35 @@ TEST(MeterIdTest, SpectatordId) MeterId id2("bar", {{"a", "1"}}); EXPECT_EQ("bar,a=1", id2.GetSpectatordId()); + // The order of tags follows the tag map's iteration order, which is unspecified, so accept either order. MeterId id3("baz", {{"a", "1"}, {"b", "2"}}); - EXPECT_EQ("baz,a=1,b=2", id3.GetSpectatordId()); + EXPECT_TRUE(id3.GetSpectatordId() == "baz,a=1,b=2" || id3.GetSpectatordId() == "baz,b=2,a=1") + << id3.GetSpectatordId(); +} + +TEST(MeterIdTest, SpectatordIdOmitsTagsThatValidationDrops) +{ + // Empty or whitespace-only keys and values are dropped from GetTags(), so they must not reach the wire either. + const MeterId id("foo", {{"a", ""}, {"b", "2"}, {" ", "x"}, {"c", " "}}); + const std::unordered_map expected = {{"b", "2"}}; + EXPECT_EQ(expected, id.GetTags()); + EXPECT_EQ("foo,b=2", id.GetSpectatordId()); +} + +TEST(MeterIdTest, WithTagAndWithTagsDropTagsSetToBlank) +{ + const MeterId id("foo", {{"a", "1"}, {"b", "2"}, {"c", "3"}}); + + const MeterId without_a = id.WithTag("a", ""); + const std::unordered_map expected_without_a = {{"b", "2"}, {"c", "3"}}; + EXPECT_EQ(expected_without_a, without_a.GetTags()); + EXPECT_TRUE(without_a.GetSpectatordId() == "foo,b=2,c=3" || without_a.GetSpectatordId() == "foo,c=3,b=2") + << without_a.GetSpectatordId(); + + const MeterId without_bc = id.WithTags({{"b", " "}, {"c", ""}}); + const std::unordered_map expected_without_bc = {{"a", "1"}}; + EXPECT_EQ(expected_without_bc, without_bc.GetTags()); + EXPECT_EQ("foo,a=1", without_bc.GetSpectatordId()); } TEST(MeterIdTest, ToString)