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

Custom Aggregation support #1899

Merged
merged 27 commits into from
Jan 23, 2023
Merged

Custom Aggregation support #1899

merged 27 commits into from
Jan 23, 2023

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Jan 5, 2023

Fixes #1664

Changes

This is ready to review now. With the change, it is possible to use the custom aggregator for the Instruments.
Also added tests for:

  • Counter Instrument to Sum Aggregation (this is default)
  • Histogram Instrument to Sum Aggregation
  • Last Value Instrument to Sum Aggregation
  • Histogram Instrument to Histogram Aggregation (this is default)
  • Counter Instrument to Histogram Aggregation

As a side change, it fixes few warning for unused variables.

Please provide a brief description of the changes here.

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 January 5, 2023 05:58
@lalitb lalitb marked this pull request as draft January 5, 2023 05:58
@codecov
Copy link

codecov bot commented Jan 5, 2023

Codecov Report

Merging #1899 (d28ba85) into main (e8fce84) will increase coverage by 0.23%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1899      +/-   ##
==========================================
+ Coverage   86.90%   87.12%   +0.23%     
==========================================
  Files         165      165              
  Lines        4594     4596       +2     
==========================================
+ Hits         3992     4004      +12     
+ Misses        602      592      -10     
Impacted Files Coverage Δ
...etry/sdk/metrics/aggregation/default_aggregation.h 72.59% <100.00%> (-6.10%) ⬇️
...telemetry/sdk/metrics/state/async_metric_storage.h 86.12% <100.00%> (ø)
...ntelemetry/sdk/metrics/state/sync_metric_storage.h 87.50% <100.00%> (+0.41%) ⬆️
sdk/src/metrics/meter.cc 61.22% <100.00%> (+7.00%) ⬆️
sdk/src/metrics/state/temporal_metric_storage.cc 98.25% <100.00%> (+0.04%) ⬆️
...include/opentelemetry/sdk/common/circular_buffer.h 100.00% <0.00%> (+2.00%) ⬆️
...telemetry/sdk/metrics/state/multi_metric_storage.h 83.34% <0.00%> (+3.34%) ⬆️

@lalitb
Copy link
Member Author

lalitb commented Jan 9, 2023

@mprzybylski Have created a draft PR, if you want to test this. Haven't added all unit-test scenarios, basically blocked on #1869 to be merged before adding tests for histogram aggregation. But should be good enough to test.

@lalitb lalitb marked this pull request as ready for review January 18, 2023 20:59
@lalitb lalitb changed the title [WIP] Custom Aggregation support Custom Aggregation support Jan 19, 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
just had one nit comment.
Thanks for the PR :)

sdk/test/metrics/sum_aggregation_test.cc Outdated Show resolved Hide resolved
sdk/test/metrics/histogram_aggregation_test.cc Outdated Show resolved Hide resolved
@esigo esigo added area:metrics OpenTelemetry metrics ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) labels Jan 22, 2023
@lalitb lalitb enabled auto-merge (squash) January 23, 2023 05:50
@lalitb lalitb merged commit 998badd into open-telemetry:main Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics OpenTelemetry metrics ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Metrics SDK] - Supporting changing Aggregation type using Views
2 participants