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

[Metrics] Switch to explicit 64 bit integers #1686

Merged
merged 25 commits into from
Oct 25, 2022
Merged
Show file tree
Hide file tree
Changes from 7 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
6 changes: 3 additions & 3 deletions api/include/opentelemetry/metrics/meter.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class Meter
* @return a shared pointer to the created Counter.
*/

virtual nostd::shared_ptr<Counter<long>> CreateLongCounter(
virtual nostd::shared_ptr<Counter<int64_t>> CreateLongCounter(
Copy link
Member

Choose a reason for hiding this comment

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

When "long" is compiled as a 32bit int, this change is an API and ABI breaking change.

This is ok as only metrics are affected, which are not GA yet.

Still, please add a CHANGELOG section to indicate this is a breaking change compared to 1.6.1

Copy link
Member

@lalitb lalitb Oct 17, 2022

Choose a reason for hiding this comment

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

Do we need to rename the method to CreateInt64Counter to be in consistent with the argument type?
If later we want to support 32bit integers at API level (say for embedded devices where memory is constraint), would it be confusing, or we can just add CreateShortCounter for it.

Copy link
Member Author

@esigo esigo Oct 20, 2022

Choose a reason for hiding this comment

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

thanks, CHANGELOG updated and the functions renamed as was suggested here #1686 (comment).

nostd::string_view name,
nostd::string_view description = "",
nostd::string_view unit = "") noexcept = 0;
Expand Down Expand Up @@ -72,7 +72,7 @@ class Meter
* @param unit the unit of metric values following https://unitsofmeasure.org/ucum.html.
* @return a shared pointer to the created Histogram.
*/
virtual nostd::shared_ptr<Histogram<long>> CreateLongHistogram(
virtual nostd::shared_ptr<Histogram<int64_t>> CreateLongHistogram(
Copy link
Member

@marcalff marcalff Oct 18, 2022

Choose a reason for hiding this comment

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

Possibly rename to CreateUInt64Histogram, and use uint64_t, to discuss

nostd::string_view name,
nostd::string_view description = "",
nostd::string_view unit = "") noexcept = 0;
Expand Down Expand Up @@ -109,7 +109,7 @@ class Meter
* @param unit the unit of metric values following https://unitsofmeasure.org/ucum.html.
* @return a shared pointer to the created UpDownCounter.
*/
virtual nostd::shared_ptr<UpDownCounter<long>> CreateLongUpDownCounter(
virtual nostd::shared_ptr<UpDownCounter<int64_t>> CreateLongUpDownCounter(
Copy link
Member

Choose a reason for hiding this comment

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

Possibly rename to CreateInt64UpDownCounter, to discuss

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, renamed all the functions as suggested here #1686 (comment)

nostd::string_view name,
nostd::string_view description = "",
nostd::string_view unit = "") noexcept = 0;
Expand Down
20 changes: 11 additions & 9 deletions api/include/opentelemetry/metrics/noop.h
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,12 @@ class NoopObservableInstrument : public ObservableInstrument
class NoopMeter final : public Meter
{
public:
nostd::shared_ptr<Counter<long>> CreateLongCounter(nostd::string_view name,
nostd::string_view description = "",
nostd::string_view unit = "") noexcept override
nostd::shared_ptr<Counter<int64_t>> CreateLongCounter(
nostd::string_view name,
nostd::string_view description = "",
nostd::string_view unit = "") noexcept override
{
return nostd::shared_ptr<Counter<long>>{new NoopCounter<long>(name, description, unit)};
return nostd::shared_ptr<Counter<int64_t>>{new NoopCounter<int64_t>(name, description, unit)};
}

nostd::shared_ptr<Counter<double>> CreateDoubleCounter(
Expand Down Expand Up @@ -120,12 +121,13 @@ class NoopMeter final : public Meter
new NoopObservableInstrument(name, description, unit));
}

nostd::shared_ptr<Histogram<long>> CreateLongHistogram(
nostd::shared_ptr<Histogram<int64_t>> CreateLongHistogram(
nostd::string_view name,
nostd::string_view description = "",
nostd::string_view unit = "") noexcept override
{
return nostd::shared_ptr<Histogram<long>>{new NoopHistogram<long>(name, description, unit)};
return nostd::shared_ptr<Histogram<int64_t>>{
new NoopHistogram<int64_t>(name, description, unit)};
}

nostd::shared_ptr<Histogram<double>> CreateDoubleHistogram(
Expand Down Expand Up @@ -154,13 +156,13 @@ class NoopMeter final : public Meter
new NoopObservableInstrument(name, description, unit));
}

nostd::shared_ptr<UpDownCounter<long>> CreateLongUpDownCounter(
nostd::shared_ptr<UpDownCounter<int64_t>> CreateLongUpDownCounter(
nostd::string_view name,
nostd::string_view description = "",
nostd::string_view unit = "") noexcept override
{
return nostd::shared_ptr<UpDownCounter<long>>{
new NoopUpDownCounter<long>(name, description, unit)};
return nostd::shared_ptr<UpDownCounter<int64_t>>{
new NoopUpDownCounter<int64_t>(name, description, unit)};
}

nostd::shared_ptr<UpDownCounter<double>> CreateDoubleUpDownCounter(
Expand Down
2 changes: 1 addition & 1 deletion api/include/opentelemetry/metrics/observer_result.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class ObserverResultT
}
};

using ObserverResult = nostd::variant<nostd::shared_ptr<ObserverResultT<long>>,
using ObserverResult = nostd::variant<nostd::shared_ptr<ObserverResultT<int64_t>>,
lalitb marked this conversation as resolved.
Show resolved Hide resolved
nostd::shared_ptr<ObserverResultT<double>>>;

} // namespace metrics
Expand Down
42 changes: 21 additions & 21 deletions api/test/metrics/noop_sync_instrument_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,42 +9,42 @@

TEST(Counter, Add)
{
std::shared_ptr<opentelemetry::metrics::Counter<long>> counter{
new opentelemetry::metrics::NoopCounter<long>("test", "none", "unitless")};
std::shared_ptr<opentelemetry::metrics::Counter<int64_t>> counter{
Copy link
Member

Choose a reason for hiding this comment

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

uint64_t, as counter is always non-negative value.

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

new opentelemetry::metrics::NoopCounter<int64_t>("test", "none", "unitless")};

std::map<std::string, std::string> labels = {{"k1", "v1"}};
counter->Add(10l, labels);
counter->Add(10l, labels, opentelemetry::context::Context{});
counter->Add(2l);
counter->Add(2l, opentelemetry::context::Context{});
counter->Add(10l, {{"k1", "1"}, {"k2", 2}});
counter->Add(10l, {{"k1", "1"}, {"k2", 2}}, opentelemetry::context::Context{});
counter->Add((int64_t)10, labels);
marcalff marked this conversation as resolved.
Show resolved Hide resolved
counter->Add((int64_t)10, labels, opentelemetry::context::Context{});
counter->Add((int64_t)2);
counter->Add((int64_t)2, opentelemetry::context::Context{});
counter->Add((int64_t)10, {{"k1", "1"}, {"k2", 2}});
counter->Add((int64_t)10, {{"k1", "1"}, {"k2", 2}}, opentelemetry::context::Context{});
}

TEST(histogram, Record)
{
std::shared_ptr<opentelemetry::metrics::Histogram<long>> counter{
new opentelemetry::metrics::NoopHistogram<long>("test", "none", "unitless")};
std::shared_ptr<opentelemetry::metrics::Histogram<int64_t>> counter{
Copy link
Member

@lalitb lalitb Oct 20, 2022

Choose a reason for hiding this comment

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

uint64_t, as histogram is always non-negative value?

Not related to this PR, but while you are here, can you rename variable name to histogram :)

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, changed to unit64_t.

new opentelemetry::metrics::NoopHistogram<int64_t>("test", "none", "unitless")};

std::map<std::string, std::string> labels = {{"k1", "v1"}};
counter->Record(10l, labels, opentelemetry::context::Context{});
counter->Record(2l, opentelemetry::context::Context{});
counter->Record((int64_t)10, labels, opentelemetry::context::Context{});
counter->Record((int64_t)2, opentelemetry::context::Context{});

counter->Record(10l, {{"k1", "1"}, {"k2", 2}}, opentelemetry::context::Context{});
counter->Record((int64_t)10, {{"k1", "1"}, {"k2", 2}}, opentelemetry::context::Context{});
}

TEST(UpDownCountr, Record)
{
std::shared_ptr<opentelemetry::metrics::UpDownCounter<long>> counter{
new opentelemetry::metrics::NoopUpDownCounter<long>("test", "none", "unitless")};
std::shared_ptr<opentelemetry::metrics::UpDownCounter<int64_t>> counter{
new opentelemetry::metrics::NoopUpDownCounter<int64_t>("test", "none", "unitless")};

std::map<std::string, std::string> labels = {{"k1", "v1"}};
counter->Add(10l, labels);
counter->Add(10l, labels, opentelemetry::context::Context{});
counter->Add(2l);
counter->Add(2l, opentelemetry::context::Context{});
counter->Add(10l, {{"k1", "1"}, {"k2", 2}});
counter->Add(10l, {{"k1", "1"}, {"k2", 2}}, opentelemetry::context::Context{});
counter->Add((int64_t)10, labels);
counter->Add((int64_t)10, labels, opentelemetry::context::Context{});
counter->Add((int64_t)2);
counter->Add((int64_t)2, opentelemetry::context::Context{});
counter->Add((int64_t)10, {{"k1", "1"}, {"k2", 2}});
counter->Add((int64_t)10, {{"k1", "1"}, {"k2", 2}}, opentelemetry::context::Context{});
}

#endif
20 changes: 10 additions & 10 deletions exporters/ostream/src/metric_exporter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -164,9 +164,9 @@ void OStreamMetricExporter::printPointData(const opentelemetry::sdk::metrics::Po
{
sout_ << nostd::get<double>(sum_point_data.value_);
}
else if (nostd::holds_alternative<long>(sum_point_data.value_))
else if (nostd::holds_alternative<int64_t>(sum_point_data.value_))
{
sout_ << nostd::get<long>(sum_point_data.value_);
sout_ << nostd::get<int64_t>(sum_point_data.value_);
}
}
else if (nostd::holds_alternative<sdk::metrics::HistogramPointData>(point_data))
Expand All @@ -179,24 +179,24 @@ void OStreamMetricExporter::printPointData(const opentelemetry::sdk::metrics::Po
{
sout_ << nostd::get<double>(histogram_point_data.sum_);
}
else if (nostd::holds_alternative<long>(histogram_point_data.sum_))
else if (nostd::holds_alternative<int64_t>(histogram_point_data.sum_))
{
sout_ << nostd::get<long>(histogram_point_data.sum_);
sout_ << nostd::get<int64_t>(histogram_point_data.sum_);
}

if (histogram_point_data.record_min_max_)
{
if (nostd::holds_alternative<long>(histogram_point_data.min_))
if (nostd::holds_alternative<int64_t>(histogram_point_data.min_))
{
sout_ << "\n min : " << nostd::get<long>(histogram_point_data.min_);
sout_ << "\n min : " << nostd::get<int64_t>(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_))
if (nostd::holds_alternative<int64_t>(histogram_point_data.max_))
{
sout_ << "\n max : " << nostd::get<long>(histogram_point_data.max_);
sout_ << "\n max : " << nostd::get<int64_t>(histogram_point_data.max_);
}
if (nostd::holds_alternative<double>(histogram_point_data.max_))
{
Expand All @@ -222,9 +222,9 @@ void OStreamMetricExporter::printPointData(const opentelemetry::sdk::metrics::Po
{
sout_ << nostd::get<double>(last_point_data.value_);
}
else if (nostd::holds_alternative<long>(last_point_data.value_))
else if (nostd::holds_alternative<int64_t>(last_point_data.value_))
{
sout_ << nostd::get<long>(last_point_data.value_);
sout_ << nostd::get<int64_t>(last_point_data.value_);
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions exporters/ostream/test/ostream_metric_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ TEST(OStreamMetricsExporter, ExportHistogramPointData)
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;
histogram_point_data2.sum_ = (int64_t)900;
metric_sdk::ResourceMetrics data;
auto resource = opentelemetry::sdk::resource::Resource::Create(
opentelemetry::sdk::resource::ResourceAttributes{});
Expand Down Expand Up @@ -190,7 +190,7 @@ TEST(OStreamMetricsExporter, ExportLastValuePointData)
last_value_point_data.is_lastvalue_valid_ = true;
last_value_point_data.sample_ts_ = opentelemetry::common::SystemTimestamp{};
metric_sdk::LastValuePointData last_value_point_data2{};
last_value_point_data2.value_ = 20l;
last_value_point_data2.value_ = (int64_t)20;
last_value_point_data2.is_lastvalue_valid_ = true;
last_value_point_data2.sample_ts_ = opentelemetry::common::SystemTimestamp{};
metric_sdk::MetricData metric_data{
Expand Down
20 changes: 10 additions & 10 deletions exporters/otlp/src/otlp_metric_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ void OtlpMetricUtils::ConvertSumMetric(const metric_sdk::MetricData &metric_data
proto_sum_point_data->set_time_unix_nano(ts);
auto sum_data = nostd::get<sdk::metrics::SumPointData>(point_data_with_attributes.point_data);

if ((nostd::holds_alternative<long>(sum_data.value_)))
if ((nostd::holds_alternative<int64_t>(sum_data.value_)))
{
proto_sum_point_data->set_as_int(nostd::get<long>(sum_data.value_));
proto_sum_point_data->set_as_int(nostd::get<int64_t>(sum_data.value_));
}
else
{
Expand Down Expand Up @@ -96,9 +96,9 @@ void OtlpMetricUtils::ConvertHistogramMetric(
auto histogram_data =
nostd::get<sdk::metrics::HistogramPointData>(point_data_with_attributes.point_data);
// sum
if ((nostd::holds_alternative<long>(histogram_data.sum_)))
if ((nostd::holds_alternative<int64_t>(histogram_data.sum_)))
{
proto_histogram_point_data->set_sum(nostd::get<long>(histogram_data.sum_));
proto_histogram_point_data->set_sum(nostd::get<int64_t>(histogram_data.sum_));
}
else
{
Expand All @@ -108,17 +108,17 @@ void OtlpMetricUtils::ConvertHistogramMetric(
proto_histogram_point_data->set_count(histogram_data.count_);
if (histogram_data.record_min_max_)
{
if (nostd::holds_alternative<long>(histogram_data.min_))
if (nostd::holds_alternative<int64_t>(histogram_data.min_))
{
proto_histogram_point_data->set_min(nostd::get<long>(histogram_data.min_));
proto_histogram_point_data->set_min(nostd::get<int64_t>(histogram_data.min_));
}
else
{
proto_histogram_point_data->set_min(nostd::get<double>(histogram_data.min_));
}
if (nostd::holds_alternative<long>(histogram_data.max_))
if (nostd::holds_alternative<int64_t>(histogram_data.max_))
{
proto_histogram_point_data->set_min(nostd::get<long>(histogram_data.max_));
proto_histogram_point_data->set_min(nostd::get<int64_t>(histogram_data.max_));
}
else
{
Expand Down Expand Up @@ -158,9 +158,9 @@ void OtlpMetricUtils::ConvertGaugeMetric(const opentelemetry::sdk::metrics::Metr
auto gauge_data =
nostd::get<sdk::metrics::LastValuePointData>(point_data_with_attributes.point_data);

if ((nostd::holds_alternative<long>(gauge_data.value_)))
if ((nostd::holds_alternative<int64_t>(gauge_data.value_)))
{
proto_gauge_point_data->set_as_int(nostd::get<long>(gauge_data.value_));
proto_gauge_point_data->set_as_int(nostd::get<int64_t>(gauge_data.value_));
}
else
{
Expand Down
8 changes: 4 additions & 4 deletions exporters/otlp/test/otlp_http_metric_exporter_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ class OtlpHttpMetricExporterTestPeer : public ::testing::Test
last_value_point_data.is_lastvalue_valid_ = true;
last_value_point_data.sample_ts_ = opentelemetry::common::SystemTimestamp{};
opentelemetry::sdk::metrics::LastValuePointData last_value_point_data2{};
last_value_point_data2.value_ = 20l;
last_value_point_data2.value_ = (int64_t)20;
last_value_point_data2.is_lastvalue_valid_ = true;
last_value_point_data2.sample_ts_ = opentelemetry::common::SystemTimestamp{};
opentelemetry::sdk::metrics::MetricData metric_data{
Expand Down Expand Up @@ -392,7 +392,7 @@ class OtlpHttpMetricExporterTestPeer : public ::testing::Test
last_value_point_data.is_lastvalue_valid_ = true;
last_value_point_data.sample_ts_ = opentelemetry::common::SystemTimestamp{};
opentelemetry::sdk::metrics::LastValuePointData last_value_point_data2{};
last_value_point_data2.value_ = 20l;
last_value_point_data2.value_ = (int64_t)20;
last_value_point_data2.is_lastvalue_valid_ = true;
last_value_point_data2.sample_ts_ = opentelemetry::common::SystemTimestamp{};
opentelemetry::sdk::metrics::MetricData metric_data{
Expand Down Expand Up @@ -492,7 +492,7 @@ class OtlpHttpMetricExporterTestPeer : public ::testing::Test
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;
histogram_point_data2.sum_ = (int64_t)900;

opentelemetry::sdk::metrics::MetricData metric_data{
opentelemetry::sdk::metrics::InstrumentDescriptor{
Expand Down Expand Up @@ -627,7 +627,7 @@ class OtlpHttpMetricExporterTestPeer : public ::testing::Test
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;
histogram_point_data2.sum_ = (int64_t)900;

opentelemetry::sdk::metrics::MetricData metric_data{
opentelemetry::sdk::metrics::InstrumentDescriptor{
Expand Down
6 changes: 3 additions & 3 deletions exporters/prometheus/src/exporter_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ std::vector<prometheus_client::MetricFamily> PrometheusExporterUtils::TranslateT
}
else
{
sum = nostd::get<long>(histogram_point_data.sum_);
sum = nostd::get<int64_t>(histogram_point_data.sum_);
}
SetData(std::vector<double>{sum, (double)histogram_point_data.count_}, boundaries,
counts, point_data_attr.attributes, time, &metric_family);
Expand Down Expand Up @@ -283,9 +283,9 @@ void PrometheusExporterUtils::SetValue(std::vector<T> values,
{
double value = 0.0;
const auto &value_var = values[0];
if (nostd::holds_alternative<long>(value_var))
if (nostd::holds_alternative<int64_t>(value_var))
{
value = nostd::get<long>(value_var);
value = nostd::get<int64_t>(value_var);
}
else
{
Expand Down
4 changes: 2 additions & 2 deletions exporters/prometheus/test/prometheus_test_helper.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ inline metric_sdk::ResourceMetrics CreateHistogramPointData()
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;
histogram_point_data2.sum_ = (int64_t)900;
metric_sdk::ResourceMetrics data;
auto resource = opentelemetry::sdk::resource::Resource::Create(
opentelemetry::sdk::resource::ResourceAttributes{});
Expand Down Expand Up @@ -90,7 +90,7 @@ inline metric_sdk::ResourceMetrics CreateLastValuePointData()
last_value_point_data.is_lastvalue_valid_ = true;
last_value_point_data.sample_ts_ = opentelemetry::common::SystemTimestamp{};
metric_sdk::LastValuePointData last_value_point_data2{};
last_value_point_data2.value_ = 20l;
last_value_point_data2.value_ = (int64_t)20;
last_value_point_data2.is_lastvalue_valid_ = true;
last_value_point_data2.sample_ts_ = opentelemetry::common::SystemTimestamp{};
metric_sdk::MetricData metric_data{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ namespace metrics
class Aggregation
{
public:
virtual void Aggregate(long value, const PointAttributes &attributes = {}) noexcept = 0;
virtual void Aggregate(int64_t value, const PointAttributes &attributes = {}) noexcept = 0;

virtual void Aggregate(double value, const PointAttributes &attributes = {}) noexcept = 0;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,8 @@ class DefaultAggregation
case InstrumentType::kHistogram: {
return (instrument_descriptor.value_type_ == InstrumentValueType::kLong)
? std::move(std::unique_ptr<Aggregation>(new LongHistogramAggregation(
static_cast<
const opentelemetry::sdk::metrics::HistogramAggregationConfig<long> *>(
aggregation_config))))
static_cast<const opentelemetry::sdk::metrics::HistogramAggregationConfig<
int64_t> *>(aggregation_config))))
Copy link
Member

Choose a reason for hiding this comment

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

Nit - this need to be double after #1665 fix. Just adding, though it would be fixed once conflict is resolved.

: std::move(std::unique_ptr<Aggregation>(new DoubleHistogramAggregation(
static_cast<const opentelemetry::sdk::metrics::HistogramAggregationConfig<
double> *>(aggregation_config))));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class DropAggregation : public Aggregation

DropAggregation(const DropPointData &) {}

void Aggregate(long /* value */, const PointAttributes & /* attributes */) noexcept override {}
void Aggregate(int64_t /* value */, const PointAttributes & /* attributes */) noexcept override {}

void Aggregate(double /* value */, const PointAttributes & /* attributes */) noexcept override {}

Expand Down
Loading