Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Metrics SDK] Histogram min/max support #1540

Merged
merged 9 commits into from
Aug 6, 2022
Merged

[Metrics SDK] Histogram min/max support #1540

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0bec005
[Metrics SDK] Histogram min/max support
esigo Aug 3, 2022
8837a96
Update proto to v0.18.0
esigo Aug 4, 2022
fe9dc5d
upgrade proto
esigo Aug 4, 2022
678e9f9
otlp min max
esigo Aug 4, 2022
dff9422
otlp http unit test
esigo Aug 4, 2022
6b6bb43
histogram point data min and max
esigo Aug 5, 2022
1952c67
HistogramDiff
esigo Aug 5, 2022
d1b4585
Merge branch 'main' into Histogram-min/max
lalitb Aug 5, 2022
6753e6f
Merge branch 'main' into Histogram-min/max
lalitb Aug 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions exporters/ostream/src/metric_exporter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,26 @@ void OStreamMetricExporter::printPointData(const opentelemetry::sdk::metrics::Po
sout_ << nostd::get<long>(histogram_point_data.sum_);
}

if (histogram_point_data.record_min_max_)
{
if (nostd::holds_alternative<long>(histogram_point_data.min_))
{
sout_ << "\n min : " << nostd::get<long>(histogram_point_data.min_);
}
else if (nostd::holds_alternative<double>(histogram_point_data.min_))
{
sout_ << "\n min : " << nostd::get<double>(histogram_point_data.min_);
}
if (nostd::holds_alternative<long>(histogram_point_data.max_))
{
sout_ << "\n max : " << nostd::get<long>(histogram_point_data.max_);
}
if (nostd::holds_alternative<double>(histogram_point_data.max_))
{
sout_ << "\n max : " << nostd::get<double>(histogram_point_data.max_);
}
}

sout_ << "\n buckets : ";
if (nostd::holds_alternative<std::list<double>>(histogram_point_data.boundaries_))
{
Expand Down
4 changes: 4 additions & 0 deletions exporters/ostream/test/ostream_metric_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,8 @@ TEST(OStreamMetricsExporter, ExportHistogramPointData)
"\n type : HistogramPointData"
"\n count : 3"
"\n sum : 900.5"
"\n min : 0"
"\n max : 0"
"\n buckets : [10.1, 20.2, 30.2, ]"
"\n counts : [200, 300, 400, 500, ]"
"\n attributes\t\t: "
Expand All @@ -147,6 +149,8 @@ TEST(OStreamMetricsExporter, ExportHistogramPointData)
"\n type : HistogramPointData"
"\n count : 3"
"\n sum : 900"
"\n min : 0"
"\n max : 0"
Copy link
Member

@lalitb lalitb Aug 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Why are these values 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was because histogram_point_data, had default min and max.
thanks, added proper min & max now.

"\n buckets : [10, 20, 30, ]"
"\n counts : [200, 300, 400, 500, ]"
"\n attributes\t\t: "
Expand Down
1 change: 1 addition & 0 deletions sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class HistogramAggregationConfig : public AggregationConfig
{
public:
std::list<T> boundaries_;
bool record_min_max_ = true;
Copy link
Member

@lalitb lalitb Aug 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Seems we are setting it, but not using it :).

Pls ignore, it's been used. Sorry about that.

};
} // namespace metrics
} // namespace sdk
Expand Down
14 changes: 11 additions & 3 deletions sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -72,6 +73,7 @@ class DoubleHistogramAggregation : public Aggregation
private:
mutable opentelemetry::common::SpinLockMutex lock_;
mutable HistogramPointData point_data_;
bool record_min_max_ = true;
};

template <class T>
Expand All @@ -83,9 +85,15 @@ void HistogramMerge(HistogramPointData &current,
{
merge.counts_[i] = current.counts_[i] + delta.counts_[i];
}
merge.boundaries_ = current.boundaries_;
merge.sum_ = nostd::get<T>(current.sum_) + nostd::get<T>(delta.sum_);
merge.count_ = current.count_ + delta.count_;
merge.boundaries_ = current.boundaries_;
merge.sum_ = nostd::get<T>(current.sum_) + nostd::get<T>(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<T>(current.min_), nostd::get<T>(delta.min_));
merge.max_ = std::max(nostd::get<T>(current.max_), nostd::get<T>(delta.max_));
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how to handle for HistogramDiff, should we set record_min_max_ to false?

Copy link
Member Author

Choose a reason for hiding this comment

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

I set it to false for now.

}
}

template <class T>
Expand Down
3 changes: 3 additions & 0 deletions sdk/include/opentelemetry/sdk/metrics/data/point_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,11 @@ class HistogramPointData

ListType boundaries_ = {};
ValueType sum_ = {};
ValueType min_ = {};
ValueType max_ = {};
std::vector<uint64_t> counts_ = {};
uint64_t count_ = {};
bool record_min_max_ = true;
};

class DropPointData
Expand Down
42 changes: 34 additions & 8 deletions sdk/src/metrics/aggregation/histogram_aggregation.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

#include <algorithm>
#include <iomanip>
ThomsonTan marked this conversation as resolved.
Show resolved Hide resolved
#ifndef ENABLE_METRICS_PREVIEW
# include "opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h"
# include "opentelemetry/version.h"
Expand All @@ -23,26 +25,38 @@ LongHistogramAggregation::LongHistogramAggregation(
{
point_data_.boundaries_ = std::list<long>{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<uint64_t>(nostd::get<std::list<long>>(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<long>::max();
point_data_.max_ = std::numeric_limits<long>::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
{
const std::lock_guard<opentelemetry::common::SpinLockMutex> locked(lock_);
point_data_.count_ += 1;
point_data_.sum_ = nostd::get<long>(point_data_.sum_) + value;
size_t index = 0;
if (record_min_max_)
{
point_data_.min_ = std::min(nostd::get<long>(point_data_.min_), 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)
{
Expand Down Expand Up @@ -93,10 +107,17 @@ DoubleHistogramAggregation::DoubleHistogramAggregation(
point_data_.boundaries_ =
std::list<double>{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<uint64_t>(nostd::get<std::list<double>>(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<double>::max();
point_data_.max_ = std::numeric_limits<double>::min();
}

DoubleHistogramAggregation::DoubleHistogramAggregation(HistogramPointData &&data)
Expand All @@ -112,7 +133,12 @@ void DoubleHistogramAggregation::Aggregate(double value, const PointAttributes &
const std::lock_guard<opentelemetry::common::SpinLockMutex> locked(lock_);
point_data_.count_ += 1;
point_data_.sum_ = nostd::get<double>(point_data_.sum_) + value;
size_t index = 0;
if (record_min_max_)
{
point_data_.min_ = std::min(nostd::get<double>(point_data_.min_), 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)
{
Expand Down
12 changes: 12 additions & 0 deletions sdk/test/metrics/aggregation_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<HistogramPointData>(aggr.ToPoint());
EXPECT_EQ(nostd::get<long>(histogram_data.min_), 12);
EXPECT_EQ(nostd::get<long>(histogram_data.max_), 100);
EXPECT_EQ(nostd::get<long>(histogram_data.sum_), 112);
EXPECT_EQ(histogram_data.count_, 2);
EXPECT_EQ(histogram_data.counts_[3], 1);
Expand All @@ -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<long>(histogram_data.min_), 12);
EXPECT_EQ(nostd::get<long>(histogram_data.max_), 252);

// Merge
LongHistogramAggregation aggr1;
Expand All @@ -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<long>(histogram_data.min_), 1);
EXPECT_EQ(nostd::get<long>(histogram_data.max_), 105);

// Diff
auto aggr4 = aggr1.Diff(aggr2);
Expand Down Expand Up @@ -170,13 +176,17 @@ 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<double>(histogram_data.min_), 12);
EXPECT_EQ(nostd::get<double>(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<HistogramPointData>(aggr.ToPoint());
EXPECT_EQ(histogram_data.count_, 4);
EXPECT_EQ(histogram_data.counts_[3], 2);
EXPECT_EQ(histogram_data.counts_[8], 1);
EXPECT_EQ(nostd::get<double>(histogram_data.sum_), 377);
EXPECT_EQ(nostd::get<double>(histogram_data.min_), 12);
EXPECT_EQ(nostd::get<double>(histogram_data.max_), 252);

// Merge
DoubleHistogramAggregation aggr1;
Expand All @@ -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<double>(histogram_data.min_), 1);
EXPECT_EQ(nostd::get<double>(histogram_data.max_), 105);

// Diff
auto aggr4 = aggr1.Diff(aggr2);
Expand Down