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 duplicate instrument registration to return same instrument #3238

Closed
wants to merge 14 commits into from

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Sep 28, 2022

Add a new cache type to handle generic lookup functionality. Wrap this new type in an new instrumentCache to be used by the resolver when resolving Aggregators. If an instrument has already been resolved, it returns those aggregations or an error.

The new cache type is defined generically so its can be unified with the meterRegistry, which duplicates the functionality. That unification is left to a follow-on PR.


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

codecov bot commented Sep 28, 2022

Codecov Report

Merging #3238 (1d4f1d2) into main (aca054b) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3238     +/-   ##
=======================================
+ Coverage   77.3%   77.4%   +0.1%     
=======================================
  Files        159     160      +1     
  Lines      11184   11228     +44     
=======================================
+ Hits        8651    8699     +48     
+ Misses      2335    2331      -4     
  Partials     198     198             
Impacted Files Coverage Δ
sdk/metric/cache.go 100.0% <100.0%> (ø)
sdk/metric/meter.go 100.0% <100.0%> (ø)
sdk/metric/pipeline.go 96.4% <100.0%> (+0.4%) ⬆️
sdk/trace/batch_span_processor.go 81.9% <0.0%> (+1.7%) ⬆️

@MrAlias MrAlias marked this pull request as ready for review September 28, 2022 16:51
@MrAlias MrAlias changed the title Add instrument cache Fix duplicate instrument registration to return same instance Sep 28, 2022
@MrAlias MrAlias changed the title Fix duplicate instrument registration to return same instance Fix duplicate instrument registration to return same instrument Sep 28, 2022
@MrAlias
Copy link
Contributor Author

MrAlias commented Sep 28, 2022

This does not resolve #3240. I'm looking into how feasible it would be to resolve that as well here, but its resolution might need to be a follow-on PR.

@MrAlias
Copy link
Contributor Author

MrAlias commented Sep 29, 2022

This does not resolve #3240. I'm looking into how feasible it would be to resolve that as well here, but its resolution might need to be a follow-on PR.

It does look like we can cache lower in the computation processing to handle #3240: MrAlias#661. I'm going to close this so it is not reviewed, and refactor a more complete solution for another PR.

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
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate instrument registration generates an error instead of a valid instrument
1 participant