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

Only registers callbacks if non-drop aggregation is used #3408

Merged
merged 20 commits into from Nov 11, 2022

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Oct 28, 2022

Fix #3053

  • The async instruments currently return an error if and only if there are no aggregators returned from a resolve. Returning no aggregators means the instrument aggregation is drop. Do not include this in the error reporting decision.
  • The instruments passed to RegisterCallback need to have some aggregation defined otherwise it is implied they have a Drop aggregation. Check that at least one instrument passed has an aggregation other than Drop before registering the callback with the pipelines.
  • Remove unneeded TODO from pipeline.go

@MrAlias MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Oct 28, 2022
@MrAlias MrAlias added this to the Metric v0.34.0 milestone Oct 28, 2022
@codecov
Copy link

codecov bot commented Oct 28, 2022

Codecov Report

Merging #3408 (3b041c5) into main (d091ba8) will increase coverage by 0.0%.
The diff coverage is 70.0%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3408   +/-   ##
=====================================
  Coverage   77.7%   77.7%           
=====================================
  Files        164     164           
  Lines      11521   11538   +17     
=====================================
+ Hits        8961    8975   +14     
- Misses      2364    2368    +4     
+ Partials     196     195    -1     
Impacted Files Coverage Δ
sdk/metric/instrument_provider.go 100.0% <ø> (+6.3%) ⬆️
sdk/metric/pipeline.go 93.5% <ø> (ø)
sdk/metric/meter.go 87.7% <70.0%> (-12.3%) ⬇️

The async instruments currently return an error if and only if there are
no aggregators returned from a resolve. Returning no aggregators means
the instrument aggregation is drop. Do not include this in the error
reporting decision.
The instruments passed to RegisterCallback need to have some aggregation
defined otherwise it is implied they have a Drop aggregation. Check that
at least one instrument passed has an aggregation other than Drop before
registering the callback with the pipelines.

Also, return an error if the user passed another API implementation of
an asynchronous instrument.
@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 28, 2022

This PR becomes even simpler if we can merge #3256 first.

@MrAlias MrAlias merged commit 308d036 into open-telemetry:main Nov 11, 2022
@MrAlias MrAlias deleted the fix-3053 branch November 11, 2022 17:11
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.

Callbacks should only be called if applicable instrument exists.
4 participants