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

Fix:1674, Add Monotonic Property to Sum Aggregation, and unit tests for Up Down Counter #1675

Merged
merged 18 commits into from
Oct 26, 2022
Merged

Fix:1674, Add Monotonic Property to Sum Aggregation, and unit tests for Up Down Counter #1675

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
a109486
add monotonicity to sum aggregation
lalitb Oct 12, 2022
f90ba7e
Merge branch 'main' into up-down-counter
esigo Oct 15, 2022
bf4e7e7
Merge branch 'main' into up-down-counter
esigo Oct 15, 2022
e880a3b
Merge branch 'main' into up-down-counter
lalitb Oct 17, 2022
bba260c
Merge branch 'main' into up-down-counter
lalitb Oct 20, 2022
e742b0f
Fix
lalitb Oct 20, 2022
18f4653
Merge branch 'up-down-counter' of github.com:lalitb/opentelemetry-cpp…
lalitb Oct 20, 2022
2ba30b8
fix
lalitb Oct 20, 2022
6285858
Merge branch 'main' into up-down-counter
esigo Oct 21, 2022
9c1ac08
Merge branch 'main' into up-down-counter
esigo Oct 21, 2022
2198b28
Merge branch 'main' into up-down-counter
esigo Oct 24, 2022
240e5f7
Merge branch 'main' into up-down-counter
esigo Oct 24, 2022
f848d02
Merge branch 'main' into up-down-counter
lalitb Oct 25, 2022
ee24e6c
fix
lalitb Oct 25, 2022
c2be9e8
fix
lalitb Oct 25, 2022
f70a8b1
fix
lalitb Oct 25, 2022
15d04d0
fix
lalitb Oct 25, 2022
6ff7036
Merge branch 'main' into up-down-counter
lalitb Oct 25, 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
3 changes: 2 additions & 1 deletion exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ class PrometheusExporterUtils
/**
* Translate the OTel metric type to Prometheus metric type
*/
static ::prometheus::MetricType TranslateType(opentelemetry::sdk::metrics::AggregationType kind);
static ::prometheus::MetricType TranslateType(opentelemetry::sdk::metrics::AggregationType kind,
bool is_monotonic = true);

/**
* Set metric data for:
Expand Down
26 changes: 22 additions & 4 deletions exporters/prometheus/src/exporter_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,14 @@ std::vector<prometheus_client::MetricFamily> PrometheusExporterUtils::TranslateT
auto time = metric_data.start_ts.time_since_epoch();
for (const auto &point_data_attr : metric_data.point_data_attr_)
{
auto kind = getAggregationType(point_data_attr.point_data);
const prometheus_client::MetricType type = TranslateType(kind);
auto kind = getAggregationType(point_data_attr.point_data);
bool is_monotonic = true;
if (kind == sdk::metrics::AggregationType::kSum)
{
is_monotonic =
nostd::get<sdk::metrics::SumPointData>(point_data_attr.point_data).is_monotonic_;
}
const prometheus_client::MetricType type = TranslateType(kind, is_monotonic);
metric_family.type = type;
if (type == prometheus_client::MetricType::Histogram) // Histogram
{
Expand Down Expand Up @@ -85,6 +91,14 @@ std::vector<prometheus_client::MetricFamily> PrometheusExporterUtils::TranslateT
std::vector<metric_sdk::ValueType> values{last_value_point_data.value_};
SetData(values, point_data_attr.attributes, type, time, &metric_family);
}
else if (nostd::holds_alternative<sdk::metrics::SumPointData>(
point_data_attr.point_data))
{
auto sum_point_data =
nostd::get<sdk::metrics::SumPointData>(point_data_attr.point_data);
std::vector<metric_sdk::ValueType> values{sum_point_data.value_};
SetData(values, point_data_attr.attributes, type, time, &metric_family);
}
else
{
OTEL_INTERNAL_LOG_WARN(
Expand Down Expand Up @@ -159,12 +173,16 @@ metric_sdk::AggregationType PrometheusExporterUtils::getAggregationType(
* Translate the OTel metric type to Prometheus metric type
*/
prometheus_client::MetricType PrometheusExporterUtils::TranslateType(
metric_sdk::AggregationType kind)
metric_sdk::AggregationType kind,
bool is_monotonic)
{
switch (kind)
{
case metric_sdk::AggregationType::kSum:
return prometheus_client::MetricType::Counter;
if (!is_monotonic)
esigo marked this conversation as resolved.
Show resolved Hide resolved
return prometheus_client::MetricType::Gauge;
else
return prometheus_client::MetricType::Counter;
case metric_sdk::AggregationType::kHistogram:
return prometheus_client::MetricType::Histogram;
case metric_sdk::AggregationType::kLastValue:
Expand Down
22 changes: 16 additions & 6 deletions sdk/include/opentelemetry/sdk/metrics/aggregation/default_aggregation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,15 @@ class DefaultAggregation
switch (instrument_descriptor.type_)
{
case InstrumentType::kCounter:
case InstrumentType::kUpDownCounter:
case InstrumentType::kObservableCounter:
return (instrument_descriptor.value_type_ == InstrumentValueType::kLong)
? std::move(std::unique_ptr<Aggregation>(new LongSumAggregation(true)))
: std::move(std::unique_ptr<Aggregation>(new DoubleSumAggregation(true)));
case InstrumentType::kUpDownCounter:
case InstrumentType::kObservableUpDownCounter:
return (instrument_descriptor.value_type_ == InstrumentValueType::kLong)
? std::move(std::unique_ptr<Aggregation>(new LongSumAggregation()))
: std::move(std::unique_ptr<Aggregation>(new DoubleSumAggregation()));
? std::move(std::unique_ptr<Aggregation>(new LongSumAggregation(false)))
: std::move(std::unique_ptr<Aggregation>(new DoubleSumAggregation(false)));
break;
case InstrumentType::kHistogram: {
if (instrument_descriptor.value_type_ == InstrumentValueType::kLong)
Expand Down Expand Up @@ -91,16 +94,23 @@ class DefaultAggregation
return std::unique_ptr<Aggregation>(new DoubleLastValueAggregation());
}
break;
case AggregationType::kSum:
case AggregationType::kSum: {
bool is_monotonic = true;
if (instrument_descriptor.type_ == InstrumentType::kUpDownCounter ||
instrument_descriptor.type_ == InstrumentType::kObservableUpDownCounter)
{
is_monotonic = false;
}
if (instrument_descriptor.value_type_ == InstrumentValueType::kLong)
{
return std::unique_ptr<Aggregation>(new LongSumAggregation());
return std::unique_ptr<Aggregation>(new LongSumAggregation(is_monotonic));
}
else
{
return std::unique_ptr<Aggregation>(new DoubleSumAggregation());
return std::unique_ptr<Aggregation>(new DoubleSumAggregation(is_monotonic));
}
break;
}
default:
return DefaultAggregation::CreateAggregation(instrument_descriptor, aggregation_config);
}
Expand Down
4 changes: 2 additions & 2 deletions sdk/include/opentelemetry/sdk/metrics/aggregation/sum_aggregation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ namespace metrics
class LongSumAggregation : public Aggregation
{
public:
LongSumAggregation();
LongSumAggregation(bool is_monotonic);
LongSumAggregation(SumPointData &&);
LongSumAggregation(const SumPointData &);

Expand All @@ -39,7 +39,7 @@ class LongSumAggregation : public Aggregation
class DoubleSumAggregation : public Aggregation
{
public:
DoubleSumAggregation();
DoubleSumAggregation(bool is_monotonic);
DoubleSumAggregation(SumPointData &&);
DoubleSumAggregation(const SumPointData &);

Expand Down
3 changes: 2 additions & 1 deletion 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 @@ -29,7 +29,8 @@ class SumPointData
SumPointData &operator=(SumPointData &&) = default;
SumPointData() = default;

ValueType value_ = {};
ValueType value_ = {};
bool is_monotonic_ = true;
};

class LastValuePointData
Expand Down
32 changes: 23 additions & 9 deletions sdk/src/metrics/aggregation/sum_aggregation.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#ifndef ENABLE_METRICS_PREVIEW
# include "opentelemetry/sdk/metrics/aggregation/sum_aggregation.h"
# include "opentelemetry/sdk/common/global_log_handler.h"
# include "opentelemetry/sdk/metrics/data/point_data.h"
# include "opentelemetry/version.h"

Expand All @@ -15,9 +16,10 @@ namespace sdk
namespace metrics
{

LongSumAggregation::LongSumAggregation()
LongSumAggregation::LongSumAggregation(bool is_monotonic)
{
point_data_.value_ = 0l;
point_data_.value_ = 0l;
point_data_.is_monotonic_ = is_monotonic;
}

LongSumAggregation::LongSumAggregation(SumPointData &&data) : point_data_{std::move(data)} {}
Expand All @@ -26,6 +28,12 @@ LongSumAggregation::LongSumAggregation(const SumPointData &data) : point_data_{d

void LongSumAggregation::Aggregate(long value, const PointAttributes & /* attributes */) noexcept
{
if (point_data_.is_monotonic_ && value < 0)
{
OTEL_INTERNAL_LOG_WARN(" Negative value ignored for Monotonic increasing measurement. Value"
<< value);
Copy link
Member

Choose a reason for hiding this comment

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

Ok for now.

In the long term, to be of any use, the warning message will need to add some context, like the name of the meter / metric involved that causes the warning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. added slightly more context here.

return;
}
const std::lock_guard<opentelemetry::common::SpinLockMutex> locked(lock_);
point_data_.value_ = nostd::get<long>(point_data_.value_) + value;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does overflow need to be handled here for the sum operation?

Copy link
Member Author

@lalitb lalitb Oct 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

With #1686 we use int64_t to store the sum. The numeric range limit of signed 64-bit is enormous, while it can still eventually overflow if the system generating measurement keeps running for long, without resetting. Unless there are real-world scenarios for the overflow, I think we can let go for now the overflow scenario (and so avoid extra validation for every measurement added). Even in the case of such real-world examples, the exporter can be configured with Delta temporality (if the backend system supports it), and so it would bypass adding the values. Otherwise, we can rely on the backend system to detect any such overflow and handle it accordingly.

}
Expand All @@ -37,7 +45,7 @@ std::unique_ptr<Aggregation> LongSumAggregation::Merge(const Aggregation &delta)
nostd::get<SumPointData>((static_cast<const LongSumAggregation &>(delta).ToPoint()))
.value_) +
nostd::get<long>(nostd::get<SumPointData>(ToPoint()).value_);
std::unique_ptr<Aggregation> aggr(new LongSumAggregation());
std::unique_ptr<Aggregation> aggr(new LongSumAggregation(point_data_.is_monotonic_));
static_cast<LongSumAggregation *>(aggr.get())->point_data_.value_ = merge_value;
return aggr;
}
Expand All @@ -49,7 +57,7 @@ std::unique_ptr<Aggregation> LongSumAggregation::Diff(const Aggregation &next) c
(static_cast<const LongSumAggregation &>(next).ToPoint()))
.value_) -
nostd::get<long>(nostd::get<SumPointData>(ToPoint()).value_);
std::unique_ptr<Aggregation> aggr(new LongSumAggregation());
std::unique_ptr<Aggregation> aggr(new LongSumAggregation(point_data_.is_monotonic_));
static_cast<LongSumAggregation *>(aggr.get())->point_data_.value_ = diff_value;
return aggr;
}
Expand All @@ -60,9 +68,10 @@ PointType LongSumAggregation::ToPoint() const noexcept
return point_data_;
}

DoubleSumAggregation::DoubleSumAggregation()
DoubleSumAggregation::DoubleSumAggregation(bool is_monotonic)
{
point_data_.value_ = 0.0;
point_data_.value_ = 0.0;
point_data_.is_monotonic_ = is_monotonic;
}

DoubleSumAggregation::DoubleSumAggregation(SumPointData &&data) : point_data_(std::move(data)) {}
Expand All @@ -72,6 +81,12 @@ DoubleSumAggregation::DoubleSumAggregation(const SumPointData &data) : point_dat
void DoubleSumAggregation::Aggregate(double value,
const PointAttributes & /* attributes */) noexcept
{
if (point_data_.is_monotonic_ && value < 0)
{
OTEL_INTERNAL_LOG_WARN(" Negative value ignored for Monotonic increasing measurement. Value"
<< value);
return;
}
const std::lock_guard<opentelemetry::common::SpinLockMutex> locked(lock_);
point_data_.value_ = nostd::get<double>(point_data_.value_) + value;
}
Expand All @@ -83,20 +98,19 @@ std::unique_ptr<Aggregation> DoubleSumAggregation::Merge(const Aggregation &delt
nostd::get<SumPointData>((static_cast<const DoubleSumAggregation &>(delta).ToPoint()))
.value_) +
nostd::get<double>(nostd::get<SumPointData>(ToPoint()).value_);
std::unique_ptr<Aggregation> aggr(new DoubleSumAggregation());
std::unique_ptr<Aggregation> aggr(new DoubleSumAggregation(point_data_.is_monotonic_));
static_cast<DoubleSumAggregation *>(aggr.get())->point_data_.value_ = merge_value;
return aggr;
}

std::unique_ptr<Aggregation> DoubleSumAggregation::Diff(const Aggregation &next) const noexcept
{

double diff_value =
nostd::get<double>(
nostd::get<SumPointData>((static_cast<const DoubleSumAggregation &>(next).ToPoint()))
.value_) -
nostd::get<double>(nostd::get<SumPointData>(ToPoint()).value_);
std::unique_ptr<Aggregation> aggr(new DoubleSumAggregation());
std::unique_ptr<Aggregation> aggr(new DoubleSumAggregation(point_data_.is_monotonic_));
static_cast<DoubleSumAggregation *>(aggr.get())->point_data_.value_ = diff_value;
return aggr;
}
Expand Down
1 change: 1 addition & 0 deletions sdk/test/metrics/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ foreach(
attributes_hashmap_test
sync_metric_storage_counter_test
sync_metric_storage_histogram_test
sync_metric_storage_up_down_counter_test
async_metric_storage_test
multi_metric_storage_test
observer_result_test
Expand Down
4 changes: 2 additions & 2 deletions sdk/test/metrics/aggregation_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ using namespace opentelemetry::sdk::metrics;
namespace nostd = opentelemetry::nostd;
TEST(Aggregation, LongSumAggregation)
{
LongSumAggregation aggr;
LongSumAggregation aggr(true);
auto data = aggr.ToPoint();
ASSERT_TRUE(nostd::holds_alternative<SumPointData>(data));
auto sum_data = nostd::get<SumPointData>(data);
Expand All @@ -28,7 +28,7 @@ TEST(Aggregation, LongSumAggregation)

TEST(Aggregation, DoubleSumAggregation)
{
DoubleSumAggregation aggr;
DoubleSumAggregation aggr(true);
auto data = aggr.ToPoint();
ASSERT_TRUE(nostd::holds_alternative<SumPointData>(data));
auto sum_data = nostd::get<SumPointData>(data);
Expand Down
4 changes: 2 additions & 2 deletions sdk/test/metrics/sync_metric_storage_counter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class MockCollectorHandle : public CollectorHandle
class WritableMetricStorageTestFixture : public ::testing::TestWithParam<AggregationTemporality>
{};

TEST_P(WritableMetricStorageTestFixture, LongSumAggregation)
TEST_P(WritableMetricStorageTestFixture, LongCounterSumAggregation)
{
AggregationTemporality temporality = GetParam();
auto sdk_start_ts = std::chrono::system_clock::now();
Expand Down Expand Up @@ -172,7 +172,7 @@ INSTANTIATE_TEST_SUITE_P(WritableMetricStorageTestLong,
::testing::Values(AggregationTemporality::kCumulative,
AggregationTemporality::kDelta));

TEST_P(WritableMetricStorageTestFixture, DoubleSumAggregation)
TEST_P(WritableMetricStorageTestFixture, DoubleCounterSumAggregation)
{
AggregationTemporality temporality = GetParam();
auto sdk_start_ts = std::chrono::system_clock::now();
Expand Down
Loading