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

lalitb · 2022-10-12T02:43:28Z

Fixes #1674

Changes

Add monotonic properly for Sum Aggregation
Select correct metric family for UpDownCounter in prometheus exporter
Add unit test for UpDownCounter.

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-12T02:55:15Z

Codecov Report

Merging #1675 (6ff7036) into main (419e2d6) will decrease coverage by 0.12%.
The diff coverage is 82.76%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1675      +/-   ##
==========================================
- Coverage   86.38%   86.26%   -0.11%     
==========================================
  Files         169      169              
  Lines        5146     5160      +14     
==========================================
+ Hits         4445     4451       +6     
- Misses        701      709       +8

Impacted Files	Coverage Δ
...elemetry/sdk/metrics/aggregation/sum_aggregation.h	`0.00% <ø> (ø)`
...nclude/opentelemetry/sdk/metrics/data/point_data.h	`100.00% <ø> (ø)`
sdk/src/metrics/aggregation/sum_aggregation.cc	`75.81% <68.75%> (-4.55%)`	⬇️
...etry/sdk/metrics/aggregation/default_aggregation.h	`78.69% <100.00%> (+3.22%)`	⬆️
ext/src/http/client/curl/http_client_curl.cc	`80.31% <0.00%> (-1.13%)`	⬇️
sdk/src/trace/batch_span_processor.cc	`90.63% <0.00%> (-0.78%)`	⬇️

ThomsonTan · 2022-10-18T00:28:54Z

sdk/src/metrics/aggregation/sum_aggregation.cc

+    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<long>(point_data_.value_) + value;


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

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.

… into up-down-counter

lalitb · 2022-10-20T23:18:04Z

Have fixed the CI errors. This should be good to review now.

esigo

LGTM
just had a few minor comments
Thanks for the PR :)

exporters/prometheus/src/exporter_utils.cc

sdk/test/metrics/sync_metric_storage_up_down_counter_test.cc

marcalff

LGTM

marcalff · 2022-10-25T07:45:46Z

sdk/src/metrics/aggregation/sum_aggregation.cc

+  if (point_data_.is_monotonic_ && value < 0)
+  {
+    OTEL_INTERNAL_LOG_WARN(" Negative value ignored for Monotonic increasing measurement. Value"
+                           << value);


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.

Thanks. added slightly more context here.

ThomsonTan · 2022-10-25T19:29:50Z

sdk/test/metrics/sync_metric_storage_up_down_counter_test.cc

+
+using namespace opentelemetry::sdk::metrics;
+using namespace opentelemetry::common;
+using M         = std::map<std::string, std::string>;


Is using M as namespace alias from some convention?

No, I don't think there is any convention. What do you recommend ?

ThomsonTan · 2022-10-25T19:31:30Z

sdk/test/metrics/sync_metric_storage_up_down_counter_test.cc

+      instr_desc, AggregationType::kSum, default_attributes_processor.get(),
+      ExemplarReservoir::GetNoExemplarReservoir(), nullptr);
+
+  storage.RecordLong(10l, KeyValueIterableView<std::map<std::string, std::string>>(attributes_get),


define the number 10l as a variable as it is shared between this and the following line?

Thanks, done

…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) ...

…or Up Down Counter (open-telemetry#1675)

add monotonicity to sum aggregation

a109486

lalitb requested a review from a team October 12, 2022 02:43

esigo and others added 3 commits October 15, 2022 08:22

Merge branch 'main' into up-down-counter

f90ba7e

Merge branch 'main' into up-down-counter

bf4e7e7

Merge branch 'main' into up-down-counter

e880a3b

ThomsonTan reviewed Oct 18, 2022

View reviewed changes

lalitb added 4 commits October 20, 2022 13:53

Merge branch 'main' into up-down-counter

bba260c

Fix

e742b0f

Merge branch 'up-down-counter' of github.com:lalitb/opentelemetry-cpp…

18f4653

… into up-down-counter

fix

2ba30b8

Merge branch 'main' into up-down-counter

6285858

esigo added the metrics label Oct 21, 2022

Merge branch 'main' into up-down-counter

9c1ac08

esigo approved these changes Oct 22, 2022

View reviewed changes

exporters/prometheus/src/exporter_utils.cc Show resolved Hide resolved

sdk/test/metrics/sync_metric_storage_up_down_counter_test.cc Outdated Show resolved Hide resolved

sdk/test/metrics/sync_metric_storage_up_down_counter_test.cc Show resolved Hide resolved

esigo added 2 commits October 24, 2022 16:46

Merge branch 'main' into up-down-counter

2198b28

Merge branch 'main' into up-down-counter

240e5f7

marcalff approved these changes Oct 25, 2022

View reviewed changes

Merge branch 'main' into up-down-counter

f848d02

ThomsonTan reviewed Oct 25, 2022

View reviewed changes

ThomsonTan approved these changes Oct 25, 2022

View reviewed changes

lalitb added 5 commits October 25, 2022 13:43

fix

ee24e6c

fix

c2be9e8

fix

f70a8b1

fix

15d04d0

Merge branch 'main' into up-down-counter

6ff7036

lalitb merged commit f17043f into open-telemetry:main Oct 26, 2022

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

Fix:1674, Add Monotonic Property to Sum Aggregation, and unit tests f…

ea9d10c

…or Up Down Counter (open-telemetry#1675)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lalitb commented Oct 12, 2022

codecov bot commented Oct 12, 2022 •

edited

Loading

ThomsonTan Oct 18, 2022

lalitb Oct 18, 2022 •

edited

Loading

lalitb commented Oct 20, 2022

esigo left a comment

marcalff left a comment

marcalff Oct 25, 2022

lalitb Oct 25, 2022

ThomsonTan Oct 25, 2022

lalitb Oct 25, 2022

ThomsonTan Oct 25, 2022

lalitb Oct 25, 2022

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

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

Conversation

lalitb commented Oct 12, 2022

Changes

codecov bot commented Oct 12, 2022 • edited Loading

Codecov Report

ThomsonTan Oct 18, 2022

Choose a reason for hiding this comment

lalitb Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

lalitb commented Oct 20, 2022

esigo left a comment

Choose a reason for hiding this comment

marcalff left a comment

Choose a reason for hiding this comment

marcalff Oct 25, 2022

Choose a reason for hiding this comment

lalitb Oct 25, 2022

Choose a reason for hiding this comment

ThomsonTan Oct 25, 2022

Choose a reason for hiding this comment

lalitb Oct 25, 2022

Choose a reason for hiding this comment

ThomsonTan Oct 25, 2022

Choose a reason for hiding this comment

lalitb Oct 25, 2022

Choose a reason for hiding this comment

codecov bot commented Oct 12, 2022 •

edited

Loading

lalitb Oct 18, 2022 •

edited

Loading