Add Counter and Histogram Aggregators #178

ankit-bhargava · 2020-07-16T03:15:12Z

This PR builds out SDK support for the Counter and Histogram Aggregators which combine updates to metric instruments in meaningful values. Each aggregator stores exportable values in a vector format for ease of use further down the metrics data pipeline. All aggregators are derived from a base Aggregator abstract class which holds functions and data relevant to all aggregators. Additional data structures, such as a vector for histogram boundaries, are added on a case by case basis. Aggregators are dependent on the Metric Instruments API (pending approval in PR # 161) and will not compile without those files.

Also included are tests for each aggregator which validate the correctness of aggregation in both single-threaded and concurrent situations.

Note: We templated aggregators to support a wider array of scalar types. This templating prevents us from using out of line declarations as we normally would. As a result, we left all implementation code in the header file. If there are any suggestions to remedy this we will gladly make the change.

This PR is part of a series of PRs to add a full implementation of metrics API and SDK support.

reyang · 2020-07-18T06:00:42Z

sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h

+
+#include <variant>
+#include <vector>
+#include <mutex>


nit - is there a reason these are not alphabetically sorted? If not, I would suggest to sort them.

the format tools seem to reorder these

reyang · 2020-07-18T06:01:18Z

sdk/include/opentelemetry/sdk/metrics/aggregator/aggregator.h

+    Aggregator() = default;
+
+    /**
+     * Recieves a captured value from the instrument and applies it to the current aggregator value.


Suggested change

* Recieves a captured value from the instrument and applies it to the current aggregator value.

* Receives a captured value from the instrument and applies it to the current aggregator value.

reyang · 2020-07-18T06:07:23Z

sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h

+    void update(T val) override
+    {
+        this->mu_.lock();
+        this->values_[0] += val;  // atomic operation


For int and long, is it possible to spin + CAS (compare and swap) to achieve better perf?
Not blocking this PR though.

reyang · 2020-07-18T06:08:18Z

sdk/include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h

+        if (this->kind_ == other.kind_)
+        {
+            this->mu_.lock();
+            this->values_[0] += other.values_[0];  // atomic operation afaik


curious, what does // atomic operation afaik mean?

sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h

reyang · 2020-07-18T06:12:00Z

sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h

+    HistogramAggregator(metrics_api::BoundInstrumentKind kind, std::vector<double> boundaries)
+    {
+        this->kind_ = kind;
+        boundaries_   = boundaries;


Do we want to validate if the boundaries is monotonic?

Added a check which throws an invalid_argument exception if provided boundaries are not sorted

reyang · 2020-07-18T06:13:24Z

sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h

+            }
+        }
+
+        // Alternate implementation with binary search


It'll be helpful to put a comment on the design decision - e.g. why would we choose linear search vs. binary search.

Included some background on the topic above the update function -- let me know if this is not enough and I'll gladly add a more thorough explanation.

reyang · 2020-07-18T06:15:48Z

sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h

+
+        for (int i = 0; i < bucketCounts_.size(); i++)
+        {
+            bucketCounts_[i] += other.bucketCounts_[i];


Who should be responsible of making sure the other.bucketCounts won't overflow? Do we in general want to check the buckets are the same before proceeding?

This is a good suggestion. When you say " other.bucketCounts won't overflow," are you referring to a scenario in which adding the values together would exceed the capacity of the data type?

For example, if other.bucketCounts_.size() <= i.

There are two separate things:

index overflow while accessing the bucketCounts_.

value overflow while performing +=.

1 is more scary.

I preempted the possibility of an index out of bounds issue by ensuring that the boundary vectors of both aggregators are identical.

For the value overflow, I can think of some hacky ways to do it, for example if abs(a)+abs(b) < 0 to indicate a wraparound. I could also subtract from a variable set to int_max. I feel like there has to be a native c++ tool/function which does this but I can't seem to find it...

reyang · 2020-07-18T06:18:04Z

sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h

+
+    /**
+     * Merges the values of two aggregators in a semantically accurate manner.
+     * A histogram aggregator can only be merged with another histogram aggregatos with the same boudnaries.


Theoretically we could merge a more accurate histogram into a less accurate one (e.g. merge [0, 1, 10, 100, 1000) into [0, 10, 1000). Probably not something we should cover here.

reyang

Please find my comments.

codecov · 2020-07-24T20:51:27Z

Codecov Report

Merging #178 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #178   +/-   ##
=======================================
  Coverage   93.75%   93.75%           
=======================================
  Files         104      104           
  Lines        3249     3249           
=======================================
  Hits         3046     3046           
  Misses        203      203

ankit-bhargava · 2020-07-24T20:54:45Z

Addressed comments and rebased. This has a dependency on PR #161 and should be ready to go once that PR has been merged.

ankit-bhargava · 2020-07-27T17:25:42Z

Failing Bazel noexcept test although I was under the impression we were allowed to throw exceptions in SDK components? Unsure why the exporter proto CMake test is failing.

…regator.h Co-authored-by: Reiley Yang <[email protected]>

sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h

…regator.h Co-authored-by: Reiley Yang <[email protected]>

reyang

LGTM.

reyang · 2020-07-29T19:47:57Z

@ankit-bhargava would you fix the CI/Format issue?

…regator.h Co-authored-by: Reiley Yang <[email protected]>

…va/opentelemetry-cpp into metrics-agg-ctrhist

ankit-bhargava · 2020-07-29T20:44:54Z

@reyang
Fixed all the formatting issues and rebased. Seeing two runtime_context files which should not be part of this PR and I'm not sure why they've been added and if they're necessary? Feel free to remove these and merge (or I can delete them).

ankit-bhargava marked this pull request as ready for review July 16, 2020 03:17

ankit-bhargava requested a review from a team July 16, 2020 03:17

ankit-bhargava mentioned this pull request Jul 16, 2020

Add Synchronous Metric Instruments SDK #179

Merged

Brandon-Kimberly mentioned this pull request Jul 16, 2020

Add MinMaxSumCount and Gauge Aggregators #181

Merged

reyang reviewed Jul 18, 2020

View reviewed changes

sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h Outdated Show resolved Hide resolved

reyang reviewed Jul 18, 2020

View reviewed changes

reyang requested changes Jul 18, 2020

View reviewed changes

ankit-bhargava mentioned this pull request Jul 20, 2020

Add Asynchronous Metric Instruments SDK #191

Merged

Brandon-Kimberly mentioned this pull request Jul 21, 2020

Add Exact Aggregator #198

Merged

ankit-bhargava force-pushed the metrics-agg-ctrhist branch from 6182512 to c75b549 Compare July 24, 2020 20:48

ankit-bhargava mentioned this pull request Jul 27, 2020

Add Sketch Aggregator #213

Merged

ankit-bhargava force-pushed the metrics-agg-ctrhist branch from 72e2354 to adb56e1 Compare July 27, 2020 17:09

ankit-bhargava added 8 commits July 27, 2020 21:24

templated aggregators

5ff10e2

test files

302ca05

final specifier

0b78eb9

cleaning up for PR

c54651c

removing irrelevant test targets

4ba08e2

incorporating feedback

0cd64d9

adding virtual functions

01afb81

timestamp virtual function

7f32074

Update sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_agg…

13f4677

…regator.h Co-authored-by: Reiley Yang <[email protected]>

reyang reviewed Jul 28, 2020

View reviewed changes

sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h Outdated Show resolved Hide resolved

Update sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_agg…

0779a49

…regator.h Co-authored-by: Reiley Yang <[email protected]>

reyang approved these changes Jul 28, 2020

View reviewed changes

reyang added the pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.) label Jul 28, 2020

reyang added 3 commits July 28, 2020 21:27

Merge branch 'master' into metrics-agg-ctrhist

1458c6d

Merge branch 'master' into metrics-agg-ctrhist

a41706f

Merge branch 'master' into metrics-agg-ctrhist

2e79b0d

satac2 and others added 17 commits July 29, 2020 12:49

Context helper functions (open-telemetry#225)

4e2aeee

templated aggregators

822e7b4

test files

bcd519d

final specifier

b8c2fba

cleaning up for PR

60865b3

removing irrelevant test targets

c6debd3

incorporating feedback

161f719

adding virtual functions

739faa6

timestamp virtual function

19f6643

remove unused import

3a33a3c

user specified exceptions

c15942e

more virtuals for sketch aggregator

2bfd39a

typos

72e437a

Update sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_agg…

73d1382

…regator.h Co-authored-by: Reiley Yang <[email protected]>

Update sdk/include/opentelemetry/sdk/metrics/aggregator/histogram_agg…

75e48e1

…regator.h Co-authored-by: Reiley Yang <[email protected]>

Merge branch 'metrics-agg-ctrhist' of https://github.com/ankit-bharga…

f5e8149

…va/opentelemetry-cpp into metrics-agg-ctrhist

formatting

8e7357e

reyang merged commit 41af3ac into open-telemetry:master Jul 30, 2020

ziqizh pushed a commit to ziqizh/opentelemetry-cpp that referenced this pull request Jul 31, 2020

Add Counter and Histogram Aggregators (open-telemetry#178)

ab882ba

ziqizh pushed a commit to ziqizh/opentelemetry-cpp that referenced this pull request Jul 31, 2020

Add Counter and Histogram Aggregators (open-telemetry#178)

7ac34b1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Counter and Histogram Aggregators #178

Add Counter and Histogram Aggregators #178

ankit-bhargava commented Jul 16, 2020

reyang Jul 18, 2020

ankit-bhargava Jul 29, 2020

reyang Jul 18, 2020

reyang Jul 18, 2020

reyang Jul 18, 2020

reyang Jul 18, 2020

ankit-bhargava Jul 21, 2020

reyang Jul 18, 2020

ankit-bhargava Jul 21, 2020

reyang Jul 18, 2020

ankit-bhargava Jul 20, 2020

reyang Jul 20, 2020

reyang Jul 20, 2020

ankit-bhargava Jul 21, 2020

reyang Jul 18, 2020

reyang left a comment

codecov bot commented Jul 24, 2020 •

edited

Loading

ankit-bhargava commented Jul 24, 2020 •

edited

Loading

ankit-bhargava commented Jul 27, 2020

reyang left a comment

reyang commented Jul 29, 2020

ankit-bhargava commented Jul 29, 2020 •

edited

Loading

	* Recieves a captured value from the instrument and applies it to the current aggregator value.
	* Receives a captured value from the instrument and applies it to the current aggregator value.

Add Counter and Histogram Aggregators #178

Add Counter and Histogram Aggregators #178

Conversation

ankit-bhargava commented Jul 16, 2020

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

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

Choose a reason for hiding this comment

reyang left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 24, 2020 • edited Loading

Codecov Report

ankit-bhargava commented Jul 24, 2020 • edited Loading

ankit-bhargava commented Jul 27, 2020

reyang left a comment

Choose a reason for hiding this comment

reyang commented Jul 29, 2020

ankit-bhargava commented Jul 29, 2020 • edited Loading

codecov bot commented Jul 24, 2020 •

edited

Loading

ankit-bhargava commented Jul 24, 2020 •

edited

Loading

ankit-bhargava commented Jul 29, 2020 •

edited

Loading