Asynchronous Instrument API non-compliant with OTel Specification: creation does not accept callbacks

[MrAlias]

Description

The OpenTelemetry specification states:

The API to construct asynchronous instruments MUST accept the following parameters:

• The name of the Instrument, following the instrument naming rule.
• An optional unit of measure, following the instrument unit rule.
• An optional description, following the instrument description rule.
• Zero or more callback functions, responsible for reporting Measurement values of the created instrument.

The API MUST support creation of asynchronous instruments by passing zero or more callback functions to be permanently registered to the newly created instrument.

Our current metric API does not accept "zero or more callback functions" for new async instruments.

[MrAlias]

This will break the symmetry with the sync instrument options, but it looks like the solution will be to add some WithCallback option for async instruments.

[jmacd]

The way I read the specification, the current asynchronous API is compliant. You wrote "Our current metric API does not accept zero or more callback functions" for new async instruments

That is true, however, I had parsed the "or" in that statement as allowing instrument creation with ONLY zero callback functions, to allow the current form of NewCallback(). A WithCallback() API sounds nice to have, but not absolutely necessary.

[Aneurysm9]

That is true, however, I had parsed the "or" in that statement as allowing instrument creation with ONLY zero callback functions, to allow the current form of NewCallback().

That's how I read it as well, but I seemed to be alone and I could see the other interpretation (must accept list of callbacks, even if its length is zero) as valid as well. v0v

[MrAlias]

I think it could be better specified if "or" is non-exclusive similar to the rest of the document, or it is meant to be exclusive. But I believe the intent is the common non-exclusive use. Meaning a users should be allowed to pass callback but they can also choose to not pass callbacks if they want.

Given the purpose of the specification is to maintain compatibility across implementations, and all the other stable releases I have looked at offer this, I think we need to as well.

[MrAlias]

This is going to be important to address at this point in time as the symmetry breaking between sync and async instrument options is one we need to fix now or forever live with. If we went stable without adjusting this we would not be able to support a callback option or would have to ignore it for sync instrument creations.

[jmacd]

But I believe the intent is the common non-exclusive use

😁 I think I wrote this sentence, and I wrote it after the current Go async instrument API was adopted thinking it would allow it to pass. To me, it's in the spirit of Go to reduce the number of optional ways of configuring something. I also see how WithCallback() makes life easier for developers with a small number of instruments, so I would not argue against it.

Considering this alongside #3452 I would suggest that WithCallback()-registered callbacks cannot be unregistered.

[MrAlias]

Considering this alongside #3452 I would suggest that WithCallback()-registered callbacks cannot be unregistered.

Agreed. It was pointed in the SIG meeting that the specification also says these should be "permanently registered" which motivated a consensus to agree with this there.

[MrAlias]

The approach in #3507 works well to ensure that the instrument the callback is registered for is the only instrument allowed to be updated:

opentelemetry-go/metric/instrument/instrument.go

Line 36 in 1867a8f

type Callback func(ctx context.Context, instrument Asynchronous) error

However, this also means the user needs to make a type conversion before they can observe. E.g.

_, err := meter.AsyncInt64().Gauge(
	"MemoryUsage",
	WithUnit(unit.Bytes),
	WithCallback(func(ctx context.Context, instrument Asynchronous) error {
		// Do Work to get the real memoryUsage.
		// mem := GatherMemory(ctx)
		mem := 75000

		instrument.(asyncint64.Gauge).Observe(ctx, int64(mem))
		return nil
	}),
)

This is not ideal. It requires users to understand this is needed and opens up the possibility of a panic if a bug is introduced.

It would be great if we could design a callback that will ensure that only the instrument it is registered for is updated, but also avoid having the user do this type conversion.

Alternates

Typed callback return values

type CallbackInt64 func(context.Context) (int64, error)
type CallbackFloat64 func(context.Context) (float64, error)

Which would mean that the async options would further have to be split by type. E.g.

func WithCallbackInt64(CallbackInt64) AsynchronousInt64Option
func WithCallbackFloat64(CallbackFloat64) AsynchronousFloat64Option

Using generics here would be helpful, but we are trying to not include them in the API.

This will have overhead in the SDK where these callback types will need to be stored separately.

Typed callback parameters

type AsynchronousInt64 interface {
	Asynchronous
	Observe(context.Context, int64, ...attribute.KeyValue)
}

type AsynchronousFloat64 interface {
	Asynchronous
	Observe(context.Context, float64, ...attribute.KeyValue)
}

type CallbackInt64 func(context.Context, AsynchronousInt64) error
type CallbackFloat64 func(context.Context, AsynchronousFloat64) error

func WithCallbackInt64(CallbackInt64) AsynchronousInt64Option
func WithCallbackFloat64(CallbackFloat64) AsynchronousFloat64Option

This will as well have overhead in the SDK where these callback types will need to be stored separately.

[MrAlias]

The "Typed callback return values" proposal would allow for the callbacks to be inlined and avoid a GC ref count on the passed instrument.

[MrAlias]

Splitting options and callbacks into the already existing async/sync packages allows this duplication to be a bit more reasonable. I'm going to update #3057 to use the "Typed callback return values" proposal and split the options this way.

[MrAlias]

The return value will need to be a slice of attribute to value pairs. Updating in the update.