Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SDK] Add AdaptingCircularBufferCounter for exponential histograms #2158

Merged
merged 17 commits into from
Jun 1, 2023

Conversation

euroelessar
Copy link
Contributor

@euroelessar euroelessar commented May 26, 2023

Updates #1391

Changes

Add AdaptingCircularBufferCounter, which will be used for buckets in exponential histograms.
This PR is mostly a rewrite of corresponding opentelemetry-java implementation and tests.

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

@euroelessar euroelessar requested a review from a team May 26, 2023 02:02
@codecov
Copy link

codecov bot commented May 26, 2023

Codecov Report

Merging #2158 (7f3687e) into main (8ba9529) will increase coverage by 0.33%.
The diff coverage is 100.00%.

❗ Current head 7f3687e differs from pull request most recent head 4a34046. Consider uploading reports for the commit 4a34046 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2158      +/-   ##
==========================================
+ Coverage   87.15%   87.48%   +0.33%     
==========================================
  Files         166      168       +2     
  Lines        4777     4869      +92     
==========================================
+ Hits         4163     4259      +96     
+ Misses        614      610       -4     
Impacted Files Coverage Δ
...e/opentelemetry/sdk/metrics/data/circular_buffer.h 100.00% <100.00%> (ø)
sdk/src/metrics/data/circular_buffer.cc 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes


size_t start_index_ = kNullIndex;
size_t end_index_ = kNullIndex;
size_t base_index_ = kNullIndex;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while these variable names are self-descriptive, can you still add a comment before each for what they represent. Thanks.

template <typename T>
void operator()(std::vector<T> &backing)
{
std::fill(backing.begin(), backing.end(), 0);
Copy link
Member

@lalitb lalitb May 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you may have to typecast to static_cast<T>(0) to get rid of CI failure.

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution. Nicely done.

@esigo esigo self-assigned this May 31, 2023
Copy link
Member

@esigo esigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
thanks for the PR :)

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

About the CI failure:

  • check CI build CI / CMake msvc (maintainer mode) (pull_request)
  • check the logs, look for circular_buffer.cc, look for error C2220
  • see suggested fix from Lalit

Thanks for the PR.

@euroelessar
Copy link
Contributor Author

Thanks for the review.
I've been looking at coming usage of these containers in histograms themselves and had to make couple of adjustments:

  1. changed index to be signed (as allowed by the spec)
  2. added moving constructors/operators (to allow making histogram data point movable)

@marcalff marcalff changed the title Add AdaptingCircularBufferCounter for exponential histograms [SDK] Add AdaptingCircularBufferCounter for exponential histograms Jun 1, 2023
@marcalff marcalff merged commit f5fd906 into open-telemetry:main Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants