Update Tests

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
-)
+)
+
+add_test(NAME MeterID-test COMMAND MeterID-test)

libs/meter/meter_id/meter_id.cpp

@@ -3,6 +3,8 @@
 #include <util.h>
 
 #include <array>
+#include <cstdint>
+#include <map>
 #include <sstream>
 
 namespace spectator {
@@ -35,22 +37,33 @@ 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<std::string, std::string> ValidateTags(const std::unordered_map<std::string, std::string>& tags)
 {
     std::unordered_map<std::string, std::string> validTags{};
 
     for (const auto& [key, value] : tags)
     {
-        if (IsEmptyOrWhitespace(key) == false && IsEmptyOrWhitespace(value) == false)
+        if (IsValidTag(key, value))
         {
             validTags[key] = value;
         }
     }
     return validTags;
 }
 
-std::string ToSpectatorId(const std::string& name, const std::unordered_map<std::string, std::string>& 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<std::string, std::string>& 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<std:
     AppendSanitized(id, name);
     for (const auto& [key, value] : tags)
     {
+        if (all_tags_valid == false && IsValidTag(key, value) == false)
+        {
+            continue;
+        }
         id += ',';
         AppendSanitized(id, key);
         id += '=';
@@ -75,7 +92,10 @@ std::string ToSpectatorId(const std::string& name, const std::unordered_map<std:
 MeterId::MeterId(const std::string& name, const std::unordered_map<std::string, std::string>& 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<std::string, std::string> 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<std::string> 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<spectator::MeterId>::operator()(const spectator::MeterId& id) const
 {
-    // Hash the name first
-    const size_t name_hash = std::hash<std::string>{}(id.GetName());
+    const std::hash<std::string> 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<std::string>{}(fst) ^ std::hash<std::string>{}(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<size_t>(Mix(hash_string(id.GetName()) + kGoldenRatio + tags_sum));
 }

libs/meter/meter_id/test_meter_id.cpp

@@ -2,6 +2,10 @@
 
 #include <gtest/gtest.h>
 
+#include <string>
+#include <unordered_map>
+#include <unordered_set>
+
 using namespace spectator;
 
 TEST(MeterIdTest, EqualsSameName)
@@ -25,6 +29,56 @@ TEST(MeterIdTest, HashSameTags)
     EXPECT_EQ(std::hash<MeterId>{}(id1), std::hash<MeterId>{}(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<MeterId>{}(id1), std::hash<MeterId>{}(id2));
+
+    const std::unordered_set<MeterId> 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<std::string, std::string> 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<std::string, std::string> 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<MeterId>{}(id1), std::hash<MeterId>{}(id2));
+}
+
+TEST(MeterIdTest, HashDistinguishesDifferentIds)
+{
+    const std::hash<MeterId> 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<std::string, std::string> 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<std::string, std::string> 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<std::string, std::string> expected_without_bc = {{"a", "1"}};
+    EXPECT_EQ(expected_without_bc, without_bc.GetTags());
+    EXPECT_EQ("foo,a=1", without_bc.GetSpectatordId());
 }
 
 TEST(MeterIdTest, ToString)