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

Handle duplicate Aggregators and log instrument conflicts #3251

Merged
merged 29 commits into from Oct 11, 2022

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Sep 30, 2022

When a Meter is asked to create an instrument that already exists...

  • If it is the exact same instrument, it should return the same instance and update the already created Aggregators.
  • If it is not the exact same, but does share a name, it should return a new instrument with new Aggregators but log the collision as a warning.

This PR updates the metric SDK to achieve these things. It does this with the following.

  • Add the cache type described in Unify metric SDK caching #3245
  • Add an instrumentCache that wraps two caches. One to cache instrument aggregations and ensure they are unique. The other to cache resolved view instrumentIDs so any conflicting creations are logged (something recommended by the spec).
  • Maintain caches at a Meter level. This ensures that no duplicate instruments are issued per instrumentation scope. It also ensures that instrument conflicts are appropriately logged as such per instrumentation scope.
  • Refactor the pipeline data storage. Instruments are de-duplicated prior to being passed to the pipeline, stop checking for duplicates when added to the pipeline.
    • This allows for the storage type to drop one level of maps.

Part of #3245
Fix #3240
Fix #3229

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

codecov bot commented Sep 30, 2022

Codecov Report

Merging #3251 (b913d7a) into main (ffa94ca) will increase coverage by 0.1%.
The diff coverage is 93.6%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3251     +/-   ##
=======================================
+ Coverage   77.5%   77.6%   +0.1%     
=======================================
  Files        160     161      +1     
  Lines      11203   11280     +77     
=======================================
+ Hits        8690    8763     +73     
- Misses      2313    2317      +4     
  Partials     200     200             
Impacted Files Coverage Δ
sdk/metric/instrument.go 83.3% <ø> (ø)
sdk/metric/pipeline.go 93.9% <90.1%> (-1.0%) ⬇️
sdk/metric/cache.go 100.0% <100.0%> (ø)
sdk/metric/meter.go 100.0% <100.0%> (ø)

@MrAlias MrAlias added the bug Something isn't working label Sep 30, 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.

Only minor nits.

sdk/metric/meter.go Outdated Show resolved Hide resolved
sdk/metric/pipeline.go Show resolved Hide resolved
sdk/metric/instrument.go Outdated Show resolved Hide resolved
sdk/metric/instrument.go Outdated Show resolved Hide resolved
sdk/metric/internal/aggregator.go Outdated Show resolved Hide resolved
sdk/metric/internal/aggregator_test.go Outdated Show resolved Hide resolved
sdk/metric/meter.go Show resolved Hide resolved
@Aneurysm9 Aneurysm9 merged commit b5292b8 into open-telemetry:main Oct 11, 2022
@MrAlias MrAlias deleted the agg-cache branch October 11, 2022 19:51
@MrAlias MrAlias mentioned this pull request Oct 11, 2022
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
None yet
3 participants