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

Metric SDK: Sum duplicate async observations regardless of filtering #4289

Merged
merged 3 commits into from
Jul 19, 2023

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Jul 7, 2023

Fixes #4217
It addresses this issue by (always) summing duplicate observations regardless of whether or not a filter is applied. This simplifies the implementation as well by removing special code for the filtered vs non-filtered case.

@codecov
Copy link

codecov bot commented Jul 7, 2023

Codecov Report

Merging #4289 (645c548) into main (f2a9f2f) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #4289     +/-   ##
=======================================
- Coverage   83.5%   83.4%   -0.1%     
=======================================
  Files        184     184             
  Lines      14365   14330     -35     
=======================================
- Hits       11996   11957     -39     
- Misses      2143    2147      +4     
  Partials     226     226             
Impacted Files Coverage Δ
sdk/metric/internal/aggregate/filter.go 100.0% <ø> (ø)
sdk/metric/internal/aggregate/sum.go 100.0% <100.0%> (ø)

... and 1 file with indirect coverage changes

@dashpole dashpole force-pushed the simplify_filtered branch from 0f4df82 to fdae38e Compare July 7, 2023 19:48
@dashpole dashpole changed the title Metric SDK: Drop non-observed attibute sets, and sum duplicate observations Metric SDK: Sum duplicate async observations regardless of filtering Jul 7, 2023
@dashpole
Copy link
Contributor Author

dashpole commented Jul 7, 2023

I'll wait to request review until I can rebase this on #4290

@dashpole dashpole force-pushed the simplify_filtered branch from fdae38e to 385c4d4 Compare July 13, 2023 17:46
@dashpole dashpole marked this pull request as ready for review July 13, 2023 17:49
@dashpole
Copy link
Contributor Author

Rebased, and ready for review!

sdk/metric/internal/aggregate/sum_test.go Outdated Show resolved Hide resolved
sdk/metric/internal/aggregate/sum_test.go Outdated Show resolved Hide resolved
sdk/metric/internal/aggregate/sum_test.go Outdated Show resolved Hide resolved
sdk/metric/meter_test.go Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@MrAlias
Copy link
Contributor

MrAlias commented Jul 17, 2023

@dashpole it looks like #4304 was merged prior to this. Can you rebase this (sorry about the conflicts)?

@open-telemetry/go-approvers PTAL at this.

@dashpole dashpole force-pushed the simplify_filtered branch from 56d3860 to 85e2d50 Compare July 17, 2023 15:45
@dashpole
Copy link
Contributor Author

no worries. Rebased

@MrAlias MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Jul 17, 2023
@MadVikingGod MadVikingGod merged commit c197fe9 into open-telemetry:main Jul 19, 2023
@MrAlias MrAlias added this to the Metric SDK v0.40.0 milestone Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Precomputed Sum aggregation should filter on collection
4 participants