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

Fix Asynchronous Counters Recording #3350

Merged
merged 11 commits into from
Oct 19, 2022
Merged

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Oct 17, 2022

Fix #3278

Currently, asynchronous counters observe values that are treated as incremental changes to already summed recordings. This is contrary to what the specification notes is expected by the user in the API:

Note: Unlike Counter.Add() which takes the increment/delta value, the callback function reports the absolute value of the counter. To determine the reported rate the counter is changing, the difference between successive measurements is used.

This fixes the implemented behavior. It adds a new precomputedSum Aggregator type that interprets the passed Aggregate values as already computed sums and returns them directly. This new Aggregator type is used for the AsyncCounter and AsyncUpDownCounter instrument kinds.

Clarify the Counter and UpDownCounter Observe values are the exact
counter value, not increments to the previous measurements.
@MrAlias MrAlias added bug Something isn't working pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Oct 17, 2022
@MrAlias MrAlias added this to the Metric v0.33.0 milestone Oct 17, 2022
@codecov
Copy link

codecov bot commented Oct 17, 2022

Codecov Report

Merging #3350 (144a05b) into main (2d02a2f) will increase coverage by 0.0%.
The diff coverage is 92.8%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3350   +/-   ##
=====================================
  Coverage   77.9%   77.9%           
=====================================
  Files        164     164           
  Lines      11319   11345   +26     
=====================================
+ Hits        8821    8847   +26     
  Misses      2301    2301           
  Partials     197     197           
Impacted Files Coverage Δ
sdk/metric/pipeline.go 93.4% <85.7%> (-0.5%) ⬇️
sdk/metric/internal/sum.go 100.0% <100.0%> (ø)
exporters/jaeger/jaeger.go 91.1% <0.0%> (+0.8%) ⬆️

@MrAlias MrAlias changed the title Update Asynchronous Instrument Recording Fix Asynchronous Counters Recording Oct 17, 2022
Copy link
Contributor

@MadVikingGod MadVikingGod left a comment

Choose a reason for hiding this comment

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

This seems reasonable.

Just for my understanding does this basically mean that async Counters are gauges with monotonic flags and temporality fields?

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

MrAlias commented Oct 19, 2022

Just for my understanding does this basically mean that async Counters are gauges with monotonic flags and temporality fields?

I think the AsyncUpDownCounter could be though of this abstractly. It is still exported as a Sum data type, by default, in the end though.

The way I understand the specification to be written is that the user of asynchronous up-down-counters and counters maintain the state of the counter themselves. It is passed to the SDK with the appropriate monotonic and temporality types to have the SDK report the sum with the right properties, but it is up to the user to ensure the validity of these properties.

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 bug Something isn't working pkg:SDK Related to an SDK package
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

AsyncCounter and AsyncUpDownCounter expected callback value changed in v0.32.0
3 participants