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.

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

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
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
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
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
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
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
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
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

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
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
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
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