From 2b2900fd8c554c4e798f9ce307d47ff7f24499bc Mon Sep 17 00:00:00 2001 From: Ehsan Saei <71217171+esigo@users.noreply.github.com> Date: Sat, 6 Aug 2022 05:18:31 +0200 Subject: [PATCH] [Metrics SDK] Histogram min/max support (#1540) --- bazel/repository.bzl | 6 +-- exporters/ostream/src/metric_exporter.cc | 20 +++++++++ exporters/ostream/test/ostream_metric_test.cc | 6 +++ exporters/otlp/src/otlp_metric_utils.cc | 19 ++++++++ .../test/otlp_http_metric_exporter_test.cc | 4 ++ .../metrics/aggregation/aggregation_config.h | 1 + .../aggregation/histogram_aggregation.h | 19 +++++--- .../sdk/metrics/data/point_data.h | 3 ++ .../aggregation/histogram_aggregation.cc | 43 +++++++++++++++---- sdk/test/metrics/aggregation_test.cc | 12 ++++++ third_party/opentelemetry-proto | 2 +- third_party_release | 2 +- 12 files changed, 119 insertions(+), 18 deletions(-) diff --git a/bazel/repository.bzl b/bazel/repository.bzl index 30a34fb2fd..e7d52b8b4e 100644 --- a/bazel/repository.bzl +++ b/bazel/repository.bzl @@ -87,10 +87,10 @@ def opentelemetry_cpp_deps(): http_archive, name = "com_github_opentelemetry_proto", build_file = "@io_opentelemetry_cpp//bazel:opentelemetry_proto.BUILD", - sha256 = "f269fbcb30e17b03caa1decd231ce826e59d7651c0f71c3b28eb5140b4bb5412", - strip_prefix = "opentelemetry-proto-0.17.0", + sha256 = "134ce87f0a623daac19b9507b92da0d9b82929e3db796bba631e422f6ea8d3b3", + strip_prefix = "opentelemetry-proto-0.18.0", urls = [ - "https://github.com/open-telemetry/opentelemetry-proto/archive/v0.17.0.tar.gz", + "https://github.com/open-telemetry/opentelemetry-proto/archive/v0.18.0.tar.gz", ], ) diff --git a/exporters/ostream/src/metric_exporter.cc b/exporters/ostream/src/metric_exporter.cc index e16fb76de3..1c8307f111 100644 --- a/exporters/ostream/src/metric_exporter.cc +++ b/exporters/ostream/src/metric_exporter.cc @@ -174,6 +174,26 @@ void OStreamMetricExporter::printPointData(const opentelemetry::sdk::metrics::Po sout_ << nostd::get(histogram_point_data.sum_); } + if (histogram_point_data.record_min_max_) + { + if (nostd::holds_alternative(histogram_point_data.min_)) + { + sout_ << "\n min : " << nostd::get(histogram_point_data.min_); + } + else if (nostd::holds_alternative(histogram_point_data.min_)) + { + sout_ << "\n min : " << nostd::get(histogram_point_data.min_); + } + if (nostd::holds_alternative(histogram_point_data.max_)) + { + sout_ << "\n max : " << nostd::get(histogram_point_data.max_); + } + if (nostd::holds_alternative(histogram_point_data.max_)) + { + sout_ << "\n max : " << nostd::get(histogram_point_data.max_); + } + } + sout_ << "\n buckets : "; if (nostd::holds_alternative>(histogram_point_data.boundaries_)) { diff --git a/exporters/ostream/test/ostream_metric_test.cc b/exporters/ostream/test/ostream_metric_test.cc index c6363143fe..0dd95bc37f 100644 --- a/exporters/ostream/test/ostream_metric_test.cc +++ b/exporters/ostream/test/ostream_metric_test.cc @@ -102,6 +102,8 @@ TEST(OStreamMetricsExporter, ExportHistogramPointData) histogram_point_data.count_ = 3; histogram_point_data.counts_ = {200, 300, 400, 500}; histogram_point_data.sum_ = 900.5; + histogram_point_data.min_ = 1.8; + histogram_point_data.max_ = 12.0; metric_sdk::HistogramPointData histogram_point_data2{}; histogram_point_data2.boundaries_ = std::list{10, 20, 30}; histogram_point_data2.count_ = 3; @@ -146,6 +148,8 @@ TEST(OStreamMetricsExporter, ExportHistogramPointData) "\n type : HistogramPointData" "\n count : 3" "\n sum : 900.5" + "\n min : 1.8" + "\n max : 12" "\n buckets : [10.1, 20.2, 30.2, ]" "\n counts : [200, 300, 400, 500, ]" "\n attributes\t\t: " @@ -154,6 +158,8 @@ TEST(OStreamMetricsExporter, ExportHistogramPointData) "\n type : HistogramPointData" "\n count : 3" "\n sum : 900" + "\n min : 0" + "\n max : 0" "\n buckets : [10, 20, 30, ]" "\n counts : [200, 300, 400, 500, ]" "\n attributes\t\t: " diff --git a/exporters/otlp/src/otlp_metric_utils.cc b/exporters/otlp/src/otlp_metric_utils.cc index 03029dad56..884a1e2c8f 100644 --- a/exporters/otlp/src/otlp_metric_utils.cc +++ b/exporters/otlp/src/otlp_metric_utils.cc @@ -106,6 +106,25 @@ void OtlpMetricUtils::ConvertHistogramMetric( } // count proto_histogram_point_data->set_count(histogram_data.count_); + if (histogram_data.record_min_max_) + { + if (nostd::holds_alternative(histogram_data.min_)) + { + proto_histogram_point_data->set_min(nostd::get(histogram_data.min_)); + } + else + { + proto_histogram_point_data->set_min(nostd::get(histogram_data.min_)); + } + if (nostd::holds_alternative(histogram_data.max_)) + { + proto_histogram_point_data->set_min(nostd::get(histogram_data.max_)); + } + else + { + proto_histogram_point_data->set_max(nostd::get(histogram_data.max_)); + } + } // buckets if ((nostd::holds_alternative>(histogram_data.boundaries_))) { diff --git a/exporters/otlp/test/otlp_http_metric_exporter_test.cc b/exporters/otlp/test/otlp_http_metric_exporter_test.cc index 46a7c5eac8..891812c4d1 100644 --- a/exporters/otlp/test/otlp_http_metric_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_metric_exporter_test.cc @@ -502,6 +502,8 @@ class OtlpHttpMetricExporterTestPeer : public ::testing::Test histogram_point_data.count_ = 3; histogram_point_data.counts_ = {200, 300, 400, 500}; histogram_point_data.sum_ = 900.5; + histogram_point_data.min_ = 1.8; + histogram_point_data.max_ = 19.0; opentelemetry::sdk::metrics::HistogramPointData histogram_point_data2{}; histogram_point_data2.boundaries_ = std::list{10, 20, 30}; histogram_point_data2.count_ = 3; @@ -551,6 +553,8 @@ class OtlpHttpMetricExporterTestPeer : public ::testing::Test auto data_points = metric["histogram"]["data_points"]; EXPECT_EQ(3, JsonToInteger(data_points[0]["count"])); EXPECT_EQ(900.5, data_points[0]["sum"].get()); + EXPECT_EQ(1.8, data_points[0]["min"].get()); + EXPECT_EQ(19, data_points[0]["max"].get()); EXPECT_EQ(4, data_points[0]["bucket_counts"].size()); if (4 == data_points[0]["bucket_counts"].size()) { diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h index bd2fe55c5a..67a092ff9c 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h @@ -21,6 +21,7 @@ class HistogramAggregationConfig : public AggregationConfig { public: std::list boundaries_; + bool record_min_max_ = true; }; } // namespace metrics } // namespace sdk diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h index e648a6a4bc..b39f442849 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h @@ -42,6 +42,7 @@ class LongHistogramAggregation : public Aggregation private: opentelemetry::common::SpinLockMutex lock_; HistogramPointData point_data_; + bool record_min_max_ = true; }; class DoubleHistogramAggregation : public Aggregation @@ -72,6 +73,7 @@ class DoubleHistogramAggregation : public Aggregation private: mutable opentelemetry::common::SpinLockMutex lock_; mutable HistogramPointData point_data_; + bool record_min_max_ = true; }; template @@ -83,9 +85,15 @@ void HistogramMerge(HistogramPointData ¤t, { merge.counts_[i] = current.counts_[i] + delta.counts_[i]; } - merge.boundaries_ = current.boundaries_; - merge.sum_ = nostd::get(current.sum_) + nostd::get(delta.sum_); - merge.count_ = current.count_ + delta.count_; + merge.boundaries_ = current.boundaries_; + merge.sum_ = nostd::get(current.sum_) + nostd::get(delta.sum_); + merge.count_ = current.count_ + delta.count_; + merge.record_min_max_ = current.record_min_max_ && delta.record_min_max_; + if (merge.record_min_max_) + { + merge.min_ = std::min(nostd::get(current.min_), nostd::get(delta.min_)); + merge.max_ = std::max(nostd::get(current.max_), nostd::get(delta.max_)); + } } template @@ -95,8 +103,9 @@ void HistogramDiff(HistogramPointData ¤t, HistogramPointData &next, Histog { diff.counts_[i] = next.counts_[i] - current.counts_[i]; } - diff.boundaries_ = current.boundaries_; - diff.count_ = next.count_ - current.count_; + diff.boundaries_ = current.boundaries_; + diff.count_ = next.count_ - current.count_; + diff.record_min_max_ = false; } } // namespace metrics diff --git a/sdk/include/opentelemetry/sdk/metrics/data/point_data.h b/sdk/include/opentelemetry/sdk/metrics/data/point_data.h index 714f96c9af..ebc4bb98f2 100644 --- a/sdk/include/opentelemetry/sdk/metrics/data/point_data.h +++ b/sdk/include/opentelemetry/sdk/metrics/data/point_data.h @@ -58,8 +58,11 @@ class HistogramPointData ListType boundaries_ = {}; ValueType sum_ = {}; + ValueType min_ = {}; + ValueType max_ = {}; std::vector counts_ = {}; uint64_t count_ = {}; + bool record_min_max_ = true; }; class DropPointData diff --git a/sdk/src/metrics/aggregation/histogram_aggregation.cc b/sdk/src/metrics/aggregation/histogram_aggregation.cc index 79f0714b54..9a81c0ac82 100644 --- a/sdk/src/metrics/aggregation/histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/histogram_aggregation.cc @@ -3,6 +3,9 @@ #ifndef ENABLE_METRICS_PREVIEW # include "opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h" +# include +# include +# include # include "opentelemetry/version.h" # include @@ -23,18 +26,25 @@ LongHistogramAggregation::LongHistogramAggregation( { point_data_.boundaries_ = std::list{0l, 5l, 10l, 25l, 50l, 75l, 100l, 250l, 500l, 1000l}; } + if (aggregation_config) + { + record_min_max_ = aggregation_config->record_min_max_; + } point_data_.counts_ = std::vector(nostd::get>(point_data_.boundaries_).size() + 1, 0); - point_data_.sum_ = 0l; - point_data_.count_ = 0; + point_data_.sum_ = 0l; + point_data_.count_ = 0; + point_data_.record_min_max_ = record_min_max_; + point_data_.min_ = std::numeric_limits::max(); + point_data_.max_ = std::numeric_limits::min(); } LongHistogramAggregation::LongHistogramAggregation(HistogramPointData &&data) - : point_data_{std::move(data)} + : point_data_{std::move(data)}, record_min_max_{point_data_.record_min_max_} {} LongHistogramAggregation::LongHistogramAggregation(const HistogramPointData &data) - : point_data_{data} + : point_data_{data}, record_min_max_{point_data_.record_min_max_} {} void LongHistogramAggregation::Aggregate(long value, const PointAttributes &attributes) noexcept @@ -42,7 +52,12 @@ void LongHistogramAggregation::Aggregate(long value, const PointAttributes &attr const std::lock_guard locked(lock_); point_data_.count_ += 1; point_data_.sum_ = nostd::get(point_data_.sum_) + value; - size_t index = 0; + if (record_min_max_) + { + point_data_.min_ = std::min(nostd::get(point_data_.min_), value); + point_data_.max_ = std::max(nostd::get(point_data_.max_), value); + } + size_t index = 0; for (auto it = nostd::get>(point_data_.boundaries_).begin(); it != nostd::get>(point_data_.boundaries_).end(); ++it) { @@ -93,10 +108,17 @@ DoubleHistogramAggregation::DoubleHistogramAggregation( point_data_.boundaries_ = std::list{0.0, 5.0, 10.0, 25.0, 50.0, 75.0, 100.0, 250.0, 500.0, 1000.0}; } + if (aggregation_config) + { + record_min_max_ = aggregation_config->record_min_max_; + } point_data_.counts_ = std::vector(nostd::get>(point_data_.boundaries_).size() + 1, 0); - point_data_.sum_ = 0.0; - point_data_.count_ = 0; + point_data_.sum_ = 0.0; + point_data_.count_ = 0; + point_data_.record_min_max_ = record_min_max_; + point_data_.min_ = std::numeric_limits::max(); + point_data_.max_ = std::numeric_limits::min(); } DoubleHistogramAggregation::DoubleHistogramAggregation(HistogramPointData &&data) @@ -112,7 +134,12 @@ void DoubleHistogramAggregation::Aggregate(double value, const PointAttributes & const std::lock_guard locked(lock_); point_data_.count_ += 1; point_data_.sum_ = nostd::get(point_data_.sum_) + value; - size_t index = 0; + if (record_min_max_) + { + point_data_.min_ = std::min(nostd::get(point_data_.min_), value); + point_data_.max_ = std::max(nostd::get(point_data_.max_), value); + } + size_t index = 0; for (auto it = nostd::get>(point_data_.boundaries_).begin(); it != nostd::get>(point_data_.boundaries_).end(); ++it) { diff --git a/sdk/test/metrics/aggregation_test.cc b/sdk/test/metrics/aggregation_test.cc index 3fb9d55126..9d84a95d81 100644 --- a/sdk/test/metrics/aggregation_test.cc +++ b/sdk/test/metrics/aggregation_test.cc @@ -81,6 +81,8 @@ TEST(Aggregation, LongHistogramAggregation) EXPECT_NO_THROW(aggr.Aggregate(12l, {})); // lies in fourth bucket EXPECT_NO_THROW(aggr.Aggregate(100l, {})); // lies in eight bucket histogram_data = nostd::get(aggr.ToPoint()); + EXPECT_EQ(nostd::get(histogram_data.min_), 12); + EXPECT_EQ(nostd::get(histogram_data.max_), 100); EXPECT_EQ(nostd::get(histogram_data.sum_), 112); EXPECT_EQ(histogram_data.count_, 2); EXPECT_EQ(histogram_data.counts_[3], 1); @@ -91,6 +93,8 @@ TEST(Aggregation, LongHistogramAggregation) EXPECT_EQ(histogram_data.count_, 4); EXPECT_EQ(histogram_data.counts_[3], 2); EXPECT_EQ(histogram_data.counts_[8], 1); + EXPECT_EQ(nostd::get(histogram_data.min_), 12); + EXPECT_EQ(nostd::get(histogram_data.max_), 252); // Merge LongHistogramAggregation aggr1; @@ -113,6 +117,8 @@ TEST(Aggregation, LongHistogramAggregation) EXPECT_EQ(histogram_data.counts_[3], 2); // 11, 13 EXPECT_EQ(histogram_data.counts_[4], 2); // 25, 28 EXPECT_EQ(histogram_data.counts_[7], 1); // 105 + EXPECT_EQ(nostd::get(histogram_data.min_), 1); + EXPECT_EQ(nostd::get(histogram_data.max_), 105); // Diff auto aggr4 = aggr1.Diff(aggr2); @@ -170,6 +176,8 @@ TEST(Aggregation, DoubleHistogramAggregation) EXPECT_EQ(histogram_data.count_, 2); EXPECT_EQ(histogram_data.counts_[3], 1); EXPECT_EQ(histogram_data.counts_[7], 1); + EXPECT_EQ(nostd::get(histogram_data.min_), 12); + EXPECT_EQ(nostd::get(histogram_data.max_), 100); EXPECT_NO_THROW(aggr.Aggregate(13.0, {})); // lies in fourth bucket EXPECT_NO_THROW(aggr.Aggregate(252.0, {})); // lies in ninth bucket histogram_data = nostd::get(aggr.ToPoint()); @@ -177,6 +185,8 @@ TEST(Aggregation, DoubleHistogramAggregation) EXPECT_EQ(histogram_data.counts_[3], 2); EXPECT_EQ(histogram_data.counts_[8], 1); EXPECT_EQ(nostd::get(histogram_data.sum_), 377); + EXPECT_EQ(nostd::get(histogram_data.min_), 12); + EXPECT_EQ(nostd::get(histogram_data.max_), 252); // Merge DoubleHistogramAggregation aggr1; @@ -199,6 +209,8 @@ TEST(Aggregation, DoubleHistogramAggregation) EXPECT_EQ(histogram_data.counts_[3], 2); // 11.0, 13.0 EXPECT_EQ(histogram_data.counts_[4], 2); // 25.1, 28.1 EXPECT_EQ(histogram_data.counts_[7], 1); // 105.0 + EXPECT_EQ(nostd::get(histogram_data.min_), 1); + EXPECT_EQ(nostd::get(histogram_data.max_), 105); // Diff auto aggr4 = aggr1.Diff(aggr2); diff --git a/third_party/opentelemetry-proto b/third_party/opentelemetry-proto index 5c2fe5ddbd..c5c8b28012 160000 --- a/third_party/opentelemetry-proto +++ b/third_party/opentelemetry-proto @@ -1 +1 @@ -Subproject commit 5c2fe5ddbd9fb9f3e08f31d20f7566a3ee3d8885 +Subproject commit c5c8b28012583fda55b0cb16f73a820722171d49 diff --git a/third_party_release b/third_party_release index e8ad402c06..8d3d131516 100644 --- a/third_party_release +++ b/third_party_release @@ -18,6 +18,6 @@ benchmark=v1.5.3 googletest=release-1.10.0-459-ga6dfd3ac ms-gsl=v3.1.0-67-g6f45293 nlohmann-json=v3.10.5 -opentelemetry-proto=v0.17.0 +opentelemetry-proto=v0.18.0 prometheus-cpp=v1.0.0 vcpkg=2022.04.12