8000 [METRICS SDK] Fix copying overflow attributes in attribute hashmap (#… · open-telemetry/opentelemetry-cpp@8f3ec92 · GitHub
[go: up one dir, main page]
Skip to content

Commit 8f3ec92

Browse files
ThomsonTanThomsonTan
authored
[METRICS SDK] Fix copying overflow attributes in attribute hashmap (#3651)
1 parent f788e35 commit 8f3ec92

File tree

3 files changed

+93
-6
lines changed

3 files changed

+93
-6
lines changed

‎CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,9 @@ Increment the:
3636
* [SDK] Implementing configurable aggregation cardinality limit
3737
[#3624](https://github.com/open-telemetry/opentelemetry-cpp/pull/3624)
3838

39+
* [SDK] Fix copying overflow attributes in metric AttributesHashMap
40+
[#3651](https://github.com/open-telemetry/opentelemetry-cpp/pull/3651)
41+
3942
Important changes:
4043

4144
* [CMAKE] Upgrade CMake minimum version to 3.16

‎sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h‎

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ class AttributesHashMapWithCustomHash
9595
return it->second.get();
9696
}
9797

98-
if (IsOverflowAttributes())
98+
if (IsOverflowAttributes(attr))
9999
{
100100
return GetOrSetOveflowAttributes(aggregation_callback);
101101
}
@@ -114,7 +114,7 @@ class AttributesHashMapWithCustomHash
114114
return it->second.get();
115115
}
116116

117-
if (IsOverflowAttributes())
117+
if (IsOverflowAttributes(attributes))
118118
{
119119
return GetOrSetOveflowAttributes(aggregation_callback);
120120
}
@@ -133,7 +133,7 @@ class AttributesHashMapWithCustomHash
133133
return it->second.get();
134134
}
135135

136-
if (IsOverflowAttributes())
136+
if (IsOverflowAttributes(attributes))
137137
{
138138
return GetOrSetOveflowAttributes(aggregation_callback);
139139
}
@@ -158,7 +158,7 @@ class AttributesHashMapWithCustomHash
158158
{
159159
it->second = std::move(aggr);
160160
}
161-
else if (IsOverflowAttributes())
161+
else if (IsOverflowAttributes(attributes))
162162
{
163163
hash_map_[kOverflowAttributes] = std::move(aggr);
164164
}
@@ -175,7 +175,7 @@ class AttributesHashMapWithCustomHash
175175
{
176176
it->second = std::move(aggr);
177177
}
178-
else 10000 if (IsOverflowAttributes())
178+
else if (IsOverflowAttributes(attributes))
179179
{
180180
hash_map_[kOverflowAttributes] = std::move(aggr);
181181
}
@@ -234,7 +234,27 @@ class AttributesHashMapWithCustomHash
234234
return result.first->second.get();
235235
}
236236

237-
bool IsOverflowAttributes() const { return (hash_map_.size() + 1 >= attributes_limit_); }
237+
bool IsOverflowAttributes(const MetricAttributes &attributes) const
238+
{
239+
// If the incoming attributes are exactly the overflow sentinel, route
240+
// directly to the overflow entry.
241+
if (attributes == kOverflowAttributes)
242+
{
243+
return true;
244+
}
245+
// Determine if overflow entry already exists.
246+
bool has_overflow = (hash_map_.find(kOverflowAttributes) != hash_map_.end());
247+
// If overflow already present, total size already includes it; trigger overflow
248+
// when current size (including overflow) is >= limit.
249+
if (has_overflow)
250+
{
251+
return hash_map_.size() >= attributes_limit_;
252+
}
253+
// If overflow not present yet, simulate adding a new distinct key. If that
254+
// would exceed the limit, we redirect to overflow instead of creating a
255+
// new real attribute entry.
256+
return (hash_map_.size() + 1) >= attributes_limit_;
257+
}
238258
};
239259

240260
using AttributesHashMap = AttributesHashMapWithCustomHash<>;

‎sdk/test/metrics/attributes_hashmap_test.cc‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,3 +158,67 @@ TEST(AttributesHashMap, HashConsistencyAcrossStringTypes)
158158
EXPECT_EQ(hash_c_str, hash_std_str_view);
159159
#endif
160160
}
161+
162+
TEST(AttributesHashMap, OverflowCardinalityLimitBehavior)
163+
{
164+
// Configure a very small limit to exercise overflow logic easily.
165+
const size_t limit = 4; // real attributes limit
166+
AttributesHashMapWithCustomHash<> map(limit);
167+
168+
// We expect to be able to insert exactly 'limit' distinct real attribute sets.
169+
// After that, further distinct attributes should route to the overflow bucket,
170+
// which should appear only once regardless of how many additional unique sets arrive.
171+
172+
// Insert distinct attributes up to the limit.
173+
for (size_t i = 0; i < limit; ++i)
174+
{
175+
MetricAttributes attr = {{"k", std::to_string(i)}};
176+
map.GetOrSetDefault(attr, []() { return std::unique_ptr<Aggregation>(new DropAggregation()); })
177+
->Aggregate(static_cast<int64_t>(1));
178+
}
179+
180+
// Size should be exactly 'limit' (no overflow yet)
181+
EXPECT_EQ(map.Size(), limit);
182+
183+
// Insert one more distinct attribute; this should not increase the real attributes count
184+
MetricAttributes overflow_trigger = {{"k", "overflow"}};
185+
map.GetOrSetDefault(overflow_trigger,
186+
[]() { return std::unique_ptr<Aggregation>(new DropAggregation()); })
187+
->Aggregate(static_cast<int64_t>(1));
188+
189+
EXPECT_EQ(map.Size(), limit);
190+
191+
// Insert several more unique attributes - size must remain constant (limit)
192+
for (size_t i = 0; i < limit - 1; ++i)
193+
{
194+
MetricAttributes extra_attr = {{"k", std::string("extra") + std::to_string(i)}};
195+
map.GetOrSetDefault(extra_attr,
196+
[]() { return std::unique_ptr<Aggregation>(new DropAggregation()); })
197+
->Aggregate(static_cast<int64_t>(1));
198+
}
199+
EXPECT_EQ(map.Size(), limit);
200+
201+
// Ensure overflow key was actually created and accessible via Get
202+
EXPECT_NE(map.Get(kOverflowAttributes), nullptr);
203+
204+
// Ensure original real attributes still present
205+
for (size_t i = 0; i < limit - 1; ++i)
206+
{
207+
MetricAttributes attr = {{"k", std::to_string(i)}};
208+
EXPECT_NE(map.Get(attr), nullptr);
6D3F 209+
}
210+
211+
// Copy the hash map to a new map in non-determistic order and verify all entries are present
212+
AttributesHashMapWithCustomHash<> map_copy(limit);
213+
map.GetAllEnteries([&map_copy](const MetricAttributes &attributes, Aggregation &) {
214+
map_copy.Set(attributes, std::unique_ptr<Aggregation>(new DropAggregation()));
215+
return true;
216+
});
217+
EXPECT_EQ(map_copy.Size(), map.Size());
218+
EXPECT_NE(map_copy.Get(kOverflowAttributes), nullptr);
219+
for (size_t i = 0; i < limit - 1; ++i)
220+
{
221+
MetricAttributes attr = {{"k", std::to_string(i)}};
222+
EXPECT_NE(map_copy.Get(attr), nullptr);
223+
}
224+
}

0 commit comments

Comments
 (0)
0