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.

Sign up for GitHub

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

Jump to bottom

Align Async Counter behavior when used with filtered attributes. #3439

Closed
MadVikingGod opened this issue Nov 3, 2022 · 3 comments · Fixed by #3549
Closed

Align Async Counter behavior when used with filtered attributes. #3439

MadVikingGod opened this issue Nov 3, 2022 · 3 comments · Fixed by #3549
Assignees
@MrAlias
Labels
area:metrics Part of OpenTelemetry Metrics bug Something isn't working pkg:SDK Related to an SDK package
Milestone
Metric v0.35.0

Comments

@MadVikingGod
Copy link
Contributor

Problem Statement

If an attribute filter is used with an Async Counter the last value observed will be exported. This

// When there is an Attribute Filter that only allows "foo" attributes.
 
ctr, _ := meter.AsyncInt64().Counter("testCounter")
...
ctr.Observe(ctx, 4, attribute.Int("Version", 1))
ctr.Observe(ctx, 5, attribute.Int("Version", 2))

Yields a Sum with a value of 5.

This has been discussed in the spec, and the expected output is a Sum with a value of 9.
open-telemetry/opentelemetry-specification#2905

Proposed Solution

Correct our instruments, filters, or aggregators to get the appropriate value.
@MadVikingGod MadVikingGod added the enhancement New feature or request label Nov 3, 2022
@MrAlias MrAlias added bug Something isn't working pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics and removed enhancement New feature or request labels Nov 3, 2022
@wisdommatt
Copy link
Contributor

@MadVikingGod , @MrAlias I would like to work on this

@MrAlias
Copy link
Contributor

MrAlias commented Nov 4, 2022 •
edited

@wisdommatt please outline the design approach you plan to take here. This is likely not going to be obvious to resolve and having consensus on the approach will help prevent wasting work.

@wisdommatt
Copy link
Contributor

@MrAlias sure, will look at the otel spec issue open-telemetry/opentelemetry-specification#2905, then share my feedback/design approach.

@MrAlias MrAlias self-assigned this Dec 19, 2022
MrAlias added a commit to MrAlias/opentelemetry-go that referenced this issue Dec 19, 2022
@MrAlias
Combine spatially aggregated precomputed vals
948932a 
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 mentioned this issue Dec 19, 2022
Merged
@MrAlias MrAlias added this to the Metric v0.35.0 milestone Dec 19, 2022
MrAlias added a commit that referenced this issue Jan 20, 2023
@MrAlias @hanyuancheung @MadVikingGod
Combine precomputed values of filtered attribute sets (#3549)
a1ce7e5 
* Combine spatially aggregated precomputed vals

Fix #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).

* Ignore false positive lint error and test method

* Add fix to changelog

* Handle edge case of exact set after filter

* Fix filter and measure algo for precomp

* Add tests for precomp sums

* Unify precomputedMap

* Adds example from supplimental guide

* Fixes for lint

* Update sdk/metric/meter_example_test.go

* Fix async example test

* Reduce duplicate code in TestAsynchronousExample

* Clarify naming and documentation

* Fix spelling errors

* Add a noop filter to default view

Co-authored-by: Chester Cheung <cheung.zhy.csu@gmail.com>
Co-authored-by: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com>
@MrAlias MrAlias mentioned this issue Jan 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@MrAlias MrAlias

Labels
area:metrics Part of OpenTelemetry Metrics bug Something isn't working pkg:SDK Related to an SDK package
Projects
No open projects
Go: Metric SDK (Beta)
Status: Done
Milestone
Metric v0.35.0
Development

Successfully merging a pull request may close this issue.

Combine precomputed values of filtered attribute sets MrAlias/opentelemetry-go
3 participants
@MadVikingGod @MrAlias @wisdommatt