From a4f05d0849358cbf8b070794290ba0b529240fa5 Mon Sep 17 00:00:00 2001 From: Ruslan Nigmatullin Date: Thu, 25 May 2023 18:57:31 -0700 Subject: [PATCH 01/12] Add AdaptingCircularBufferCounter for exponential histograms --- .../sdk/metrics/data/circular_buffer.h | 150 ++++++++++++++ sdk/src/metrics/data/circular_buffer.cc | 185 ++++++++++++++++++ sdk/test/metrics/BUILD | 15 ++ sdk/test/metrics/circular_buffer_test.cc | 137 +++++++++++++ 4 files changed, 487 insertions(+) create mode 100644 sdk/include/opentelemetry/sdk/metrics/data/circular_buffer.h create mode 100644 sdk/src/metrics/data/circular_buffer.cc create mode 100644 sdk/test/metrics/circular_buffer_test.cc diff --git a/sdk/include/opentelemetry/sdk/metrics/data/circular_buffer.h b/sdk/include/opentelemetry/sdk/metrics/data/circular_buffer.h new file mode 100644 index 0000000000..0980a6ffaa --- /dev/null +++ b/sdk/include/opentelemetry/sdk/metrics/data/circular_buffer.h @@ -0,0 +1,150 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +#pragma once + +#include "opentelemetry/nostd/variant.h" + +#include +#include + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace sdk +{ +namespace metrics +{ + +/** + * An integer array that automatically expands its memory consumption (via copy/allocation) when + * reaching limits. This assumes counts remain low, to lower memory overhead. + * + * This class is NOT thread-safe. It is expected to be behind a synchronized incrementer. + * + * Instances start by attempting to store one-byte per-cell in the integer array. As values grow, + * this will automatically instantiate the next-size integer array (byte => short => int => long) + * and copy over values into the larger array. This class expects most usage to remain within the + * uint8_t boundary (e.g. cell values < 256). + */ +class AdaptingIntegerArray +{ +public: + // Construct an adapting integer array of a given size. + explicit AdaptingIntegerArray(size_t size) : backing_(std::vector(size, 0)) {} + AdaptingIntegerArray(const AdaptingIntegerArray &other) = default; + AdaptingIntegerArray &operator=(const AdaptingIntegerArray &other) = default; + + /** + * Increments the value at the specified index by the given count in the array. + * + * @param index The index of the value to increment. + * @param count The count by which to increment the value. + */ + void Increment(size_t index, uint64_t count); + + /** + * Returns the value at the specified index from the array. + * + * @param index The index of the value to retrieve. + * @return The value at the specified index. + */ + uint64_t Get(size_t index) const; + + /** + * Returns the size of the array. + * + * @return The size of the array. + */ + size_t Size() const; + + /** + * Clears the array, resetting all values to zero. + */ + void Clear(); + +private: + void EnlargeToFit(uint64_t value); + + nostd::variant, + std::vector, + std::vector, + std::vector> + backing_; +}; + +/** + * A circle-buffer-backed exponential counter. + * + * The first recorded value becomes the 'base_index'. Going backwards leads to start/stop index. + * + * This expand start/end index as it sees values. + * + * This class is NOT thread-safe. It is expected to be behind a synchronized incrementer. + */ +class AdaptingCircularBufferCounter +{ +public: + explicit AdaptingCircularBufferCounter(size_t max_size) : backing_(max_size) {} + AdaptingCircularBufferCounter(const AdaptingCircularBufferCounter &other) = default; + AdaptingCircularBufferCounter &operator=(const AdaptingCircularBufferCounter &other) = default; + + /** + * The first index with a recording. May be negative. + * + * Note: the returned value is not meaningful when Empty returns true. + * + * @return the first index with a recording. + */ + size_t StartIndex() const { return start_index_; } + + /** + * The last index with a recording. May be negative. + * + * Note: the returned value is not meaningful when Empty returns true. + * + * @return The last index with a recording. + */ + size_t EndIndex() const { return end_index_; } + + /** + * Returns true if no recordings, false if at least one recording. + */ + bool Empty() const { return base_index_ == kNullIndex; } + + /** + * Returns the maximum number of buckets allowed in this counter. + */ + size_t MaxSize() const { return backing_.Size(); } + + /** Resets all bucket counts to zero and resets index start/end tracking. **/ + void Clear(); + + /** + * Persist new data at index, incrementing by delta amount. + * + * @param index The index of where to perform the incrementation. + * @param delta How much to increment the index by. + * @return success status. + */ + bool Increment(size_t index, uint64_t delta); + + /** + * Get the number of recordings for the given index. + * + * @return the number of recordings for the index, or 0 if the index is out of bounds. + */ + uint64_t Get(int index); + +private: + size_t ToBufferIndex(size_t index) const; + + static constexpr size_t kNullIndex = std::numeric_limits::max(); + + size_t start_index_ = kNullIndex; + size_t end_index_ = kNullIndex; + size_t base_index_ = kNullIndex; + AdaptingIntegerArray backing_; +}; + +} // namespace metrics +} // namespace sdk +OPENTELEMETRY_END_NAMESPACE \ No newline at end of file diff --git a/sdk/src/metrics/data/circular_buffer.cc b/sdk/src/metrics/data/circular_buffer.cc new file mode 100644 index 0000000000..e8248953be --- /dev/null +++ b/sdk/src/metrics/data/circular_buffer.cc @@ -0,0 +1,185 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +#include "opentelemetry/sdk/metrics/data/circular_buffer.h" + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace sdk +{ +namespace metrics +{ + +namespace +{ + +struct AdaptingIntegerArrayIncrement +{ + size_t index; + uint64_t count; + + template + uint64_t operator()(T &backing) + { + const auto current = backing[index]; + const uint64_t result = current + count; + OPENTELEMETRY_LIKELY_IF(result <= uint64_t(std::numeric_limits::max())) + { + backing[index] = result; + return 0; + } + return result; + } +}; + +struct AdaptingIntegerArrayGet +{ + size_t index; + + template + uint64_t operator()(T &backing) + { + return backing[index]; + } +}; + +struct AdaptingIntegerArraySize +{ + template + uint64_t operator()(T &backing) + { + return backing.size(); + } +}; + +struct AdaptingIntegerArrayClear +{ + template + void operator()(T &backing) + { + backing.assign(backing.size(), 0); + } +}; + +struct AdaptingIntegerArrayCopy +{ + template + void operator()(const T1 &from, T2 &to) + { + std::copy(from.begin(), from.end(), to.begin()); + } +}; + +} // namespace + +void AdaptingIntegerArray::Increment(size_t index, uint64_t count) +{ + const uint64_t result = nostd::visit(AdaptingIntegerArrayIncrement{index, count}, backing_); + OPENTELEMETRY_LIKELY_IF(result == 0) { return; } + EnlargeToFit(result); + Increment(index, count); +} + +uint64_t AdaptingIntegerArray::Get(size_t index) const +{ + return nostd::visit(AdaptingIntegerArrayGet{index}, backing_); +} + +size_t AdaptingIntegerArray::Size() const +{ + return nostd::visit(AdaptingIntegerArraySize{}, backing_); +} + +void AdaptingIntegerArray::Clear() +{ + nostd::visit(AdaptingIntegerArrayClear{}, backing_); +} + +void AdaptingIntegerArray::EnlargeToFit(uint64_t value) +{ + const size_t backing_size = Size(); + decltype(backing_) backing; + if (value <= std::numeric_limits::max()) + { + backing = std::vector(backing_size, 0); + } + else if (value <= std::numeric_limits::max()) + { + backing = std::vector(backing_size, 0); + } + else + { + backing = std::vector(backing_size, 0); + } + std::swap(backing_, backing); + nostd::visit(AdaptingIntegerArrayCopy{}, backing, backing_); +} + +void AdaptingCircularBufferCounter::Clear() +{ + start_index_ = kNullIndex; + end_index_ = kNullIndex; + base_index_ = kNullIndex; + backing_.Clear(); +} + +bool AdaptingCircularBufferCounter::Increment(size_t index, uint64_t delta) +{ + if (Empty()) + { + start_index_ = index; + end_index_ = index; + base_index_ = index; + backing_.Increment(0, delta); + return true; + } + + if (index > end_index_) + { + // Move end, check max size. + if (index + 1 > backing_.Size() + start_index_) + { + return false; + } + end_index_ = index; + } + else if (index < start_index_) + { + // Move end, check max size. + if (end_index_ + 1 > backing_.Size() + index) + { + return false; + } + start_index_ = index; + } + const size_t realIdx = ToBufferIndex(index); + backing_.Increment(realIdx, delta); + return true; +} + +uint64_t AdaptingCircularBufferCounter::Get(int index) +{ + if (index < start_index_ || index > end_index_) + { + return 0; + } + return backing_.Get(ToBufferIndex(index)); +} + +size_t AdaptingCircularBufferCounter::ToBufferIndex(size_t index) const +{ + // Figure out the index relative to the start of the circular buffer. + if (index < base_index_) + { + return index + backing_.Size() - base_index_; + } + size_t result = index - base_index_; + if (result >= backing_.Size()) + { + result -= backing_.Size(); + } + return result; +} + +} // namespace metrics +} // namespace sdk +OPENTELEMETRY_END_NAMESPACE \ No newline at end of file diff --git a/sdk/test/metrics/BUILD b/sdk/test/metrics/BUILD index d7ed9e5fb6..5f9cc6f635 100644 --- a/sdk/test/metrics/BUILD +++ b/sdk/test/metrics/BUILD @@ -270,6 +270,21 @@ cc_test( ], ) +cc_test( + name = "circular_buffer_test", + srcs = [ + "circular_buffer_test.cc", + ], + tags = [ + "metrics", + "test", + ], + deps = [ + "//sdk/src/metrics", + "@com_google_googletest//:gtest_main", + ], +) + cc_test( name = "histogram_aggregation_test", srcs = [ diff --git a/sdk/test/metrics/circular_buffer_test.cc b/sdk/test/metrics/circular_buffer_test.cc new file mode 100644 index 0000000000..64b9e18e76 --- /dev/null +++ b/sdk/test/metrics/circular_buffer_test.cc @@ -0,0 +1,137 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +#include "opentelemetry/sdk/metrics/data/circular_buffer.h" +#include +#include + +using namespace opentelemetry::sdk::metrics; +namespace nostd = opentelemetry::nostd; + +class AdaptingIntegerArrayTest : public testing::TestWithParam +{}; + +INSTANTIATE_TEST_SUITE_P(InterestingValues, + AdaptingIntegerArrayTest, + testing::Values(1, + std::numeric_limits::max() + 1ull, + std::numeric_limits::max() + 1ull, + std::numeric_limits::max() + 1ull)); + +TEST_P(AdaptingIntegerArrayTest, PreservesSizeOnEnlargement) +{ + AdaptingIntegerArray counter(10); + EXPECT_EQ(counter.Size(), 10); + counter.Increment(0, GetParam()); + EXPECT_EQ(counter.Size(), 10); +} + +TEST_P(AdaptingIntegerArrayTest, IncrementAndGet) +{ + AdaptingIntegerArray counter(10); + for (int idx = 0; idx < 10; idx += 1) + { + EXPECT_EQ(counter.Get(idx), 0); + counter.Increment(idx, 1); + EXPECT_EQ(counter.Get(idx), 1); + counter.Increment(idx, GetParam()); + EXPECT_EQ(counter.Get(idx), GetParam() + 1); + } +} + +TEST_P(AdaptingIntegerArrayTest, Copy) +{ + AdaptingIntegerArray counter(10); + counter.Increment(0, GetParam()); + EXPECT_EQ(counter.Get(0), GetParam()); + + AdaptingIntegerArray copy = counter; + EXPECT_EQ(copy.Get(0), GetParam()); + + counter.Increment(0, 1); + EXPECT_EQ(counter.Get(0), GetParam() + 1); + EXPECT_EQ(copy.Get(0), GetParam()); +} + +TEST_P(AdaptingIntegerArrayTest, Clear) +{ + AdaptingIntegerArray counter(10); + counter.Increment(0, GetParam()); + EXPECT_EQ(counter.Get(0), GetParam()); + + counter.Clear(); + counter.Increment(0, 1); + EXPECT_EQ(counter.Get(0), 1); +} + +class AdaptingCircularBufferCounterTest : public ::testing::Test +{ +protected: + AdaptingCircularBufferCounter newCounter(int maxBuckets) + { + return AdaptingCircularBufferCounter(maxBuckets); + } +}; + +TEST_F(AdaptingCircularBufferCounterTest, ReturnsZeroOutsidePopulatedRange) +{ + AdaptingCircularBufferCounter counter{10}; + EXPECT_EQ(counter.Get(0), 0); + EXPECT_EQ(counter.Get(100), 0); + counter.Increment(2, 1); + counter.Increment(99, 1); + EXPECT_EQ(counter.Get(0), 0); + EXPECT_EQ(counter.Get(100), 0); +} + +TEST_F(AdaptingCircularBufferCounterTest, ExpandLower) +{ + AdaptingCircularBufferCounter counter{160}; + EXPECT_TRUE(counter.Increment(10, 1)); + // Add BEFORE the initial see (array index 0) and make sure we wrap around the datastructure. + EXPECT_TRUE(counter.Increment(0, 1)); + EXPECT_EQ(counter.Get(10), 1); + EXPECT_EQ(counter.Get(0), 1); + EXPECT_EQ(counter.StartIndex(), 0); + EXPECT_EQ(counter.EndIndex(), 10); + // Add AFTER initial entry and just push back end. + EXPECT_TRUE(counter.Increment(20, 1)); + EXPECT_EQ(counter.Get(20), 1); + EXPECT_EQ(counter.Get(10), 1); + EXPECT_EQ(counter.Get(0), 1); + EXPECT_EQ(counter.StartIndex(), 0); + EXPECT_EQ(counter.EndIndex(), 20); +} + +TEST_F(AdaptingCircularBufferCounterTest, ShouldFailAtLimit) +{ + AdaptingCircularBufferCounter counter{160}; + EXPECT_TRUE(counter.Increment(0, 1)); + EXPECT_TRUE(counter.Increment(120, 1)); + // Check state + EXPECT_EQ(counter.StartIndex(), 0); + EXPECT_EQ(counter.EndIndex(), 120); + EXPECT_EQ(counter.Get(0), 1); + EXPECT_EQ(counter.Get(120), 1); + // Adding over the maximum # of buckets + EXPECT_FALSE(counter.Increment(3000, 1)); +} + +TEST_F(AdaptingCircularBufferCounterTest, ShouldCopyCounters) +{ + AdaptingCircularBufferCounter counter{2}; + EXPECT_TRUE(counter.Increment(2, 1)); + EXPECT_TRUE(counter.Increment(1, 1)); + EXPECT_FALSE(counter.Increment(3, 1)); + + AdaptingCircularBufferCounter copy{counter}; + EXPECT_EQ(counter.Get(2), 1); + EXPECT_EQ(copy.Get(2), 1); + EXPECT_EQ(copy.MaxSize(), counter.MaxSize()); + EXPECT_EQ(copy.StartIndex(), counter.StartIndex()); + EXPECT_EQ(copy.EndIndex(), counter.EndIndex()); + // Mutate copy and make sure original is unchanged. + EXPECT_TRUE(copy.Increment(2, 1)); + EXPECT_EQ(copy.Get(2), 2); + EXPECT_EQ(counter.Get(2), 1); +} \ No newline at end of file From 5b8cdb8be8a79909ae5066eda8ac0d6e25cf103a Mon Sep 17 00:00:00 2001 From: Ruslan Nigmatullin Date: Thu, 25 May 2023 19:08:02 -0700 Subject: [PATCH 02/12] do not use TEST_F unless needed --- sdk/test/metrics/circular_buffer_test.cc | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/sdk/test/metrics/circular_buffer_test.cc b/sdk/test/metrics/circular_buffer_test.cc index 64b9e18e76..114cab3ee2 100644 --- a/sdk/test/metrics/circular_buffer_test.cc +++ b/sdk/test/metrics/circular_buffer_test.cc @@ -64,16 +64,7 @@ TEST_P(AdaptingIntegerArrayTest, Clear) EXPECT_EQ(counter.Get(0), 1); } -class AdaptingCircularBufferCounterTest : public ::testing::Test -{ -protected: - AdaptingCircularBufferCounter newCounter(int maxBuckets) - { - return AdaptingCircularBufferCounter(maxBuckets); - } -}; - -TEST_F(AdaptingCircularBufferCounterTest, ReturnsZeroOutsidePopulatedRange) +TEST(AdaptingCircularBufferCounterTest, ReturnsZeroOutsidePopulatedRange) { AdaptingCircularBufferCounter counter{10}; EXPECT_EQ(counter.Get(0), 0); @@ -84,7 +75,7 @@ TEST_F(AdaptingCircularBufferCounterTest, ReturnsZeroOutsidePopulatedRange) EXPECT_EQ(counter.Get(100), 0); } -TEST_F(AdaptingCircularBufferCounterTest, ExpandLower) +TEST(AdaptingCircularBufferCounterTest, ExpandLower) { AdaptingCircularBufferCounter counter{160}; EXPECT_TRUE(counter.Increment(10, 1)); @@ -103,7 +94,7 @@ TEST_F(AdaptingCircularBufferCounterTest, ExpandLower) EXPECT_EQ(counter.EndIndex(), 20); } -TEST_F(AdaptingCircularBufferCounterTest, ShouldFailAtLimit) +TEST(AdaptingCircularBufferCounterTest, ShouldFailAtLimit) { AdaptingCircularBufferCounter counter{160}; EXPECT_TRUE(counter.Increment(0, 1)); @@ -117,7 +108,7 @@ TEST_F(AdaptingCircularBufferCounterTest, ShouldFailAtLimit) EXPECT_FALSE(counter.Increment(3000, 1)); } -TEST_F(AdaptingCircularBufferCounterTest, ShouldCopyCounters) +TEST(AdaptingCircularBufferCounterTest, ShouldCopyCounters) { AdaptingCircularBufferCounter counter{2}; EXPECT_TRUE(counter.Increment(2, 1)); From 59c9897cd068771741d23eb6a3909d78903992bd Mon Sep 17 00:00:00 2001 From: Ruslan Nigmatullin Date: Thu, 25 May 2023 23:13:03 -0700 Subject: [PATCH 03/12] add one more test and update CMakeLists.txt so that code coverage is aware about new code --- sdk/src/metrics/CMakeLists.txt | 1 + sdk/test/metrics/CMakeLists.txt | 1 + sdk/test/metrics/circular_buffer_test.cc | 20 +++++++++++++++++++- 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/sdk/src/metrics/CMakeLists.txt b/sdk/src/metrics/CMakeLists.txt index 46d6d67212..db8283fc43 100644 --- a/sdk/src/metrics/CMakeLists.txt +++ b/sdk/src/metrics/CMakeLists.txt @@ -17,6 +17,7 @@ add_library( aggregation/histogram_aggregation.cc aggregation/lastvalue_aggregation.cc aggregation/sum_aggregation.cc + data/circular_buffer.cc exemplar/filter.cc exemplar/reservoir.cc sync_instruments.cc) diff --git a/sdk/test/metrics/CMakeLists.txt b/sdk/test/metrics/CMakeLists.txt index c167a27dd5..2c4a7814c2 100644 --- a/sdk/test/metrics/CMakeLists.txt +++ b/sdk/test/metrics/CMakeLists.txt @@ -14,6 +14,7 @@ foreach( histogram_aggregation_test attributes_processor_test attributes_hashmap_test + circular_buffer_test histogram_test sync_metric_storage_counter_test sync_metric_storage_histogram_test diff --git a/sdk/test/metrics/circular_buffer_test.cc b/sdk/test/metrics/circular_buffer_test.cc index 114cab3ee2..e09e90ec0d 100644 --- a/sdk/test/metrics/circular_buffer_test.cc +++ b/sdk/test/metrics/circular_buffer_test.cc @@ -125,4 +125,22 @@ TEST(AdaptingCircularBufferCounterTest, ShouldCopyCounters) EXPECT_TRUE(copy.Increment(2, 1)); EXPECT_EQ(copy.Get(2), 2); EXPECT_EQ(counter.Get(2), 1); -} \ No newline at end of file +} + +TEST(AdaptingCircularBufferCounterTest, Clear) +{ + AdaptingCircularBufferCounter counter{10}; + EXPECT_TRUE(counter.Empty()); + EXPECT_TRUE(counter.Increment(2, 1)); + EXPECT_FALSE(counter.Empty()); + EXPECT_TRUE(counter.Increment(8, 1)); + // Check state. + EXPECT_EQ(counter.StartIndex(), 2); + EXPECT_EQ(counter.EndIndex(), 8); + EXPECT_EQ(counter.Get(2), 1); + EXPECT_EQ(counter.Get(8), 1); + // Clear and verify. + EXPECT_FALSE(counter.Empty()); + counter.Clear(); + EXPECT_TRUE(counter.Empty()); +} From 4d7f7691730e2e47bbb6352d8ee2c2587675f9a5 Mon Sep 17 00:00:00 2001 From: Ruslan Nigmatullin Date: Fri, 26 May 2023 00:19:38 -0700 Subject: [PATCH 04/12] rename test to avoid name conflict --- sdk/test/metrics/BUILD | 4 ++-- sdk/test/metrics/CMakeLists.txt | 2 +- ...ircular_buffer_test.cc => circular_buffer_counter_test.cc} | 0 3 files changed, 3 insertions(+), 3 deletions(-) rename sdk/test/metrics/{circular_buffer_test.cc => circular_buffer_counter_test.cc} (100%) diff --git a/sdk/test/metrics/BUILD b/sdk/test/metrics/BUILD index 5f9cc6f635..df56f9c67d 100644 --- a/sdk/test/metrics/BUILD +++ b/sdk/test/metrics/BUILD @@ -271,9 +271,9 @@ cc_test( ) cc_test( - name = "circular_buffer_test", + name = "circular_buffer_counter_test", srcs = [ - "circular_buffer_test.cc", + "circular_buffer_counter_test.cc", ], tags = [ "metrics", diff --git a/sdk/test/metrics/CMakeLists.txt b/sdk/test/metrics/CMakeLists.txt index 2c4a7814c2..47598fbbb8 100644 --- a/sdk/test/metrics/CMakeLists.txt +++ b/sdk/test/metrics/CMakeLists.txt @@ -14,7 +14,7 @@ foreach( histogram_aggregation_test attributes_processor_test attributes_hashmap_test - circular_buffer_test + circular_buffer_counter_test histogram_test sync_metric_storage_counter_test sync_metric_storage_histogram_test diff --git a/sdk/test/metrics/circular_buffer_test.cc b/sdk/test/metrics/circular_buffer_counter_test.cc similarity index 100% rename from sdk/test/metrics/circular_buffer_test.cc rename to sdk/test/metrics/circular_buffer_counter_test.cc From 553a6b7cb9a88d76120cfa8f9a0ac32c198fc515 Mon Sep 17 00:00:00 2001 From: Ruslan Nigmatullin Date: Fri, 26 May 2023 00:32:08 -0700 Subject: [PATCH 05/12] format fixes --- sdk/test/metrics/circular_buffer_counter_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/test/metrics/circular_buffer_counter_test.cc b/sdk/test/metrics/circular_buffer_counter_test.cc index e09e90ec0d..47734a2e17 100644 --- a/sdk/test/metrics/circular_buffer_counter_test.cc +++ b/sdk/test/metrics/circular_buffer_counter_test.cc @@ -2,11 +2,11 @@ // SPDX-License-Identifier: Apache-2.0 #include "opentelemetry/sdk/metrics/data/circular_buffer.h" + #include #include using namespace opentelemetry::sdk::metrics; -namespace nostd = opentelemetry::nostd; class AdaptingIntegerArrayTest : public testing::TestWithParam {}; From 773775ebe8f2c79d1723f99bdb2ed54c9f648843 Mon Sep 17 00:00:00 2001 From: Ruslan Nigmatullin Date: Fri, 26 May 2023 00:42:42 -0700 Subject: [PATCH 06/12] silence msvc --- sdk/src/metrics/data/circular_buffer.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/src/metrics/data/circular_buffer.cc b/sdk/src/metrics/data/circular_buffer.cc index e8248953be..cc137a2c2f 100644 --- a/sdk/src/metrics/data/circular_buffer.cc +++ b/sdk/src/metrics/data/circular_buffer.cc @@ -24,7 +24,7 @@ struct AdaptingIntegerArrayIncrement const uint64_t result = current + count; OPENTELEMETRY_LIKELY_IF(result <= uint64_t(std::numeric_limits::max())) { - backing[index] = result; + backing[index] = static_cast(result); return 0; } return result; From 12a445056aa47e819a91296ed45bc22203ceb333 Mon Sep 17 00:00:00 2001 From: Ruslan Nigmatullin Date: Fri, 26 May 2023 01:00:18 -0700 Subject: [PATCH 07/12] improve coverage a bit and simplify ToBufferIndex --- sdk/src/metrics/data/circular_buffer.cc | 28 ++++++++----------- .../metrics/circular_buffer_counter_test.cc | 19 +++++++------ 2 files changed, 22 insertions(+), 25 deletions(-) diff --git a/sdk/src/metrics/data/circular_buffer.cc b/sdk/src/metrics/data/circular_buffer.cc index cc137a2c2f..de92ca927b 100644 --- a/sdk/src/metrics/data/circular_buffer.cc +++ b/sdk/src/metrics/data/circular_buffer.cc @@ -18,13 +18,12 @@ struct AdaptingIntegerArrayIncrement uint64_t count; template - uint64_t operator()(T &backing) + uint64_t operator()(std::vector &backing) { - const auto current = backing[index]; - const uint64_t result = current + count; - OPENTELEMETRY_LIKELY_IF(result <= uint64_t(std::numeric_limits::max())) + const uint64_t result = backing[index] + count; + OPENTELEMETRY_LIKELY_IF(result <= uint64_t(std::numeric_limits::max())) { - backing[index] = static_cast(result); + backing[index] = static_cast(result); return 0; } return result; @@ -36,7 +35,7 @@ struct AdaptingIntegerArrayGet size_t index; template - uint64_t operator()(T &backing) + uint64_t operator()(const std::vector &backing) { return backing[index]; } @@ -45,7 +44,7 @@ struct AdaptingIntegerArrayGet struct AdaptingIntegerArraySize { template - uint64_t operator()(T &backing) + uint64_t operator()(const std::vector &backing) { return backing.size(); } @@ -54,7 +53,7 @@ struct AdaptingIntegerArraySize struct AdaptingIntegerArrayClear { template - void operator()(T &backing) + void operator()(std::vector &backing) { backing.assign(backing.size(), 0); } @@ -63,7 +62,7 @@ struct AdaptingIntegerArrayClear struct AdaptingIntegerArrayCopy { template - void operator()(const T1 &from, T2 &to) + void operator()(const std::vector &from, std::vector &to) { std::copy(from.begin(), from.end(), to.begin()); } @@ -151,8 +150,7 @@ bool AdaptingCircularBufferCounter::Increment(size_t index, uint64_t delta) } start_index_ = index; } - const size_t realIdx = ToBufferIndex(index); - backing_.Increment(realIdx, delta); + backing_.Increment(ToBufferIndex(index), delta); return true; } @@ -170,14 +168,10 @@ size_t AdaptingCircularBufferCounter::ToBufferIndex(size_t index) const // Figure out the index relative to the start of the circular buffer. if (index < base_index_) { + // If index is before the base one, wrap around. return index + backing_.Size() - base_index_; } - size_t result = index - base_index_; - if (result >= backing_.Size()) - { - result -= backing_.Size(); - } - return result; + return index - base_index_; } } // namespace metrics diff --git a/sdk/test/metrics/circular_buffer_counter_test.cc b/sdk/test/metrics/circular_buffer_counter_test.cc index 47734a2e17..a8218d9e6b 100644 --- a/sdk/test/metrics/circular_buffer_counter_test.cc +++ b/sdk/test/metrics/circular_buffer_counter_test.cc @@ -96,16 +96,19 @@ TEST(AdaptingCircularBufferCounterTest, ExpandLower) TEST(AdaptingCircularBufferCounterTest, ShouldFailAtLimit) { - AdaptingCircularBufferCounter counter{160}; - EXPECT_TRUE(counter.Increment(0, 1)); - EXPECT_TRUE(counter.Increment(120, 1)); + AdaptingCircularBufferCounter counter{10}; + EXPECT_TRUE(counter.Increment(10, 1)); + EXPECT_TRUE(counter.Increment(15, 2)); + EXPECT_TRUE(counter.Increment(6, 3)); // Check state - EXPECT_EQ(counter.StartIndex(), 0); - EXPECT_EQ(counter.EndIndex(), 120); - EXPECT_EQ(counter.Get(0), 1); - EXPECT_EQ(counter.Get(120), 1); + EXPECT_EQ(counter.StartIndex(), 6); + EXPECT_EQ(counter.EndIndex(), 15); + EXPECT_EQ(counter.Get(6), 3); + EXPECT_EQ(counter.Get(10), 1); + EXPECT_EQ(counter.Get(15), 2); // Adding over the maximum # of buckets - EXPECT_FALSE(counter.Increment(3000, 1)); + EXPECT_FALSE(counter.Increment(5, 1)); + EXPECT_FALSE(counter.Increment(16, 1)); } TEST(AdaptingCircularBufferCounterTest, ShouldCopyCounters) From e502a8b94bec0173e169f9296e642fe3cbe27e10 Mon Sep 17 00:00:00 2001 From: Ruslan Nigmatullin Date: Fri, 26 May 2023 01:02:53 -0700 Subject: [PATCH 08/12] replace erronous int by size_t & fix misleading comment --- .../opentelemetry/sdk/metrics/data/circular_buffer.h | 10 +++++----- sdk/src/metrics/data/circular_buffer.cc | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/data/circular_buffer.h b/sdk/include/opentelemetry/sdk/metrics/data/circular_buffer.h index 0980a6ffaa..80b179e319 100644 --- a/sdk/include/opentelemetry/sdk/metrics/data/circular_buffer.h +++ b/sdk/include/opentelemetry/sdk/metrics/data/circular_buffer.h @@ -21,9 +21,9 @@ namespace metrics * This class is NOT thread-safe. It is expected to be behind a synchronized incrementer. * * Instances start by attempting to store one-byte per-cell in the integer array. As values grow, - * this will automatically instantiate the next-size integer array (byte => short => int => long) - * and copy over values into the larger array. This class expects most usage to remain within the - * uint8_t boundary (e.g. cell values < 256). + * this will automatically instantiate the next-size integer array (uint8_t -> uint16_t -> uint32_t + * -> uint64_t) and copy over values into the larger array. This class expects most usage to remain + * within the uint8_t boundary (e.g. cell values < 256). */ class AdaptingIntegerArray { @@ -132,7 +132,7 @@ class AdaptingCircularBufferCounter * * @return the number of recordings for the index, or 0 if the index is out of bounds. */ - uint64_t Get(int index); + uint64_t Get(size_t index); private: size_t ToBufferIndex(size_t index) const; @@ -147,4 +147,4 @@ class AdaptingCircularBufferCounter } // namespace metrics } // namespace sdk -OPENTELEMETRY_END_NAMESPACE \ No newline at end of file +OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/src/metrics/data/circular_buffer.cc b/sdk/src/metrics/data/circular_buffer.cc index de92ca927b..49b0addb2b 100644 --- a/sdk/src/metrics/data/circular_buffer.cc +++ b/sdk/src/metrics/data/circular_buffer.cc @@ -154,7 +154,7 @@ bool AdaptingCircularBufferCounter::Increment(size_t index, uint64_t delta) return true; } -uint64_t AdaptingCircularBufferCounter::Get(int index) +uint64_t AdaptingCircularBufferCounter::Get(size_t index) { if (index < start_index_ || index > end_index_) { @@ -176,4 +176,4 @@ size_t AdaptingCircularBufferCounter::ToBufferIndex(size_t index) const } // namespace metrics } // namespace sdk -OPENTELEMETRY_END_NAMESPACE \ No newline at end of file +OPENTELEMETRY_END_NAMESPACE From 2dd5ed9780d4bc9fdea8197ca2fc860feebd40bc Mon Sep 17 00:00:00 2001 From: Ruslan Nigmatullin Date: Fri, 26 May 2023 15:45:00 -0700 Subject: [PATCH 09/12] hopefuly fix msvc a bit more --- sdk/src/metrics/data/circular_buffer.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/sdk/src/metrics/data/circular_buffer.cc b/sdk/src/metrics/data/circular_buffer.cc index 49b0addb2b..4bce412788 100644 --- a/sdk/src/metrics/data/circular_buffer.cc +++ b/sdk/src/metrics/data/circular_buffer.cc @@ -55,7 +55,7 @@ struct AdaptingIntegerArrayClear template void operator()(std::vector &backing) { - backing.assign(backing.size(), 0); + std::fill(backing.begin(), backing.end(), 0); } }; @@ -64,7 +64,10 @@ struct AdaptingIntegerArrayCopy template void operator()(const std::vector &from, std::vector &to) { - std::copy(from.begin(), from.end(), to.begin()); + for (size_t i = 0; i < from.size(); i++) + { + to[i] = static_cast(from[i]); + } } }; From 71cb32cb8ed91acea18c027e9b5ffa11e70fa814 Mon Sep 17 00:00:00 2001 From: Ruslan Nigmatullin Date: Wed, 31 May 2023 13:06:03 -0700 Subject: [PATCH 10/12] fix msvc, make index signed, add moving constructors --- .../sdk/metrics/data/circular_buffer.h | 22 +++++++++++-------- sdk/src/metrics/data/circular_buffer.cc | 14 ++++++------ 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/data/circular_buffer.h b/sdk/include/opentelemetry/sdk/metrics/data/circular_buffer.h index 80b179e319..d66f9b38b8 100644 --- a/sdk/include/opentelemetry/sdk/metrics/data/circular_buffer.h +++ b/sdk/include/opentelemetry/sdk/metrics/data/circular_buffer.h @@ -31,7 +31,9 @@ class AdaptingIntegerArray // Construct an adapting integer array of a given size. explicit AdaptingIntegerArray(size_t size) : backing_(std::vector(size, 0)) {} AdaptingIntegerArray(const AdaptingIntegerArray &other) = default; + AdaptingIntegerArray(AdaptingIntegerArray &&other) = default; AdaptingIntegerArray &operator=(const AdaptingIntegerArray &other) = default; + AdaptingIntegerArray &operator=(AdaptingIntegerArray &&other) = default; /** * Increments the value at the specified index by the given count in the array. @@ -85,7 +87,9 @@ class AdaptingCircularBufferCounter public: explicit AdaptingCircularBufferCounter(size_t max_size) : backing_(max_size) {} AdaptingCircularBufferCounter(const AdaptingCircularBufferCounter &other) = default; + AdaptingCircularBufferCounter(AdaptingCircularBufferCounter &&other) = default; AdaptingCircularBufferCounter &operator=(const AdaptingCircularBufferCounter &other) = default; + AdaptingCircularBufferCounter &operator=(AdaptingCircularBufferCounter &&other) = default; /** * The first index with a recording. May be negative. @@ -94,7 +98,7 @@ class AdaptingCircularBufferCounter * * @return the first index with a recording. */ - size_t StartIndex() const { return start_index_; } + int32_t StartIndex() const { return start_index_; } /** * The last index with a recording. May be negative. @@ -103,7 +107,7 @@ class AdaptingCircularBufferCounter * * @return The last index with a recording. */ - size_t EndIndex() const { return end_index_; } + int32_t EndIndex() const { return end_index_; } /** * Returns true if no recordings, false if at least one recording. @@ -125,23 +129,23 @@ class AdaptingCircularBufferCounter * @param delta How much to increment the index by. * @return success status. */ - bool Increment(size_t index, uint64_t delta); + bool Increment(int32_t index, uint64_t delta); /** * Get the number of recordings for the given index. * * @return the number of recordings for the index, or 0 if the index is out of bounds. */ - uint64_t Get(size_t index); + uint64_t Get(int32_t index); private: - size_t ToBufferIndex(size_t index) const; + size_t ToBufferIndex(int32_t index) const; - static constexpr size_t kNullIndex = std::numeric_limits::max(); + static constexpr int32_t kNullIndex = std::numeric_limits::min(); - size_t start_index_ = kNullIndex; - size_t end_index_ = kNullIndex; - size_t base_index_ = kNullIndex; + int32_t start_index_ = kNullIndex; + int32_t end_index_ = kNullIndex; + int32_t base_index_ = kNullIndex; AdaptingIntegerArray backing_; }; diff --git a/sdk/src/metrics/data/circular_buffer.cc b/sdk/src/metrics/data/circular_buffer.cc index 4bce412788..f9c44c0cdf 100644 --- a/sdk/src/metrics/data/circular_buffer.cc +++ b/sdk/src/metrics/data/circular_buffer.cc @@ -44,7 +44,7 @@ struct AdaptingIntegerArrayGet struct AdaptingIntegerArraySize { template - uint64_t operator()(const std::vector &backing) + size_t operator()(const std::vector &backing) { return backing.size(); } @@ -55,7 +55,7 @@ struct AdaptingIntegerArrayClear template void operator()(std::vector &backing) { - std::fill(backing.begin(), backing.end(), 0); + std::fill(backing.begin(), backing.end(), static_cast(0)); } }; @@ -124,7 +124,7 @@ void AdaptingCircularBufferCounter::Clear() backing_.Clear(); } -bool AdaptingCircularBufferCounter::Increment(size_t index, uint64_t delta) +bool AdaptingCircularBufferCounter::Increment(int32_t index, uint64_t delta) { if (Empty()) { @@ -157,7 +157,7 @@ bool AdaptingCircularBufferCounter::Increment(size_t index, uint64_t delta) return true; } -uint64_t AdaptingCircularBufferCounter::Get(size_t index) +uint64_t AdaptingCircularBufferCounter::Get(int32_t index) { if (index < start_index_ || index > end_index_) { @@ -166,15 +166,15 @@ uint64_t AdaptingCircularBufferCounter::Get(size_t index) return backing_.Get(ToBufferIndex(index)); } -size_t AdaptingCircularBufferCounter::ToBufferIndex(size_t index) const +size_t AdaptingCircularBufferCounter::ToBufferIndex(int32_t index) const { // Figure out the index relative to the start of the circular buffer. if (index < base_index_) { // If index is before the base one, wrap around. - return index + backing_.Size() - base_index_; + return static_cast(index + backing_.Size() - base_index_); } - return index - base_index_; + return static_cast(index - base_index_); } } // namespace metrics From 7f3687e98980848fa8e9e22d896bfeccb64d5822 Mon Sep 17 00:00:00 2001 From: Ruslan Nigmatullin Date: Wed, 31 May 2023 13:10:37 -0700 Subject: [PATCH 11/12] add comments --- .../opentelemetry/sdk/metrics/data/circular_buffer.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/data/circular_buffer.h b/sdk/include/opentelemetry/sdk/metrics/data/circular_buffer.h index d66f9b38b8..1608a3a011 100644 --- a/sdk/include/opentelemetry/sdk/metrics/data/circular_buffer.h +++ b/sdk/include/opentelemetry/sdk/metrics/data/circular_buffer.h @@ -143,9 +143,13 @@ class AdaptingCircularBufferCounter static constexpr int32_t kNullIndex = std::numeric_limits::min(); + // Index of the first populated element, may be kNullIndex if container is empty. int32_t start_index_ = kNullIndex; - int32_t end_index_ = kNullIndex; - int32_t base_index_ = kNullIndex; + // Index of the last populated element, may be kNullIndex if container is empty. + int32_t end_index_ = kNullIndex; + // Index corresponding to the element located at the start of the backing array, may be kNullIndex + // if container is empty. + int32_t base_index_ = kNullIndex; AdaptingIntegerArray backing_; }; From 4a34046d5a7913a533ff668b0a9a441931570816 Mon Sep 17 00:00:00 2001 From: Ruslan Nigmatullin Date: Wed, 31 May 2023 20:48:14 -0700 Subject: [PATCH 12/12] fix one more sign compiler warning --- sdk/src/metrics/data/circular_buffer.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/src/metrics/data/circular_buffer.cc b/sdk/src/metrics/data/circular_buffer.cc index f9c44c0cdf..9117e67ae5 100644 --- a/sdk/src/metrics/data/circular_buffer.cc +++ b/sdk/src/metrics/data/circular_buffer.cc @@ -138,7 +138,7 @@ bool AdaptingCircularBufferCounter::Increment(int32_t index, uint64_t delta) if (index > end_index_) { // Move end, check max size. - if (index + 1 > backing_.Size() + start_index_) + if (index + 1 > static_cast(backing_.Size()) + start_index_) { return false; } @@ -147,7 +147,7 @@ bool AdaptingCircularBufferCounter::Increment(int32_t index, uint64_t delta) else if (index < start_index_) { // Move end, check max size. - if (end_index_ + 1 > backing_.Size() + index) + if (end_index_ + 1 > static_cast(backing_.Size()) + index) { return false; }