-
Notifications
You must be signed in to change notification settings - Fork 446
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
Changes from 7 commits
215a043
e3283a8
222f6af
f6654ac
4f6049c
ae642d9
1f467f6
f5237e3
d42179b
1e1ee23
0eb1465
36b80e4
10ae974
2c0189f
5674bf7
3b84aa8
ff1229d
40f1fb8
23ea59c
22cac63
5ea4784
e5c2ea6
b7c50ef
7bf9130
cf7f873
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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( | ||
nostd::string_view name, | ||
nostd::string_view description = "", | ||
nostd::string_view unit = "") noexcept = 0; | ||
|
@@ -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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Possibly rename to CreateInt64UpDownCounter, to discuss There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. uint64_t, as counter is always non-negative value. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks, changed to |
||
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)))); | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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).