Verify compliant metric API specification implementation: Instrument General Characteristics

[MrAlias]

• Identify all the normative requirements, recommendations, and options the specification defines as comments to this issue
  • Also ensure the same for the top-level "Instrument" section: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.14.0/specification/metrics/api.md#instrument
• Ensure the current metric API implementation is compliant with these normative requirements, recommendations, and options in those comments.

[MrAlias]

#3452

[MrAlias]

#3451

[MrAlias]

Language-level features such as the distinction between integer and floating point numbers SHOULD be considered as identifying.

Our instruments are partitioned on number type (int64 and float64). These are distinctions between values as they are distinct types (contained in separate packages).

[MrAlias]

When more than one Instrument of the same name is created for identical Meters [...] the implementation MUST create a valid Instrument in every case.

When more than one distinct Instrument is registered with the same name for identical Meters, the implementation SHOULD emit a warning to the user informing them of duplicate registration conflict(s).

Both of these statements apply to the implementation of the API not the API.

[MrAlias]

When more than one Instrument of the same name is created for identical Meters [...] the implementation MUST create a valid Instrument in every case.

When more than one distinct Instrument is registered with the same name for identical Meters, the implementation SHOULD emit a warning to the user informing them of duplicate registration conflict(s).

Both of these statements apply to the implementation of the API not the API.

Our no-op implementation does not comply with the second statement. This seems like a bug in the specification though.

[MrAlias]

Our no-op implementation does not comply with the second statement. This seems like a bug in the specification though.

open-telemetry/opentelemetry-specification#3071

[MrAlias]

Distinct Meters MUST be treated as separate namespaces for the purposes of detecting duplicate instrument registration conflicts.

Again this is a criteria for the implementation of the API, the API holds not state therefore it cannot detect duplicate instrument registration conflicts.

Our no-op implementation does not comply with this statement. Again though, I think this is a bug in the specification (see open-telemetry/opentelemetry-specification#3071)

[MrAlias]

Instrument names MUST conform to the following syntax [...]

It is not clear if this must be validated in the API or not. Tracking this question in open-telemetry/opentelemetry-specification#3072.

[MrAlias]

The unit is an optional string provided by the author of the Instrument. It SHOULD be treated as an opaque string from the API and SDK (e.g. the SDK is not expected to validate the unit of measurement, or perform the unit conversion).

The unit option is not validated or transformed by the API (or SDK for that matter).

opentelemetry-go/metric/instrument/config.go

Lines 63 to 69 in a54167d

	// WithUnit applies provided unit.
	func WithUnit(u unit.Unit) Option {
	return optionFunc(func(cfg Config) Config {
	cfg.unit = u
	return cfg
	})
	}

The unit package defines the Unit type over the string type meaning it provides common units but still ultimately treats the unit as an "opaque string".

opentelemetry-go/metric/unit/unit.go

Lines 17 to 25 in a54167d

	// Unit is a determinate standard quantity of measurement.
	type Unit string
	// Units defined by OpenTelemetry.
	const (
	Dimensionless Unit = "1"
	Bytes Unit = "By"
	Milliseconds Unit = "ms"
	)

Thus, our implementation complies.

[MrAlias]

If the unit is not provided or the unit is null, the API and SDK MUST make sure that the behavior is the same as an empty unit string.

In Go the default zero value of a Unit, which wraps a string, is an empty string.

Our implementation is compliant with this.

[MrAlias]

[A unit] MUST be case-sensitive (e.g. kb and kB are different units), ASCII string.

In Go the strings are case-sensitive. A Unit wraps a string and is therefore also case sensitive.

Our implementation is compliant with this.

[MrAlias]

The description is an optional free-form text provided by the author of the instrument. It MUST be treated as an opaque string from the API and SDK.

The instrument description is defined as a string directly:

opentelemetry-go/metric/instrument/config.go

Lines 25 to 28 in a54167d

	// Description describes the instrument in human-readable terms.
	func (cfg Config) Description() string {
	return cfg.description
	}

It is not validated by the API:

opentelemetry-go/metric/instrument/config.go

Lines 55 to 61 in a54167d

	// WithDescription applies provided description.
	func WithDescription(desc string) Option {
	return optionFunc(func(cfg Config) Config {
	cfg.description = desc
	return cfg
	})
	}

opentelemetry-go/metric/instrument/config.go

Lines 40 to 47 in a54167d

	// NewConfig creates a new Config and applies all the given options.
	func NewConfig(opts ...Option) Config {
	var config Config
	for _, o := range opts {
	config = o.applyInstrument(config)
	}
	return config
	}

Our implementation is compliant with this.

[MrAlias]

If the description is not provided or the description is null, the API and SDK MUST make sure that the behavior is the same as an empty description string.

The default description is an empty string. Our implementation is compliant with this.

[MrAlias]

[The description] MUST support BMP (Unicode Plane 0), which is basically only the first three bytes of UTF-8 (or utf8mb3). OpenTelemetry API authors MAY decide if they want to support more Unicode Planes.

Go strings are equivalently slices of bytes, they can reliably represent this encoding and the optional additional Unicode. It truly treats the description as an opaque string.

Our implementation is compliant with this.

[MrAlias]

The API to construct synchronous 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.

The Meter is the creator of these instruments and it does accept these parameters:

opentelemetry-go/metric/meter.go

Lines 44 to 55 in efd8a7d

	// 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.Option) (syncint64.Counter, 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.Option) (syncint64.UpDownCounter, 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.Option) (syncint64.Histogram, error)

opentelemetry-go/metric/meter.go

Lines 70 to 82 in efd8a7d

	// 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.Option) (syncfloat64.Counter, 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.Option) (syncfloat64.UpDownCounter, 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.Option) (syncfloat64.Histogram, error)

And the options are defined here:

opentelemetry-go/metric/instrument/config.go

Lines 55 to 69 in efd8a7d

	// WithDescription applies provided description.
	func WithDescription(desc string) Option {
	return optionFunc(func(cfg Config) Config {
	cfg.description = desc
	return cfg
	})
	}
	// WithUnit applies provided unit.
	func WithUnit(u unit.Unit) Option {
	return optionFunc(func(cfg Config) Config {
	cfg.unit = u
	return cfg
	})
	}

There is still the open question about the instrument name validation (mentioned here), but outside of that our implementation is compliant.

It's important to note, we are looking to change the options passed to this API in #3507, but it will preserve this API conforming to this requirement.

[MrAlias]

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 Meter is the API for creating these instruments and it accepts all these parameters except the callback functions.

opentelemetry-go/metric/meter.go

Lines 56 to 68 in efd8a7d

	// 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.Option) (asyncint64.Counter, 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.Option) (asyncint64.UpDownCounter, 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.Option) (asyncint64.Gauge, error)

opentelemetry-go/metric/meter.go

Lines 82 to 96 in efd8a7d

	Float64Histogram(name string, options ...instrument.Option) (syncfloat64.Histogram, 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.Option) (asyncfloat64.Counter, 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.Option) (asyncfloat64.UpDownCounter, 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.Option) (asyncfloat64.Gauge, error)

And the options are defined here:

opentelemetry-go/metric/instrument/config.go

Lines 55 to 69 in efd8a7d

	// WithDescription applies provided description.
	func WithDescription(desc string) Option {
	return optionFunc(func(cfg Config) Config {
	cfg.description = desc
	return cfg
	})
	}
	// WithUnit applies provided unit.
	func WithUnit(u unit.Unit) Option {
	return optionFunc(func(cfg Config) Config {
	cfg.unit = u
	return cfg
	})
	}

There is still the open question about the instrument name validation (mentioned here), but outside of that our implementation is compliant.

Currently the API only allows the creation of these instruments with zero callbacks. #3507 has been created to address the non-compliance of the API not allowing the creation of more than zero callbacks.

[MrAlias]

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 implementation is not currently compliant. It is tracked in #3451 and being addressed in #3507.

[MrAlias]

The API SHOULD support registration of callback functions associated with asynchronous instruments after they are created.

Our implementation supports this recommended feature.

opentelemetry-go/metric/meter.go

Lines 98 to 109 in efd8a7d

	// RegisterCallback registers f to be called during the collection of a
	// measurement cycle.
	//
	// If Unregister of the returned Registration is called, f needs to be
	// unregistered and not called during collection.
	//
	// The instruments f is registered with are the only instruments that f may
	// observe values for.
	//
	// If no instruments are passed, f should not be registered nor called
	// during collection.
	RegisterCallback(instruments []instrument.Asynchronous, f func(context.Context)) (Registration, error)

[MrAlias]

Where the API supports registration of callback functions after asynchronous instrumentation creation, the user MUST be able to undo registration of the specific callback after its registration by some means.

The Registration type returned from the RegisterCallback method allows users to "undo [the] registration".

opentelemetry-go/metric/meter.go

Lines 112 to 119 in efd8a7d

	// Registration is an token representing the unique registration of a callback
	// for a set of instruments with a Meter.
	type Registration interface {
	// Unregister removes the callback registration from a Meter.
	//
	// This method needs to be idempotent and concurrent safe.
	Unregister() error
	}

Our implementation is compliant.

[MrAlias]

Every currently registered Callback associated with a set of instruments MUST be evaluated exactly once during collection prior to reading data for that instrument set.

This does not seem to apply to the API, but to it's implementation.

[MrAlias]

Callback functions MUST be documented as follows for the end user:

• Callback functions SHOULD be reentrant safe. The SDK expects to evaluate callbacks for each MetricReader independently.
• Callback functions SHOULD NOT take an indefinite amount of time.
• Callback functions SHOULD NOT make duplicate observations (more than one Measurement with the same attributes) across all registered callbacks.

Our implementation is currently not compliant.

• Add single instrument callback and split metric instrument configuration #3507 is adding documentation for callbacks passed to instruments on creation that include information about the last two items.

  opentelemetry-go/metric/instrument/asyncfloat64.go

  Lines 37 to 44 in 013a228

  	// 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 be concurrent safe.
  	type Float64Callback func(context.Context, Float64Observer) error

  opentelemetry-go/metric/instrument/asyncint64.go

  Lines 38 to 45 in 013a228

  	// 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 be concurrent safe.
  	type Int64Callback func(context.Context, Int64Observer) error

  (I'm interpreting concurrent safe is a Go-ism for what the specification calls reentrant safe). It will need to be updated to address the other item (non-duplicative).
• A new issues is needed to track the task of documenting the callback passed to the RegisterCallback method.

[MrAlias]

• Add single instrument callback and split metric instrument configuration #3507 is adding documentation for callbacks passed to instruments on creation that include information about the last two items. [...] It will need to be updated to address the other item (non-duplicative).

Updated in 2fcf8cd

[MrAlias]

• A new issues is needed to track the task of documenting the callback passed to the RegisterCallback method.

Tracking in #3563

[MrAlias]

The resulting behavior when a callback violates any of these RECOMMENDATIONS is explicitly not specified at the API level.

OpenTelemetry APIauthors MAY decide what is the idiomatic approach for capturing measurements from callback functions.

These both look like incorrect usages of normative key words. For what it's worth, we do decide on an idiomatic approach to capturing measurements 🤷 .

[MrAlias]

Callbacks registered at the time of instrument creation MUST apply to the single instruments which is under construction.

Currently we do not comply with this. It is tracked in #3451 and being addressed in #3507.

[MrAlias]

Callbacks registered after the time of instrument creation MAY be associated with multiple instruments.

We allow this:

opentelemetry-go/metric/meter.go

Line 109 in efd8a7d

RegisterCallback(instruments []instrument.Asynchronous, f func(context.Context)) (Registration, error)

[MrAlias]

Idiomatic APIs for multiple-instrument Callbacks MUST distinguish the instrument associated with each observed Measurement value.

Our API currently does not do this. It assumes the association is correctly done within a Callback by a user.

I think our API may be out of compliance here. It seems relevant to #3519, but that issue may be more motivated now that it has been identified our implementation is out of compliance.

[MrAlias]

Multiple-instrument Callbacks MUST be associated at the time of registration with a declared set of asynchronous instruments from the same Meter instance.

Our API is designed to support this. The related instruments should be passed with the instruments they expect to update. However, this is not enforced through any code.

[MrAlias]

The API MUST treat observations from a single Callback as logically taking place at a single instant, such that when recorded, observations from a single callback MUST be reported with identical timestamps.

Again, this looks to be an implementation detail. The API does not call the Callback in a collection cycle, the SDK does.

[MrAlias]

The API SHOULD provide some way to pass state to the callback.

The parameter state is under-defined. It is not clear what constitutes as state, but we do pass a context.Context. In a general, non OTel specific, understanding this would seem to satisfy this criteria. Given the lack of OTel specific definition, it looks this recommendation is satisfied.

[MrAlias]

OpenTelemetry API authors MAY decide what is the idiomatic approach (e.g. it could be an additional parameter to the callback function, or captured by the lambda closure, or something else).

Not really a normative option, but we do make a decision here. 🤷

[MrAlias]

Our no-op implementation does not comply with the second statement. This seems like a bug in the specification though.

The specification is going to move slower than we can. I've opened #3565 to track a local fix.

[MrAlias]

Remaining tasks:

• Open question: Are metric APIs expected to validate instrument names? opentelemetry-specification#3072
• Asynchronous Instrument API non-compliant with OTel Specification: creation does not accept callbacks #3451
• Add single instrument callback and split metric instrument configuration #3507
• Document the callback registered with a Meter according to the specification #3563
• Redesign of Asynchronous Instrument Registration API via the Meter #3519
• Log a warning for duplicate instrument creation from the No-op Meter #3565

[MrAlias]

All outstanding items have been resolved. Closing.