[API Epic 1/4] Metric api prototype

[MadVikingGod]

This is a series of patches to update the metrics API.

1. New API (This patch)
2. Move retained packages under SDK [API Epic 2/4] Move packages to sdk #2585
3. Create a wrapper for current SDK to be used as new API [API EPIC 3/4] SDK Wrapper #2586
4. Fix remaining tests and examples. [API EPIC 4/4] Fix tests and examples #2587

This change is ONLY the changes in the API package. It leaves portions of the repository[1] in a broken state

This is designed to capture feedback around #2526.

How is this different from the original proposal:

• The instruments are under the metric/instrument package. This was done to provide more hints to what is and is not an insturment
• The Asynchronous and Synchronous are now unexported methods. This means that ALL instruments must embed these interfaces, but it also reduces the API surface area. This was done in anticipation of questions like "What does Synchronous() do?" "Nothing it is used to group instruments"
• I added a Nonrecording API. This is not strictly required by the spec, but is useful for creating examples, and for the default when we create a global API.
• Change the name and shape of NewCallback, now RegisterCallback

When doing this work I also found that this API would not be compliant with the spec. The spec requires that Asynchronous instruments (all flavors) accept a Callback Function as a creation parameter. Right now callbacks can only be registered via the Meter.RegisterCallback, and these callback will have a different shape from one registered at creation. The reason is a closure at creation won't be able to close over the instrument, so it will have to take the instrument as a parameter. For example:

counter, err := meter.AsyncInt64().Counter("Example",
    instrument.WithCallback(func(ctx context.Context, ctr asyncint64.Counter) { // <== This func can't have the same form as the Meter Callback
        // This is because you can't create a closure over counter.
        // There could be a work around here, but that might depend on how it's implemented in the sdk.

        // Normal Callback work
       ctr.Observe(ctx, 2)
})

[1] Portions of the SDK broken by this change
// Shouldn't require the sdk
bridge/opencensus
internal/metric // Probably will be removed
// Actual SDK changes
sdk/metric
sdk/export/metric
// Requires the sdk/metric Exporter interface
exporters/prometheus
exporters/otlp/otlpmetric
exporters/stdout/stdoutmetric
exporters/otlp/otlpmetric/otlpmetrichttp
exporters/otlp/otlpmetric/otlpmetricgrpc
// Requires an exporter
example/opencensus
example/prometheus

[jmacd]

The instruments are under the metric/instrument package.

Looks good 👍

The Asynchronous and Synchronous are now unexported methods.

Nice, I haven't seen this trick before 💥

I added a Nonrecording API.

This raises a flag or two--depends on how the global package is structured. I think there have been complaints that Noop implementations clutter the documentation, but I don't object.

Change the name and shape of NewCallback

Looks good (no error, 👍).

these callback will have a different shape from one registered at creation

Yeah, I admit I don't like having two ways to do this, and I'd rather have batch observations than singleton observations if I am forced to choose. The existing API's support for both also sickens me, thus my support for changing the specification. (open-telemetry/opentelemetry-specification#2281)

The instrument.WithCallback(func(ctx context.Context, ctr asyncint64.Counter)) lets us use context consistently in both forms of callback (i.e., with RegisterCallback), but to me feels like unnecessary indirection. The pattern found in the existing API originates in OpenCensus, the idea being func(recorder interface{ Observe(value float64, attrs ...attribute.KeyValue) }) or func(recorder interface{ Observe(value int64, attrs ...attribute.KeyValue) }). Of course then, the user doesn't have a need for the instrument object that is returned for use w/ RegisterCallback, and the value returned from the constructor is confusingly useless.

I have weakly mixed feelings at this point. Providing singleton observation APIs seems to add more surface area while admittedly making the user's life easier.

(Public service announcement: I'm working on a change to the SDK to meet @MadVikingGod in the middle here, in parallel. Thanks @MadVikingGod for taking on the API-side effort and really digging in!)

[MadVikingGod]

This has been open for a bit, I'm bumping to see if I could get any feedback. I know it's a lot of removed code, and I'm sorry.

Where I would especially like feedback is:

1. Are the docstrings enough? Should there be any more, what is missing?
2. Are the examples sufficient? I created three simple examples of sync and async instruments. Would this be an API that you could use?
3. Do we need a way to register a callback at instrument creation, or is the meter level callback acceptable?

[jmacd]

@MadVikingGod for your question 3 specifically,

Do we need a way to register a callback at instrument creation, or is the meter level callback acceptable?

I'm looking at whether there could be an option to do this that is semantic sugar for creating a meter-level callback and binding it in a simpler API. This leads back to a topic about generics, since we have both integer and floating-point cases.

There's a question of whether the instrument options need to become int64/float64 type-aware. If so, we could have a type-specific option meant for use with asynchronous instruments. (A caveat is that if we come to a discussion about unregistering callbacks at the OTel specification level, we'll have to craft something that lets you unregister them too.)

For example, in the instrument package:

type SingleObserver[T int64 | float64] interface {
	Observe(value T, attrs ...attribute.KeyValue)
}

type AnyObserver[T int64 | float64] interface {
	Observe(ctx context.Context, value T, attrs ...attribute.KeyValue)
	Asynchronous
}

(for convenience, I have also used:)

func Must[T any](t T, e error) T {
	if e != nil {
		panic(e)
	}
	return t
}

An adapter type:

type singleton[T int64 | float64] struct {
	ctx context.Context
	obs AnyObserver[T]
}

func (s singleton[T]) Observe(value T, attrs ...attribute.KeyValue) {
	s.obs.Observe(s.ctx, value, attrs...)
}

The semantic sugar happens here :

func RegisterSingleCallback[T int64 | float64](m metric.Meter, obs AnyObserver[T], f func(SingleObserver[T])) (metric.Callback, error) {
	return m.RegisterCallback([]instrument.Asynchronous{obs}, func(ctx context.Context) {
		f(singleton[T]{
			ctx: ctx,
			obs: obs,
		})
	})
}

and it would be used like:

func setup(meter metric.Meter) {

	instrument.RegisterSingleCallback[int64](
		meter,
		instrument.Must(meter.AsyncInt64().Counter("Example")),
		func(ctr instrument.SingleObserver[int64]) {
			ctr.Observe(2)
		},
	)
}

[MadVikingGod]

For the ability to register a single callback what about this API: Callback API ?

Could that be implemented by the SDK? I would assume that at instrument creation time the meter would check if config.callback is empty and the instrument is async (maybe we need to split async and sync configs?), and if there is a callback just wrap with something akin to

//inst successfully created already 
m.RegisterCallback(func(ctx context) {
    config.callback(ctx, inst)
}

[jmacd]

what about this API (link)?

My first sense is that it's a lot of typing for the API author 😀 .

I like the idea that the instrument is the observer, instead of the SingleObserver in my example -- I had removed the context.Context argument in my generic example above, but I think we want to standardize timeouts for these callbacks. This suggests that if we are going to avoid generics, there will be 6 different single-option callbacks. It feels like a lot of extra work to (a) make sure callback options aren't used w/ sync instruments by mistake, (b) make sure int callbacks aren't used w/ float instruments, etc.

Could that be implemented by the SDK?

Yes, as you described. It sounds like the Config struct would have two new fields,

type Config struct {
  Description string. // existing
  Unit unit.Unit. // existing
  IntObserver interface {
    Asynchronous
    Observe(context.Context, int64, ...attribute.KeyValue)
  }
  FloatObserver interface {
    Asynchronous
    Observe(context.Context, float64, ...attribute.KeyValue)
  }
}

and the SDK would be required to internally register a callback and provide the simple adapters needed. 👍

[MadVikingGod]

So I forgot to include a config.Callback() retrieval function. But just like the API requires the SDK to manage the Description and Unit from the config it would be the responsibility of the SDK to register the callback at instrument creation.

My hangup on this approach is that instrument Options are now split between the package instrument, asyncint64, and asyncfloat64, which is a mess. I don't think it would be very approachable.

[jmacd]

Another idea: have each asynchronous instrument support an additional RegisterCallback() method for callbacks on a single instrument. You could rewrite a single-callback w/ a loop like this:

meter.AsyncInt64().Gauge("name", WithCallback(func (...) {
   for _, thing := range things {
      obs.Observe(thing.value, attribute.String("thing.name", thing.name))
   }
}))

by multiple single-instrument callbacks:

gauge, _ := meter.AsyncInt64().Gauge("name")

for i := range things {
  gauge.RegisterCallback(func (...) {
      obs.Observe(things[i].value, attribute.String("thing.name", things[i].name))
  })
}

[jmacd]

TL;DR

I still approve this PR. There's discussion above on how to make the instruments a little more convenient for singleton use.

The specification proposal in open-telemetry/opentelemetry-specification#2317 suggests that callbacks could be unregistered, and gives the languages freedom on how to make this idiomatic. If accepted, I'd argue the RegisterCallback() mechanism here to be the primitive one (returns (Callback, error)) and we could add a per-instrument helper (that would also return (Callback, error)). Callback would be interface { Unregister() error }.

[MadVikingGod]

So I don't see an appreciable difference between meter.RegisterCallback() and meter.AsyncInt64().RegisterCallback(), if we were to go down that path, I would suggest we just stick with the single Register at the meter level.
The proposal I had was to have an InstrumentOption, so that the call would be something along the lines of meter.AsyncInt64().Counter("stuff", asynint64.WithCallback(...)). I could see two paths forward for that kind of change:

1. You can't unregister those. If you need to unregister, use the meter.RegisterCallback().
2. Change the WithCallback() to return (InstrumentOption, Callback). This would mean that you can't use the form I outlined above. This would also be a departure from ALL of the other Options we have defined, not a show stopper, but something to consider.

I wonder why is it the Callback, or more precisely the token returned, responsible for the unregistration? This would seem more natural the provenance of the Meter than of some token or of the instrument. Now if that is the case would it be possible to use the function as the token itself?

[jmacd]

Now if that is the case would it be possible to use the function as the token itself?

I think the answer is no, you can't compare functions (compile time) and attempts to hash them at runtime panic.

https://go.dev/play/p/r_lE0t_-CEs

I don't care whether Callback is an interface { Unregister() error } or the alternative, where Callback is a placeholder interface and Meter supports interface { Unregister(Callback) error }.

[MadVikingGod]

On more thought on the topic of Unregister, what if we had a different method for registering a callback that can be canceled? This solves the use case of Unregistering callbacks, and also has the benefit that most users won't have to deal with the callback token unless they need to use the feature.

So my proposal is to have:

type Meter interface{
...
RegisterCallback() error
RegisterCancelableCallback() (CallbackToken, error)
Unregister(CallbackToken) error
}
type CallbackToken interface{}

[bogdandrutu]

I think this is not a good package name for the functionality. It does not reflect any "metric" functionality. @rakyll would like your input here, since I know you have good input for these cases.

[rakyll]

Bogdan, do you think splitting/organizing packages into type based units like here is a problem? Even though the package names don't represent what's in them, I don't have a good suggestion. asyncfloat64instrument would be too verbose and putting all instruments in the same package might be too big. I think the choice here is a fair compromise if we need to split instruments into multiple packages. Disclaimer: I didn't have much time to think to come up with a better structure.

[bogdandrutu]

In golang the type name is not very important since majority of the times you don't use that in the assignments or declaration. That is one of the main reason I believe that the split in the package base on the input type does not make sense.

I think something like 'instrument.Float64Counter' and 'instrument.Float64ObserverCounter' is better because of that

[jmacd]

@bogdandrutu There are 12 instruments in total, (int64, float64) x (6 instruments). This prototype has them in 4 packages, 3 instruments each. How many packages would you propose having?

[MadVikingGod]

So I created a change that explored this approach. Here.

There are pros and cons to both approaches. There is also a takeaway that I will be incorporating into this patch.

The pros for grouping everything into the instrument package is that when looking for the kinds of instruments they are all documented together. This is a nice feature if we can have minimal other things in the package.
The downside of that approach is that the names become very unwieldy.
What is now a

asyncfloat64.UpDownCounter

Becomes a

insturment.AsyncFloat64UpDownCounter

The other pro is that we could get rid of the namespacing we do, the Meter Interface would then become the 12 Create functions + register. This was discussed in #2526, and this API in the PR is made this way because of that discussion.

The one takeaway I have is that the nonrecording implementations can be extracted into a separate package. That I will incorporate regardless of this discussion.

[bogdandrutu]

Personally I prefer the one package approach, and that package can either be "metric" where Meter and MeterProvider are defined.

I also believe that "Sync" can be removed from the names to shorter the common case:

+ Meter
  - ObservableInt64Counter(string, ...InstrumentOption) ObservableInt64Counter
  - ObservableInt64UpDownCounter(string, ...InstrumentOption) ObservableInt64UpDownCounter
  - ObservableInt64Gauge(string, ...InstrumentOption) ObservableInt64Gauge

  - ObservableFloat64Counter(string, ...InstrumentOption) ObservableFloat64Counter
  - ObservableFloat64UpDownCounter(string, ...InstrumentOption) ObservableFloat64UpDownCounter
  - ObservableFloat64Gauge(string, ...InstrumentOption) ObservableFloat64Gauge

  - Int64Counter(string, ...InstrumentOption) Int64Counter
  - Int64UpDownCounter(string, ...InstrumentOption) Int64UpDownCounter
  - Int64Histogram(string, ...InstrumentOption) Int64Histogram

  - Float64Counter(string, ...InstrumentOption) Float64Counter
  - Float64UpDownCounter(string, ...InstrumentOption) Float64UpDownCounter
  - Float64Histogram(string, ...InstrumentOption) Float64Histogram

[XSAM]

I slightly prefer the one package approach. End-users can find all instruments in the same place. When the generics feature is ready, we can have intuitive naming for generics functions without creating a new package:

I have no opinion for Async/Observable naming.

+ Meter
  - AsyncCounter(T, ...InstrumentOption) AsyncCounter
  - AsyncUpDownCounter(T, ...InstrumentOption) AsyncUpDownCounter
  - AsyncGauge(T, ...InstrumentOption) AsyncGauge

  - Counter(T, ...InstrumentOption) Counter
  - UpDownCounter(T, ...InstrumentOption) UpDownCounter
  - Histogram(T, ...InstrumentOption) Histogram

[MadVikingGod]

When reviewing this there is one place we do stand out from the spec. The spec suggests (MAY) using Int64Counter and Int64ObservableCounter for what we call syncint64.Counter, and asyncint64.Counter.

I think using async vs observable is an acceptable departure because go already has a built-in async primitive go so we have no chance of async being confused with async in the rust/js way.

[dashpole]

Non-blocking feedback. Looks clean.

[MrAlias]

I'm with @dashpole on slightly prefering InstrumentProvider, but not blocking.

[MadVikingGod]

With the two preferences and no preference the other way I'll change it to InstrumentProvider

[bogdandrutu]

@MadVikingGod

When reviewing this there is one place we do stand out from the spec. The spec suggests (MAY) using Int64Counter and Int64ObservableCounter for what we call syncint64.Counter, and asyncint64.Counter.

Agree with your reasoning for the async, but I see very small value (I think it hurts actually) in having "sync" prefix, so because of that I think going with the recommendation "nothing for sync, and use observable for async" makes more sense, and shorten the common case.