Add Aggregation as part of metrics SDK. #1178

lalitb · 2022-01-21T00:44:15Z

Changes

Splitting #1173 into smaller PRs for ease of review. This PR adds aggregation for Sum, LastValue, and Histogram.

Implementation is based on the specs here:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#aggregation

Please provide a brief description of the changes here.

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-01-21T00:51:11Z

Codecov Report

Merging #1178 (80a710e) into main (b6a28df) will increase coverage by 0.10%.
The diff coverage is 95.70%.

@@            Coverage Diff             @@
##             main    #1178      +/-   ##
==========================================
+ Coverage   93.35%   93.44%   +0.10%     
==========================================
  Files         177      185       +8     
  Lines        6502     6720     +218     
==========================================
+ Hits         6069     6279     +210     
- Misses        433      441       +8

Impacted Files	Coverage Δ
...ry/sdk/metrics/aggregation/lastvalue_aggregation.h	`0.00% <0.00%> (ø)`
sdk/include/opentelemetry/sdk/metrics/view/view.h	`63.64% <ø> (ø)`
...lemetry/sdk/metrics/aggregation/drop_aggregation.h	`25.00% <25.00%> (ø)`
...ry/sdk/metrics/aggregation/histogram_aggregation.h	`75.00% <75.00%> (ø)`
...elemetry/sdk/metrics/aggregation/sum_aggregation.h	`75.00% <75.00%> (ø)`
...pentelemetry/sdk/metrics/aggregation/aggregation.h	`100.00% <100.00%> (ø)`
...k/src/metrics/aggregation/histogram_aggregation.cc	`100.00% <100.00%> (ø)`
...k/src/metrics/aggregation/lastvalue_aggregation.cc	`100.00% <100.00%> (ø)`
sdk/src/metrics/aggregation/sum_aggregation.cc	`100.00% <100.00%> (ø)`
sdk/test/metrics/aggregation_test.cc	`100.00% <100.00%> (ø)`
... and 2 more

lalitb · 2022-01-21T00:57:36Z

sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation.h

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


Preferred overloaded methods over the template ( for double and long type) to avoid having header-only metrics sdk. The template use had a cascading effect here, forcing to make all inter-related classes as templates. Since this is not a customer-facing API, we have the flexibility to change it later.

…pp into aggregation-sdk-3

esigo · 2022-01-21T09:41:52Z

sdk/test/metrics/BUILD

+    name = "view_registry_test",
+    srcs = [
+        "view_registry_test.cc",
+    ],


Would it be possible to add tags here?

Suggested change

],

],

tags = [

"metrics",

"test",

],

Thanks. Added it.

esigo · 2022-01-21T09:42:12Z

sdk/test/metrics/BUILD

+    name = "aggregation_test",
+    srcs = [
+        "aggregation_test.cc",
+    ],


Suggested change

],

],

tags = [

"metrics",

"test",

],

esigo · 2022-01-21T20:05:24Z

sdk/src/metrics/aggregation/lastvalue_aggregation.cc

+
+PointType LongLastValueAggregation::Collect() noexcept
+{
+  if (!is_lastvalue_valid_)


Don't we need to protect is_lastvalue_valid_ here?

Good point. I thought it should be fine to return an invalid value in given Collect() call, as long as the subsequent call would return a valid value. Worst case, we may miss a given metric (first few ones depending on collect interval) in a series of Collect() calls, but this way we can avoid a lock.

I'm afraid this would be a UB, and the lock for reading is unavoidable.

I thought UB should be fine as long as is_lastvalue_valid_ returns either true/false (was not expecting strong consistency here). As in one of the next few iterations, it would be true eventually. But have added the lock if it's causing confusion.

esigo · 2022-01-22T10:47:42Z

sdk/test/metrics/aggregation_test.cc

+  ASSERT_TRUE(nostd::holds_alternative<std::vector<long>>(histogram_data.boundaries_));
+  EXPECT_EQ(nostd::get<long>(histogram_data.sum_), 0);
+  EXPECT_EQ(histogram_data.count_, 0);
+  EXPECT_NO_THROW(aggr.Aggregate(12l, {}));   // lies in third bucket (indexed at 0)


The comments are not correct. also in DoubleHistogramAggregation

Do you mean 12 does lie in 3rd bucket? The default buckets here are: [-infinity to 0], [0 to 5], [5 to10], [10 to 25] , and so on (refer - https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#histogram-aggregation ). So 12 will lie in the third bucket.

No I mean indexed at 0 part, this should be 2 or I'm getting sth wrong here?

Yes, have only kept the bucket number now. Thanks for noticing it.

esigo

Thanks for the PR. LGTM

ThomsonTan · 2022-01-26T18:38:23Z

sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation.h

+{
+namespace metrics
+{
+class InstrumentMonotonicityAwareAggregation


Seems InstrumentMonotonicityAwareAggregation should be a subclass of Aggretation?

Yes or else we can also move this functionality within the Aggregation class. I thought of keeping it a separate class as not sure right now how to use it for Asynchronous aggregation. We can refactor this later once we have more complete implementation.

lalitb added 3 commits January 20, 2022 16:32

Aggregation SDK

20b6614

Aggregation SDK src

3bcf3d9

remove old aggregation header

e8cd8d3

lalitb requested a review from a team January 21, 2022 00:44

Merge branch 'main' into aggregation-sdk-3

0a74c97

lalitb commented Jan 21, 2022

View reviewed changes

lalitb added 2 commits January 20, 2022 18:07

remove unwanted code

69278f7

Merge branch 'aggregation-sdk-3' of github.com:lalitb/opentelemetry-c…

8f1e974

…pp into aggregation-sdk-3

esigo reviewed Jan 21, 2022

View reviewed changes

esigo reviewed Jan 22, 2022

View reviewed changes

lalitb added 2 commits January 22, 2022 11:22

review comments

7b8ba06

review comment

2e8cbda

esigo approved these changes Jan 23, 2022

View reviewed changes

This was referenced Jan 25, 2022

Add @esigo as approver lalitb/opentelemetry-cpp#80

Closed

Add @esigo as approver #1183

Merged

Merge branch 'main' into aggregation-sdk-3

b0fb78d

esigo self-requested a review January 26, 2022 18:07

esigo approved these changes Jan 26, 2022

View reviewed changes

ThomsonTan reviewed Jan 26, 2022

View reviewed changes

lalitb mentioned this pull request Jan 28, 2022

Sync and Async Instruments SDK #1184

Merged

3 tasks

lalitb and others added 3 commits February 1, 2022 08:49

Merge branch 'main' into aggregation-sdk-3

d63b1e3

Merge branch 'main' into aggregation-sdk-3

e08db54

fix format after merge

80a710e

lalitb merged commit 44795b6 into open-telemetry:main Feb 5, 2022

lalitb mentioned this pull request Mar 28, 2022

Synchronous Metric collection (Delta , Cumulative) #1265

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Aggregation as part of metrics SDK. #1178

Add Aggregation as part of metrics SDK. #1178

lalitb commented Jan 21, 2022 •

edited

Loading

codecov bot commented Jan 21, 2022 •

edited

Loading

lalitb Jan 21, 2022 •

edited

Loading

esigo Jan 21, 2022

lalitb Jan 22, 2022

esigo Jan 21, 2022

lalitb Jan 22, 2022

esigo Jan 21, 2022

lalitb Jan 21, 2022 •

edited

Loading

esigo Jan 22, 2022

lalitb Jan 22, 2022

esigo Jan 22, 2022

lalitb Jan 22, 2022

esigo Jan 22, 2022

lalitb Jan 23, 2022

esigo left a comment

ThomsonTan Jan 26, 2022

lalitb Jan 27, 2022

-    ],
+    ],
+    tags = [
+        "metrics",
+        "test",
+    ],

Add Aggregation as part of metrics SDK. #1178

Add Aggregation as part of metrics SDK. #1178

Conversation

lalitb commented Jan 21, 2022 • edited Loading

Changes

codecov bot commented Jan 21, 2022 • edited Loading

Codecov Report

lalitb Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalitb Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esigo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalitb commented Jan 21, 2022 •

edited

Loading

codecov bot commented Jan 21, 2022 •

edited

Loading

lalitb Jan 21, 2022 •

edited

Loading

lalitb Jan 21, 2022 •

edited

Loading