[Metrics] Switch to explicit 64 bit integers by esigo · Pull Request #1686 · open-telemetry/opentelemetry-cpp

esigo · 2022-10-15T10:42:37Z

Fixes #1639 (issue)

Changes

"Long" instruments changed to accept int64_t integer values, which are unambiguously 64 bit depth.

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

codecov · 2022-10-15T10:55:23Z

Codecov Report

Merging #1686 (cf7f873) into main (5258563) will increase coverage by 0.04%.
The diff coverage is 71.70%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1686      +/-   ##
==========================================
+ Coverage   86.31%   86.35%   +0.04%     
==========================================
  Files         169      169              
  Lines        5156     5154       -2     
==========================================
  Hits         4450     4450              
+ Misses        706      704       -2

Impacted Files	Coverage Δ
api/include/opentelemetry/metrics/meter.h	`100.00% <ø> (ø)`
api/include/opentelemetry/metrics/noop.h	`40.75% <0.00%> (ø)`
...pi/include/opentelemetry/metrics/observer_result.h	`100.00% <ø> (ø)`
...pentelemetry/sdk/metrics/aggregation/aggregation.h	`100.00% <ø> (ø)`
...ry/sdk/metrics/aggregation/histogram_aggregation.h	`90.00% <0.00%> (ø)`
...ry/sdk/metrics/aggregation/lastvalue_aggregation.h	`0.00% <0.00%> (ø)`
...elemetry/sdk/metrics/aggregation/sum_aggregation.h	`0.00% <0.00%> (ø)`
...nclude/opentelemetry/sdk/metrics/data/point_data.h	`100.00% <ø> (ø)`
...emetry/sdk/metrics/exemplar/always_sample_filter.h	`100.00% <ø> (ø)`
...nclude/opentelemetry/sdk/metrics/exemplar/filter.h	`100.00% <ø> (ø)`
... and 27 more

marcalff

LGTM in general, see minor comments.

marcalff · 2022-10-17T20:01:05Z

   */

-  virtual nostd::shared_ptr<Counter<long>> CreateLongCounter(
+  virtual nostd::shared_ptr<Counter<int64_t>> CreateLongCounter(


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

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.

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

lalitb · 2022-10-17T22:10:45Z

-                             const opentelemetry::sdk::metrics::HistogramAggregationConfig<long> *>(
-                             aggregation_config))))
+                         static_cast<const opentelemetry::sdk::metrics::HistogramAggregationConfig<
+                             int64_t> *>(aggregation_config))))


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

lalitb · 2022-10-17T22:21:58Z

   */

-  virtual nostd::shared_ptr<Counter<long>> CreateLongCounter(
+  virtual nostd::shared_ptr<Counter<int64_t>> CreateLongCounter(


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.

marcalff

+1 on replacing "long" with a a 64bit int.

Given the impact on all the method names, and questions about signed vs unsigned,
I suggest we all agree first on final names and types,
to avoid changing the patch many times, it is tedious enough.

marcalff · 2022-10-18T19:56:28Z

   */

-  virtual nostd::shared_ptr<Counter<long>> CreateLongCounter(
+  virtual nostd::shared_ptr<Counter<int64_t>> CreateInt64Counter(


In the spec
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#meter

the spec suggests UInt64 for counter and histogram (unsigned 64 bit), and Int64 (signed) for up down counters.

Not sure if we should have all "long" metrics use Int64, or have UInt64 / Int64 depending on the type of counter.

Possibly rename to CreateUInt64Counter, and use uint64_t, to discuss

Yeah good point. There are two options

Use Int64 in method name for synchronous UpDownCounter, and UInt64 for all other instruments. And use uint64_t/int64_t as appropriate accordingly for measurement value. Also inline with the specs recommendation.

Use Int64 in method name for all the instruments, and int64_t as measurement value, and do extra validation in the method whether passed value is positive or negative. So if negative value is passed for Counter instrument, drop the value.

First option looks better to me. Irrespective of what we choose, the aggregation will always use int64_t type for the aggregated value.

First option sounds better, with strong types, and inlined with specs.
So, leaning to:

CreateUInt64Counter, using uint64_t

CreateUInt64Histogram, using uint64_t

CreateInt64UpDownCounter, using int64_t

CreateUInt64ObservableCounter

CreateUInt64ObservableGauge

CreateInt64ObservableUpDownCounter

Let's discuss in SIG meeting.

Thanks, as discussed I renamed the functions.

marcalff · 2022-10-18T19:58:21Z

   * @return a shared pointer to the created Histogram.
   */
-  virtual nostd::shared_ptr<Histogram<long>> CreateLongHistogram(
+  virtual nostd::shared_ptr<Histogram<int64_t>> CreateLongHistogram(


Possibly rename to CreateUInt64Histogram, and use uint64_t, to discuss

marcalff · 2022-10-18T19:58:52Z

   * @return a shared pointer to the created UpDownCounter.
   */
-  virtual nostd::shared_ptr<UpDownCounter<long>> CreateLongUpDownCounter(
+  virtual nostd::shared_ptr<UpDownCounter<int64_t>> CreateLongUpDownCounter(


Possibly rename to CreateInt64UpDownCounter, to discuss

Thanks, renamed all the functions as suggested here #1686 (comment)

lalitb · 2022-10-20T23:53:25Z

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


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

thanks, changed to unit64_t.

lalitb · 2022-10-21T00:00:32Z

 }

-static metrics_sdk::MetricData CreateObservableGaugeAggregationData()
+static metrics_sdk::MetricData CreateUInt64ObservableGaugeAggregationData()


this method is creating double type values, rename as double or keep as earlier.

thanks, fixed

lalitb · 2022-10-21T04:53:35Z

   * @param unit the unit of metric values following https://unitsofmeasure.org/ucum.html.
   */
-  virtual nostd::shared_ptr<ObservableInstrument> CreateLongObservableGauge(
+  virtual nostd::shared_ptr<ObservableInstrument> CreateUInt64ObservableGauge(


It's not explicit in specs that the recorded gauge values should be only positive. Which means it can be both positive and negative. Also, as an example - recorded temperature can be negative.
Sorry for creating confusion, I should have seen it earlier. I think we should change it to CreateInt64ObservableGauge

thanks, fixed

lalitb · 2022-10-21T05:00:09Z

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


uint64_t, as counter is always non-negative value.

thanks, fixed

lalitb · 2022-10-21T05:03:38Z

 TEST(OtlpMetricSerializationTest, ObservableGauge)
 {
-  metrics_sdk::MetricData data = CreateObservableGaugeAggregationData();
+  metrics_sdk::MetricData data = CreateUInt64ObservableGaugeAggregationData();


this method is creating double type values, rename as double or keep as earlier.

thanks, fixed

lalitb · 2022-10-21T05:44:19Z

      nostd::string_view unit        = "") noexcept override;

-  nostd::shared_ptr<opentelemetry::metrics::ObservableInstrument> CreateLongObservableGauge(
+  nostd::shared_ptr<opentelemetry::metrics::ObservableInstrument> CreateUInt64ObservableGauge(


As earlier comment, this can be changed to CreateInt64ObservableGauge

thanks, fixed

lalitb · 2022-10-21T05:49:14Z

 // Copyright The OpenTelemetry Authors
 // SPDX-License-Identifier: Apache-2.0

+#include <cstdint>


nit - this should be after ENABLE_METRICS_PREVIEW

thanks, fixed

lalitb · 2022-10-21T05:50:04Z

 }

-nostd::shared_ptr<opentelemetry::metrics::ObservableInstrument> Meter::CreateLongObservableGauge(
+nostd::shared_ptr<opentelemetry::metrics::ObservableInstrument> Meter::CreateUInt64ObservableGauge(


As earlier comment on api, this should be CreateInt64ObservableGauge

thanks, fixed

lalitb · 2022-10-21T05:52:22Z

  // using various numeric types.
  ExactAggregator<int> agg_int(metrics_api::InstrumentKind::Counter);
-  ExactAggregator<long> agg_long(metrics_api::InstrumentKind::Counter);
+  ExactAggregator<int64_t> agg_long(metrics_api::InstrumentKind::Counter);


These are for old metrics implementation :)

Thanks, reverted old metrics 😄

lalitb · 2022-10-22T01:14:19Z

   * @param unit the unit of metric values following https://unitsofmeasure.org/ucum.html.
   */
-  virtual nostd::shared_ptr<ObservableInstrument> CreateLongObservableCounter(
+  virtual nostd::shared_ptr<ObservableInstrument> CreateUInt64ObservableCounter(


Hate to do this, but Observable Counter can be negative also, so this should be CreateInt64ObservableCounter :(. I confirmed offline with specs author. Sorry about multiple iterations here, but this is important to do it right before GA.

thanks, yes it's important we leave no changes needed after GA :)
changed to CreateInt64ObservableCounter

lalitb

LGTM. Thanks for working on it :)

marcalff

LGTM, thanks for the fix.

…ad-local-stack * commit '9acde87b08b225ce511fa8a20c6cba14f2921518': (36 commits) Prepare v1.7.0 release with Metrics API/SDK GA. (open-telemetry#1721) Fix: 1712 - Validate Instrument meta data (name, unit, description) (open-telemetry#1713) Document libthrift 0.12.0 doesn't work with Jaeger exporter (open-telemetry#1714) Fix:1674, Add Monotonic Property to Sum Aggregation, and unit tests for Up Down Counter (open-telemetry#1675) [Metrics SDK] Move Metrics Exemplar processing behind feature flag (open-telemetry#1710) [Metrics API/SDK] Change Meter API/SDK to return nostd::unique_ptr for Sync Instruments (open-telemetry#1707) [Metrics] Switch to explicit 64 bit integers (open-telemetry#1686) Add metrics e2e test to asan & tsan CI (open-telemetry#1670) Add otlp-grpc example bazel (open-telemetry#1708) [Metrics SDK] Add support for Pull Metric Reader (open-telemetry#1701) Fix debug log of OTLP HTTP exporter and ES log exporter (open-telemetry#1703) [SEMANTIC CONVENTIONS] Upgrade to version 1.14.0 (open-telemetry#1697) Fix a potential precision loss on integer in ReservoirCellIndexFor (open-telemetry#1696) fix Histogram crash (open-telemetry#1685) Fix:1676 Segfault when short export period is used for metrics (open-telemetry#1682) Add timeout support to MeterContext::ForceFlush (open-telemetry#1673) Add CMake OTELCPP_MAINTAINER_MODE (open-telemetry#1650) [DOCS] - Minor updates to OStream Metrics exporter documentation (open-telemetry#1679) Fix:open-telemetry#1575 API Documentation for Metrics SDK and API (open-telemetry#1678) Fixes open-telemetry#249 (open-telemetry#1677) ...

Switch to explicit 64 bit integers

215a043

esigo and others added 6 commits October 15, 2022 11:01

fix

e3283a8

fmt

222f6af

fix Windows

f6654ac

Merge branch 'main' into integers-1639

4f6049c

fix

ae642d9

fix

1f467f6

esigo changed the title ~~Switch to explicit 64 bit integers~~ [Metrics] Switch to explicit 64 bit integers Oct 15, 2022

esigo marked this pull request as ready for review October 15, 2022 14:56

esigo requested a review from a team October 15, 2022 14:56

esigo added the metrics label Oct 15, 2022

marcalff reviewed Oct 17, 2022

View reviewed changes

lalitb reviewed Oct 17, 2022

View reviewed changes

lalitb mentioned this pull request Oct 18, 2022

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

Merged

3 tasks

esigo added 3 commits October 18, 2022 16:43

review comments

f5237e3

Merge branch 'main' into integers-1639

d42179b

fix

1e1ee23

marcalff reviewed Oct 18, 2022

View reviewed changes

esigo added 5 commits October 20, 2022 17:12

string types

0eb1465

fix

36b80e4

fix

10ae974

fix

2c0189f

fix

5674bf7

ThomsonTan reviewed Oct 20, 2022

View reviewed changes

Comment thread api/include/opentelemetry/metrics/meter.h

Merge branch 'main' into integers-1639

3b84aa8

lalitb reviewed Oct 20, 2022

View reviewed changes

Comment thread api/include/opentelemetry/metrics/meter.h

lalitb reviewed Oct 20, 2022

View reviewed changes

Comment thread api/include/opentelemetry/metrics/observer_result.h

lalitb reviewed Oct 21, 2022

View reviewed changes

esigo and others added 6 commits October 21, 2022 18:07

comments

ff1229d

revert legacy metrics

40f1fb8

gix CI

23ea59c

Merge branch 'main' into integers-1639

22cac63

Merge branch 'main' into integers-1639

5ea4784

fmt

e5c2ea6

lalitb reviewed Oct 22, 2022

View reviewed changes

esigo and others added 3 commits October 22, 2022 07:27

CreateInt64ObservableCounter

b7c50ef

Merge branch 'main' into integers-1639

7bf9130

Merge branch 'main' into integers-1639

cf7f873

esigo mentioned this pull request Oct 24, 2022

[Metrics API/SDK] Change Meter API/SDK to return nostd::unique_ptr for Sync Instruments #1707

Merged

3 tasks

lalitb approved these changes Oct 24, 2022

View reviewed changes

marcalff approved these changes Oct 25, 2022

View reviewed changes

esigo merged commit 7fd76ee into open-telemetry:main Oct 25, 2022

esigo deleted the integers-1639 branch October 25, 2022 14:57

esigo mentioned this pull request Oct 28, 2022

Prepare v1.7.0 release with Metrics API/SDK GA. #1721

Merged

yxue pushed a commit to yxue/opentelemetry-cpp that referenced this pull request Dec 5, 2022

[Metrics] Switch to explicit 64 bit integers (open-telemetry#1686)

16c5d00

yxue pushed a commit to yxue/opentelemetry-cpp that referenced this pull request Dec 5, 2022

[Metrics] Switch to explicit 64 bit integers (open-telemetry#1686)

b7ce332

ThomsonTan mentioned this pull request Dec 6, 2023

Update for the ValueType change in the main repo open-telemetry/opentelemetry-cpp-contrib#347

Merged

Conversation

esigo commented Oct 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

codecov Bot commented Oct 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

marcalff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lalitb Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esigo Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lalitb Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcalff left a comment

Choose a reason for hiding this comment

Uh oh!

marcalff Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lalitb Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcalff Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lalitb Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

esigo commented Oct 15, 2022 •

edited

Loading

codecov Bot commented Oct 15, 2022 •

edited

Loading

lalitb Oct 17, 2022 •

edited

Loading

esigo Oct 20, 2022 •

edited

Loading

lalitb Oct 17, 2022 •

edited

Loading

marcalff Oct 18, 2022 •

edited

Loading

lalitb Oct 18, 2022 •

edited

Loading

marcalff Oct 18, 2022 •

edited

Loading

lalitb Oct 20, 2022 •

edited

Loading