Skip to content

Commit d8ae09e

Browse files
lalitbThomsonTan
lalitb
and
ThomsonTan
authored
[Metrics SDK] Fix hash calculation for nostd::string (open-telemetry#2999)
* add test to validate cardinaity limit * format * warning * Format * maint mode CI * modify test to reproduce the issue * Format * remove vscode settings * remove redundant test * remove unused code * fix to calculate hash on string_view * remove iostream * template specialization for const char *, and varous string type tests * unused variable warning * more warnings * format * fix use-after-stack-scope * format * another try * format --------- Co-authored-by: Tom Tan <Tom.Tan@microsoft.com>
1 parent 323a279 commit d8ae09e

File tree

3 files changed

+122
-6
lines changed

3 files changed

+122
-6
lines changed

‎sdk/include/opentelemetry/sdk/common/attributemap_hash.h

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,15 @@ inline void GetHash(size_t &seed, const std::vector<T> &arg)
3535
}
3636
}
3737

38+
// Specialization for const char*
39+
// this creates an intermediate string.
40+
template <>
41+
inline void GetHash<const char *>(size_t &seed, const char *const &arg)
42+
{
43+
std::hash<std::string> hasher;
44+
seed ^= hasher(std::string(arg)) + 0x9e3779b9 + (seed << 6) + (seed >> 2);
45+
}
46+
3847
struct GetHashForAttributeValueVisitor
3948
{
4049
GetHashForAttributeValueVisitor(size_t &seed) : seed_(seed) {}
@@ -71,7 +80,7 @@ inline size_t GetHashForAttributeMap(
7180
{
7281
return true;
7382
}
74-
GetHash(seed, key.data());
83+
GetHash(seed, key);
7584
auto attr_val = nostd::visit(converter, value);
7685
nostd::visit(GetHashForAttributeValueVisitor(seed), attr_val);
7786
return true;

‎sdk/test/metrics/attributes_hashmap_test.cc

Lines changed: 75 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ namespace nostd = opentelemetry::nostd;
1313

1414
TEST(AttributesHashMap, BasicTests)
1515
{
16-
1716
// Empty map
1817
AttributesHashMap hash_map;
1918
EXPECT_EQ(hash_map.Size(), 0);
@@ -73,3 +72,78 @@ TEST(AttributesHashMap, BasicTests)
7372
});
7473
EXPECT_EQ(count, hash_map.Size());
7574
}
75+
76+
std::string make_unique_string(const char *str)
77+
{
78+
return std::string(str);
79+
}
80+
81+
TEST(AttributesHashMap, HashWithKeyValueIterable)
82+
{
83+
std::string key1 = make_unique_string("k1");
84+
std::string value1 = make_unique_string("v1");
85+
std::string key2 = make_unique_string("k2");
86+
std::string value2 = make_unique_string("v2");
87+
std::string key3 = make_unique_string("k3");
88+
std::string value3 = make_unique_string("v3");
89+
90+
// Create mock KeyValueIterable instances with the same content but different variables
91+
std::map<std::string, std::string> attributes1({{key1, value1}, {key2, value2}});
92+
std::map<std::string, std::string> attributes2({{key1, value1}, {key2, value2}});
93+
std::map<std::string, std::string> attributes3({{key1, value1}, {key2, value2}, {key3, value3}});
94+
95+
// Create a callback that filters "k3" key
96+
auto is_key_filter_k3_callback = [](nostd::string_view key) {
97+
if (key == "k3")
98+
{
99+
return false;
100+
}
101+
return true;
102+
};
103+
// Calculate hash
104+
size_t hash1 = opentelemetry::sdk::common::GetHashForAttributeMap(
105+
opentelemetry::common::KeyValueIterableView<std::map<std::string, std::string>>(attributes1),
106+
is_key_filter_k3_callback);
107+
size_t hash2 = opentelemetry::sdk::common::GetHashForAttributeMap(
108+
opentelemetry::common::KeyValueIterableView<std::map<std::string, std::string>>(attributes2),
109+
is_key_filter_k3_callback);
110+
111+
size_t hash3 = opentelemetry::sdk::common::GetHashForAttributeMap(
112+
opentelemetry::common::KeyValueIterableView<std::map<std::string, std::string>>(attributes3),
113+
is_key_filter_k3_callback);
114+
115+
// Expect the hashes to be the same because the content is the same
116+
EXPECT_EQ(hash1, hash2);
117+
// Expect the hashes to be the same because the content is the same
118+
EXPECT_EQ(hash1, hash3);
119+
}
120+
121+
TEST(AttributesHashMap, HashConsistencyAcrossStringTypes)
122+
{
123+
const char *c_str = "teststring";
124+
std::string std_str = "teststring";
125+
nostd::string_view nostd_str_view = "teststring";
126+
#if __cplusplus >= 201703L
127+
std::string_view std_str_view = "teststring";
128+
#endif
129+
130+
size_t hash_c_str = 0;
131+
size_t hash_std_str = 0;
132+
size_t hash_nostd_str_view = 0;
133+
#if __cplusplus >= 201703L
134+
size_t hash_std_str_view = 0;
135+
#endif
136+
137+
opentelemetry::sdk::common::GetHash(hash_c_str, c_str);
138+
opentelemetry::sdk::common::GetHash(hash_std_str, std_str);
139+
opentelemetry::sdk::common::GetHash(hash_nostd_str_view, nostd_str_view);
140+
#if __cplusplus >= 201703L
141+
opentelemetry::sdk::common::GetHash(hash_std_str_view, std_str_view);
142+
#endif
143+
144+
EXPECT_EQ(hash_c_str, hash_std_str);
145+
EXPECT_EQ(hash_c_str, hash_nostd_str_view);
146+
#if __cplusplus >= 201703L
147+
EXPECT_EQ(hash_c_str, hash_std_str_view);
148+
#endif
149+
}

‎sdk/test/metrics/cardinality_limit_test.cc

Lines changed: 37 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -47,14 +47,47 @@ TEST(CardinalityLimit, AttributesHashMapBasicTests)
4747
->Aggregate(record_value);
4848
}
4949
EXPECT_EQ(hash_map.Size(), 10); // only one more metric point should be added as overflow.
50+
// record 5 more measurements to already existing (and not-overflow) metric points. They
51+
// should get aggregated to these existing metric points.
52+
for (auto i = 0; i < 5; i++)
53+
{
54+
FilteredOrderedAttributeMap attributes = {{"key", std::to_string(i)}};
55+
auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(attributes);
56+
static_cast<LongSumAggregation *>(
57+
hash_map.GetOrSetDefault(attributes, aggregation_callback, hash))
58+
->Aggregate(record_value);
59+
}
60+
EXPECT_EQ(hash_map.Size(), 10); // no new metric point added
61+
5062
// get the overflow metric point
51-
auto agg = hash_map.GetOrSetDefault(
63+
auto agg1 = hash_map.GetOrSetDefault(
5264
FilteredOrderedAttributeMap({{kAttributesLimitOverflowKey, kAttributesLimitOverflowValue}}),
5365
aggregation_callback, kOverflowAttributesHash);
54-
EXPECT_NE(agg, nullptr);
55-
auto sum_agg = static_cast<LongSumAggregation *>(agg);
56-
EXPECT_EQ(nostd::get<int64_t>(nostd::get<SumPointData>(sum_agg->ToPoint()).value_),
66+
EXPECT_NE(agg1, nullptr);
67+
auto sum_agg1 = static_cast<LongSumAggregation *>(agg1);
68+
EXPECT_EQ(nostd::get<int64_t>(nostd::get<SumPointData>(sum_agg1->ToPoint()).value_),
5769
record_value * 6); // 1 from previous 10, 5 from current 5.
70+
// get remaining metric points
71+
for (auto i = 0; i < 9; i++)
72+
{
73+
FilteredOrderedAttributeMap attributes = {{"key", std::to_string(i)}};
74+
auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(attributes);
75+
auto agg2 = hash_map.GetOrSetDefault(
76+
FilteredOrderedAttributeMap({{kAttributesLimitOverflowKey, kAttributesLimitOverflowValue}}),
77+
aggregation_callback, hash);
78+
EXPECT_NE(agg2, nullptr);
79+
auto sum_agg2 = static_cast<LongSumAggregation *>(agg2);
80+
if (i < 5)
81+
{
82+
EXPECT_EQ(nostd::get<int64_t>(nostd::get<SumPointData>(sum_agg2->ToPoint()).value_),
83+
record_value * 2); // 1 from first recording, 1 from third recording
84+
}
85+
else
86+
{
87+
EXPECT_EQ(nostd::get<int64_t>(nostd::get<SumPointData>(sum_agg2->ToPoint()).value_),
88+
record_value); // 1 from first recording
89+
}
90+
}
5891
}
5992

6093
class WritableMetricStorageCardinalityLimitTestFixture

0 commit comments

Comments
 (0)