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

Combine precomputed values of filtered attribute sets #3549

Merged
merged 29 commits into from
Jan 20, 2023

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Dec 19, 2022

Fix #3439

When an attribute filter drops a distinguishing attribute during the aggregation of a precomputed sum that value needs to be added to any existing one for an equivalent set1. This changes the current behavior of overriding the equivalent set's value.

Footnotes

  1. https://github.com/open-telemetry/opentelemetry-specification/issues/2905

Fix open-telemetry#3439

When an attribute filter drops a distinguishing attribute during the
aggregation of a precomputed sum add that value to existing, instead of
just setting the value as an override (current behavior).
@MrAlias MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Dec 19, 2022
@MrAlias MrAlias added this to the Metric v0.35.0 milestone Dec 19, 2022
@codecov
Copy link

codecov bot commented Dec 19, 2022

Codecov Report

Merging #3549 (1c25a20) into main (88f6000) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3549     +/-   ##
=======================================
+ Coverage   79.0%   79.1%   +0.1%     
=======================================
  Files        170     170             
  Lines      12585   12654     +69     
=======================================
+ Hits        9948   10018     +70     
  Misses      2424    2424             
+ Partials     213     212      -1     
Impacted Files Coverage Δ
sdk/metric/internal/filter.go 100.0% <100.0%> (ø)
sdk/metric/internal/sum.go 100.0% <100.0%> (ø)
exporters/jaeger/jaeger.go 90.3% <0.0%> (-0.9%) ⬇️
sdk/metric/metricdata/metricdatatest/assertion.go 83.6% <0.0%> (+3.0%) ⬆️

@MrAlias MrAlias changed the title Combine spatially aggregated precomputed vals Combine spatially aggregated sets for precomputed values Dec 19, 2022
@MrAlias MrAlias marked this pull request as ready for review December 19, 2022 18:34
@MrAlias MrAlias changed the title Combine spatially aggregated sets for precomputed values Combine precomputed values of filtered attribute sets Dec 19, 2022
@MrAlias

This comment was marked as resolved.

@MrAlias MrAlias closed this Dec 20, 2022
@MrAlias MrAlias reopened this Dec 20, 2022
@MrAlias MrAlias marked this pull request as draft December 20, 2022 22:02
@MrAlias

This comment was marked as resolved.

@MrAlias MrAlias marked this pull request as ready for review December 20, 2022 22:19
@MrAlias

This comment was marked as outdated.

Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

I still have trouble understanding the code structure a bit, but the tests help me believe that it works correctly. Comments are just for my understanding, and possibly for additional comments in the code

sdk/metric/internal/filter.go Outdated Show resolved Hide resolved
sdk/metric/internal/filter.go Show resolved Hide resolved
sdk/metric/internal/filter.go Outdated Show resolved Hide resolved
sdk/metric/internal/sum.go Outdated Show resolved Hide resolved
@MrAlias
Copy link
Contributor Author

MrAlias commented Jan 20, 2023

Tracking link CI failure here: #2957 (comment)

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
Status: Done
Development

Successfully merging this pull request may close these issues.

Align Async Counter behavior when used with filtered attributes.
5 participants