[proposal] Move instrument creation configuration to otel/metric

[MrAlias]

Background

Currently the following declarations are made in the go.opentelemetry.io/otel/metric/instrument:

• Options:
  • Int64CounterOption
  • Int64UpDownCounterOption
  • Int64HistogramOption
  • Int64ObservableCounterOption
  • Int64ObservableUpDownCounterOption
  • Int64ObservableGaugeOption
  • Float64CounterOption
  • Float64UpDownCounterOption
  • Float64HistogramOption
  • Float64ObservableCounterOption
  • Float64ObservableUpDownCounterOption
  • Float64ObservableGaugeOption
• Configs:
  • Int64CounterConfig
  • Int64UpDownCounterConfig
  • Int64HistogramConfig
  • Int64ObservableCounterConfig
  • Int64ObservableUpDownCounterConfig
  • Int64ObservableGaugeConfig
  • Float64CounterConfig
  • Float64UpDownCounterConfig
  • Float64HistogramConfig
  • Float64ObservableCounterConfig
  • Float64ObservableUpDownCounterConfig
  • Float64ObservableGaugeConfig
• New config funcs:
  • NewInt64CounterConfig
  • NewInt64UpDownCounterConfig
  • NewInt64HistogramConfig
  • NewInt64ObservableCounterConfig
  • NewInt64ObservableUpDownCounterConfig
  • NewInt64ObservableGaugeConfig
  • NewFloat64CounterConfig
  • NewFloat64UpDownCounterConfig
  • NewFloat64HistogramConfig
  • NewFloat64ObservableCounterConfig
  • NewFloat64ObservableUpDownCounterConfig
  • NewFloat64ObservableGaugeConfig
• Option creation funcs:
  • WithUnit
  • WithDescription
  • WithInt64Callback
  • WithFloat64Callback

While each option and config are related to an instrument defined in go.opentelemetry.io/otel/metric/instrument, they are never used there. The options are all used when calling a method from a Meter in the go.opentelemetry.io/otel/metric package:

opentelemetry-go/metric/meter.go

Lines 54 to 106 in eb2b89f

	// Int64Counter returns a new instrument identified by name and configured
	// with options. The instrument is used to synchronously record increasing
	// int64 measurements during a computational operation.
	Int64Counter(name string, options ...instrument.Int64CounterOption) (instrument.Int64Counter, error)
	// Int64UpDownCounter returns a new instrument identified by name and
	// configured with options. The instrument is used to synchronously record
	// int64 measurements during a computational operation.
	Int64UpDownCounter(name string, options ...instrument.Int64UpDownCounterOption) (instrument.Int64UpDownCounter, error)
	// Int64Histogram returns a new instrument identified by name and
	// configured with options. The instrument is used to synchronously record
	// the distribution of int64 measurements during a computational operation.
	Int64Histogram(name string, options ...instrument.Int64HistogramOption) (instrument.Int64Histogram, error)
	// Int64ObservableCounter returns a new instrument identified by name and
	// configured with options. The instrument is used to asynchronously record
	// increasing int64 measurements once per a measurement collection cycle.
	Int64ObservableCounter(name string, options ...instrument.Int64ObservableCounterOption) (instrument.Int64ObservableCounter, error)
	// Int64ObservableUpDownCounter returns a new instrument identified by name
	// and configured with options. The instrument is used to asynchronously
	// record int64 measurements once per a measurement collection cycle.
	Int64ObservableUpDownCounter(name string, options ...instrument.Int64ObservableUpDownCounterOption) (instrument.Int64ObservableUpDownCounter, error)
	// Int64ObservableGauge returns a new instrument identified by name and
	// configured with options. The instrument is used to asynchronously record
	// instantaneous int64 measurements once per a measurement collection
	// cycle.
	Int64ObservableGauge(name string, options ...instrument.Int64ObservableGaugeOption) (instrument.Int64ObservableGauge, error)
	// Float64Counter returns a new instrument identified by name and
	// configured with options. The instrument is used to synchronously record
	// increasing float64 measurements during a computational operation.
	Float64Counter(name string, options ...instrument.Float64CounterOption) (instrument.Float64Counter, error)
	// Float64UpDownCounter returns a new instrument identified by name and
	// configured with options. The instrument is used to synchronously record
	// float64 measurements during a computational operation.
	Float64UpDownCounter(name string, options ...instrument.Float64UpDownCounterOption) (instrument.Float64UpDownCounter, error)
	// Float64Histogram returns a new instrument identified by name and
	// configured with options. The instrument is used to synchronously record
	// the distribution of float64 measurements during a computational
	// operation.
	Float64Histogram(name string, options ...instrument.Float64HistogramOption) (instrument.Float64Histogram, error)
	// Float64ObservableCounter returns a new instrument identified by name and
	// configured with options. The instrument is used to asynchronously record
	// increasing float64 measurements once per a measurement collection cycle.
	Float64ObservableCounter(name string, options ...instrument.Float64ObservableCounterOption) (instrument.Float64ObservableCounter, error)
	// Float64ObservableUpDownCounter returns a new instrument identified by
	// name and configured with options. The instrument is used to
	// asynchronously record float64 measurements once per a measurement
	// collection cycle.
	Float64ObservableUpDownCounter(name string, options ...instrument.Float64ObservableUpDownCounterOption) (instrument.Float64ObservableUpDownCounter, error)
	// Float64ObservableGauge returns a new instrument identified by name and
	// configured with options. The instrument is used to asynchronously record
	// instantaneous float64 measurements once per a measurement collection
	// cycle.
	Float64ObservableGauge(name string, options ...instrument.Float64ObservableGaugeOption) (instrument.Float64ObservableGauge, error)

Issue

Having these configuration options exist in a package that they are not used by seems incorrect.

1. Adds import to API users
   • A user will have to import the go.opentelemetry.io/otel/metric to create a meter and an instrument, but they will also have to import go.opentelemetry.io/otel/metric/instrument to configure the instrument they are creating.
   • If the options for an instrument lived in go.opentelemetry.io/otel/metric a user may not need to import go.opentelemetry.io/otel/metric/instrument
2. Adds cognitive overhead to new API users learning how to create an instrument
   • New users reading the documentation on how to create an instrument will have to go to a different package to learn how to configure that instrument.
   • If the options lived in go.opentelemetry.io/otel/metric a user would be able to read the documentation of that package alone and understand how to create and configure an instrument
3. Bloats the go.opentelemetry.io/otel/metric/instrument package
   • Having 40 declarations in go.opentelemetry.io/otel/metric/instrument that are not used by go.opentelemetry.io/otel/metric/instrument distracts from the things that the package is dedicated to providing (instrument types).

Proposal

Move all of the listed configuration declarations to go.opentelemetry.io/otel/metric.

[MrAlias]

cc @pellared @seh

[pellared]

In my opinion, it would make sense if we also move the instrument types. For example a type like instrument.Int64Counter which is returned by Int64Counter(name string, options ...instrument.Int64CounterOption) (instrument.Int64Counter, error).

If we do not do that the API users would probably still need to import the instrument package in order to pass the instrument objects between functions in the instrumentation libraries or application code (for "domain" metrics). Example: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/e0d1de8735ab70a480002a1972e8ffa15e7f46f9/instrumentation/net/http/otelhttp/handler.go#L53-L54.

As far as I see, if we move these types it would simply mean that we would move the whole instrument package into metric. Any reasons why we would not like to do that?

[pellared]

It would mean that two "levels" of option would leave in a single package. Options for creating a meter and options for creating instruments. I am a little afraid of a possible name collision of those two options (like WithAttributes) therefore maybe we should have some different pattern for naming these options? E.g. the instrument may not need the With prefix. Then the usage could look like:

usage, err := meter.Int64ObservableUpDownCounter(
	"db.client.connections.usage",
	metric.Unit("{connection}"),
	metric.Description("The number of connections that are currently in state described by the state attribute"),
)

Similar pattern is adopted in https://pkg.go.dev/google.golang.org/grpc

[MrAlias]

It would mean that two "levels" of option would leave in a single package. Options for creating a meter and options for creating instruments. I am a little afraid of a possible name collision of those two options (like WithAttributes) therefore maybe we should have some different pattern for naming these options? E.g. the instrument may not need the With prefix. Then the usage could look like:

usage, err := meter.Int64ObservableUpDownCounter(
	"db.client.connections.usage",
	metric.Unit("{connection}"),
	metric.Description("The number of connections that are currently in state described by the state attribute"),
)

Similar pattern is adopted in https://pkg.go.dev/google.golang.org/grpc

I'm hesitant to deviate from our established options pattern here. I don't see how we can normatively state how option should be named if we sometimes allow this kind of exception. There isn't a reason that I can see that will explain what we have done to this point and still allow this kind of deviation.

[MrAlias]

With the reduction of measurement options added in #3971 reduced to 3 (9 total added declarations), this move is less compelling. There still seems like a motivation to want the types and functions to be in the same package they are used, but there is less bloat to otel/metric/instrument.

I think if we wanted to move the instrument configuration we will likely also want to move the whole instrument package. There will be little left of instrument if we do not.

[seh]

I've read that meters have instruments, but it's not clear to me yet whether API callers care about the distinction. Forcing mention of the separate package into calling code is a distraction. The separation may be useful to SDK authors. If it is, why is that so?

[MrAlias]

If it is, why is that so?

It doesn't make too much of a difference for an SDK. They need to import two package as well in the implementation.

I'm pretty sure the original reason for this was just to reduce the docs surface area. Which I'm reconsidering as a negative as it requires looking elsewhere for the full docs.

[seh]

In order to justify the separation of packages, I'd look for things that instrument doesn't export that metric shouldn't know about, or cases where a caller would use only instrument and isn't concerned with anything in metric. If we don't have either of those situations arising, I don't think the separation is justified. Adjust some type or function names may be necessary.

[MrAlias]

In order to justify the separation of packages, I'd look for things that instrument doesn't export that metric shouldn't know about, or cases where a caller would use only instrument and isn't concerned with anything in metric. If we don't have either of those situations arising, I don't think the separation is justified. Adjust some type or function names may be necessary.

As far as I know, neither of these conditions exist.

The flattening is prototyped in #4018 if you want to take a look.