Cache instruments so repeatedly creating identical instruments doesn't leak memory

[dashpole]

Resolve #3527
Fixes open-telemetry/opentelemetry-go-contrib#4226.

This is one potential solution to the problem of creating multiple instances of an instrumentation library leaking memory. Alternatively, we could require users to cache the instrumentation library instance itself, as was done in googleapis/google-api-go-client#2329.

This works well for synchronous instruments, as making observations on the instrument from multiple instances of a library correctly aggregates across instances. But for async instruments, this is a bit more confusing. e.g. what should happen if I instantiate two instances of the runtime metrics library? Presumably the last observation should win?

This also only solves the problem of caching instruments themselves. The pattern of making multiple instances of an instrumentation library would still leak memory if each instance created and registered a new Callback. If RegisterCallback is used, the callback can be Unregistered(), but if the callback is passed as an option to the instrument, it will exist forever, which would also leak memory.

TODO:

• Benchmarks
• Cache callbacks as well

[codecov]

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (cef39a1) 82.3% compared to head (55829b3) 82.3%.

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #4820   +/-   ##
=====================================
  Coverage   82.3%   82.3%           
=====================================
  Files        226     226           
  Lines      18481   18557   +76     
=====================================
+ Hits       15222   15286   +64     
- Misses      2973    2983   +10     
- Partials     286     288    +2     

Files	Coverage Δ
sdk/metric/cache.go	100.0% <100.0%> (ø)
sdk/metric/meter.go	90.4% <81.6%> (-2.2%)	⬇️

... and 1 file with indirect coverage changes

[MrAlias]

I think this looks like a decent proposal. Looking back through the record, we had originally tried to do this.

[dashpole]

Ah, cool. I'd definitely like feedback from @MadVikingGod, then

[MadVikingGod]

Ah, cool. I'd definitely like feedback from @MadVikingGod, then

That work was pulled out because of added complexity, trying to get something working, and concerns over memory freeing.

At first glance, this looks to add caching in a simple way. I would suggest putting a benchmark to ensure that it performs better when reused and doesn't affect single-use too much.

[MrAlias]

It isn't clear if the aggregators cache is still needed if we are caching instruments.

I think this is still going to be needed to ensure view modified instruments that result in the same aggregators get the same aggregators. But I haven't tested it.

[MrAlias]

The pattern of making multiple instances of an instrumentation library would still leak memory if each instance created and registered a new Callback.

This is a bit worrisome. Solving the issue to return the same instrument instance may lead users into thinking they can just make the same calls repeatedly.

Should we document that only first call using With*Callback will be honored? And users should use RegisterCallback after that?

[dashpole]

Looks like python logs a warning and ignores callbacks after the first: https://github.com/open-telemetry/opentelemetry-python/blob/975733c71473cddddd0859c6fcbd2b02405f7e12/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py#L171

[MrAlias]

This should resolve #3527

[MrAlias]

Looks like python logs a warning and ignores callbacks after the first: https://github.com/open-telemetry/opentelemetry-python/blob/975733c71473cddddd0859c6fcbd2b02405f7e12/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py#L171

+1 to this behavior being implemented here.

[dashpole]

This now ignores callbacks after the first.

For benchmarking, If I run the existing BenchmarkInstrumentCreation, it shows a substantial increase in performance because it is now returning the cached version instead of making a new one each time:

From main

$ go test -bench=BenchmarkInstrumentCreation
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/sdk/metric
cpu: Intel(R) Xeon(R) CPU @ 2.20GHz
BenchmarkInstrumentCreation-24    	   98368	     11940 ns/op	    3043 B/op	      69 allocs/op
PASS
ok  	go.opentelemetry.io/otel/sdk/metric	1.379s

This PR:

$ go test -bench=BenchmarkInstrumentCreation
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/sdk/metric
cpu: Intel(R) Xeon(R) CPU @ 2.20GHz
BenchmarkInstrumentCreation-24    	  318759	      3187 ns/op	     480 B/op	       6 allocs/op
PASS
ok  	go.opentelemetry.io/otel/sdk/metric	1.144s

If I move NewMeterProvider and Meter() inside the loop (so that it isn't using the cached one), I get

From main

$ go test -bench=BenchmarkInstrumentCreation
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/sdk/metric
cpu: Intel(R) Xeon(R) CPU @ 2.20GHz
BenchmarkInstrumentCreation-24    	   32074	     37227 ns/op	   18326 B/op	     225 allocs/op
PASS
ok  	go.opentelemetry.io/otel/sdk/metric	1.651s

This PR:

$ go test -bench=BenchmarkInstrumentCreation
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/sdk/metric
cpu: Intel(R) Xeon(R) CPU @ 2.20GHz
BenchmarkInstrumentCreation-24    	   27579	     44411 ns/op	   23228 B/op	     237 allocs/op
PASS
ok  	go.opentelemetry.io/otel/sdk/metric	1.745s

Which is slightly slower, but that is partially because we are doing some extra instantiation of the caches in the MeterProvider.