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

Add Aggregation Storage #1213

Merged
merged 10 commits into from
Feb 16, 2022
Merged

Add Aggregation Storage #1213

merged 10 commits into from
Feb 16, 2022

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Feb 15, 2022

Fixes #1197 #1202

Changes

As discussed in today's community meeting, this PR adds the first part of Aggregation Storage - to consume/record the measurements and store them against the Aggregation objects. To achieve thread-safe aggregation storage, we use a non-blocking mutex lock SpinLockMutex. The benchmark tests are added, and will continue to look into lock-free concurrent storage libraries in C++ ( similar to [ConcurrentHashMap ](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentHashMap.html in java ) which provides lock free retrievals. Some of the available options to look into:

  1. Atomic Unordered map from Facebook - https://github.com/facebook/folly/blob/main/folly/AtomicUnorderedMap.h
  2. Intel thread building blocks - https://github.com/oneapi-src/oneTBB

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

@lalitb lalitb requested a review from a team February 15, 2022 02:46
@codecov
Copy link

codecov bot commented Feb 15, 2022

Codecov Report

Merging #1213 (7b9fb31) into main (15c6f33) will decrease coverage by 0.37%.
The diff coverage is 77.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1213      +/-   ##
==========================================
- Coverage   93.35%   92.99%   -0.36%     
==========================================
  Files         193      196       +3     
  Lines        6908     7038     +130     
==========================================
+ Hits         6448     6544      +96     
- Misses        460      494      +34     
Impacted Files Coverage Δ
.../opentelemetry/sdk/metrics/measurement_processor.h 40.00% <0.00%> (-2.10%) ⬇️
...ntelemetry/sdk/metrics/state/sync_metric_storage.h 52.95% <48.94%> (-1.60%) ⬇️
...entelemetry/sdk/metrics/state/attributes_hashmap.h 93.94% <93.94%> (ø)
sdk/test/metrics/attributes_hashmap_test.cc 100.00% <100.00%> (ø)
sdk/test/metrics/sync_metric_storage_test.cc 100.00% <100.00%> (ø)
...etry/sdk/metrics/aggregation/default_aggregation.h 0.00% <0.00%> (ø)
...ntelemetry/sdk/metrics/view/attributes_processor.h 100.00% <0.00%> (+18.75%) ⬆️
...lemetry/sdk/metrics/aggregation/drop_aggregation.h 50.00% <0.00%> (+25.00%) ⬆️

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 :)

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.

[Metrics SDK] Implement Writable Storage for Aggregation
2 participants