From 5ff10e2a616b119fa0380e7ec54df548b2b1cd74 Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Wed, 15 Jul 2020 11:36:14 -0400 Subject: [PATCH 01/30] templated aggregators --- .../sdk/metrics/aggregator/aggregator.h | 103 +++++++++++ .../metrics/aggregator/counter_aggregator.h | 104 ++++++++++++ .../metrics/aggregator/histogram_aggregator.h | 160 ++++++++++++++++++ .../aggregator/min_max_sum_count_aggregator.h | 120 +++++++++++++ 4 files changed, 487 insertions(+) create mode 100644 sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h create mode 100644 sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h create mode 100644 sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h create mode 100644 sdk/include/opentelemetry/sdk/metrics/aggregator/min_max_sum_count_aggregator.h diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h new file mode 100644 index 0000000000..00ed434f07 --- /dev/null +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h @@ -0,0 +1,103 @@ +#pragma once + +#include "opentelemetry/version.h" +#include "opentelemetry/metrics/instrument.h" + +#include +#include +#include +#include + +namespace metrics_api = opentelemetry::metrics; + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace sdk +{ +namespace metrics +{ +/* + * Performs calculations necessary to combine updates from instruments into an + * insightful value. + * Also stores current instrument values and checkpoints collected at intervals + * governing the entire pipeline. + */ +template +class Aggregator +{ +public: + + Aggregator() = default; + + /** + * Recieves a captured value from the instrument and applies it to the current aggregator value. + * + * @param val, the raw value used in aggregation + * @return none + */ + virtual void update(T val) = 0; + + /** + * Checkpoints the current value. This function will overwrite the current checkpoint with the + * current value. + * + * @param none + * @return none + */ + virtual void checkpoint() = 0; + + /** + * Merges the values of two aggregators in a semantically accurate manner. + * Merging will occur differently for different aggregators depending on the + * way values are tracked. + * + * @param other, the aggregator with merge with + * @return none + */ + void merge(std::shared_ptr other); + + /** + * Returns the checkpointed value + * + * @param none + * @return the value of the checkpoint + */ + virtual std::vector get_checkpoint() = 0; + + /** + * Returns the current value + * + * @param none + * @return the present aggregator value + */ + virtual std::vector get_values() = 0; + + /** + * Returns the instrument kind which this aggregator is associated with + * + * @param none + * @return the BoundInstrumentKind of the aggregator's owner + */ + virtual opentelemetry::metrics::BoundInstrumentKind get_kind() + { + return kind_; + } + + // Custom copy constructor to handle the mutex + Aggregator(const Aggregator &cp) { + values_ = cp.values_; + checkpoint_ = cp.checkpoint_; + kind_ = cp.kind_; + // use default initialized mutex as they cannot be copied + } + +protected: + std::vector values_; + std::vector checkpoint_; + opentelemetry::metrics::BoundInstrumentKind kind_; + std::mutex mu_; + +}; + +} // namespace metrics +} // namespace sdk +OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h new file mode 100644 index 0000000000..8d50ed830d --- /dev/null +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h @@ -0,0 +1,104 @@ +#pragma once + +#include "opentelemetry/version.h" +#include "opentelemetry/metrics/instrument.h" +#include "opentelemetry/sdk/metrics/aggregator/aggregator.h" + +#include +#include +#include +#include + +namespace metrics_api = opentelemetry::metrics; + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace sdk +{ +namespace metrics +{ + +template +class CounterAggregator : public Aggregator +{ + +public: + CounterAggregator(metrics_api::BoundInstrumentKind kind) + { + this->kind_ = kind; + this->values_ = std::vector(1, 0); + this->checkpoint_ = std::vector(1, 0); + } + + /** + * Recieves a captured value from the instrument and applies it to the current aggregator value. + * + * @param val, the raw value used in aggregation + * @return none + */ + void update(T val) override + { + this->mu_.lock(); + this->values_[0] += val; // atomic operation + this->mu_.unlock(); + } + + /** + * Checkpoints the current value. This function will overwrite the current checkpoint with the + * current value. + * + * @param none + * @return none + */ + void checkpoint() override + { + this->checkpoint_ = this->values_; + this->values_[0] = 0; + } + + /** + * Merges the values of two aggregators in a semantically accurate manner. + * In this case, merging only requires the the current values of the two aggregators be summed. + * + * @param other, the aggregator with merge with + * @return none + */ + void merge(CounterAggregator other) + { + if (this->kind_ == other.kind_) + { + this->mu_.lock(); + this->values_[0] += other.values_[0]; // atomic operation afaik + this->mu_.unlock(); + } + else + { + //AggregatorMismatch Exception + } + } + + /** + * Returns the checkpointed value + * + * @param none + * @return the value of the checkpoint + */ + std::vector get_checkpoint() override + { + return this->checkpoint_; + } + + /** + * Returns the instrument kind which this aggregator is associated with + * + * @param none + * @return the BoundInstrumentKind of the aggregator's owner + */ + std::vector get_values() override{ + return this->values_; + } + +}; + +} +} +OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h new file mode 100644 index 0000000000..b0d705317d --- /dev/null +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h @@ -0,0 +1,160 @@ +#pragma once + +#include "opentelemetry/version.h" +#include "opentelemetry/metrics/instrument.h" +#include "opentelemetry/sdk/metrics/aggregator/aggregator.h" + +#include +#include +#include +#include + +namespace metrics_api = opentelemetry::metrics; + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace sdk +{ +namespace metrics +{ +template +class HistogramAggregator : public Aggregator +{ + +public: + + /** + * Constructor for the histogram aggregator. A sorted vector of boundaries is expected and boundaries are + * doubles regardless of the aggregator's templated data type. + * + * Sum is stored in values_[0] + * Count is stored in position_[1] + */ + HistogramAggregator(metrics_api::BoundInstrumentKind kind, std::vector boundaries) + { + this->kind_ = kind; + boundaries_ = boundaries; + this->values_ = std::vector(2, 0); + this->checkpoint_ = std::vector(2, 0); + bucketCounts_ = std::vector(boundaries_.size() + 1, 0); + } + + /** + * Recieves a captured value from the instrument and inserts it into the current histogram counts. + * Depending on the use case, linear search or binary search based implementations may be preferred. + * In a uniformly distributed dataset, linear search outperforms binary search until 512 buckets. + * + * @param val, the raw value used in aggregation + * @return none + */ + void update(T val) override + { + + int bucketID = boundaries_.size(); + for (int i = 0; i < boundaries_.size(); i++) + { + if (val < boundaries_[i]) // concurrent read is thread-safe + { + bucketID = i; + break; + } + } + + // Alternate implementation with binary search + //auto pos = std::lower_bound (boundaries_.begin(), boundaries_.end(), val); + //bucketCounts_[pos-boundaries_.begin()] += 1; + + this->mu_.lock(); + this->values_[0] += val; + this->values_[1] += 1; + bucketCounts_[bucketID] += 1; + this->mu_.unlock(); + } + + /** + * Checkpoints the current value. This function will overwrite the current checkpoint with the + * current value. + * + * @param none + * @return none + */ + void checkpoint() override + { + this->checkpoint_ = this->values_; + this->values_[0]=0; + this->values_[1]=0; + } + + /** + * Merges the values of two aggregators in a semantically accurate manner. + * A histogram aggregator can only be merged with another histogram aggregatos with the same boudnaries. + * A histogram merge first adds the sum and count values then iterates over the adds the bucket counts + * element by element. + * + * @param other, the aggregator with merge with + * @return none + */ + void merge(HistogramAggregator other) + { + this->mu_.lock(); + this->values_[0] += other.values_[0]; + this->values_[1] += other.values_[1]; + + for (int i = 0; i < bucketCounts_.size(); i++) + { + bucketCounts_[i] += other.bucketCounts_[i]; + } + this->mu_.unlock(); + } + + /** + * Returns the checkpointed value + * + * @param none + * @return the value of the checkpoint + */ + std::vector get_checkpoint() override + { + return this->checkpoint_; // what happens if there isn't a checkpoint? + } + + /** + * Returns the current value + * + * @param none + * @return the present aggregator value + */ + std::vector get_values() override + { + return this->values_; + } + + /** + * Returns the bucket boundaries specified at this aggregator's creation. + * + * @param none + * @return the aggregator boundaries + */ + std::vector get_boundaries() + { + return bucketCounts_; + } + + /** + * Returns the current counts for each bucket . + * + * @param none + * @return the aggregator bucket counts + */ + std::vector get_counts() + { + return bucketCounts_; + } + +private: + std::vector boundaries_; + std::vector bucketCounts_; +}; + +} +} +OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/min_max_sum_count_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/min_max_sum_count_aggregator.h new file mode 100644 index 0000000000..298abba441 --- /dev/null +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/min_max_sum_count_aggregator.h @@ -0,0 +1,120 @@ +#pragma once + +#include "opentelemetry/version.h" +#include "opentelemetry/metrics/instrument.h" +#include "opentelemetry/sdk/metrics/aggregator/aggregator.h" + +#include +#include +#include +#include + +namespace metrics_api = opentelemetry::metrics; + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace sdk +{ +namespace metrics +{ +template +class MinMaxSumCountAggregator : public Aggregator +{ +public: + explicit MinMaxSumCountAggregator(metrics_api::BoundInstrumentKind kind) + { + static_assert(std::is_arithmetic::value, "Not an arithmetic type"); + this->kind_ = kind; + this->values_ = std::vector(4, 0); // {min, max, sum count} + this->checkpoint_ = this->values_; + } + + /** + * Receives a captured value from the instrument and applies it to the current aggregator value. + * + * @param val, the raw value used in aggregation + */ + void update(T val) override + { + this->mu_.lock(); + + if (this->values_[3] == 0 || val < this->values_[0]) // set min + this->values_[0] = val; + if (this->values_[3] == 0 || val > this->values_[1]) // set max + this->values_[1] = val; + + this->values_[2] += val; // compute sum + this->values_[3]++; // increment count + + this->mu_.unlock(); + } + + /** + * Checkpoints the current value. This function will overwrite the current checkpoint with the + * current value. + * + */ + void checkpoint() override + { + this->mu_.lock(); + this->checkpoint_ = this->values_; + // Reset the values + this->values_[0] = 0; + this->values_[1] = 0; + this->values_[2] = 0; + this->values_[3] = 0; + this->mu_.unlock(); + } + + /** + * Merges two MinMaxSumCount aggregators together + * + * @param other the aggregator to merge with this aggregator + */ + void merge(const MinMaxSumCountAggregator &other) + { + if (this->kind_ == other.kind_) + { + this->mu_.lock(); + // set min + if (other.values_[0] < this->values_[0]) + this->values_[0] = other.values_[0]; + // set max + if (other.values_[1] > this->values_[1]) + this->values_[1] = other.values_[1]; + // set sum + this->values_[2] += other.values_[2]; + // set count + this->values_[3] += other.values_[3]; + this->mu_.unlock(); + } + else + { + // Log error + return; + } + } + + /** + * Returns the checkpointed value + * + * @return the value of the checkpoint + */ + std::vector get_checkpoint() override + { + return this->checkpoint_; + } + + /** + * Returns the values currently held by the aggregator + * + * @return the values held by the aggregator + */ + std::vector get_values() override + { + return this->values_; + } + +}; +} +} +OPENTELEMETRY_END_NAMESPACE From 302ca056bdff7ac95a1587fb79f53e43d4b457dc Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Wed, 15 Jul 2020 11:52:11 -0400 Subject: [PATCH 02/30] test files --- sdk/test/metrics/BUILD | 33 +++++ sdk/test/metrics/counter_aggregator_test.cc | 111 ++++++++++++++ sdk/test/metrics/histogram_aggregator_test.cc | 135 ++++++++++++++++++ 3 files changed, 279 insertions(+) create mode 100644 sdk/test/metrics/counter_aggregator_test.cc create mode 100644 sdk/test/metrics/histogram_aggregator_test.cc diff --git a/sdk/test/metrics/BUILD b/sdk/test/metrics/BUILD index e2d5c5cb71..8121a723f7 100644 --- a/sdk/test/metrics/BUILD +++ b/sdk/test/metrics/BUILD @@ -2,6 +2,39 @@ cc_test( name = "meter_provider_sdk_test", srcs = [ "meter_provider_sdk_test.cc", +], + deps = [ + "//sdk/src/metrics", + "@com_google_googletest//:gtest_main", + ], +) + +cc_test( + name = "counter_aggregator_test", + srcs = [ + "counter_aggregator_test.cc", + ], + deps = [ + "//sdk/src/metrics", + "@com_google_googletest//:gtest_main", + ], +) + +cc_test( + name = "histogram_aggregator_test", + srcs = [ + "histogram_aggregator_test.cc", + ], + deps = [ + "//sdk/src/metrics", + "@com_google_googletest//:gtest_main", + ], +) + +cc_test( + name = "metric_instrument_test", + srcs = [ + "metric_instrument_test.cc", ], deps = [ "//sdk/src/metrics", diff --git a/sdk/test/metrics/counter_aggregator_test.cc b/sdk/test/metrics/counter_aggregator_test.cc new file mode 100644 index 0000000000..3fea68ecaa --- /dev/null +++ b/sdk/test/metrics/counter_aggregator_test.cc @@ -0,0 +1,111 @@ +#include "opentelemetry/sdk/metrics/aggregator/counter_aggregator.h" + +#include +#include +#include + +namespace metrics_api = opentelemetry::metrics; + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace sdk +{ +namespace metrics +{ + +TEST(CounterAggregator, NoUpdates) +{ + CounterAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter); + + EXPECT_EQ(alpha.get_checkpoint().size(), 1); + EXPECT_EQ(alpha.get_checkpoint()[0], 0); + + alpha.checkpoint(); + EXPECT_EQ(alpha.get_checkpoint().size(), 1); + EXPECT_EQ(alpha.get_checkpoint()[0], 0); +} + +TEST(CounterAggregator, Update) +{ + CounterAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter); + CounterAggregator beta(metrics_api::BoundInstrumentKind::BoundIntCounter); + + for (int i = 0; i<123456; i++){ + alpha.update(1); + } + + int sum = 0; + for (int i = 0; i < 100; i++){ + int tmp = std::rand(); + beta.update(tmp); + sum+=tmp; + } + + EXPECT_EQ(alpha.get_checkpoint()[0], 0); // checkpoint shouldn't change even with updates + EXPECT_EQ(beta.get_checkpoint()[0], 0); + + alpha.checkpoint(); + beta.checkpoint(); + + EXPECT_EQ(alpha.get_checkpoint()[0], 123456); + EXPECT_EQ(beta.get_checkpoint()[0], sum); + + alpha.update(15); + alpha.checkpoint(); + EXPECT_EQ(alpha.get_checkpoint()[0], 15); //reset to 0 after first checkpoint call +} + +// callback update function used to test concurrent calls +void incrementingCallback(Aggregator & agg) +{ + for (int i = 0; i< 2000000; i++){ + agg.update(1); + } +} + +TEST(CounterAggregator, Concurrency){ + CounterAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter); + + // spawn new threads that initiate the callback + std::thread first (incrementingCallback, std::ref(alpha)); + std::thread second (incrementingCallback, std::ref(alpha)); + + + first.join(); + second.join(); + + alpha.checkpoint(); + + // race conditions result in values below 2*2000000 + EXPECT_EQ(alpha.get_checkpoint()[0], 2*2000000); +} + +TEST(CounterAggregator, Merge) +{ + CounterAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter); + CounterAggregator beta(metrics_api::BoundInstrumentKind::BoundIntCounter); + + alpha.merge(beta); + + alpha.checkpoint(); + EXPECT_EQ(alpha.get_checkpoint()[0], 0); //merge with no updates + + for (int i = 0; i<500; i++){ + alpha.update(1); + } + + for (int i = 0; i<700; i++){ + beta.update(1); + } + + alpha.merge(beta); + alpha.checkpoint(); + EXPECT_EQ(alpha.get_checkpoint()[0], 1200); + + //HistogramAggregator gamma(metrics_api::BoundInstrumentKind::BoundValueRecorder); + //ASSERT_THROW(alpha.merge(gamma), AggregatorMismatch); +} + + +} // namespace sdk +} // namespace metrics +OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/test/metrics/histogram_aggregator_test.cc b/sdk/test/metrics/histogram_aggregator_test.cc new file mode 100644 index 0000000000..4153049d4f --- /dev/null +++ b/sdk/test/metrics/histogram_aggregator_test.cc @@ -0,0 +1,135 @@ +#include "opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h" + +#include +#include +#include +// #include + +namespace metrics_api = opentelemetry::metrics; + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace sdk +{ +namespace metrics +{ + +// Test updating with a uniform set of updates +TEST(Histogram, Uniform) +{ + std::vector boundaries{10,20,30,40,50}; + HistogramAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); + + alpha.checkpoint(); + EXPECT_EQ(alpha.get_checkpoint().size(),2); + EXPECT_EQ(alpha.get_counts().size(),6); + + for (int i = 0; i< 60; i++){ + alpha.update(i); + } + + alpha.checkpoint(); + + EXPECT_EQ(alpha.get_checkpoint()[0], 1770); + EXPECT_EQ(alpha.get_checkpoint()[1], 60); + + std::vector correct = {10,10,10,10,10,10}; + EXPECT_EQ(alpha.get_counts(), correct); +} + +// Test updating with a normal distribution +TEST(Histogram, Normal) +{ + std::vector boundaries{2,4,6,8,10,12}; + HistogramAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); + + std::vector vals{1,3,3,5,5,5,7,7,7,7,9,9,9,11,11,13}; + for (int i : vals){ + alpha.update(i); + } + + alpha.checkpoint(); + + EXPECT_EQ(alpha.get_checkpoint()[0], std::accumulate(vals.begin(),vals.end(),0)); + EXPECT_EQ(alpha.get_checkpoint()[1], vals.size()); + + std::vector correct = {1,2,3,4,3,2,1}; + EXPECT_EQ(alpha.get_counts(), correct); +} + +TEST(Histogram, Merge){ + std::vector boundaries{2,4,6,8,10,12}; + HistogramAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); + HistogramAggregator beta(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); + + std::vector vals{1,3,3,5,5,5,7,7,7,7,9,9,9,11,11,13}; + for (int i : vals){ + alpha.update(i); + } + + std::vector otherVals{1,1,1,1,11,11,13,13,13,15}; + for (int i : otherVals){ + beta.update(i); + } + alpha.merge(beta); + alpha.checkpoint(); + + EXPECT_EQ(alpha.get_checkpoint()[0], std::accumulate(vals.begin(),vals.end(),0)+std::accumulate(otherVals.begin(), otherVals.end(), 0)); + EXPECT_EQ(alpha.get_checkpoint()[1], vals.size()+otherVals.size()); + + std::vector correct = {5,2,3,4,3,4,5}; + EXPECT_EQ(alpha.get_counts(), correct); +} + +// Update callback used to validate multi-threaded performance +void histogramUpdateCallback(Aggregator & agg, std::vector vals){ + for (int i: vals){ + agg.update(i); + } +} + +int randVal(){ + return rand() % 15; +} + +TEST(Histogram, Concurrency){ + std::vector boundaries{2,4,6,8,10,12}; + HistogramAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); + + std::vector vals1(1000); + std::generate(vals1.begin(),vals1.end(),randVal); + + std::vector vals2(1000); + std::generate(vals2.begin(),vals2.end(),randVal); + + std::thread first (histogramUpdateCallback, std::ref(alpha), vals1); + std::thread second (histogramUpdateCallback, std::ref(alpha), vals2); + + first.join(); + second.join(); + + HistogramAggregator beta(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); + + + // Timing harness to compare linear and binary insertion + // auto start = std::chrono::system_clock::now(); + for (int i: vals1){ + beta.update(i); + } + for (int i: vals2){ + beta.update(i); + } + //auto end = std::chrono::system_clock::now(); + //auto elapsed = std::chrono::duration_cast(end - start); + //std::cout <<"Update time: " < Date: Wed, 15 Jul 2020 12:59:59 -0400 Subject: [PATCH 03/30] final specifier --- .../opentelemetry/sdk/metrics/aggregator/counter_aggregator.h | 2 +- .../opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h index 8d50ed830d..29f4f9356a 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h @@ -18,7 +18,7 @@ namespace metrics { template -class CounterAggregator : public Aggregator +class CounterAggregator final : public Aggregator { public: diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h index b0d705317d..5777917e34 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h @@ -17,7 +17,7 @@ namespace sdk namespace metrics { template -class HistogramAggregator : public Aggregator +class HistogramAggregator final : public Aggregator { public: From c54651cc5689498f9247f8a31f06f238499426dc Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Wed, 15 Jul 2020 23:08:27 -0400 Subject: [PATCH 04/30] cleaning up for PR --- .../aggregator/min_max_sum_count_aggregator.h | 120 ------------------ 1 file changed, 120 deletions(-) delete mode 100644 sdk/include/opentelemetry/sdk/metrics/aggregator/min_max_sum_count_aggregator.h diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/min_max_sum_count_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/min_max_sum_count_aggregator.h deleted file mode 100644 index 298abba441..0000000000 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/min_max_sum_count_aggregator.h +++ /dev/null @@ -1,120 +0,0 @@ -#pragma once - -#include "opentelemetry/version.h" -#include "opentelemetry/metrics/instrument.h" -#include "opentelemetry/sdk/metrics/aggregator/aggregator.h" - -#include -#include -#include -#include - -namespace metrics_api = opentelemetry::metrics; - -OPENTELEMETRY_BEGIN_NAMESPACE -namespace sdk -{ -namespace metrics -{ -template -class MinMaxSumCountAggregator : public Aggregator -{ -public: - explicit MinMaxSumCountAggregator(metrics_api::BoundInstrumentKind kind) - { - static_assert(std::is_arithmetic::value, "Not an arithmetic type"); - this->kind_ = kind; - this->values_ = std::vector(4, 0); // {min, max, sum count} - this->checkpoint_ = this->values_; - } - - /** - * Receives a captured value from the instrument and applies it to the current aggregator value. - * - * @param val, the raw value used in aggregation - */ - void update(T val) override - { - this->mu_.lock(); - - if (this->values_[3] == 0 || val < this->values_[0]) // set min - this->values_[0] = val; - if (this->values_[3] == 0 || val > this->values_[1]) // set max - this->values_[1] = val; - - this->values_[2] += val; // compute sum - this->values_[3]++; // increment count - - this->mu_.unlock(); - } - - /** - * Checkpoints the current value. This function will overwrite the current checkpoint with the - * current value. - * - */ - void checkpoint() override - { - this->mu_.lock(); - this->checkpoint_ = this->values_; - // Reset the values - this->values_[0] = 0; - this->values_[1] = 0; - this->values_[2] = 0; - this->values_[3] = 0; - this->mu_.unlock(); - } - - /** - * Merges two MinMaxSumCount aggregators together - * - * @param other the aggregator to merge with this aggregator - */ - void merge(const MinMaxSumCountAggregator &other) - { - if (this->kind_ == other.kind_) - { - this->mu_.lock(); - // set min - if (other.values_[0] < this->values_[0]) - this->values_[0] = other.values_[0]; - // set max - if (other.values_[1] > this->values_[1]) - this->values_[1] = other.values_[1]; - // set sum - this->values_[2] += other.values_[2]; - // set count - this->values_[3] += other.values_[3]; - this->mu_.unlock(); - } - else - { - // Log error - return; - } - } - - /** - * Returns the checkpointed value - * - * @return the value of the checkpoint - */ - std::vector get_checkpoint() override - { - return this->checkpoint_; - } - - /** - * Returns the values currently held by the aggregator - * - * @return the values held by the aggregator - */ - std::vector get_values() override - { - return this->values_; - } - -}; -} -} -OPENTELEMETRY_END_NAMESPACE From 4ba08e23de36c382efed63152b61a2e573fab333 Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Wed, 15 Jul 2020 23:11:08 -0400 Subject: [PATCH 05/30] removing irrelevant test targets --- sdk/test/metrics/BUILD | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/sdk/test/metrics/BUILD b/sdk/test/metrics/BUILD index 8121a723f7..cd82d5c4cb 100644 --- a/sdk/test/metrics/BUILD +++ b/sdk/test/metrics/BUILD @@ -31,13 +31,3 @@ cc_test( ], ) -cc_test( - name = "metric_instrument_test", - srcs = [ - "metric_instrument_test.cc", - ], - deps = [ - "//sdk/src/metrics", - "@com_google_googletest//:gtest_main", - ], -) From 0cd64d93a312cb9ede478982188be54956df6dcc Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Tue, 21 Jul 2020 00:21:28 -0400 Subject: [PATCH 06/30] incorporating feedback --- .../sdk/metrics/aggregator/aggregator.h | 45 ++++++++++++++---- .../metrics/aggregator/counter_aggregator.h | 21 ++++----- .../metrics/aggregator/histogram_aggregator.h | 46 +++++++++++++------ sdk/test/metrics/counter_aggregator_test.cc | 12 ++--- sdk/test/metrics/histogram_aggregator_test.cc | 28 ++++++++--- 5 files changed, 105 insertions(+), 47 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h index 00ed434f07..c17a5ae8da 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h @@ -1,12 +1,12 @@ #pragma once -#include "opentelemetry/version.h" +#include #include "opentelemetry/metrics/instrument.h" - +#include "opentelemetry/version.h" #include #include -#include -#include + + namespace metrics_api = opentelemetry::metrics; @@ -15,6 +15,17 @@ namespace sdk { namespace metrics { + +enum class AggregatorKind +{ + Counter = 0, + MinMaxSumCount = 1, + Gauge = 2, + Sketch = 3, + Histogram = 4, + Exact = 5, +}; + /* * Performs calculations necessary to combine updates from instruments into an * insightful value. @@ -29,7 +40,7 @@ class Aggregator Aggregator() = default; /** - * Recieves a captured value from the instrument and applies it to the current aggregator value. + * Receives a captured value from the instrument and applies it to the current aggregator value. * * @param val, the raw value used in aggregation * @return none @@ -53,7 +64,7 @@ class Aggregator * @param other, the aggregator with merge with * @return none */ - void merge(std::shared_ptr other); + void merge(Aggregator * other); /** * Returns the checkpointed value @@ -75,26 +86,40 @@ class Aggregator * Returns the instrument kind which this aggregator is associated with * * @param none - * @return the BoundInstrumentKind of the aggregator's owner + * @return the InstrumentKind of the aggregator's owner */ - virtual opentelemetry::metrics::BoundInstrumentKind get_kind() + virtual opentelemetry::metrics::InstrumentKind get_instrument_kind() final { return kind_; } + /** + * Returns the type of this aggregator + * + * @param none + * @return the AggregatorKind of this instrument + */ + virtual AggregatorKind get_aggregator_kind() final + { + return agg_kind_; + } + // Custom copy constructor to handle the mutex - Aggregator(const Aggregator &cp) { + Aggregator(const Aggregator &cp) + { values_ = cp.values_; checkpoint_ = cp.checkpoint_; kind_ = cp.kind_; + agg_kind_ = cp.agg_kind_; // use default initialized mutex as they cannot be copied } protected: std::vector values_; std::vector checkpoint_; - opentelemetry::metrics::BoundInstrumentKind kind_; + opentelemetry::metrics::InstrumentKind kind_; std::mutex mu_; + AggregatorKind agg_kind_; }; diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h index 29f4f9356a..ecdfae35aa 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h @@ -7,7 +7,6 @@ #include #include #include -#include namespace metrics_api = opentelemetry::metrics; @@ -22,11 +21,12 @@ class CounterAggregator final : public Aggregator { public: - CounterAggregator(metrics_api::BoundInstrumentKind kind) + CounterAggregator(metrics_api::InstrumentKind kind) { this->kind_ = kind; this->values_ = std::vector(1, 0); this->checkpoint_ = std::vector(1, 0); + this->agg_kind_ = AggregatorKind::Counter; } /** @@ -64,15 +64,13 @@ class CounterAggregator final : public Aggregator */ void merge(CounterAggregator other) { - if (this->kind_ == other.kind_) - { + if (this->agg_kind_ == other.agg_kind_) { this->mu_.lock(); - this->values_[0] += other.values_[0]; // atomic operation afaik + this->values_[0] += other.values_[0]; this->mu_.unlock(); } - else - { - //AggregatorMismatch Exception + else { + throw std::invalid_argument("Aggregators of different types cannot be merged."); } } @@ -88,12 +86,13 @@ class CounterAggregator final : public Aggregator } /** - * Returns the instrument kind which this aggregator is associated with + * Returns the current values * * @param none - * @return the BoundInstrumentKind of the aggregator's owner + * @return the present aggregator values */ - std::vector get_values() override{ + std::vector get_values() override + { return this->values_; } diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h index 5777917e34..995ad7ae87 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h @@ -1,13 +1,13 @@ #pragma once -#include "opentelemetry/version.h" +#include +#include #include "opentelemetry/metrics/instrument.h" +#include "opentelemetry/version.h" #include "opentelemetry/sdk/metrics/aggregator/aggregator.h" - +#include #include #include -#include -#include namespace metrics_api = opentelemetry::metrics; @@ -16,6 +16,7 @@ namespace sdk { namespace metrics { + template class HistogramAggregator final : public Aggregator { @@ -29,9 +30,13 @@ class HistogramAggregator final : public Aggregator * Sum is stored in values_[0] * Count is stored in position_[1] */ - HistogramAggregator(metrics_api::BoundInstrumentKind kind, std::vector boundaries) + HistogramAggregator(metrics_api::InstrumentKind kind, std::vector boundaries) { + if (!std::is_sorted(boundaries.begin(),boundaries.end())){ + throw std::invalid_argument("Histogram boundaries must be monotonic."); + } this->kind_ = kind; + this->agg_kind_ = AggregatorKind::Histogram; boundaries_ = boundaries; this->values_ = std::vector(2, 0); this->checkpoint_ = std::vector(2, 0); @@ -40,15 +45,19 @@ class HistogramAggregator final : public Aggregator /** * Recieves a captured value from the instrument and inserts it into the current histogram counts. - * Depending on the use case, linear search or binary search based implementations may be preferred. - * In a uniformly distributed dataset, linear search outperforms binary search until 512 buckets. + * + * Depending on the use case, a linear search or binary search based implementation may be preferred. + * In uniformly distributed datasets, linear search outperforms binary search until 512 buckets. However, + * if the distribution is strongly skewed right ( for example server latency where most values may be <10ms + * but the range is from 0 - 1000 ms), a linear search could be superior even with more than 500 buckets as + * almost all values inserted would be at the beginning of the boundaries array and thus found more quickly + * through linear search. * * @param val, the raw value used in aggregation * @return none */ void update(T val) override { - int bucketID = boundaries_.size(); for (int i = 0; i < boundaries_.size(); i++) { @@ -60,8 +69,8 @@ class HistogramAggregator final : public Aggregator } // Alternate implementation with binary search - //auto pos = std::lower_bound (boundaries_.begin(), boundaries_.end(), val); - //bucketCounts_[pos-boundaries_.begin()] += 1; + // auto pos = std::lower_bound (boundaries_.begin(), boundaries_.end(), val); + // bucketCounts_[pos-boundaries_.begin()] += 1; this->mu_.lock(); this->values_[0] += val; @@ -86,7 +95,7 @@ class HistogramAggregator final : public Aggregator /** * Merges the values of two aggregators in a semantically accurate manner. - * A histogram aggregator can only be merged with another histogram aggregatos with the same boudnaries. + * A histogram aggregator can only be merged with another histogram aggregator that has the same boudnaries. * A histogram merge first adds the sum and count values then iterates over the adds the bucket counts * element by element. * @@ -96,6 +105,15 @@ class HistogramAggregator final : public Aggregator void merge(HistogramAggregator other) { this->mu_.lock(); + + // Ensure that incorrect types are not merged + if (this->agg_kind_ != other.agg_kind_){ + throw std::invalid_argument("Aggregators of different types cannot be merged."); + // Reject histogram merges with differing boundary vectors + } else if (other.boundaries_ != this->boundaries_){ + throw std::invalid_argument("Histogram boundaries do not match."); + } + this->values_[0] += other.values_[0]; this->values_[1] += other.values_[1]; @@ -114,14 +132,14 @@ class HistogramAggregator final : public Aggregator */ std::vector get_checkpoint() override { - return this->checkpoint_; // what happens if there isn't a checkpoint? + return this->checkpoint_; } /** - * Returns the current value + * Returns the current values * * @param none - * @return the present aggregator value + * @return the present aggregator values */ std::vector get_values() override { diff --git a/sdk/test/metrics/counter_aggregator_test.cc b/sdk/test/metrics/counter_aggregator_test.cc index 3fea68ecaa..8208983b89 100644 --- a/sdk/test/metrics/counter_aggregator_test.cc +++ b/sdk/test/metrics/counter_aggregator_test.cc @@ -14,7 +14,7 @@ namespace metrics TEST(CounterAggregator, NoUpdates) { - CounterAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter); + CounterAggregator alpha(metrics_api::InstrumentKind::Counter); EXPECT_EQ(alpha.get_checkpoint().size(), 1); EXPECT_EQ(alpha.get_checkpoint()[0], 0); @@ -26,8 +26,8 @@ TEST(CounterAggregator, NoUpdates) TEST(CounterAggregator, Update) { - CounterAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter); - CounterAggregator beta(metrics_api::BoundInstrumentKind::BoundIntCounter); + CounterAggregator alpha(metrics_api::InstrumentKind::Counter); + CounterAggregator beta(metrics_api::InstrumentKind::Counter); for (int i = 0; i<123456; i++){ alpha.update(1); @@ -63,7 +63,7 @@ void incrementingCallback(Aggregator & agg) } TEST(CounterAggregator, Concurrency){ - CounterAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter); + CounterAggregator alpha(metrics_api::InstrumentKind::Counter); // spawn new threads that initiate the callback std::thread first (incrementingCallback, std::ref(alpha)); @@ -81,8 +81,8 @@ TEST(CounterAggregator, Concurrency){ TEST(CounterAggregator, Merge) { - CounterAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter); - CounterAggregator beta(metrics_api::BoundInstrumentKind::BoundIntCounter); + CounterAggregator alpha(metrics_api::InstrumentKind::Counter); + CounterAggregator beta(metrics_api::InstrumentKind::Counter); alpha.merge(beta); diff --git a/sdk/test/metrics/histogram_aggregator_test.cc b/sdk/test/metrics/histogram_aggregator_test.cc index 4153049d4f..4b6cbd795c 100644 --- a/sdk/test/metrics/histogram_aggregator_test.cc +++ b/sdk/test/metrics/histogram_aggregator_test.cc @@ -3,6 +3,7 @@ #include #include #include +#include // #include namespace metrics_api = opentelemetry::metrics; @@ -17,7 +18,9 @@ namespace metrics TEST(Histogram, Uniform) { std::vector boundaries{10,20,30,40,50}; - HistogramAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); + HistogramAggregator alpha(metrics_api::InstrumentKind::Counter, boundaries); + + EXPECT_EQ(alpha.get_aggregator_kind(), AggregatorKind::Histogram); alpha.checkpoint(); EXPECT_EQ(alpha.get_checkpoint().size(),2); @@ -40,7 +43,7 @@ TEST(Histogram, Uniform) TEST(Histogram, Normal) { std::vector boundaries{2,4,6,8,10,12}; - HistogramAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); + HistogramAggregator alpha(metrics_api::InstrumentKind::Counter, boundaries); std::vector vals{1,3,3,5,5,5,7,7,7,7,9,9,9,11,11,13}; for (int i : vals){ @@ -58,8 +61,8 @@ TEST(Histogram, Normal) TEST(Histogram, Merge){ std::vector boundaries{2,4,6,8,10,12}; - HistogramAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); - HistogramAggregator beta(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); + HistogramAggregator alpha(metrics_api::InstrumentKind::Counter, boundaries); + HistogramAggregator beta(metrics_api::InstrumentKind::Counter, boundaries); std::vector vals{1,3,3,5,5,5,7,7,7,7,9,9,9,11,11,13}; for (int i : vals){ @@ -70,6 +73,7 @@ TEST(Histogram, Merge){ for (int i : otherVals){ beta.update(i); } + alpha.merge(beta); alpha.checkpoint(); @@ -93,7 +97,7 @@ int randVal(){ TEST(Histogram, Concurrency){ std::vector boundaries{2,4,6,8,10,12}; - HistogramAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); + HistogramAggregator alpha(metrics_api::InstrumentKind::Counter, boundaries); std::vector vals1(1000); std::generate(vals1.begin(),vals1.end(),randVal); @@ -107,7 +111,7 @@ TEST(Histogram, Concurrency){ first.join(); second.join(); - HistogramAggregator beta(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); + HistogramAggregator beta(metrics_api::InstrumentKind::Counter, boundaries); // Timing harness to compare linear and binary insertion @@ -129,6 +133,18 @@ TEST(Histogram, Concurrency){ EXPECT_EQ(alpha.get_counts(), beta.get_counts()); } +TEST(Histogram, Errors){ + std::vector boundaries{2,4,6,8,10,12}; + std::vector boundaries2{1,4,6,8,10,12}; + std::vector unsortedBoundaries{10,12,4,6,8}; + EXPECT_ANY_THROW(HistogramAggregator alpha(metrics_api::InstrumentKind::Counter, unsortedBoundaries)); + + HistogramAggregator beta(metrics_api::InstrumentKind::Counter, boundaries); + HistogramAggregator gamma(metrics_api::InstrumentKind::Counter, boundaries2); + + EXPECT_ANY_THROW(beta.merge(gamma)); +} + } // namespace sdk } // namespace metrics From 01afb81612b19931782a9930cb7888fddb07536a Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Fri, 24 Jul 2020 16:29:23 -0400 Subject: [PATCH 07/30] adding virtual functions --- .../sdk/metrics/aggregator/aggregator.h | 51 ++++++++++++------- .../metrics/aggregator/counter_aggregator.h | 4 +- .../metrics/aggregator/histogram_aggregator.h | 17 +++++-- 3 files changed, 50 insertions(+), 22 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h index c17a5ae8da..fe2ec1bb90 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h @@ -7,7 +7,6 @@ #include - namespace metrics_api = opentelemetry::metrics; OPENTELEMETRY_BEGIN_NAMESPACE @@ -18,12 +17,12 @@ namespace metrics enum class AggregatorKind { - Counter = 0, - MinMaxSumCount = 1, - Gauge = 2, - Sketch = 3, - Histogram = 4, - Exact = 5, + Counter = 0, + MinMaxSumCount = 1, + Gauge = 2, + Sketch = 3, + Histogram = 4, + Exact = 5, }; /* @@ -39,6 +38,8 @@ class Aggregator Aggregator() = default; + virtual ~Aggregator() = default; + /** * Receives a captured value from the instrument and applies it to the current aggregator value. * @@ -83,27 +84,43 @@ class Aggregator virtual std::vector get_values() = 0; /** - * Returns the instrument kind which this aggregator is associated with - * - * @param none - * @return the InstrumentKind of the aggregator's owner - */ + * Returns the instrument kind which this aggregator is associated with + * + * @param none + * @return the InstrumentKind of the aggregator's owner + */ virtual opentelemetry::metrics::InstrumentKind get_instrument_kind() final { return kind_; } /** - * Returns the type of this aggregator - * - * @param none - * @return the AggregatorKind of this instrument - */ + * Returns the type of this aggregator + * + * @param none + * @return the AggregatorKind of this instrument + */ virtual AggregatorKind get_aggregator_kind() final { return agg_kind_; } + virtual std::vector get_boundaries() { + return std::vector(); + } + + virtual std::vector get_counts() { + return std::vector(); + } + + virtual bool get_quant_estimation () { + return false; + } + + virtual T get_quantiles(double q) { + return values_[0]; + } + // Custom copy constructor to handle the mutex Aggregator(const Aggregator &cp) { diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h index ecdfae35aa..61e68e840c 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h @@ -80,7 +80,7 @@ class CounterAggregator final : public Aggregator * @param none * @return the value of the checkpoint */ - std::vector get_checkpoint() override + virtual std::vector get_checkpoint() override { return this->checkpoint_; } @@ -91,7 +91,7 @@ class CounterAggregator final : public Aggregator * @param none * @return the present aggregator values */ - std::vector get_values() override + virtual std::vector get_values() override { return this->values_; } diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h index 995ad7ae87..9fa70ac524 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h @@ -152,9 +152,9 @@ class HistogramAggregator final : public Aggregator * @param none * @return the aggregator boundaries */ - std::vector get_boundaries() + virtual std::vector get_boundaries() override { - return bucketCounts_; + return boundaries_; } /** @@ -163,11 +163,22 @@ class HistogramAggregator final : public Aggregator * @param none * @return the aggregator bucket counts */ - std::vector get_counts() + virtual std::vector get_counts() override { return bucketCounts_; } + HistogramAggregator(const HistogramAggregator &cp) + { + this->values_ = cp.values_; + this->checkpoint_ = cp.checkpoint_; + this->kind_ = cp.kind_; + this->agg_kind_ = cp.agg_kind_; + boundaries_ = cp.boundaries_; + bucketCounts_ = cp.bucketCounts_; + // use default initialized mutex as they cannot be copied + } + private: std::vector boundaries_; std::vector bucketCounts_; From 7f32074880751d2ee4f9ebb1811f322f71db960a Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Sun, 26 Jul 2020 00:05:25 -0400 Subject: [PATCH 08/30] timestamp virtual function --- .../opentelemetry/sdk/metrics/aggregator/aggregator.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h index fe2ec1bb90..638ae5e451 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h @@ -1,6 +1,7 @@ #pragma once #include +#include "opentelemetry/core/timestamp.h" #include "opentelemetry/metrics/instrument.h" #include "opentelemetry/version.h" #include @@ -105,22 +106,31 @@ class Aggregator return agg_kind_; } + // virtual function to be overriden for the Histogram Aggregator virtual std::vector get_boundaries() { return std::vector(); } + // virtual function to be overriden for the Histogram Aggregator virtual std::vector get_counts() { return std::vector(); } + // virtual function to be overriden for Exact and Sketch Aggregators virtual bool get_quant_estimation () { return false; } + // virtual function to be overriden for Exact and Sketch Aggregators virtual T get_quantiles(double q) { return values_[0]; } + // virtual function to be overriden for Gauge Aggregator + virtual core::SystemTimestamp get_checkpoint_timestamp() { + return core::SystemTimestamp(); + } + // Custom copy constructor to handle the mutex Aggregator(const Aggregator &cp) { From 47a58f89a32da5a5c7d667c7f2620c69ae3b6e38 Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Mon, 27 Jul 2020 12:23:34 -0400 Subject: [PATCH 09/30] remove unused import --- .../opentelemetry/sdk/metrics/aggregator/aggregator.h | 1 - .../sdk/metrics/aggregator/counter_aggregator.h | 5 ++--- .../sdk/metrics/aggregator/histogram_aggregator.h | 1 - 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h index 638ae5e451..7ae8717c89 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h @@ -4,7 +4,6 @@ #include "opentelemetry/core/timestamp.h" #include "opentelemetry/metrics/instrument.h" #include "opentelemetry/version.h" -#include #include diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h index 61e68e840c..4a2edac875 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h @@ -1,12 +1,11 @@ #pragma once +#include #include "opentelemetry/version.h" #include "opentelemetry/metrics/instrument.h" #include "opentelemetry/sdk/metrics/aggregator/aggregator.h" - -#include #include -#include + namespace metrics_api = opentelemetry::metrics; diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h index 9fa70ac524..acddb6a568 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h @@ -6,7 +6,6 @@ #include "opentelemetry/version.h" #include "opentelemetry/sdk/metrics/aggregator/aggregator.h" #include -#include #include namespace metrics_api = opentelemetry::metrics; From 98d80d1593c156d6499c6362943b6c5156c9c61c Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Mon, 27 Jul 2020 21:24:08 -0400 Subject: [PATCH 10/30] user specified exceptions --- .../metrics/aggregator/counter_aggregator.h | 5 ++++ .../metrics/aggregator/histogram_aggregator.h | 28 +++++++++++++++---- sdk/test/metrics/histogram_aggregator_test.cc | 3 ++ 3 files changed, 31 insertions(+), 5 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h index 4a2edac875..84da44b999 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h @@ -66,10 +66,15 @@ class CounterAggregator final : public Aggregator if (this->agg_kind_ == other.agg_kind_) { this->mu_.lock(); this->values_[0] += other.values_[0]; + this->checkpoint_[0] += other.checkpoint_[0]; this->mu_.unlock(); } else { +#if __EXCEPTIONS throw std::invalid_argument("Aggregators of different types cannot be merged."); +#else + std::terminate(); +#endif } } diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h index acddb6a568..0a9e776d93 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h @@ -32,7 +32,11 @@ class HistogramAggregator final : public Aggregator HistogramAggregator(metrics_api::InstrumentKind kind, std::vector boundaries) { if (!std::is_sorted(boundaries.begin(),boundaries.end())){ +#if __EXCEPTIONS throw std::invalid_argument("Histogram boundaries must be monotonic."); +#else + std::terminate(); +#endif } this->kind_ = kind; this->agg_kind_ = AggregatorKind::Histogram; @@ -40,6 +44,7 @@ class HistogramAggregator final : public Aggregator this->values_ = std::vector(2, 0); this->checkpoint_ = std::vector(2, 0); bucketCounts_ = std::vector(boundaries_.size() + 1, 0); + bucketCounts_ckpt_ = std::vector(boundaries_.size() + 1, 0); } /** @@ -58,7 +63,7 @@ class HistogramAggregator final : public Aggregator void update(T val) override { int bucketID = boundaries_.size(); - for (int i = 0; i < boundaries_.size(); i++) + for (size_t i = 0; i < boundaries_.size(); i++) { if (val < boundaries_[i]) // concurrent read is thread-safe { @@ -88,8 +93,10 @@ class HistogramAggregator final : public Aggregator void checkpoint() override { this->checkpoint_ = this->values_; - this->values_[0]=0; - this->values_[1]=0; + this->values_[0] = 0; + this->values_[1] = 0; + bucketCounts_ckpt_ = bucketCounts_; + std::fill(bucketCounts_.begin(), bucketCounts_.end(), 0); } /** @@ -107,18 +114,27 @@ class HistogramAggregator final : public Aggregator // Ensure that incorrect types are not merged if (this->agg_kind_ != other.agg_kind_){ +#if __EXCEPTIONS throw std::invalid_argument("Aggregators of different types cannot be merged."); +#else + std::terminate(); +#endif // Reject histogram merges with differing boundary vectors } else if (other.boundaries_ != this->boundaries_){ +#if __EXCEPTIONS throw std::invalid_argument("Histogram boundaries do not match."); +#else + std::terminate(); +#endif } this->values_[0] += other.values_[0]; this->values_[1] += other.values_[1]; - for (int i = 0; i < bucketCounts_.size(); i++) + for (size_t i = 0; i < bucketCounts_.size(); i++) { bucketCounts_[i] += other.bucketCounts_[i]; + bucketCounts_ckpt_[i] += other.bucketCounts_ckpt_[i]; } this->mu_.unlock(); } @@ -164,7 +180,7 @@ class HistogramAggregator final : public Aggregator */ virtual std::vector get_counts() override { - return bucketCounts_; + return bucketCounts_ckpt_; } HistogramAggregator(const HistogramAggregator &cp) @@ -175,12 +191,14 @@ class HistogramAggregator final : public Aggregator this->agg_kind_ = cp.agg_kind_; boundaries_ = cp.boundaries_; bucketCounts_ = cp.bucketCounts_; + bucketCounts_ckpt_ = cp.bucketCounts_ckpt_; // use default initialized mutex as they cannot be copied } private: std::vector boundaries_; std::vector bucketCounts_; + std::vector bucketCounts_ckpt_; }; } diff --git a/sdk/test/metrics/histogram_aggregator_test.cc b/sdk/test/metrics/histogram_aggregator_test.cc index 4b6cbd795c..fceb7fe13a 100644 --- a/sdk/test/metrics/histogram_aggregator_test.cc +++ b/sdk/test/metrics/histogram_aggregator_test.cc @@ -133,6 +133,8 @@ TEST(Histogram, Concurrency){ EXPECT_EQ(alpha.get_counts(), beta.get_counts()); } +#if __EXCEPTIONS + TEST(Histogram, Errors){ std::vector boundaries{2,4,6,8,10,12}; std::vector boundaries2{1,4,6,8,10,12}; @@ -145,6 +147,7 @@ TEST(Histogram, Errors){ EXPECT_ANY_THROW(beta.merge(gamma)); } +#endif } // namespace sdk } // namespace metrics From f96c79483a8a0aafd3b4c6918cac4f08ebd90825 Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Tue, 28 Jul 2020 14:02:20 -0400 Subject: [PATCH 11/30] more virtuals for sketch aggregator --- .../opentelemetry/sdk/metrics/aggregator/aggregator.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h index 7ae8717c89..eddb538b8d 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h @@ -125,6 +125,16 @@ class Aggregator return values_[0]; } + // virtual function to be overriden for Sketch Aggregator + virtual double get_error_bound(double q) { + return 0; + } + + // virtual function to be overriden for Sketch Aggregator + virtual double get_max_buckets(double q) { + return 0; + } + // virtual function to be overriden for Gauge Aggregator virtual core::SystemTimestamp get_checkpoint_timestamp() { return core::SystemTimestamp(); From f23ca9a27d9d9a854f2cf112876f0d58c35f2867 Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Tue, 28 Jul 2020 14:09:57 -0400 Subject: [PATCH 12/30] typos --- sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h index eddb538b8d..eb28453f6c 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h @@ -126,12 +126,12 @@ class Aggregator } // virtual function to be overriden for Sketch Aggregator - virtual double get_error_bound(double q) { + virtual double get_error_bound() { return 0; } // virtual function to be overriden for Sketch Aggregator - virtual double get_max_buckets(double q) { + virtual size_t get_max_buckets() { return 0; } From 13f467725ab960af083fb8f3f6f33bc8783e601f Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Tue, 28 Jul 2020 15:54:45 -0400 Subject: [PATCH 13/30] Update sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h Co-authored-by: Reiley Yang --- .../opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h index 0a9e776d93..9303377fba 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h @@ -31,7 +31,7 @@ class HistogramAggregator final : public Aggregator */ HistogramAggregator(metrics_api::InstrumentKind kind, std::vector boundaries) { - if (!std::is_sorted(boundaries.begin(),boundaries.end())){ + if (!std::is_sorted(boundaries.begin(), boundaries.end())){ #if __EXCEPTIONS throw std::invalid_argument("Histogram boundaries must be monotonic."); #else From 0779a49ce58ef8475540b9d8429331a262b33fea Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Tue, 28 Jul 2020 15:55:01 -0400 Subject: [PATCH 14/30] Update sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h Co-authored-by: Reiley Yang --- .../opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h index 9303377fba..0d3df40890 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h @@ -52,7 +52,7 @@ class HistogramAggregator final : public Aggregator * * Depending on the use case, a linear search or binary search based implementation may be preferred. * In uniformly distributed datasets, linear search outperforms binary search until 512 buckets. However, - * if the distribution is strongly skewed right ( for example server latency where most values may be <10ms + * if the distribution is strongly skewed right (for example server latency where most values may be <10ms * but the range is from 0 - 1000 ms), a linear search could be superior even with more than 500 buckets as * almost all values inserted would be at the beginning of the boundaries array and thus found more quickly * through linear search. From 4e2aeee8f38d9928cb443b76cb6f62fbb3c4c8a9 Mon Sep 17 00:00:00 2001 From: Sam Atac <65615762+satac2@users.noreply.github.com> Date: Wed, 29 Jul 2020 14:49:13 -0500 Subject: [PATCH 15/30] Context helper functions (#225) --- .../opentelemetry/context/runtime_context.h | 40 ++++++++++++++++++ api/test/context/runtime_context_test.cc | 41 +++++++++++++++++++ 2 files changed, 81 insertions(+) diff --git a/api/include/opentelemetry/context/runtime_context.h b/api/include/opentelemetry/context/runtime_context.h index 7ad3b73bdc..fabd353a84 100644 --- a/api/include/opentelemetry/context/runtime_context.h +++ b/api/include/opentelemetry/context/runtime_context.h @@ -47,6 +47,46 @@ class RuntimeContext static RuntimeContext *context_handler_; + // Sets the Key and Value into the passed in context or if a context is not + // passed in, the RuntimeContext. + // Should be used to SetValues to the current RuntimeContext, is essentially + // equivalent to RuntimeContext::GetCurrent().SetValue(key,value). Keep in + // mind that the current RuntimeContext will not be changed, and the new + // context will be returned. + static Context SetValue(nostd::string_view key, + ContextValue value, + Context *context = nullptr) noexcept + { + Context temp_context; + if (context == nullptr) + { + temp_context = GetCurrent(); + } + else + { + temp_context = *context; + } + return temp_context.SetValue(key, value); + } + + // Returns the value associated with the passed in key and either the + // passed in context* or the runtime context if a context is not passed in. + // Should be used to get values from the current RuntimeContext, is + // essentially equivalent to RuntimeContext::GetCurrent().GetValue(key). + static ContextValue GetValue(nostd::string_view key, Context *context = nullptr) noexcept + { + Context temp_context; + if (context == nullptr) + { + temp_context = GetCurrent(); + } + else + { + temp_context = *context; + } + return temp_context.GetValue(key); + } + protected: // Provides a token with the passed in context Token CreateToken(Context context) noexcept { return Token(context); } diff --git a/api/test/context/runtime_context_test.cc b/api/test/context/runtime_context_test.cc index b70d69dce8..5c0dc565d4 100644 --- a/api/test/context/runtime_context_test.cc +++ b/api/test/context/runtime_context_test.cc @@ -59,3 +59,44 @@ TEST(RuntimeContextTest, ThreeAttachDetach) EXPECT_TRUE(context::RuntimeContext::Detach(foo_context_token)); EXPECT_TRUE(context::RuntimeContext::Detach(test_context_token)); } + +// Tests that SetValue returns a context with the passed in data and the +// RuntimeContext data when a context is not passed into the +// RuntimeContext::SetValue method. +TEST(RuntimeContextTest, SetValueRuntimeContext) +{ + context::Context foo_context = context::Context("foo_key", (int64_t)596); + context::RuntimeContext::Token old_context_token = context::RuntimeContext::Attach(foo_context); + context::Context test_context = context::RuntimeContext::SetValue("test_key", (int64_t)123); + EXPECT_EQ(nostd::get(test_context.GetValue("test_key")), 123); + EXPECT_EQ(nostd::get(test_context.GetValue("foo_key")), 596); +} + +// Tests that SetValue returns a context with the passed in data and the +// passed in context data when a context* is passed into the +// RuntimeContext::SetValue method. +TEST(RuntimeContextTest, SetValueOtherContext) +{ + context::Context foo_context = context::Context("foo_key", (int64_t)596); + context::Context test_context = + context::RuntimeContext::SetValue("test_key", (int64_t)123, &foo_context); + EXPECT_EQ(nostd::get(test_context.GetValue("test_key")), 123); + EXPECT_EQ(nostd::get(test_context.GetValue("foo_key")), 596); +} + +// Tests that SetValue returns the ContextValue associated with the +// passed in string and the current Runtime Context +TEST(RuntimeContextTest, GetValueRuntimeContext) +{ + context::Context foo_context = context::Context("foo_key", (int64_t)596); + context::RuntimeContext::Token old_context_token = context::RuntimeContext::Attach(foo_context); + EXPECT_EQ(nostd::get(context::RuntimeContext::GetValue("foo_key")), 596); +} + +// Tests that SetValue returns the ContextValue associated with the +// passed in string and the passed in context +TEST(RuntimeContextTest, GetValueOtherContext) +{ + context::Context foo_context = context::Context("foo_key", (int64_t)596); + EXPECT_EQ(nostd::get(context::RuntimeContext::GetValue("foo_key", &foo_context)), 596); +} From 822e7b4d9da1072a773c720407e4e71217152523 Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Wed, 15 Jul 2020 11:36:14 -0400 Subject: [PATCH 16/30] templated aggregators --- .../sdk/metrics/aggregator/aggregator.h | 103 +++++++++++ .../metrics/aggregator/counter_aggregator.h | 104 ++++++++++++ .../metrics/aggregator/histogram_aggregator.h | 160 ++++++++++++++++++ .../aggregator/min_max_sum_count_aggregator.h | 120 +++++++++++++ 4 files changed, 487 insertions(+) create mode 100644 sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h create mode 100644 sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h create mode 100644 sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h create mode 100644 sdk/include/opentelemetry/sdk/metrics/aggregator/min_max_sum_count_aggregator.h diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h new file mode 100644 index 0000000000..00ed434f07 --- /dev/null +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h @@ -0,0 +1,103 @@ +#pragma once + +#include "opentelemetry/version.h" +#include "opentelemetry/metrics/instrument.h" + +#include +#include +#include +#include + +namespace metrics_api = opentelemetry::metrics; + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace sdk +{ +namespace metrics +{ +/* + * Performs calculations necessary to combine updates from instruments into an + * insightful value. + * Also stores current instrument values and checkpoints collected at intervals + * governing the entire pipeline. + */ +template +class Aggregator +{ +public: + + Aggregator() = default; + + /** + * Recieves a captured value from the instrument and applies it to the current aggregator value. + * + * @param val, the raw value used in aggregation + * @return none + */ + virtual void update(T val) = 0; + + /** + * Checkpoints the current value. This function will overwrite the current checkpoint with the + * current value. + * + * @param none + * @return none + */ + virtual void checkpoint() = 0; + + /** + * Merges the values of two aggregators in a semantically accurate manner. + * Merging will occur differently for different aggregators depending on the + * way values are tracked. + * + * @param other, the aggregator with merge with + * @return none + */ + void merge(std::shared_ptr other); + + /** + * Returns the checkpointed value + * + * @param none + * @return the value of the checkpoint + */ + virtual std::vector get_checkpoint() = 0; + + /** + * Returns the current value + * + * @param none + * @return the present aggregator value + */ + virtual std::vector get_values() = 0; + + /** + * Returns the instrument kind which this aggregator is associated with + * + * @param none + * @return the BoundInstrumentKind of the aggregator's owner + */ + virtual opentelemetry::metrics::BoundInstrumentKind get_kind() + { + return kind_; + } + + // Custom copy constructor to handle the mutex + Aggregator(const Aggregator &cp) { + values_ = cp.values_; + checkpoint_ = cp.checkpoint_; + kind_ = cp.kind_; + // use default initialized mutex as they cannot be copied + } + +protected: + std::vector values_; + std::vector checkpoint_; + opentelemetry::metrics::BoundInstrumentKind kind_; + std::mutex mu_; + +}; + +} // namespace metrics +} // namespace sdk +OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h new file mode 100644 index 0000000000..8d50ed830d --- /dev/null +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h @@ -0,0 +1,104 @@ +#pragma once + +#include "opentelemetry/version.h" +#include "opentelemetry/metrics/instrument.h" +#include "opentelemetry/sdk/metrics/aggregator/aggregator.h" + +#include +#include +#include +#include + +namespace metrics_api = opentelemetry::metrics; + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace sdk +{ +namespace metrics +{ + +template +class CounterAggregator : public Aggregator +{ + +public: + CounterAggregator(metrics_api::BoundInstrumentKind kind) + { + this->kind_ = kind; + this->values_ = std::vector(1, 0); + this->checkpoint_ = std::vector(1, 0); + } + + /** + * Recieves a captured value from the instrument and applies it to the current aggregator value. + * + * @param val, the raw value used in aggregation + * @return none + */ + void update(T val) override + { + this->mu_.lock(); + this->values_[0] += val; // atomic operation + this->mu_.unlock(); + } + + /** + * Checkpoints the current value. This function will overwrite the current checkpoint with the + * current value. + * + * @param none + * @return none + */ + void checkpoint() override + { + this->checkpoint_ = this->values_; + this->values_[0] = 0; + } + + /** + * Merges the values of two aggregators in a semantically accurate manner. + * In this case, merging only requires the the current values of the two aggregators be summed. + * + * @param other, the aggregator with merge with + * @return none + */ + void merge(CounterAggregator other) + { + if (this->kind_ == other.kind_) + { + this->mu_.lock(); + this->values_[0] += other.values_[0]; // atomic operation afaik + this->mu_.unlock(); + } + else + { + //AggregatorMismatch Exception + } + } + + /** + * Returns the checkpointed value + * + * @param none + * @return the value of the checkpoint + */ + std::vector get_checkpoint() override + { + return this->checkpoint_; + } + + /** + * Returns the instrument kind which this aggregator is associated with + * + * @param none + * @return the BoundInstrumentKind of the aggregator's owner + */ + std::vector get_values() override{ + return this->values_; + } + +}; + +} +} +OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h new file mode 100644 index 0000000000..b0d705317d --- /dev/null +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h @@ -0,0 +1,160 @@ +#pragma once + +#include "opentelemetry/version.h" +#include "opentelemetry/metrics/instrument.h" +#include "opentelemetry/sdk/metrics/aggregator/aggregator.h" + +#include +#include +#include +#include + +namespace metrics_api = opentelemetry::metrics; + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace sdk +{ +namespace metrics +{ +template +class HistogramAggregator : public Aggregator +{ + +public: + + /** + * Constructor for the histogram aggregator. A sorted vector of boundaries is expected and boundaries are + * doubles regardless of the aggregator's templated data type. + * + * Sum is stored in values_[0] + * Count is stored in position_[1] + */ + HistogramAggregator(metrics_api::BoundInstrumentKind kind, std::vector boundaries) + { + this->kind_ = kind; + boundaries_ = boundaries; + this->values_ = std::vector(2, 0); + this->checkpoint_ = std::vector(2, 0); + bucketCounts_ = std::vector(boundaries_.size() + 1, 0); + } + + /** + * Recieves a captured value from the instrument and inserts it into the current histogram counts. + * Depending on the use case, linear search or binary search based implementations may be preferred. + * In a uniformly distributed dataset, linear search outperforms binary search until 512 buckets. + * + * @param val, the raw value used in aggregation + * @return none + */ + void update(T val) override + { + + int bucketID = boundaries_.size(); + for (int i = 0; i < boundaries_.size(); i++) + { + if (val < boundaries_[i]) // concurrent read is thread-safe + { + bucketID = i; + break; + } + } + + // Alternate implementation with binary search + //auto pos = std::lower_bound (boundaries_.begin(), boundaries_.end(), val); + //bucketCounts_[pos-boundaries_.begin()] += 1; + + this->mu_.lock(); + this->values_[0] += val; + this->values_[1] += 1; + bucketCounts_[bucketID] += 1; + this->mu_.unlock(); + } + + /** + * Checkpoints the current value. This function will overwrite the current checkpoint with the + * current value. + * + * @param none + * @return none + */ + void checkpoint() override + { + this->checkpoint_ = this->values_; + this->values_[0]=0; + this->values_[1]=0; + } + + /** + * Merges the values of two aggregators in a semantically accurate manner. + * A histogram aggregator can only be merged with another histogram aggregatos with the same boudnaries. + * A histogram merge first adds the sum and count values then iterates over the adds the bucket counts + * element by element. + * + * @param other, the aggregator with merge with + * @return none + */ + void merge(HistogramAggregator other) + { + this->mu_.lock(); + this->values_[0] += other.values_[0]; + this->values_[1] += other.values_[1]; + + for (int i = 0; i < bucketCounts_.size(); i++) + { + bucketCounts_[i] += other.bucketCounts_[i]; + } + this->mu_.unlock(); + } + + /** + * Returns the checkpointed value + * + * @param none + * @return the value of the checkpoint + */ + std::vector get_checkpoint() override + { + return this->checkpoint_; // what happens if there isn't a checkpoint? + } + + /** + * Returns the current value + * + * @param none + * @return the present aggregator value + */ + std::vector get_values() override + { + return this->values_; + } + + /** + * Returns the bucket boundaries specified at this aggregator's creation. + * + * @param none + * @return the aggregator boundaries + */ + std::vector get_boundaries() + { + return bucketCounts_; + } + + /** + * Returns the current counts for each bucket . + * + * @param none + * @return the aggregator bucket counts + */ + std::vector get_counts() + { + return bucketCounts_; + } + +private: + std::vector boundaries_; + std::vector bucketCounts_; +}; + +} +} +OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/min_max_sum_count_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/min_max_sum_count_aggregator.h new file mode 100644 index 0000000000..298abba441 --- /dev/null +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/min_max_sum_count_aggregator.h @@ -0,0 +1,120 @@ +#pragma once + +#include "opentelemetry/version.h" +#include "opentelemetry/metrics/instrument.h" +#include "opentelemetry/sdk/metrics/aggregator/aggregator.h" + +#include +#include +#include +#include + +namespace metrics_api = opentelemetry::metrics; + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace sdk +{ +namespace metrics +{ +template +class MinMaxSumCountAggregator : public Aggregator +{ +public: + explicit MinMaxSumCountAggregator(metrics_api::BoundInstrumentKind kind) + { + static_assert(std::is_arithmetic::value, "Not an arithmetic type"); + this->kind_ = kind; + this->values_ = std::vector(4, 0); // {min, max, sum count} + this->checkpoint_ = this->values_; + } + + /** + * Receives a captured value from the instrument and applies it to the current aggregator value. + * + * @param val, the raw value used in aggregation + */ + void update(T val) override + { + this->mu_.lock(); + + if (this->values_[3] == 0 || val < this->values_[0]) // set min + this->values_[0] = val; + if (this->values_[3] == 0 || val > this->values_[1]) // set max + this->values_[1] = val; + + this->values_[2] += val; // compute sum + this->values_[3]++; // increment count + + this->mu_.unlock(); + } + + /** + * Checkpoints the current value. This function will overwrite the current checkpoint with the + * current value. + * + */ + void checkpoint() override + { + this->mu_.lock(); + this->checkpoint_ = this->values_; + // Reset the values + this->values_[0] = 0; + this->values_[1] = 0; + this->values_[2] = 0; + this->values_[3] = 0; + this->mu_.unlock(); + } + + /** + * Merges two MinMaxSumCount aggregators together + * + * @param other the aggregator to merge with this aggregator + */ + void merge(const MinMaxSumCountAggregator &other) + { + if (this->kind_ == other.kind_) + { + this->mu_.lock(); + // set min + if (other.values_[0] < this->values_[0]) + this->values_[0] = other.values_[0]; + // set max + if (other.values_[1] > this->values_[1]) + this->values_[1] = other.values_[1]; + // set sum + this->values_[2] += other.values_[2]; + // set count + this->values_[3] += other.values_[3]; + this->mu_.unlock(); + } + else + { + // Log error + return; + } + } + + /** + * Returns the checkpointed value + * + * @return the value of the checkpoint + */ + std::vector get_checkpoint() override + { + return this->checkpoint_; + } + + /** + * Returns the values currently held by the aggregator + * + * @return the values held by the aggregator + */ + std::vector get_values() override + { + return this->values_; + } + +}; +} +} +OPENTELEMETRY_END_NAMESPACE From bcd519dd3b242d148cc12cad934edcb301f69d6d Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Wed, 15 Jul 2020 11:52:11 -0400 Subject: [PATCH 17/30] test files --- sdk/test/metrics/BUILD | 33 +++++ sdk/test/metrics/counter_aggregator_test.cc | 111 ++++++++++++++ sdk/test/metrics/histogram_aggregator_test.cc | 135 ++++++++++++++++++ 3 files changed, 279 insertions(+) create mode 100644 sdk/test/metrics/counter_aggregator_test.cc create mode 100644 sdk/test/metrics/histogram_aggregator_test.cc diff --git a/sdk/test/metrics/BUILD b/sdk/test/metrics/BUILD index e2d5c5cb71..8121a723f7 100644 --- a/sdk/test/metrics/BUILD +++ b/sdk/test/metrics/BUILD @@ -2,6 +2,39 @@ cc_test( name = "meter_provider_sdk_test", srcs = [ "meter_provider_sdk_test.cc", +], + deps = [ + "//sdk/src/metrics", + "@com_google_googletest//:gtest_main", + ], +) + +cc_test( + name = "counter_aggregator_test", + srcs = [ + "counter_aggregator_test.cc", + ], + deps = [ + "//sdk/src/metrics", + "@com_google_googletest//:gtest_main", + ], +) + +cc_test( + name = "histogram_aggregator_test", + srcs = [ + "histogram_aggregator_test.cc", + ], + deps = [ + "//sdk/src/metrics", + "@com_google_googletest//:gtest_main", + ], +) + +cc_test( + name = "metric_instrument_test", + srcs = [ + "metric_instrument_test.cc", ], deps = [ "//sdk/src/metrics", diff --git a/sdk/test/metrics/counter_aggregator_test.cc b/sdk/test/metrics/counter_aggregator_test.cc new file mode 100644 index 0000000000..3fea68ecaa --- /dev/null +++ b/sdk/test/metrics/counter_aggregator_test.cc @@ -0,0 +1,111 @@ +#include "opentelemetry/sdk/metrics/aggregator/counter_aggregator.h" + +#include +#include +#include + +namespace metrics_api = opentelemetry::metrics; + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace sdk +{ +namespace metrics +{ + +TEST(CounterAggregator, NoUpdates) +{ + CounterAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter); + + EXPECT_EQ(alpha.get_checkpoint().size(), 1); + EXPECT_EQ(alpha.get_checkpoint()[0], 0); + + alpha.checkpoint(); + EXPECT_EQ(alpha.get_checkpoint().size(), 1); + EXPECT_EQ(alpha.get_checkpoint()[0], 0); +} + +TEST(CounterAggregator, Update) +{ + CounterAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter); + CounterAggregator beta(metrics_api::BoundInstrumentKind::BoundIntCounter); + + for (int i = 0; i<123456; i++){ + alpha.update(1); + } + + int sum = 0; + for (int i = 0; i < 100; i++){ + int tmp = std::rand(); + beta.update(tmp); + sum+=tmp; + } + + EXPECT_EQ(alpha.get_checkpoint()[0], 0); // checkpoint shouldn't change even with updates + EXPECT_EQ(beta.get_checkpoint()[0], 0); + + alpha.checkpoint(); + beta.checkpoint(); + + EXPECT_EQ(alpha.get_checkpoint()[0], 123456); + EXPECT_EQ(beta.get_checkpoint()[0], sum); + + alpha.update(15); + alpha.checkpoint(); + EXPECT_EQ(alpha.get_checkpoint()[0], 15); //reset to 0 after first checkpoint call +} + +// callback update function used to test concurrent calls +void incrementingCallback(Aggregator & agg) +{ + for (int i = 0; i< 2000000; i++){ + agg.update(1); + } +} + +TEST(CounterAggregator, Concurrency){ + CounterAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter); + + // spawn new threads that initiate the callback + std::thread first (incrementingCallback, std::ref(alpha)); + std::thread second (incrementingCallback, std::ref(alpha)); + + + first.join(); + second.join(); + + alpha.checkpoint(); + + // race conditions result in values below 2*2000000 + EXPECT_EQ(alpha.get_checkpoint()[0], 2*2000000); +} + +TEST(CounterAggregator, Merge) +{ + CounterAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter); + CounterAggregator beta(metrics_api::BoundInstrumentKind::BoundIntCounter); + + alpha.merge(beta); + + alpha.checkpoint(); + EXPECT_EQ(alpha.get_checkpoint()[0], 0); //merge with no updates + + for (int i = 0; i<500; i++){ + alpha.update(1); + } + + for (int i = 0; i<700; i++){ + beta.update(1); + } + + alpha.merge(beta); + alpha.checkpoint(); + EXPECT_EQ(alpha.get_checkpoint()[0], 1200); + + //HistogramAggregator gamma(metrics_api::BoundInstrumentKind::BoundValueRecorder); + //ASSERT_THROW(alpha.merge(gamma), AggregatorMismatch); +} + + +} // namespace sdk +} // namespace metrics +OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/test/metrics/histogram_aggregator_test.cc b/sdk/test/metrics/histogram_aggregator_test.cc new file mode 100644 index 0000000000..4153049d4f --- /dev/null +++ b/sdk/test/metrics/histogram_aggregator_test.cc @@ -0,0 +1,135 @@ +#include "opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h" + +#include +#include +#include +// #include + +namespace metrics_api = opentelemetry::metrics; + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace sdk +{ +namespace metrics +{ + +// Test updating with a uniform set of updates +TEST(Histogram, Uniform) +{ + std::vector boundaries{10,20,30,40,50}; + HistogramAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); + + alpha.checkpoint(); + EXPECT_EQ(alpha.get_checkpoint().size(),2); + EXPECT_EQ(alpha.get_counts().size(),6); + + for (int i = 0; i< 60; i++){ + alpha.update(i); + } + + alpha.checkpoint(); + + EXPECT_EQ(alpha.get_checkpoint()[0], 1770); + EXPECT_EQ(alpha.get_checkpoint()[1], 60); + + std::vector correct = {10,10,10,10,10,10}; + EXPECT_EQ(alpha.get_counts(), correct); +} + +// Test updating with a normal distribution +TEST(Histogram, Normal) +{ + std::vector boundaries{2,4,6,8,10,12}; + HistogramAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); + + std::vector vals{1,3,3,5,5,5,7,7,7,7,9,9,9,11,11,13}; + for (int i : vals){ + alpha.update(i); + } + + alpha.checkpoint(); + + EXPECT_EQ(alpha.get_checkpoint()[0], std::accumulate(vals.begin(),vals.end(),0)); + EXPECT_EQ(alpha.get_checkpoint()[1], vals.size()); + + std::vector correct = {1,2,3,4,3,2,1}; + EXPECT_EQ(alpha.get_counts(), correct); +} + +TEST(Histogram, Merge){ + std::vector boundaries{2,4,6,8,10,12}; + HistogramAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); + HistogramAggregator beta(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); + + std::vector vals{1,3,3,5,5,5,7,7,7,7,9,9,9,11,11,13}; + for (int i : vals){ + alpha.update(i); + } + + std::vector otherVals{1,1,1,1,11,11,13,13,13,15}; + for (int i : otherVals){ + beta.update(i); + } + alpha.merge(beta); + alpha.checkpoint(); + + EXPECT_EQ(alpha.get_checkpoint()[0], std::accumulate(vals.begin(),vals.end(),0)+std::accumulate(otherVals.begin(), otherVals.end(), 0)); + EXPECT_EQ(alpha.get_checkpoint()[1], vals.size()+otherVals.size()); + + std::vector correct = {5,2,3,4,3,4,5}; + EXPECT_EQ(alpha.get_counts(), correct); +} + +// Update callback used to validate multi-threaded performance +void histogramUpdateCallback(Aggregator & agg, std::vector vals){ + for (int i: vals){ + agg.update(i); + } +} + +int randVal(){ + return rand() % 15; +} + +TEST(Histogram, Concurrency){ + std::vector boundaries{2,4,6,8,10,12}; + HistogramAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); + + std::vector vals1(1000); + std::generate(vals1.begin(),vals1.end(),randVal); + + std::vector vals2(1000); + std::generate(vals2.begin(),vals2.end(),randVal); + + std::thread first (histogramUpdateCallback, std::ref(alpha), vals1); + std::thread second (histogramUpdateCallback, std::ref(alpha), vals2); + + first.join(); + second.join(); + + HistogramAggregator beta(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); + + + // Timing harness to compare linear and binary insertion + // auto start = std::chrono::system_clock::now(); + for (int i: vals1){ + beta.update(i); + } + for (int i: vals2){ + beta.update(i); + } + //auto end = std::chrono::system_clock::now(); + //auto elapsed = std::chrono::duration_cast(end - start); + //std::cout <<"Update time: " < Date: Wed, 15 Jul 2020 12:59:59 -0400 Subject: [PATCH 18/30] final specifier --- .../opentelemetry/sdk/metrics/aggregator/counter_aggregator.h | 2 +- .../opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h index 8d50ed830d..29f4f9356a 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h @@ -18,7 +18,7 @@ namespace metrics { template -class CounterAggregator : public Aggregator +class CounterAggregator final : public Aggregator { public: diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h index b0d705317d..5777917e34 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h @@ -17,7 +17,7 @@ namespace sdk namespace metrics { template -class HistogramAggregator : public Aggregator +class HistogramAggregator final : public Aggregator { public: From 60865b3c1f2702b937de12f2804cd1540ece638c Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Wed, 15 Jul 2020 23:08:27 -0400 Subject: [PATCH 19/30] cleaning up for PR --- .../aggregator/min_max_sum_count_aggregator.h | 120 ------------------ 1 file changed, 120 deletions(-) delete mode 100644 sdk/include/opentelemetry/sdk/metrics/aggregator/min_max_sum_count_aggregator.h diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/min_max_sum_count_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/min_max_sum_count_aggregator.h deleted file mode 100644 index 298abba441..0000000000 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/min_max_sum_count_aggregator.h +++ /dev/null @@ -1,120 +0,0 @@ -#pragma once - -#include "opentelemetry/version.h" -#include "opentelemetry/metrics/instrument.h" -#include "opentelemetry/sdk/metrics/aggregator/aggregator.h" - -#include -#include -#include -#include - -namespace metrics_api = opentelemetry::metrics; - -OPENTELEMETRY_BEGIN_NAMESPACE -namespace sdk -{ -namespace metrics -{ -template -class MinMaxSumCountAggregator : public Aggregator -{ -public: - explicit MinMaxSumCountAggregator(metrics_api::BoundInstrumentKind kind) - { - static_assert(std::is_arithmetic::value, "Not an arithmetic type"); - this->kind_ = kind; - this->values_ = std::vector(4, 0); // {min, max, sum count} - this->checkpoint_ = this->values_; - } - - /** - * Receives a captured value from the instrument and applies it to the current aggregator value. - * - * @param val, the raw value used in aggregation - */ - void update(T val) override - { - this->mu_.lock(); - - if (this->values_[3] == 0 || val < this->values_[0]) // set min - this->values_[0] = val; - if (this->values_[3] == 0 || val > this->values_[1]) // set max - this->values_[1] = val; - - this->values_[2] += val; // compute sum - this->values_[3]++; // increment count - - this->mu_.unlock(); - } - - /** - * Checkpoints the current value. This function will overwrite the current checkpoint with the - * current value. - * - */ - void checkpoint() override - { - this->mu_.lock(); - this->checkpoint_ = this->values_; - // Reset the values - this->values_[0] = 0; - this->values_[1] = 0; - this->values_[2] = 0; - this->values_[3] = 0; - this->mu_.unlock(); - } - - /** - * Merges two MinMaxSumCount aggregators together - * - * @param other the aggregator to merge with this aggregator - */ - void merge(const MinMaxSumCountAggregator &other) - { - if (this->kind_ == other.kind_) - { - this->mu_.lock(); - // set min - if (other.values_[0] < this->values_[0]) - this->values_[0] = other.values_[0]; - // set max - if (other.values_[1] > this->values_[1]) - this->values_[1] = other.values_[1]; - // set sum - this->values_[2] += other.values_[2]; - // set count - this->values_[3] += other.values_[3]; - this->mu_.unlock(); - } - else - { - // Log error - return; - } - } - - /** - * Returns the checkpointed value - * - * @return the value of the checkpoint - */ - std::vector get_checkpoint() override - { - return this->checkpoint_; - } - - /** - * Returns the values currently held by the aggregator - * - * @return the values held by the aggregator - */ - std::vector get_values() override - { - return this->values_; - } - -}; -} -} -OPENTELEMETRY_END_NAMESPACE From c6debd371b169f636d2e807077a52d2dc240f4ac Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Wed, 15 Jul 2020 23:11:08 -0400 Subject: [PATCH 20/30] removing irrelevant test targets --- sdk/test/metrics/BUILD | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/sdk/test/metrics/BUILD b/sdk/test/metrics/BUILD index 8121a723f7..cd82d5c4cb 100644 --- a/sdk/test/metrics/BUILD +++ b/sdk/test/metrics/BUILD @@ -31,13 +31,3 @@ cc_test( ], ) -cc_test( - name = "metric_instrument_test", - srcs = [ - "metric_instrument_test.cc", - ], - deps = [ - "//sdk/src/metrics", - "@com_google_googletest//:gtest_main", - ], -) From 161f719410f6347ab0ae6f13afce4e5d743fd260 Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Tue, 21 Jul 2020 00:21:28 -0400 Subject: [PATCH 21/30] incorporating feedback --- .../sdk/metrics/aggregator/aggregator.h | 45 ++++++++++++++---- .../metrics/aggregator/counter_aggregator.h | 21 ++++----- .../metrics/aggregator/histogram_aggregator.h | 46 +++++++++++++------ sdk/test/metrics/counter_aggregator_test.cc | 12 ++--- sdk/test/metrics/histogram_aggregator_test.cc | 28 ++++++++--- 5 files changed, 105 insertions(+), 47 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h index 00ed434f07..c17a5ae8da 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h @@ -1,12 +1,12 @@ #pragma once -#include "opentelemetry/version.h" +#include #include "opentelemetry/metrics/instrument.h" - +#include "opentelemetry/version.h" #include #include -#include -#include + + namespace metrics_api = opentelemetry::metrics; @@ -15,6 +15,17 @@ namespace sdk { namespace metrics { + +enum class AggregatorKind +{ + Counter = 0, + MinMaxSumCount = 1, + Gauge = 2, + Sketch = 3, + Histogram = 4, + Exact = 5, +}; + /* * Performs calculations necessary to combine updates from instruments into an * insightful value. @@ -29,7 +40,7 @@ class Aggregator Aggregator() = default; /** - * Recieves a captured value from the instrument and applies it to the current aggregator value. + * Receives a captured value from the instrument and applies it to the current aggregator value. * * @param val, the raw value used in aggregation * @return none @@ -53,7 +64,7 @@ class Aggregator * @param other, the aggregator with merge with * @return none */ - void merge(std::shared_ptr other); + void merge(Aggregator * other); /** * Returns the checkpointed value @@ -75,26 +86,40 @@ class Aggregator * Returns the instrument kind which this aggregator is associated with * * @param none - * @return the BoundInstrumentKind of the aggregator's owner + * @return the InstrumentKind of the aggregator's owner */ - virtual opentelemetry::metrics::BoundInstrumentKind get_kind() + virtual opentelemetry::metrics::InstrumentKind get_instrument_kind() final { return kind_; } + /** + * Returns the type of this aggregator + * + * @param none + * @return the AggregatorKind of this instrument + */ + virtual AggregatorKind get_aggregator_kind() final + { + return agg_kind_; + } + // Custom copy constructor to handle the mutex - Aggregator(const Aggregator &cp) { + Aggregator(const Aggregator &cp) + { values_ = cp.values_; checkpoint_ = cp.checkpoint_; kind_ = cp.kind_; + agg_kind_ = cp.agg_kind_; // use default initialized mutex as they cannot be copied } protected: std::vector values_; std::vector checkpoint_; - opentelemetry::metrics::BoundInstrumentKind kind_; + opentelemetry::metrics::InstrumentKind kind_; std::mutex mu_; + AggregatorKind agg_kind_; }; diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h index 29f4f9356a..ecdfae35aa 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h @@ -7,7 +7,6 @@ #include #include #include -#include namespace metrics_api = opentelemetry::metrics; @@ -22,11 +21,12 @@ class CounterAggregator final : public Aggregator { public: - CounterAggregator(metrics_api::BoundInstrumentKind kind) + CounterAggregator(metrics_api::InstrumentKind kind) { this->kind_ = kind; this->values_ = std::vector(1, 0); this->checkpoint_ = std::vector(1, 0); + this->agg_kind_ = AggregatorKind::Counter; } /** @@ -64,15 +64,13 @@ class CounterAggregator final : public Aggregator */ void merge(CounterAggregator other) { - if (this->kind_ == other.kind_) - { + if (this->agg_kind_ == other.agg_kind_) { this->mu_.lock(); - this->values_[0] += other.values_[0]; // atomic operation afaik + this->values_[0] += other.values_[0]; this->mu_.unlock(); } - else - { - //AggregatorMismatch Exception + else { + throw std::invalid_argument("Aggregators of different types cannot be merged."); } } @@ -88,12 +86,13 @@ class CounterAggregator final : public Aggregator } /** - * Returns the instrument kind which this aggregator is associated with + * Returns the current values * * @param none - * @return the BoundInstrumentKind of the aggregator's owner + * @return the present aggregator values */ - std::vector get_values() override{ + std::vector get_values() override + { return this->values_; } diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h index 5777917e34..995ad7ae87 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h @@ -1,13 +1,13 @@ #pragma once -#include "opentelemetry/version.h" +#include +#include #include "opentelemetry/metrics/instrument.h" +#include "opentelemetry/version.h" #include "opentelemetry/sdk/metrics/aggregator/aggregator.h" - +#include #include #include -#include -#include namespace metrics_api = opentelemetry::metrics; @@ -16,6 +16,7 @@ namespace sdk { namespace metrics { + template class HistogramAggregator final : public Aggregator { @@ -29,9 +30,13 @@ class HistogramAggregator final : public Aggregator * Sum is stored in values_[0] * Count is stored in position_[1] */ - HistogramAggregator(metrics_api::BoundInstrumentKind kind, std::vector boundaries) + HistogramAggregator(metrics_api::InstrumentKind kind, std::vector boundaries) { + if (!std::is_sorted(boundaries.begin(),boundaries.end())){ + throw std::invalid_argument("Histogram boundaries must be monotonic."); + } this->kind_ = kind; + this->agg_kind_ = AggregatorKind::Histogram; boundaries_ = boundaries; this->values_ = std::vector(2, 0); this->checkpoint_ = std::vector(2, 0); @@ -40,15 +45,19 @@ class HistogramAggregator final : public Aggregator /** * Recieves a captured value from the instrument and inserts it into the current histogram counts. - * Depending on the use case, linear search or binary search based implementations may be preferred. - * In a uniformly distributed dataset, linear search outperforms binary search until 512 buckets. + * + * Depending on the use case, a linear search or binary search based implementation may be preferred. + * In uniformly distributed datasets, linear search outperforms binary search until 512 buckets. However, + * if the distribution is strongly skewed right ( for example server latency where most values may be <10ms + * but the range is from 0 - 1000 ms), a linear search could be superior even with more than 500 buckets as + * almost all values inserted would be at the beginning of the boundaries array and thus found more quickly + * through linear search. * * @param val, the raw value used in aggregation * @return none */ void update(T val) override { - int bucketID = boundaries_.size(); for (int i = 0; i < boundaries_.size(); i++) { @@ -60,8 +69,8 @@ class HistogramAggregator final : public Aggregator } // Alternate implementation with binary search - //auto pos = std::lower_bound (boundaries_.begin(), boundaries_.end(), val); - //bucketCounts_[pos-boundaries_.begin()] += 1; + // auto pos = std::lower_bound (boundaries_.begin(), boundaries_.end(), val); + // bucketCounts_[pos-boundaries_.begin()] += 1; this->mu_.lock(); this->values_[0] += val; @@ -86,7 +95,7 @@ class HistogramAggregator final : public Aggregator /** * Merges the values of two aggregators in a semantically accurate manner. - * A histogram aggregator can only be merged with another histogram aggregatos with the same boudnaries. + * A histogram aggregator can only be merged with another histogram aggregator that has the same boudnaries. * A histogram merge first adds the sum and count values then iterates over the adds the bucket counts * element by element. * @@ -96,6 +105,15 @@ class HistogramAggregator final : public Aggregator void merge(HistogramAggregator other) { this->mu_.lock(); + + // Ensure that incorrect types are not merged + if (this->agg_kind_ != other.agg_kind_){ + throw std::invalid_argument("Aggregators of different types cannot be merged."); + // Reject histogram merges with differing boundary vectors + } else if (other.boundaries_ != this->boundaries_){ + throw std::invalid_argument("Histogram boundaries do not match."); + } + this->values_[0] += other.values_[0]; this->values_[1] += other.values_[1]; @@ -114,14 +132,14 @@ class HistogramAggregator final : public Aggregator */ std::vector get_checkpoint() override { - return this->checkpoint_; // what happens if there isn't a checkpoint? + return this->checkpoint_; } /** - * Returns the current value + * Returns the current values * * @param none - * @return the present aggregator value + * @return the present aggregator values */ std::vector get_values() override { diff --git a/sdk/test/metrics/counter_aggregator_test.cc b/sdk/test/metrics/counter_aggregator_test.cc index 3fea68ecaa..8208983b89 100644 --- a/sdk/test/metrics/counter_aggregator_test.cc +++ b/sdk/test/metrics/counter_aggregator_test.cc @@ -14,7 +14,7 @@ namespace metrics TEST(CounterAggregator, NoUpdates) { - CounterAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter); + CounterAggregator alpha(metrics_api::InstrumentKind::Counter); EXPECT_EQ(alpha.get_checkpoint().size(), 1); EXPECT_EQ(alpha.get_checkpoint()[0], 0); @@ -26,8 +26,8 @@ TEST(CounterAggregator, NoUpdates) TEST(CounterAggregator, Update) { - CounterAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter); - CounterAggregator beta(metrics_api::BoundInstrumentKind::BoundIntCounter); + CounterAggregator alpha(metrics_api::InstrumentKind::Counter); + CounterAggregator beta(metrics_api::InstrumentKind::Counter); for (int i = 0; i<123456; i++){ alpha.update(1); @@ -63,7 +63,7 @@ void incrementingCallback(Aggregator & agg) } TEST(CounterAggregator, Concurrency){ - CounterAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter); + CounterAggregator alpha(metrics_api::InstrumentKind::Counter); // spawn new threads that initiate the callback std::thread first (incrementingCallback, std::ref(alpha)); @@ -81,8 +81,8 @@ TEST(CounterAggregator, Concurrency){ TEST(CounterAggregator, Merge) { - CounterAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter); - CounterAggregator beta(metrics_api::BoundInstrumentKind::BoundIntCounter); + CounterAggregator alpha(metrics_api::InstrumentKind::Counter); + CounterAggregator beta(metrics_api::InstrumentKind::Counter); alpha.merge(beta); diff --git a/sdk/test/metrics/histogram_aggregator_test.cc b/sdk/test/metrics/histogram_aggregator_test.cc index 4153049d4f..4b6cbd795c 100644 --- a/sdk/test/metrics/histogram_aggregator_test.cc +++ b/sdk/test/metrics/histogram_aggregator_test.cc @@ -3,6 +3,7 @@ #include #include #include +#include // #include namespace metrics_api = opentelemetry::metrics; @@ -17,7 +18,9 @@ namespace metrics TEST(Histogram, Uniform) { std::vector boundaries{10,20,30,40,50}; - HistogramAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); + HistogramAggregator alpha(metrics_api::InstrumentKind::Counter, boundaries); + + EXPECT_EQ(alpha.get_aggregator_kind(), AggregatorKind::Histogram); alpha.checkpoint(); EXPECT_EQ(alpha.get_checkpoint().size(),2); @@ -40,7 +43,7 @@ TEST(Histogram, Uniform) TEST(Histogram, Normal) { std::vector boundaries{2,4,6,8,10,12}; - HistogramAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); + HistogramAggregator alpha(metrics_api::InstrumentKind::Counter, boundaries); std::vector vals{1,3,3,5,5,5,7,7,7,7,9,9,9,11,11,13}; for (int i : vals){ @@ -58,8 +61,8 @@ TEST(Histogram, Normal) TEST(Histogram, Merge){ std::vector boundaries{2,4,6,8,10,12}; - HistogramAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); - HistogramAggregator beta(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); + HistogramAggregator alpha(metrics_api::InstrumentKind::Counter, boundaries); + HistogramAggregator beta(metrics_api::InstrumentKind::Counter, boundaries); std::vector vals{1,3,3,5,5,5,7,7,7,7,9,9,9,11,11,13}; for (int i : vals){ @@ -70,6 +73,7 @@ TEST(Histogram, Merge){ for (int i : otherVals){ beta.update(i); } + alpha.merge(beta); alpha.checkpoint(); @@ -93,7 +97,7 @@ int randVal(){ TEST(Histogram, Concurrency){ std::vector boundaries{2,4,6,8,10,12}; - HistogramAggregator alpha(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); + HistogramAggregator alpha(metrics_api::InstrumentKind::Counter, boundaries); std::vector vals1(1000); std::generate(vals1.begin(),vals1.end(),randVal); @@ -107,7 +111,7 @@ TEST(Histogram, Concurrency){ first.join(); second.join(); - HistogramAggregator beta(metrics_api::BoundInstrumentKind::BoundIntCounter, boundaries); + HistogramAggregator beta(metrics_api::InstrumentKind::Counter, boundaries); // Timing harness to compare linear and binary insertion @@ -129,6 +133,18 @@ TEST(Histogram, Concurrency){ EXPECT_EQ(alpha.get_counts(), beta.get_counts()); } +TEST(Histogram, Errors){ + std::vector boundaries{2,4,6,8,10,12}; + std::vector boundaries2{1,4,6,8,10,12}; + std::vector unsortedBoundaries{10,12,4,6,8}; + EXPECT_ANY_THROW(HistogramAggregator alpha(metrics_api::InstrumentKind::Counter, unsortedBoundaries)); + + HistogramAggregator beta(metrics_api::InstrumentKind::Counter, boundaries); + HistogramAggregator gamma(metrics_api::InstrumentKind::Counter, boundaries2); + + EXPECT_ANY_THROW(beta.merge(gamma)); +} + } // namespace sdk } // namespace metrics From 739faa6cd5613c079472890f970eb24d7fccedbe Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Fri, 24 Jul 2020 16:29:23 -0400 Subject: [PATCH 22/30] adding virtual functions --- .../sdk/metrics/aggregator/aggregator.h | 51 ++++++++++++------- .../metrics/aggregator/counter_aggregator.h | 4 +- .../metrics/aggregator/histogram_aggregator.h | 17 +++++-- 3 files changed, 50 insertions(+), 22 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h index c17a5ae8da..fe2ec1bb90 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h @@ -7,7 +7,6 @@ #include - namespace metrics_api = opentelemetry::metrics; OPENTELEMETRY_BEGIN_NAMESPACE @@ -18,12 +17,12 @@ namespace metrics enum class AggregatorKind { - Counter = 0, - MinMaxSumCount = 1, - Gauge = 2, - Sketch = 3, - Histogram = 4, - Exact = 5, + Counter = 0, + MinMaxSumCount = 1, + Gauge = 2, + Sketch = 3, + Histogram = 4, + Exact = 5, }; /* @@ -39,6 +38,8 @@ class Aggregator Aggregator() = default; + virtual ~Aggregator() = default; + /** * Receives a captured value from the instrument and applies it to the current aggregator value. * @@ -83,27 +84,43 @@ class Aggregator virtual std::vector get_values() = 0; /** - * Returns the instrument kind which this aggregator is associated with - * - * @param none - * @return the InstrumentKind of the aggregator's owner - */ + * Returns the instrument kind which this aggregator is associated with + * + * @param none + * @return the InstrumentKind of the aggregator's owner + */ virtual opentelemetry::metrics::InstrumentKind get_instrument_kind() final { return kind_; } /** - * Returns the type of this aggregator - * - * @param none - * @return the AggregatorKind of this instrument - */ + * Returns the type of this aggregator + * + * @param none + * @return the AggregatorKind of this instrument + */ virtual AggregatorKind get_aggregator_kind() final { return agg_kind_; } + virtual std::vector get_boundaries() { + return std::vector(); + } + + virtual std::vector get_counts() { + return std::vector(); + } + + virtual bool get_quant_estimation () { + return false; + } + + virtual T get_quantiles(double q) { + return values_[0]; + } + // Custom copy constructor to handle the mutex Aggregator(const Aggregator &cp) { diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h index ecdfae35aa..61e68e840c 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h @@ -80,7 +80,7 @@ class CounterAggregator final : public Aggregator * @param none * @return the value of the checkpoint */ - std::vector get_checkpoint() override + virtual std::vector get_checkpoint() override { return this->checkpoint_; } @@ -91,7 +91,7 @@ class CounterAggregator final : public Aggregator * @param none * @return the present aggregator values */ - std::vector get_values() override + virtual std::vector get_values() override { return this->values_; } diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h index 995ad7ae87..9fa70ac524 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h @@ -152,9 +152,9 @@ class HistogramAggregator final : public Aggregator * @param none * @return the aggregator boundaries */ - std::vector get_boundaries() + virtual std::vector get_boundaries() override { - return bucketCounts_; + return boundaries_; } /** @@ -163,11 +163,22 @@ class HistogramAggregator final : public Aggregator * @param none * @return the aggregator bucket counts */ - std::vector get_counts() + virtual std::vector get_counts() override { return bucketCounts_; } + HistogramAggregator(const HistogramAggregator &cp) + { + this->values_ = cp.values_; + this->checkpoint_ = cp.checkpoint_; + this->kind_ = cp.kind_; + this->agg_kind_ = cp.agg_kind_; + boundaries_ = cp.boundaries_; + bucketCounts_ = cp.bucketCounts_; + // use default initialized mutex as they cannot be copied + } + private: std::vector boundaries_; std::vector bucketCounts_; From 19f664370ce91e6f2c995194e55cbe0845505b06 Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Sun, 26 Jul 2020 00:05:25 -0400 Subject: [PATCH 23/30] timestamp virtual function --- .../opentelemetry/sdk/metrics/aggregator/aggregator.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h index fe2ec1bb90..638ae5e451 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h @@ -1,6 +1,7 @@ #pragma once #include +#include "opentelemetry/core/timestamp.h" #include "opentelemetry/metrics/instrument.h" #include "opentelemetry/version.h" #include @@ -105,22 +106,31 @@ class Aggregator return agg_kind_; } + // virtual function to be overriden for the Histogram Aggregator virtual std::vector get_boundaries() { return std::vector(); } + // virtual function to be overriden for the Histogram Aggregator virtual std::vector get_counts() { return std::vector(); } + // virtual function to be overriden for Exact and Sketch Aggregators virtual bool get_quant_estimation () { return false; } + // virtual function to be overriden for Exact and Sketch Aggregators virtual T get_quantiles(double q) { return values_[0]; } + // virtual function to be overriden for Gauge Aggregator + virtual core::SystemTimestamp get_checkpoint_timestamp() { + return core::SystemTimestamp(); + } + // Custom copy constructor to handle the mutex Aggregator(const Aggregator &cp) { From 3a33a3cae0ce002cb0f1d545712326887404acde Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Mon, 27 Jul 2020 12:23:34 -0400 Subject: [PATCH 24/30] remove unused import --- .../opentelemetry/sdk/metrics/aggregator/aggregator.h | 1 - .../sdk/metrics/aggregator/counter_aggregator.h | 5 ++--- .../sdk/metrics/aggregator/histogram_aggregator.h | 1 - 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h index 638ae5e451..7ae8717c89 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h @@ -4,7 +4,6 @@ #include "opentelemetry/core/timestamp.h" #include "opentelemetry/metrics/instrument.h" #include "opentelemetry/version.h" -#include #include diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h index 61e68e840c..4a2edac875 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h @@ -1,12 +1,11 @@ #pragma once +#include #include "opentelemetry/version.h" #include "opentelemetry/metrics/instrument.h" #include "opentelemetry/sdk/metrics/aggregator/aggregator.h" - -#include #include -#include + namespace metrics_api = opentelemetry::metrics; diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h index 9fa70ac524..acddb6a568 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h @@ -6,7 +6,6 @@ #include "opentelemetry/version.h" #include "opentelemetry/sdk/metrics/aggregator/aggregator.h" #include -#include #include namespace metrics_api = opentelemetry::metrics; From c15942e16fb69aaeb79a9a6bf5a30cd195b5254c Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Mon, 27 Jul 2020 21:24:08 -0400 Subject: [PATCH 25/30] user specified exceptions --- .../metrics/aggregator/counter_aggregator.h | 5 ++++ .../metrics/aggregator/histogram_aggregator.h | 28 +++++++++++++++---- sdk/test/metrics/histogram_aggregator_test.cc | 3 ++ 3 files changed, 31 insertions(+), 5 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h index 4a2edac875..84da44b999 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h @@ -66,10 +66,15 @@ class CounterAggregator final : public Aggregator if (this->agg_kind_ == other.agg_kind_) { this->mu_.lock(); this->values_[0] += other.values_[0]; + this->checkpoint_[0] += other.checkpoint_[0]; this->mu_.unlock(); } else { +#if __EXCEPTIONS throw std::invalid_argument("Aggregators of different types cannot be merged."); +#else + std::terminate(); +#endif } } diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h index acddb6a568..0a9e776d93 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h @@ -32,7 +32,11 @@ class HistogramAggregator final : public Aggregator HistogramAggregator(metrics_api::InstrumentKind kind, std::vector boundaries) { if (!std::is_sorted(boundaries.begin(),boundaries.end())){ +#if __EXCEPTIONS throw std::invalid_argument("Histogram boundaries must be monotonic."); +#else + std::terminate(); +#endif } this->kind_ = kind; this->agg_kind_ = AggregatorKind::Histogram; @@ -40,6 +44,7 @@ class HistogramAggregator final : public Aggregator this->values_ = std::vector(2, 0); this->checkpoint_ = std::vector(2, 0); bucketCounts_ = std::vector(boundaries_.size() + 1, 0); + bucketCounts_ckpt_ = std::vector(boundaries_.size() + 1, 0); } /** @@ -58,7 +63,7 @@ class HistogramAggregator final : public Aggregator void update(T val) override { int bucketID = boundaries_.size(); - for (int i = 0; i < boundaries_.size(); i++) + for (size_t i = 0; i < boundaries_.size(); i++) { if (val < boundaries_[i]) // concurrent read is thread-safe { @@ -88,8 +93,10 @@ class HistogramAggregator final : public Aggregator void checkpoint() override { this->checkpoint_ = this->values_; - this->values_[0]=0; - this->values_[1]=0; + this->values_[0] = 0; + this->values_[1] = 0; + bucketCounts_ckpt_ = bucketCounts_; + std::fill(bucketCounts_.begin(), bucketCounts_.end(), 0); } /** @@ -107,18 +114,27 @@ class HistogramAggregator final : public Aggregator // Ensure that incorrect types are not merged if (this->agg_kind_ != other.agg_kind_){ +#if __EXCEPTIONS throw std::invalid_argument("Aggregators of different types cannot be merged."); +#else + std::terminate(); +#endif // Reject histogram merges with differing boundary vectors } else if (other.boundaries_ != this->boundaries_){ +#if __EXCEPTIONS throw std::invalid_argument("Histogram boundaries do not match."); +#else + std::terminate(); +#endif } this->values_[0] += other.values_[0]; this->values_[1] += other.values_[1]; - for (int i = 0; i < bucketCounts_.size(); i++) + for (size_t i = 0; i < bucketCounts_.size(); i++) { bucketCounts_[i] += other.bucketCounts_[i]; + bucketCounts_ckpt_[i] += other.bucketCounts_ckpt_[i]; } this->mu_.unlock(); } @@ -164,7 +180,7 @@ class HistogramAggregator final : public Aggregator */ virtual std::vector get_counts() override { - return bucketCounts_; + return bucketCounts_ckpt_; } HistogramAggregator(const HistogramAggregator &cp) @@ -175,12 +191,14 @@ class HistogramAggregator final : public Aggregator this->agg_kind_ = cp.agg_kind_; boundaries_ = cp.boundaries_; bucketCounts_ = cp.bucketCounts_; + bucketCounts_ckpt_ = cp.bucketCounts_ckpt_; // use default initialized mutex as they cannot be copied } private: std::vector boundaries_; std::vector bucketCounts_; + std::vector bucketCounts_ckpt_; }; } diff --git a/sdk/test/metrics/histogram_aggregator_test.cc b/sdk/test/metrics/histogram_aggregator_test.cc index 4b6cbd795c..fceb7fe13a 100644 --- a/sdk/test/metrics/histogram_aggregator_test.cc +++ b/sdk/test/metrics/histogram_aggregator_test.cc @@ -133,6 +133,8 @@ TEST(Histogram, Concurrency){ EXPECT_EQ(alpha.get_counts(), beta.get_counts()); } +#if __EXCEPTIONS + TEST(Histogram, Errors){ std::vector boundaries{2,4,6,8,10,12}; std::vector boundaries2{1,4,6,8,10,12}; @@ -145,6 +147,7 @@ TEST(Histogram, Errors){ EXPECT_ANY_THROW(beta.merge(gamma)); } +#endif } // namespace sdk } // namespace metrics From 2bfd39a69f07c537e87d8bcad37bfe94cab09d3d Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Tue, 28 Jul 2020 14:02:20 -0400 Subject: [PATCH 26/30] more virtuals for sketch aggregator --- .../opentelemetry/sdk/metrics/aggregator/aggregator.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h index 7ae8717c89..eddb538b8d 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h @@ -125,6 +125,16 @@ class Aggregator return values_[0]; } + // virtual function to be overriden for Sketch Aggregator + virtual double get_error_bound(double q) { + return 0; + } + + // virtual function to be overriden for Sketch Aggregator + virtual double get_max_buckets(double q) { + return 0; + } + // virtual function to be overriden for Gauge Aggregator virtual core::SystemTimestamp get_checkpoint_timestamp() { return core::SystemTimestamp(); From 72e437a888f78a74d04436ed54b21b1896127140 Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Tue, 28 Jul 2020 14:09:57 -0400 Subject: [PATCH 27/30] typos --- sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h index eddb538b8d..eb28453f6c 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h @@ -126,12 +126,12 @@ class Aggregator } // virtual function to be overriden for Sketch Aggregator - virtual double get_error_bound(double q) { + virtual double get_error_bound() { return 0; } // virtual function to be overriden for Sketch Aggregator - virtual double get_max_buckets(double q) { + virtual size_t get_max_buckets() { return 0; } From 73d13820394ea2dafd370bb116fbed9af869e5d1 Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Tue, 28 Jul 2020 15:54:45 -0400 Subject: [PATCH 28/30] Update sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h Co-authored-by: Reiley Yang --- .../opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h index 0a9e776d93..9303377fba 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h @@ -31,7 +31,7 @@ class HistogramAggregator final : public Aggregator */ HistogramAggregator(metrics_api::InstrumentKind kind, std::vector boundaries) { - if (!std::is_sorted(boundaries.begin(),boundaries.end())){ + if (!std::is_sorted(boundaries.begin(), boundaries.end())){ #if __EXCEPTIONS throw std::invalid_argument("Histogram boundaries must be monotonic."); #else From 75e48e1eee5f40a530eed9db00fc1dcafcd406af Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Tue, 28 Jul 2020 15:55:01 -0400 Subject: [PATCH 29/30] Update sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h Co-authored-by: Reiley Yang --- .../opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h index 9303377fba..0d3df40890 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h @@ -52,7 +52,7 @@ class HistogramAggregator final : public Aggregator * * Depending on the use case, a linear search or binary search based implementation may be preferred. * In uniformly distributed datasets, linear search outperforms binary search until 512 buckets. However, - * if the distribution is strongly skewed right ( for example server latency where most values may be <10ms + * if the distribution is strongly skewed right (for example server latency where most values may be <10ms * but the range is from 0 - 1000 ms), a linear search could be superior even with more than 500 buckets as * almost all values inserted would be at the beginning of the boundaries array and thus found more quickly * through linear search. From 8e7357e124f36d249542260dcecff5b9228b76ca Mon Sep 17 00:00:00 2001 From: Ankit Bhargava Date: Wed, 29 Jul 2020 16:25:11 -0400 Subject: [PATCH 30/30] formatting --- .../sdk/metrics/aggregator/aggregator.h | 237 ++++++------- .../metrics/aggregator/counter_aggregator.h | 154 ++++----- .../metrics/aggregator/histogram_aggregator.h | 325 +++++++++--------- sdk/test/metrics/BUILD | 3 +- sdk/test/metrics/counter_aggregator_test.cc | 160 ++++----- sdk/test/metrics/histogram_aggregator_test.cc | 239 +++++++------ 6 files changed, 548 insertions(+), 570 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h index eb28453f6c..b997514208 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h @@ -1,11 +1,10 @@ #pragma once #include +#include #include "opentelemetry/core/timestamp.h" #include "opentelemetry/metrics/instrument.h" #include "opentelemetry/version.h" -#include - namespace metrics_api = opentelemetry::metrics; @@ -17,12 +16,12 @@ namespace metrics enum class AggregatorKind { - Counter = 0, - MinMaxSumCount = 1, - Gauge = 2, - Sketch = 3, - Histogram = 4, - Exact = 5, + Counter = 0, + MinMaxSumCount = 1, + Gauge = 2, + Sketch = 3, + Histogram = 4, + Exact = 5, }; /* @@ -31,132 +30,110 @@ enum class AggregatorKind * Also stores current instrument values and checkpoints collected at intervals * governing the entire pipeline. */ -template +template class Aggregator { public: - - Aggregator() = default; - - virtual ~Aggregator() = default; - - /** - * Receives a captured value from the instrument and applies it to the current aggregator value. - * - * @param val, the raw value used in aggregation - * @return none - */ - virtual void update(T val) = 0; - - /** - * Checkpoints the current value. This function will overwrite the current checkpoint with the - * current value. - * - * @param none - * @return none - */ - virtual void checkpoint() = 0; - - /** - * Merges the values of two aggregators in a semantically accurate manner. - * Merging will occur differently for different aggregators depending on the - * way values are tracked. - * - * @param other, the aggregator with merge with - * @return none - */ - void merge(Aggregator * other); - - /** - * Returns the checkpointed value - * - * @param none - * @return the value of the checkpoint - */ - virtual std::vector get_checkpoint() = 0; - - /** - * Returns the current value - * - * @param none - * @return the present aggregator value - */ - virtual std::vector get_values() = 0; - - /** - * Returns the instrument kind which this aggregator is associated with - * - * @param none - * @return the InstrumentKind of the aggregator's owner - */ - virtual opentelemetry::metrics::InstrumentKind get_instrument_kind() final - { - return kind_; - } - - /** - * Returns the type of this aggregator - * - * @param none - * @return the AggregatorKind of this instrument - */ - virtual AggregatorKind get_aggregator_kind() final - { - return agg_kind_; - } - - // virtual function to be overriden for the Histogram Aggregator - virtual std::vector get_boundaries() { - return std::vector(); - } - - // virtual function to be overriden for the Histogram Aggregator - virtual std::vector get_counts() { - return std::vector(); - } - - // virtual function to be overriden for Exact and Sketch Aggregators - virtual bool get_quant_estimation () { - return false; - } - - // virtual function to be overriden for Exact and Sketch Aggregators - virtual T get_quantiles(double q) { - return values_[0]; - } - - // virtual function to be overriden for Sketch Aggregator - virtual double get_error_bound() { - return 0; - } - - // virtual function to be overriden for Sketch Aggregator - virtual size_t get_max_buckets() { - return 0; - } - - // virtual function to be overriden for Gauge Aggregator - virtual core::SystemTimestamp get_checkpoint_timestamp() { - return core::SystemTimestamp(); - } - - // Custom copy constructor to handle the mutex - Aggregator(const Aggregator &cp) - { - values_ = cp.values_; - checkpoint_ = cp.checkpoint_; - kind_ = cp.kind_; - agg_kind_ = cp.agg_kind_; - // use default initialized mutex as they cannot be copied - } - + Aggregator() = default; + + virtual ~Aggregator() = default; + + /** + * Receives a captured value from the instrument and applies it to the current aggregator value. + * + * @param val, the raw value used in aggregation + * @return none + */ + virtual void update(T val) = 0; + + /** + * Checkpoints the current value. This function will overwrite the current checkpoint with the + * current value. + * + * @param none + * @return none + */ + virtual void checkpoint() = 0; + + /** + * Merges the values of two aggregators in a semantically accurate manner. + * Merging will occur differently for different aggregators depending on the + * way values are tracked. + * + * @param other, the aggregator with merge with + * @return none + */ + void merge(Aggregator *other); + + /** + * Returns the checkpointed value + * + * @param none + * @return the value of the checkpoint + */ + virtual std::vector get_checkpoint() = 0; + + /** + * Returns the current value + * + * @param none + * @return the present aggregator value + */ + virtual std::vector get_values() = 0; + + /** + * Returns the instrument kind which this aggregator is associated with + * + * @param none + * @return the InstrumentKind of the aggregator's owner + */ + virtual opentelemetry::metrics::InstrumentKind get_instrument_kind() final { return kind_; } + + /** + * Returns the type of this aggregator + * + * @param none + * @return the AggregatorKind of this instrument + */ + virtual AggregatorKind get_aggregator_kind() final { return agg_kind_; } + + // virtual function to be overriden for the Histogram Aggregator + virtual std::vector get_boundaries() { return std::vector(); } + + // virtual function to be overriden for the Histogram Aggregator + virtual std::vector get_counts() { return std::vector(); } + + // virtual function to be overriden for Exact and Sketch Aggregators + virtual bool get_quant_estimation() { return false; } + + // virtual function to be overriden for Exact and Sketch Aggregators + virtual T get_quantiles(double q) { return values_[0]; } + + // virtual function to be overriden for Sketch Aggregator + virtual double get_error_bound() { return 0; } + + // virtual function to be overriden for Sketch Aggregator + virtual size_t get_max_buckets() { return 0; } + + // virtual function to be overriden for Gauge Aggregator + virtual core::SystemTimestamp get_checkpoint_timestamp() { return core::SystemTimestamp(); } + + // Custom copy constructor to handle the mutex + Aggregator(const Aggregator &cp) + { + values_ = cp.values_; + checkpoint_ = cp.checkpoint_; + kind_ = cp.kind_; + agg_kind_ = cp.agg_kind_; + // use default initialized mutex as they cannot be copied + } + protected: - std::vector values_; - std::vector checkpoint_; - opentelemetry::metrics::InstrumentKind kind_; - std::mutex mu_; - AggregatorKind agg_kind_; - + std::vector values_; + std::vector checkpoint_; + opentelemetry::metrics::InstrumentKind kind_; + std::mutex mu_; + AggregatorKind agg_kind_; }; } // namespace metrics diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h index 84da44b999..0bf44f6421 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h @@ -1,11 +1,10 @@ #pragma once #include -#include "opentelemetry/version.h" +#include #include "opentelemetry/metrics/instrument.h" #include "opentelemetry/sdk/metrics/aggregator/aggregator.h" -#include - +#include "opentelemetry/version.h" namespace metrics_api = opentelemetry::metrics; @@ -15,93 +14,88 @@ namespace sdk namespace metrics { -template +template class CounterAggregator final : public Aggregator { - + public: - CounterAggregator(metrics_api::InstrumentKind kind) - { - this->kind_ = kind; - this->values_ = std::vector(1, 0); - this->checkpoint_ = std::vector(1, 0); - this->agg_kind_ = AggregatorKind::Counter; - } - - /** - * Recieves a captured value from the instrument and applies it to the current aggregator value. - * - * @param val, the raw value used in aggregation - * @return none - */ - void update(T val) override - { - this->mu_.lock(); - this->values_[0] += val; // atomic operation - this->mu_.unlock(); - } - - /** - * Checkpoints the current value. This function will overwrite the current checkpoint with the - * current value. - * - * @param none - * @return none - */ - void checkpoint() override + CounterAggregator(metrics_api::InstrumentKind kind) + { + this->kind_ = kind; + this->values_ = std::vector(1, 0); + this->checkpoint_ = std::vector(1, 0); + this->agg_kind_ = AggregatorKind::Counter; + } + + /** + * Recieves a captured value from the instrument and applies it to the current aggregator value. + * + * @param val, the raw value used in aggregation + * @return none + */ + void update(T val) override + { + this->mu_.lock(); + this->values_[0] += val; // atomic operation + this->mu_.unlock(); + } + + /** + * Checkpoints the current value. This function will overwrite the current checkpoint with the + * current value. + * + * @param none + * @return none + */ + void checkpoint() override + { + this->checkpoint_ = this->values_; + this->values_[0] = 0; + } + + /** + * Merges the values of two aggregators in a semantically accurate manner. + * In this case, merging only requires the the current values of the two aggregators be summed. + * + * @param other, the aggregator with merge with + * @return none + */ + void merge(CounterAggregator other) + { + if (this->agg_kind_ == other.agg_kind_) { - this->checkpoint_ = this->values_; - this->values_[0] = 0; + this->mu_.lock(); + this->values_[0] += other.values_[0]; + this->checkpoint_[0] += other.checkpoint_[0]; + this->mu_.unlock(); } - - /** - * Merges the values of two aggregators in a semantically accurate manner. - * In this case, merging only requires the the current values of the two aggregators be summed. - * - * @param other, the aggregator with merge with - * @return none - */ - void merge(CounterAggregator other) + else { - if (this->agg_kind_ == other.agg_kind_) { - this->mu_.lock(); - this->values_[0] += other.values_[0]; - this->checkpoint_[0] += other.checkpoint_[0]; - this->mu_.unlock(); - } - else { #if __EXCEPTIONS - throw std::invalid_argument("Aggregators of different types cannot be merged."); + throw std::invalid_argument("Aggregators of different types cannot be merged."); #else - std::terminate(); + std::terminate(); #endif - } - } - - /** - * Returns the checkpointed value - * - * @param none - * @return the value of the checkpoint - */ - virtual std::vector get_checkpoint() override - { - return this->checkpoint_; } - - /** - * Returns the current values - * - * @param none - * @return the present aggregator values - */ - virtual std::vector get_values() override - { - return this->values_; - } - + } + + /** + * Returns the checkpointed value + * + * @param none + * @return the value of the checkpoint + */ + virtual std::vector get_checkpoint() override { return this->checkpoint_; } + + /** + * Returns the current values + * + * @param none + * @return the present aggregator values + */ + virtual std::vector get_values() override { return this->values_; } }; -} -} +} // namespace metrics +} // namespace sdk OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h index 0d3df40890..78b024eaec 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h @@ -2,11 +2,11 @@ #include #include -#include "opentelemetry/metrics/instrument.h" -#include "opentelemetry/version.h" -#include "opentelemetry/sdk/metrics/aggregator/aggregator.h" #include #include +#include "opentelemetry/metrics/instrument.h" +#include "opentelemetry/sdk/metrics/aggregator/aggregator.h" +#include "opentelemetry/version.h" namespace metrics_api = opentelemetry::metrics; @@ -16,191 +16,182 @@ namespace sdk namespace metrics { -template +template class HistogramAggregator final : public Aggregator { - + public: - - /** - * Constructor for the histogram aggregator. A sorted vector of boundaries is expected and boundaries are - * doubles regardless of the aggregator's templated data type. - * - * Sum is stored in values_[0] - * Count is stored in position_[1] - */ - HistogramAggregator(metrics_api::InstrumentKind kind, std::vector boundaries) + /** + * Constructor for the histogram aggregator. A sorted vector of boundaries is expected and + * boundaries are doubles regardless of the aggregator's templated data type. + * + * Sum is stored in values_[0] + * Count is stored in position_[1] + */ + HistogramAggregator(metrics_api::InstrumentKind kind, std::vector boundaries) + { + if (!std::is_sorted(boundaries.begin(), boundaries.end())) { - if (!std::is_sorted(boundaries.begin(), boundaries.end())){ #if __EXCEPTIONS - throw std::invalid_argument("Histogram boundaries must be monotonic."); + throw std::invalid_argument("Histogram boundaries must be monotonic."); #else - std::terminate(); + std::terminate(); #endif - } - this->kind_ = kind; - this->agg_kind_ = AggregatorKind::Histogram; - boundaries_ = boundaries; - this->values_ = std::vector(2, 0); - this->checkpoint_ = std::vector(2, 0); - bucketCounts_ = std::vector(boundaries_.size() + 1, 0); - bucketCounts_ckpt_ = std::vector(boundaries_.size() + 1, 0); - } - - /** - * Recieves a captured value from the instrument and inserts it into the current histogram counts. - * - * Depending on the use case, a linear search or binary search based implementation may be preferred. - * In uniformly distributed datasets, linear search outperforms binary search until 512 buckets. However, - * if the distribution is strongly skewed right (for example server latency where most values may be <10ms - * but the range is from 0 - 1000 ms), a linear search could be superior even with more than 500 buckets as - * almost all values inserted would be at the beginning of the boundaries array and thus found more quickly - * through linear search. - * - * @param val, the raw value used in aggregation - * @return none - */ - void update(T val) override - { - int bucketID = boundaries_.size(); - for (size_t i = 0; i < boundaries_.size(); i++) - { - if (val < boundaries_[i]) // concurrent read is thread-safe - { - bucketID = i; - break; - } - } - - // Alternate implementation with binary search - // auto pos = std::lower_bound (boundaries_.begin(), boundaries_.end(), val); - // bucketCounts_[pos-boundaries_.begin()] += 1; - - this->mu_.lock(); - this->values_[0] += val; - this->values_[1] += 1; - bucketCounts_[bucketID] += 1; - this->mu_.unlock(); } - - /** - * Checkpoints the current value. This function will overwrite the current checkpoint with the - * current value. - * - * @param none - * @return none - */ - void checkpoint() override + this->kind_ = kind; + this->agg_kind_ = AggregatorKind::Histogram; + boundaries_ = boundaries; + this->values_ = std::vector(2, 0); + this->checkpoint_ = std::vector(2, 0); + bucketCounts_ = std::vector(boundaries_.size() + 1, 0); + bucketCounts_ckpt_ = std::vector(boundaries_.size() + 1, 0); + } + + /** + * Recieves a captured value from the instrument and inserts it into the current histogram counts. + * + * Depending on the use case, a linear search or binary search based implementation may be + * preferred. In uniformly distributed datasets, linear search outperforms binary search until 512 + * buckets. However, if the distribution is strongly skewed right (for example server latency + * where most values may be <10ms but the range is from 0 - 1000 ms), a linear search could be + * superior even with more than 500 buckets as almost all values inserted would be at the + * beginning of the boundaries array and thus found more quickly through linear search. + * + * @param val, the raw value used in aggregation + * @return none + */ + void update(T val) override + { + int bucketID = boundaries_.size(); + for (size_t i = 0; i < boundaries_.size(); i++) { - this->checkpoint_ = this->values_; - this->values_[0] = 0; - this->values_[1] = 0; - bucketCounts_ckpt_ = bucketCounts_; - std::fill(bucketCounts_.begin(), bucketCounts_.end(), 0); + if (val < boundaries_[i]) // concurrent read is thread-safe + { + bucketID = i; + break; + } } - - /** - * Merges the values of two aggregators in a semantically accurate manner. - * A histogram aggregator can only be merged with another histogram aggregator that has the same boudnaries. - * A histogram merge first adds the sum and count values then iterates over the adds the bucket counts - * element by element. - * - * @param other, the aggregator with merge with - * @return none - */ - void merge(HistogramAggregator other) + + // Alternate implementation with binary search + // auto pos = std::lower_bound (boundaries_.begin(), boundaries_.end(), val); + // bucketCounts_[pos-boundaries_.begin()] += 1; + + this->mu_.lock(); + this->values_[0] += val; + this->values_[1] += 1; + bucketCounts_[bucketID] += 1; + this->mu_.unlock(); + } + + /** + * Checkpoints the current value. This function will overwrite the current checkpoint with the + * current value. + * + * @param none + * @return none + */ + void checkpoint() override + { + this->checkpoint_ = this->values_; + this->values_[0] = 0; + this->values_[1] = 0; + bucketCounts_ckpt_ = bucketCounts_; + std::fill(bucketCounts_.begin(), bucketCounts_.end(), 0); + } + + /** + * Merges the values of two aggregators in a semantically accurate manner. + * A histogram aggregator can only be merged with another histogram aggregator that has the same + * boudnaries. A histogram merge first adds the sum and count values then iterates over the adds + * the bucket counts element by element. + * + * @param other, the aggregator with merge with + * @return none + */ + void merge(HistogramAggregator other) + { + this->mu_.lock(); + + // Ensure that incorrect types are not merged + if (this->agg_kind_ != other.agg_kind_) { - this->mu_.lock(); - - // Ensure that incorrect types are not merged - if (this->agg_kind_ != other.agg_kind_){ #if __EXCEPTIONS - throw std::invalid_argument("Aggregators of different types cannot be merged."); + throw std::invalid_argument("Aggregators of different types cannot be merged."); #else - std::terminate(); + std::terminate(); #endif - // Reject histogram merges with differing boundary vectors - } else if (other.boundaries_ != this->boundaries_){ + // Reject histogram merges with differing boundary vectors + } + else if (other.boundaries_ != this->boundaries_) + { #if __EXCEPTIONS - throw std::invalid_argument("Histogram boundaries do not match."); + throw std::invalid_argument("Histogram boundaries do not match."); #else - std::terminate(); + std::terminate(); #endif - } - - this->values_[0] += other.values_[0]; - this->values_[1] += other.values_[1]; - - for (size_t i = 0; i < bucketCounts_.size(); i++) - { - bucketCounts_[i] += other.bucketCounts_[i]; - bucketCounts_ckpt_[i] += other.bucketCounts_ckpt_[i]; - } - this->mu_.unlock(); } - - /** - * Returns the checkpointed value - * - * @param none - * @return the value of the checkpoint - */ - std::vector get_checkpoint() override - { - return this->checkpoint_; - } - - /** - * Returns the current values - * - * @param none - * @return the present aggregator values - */ - std::vector get_values() override - { - return this->values_; - } - - /** - * Returns the bucket boundaries specified at this aggregator's creation. - * - * @param none - * @return the aggregator boundaries - */ - virtual std::vector get_boundaries() override - { - return boundaries_; - } - - /** - * Returns the current counts for each bucket . - * - * @param none - * @return the aggregator bucket counts - */ - virtual std::vector get_counts() override - { - return bucketCounts_ckpt_; - } - - HistogramAggregator(const HistogramAggregator &cp) + + this->values_[0] += other.values_[0]; + this->values_[1] += other.values_[1]; + + for (size_t i = 0; i < bucketCounts_.size(); i++) { - this->values_ = cp.values_; - this->checkpoint_ = cp.checkpoint_; - this->kind_ = cp.kind_; - this->agg_kind_ = cp.agg_kind_; - boundaries_ = cp.boundaries_; - bucketCounts_ = cp.bucketCounts_; - bucketCounts_ckpt_ = cp.bucketCounts_ckpt_; - // use default initialized mutex as they cannot be copied + bucketCounts_[i] += other.bucketCounts_[i]; + bucketCounts_ckpt_[i] += other.bucketCounts_ckpt_[i]; } - + this->mu_.unlock(); + } + + /** + * Returns the checkpointed value + * + * @param none + * @return the value of the checkpoint + */ + std::vector get_checkpoint() override { return this->checkpoint_; } + + /** + * Returns the current values + * + * @param none + * @return the present aggregator values + */ + std::vector get_values() override { return this->values_; } + + /** + * Returns the bucket boundaries specified at this aggregator's creation. + * + * @param none + * @return the aggregator boundaries + */ + virtual std::vector get_boundaries() override { return boundaries_; } + + /** + * Returns the current counts for each bucket . + * + * @param none + * @return the aggregator bucket counts + */ + virtual std::vector get_counts() override { return bucketCounts_ckpt_; } + + HistogramAggregator(const HistogramAggregator &cp) + { + this->values_ = cp.values_; + this->checkpoint_ = cp.checkpoint_; + this->kind_ = cp.kind_; + this->agg_kind_ = cp.agg_kind_; + boundaries_ = cp.boundaries_; + bucketCounts_ = cp.bucketCounts_; + bucketCounts_ckpt_ = cp.bucketCounts_ckpt_; + // use default initialized mutex as they cannot be copied + } + private: - std::vector boundaries_; - std::vector bucketCounts_; - std::vector bucketCounts_ckpt_; + std::vector boundaries_; + std::vector bucketCounts_; + std::vector bucketCounts_ckpt_; }; -} -} +} // namespace metrics +} // namespace sdk OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/test/metrics/BUILD b/sdk/test/metrics/BUILD index cd82d5c4cb..4ce50b36e2 100644 --- a/sdk/test/metrics/BUILD +++ b/sdk/test/metrics/BUILD @@ -2,7 +2,7 @@ cc_test( name = "meter_provider_sdk_test", srcs = [ "meter_provider_sdk_test.cc", -], + ], deps = [ "//sdk/src/metrics", "@com_google_googletest//:gtest_main", @@ -30,4 +30,3 @@ cc_test( "@com_google_googletest//:gtest_main", ], ) - diff --git a/sdk/test/metrics/counter_aggregator_test.cc b/sdk/test/metrics/counter_aggregator_test.cc index 8208983b89..1e2b1dce8c 100644 --- a/sdk/test/metrics/counter_aggregator_test.cc +++ b/sdk/test/metrics/counter_aggregator_test.cc @@ -1,8 +1,8 @@ #include "opentelemetry/sdk/metrics/aggregator/counter_aggregator.h" #include -#include #include +#include namespace metrics_api = opentelemetry::metrics; @@ -14,98 +14,102 @@ namespace metrics TEST(CounterAggregator, NoUpdates) { - CounterAggregator alpha(metrics_api::InstrumentKind::Counter); - - EXPECT_EQ(alpha.get_checkpoint().size(), 1); - EXPECT_EQ(alpha.get_checkpoint()[0], 0); - - alpha.checkpoint(); - EXPECT_EQ(alpha.get_checkpoint().size(), 1); - EXPECT_EQ(alpha.get_checkpoint()[0], 0); + CounterAggregator alpha(metrics_api::InstrumentKind::Counter); + + EXPECT_EQ(alpha.get_checkpoint().size(), 1); + EXPECT_EQ(alpha.get_checkpoint()[0], 0); + + alpha.checkpoint(); + EXPECT_EQ(alpha.get_checkpoint().size(), 1); + EXPECT_EQ(alpha.get_checkpoint()[0], 0); } TEST(CounterAggregator, Update) { - CounterAggregator alpha(metrics_api::InstrumentKind::Counter); - CounterAggregator beta(metrics_api::InstrumentKind::Counter); - - for (int i = 0; i<123456; i++){ - alpha.update(1); - } - - int sum = 0; - for (int i = 0; i < 100; i++){ - int tmp = std::rand(); - beta.update(tmp); - sum+=tmp; - } - - EXPECT_EQ(alpha.get_checkpoint()[0], 0); // checkpoint shouldn't change even with updates - EXPECT_EQ(beta.get_checkpoint()[0], 0); - - alpha.checkpoint(); - beta.checkpoint(); - - EXPECT_EQ(alpha.get_checkpoint()[0], 123456); - EXPECT_EQ(beta.get_checkpoint()[0], sum); - - alpha.update(15); - alpha.checkpoint(); - EXPECT_EQ(alpha.get_checkpoint()[0], 15); //reset to 0 after first checkpoint call + CounterAggregator alpha(metrics_api::InstrumentKind::Counter); + CounterAggregator beta(metrics_api::InstrumentKind::Counter); + + for (int i = 0; i < 123456; i++) + { + alpha.update(1); + } + + int sum = 0; + for (int i = 0; i < 100; i++) + { + int tmp = std::rand(); + beta.update(tmp); + sum += tmp; + } + + EXPECT_EQ(alpha.get_checkpoint()[0], 0); // checkpoint shouldn't change even with updates + EXPECT_EQ(beta.get_checkpoint()[0], 0); + + alpha.checkpoint(); + beta.checkpoint(); + + EXPECT_EQ(alpha.get_checkpoint()[0], 123456); + EXPECT_EQ(beta.get_checkpoint()[0], sum); + + alpha.update(15); + alpha.checkpoint(); + EXPECT_EQ(alpha.get_checkpoint()[0], 15); // reset to 0 after first checkpoint call } // callback update function used to test concurrent calls -void incrementingCallback(Aggregator & agg) +void incrementingCallback(Aggregator &agg) { - for (int i = 0; i< 2000000; i++){ - agg.update(1); - } + for (int i = 0; i < 2000000; i++) + { + agg.update(1); + } } -TEST(CounterAggregator, Concurrency){ - CounterAggregator alpha(metrics_api::InstrumentKind::Counter); - - // spawn new threads that initiate the callback - std::thread first (incrementingCallback, std::ref(alpha)); - std::thread second (incrementingCallback, std::ref(alpha)); - - - first.join(); - second.join(); - - alpha.checkpoint(); - - // race conditions result in values below 2*2000000 - EXPECT_EQ(alpha.get_checkpoint()[0], 2*2000000); +TEST(CounterAggregator, Concurrency) +{ + CounterAggregator alpha(metrics_api::InstrumentKind::Counter); + + // spawn new threads that initiate the callback + std::thread first(incrementingCallback, std::ref(alpha)); + std::thread second(incrementingCallback, std::ref(alpha)); + + first.join(); + second.join(); + + alpha.checkpoint(); + + // race conditions result in values below 2*2000000 + EXPECT_EQ(alpha.get_checkpoint()[0], 2 * 2000000); } TEST(CounterAggregator, Merge) { - CounterAggregator alpha(metrics_api::InstrumentKind::Counter); - CounterAggregator beta(metrics_api::InstrumentKind::Counter); - - alpha.merge(beta); - - alpha.checkpoint(); - EXPECT_EQ(alpha.get_checkpoint()[0], 0); //merge with no updates - - for (int i = 0; i<500; i++){ - alpha.update(1); - } - - for (int i = 0; i<700; i++){ - beta.update(1); - } - - alpha.merge(beta); - alpha.checkpoint(); - EXPECT_EQ(alpha.get_checkpoint()[0], 1200); - - //HistogramAggregator gamma(metrics_api::BoundInstrumentKind::BoundValueRecorder); - //ASSERT_THROW(alpha.merge(gamma), AggregatorMismatch); -} + CounterAggregator alpha(metrics_api::InstrumentKind::Counter); + CounterAggregator beta(metrics_api::InstrumentKind::Counter); + alpha.merge(beta); + + alpha.checkpoint(); + EXPECT_EQ(alpha.get_checkpoint()[0], 0); // merge with no updates + + for (int i = 0; i < 500; i++) + { + alpha.update(1); + } + + for (int i = 0; i < 700; i++) + { + beta.update(1); + } + + alpha.merge(beta); + alpha.checkpoint(); + EXPECT_EQ(alpha.get_checkpoint()[0], 1200); + + // HistogramAggregator gamma(metrics_api::BoundInstrumentKind::BoundValueRecorder); + // ASSERT_THROW(alpha.merge(gamma), AggregatorMismatch); +} -} // namespace sdk } // namespace metrics +} // namespace sdk OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/test/metrics/histogram_aggregator_test.cc b/sdk/test/metrics/histogram_aggregator_test.cc index fceb7fe13a..146c552bb0 100644 --- a/sdk/test/metrics/histogram_aggregator_test.cc +++ b/sdk/test/metrics/histogram_aggregator_test.cc @@ -1,9 +1,9 @@ #include "opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h" #include -#include -#include #include +#include +#include // #include namespace metrics_api = opentelemetry::metrics; @@ -17,138 +17,151 @@ namespace metrics // Test updating with a uniform set of updates TEST(Histogram, Uniform) { - std::vector boundaries{10,20,30,40,50}; - HistogramAggregator alpha(metrics_api::InstrumentKind::Counter, boundaries); - - EXPECT_EQ(alpha.get_aggregator_kind(), AggregatorKind::Histogram); - - alpha.checkpoint(); - EXPECT_EQ(alpha.get_checkpoint().size(),2); - EXPECT_EQ(alpha.get_counts().size(),6); - - for (int i = 0; i< 60; i++){ - alpha.update(i); - } - - alpha.checkpoint(); - - EXPECT_EQ(alpha.get_checkpoint()[0], 1770); - EXPECT_EQ(alpha.get_checkpoint()[1], 60); - - std::vector correct = {10,10,10,10,10,10}; - EXPECT_EQ(alpha.get_counts(), correct); + std::vector boundaries{10, 20, 30, 40, 50}; + HistogramAggregator alpha(metrics_api::InstrumentKind::Counter, boundaries); + + EXPECT_EQ(alpha.get_aggregator_kind(), AggregatorKind::Histogram); + + alpha.checkpoint(); + EXPECT_EQ(alpha.get_checkpoint().size(), 2); + EXPECT_EQ(alpha.get_counts().size(), 6); + + for (int i = 0; i < 60; i++) + { + alpha.update(i); + } + + alpha.checkpoint(); + + EXPECT_EQ(alpha.get_checkpoint()[0], 1770); + EXPECT_EQ(alpha.get_checkpoint()[1], 60); + + std::vector correct = {10, 10, 10, 10, 10, 10}; + EXPECT_EQ(alpha.get_counts(), correct); } // Test updating with a normal distribution TEST(Histogram, Normal) { - std::vector boundaries{2,4,6,8,10,12}; - HistogramAggregator alpha(metrics_api::InstrumentKind::Counter, boundaries); - - std::vector vals{1,3,3,5,5,5,7,7,7,7,9,9,9,11,11,13}; - for (int i : vals){ - alpha.update(i); - } - - alpha.checkpoint(); - - EXPECT_EQ(alpha.get_checkpoint()[0], std::accumulate(vals.begin(),vals.end(),0)); - EXPECT_EQ(alpha.get_checkpoint()[1], vals.size()); - - std::vector correct = {1,2,3,4,3,2,1}; - EXPECT_EQ(alpha.get_counts(), correct); + std::vector boundaries{2, 4, 6, 8, 10, 12}; + HistogramAggregator alpha(metrics_api::InstrumentKind::Counter, boundaries); + + std::vector vals{1, 3, 3, 5, 5, 5, 7, 7, 7, 7, 9, 9, 9, 11, 11, 13}; + for (int i : vals) + { + alpha.update(i); + } + + alpha.checkpoint(); + + EXPECT_EQ(alpha.get_checkpoint()[0], std::accumulate(vals.begin(), vals.end(), 0)); + EXPECT_EQ(alpha.get_checkpoint()[1], vals.size()); + + std::vector correct = {1, 2, 3, 4, 3, 2, 1}; + EXPECT_EQ(alpha.get_counts(), correct); } -TEST(Histogram, Merge){ - std::vector boundaries{2,4,6,8,10,12}; - HistogramAggregator alpha(metrics_api::InstrumentKind::Counter, boundaries); - HistogramAggregator beta(metrics_api::InstrumentKind::Counter, boundaries); - - std::vector vals{1,3,3,5,5,5,7,7,7,7,9,9,9,11,11,13}; - for (int i : vals){ - alpha.update(i); - } - - std::vector otherVals{1,1,1,1,11,11,13,13,13,15}; - for (int i : otherVals){ - beta.update(i); - } - - alpha.merge(beta); - alpha.checkpoint(); - - EXPECT_EQ(alpha.get_checkpoint()[0], std::accumulate(vals.begin(),vals.end(),0)+std::accumulate(otherVals.begin(), otherVals.end(), 0)); - EXPECT_EQ(alpha.get_checkpoint()[1], vals.size()+otherVals.size()); - - std::vector correct = {5,2,3,4,3,4,5}; - EXPECT_EQ(alpha.get_counts(), correct); +TEST(Histogram, Merge) +{ + std::vector boundaries{2, 4, 6, 8, 10, 12}; + HistogramAggregator alpha(metrics_api::InstrumentKind::Counter, boundaries); + HistogramAggregator beta(metrics_api::InstrumentKind::Counter, boundaries); + + std::vector vals{1, 3, 3, 5, 5, 5, 7, 7, 7, 7, 9, 9, 9, 11, 11, 13}; + for (int i : vals) + { + alpha.update(i); + } + + std::vector otherVals{1, 1, 1, 1, 11, 11, 13, 13, 13, 15}; + for (int i : otherVals) + { + beta.update(i); + } + + alpha.merge(beta); + alpha.checkpoint(); + + EXPECT_EQ(alpha.get_checkpoint()[0], std::accumulate(vals.begin(), vals.end(), 0) + + std::accumulate(otherVals.begin(), otherVals.end(), 0)); + EXPECT_EQ(alpha.get_checkpoint()[1], vals.size() + otherVals.size()); + + std::vector correct = {5, 2, 3, 4, 3, 4, 5}; + EXPECT_EQ(alpha.get_counts(), correct); } // Update callback used to validate multi-threaded performance -void histogramUpdateCallback(Aggregator & agg, std::vector vals){ - for (int i: vals){ - agg.update(i); - } +void histogramUpdateCallback(Aggregator &agg, std::vector vals) +{ + for (int i : vals) + { + agg.update(i); + } } -int randVal(){ - return rand() % 15; +int randVal() +{ + return rand() % 15; } -TEST(Histogram, Concurrency){ - std::vector boundaries{2,4,6,8,10,12}; - HistogramAggregator alpha(metrics_api::InstrumentKind::Counter, boundaries); - - std::vector vals1(1000); - std::generate(vals1.begin(),vals1.end(),randVal); - - std::vector vals2(1000); - std::generate(vals2.begin(),vals2.end(),randVal); - - std::thread first (histogramUpdateCallback, std::ref(alpha), vals1); - std::thread second (histogramUpdateCallback, std::ref(alpha), vals2); - - first.join(); - second.join(); - - HistogramAggregator beta(metrics_api::InstrumentKind::Counter, boundaries); - - - // Timing harness to compare linear and binary insertion - // auto start = std::chrono::system_clock::now(); - for (int i: vals1){ - beta.update(i); - } - for (int i: vals2){ - beta.update(i); - } - //auto end = std::chrono::system_clock::now(); - //auto elapsed = std::chrono::duration_cast(end - start); - //std::cout <<"Update time: " < boundaries{2, 4, 6, 8, 10, 12}; + HistogramAggregator alpha(metrics_api::InstrumentKind::Counter, boundaries); + + std::vector vals1(1000); + std::generate(vals1.begin(), vals1.end(), randVal); + + std::vector vals2(1000); + std::generate(vals2.begin(), vals2.end(), randVal); + + std::thread first(histogramUpdateCallback, std::ref(alpha), vals1); + std::thread second(histogramUpdateCallback, std::ref(alpha), vals2); + + first.join(); + second.join(); + + HistogramAggregator beta(metrics_api::InstrumentKind::Counter, boundaries); + + // Timing harness to compare linear and binary insertion + // auto start = std::chrono::system_clock::now(); + for (int i : vals1) + { + beta.update(i); + } + for (int i : vals2) + { + beta.update(i); + } + // auto end = std::chrono::system_clock::now(); + // auto elapsed = std::chrono::duration_cast(end - start); + // std::cout <<"Update time: " < boundaries{2,4,6,8,10,12}; - std::vector boundaries2{1,4,6,8,10,12}; - std::vector unsortedBoundaries{10,12,4,6,8}; - EXPECT_ANY_THROW(HistogramAggregator alpha(metrics_api::InstrumentKind::Counter, unsortedBoundaries)); - - HistogramAggregator beta(metrics_api::InstrumentKind::Counter, boundaries); - HistogramAggregator gamma(metrics_api::InstrumentKind::Counter, boundaries2); - - EXPECT_ANY_THROW(beta.merge(gamma)); +TEST(Histogram, Errors) +{ + std::vector boundaries{2, 4, 6, 8, 10, 12}; + std::vector boundaries2{1, 4, 6, 8, 10, 12}; + std::vector unsortedBoundaries{10, 12, 4, 6, 8}; + EXPECT_ANY_THROW( + HistogramAggregator alpha(metrics_api::InstrumentKind::Counter, unsortedBoundaries)); + + HistogramAggregator beta(metrics_api::InstrumentKind::Counter, boundaries); + HistogramAggregator gamma(metrics_api::InstrumentKind::Counter, boundaries2); + + EXPECT_ANY_THROW(beta.merge(gamma)); } #endif -} // namespace sdk } // namespace metrics +} // namespace sdk OPENTELEMETRY_END_NAMESPACE