Base2 exponential histogram aggregation

[ethandmd]

Fixes #1391

Changes

Add base2 exponential histogram aggregation.

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

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: ethandmd / name: ethan (71855a9, 66c5a6b, 42a904e, e5e7b20, 0f54022, 055d331, 6468765, dc5d956, 775688a, 2bfd9da, 8f2130c, 53e025e, b2dee01, 5641bbd, 423e92d, 84c09d1, 4397886, e69e73c, ab5441e, 81ea997, 20ba935, 9040c38, 338cce4, 17c94ca, ea3190d, 712e538, ea50726, 21e1d34, 250de89, 0fc7071, b2d7a02, 556eb89, a7a1234, 1d96b3e, f184cec, bc96a2f, b9e5df6, 50ca1c0, f917c08, ca542f7, d4cbcbc)
• ✅ login: lalitb / name: Lalit Kumar Bhasin (9adcfac, 262425e, 43c0e00, beb9227)
• ✅ login: felipesantosk / name: Felipe C. Dos Santos (531ac16, de04603, 293e2dd, b03d915, e4daa7b, 9146925, 9b9d8c1, 8cf15c4, 4376128, 4408d33, c7ca650, b1f54d4, ffbd2fd, a9737bf)
• ✅ login: ThomsonTan / name: Tom Tan (db95400, f98d198, e550710)
• ✅ login: euroelessar / name: Ruslan Nigmatullin (b62565f)

[netlify]

✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name	Link
🔨 Latest commit	beb9227
🔍 Latest deploy log	https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/680bc887d6b270000856253d

[ethandmd]

@lalitb @ThomsonTan @marcalff @dbarker

In order to satisfy CLA check had to remove some @{corporate}.com alias from the commit history by using a new branch. I am closing the previous WIP PR and @felipesantosk and I will be working here.

Looking forward to your review and comments :)

[codecov]

Codecov Report

Attention: Patch coverage is 92.04893% with 26 lines in your changes missing coverage. Please review.

Project coverage is 89.67%. Comparing base (3e761d0) to head (beb9227).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...egation/base2_exponential_histogram_aggregation.cc	90.64%	22 Missing ⚠️
...etry/sdk/metrics/aggregation/default_aggregation.h	50.00%	2 Missing ⚠️
exporters/ostream/src/metric_exporter.cc	95.46%	1 Missing ⚠️
...nclude/opentelemetry/sdk/metrics/data/point_data.h	95.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3346      +/-   ##
==========================================
+ Coverage   89.55%   89.67%   +0.12%     
==========================================
  Files         210      211       +1     
  Lines        6505     6832     +327     
==========================================
+ Hits         5825     6126     +301     
- Misses        680      706      +26     

Files with missing lines	Coverage Δ
exporters/ostream/test/ostream_metric_test.cc	100.00% <100.00%> (ø)
...pentelemetry/sdk/metrics/aggregation/aggregation.h	100.00% <ø> (ø)
...metry/sdk/metrics/aggregation/aggregation_config.h	100.00% <ø> (ø)
...e/opentelemetry/sdk/metrics/data/circular_buffer.h	100.00% <ø> (ø)
sdk/src/metrics/data/circular_buffer.cc	100.00% <100.00%> (ø)
exporters/ostream/src/metric_exporter.cc	91.55% <95.46%> (+0.65%)	⬆️
...nclude/opentelemetry/sdk/metrics/data/point_data.h	96.16% <95.00%> (-3.84%)	⬇️
...etry/sdk/metrics/aggregation/default_aggregation.h	79.02% <50.00%> (-1.50%)	⬇️
...egation/base2_exponential_histogram_aggregation.cc	90.64% <90.64%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ThomsonTan]

Is the boundary configuration still needed? Remove it if no.

[lalitb]

Did a quick test on the effect of adding a Base2ExponentialHistogramPointData as new variant alternative. As the size of a variant type is determined by the size of its largest alternative, there seems to increase in ~50 bytes per aggregated value. For the maximum cardinality limit of 2000, this would account for 100KB worst scenario peek increase per configured instrument.

Refer - https://godbolt.org/z/cG6b6Ks1d

output:
sizeof(PointTypeWithBase2): 176 bytes
sizeof(PointTypeWithoutBase2): 120 bytes

I believe this increase is reasonable, unless there are specific concerns about memory usage at scale - with large number of configured instruments. @ThomsonTan ?

[ThomsonTan]

Thanks for trying this. This extra memory consumption could be an issue for the users who are not using exponential histogram, as we've seen some user have very large number of instruments configured. @ethandmd , is it possible to make the feature opt-in to avoid taking the extra memory for users who don't use this feature?

[lalitb]

We can also modify the Base2ExponentialHistogramPointData to contain unique_ptr to AdaptingCircularBufferCounter, as that seems to be inflating the size. Something like:

class Base2ExponentialHistogramPointDataUniquePtr {
public:
  uint64_t count_{};
  double sum_{};
  int32_t scale_{};
  uint64_t zero_count_{};
  std::unique_ptr<AdaptingCircularBufferCounter> positive_buckets_;
  std::unique_ptr<AdaptingCircularBufferCounter> negative_buckets_;
  double min_{}, max_{}, zero_threshold_{};
  bool record_min_max_ = true;
  size_t max_buckets_{};

  Base2ExponentialHistogramPointDataUniquePtr()
      : positive_buckets_{std::make_unique<AdaptingCircularBufferCounter>(0)},
        negative_buckets_{std::make_unique<AdaptingCircularBufferCounter>(0)} {}
};

The size seems to be reduced to the earlier size with that: - https://godbolt.org/z/zvdbfYYbs

sizeof(PointTypeWithBase2): 176 bytes
sizeof(PointTypeWithoutBase2): 120 bytes
sizeof(PointTypeWithBase2UniquePtr): 120 bytes

[ethandmd]

Thanks for the analysis! Let me take a look

[ThomsonTan]

One more suggestion is moving below 2 files together and put them at the end of the class, which will make the padding more compact.

  int32_t scale_{};
  bool record_min_max_ = true;