Skip to content

Custom Aggregation support - #1899

Merged
lalitb merged 27 commits into
open-telemetry:mainfrom
lalitb:custom-agg
Jan 23, 2023
Merged

Custom Aggregation support#1899
lalitb merged 27 commits into
open-telemetry:mainfrom
lalitb:custom-agg

Conversation

@lalitb

@lalitb lalitb commented Jan 5, 2023

Copy link
Copy Markdown
Member

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

codecov Bot commented Jan 5, 2023

Copy link
Copy Markdown

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

lalitb commented Jan 9, 2023

Copy link
Copy Markdown
Member Author

@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

@esigo esigo left a comment

Copy link
Copy Markdown
Member

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

Comment thread sdk/test/metrics/sum_aggregation_test.cc Outdated
Comment thread sdk/test/metrics/histogram_aggregation_test.cc Outdated
@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