Redesign RegisterCallback API

[MrAlias]

Fix #3519
Fix #3583

Issues

As stated in #3519 and #3583, the current RegisterCallback method and related Callback have the following issues:

• Not compliant with specification recommendations and requirements.
• Require the user to propagate a context for valid observations.
• Allow for the observation of unregistered instruments.
• Allow for the observation of instruments from other Meters.

Redesign Callback

The Callback type is updated to accept a new Observer type.

type Callback func(context.Context, Observer) error

The Observer type matches what was introduced in #3507 for single instrument callbacks. It is used to make an observation for an instrument that is passed as an argument to one of its "typed" methods.

type Observer interface {
	ObserveFloat64(obsrv instrument.Float64Observer, value float64, attributes ...attribute.KeyValue)
	ObserveInt64(obsrv instrument.Int64Observer, value int64, attributes ...attribute.KeyValue)
}

This new design allows for the SDK implementation to hold a registry of instruments a Callback is registered with. When the callback is executed the SDK can then check that the observations made by a user apply to one of these registered instruments. It is able to block observations of non-SDK/non-same-Meter instruments and doesn't require the user to complete a context passing operation.

[codecov]

Codecov Report

Merging #3584 (eec9b94) into main (f941b3a) will increase coverage by 0.1%.
The diff coverage is 92.3%.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #3584     +/-   ##
=======================================
+ Coverage   78.9%   79.0%   +0.1%     
=======================================
  Files        170     170             
  Lines      12460   12587    +127     
=======================================
+ Hits        9834    9950    +116     
- Misses      2417    2424      +7     
- Partials     209     213      +4     

Impacted Files	Coverage Δ
metric/internal/global/instruments.go	51.8% <0.0%> (-2.5%)	⬇️
sdk/metric/meter.go	86.5% <94.3%> (+7.1%)	⬆️
sdk/metric/instrument.go	95.8% <100.0%> (+2.9%)	⬆️
sdk/metric/pipeline.go	91.3% <100.0%> (ø)
metric/internal/global/meter.go	93.8% <0.0%> (-1.5%)	⬇️
exporters/jaeger/jaeger.go	90.3% <0.0%> (-0.9%)	⬇️

[MadVikingGod]

There should never be a case where users use instrument.Observe after this change. Can you link where you see that?

When you use the instrument's WithCallback() you use an [Int|Float]64Observer, and in that callback you use inst.Observe(). When you use RegisterCallback() if you use an inst.Observe() that is an error, you are instead supposed to use obs.Observe[Int|Float]64(inst, ...).

This will trip new users up.

[MadVikingGod]

The delegated instruments are still not useable, I updated the gist to the new failure point.

[MrAlias]

When you use the instrument's WithCallback() you use an [Int|Float]64Observer, and in that callback you use inst.Observe(). When you use RegisterCallback() if you use an inst.Observe() that is an error, you are instead supposed to use obs.Observe[Int|Float]64(inst, ...).

If by inst.Observe() you mean a user calls Observe on an instrument directly when they observe in a callback registered at creation, this is not true. As per your suggestion in #3507 (comment), the callbacks registered at creation are also observed via a passed parameters to the callback:

opentelemetry-go/metric/instrument/asyncfloat64.go

Lines 61 to 73 in e8c6e45

	// Float64Callback is a function registered with a Meter that makes
	// observations for a Float64Observer it is registered with.
	//
	// The function needs to complete in a finite amount of time and the deadline
	// of the passed context is expected to be honored.
	//
	// The function needs to make unique observations across all registered
	// Float64Callbacks. Meaning, it should not report measurements with the same
	// attributes as another Float64Callbacks also registered for the same
	// instrument.
	//
	// The function needs to be concurrent safe.
	type Float64Callback func(context.Context, Float64Observer) error

opentelemetry-go/metric/instrument/asyncint64.go

Lines 62 to 74 in e8c6e45

	// Int64Callback is a function registered with a Meter that makes
	// observations for an Int64Observer it is registered with.
	//
	// The function needs to complete in a finite amount of time and the deadline
	// of the passed context is expected to be honored.
	//
	// The function needs to make unique observations across all registered
	// Int64Callback. Meaning, it should not report measurements with the same
	// attributes as another Int64Callbacks also registered for the same
	// instrument.
	//
	// The function needs to be concurrent safe.
	type Int64Callback func(context.Context, Int64Observer) error

For example:

opentelemetry-go/metric/example_test.go

Lines 58 to 74 in e8c6e45

	instrument.WithInt64Callback(func(ctx context.Context, inst instrument.Int64Observer) error {
	// Do the real work here to get the real disk usage. For example,
	//
	// usage, err := GetDiskUsage(diskID)
	// if err != nil {
	// if retryable(err) {
	// // Retry the usage measurement.
	// } else {
	// return err
	// }
	// }
	//
	// For demonstration purpose, a static value is used here.
	usage := 75000
	inst.Observe(ctx, int64(usage), attribute.Int("disk.id", 3))
	return nil
	}),

[MadVikingGod]

I've checked that the delegates work. I've not had time to check the rest of the changes.

If this doesn't clear it, you can override the Request changes.