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 Exact Aggregator #198

Merged
merged 23 commits into from
Jul 30, 2020
Merged

Add Exact Aggregator #198

merged 23 commits into from
Jul 30, 2020

Conversation

Brandon-Kimberly
Copy link
Contributor

This PR builds out SDK support for the Exact Aggregator which combines 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. Additionally, these aggregators depend on the base Aggregator interface in PR #178.

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.

@Brandon-Kimberly Brandon-Kimberly requested a review from a team July 21, 2020 19:58
@Brandon-Kimberly Brandon-Kimberly changed the title Sketch agg Add Sketch Aggregator Jul 21, 2020
@Brandon-Kimberly Brandon-Kimberly changed the title Add Sketch Aggregator Add Exact Aggregator Jul 21, 2020
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@reyang 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 30, 2020
@codecov
Copy link

codecov bot commented Jul 30, 2020

Codecov Report

Merging #198 into master will increase coverage by 0.14%.
The diff coverage is 97.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #198      +/-   ##
==========================================
+ Coverage   93.84%   93.99%   +0.14%     
==========================================
  Files         109      111       +2     
  Lines        3512     3661     +149     
==========================================
+ Hits         3296     3441     +145     
- Misses        216      220       +4     
Impacted Files Coverage Δ
...elemetry/sdk/metrics/aggregator/exact_aggregator.h 89.74% <89.74%> (ø)
sdk/test/metrics/exact_aggregator_test.cc 100.00% <100.00%> (ø)

@Brandon-Kimberly
Copy link
Contributor Author

@reyang This PR is ready to merge, pending final approval :)

@reyang reyang merged commit e73dffa into open-telemetry:master Jul 30, 2020
ziqizh pushed a commit to ziqizh/opentelemetry-cpp that referenced this pull request Jul 31, 2020
ziqizh pushed a commit to ziqizh/opentelemetry-cpp that referenced this pull request Jul 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants