Skip to content

Commit

Permalink
[Metrics SDK] Change boundry type to double for Explicit Bucket His…
Browse files Browse the repository at this point in the history 
…togram Aggregation, and change default bucket range (#1626)

* fix bucket boundaries type for histogram aggregation

* update bucket range, and fix build
  • Loading branch information
@lalitb
lalitb authored Oct 2, 2022
1 parent 9e87a6e commit d127140
Show file tree
Hide file tree
Showing 11 changed files with 33 additions and 66 deletions.
11 changes: 1 addition & 10 deletions exporters/ostream/src/metric_exporter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,16 +205,7 @@ void OStreamMetricExporter::printPointData(const opentelemetry::sdk::metrics::Po
}

sout_ << "\n buckets : ";
if (nostd::holds_alternative<std::list<double>>(histogram_point_data.boundaries_))
{
auto &double_boundaries = nostd::get<std::list<double>>(histogram_point_data.boundaries_);
printVec(sout_, double_boundaries);
}
else if (nostd::holds_alternative<std::list<long>>(histogram_point_data.boundaries_))
{
auto &long_boundaries = nostd::get<std::list<long>>(histogram_point_data.boundaries_);
printVec(sout_, long_boundaries);
}
printVec(sout_, histogram_point_data.boundaries_);

sout_ << "\n counts : ";
printVec(sout_, histogram_point_data.counts_);
Expand Down
2 changes: 1 addition & 1 deletion exporters/ostream/test/ostream_metric_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ TEST(OStreamMetricsExporter, ExportHistogramPointData)
histogram_point_data.min_ = 1.8;
histogram_point_data.max_ = 12.0;
metric_sdk::HistogramPointData histogram_point_data2{};
histogram_point_data2.boundaries_ = std::list<long>{10, 20, 30};
histogram_point_data2.boundaries_ = std::list<double>{10.0, 20.0, 30.0};
histogram_point_data2.count_ = 3;
histogram_point_data2.counts_ = {200, 300, 400, 500};
histogram_point_data2.sum_ = 900l;
Expand Down
17 changes: 3 additions & 14 deletions exporters/otlp/src/otlp_metric_utils.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,21 +126,10 @@ void OtlpMetricUtils::ConvertHistogramMetric(
}
}
// buckets
if ((nostd::holds_alternative<std::list<double>>(histogram_data.boundaries_)))
{
auto boundaries = nostd::get<std::list<double>>(histogram_data.boundaries_);
for (auto bound : boundaries)
{
proto_histogram_point_data->add_explicit_bounds(bound);
}
}
else

for (auto bound : histogram_data.boundaries_)
{
auto boundaries = nostd::get<std::list<long>>(histogram_data.boundaries_);
for (auto bound : boundaries)
{
proto_histogram_point_data->add_explicit_bounds(bound);
}
proto_histogram_point_data->add_explicit_bounds(bound);
}
// bucket counts
for (auto bucket_value : histogram_data.counts_)
Expand Down
8 changes: 4 additions & 4 deletions exporters/otlp/test/otlp_http_metric_exporter_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -482,14 +482,14 @@ class OtlpHttpMetricExporterTestPeer : public ::testing::Test
auto exporter = GetExporter(std::unique_ptr<OtlpHttpClient>{mock_otlp_http_client});

opentelemetry::sdk::metrics::HistogramPointData histogram_point_data{};
histogram_point_data.boundaries_ = std::list<double>{10.1, 20.2, 30.2};
histogram_point_data.boundaries_ = {10.1, 20.2, 30.2};
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<long>{10, 20, 30};
histogram_point_data2.boundaries_ = {10.0, 20.0, 30.0};
histogram_point_data2.count_ = 3;
histogram_point_data2.counts_ = {200, 300, 400, 500};
histogram_point_data2.sum_ = 900l;
Expand Down Expand Up @@ -619,12 +619,12 @@ class OtlpHttpMetricExporterTestPeer : public ::testing::Test
"library_name", "1.5.0");

opentelemetry::sdk::metrics::HistogramPointData histogram_point_data{};
histogram_point_data.boundaries_ = std::list<double>{10.1, 20.2, 30.2};
histogram_point_data.boundaries_ = {10.1, 20.2, 30.2};
histogram_point_data.count_ = 3;
histogram_point_data.counts_ = {200, 300, 400, 500};
histogram_point_data.sum_ = 900.5;
opentelemetry::sdk::metrics::HistogramPointData histogram_point_data2{};
histogram_point_data2.boundaries_ = std::list<long>{10, 20, 30};
histogram_point_data2.boundaries_ = {10.0, 20.0, 30.0};
histogram_point_data2.count_ = 3;
histogram_point_data2.counts_ = {200, 300, 400, 500};
histogram_point_data2.sum_ = 900l;
Expand Down
6 changes: 3 additions & 3 deletions exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ class PrometheusExporterUtils
*/
template <typename T>
static void SetData(std::vector<T> values,
const opentelemetry::sdk::metrics::ListType &boundaries,
const std::list<double> &boundaries,
const std::vector<uint64_t> &counts,
const opentelemetry::sdk::metrics::PointAttributes &labels,
std::chrono::nanoseconds time,
Expand Down Expand Up @@ -103,9 +103,9 @@ class PrometheusExporterUtils
/**
* Handle Histogram
*/
template <typename T, typename U>
template <typename T>
static void SetValue(std::vector<T> values,
const std::list<U> &boundaries,
const std::list<double> &boundaries,
const std::vector<uint64_t> &counts,
::prometheus::ClientMetric *metric);
};
Expand Down
15 changes: 4 additions & 11 deletions exporters/prometheus/src/exporter_utils.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ void PrometheusExporterUtils::SetData(std::vector<T> values,
*/
template <typename T>
void PrometheusExporterUtils::SetData(std::vector<T> values,
const opentelemetry::sdk::metrics::ListType &boundaries,
const std::list<double> &boundaries,
const std::vector<uint64_t> &counts,
const metric_sdk::PointAttributes &labels,
std::chrono::nanoseconds time,
Expand All @@ -206,14 +206,7 @@ void PrometheusExporterUtils::SetData(std::vector<T> values,
metric_family->metric.emplace_back();
prometheus_client::ClientMetric &metric = metric_family->metric.back();
SetMetricBasic(metric, time, labels);
if (nostd::holds_alternative<std::list<long>>(boundaries))
{
SetValue(values, nostd::get<std::list<long>>(boundaries), counts, &metric);
}
else
{
SetValue(values, nostd::get<std::list<double>>(boundaries), counts, &metric);
}
SetValue(values, boundaries, counts, &metric);
}

/**
Expand Down Expand Up @@ -321,9 +314,9 @@ void PrometheusExporterUtils::SetValue(std::vector<T> values,
/**
* Handle Histogram
*/
template <typename T, typename U>
template <typename T>
void PrometheusExporterUtils::SetValue(std::vector<T> values,
const std::list<U> &boundaries,
const std::list<double> &boundaries,
const std::vector<uint64_t> &counts,
prometheus_client::ClientMetric *metric)
{
Expand Down
4 changes: 2 additions & 2 deletions exporters/prometheus/test/prometheus_test_helper.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,12 @@ inline metric_sdk::ResourceMetrics CreateSumPointData()
inline metric_sdk::ResourceMetrics CreateHistogramPointData()
{
metric_sdk::HistogramPointData histogram_point_data{};
histogram_point_data.boundaries_ = std::list<double>{10.1, 20.2, 30.2};
histogram_point_data.boundaries_ = {10.1, 20.2, 30.2};
histogram_point_data.count_ = 3;
histogram_point_data.counts_ = {200, 300, 400, 500};
histogram_point_data.sum_ = 900.5;
metric_sdk::HistogramPointData histogram_point_data2{};
histogram_point_data2.boundaries_ = std::list<long>{10, 20, 30};
histogram_point_data2.boundaries_ = {10.0, 20.0, 30.0};
histogram_point_data2.count_ = 3;
histogram_point_data2.counts_ = {200, 300, 400, 500};
histogram_point_data2.sum_ = 900l;
Expand Down
2 changes: 1 addition & 1 deletion sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ template <typename T>
class HistogramAggregationConfig : public AggregationConfig
{
public:
std::list<T> boundaries_;
std::list<double> boundaries_;
bool record_min_max_ = true;
};
} // namespace metrics
Expand Down
5 changes: 2 additions & 3 deletions sdk/include/opentelemetry/sdk/metrics/data/point_data.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ namespace metrics
{

using ValueType = nostd::variant<long, double>;
using ListType = nostd::variant<std::list<long>, std::list<double>>;

// TODO: remove ctors and initializers from below classes when GCC<5 stops shipping on Ubuntu

Expand Down Expand Up @@ -55,8 +54,8 @@ class HistogramPointData
HistogramPointData &operator=(HistogramPointData &&) = default;
HistogramPointData(const HistogramPointData &) = default;
HistogramPointData() = default;

ListType boundaries_ = {};
HistogramPointData(std::list<double> &boundaries) : boundaries_(boundaries) {}
std::list<double> boundaries_ = {};
ValueType sum_ = {};
ValueType min_ = {};
ValueType max_ = {};
Expand Down
16 changes: 7 additions & 9 deletions sdk/src/metrics/aggregation/histogram_aggregation.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,15 @@ LongHistogramAggregation::LongHistogramAggregation(
}
else
{
point_data_.boundaries_ = std::list<long>{0l, 5l, 10l, 25l, 50l, 75l, 100l, 250l, 500l, 1000l};
point_data_.boundaries_ = {0.0, 5.0, 10.0, 25.0, 50.0, 75.0, 100.0, 250.0,
500.0, 750.0, 1000.0, 2500.0, 5000.0, 7500.0, 10000.0};
}

if (aggregation_config)
{
record_min_max_ = aggregation_config->record_min_max_;
}
point_data_.counts_ =
std::vector<uint64_t>(nostd::get<std::list<long>>(point_data_.boundaries_).size() + 1, 0);
point_data_.counts_ = std::vector<uint64_t>(point_data_.boundaries_.size() + 1, 0);
point_data_.sum_ = 0l;
point_data_.count_ = 0;
point_data_.record_min_max_ = record_min_max_;
Expand Down Expand Up @@ -59,8 +60,7 @@ void LongHistogramAggregation::Aggregate(long value,
point_data_.max_ = std::max(nostd::get<long>(point_data_.max_), value);
}
size_t index = 0;
for (auto it = nostd::get<std::list<long>>(point_data_.boundaries_).begin();
it != nostd::get<std::list<long>>(point_data_.boundaries_).end(); ++it)
for (auto it = point_data_.boundaries_.begin(); it != point_data_.boundaries_.end(); ++it)
{
if (value < *it)
{
Expand Down Expand Up @@ -114,8 +114,7 @@ DoubleHistogramAggregation::DoubleHistogramAggregation(
{
record_min_max_ = aggregation_config->record_min_max_;
}
point_data_.counts_ =
std::vector<uint64_t>(nostd::get<std::list<double>>(point_data_.boundaries_).size() + 1, 0);
point_data_.counts_ = std::vector<uint64_t>(point_data_.boundaries_.size() + 1, 0);
point_data_.sum_ = 0.0;
point_data_.count_ = 0;
point_data_.record_min_max_ = record_min_max_;
Expand Down Expand Up @@ -143,8 +142,7 @@ void DoubleHistogramAggregation::Aggregate(double value,
point_data_.max_ = std::max(nostd::get<double>(point_data_.max_), value);
}
size_t index = 0;
for (auto it = nostd::get<std::list<double>>(point_data_.boundaries_).begin();
it != nostd::get<std::list<double>>(point_data_.boundaries_).end(); ++it)
for (auto it = point_data_.boundaries_.begin(); it != point_data_.boundaries_.end(); ++it)
{
if (value < *it)
{
Expand Down
13 changes: 5 additions & 8 deletions sdk/test/metrics/aggregation_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ TEST(Aggregation, LongHistogramAggregation)
ASSERT_TRUE(nostd::holds_alternative<HistogramPointData>(data));
auto histogram_data = nostd::get<HistogramPointData>(data);
ASSERT_TRUE(nostd::holds_alternative<long>(histogram_data.sum_));
ASSERT_TRUE(nostd::holds_alternative<std::list<long>>(histogram_data.boundaries_));
EXPECT_EQ(nostd::get<long>(histogram_data.sum_), 0);
EXPECT_EQ(histogram_data.count_, 0);
aggr.Aggregate(12l, {}); // lies in fourth bucket
Expand Down Expand Up @@ -134,14 +133,14 @@ TEST(Aggregation, LongHistogramAggregationBoundaries)
{
nostd::shared_ptr<opentelemetry::sdk::metrics::HistogramAggregationConfig<long>>
aggregation_config{new opentelemetry::sdk::metrics::HistogramAggregationConfig<long>};
std::list<long> user_boundaries = {0, 50, 100, 250, 500, 750, 1000, 2500, 5000, 10000};
aggregation_config->boundaries_ = user_boundaries;
std::list<double> user_boundaries = {0.0, 50.0, 100.0, 250.0, 500.0,
750.0, 1000.0, 2500.0, 5000.0, 10000.0};
aggregation_config->boundaries_ = user_boundaries;
LongHistogramAggregation aggr{aggregation_config.get()};
auto data = aggr.ToPoint();
ASSERT_TRUE(nostd::holds_alternative<HistogramPointData>(data));
auto histogram_data = nostd::get<HistogramPointData>(data);
ASSERT_TRUE(nostd::holds_alternative<std::list<long>>(histogram_data.boundaries_));
EXPECT_EQ(nostd::get<std::list<long>>(histogram_data.boundaries_), user_boundaries);
EXPECT_EQ(histogram_data.boundaries_, user_boundaries);
}

TEST(Aggregation, DoubleHistogramAggregationBoundaries)
Expand All @@ -155,8 +154,7 @@ TEST(Aggregation, DoubleHistogramAggregationBoundaries)
auto data = aggr.ToPoint();
ASSERT_TRUE(nostd::holds_alternative<HistogramPointData>(data));
auto histogram_data = nostd::get<HistogramPointData>(data);
ASSERT_TRUE(nostd::holds_alternative<std::list<double>>(histogram_data.boundaries_));
EXPECT_EQ(nostd::get<std::list<double>>(histogram_data.boundaries_), user_boundaries);
EXPECT_EQ(histogram_data.boundaries_, user_boundaries);
}

TEST(Aggregation, DoubleHistogramAggregation)
Expand All @@ -166,7 +164,6 @@ TEST(Aggregation, DoubleHistogramAggregation)
ASSERT_TRUE(nostd::holds_alternative<HistogramPointData>(data));
auto histogram_data = nostd::get<HistogramPointData>(data);
ASSERT_TRUE(nostd::holds_alternative<double>(histogram_data.sum_));
ASSERT_TRUE(nostd::holds_alternative<std::list<double>>(histogram_data.boundaries_));
EXPECT_EQ(nostd::get<double>(histogram_data.sum_), 0);
EXPECT_EQ(histogram_data.count_, 0);
aggr.Aggregate(12.0, {}); // lies in fourth bucket
Expand Down

2 comments on commit d127140

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'OpenTelemetry-cpp api Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: d127140 Previous: 9e87a6e Ratio
BM_ExtractBaggageWith180Entries 6.1354156617438145 ns/iter 1.6198604891008388 ns/iter 3.79

This comment was automatically generated by workflow using github-action-benchmark.

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'OpenTelemetry-cpp sdk Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: d127140 Previous: 9e87a6e Ratio
BM_BaselineBuffer/2 5451543.33114624 ns/iter 2160356.283187866 ns/iter 2.52
BM_LockFreeBuffer/1 1084582.4412569741 ns/iter 311929.84262425307 ns/iter 3.48
BM_LockFreeBuffer/2 2864707.7083587646 ns/iter 1009629.0111541748 ns/iter 2.84

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.