Verify compliant metric SDK specification implementation: MeterProvider/Resolving duplicate instrument registration conflicts

[MrAlias]

• Identify all the normative requirements, recommendations, and options the specification defines as comments to this issue
• Ensure the current metric SDK implementation is compliant with these normative requirements, recommendations, and options in those comments.

[MrAlias]

When more than one Instrument of the same name is created for identical Meters, denoted duplicate instrument registration, the Meter MUST create a valid Instrument in every case. Here, "valid" means an instrument that is functional and can be expected to export data, despite potentially creating a semantic error in the data model.

I'm guessing this is scoped to a single MeterProvider (see open-telemetry/opentelemetry-specification#3538).

There is an open issue tracking the casing collision of name which relates to this: #3835

In that issue it appears to report that our duplicate instrument registration will not detect instrument conflicts that have the same case-insensitive name. However, it does comply with this section still: a "valid", though incorrect, instrument is returned.

In general our duplicate instrument registration detection is done:

opentelemetry-go/sdk/metric/pipeline.go

Lines 323 to 325 in 135c64f

	// If there is a conflict, the specification says the view should
	// still be applied and a warning should be logged.
	i.logConflict(id)

With logConflict declared as:

opentelemetry-go/sdk/metric/pipeline.go

Lines 349 to 367 in 135c64f

	// logConflict validates if an instrument with the same name as id has already
	// been created. If that instrument conflicts with id, a warning is logged.
	func (i *inserter[N]) logConflict(id streamID) {
	existing := i.views.Lookup(id.Name, func() streamID { return id })
	if id == existing {
	return
	}
	global.Info(
	"duplicate metric stream definitions",
	"names", fmt.Sprintf("%q, %q", existing.Name, id.Name),
	"descriptions", fmt.Sprintf("%q, %q", existing.Description, id.Description),
	"units", fmt.Sprintf("%s, %s", existing.Unit, id.Unit),
	"numbers", fmt.Sprintf("%s, %s", existing.Number, id.Number),
	"aggregations", fmt.Sprintf("%s, %s", existing.Aggregation, id.Aggregation),
	"monotonics", fmt.Sprintf("%t, %t", existing.Monotonic, id.Monotonic),
	"temporalities", fmt.Sprintf("%s, %s", existing.Temporality.String(), id.Temporality.String()),
	)
	}

The processing of the conflict does not change the aggregation returned:

opentelemetry-go/sdk/metric/pipeline.go

Lines 323 to 346 in 135c64f

	// If there is a conflict, the specification says the view should
	// still be applied and a warning should be logged.
	i.logConflict(id)
	cv := i.aggregators.Lookup(id, func() aggVal[N] {
	agg, err := i.aggregator(stream.Aggregation, kind, id.Temporality, id.Monotonic)
	if err != nil {
	return aggVal[N]{nil, err}
	}
	if agg == nil { // Drop aggregator.
	return aggVal[N]{nil, nil}
	}
	if stream.AttributeFilter != nil {
	agg = internal.NewFilter(agg, stream.AttributeFilter)
	}
	i.pipeline.addSync(scope, instrumentSync{
	name: stream.Name,
	description: stream.Description,
	unit: stream.Unit,
	aggregator: agg,
	})
	return aggVal[N]{agg, err}
	})
	return cv.Aggregator, cv.Err

Meaning that in the case of duplicate instrument registrations a valid instrument is still created.

[MrAlias]

Based on the recommendations from the data model, the SDK MUST aggregate data from identical Instruments together in its export pipeline.

As noted above, the OTel Go implementation is not compliant. The missing name conflict aggregation is being tracked in #3835.

Otherwise, since identical [describes instances where all identifying fields are equal" and the identifying fields are:

• The name of the Instrument
• The kind of the Instrument - whether it is a Counter or one of the other kinds, whether it is synchronous or asynchronous
• An optional unit of measure
• An optional description
• note: we do not currently support advice so it is not considered here

And the cache look up key:

opentelemetry-go/sdk/metric/pipeline.go

Line 326 in 135c64f

cv := i.aggregators.Lookup(id, func() aggVal[N] {

is missing the instrument kind, and contains the additional aggregation name, monotonicity, temporality, and number:

opentelemetry-go/sdk/metric/instrument.go

Lines 152 to 171 in 135c64f

	// streamID are the identifying properties of a stream.
	type streamID struct {
	// Name is the name of the stream.
	Name string
	// Description is the description of the stream.
	Description string
	// Unit is the unit of the stream.
	Unit string
	// Aggregation is the aggregation data type of the stream.
	Aggregation string
	// Monotonic is the monotonicity of an instruments data type. This field is
	// not used for all data types, so a zero value needs to be understood in the
	// context of Aggregation.
	Monotonic bool
	// Temporality is the temporality of a stream's data type. This field is
	// not used by some data types.
	Temporality metricdata.Temporality
	// Number is the number type of the stream.
	Number string
	}

The implementation is not compliant:

• If two different instruments with the same, name, unit, kind, and description but of different number types (i.e. int64 and float64) are created they will use different aggregators

[MrAlias]

Otherwise, since identical [describes instances where all identifying fields are equal" and the identifying fields are:

• The name of the Instrument
• The kind of the Instrument - whether it is a Counter or one of the other kinds, whether it is synchronous or asynchronous
• An optional unit of measure
• An optional description
• note: we do not currently support advice so it is not considered here

And the cache look up key:

opentelemetry-go/sdk/metric/pipeline.go

Line 326 in 135c64f

cv := i.aggregators.Lookup(id, func() aggVal[N] {

is missing the instrument kind, and contains the additional aggregation name, monotonicity, temporality, and number:

opentelemetry-go/sdk/metric/instrument.go

Lines 152 to 171 in 135c64f

	// streamID are the identifying properties of a stream.
	type streamID struct {
	// Name is the name of the stream.
	Name string
	// Description is the description of the stream.
	Description string
	// Unit is the unit of the stream.
	Unit string
	// Aggregation is the aggregation data type of the stream.
	Aggregation string
	// Monotonic is the monotonicity of an instruments data type. This field is
	// not used for all data types, so a zero value needs to be understood in the
	// context of Aggregation.
	Monotonic bool
	// Temporality is the temporality of a stream's data type. This field is
	// not used by some data types.
	Temporality metricdata.Temporality
	// Number is the number type of the stream.
	Number string
	}

The implementation is not compliant:

• If two different instruments with the same, name, unit, kind, and description but of different number types (i.e. int64 and float64) are created they will use different aggregators

Tracking with #4201

[MrAlias]

When a duplicate instrument registration occurs, and it is not corrected with a View, a warning SHOULD be emitted.

Implemented here:

opentelemetry-go/sdk/metric/pipeline.go

Line 325 in 135c64f

i.logConflict(id)

This is not warning though:

opentelemetry-go/sdk/metric/pipeline.go

Lines 357 to 366 in 135c64f

	global.Info(
	"duplicate metric stream definitions",
	"names", fmt.Sprintf("%q, %q", existing.Name, id.Name),
	"descriptions", fmt.Sprintf("%q, %q", existing.Description, id.Description),
	"units", fmt.Sprintf("%s, %s", existing.Unit, id.Unit),
	"numbers", fmt.Sprintf("%s, %s", existing.Number, id.Number),
	"aggregations", fmt.Sprintf("%s, %s", existing.Aggregation, id.Aggregation),
	"monotonics", fmt.Sprintf("%t, %t", existing.Monotonic, id.Monotonic),
	"temporalities", fmt.Sprintf("%s, %s", existing.Temporality.String(), id.Temporality.String()),
	)

It was added prior to Warn being added.

[MrAlias]

When a duplicate instrument registration occurs, and it is not corrected with a View, a warning SHOULD be emitted.

Implemented here:

opentelemetry-go/sdk/metric/pipeline.go

Line 325 in 135c64f

i.logConflict(id)

This is not warning though:

opentelemetry-go/sdk/metric/pipeline.go

Lines 357 to 366 in 135c64f

	global.Info(
	"duplicate metric stream definitions",
	"names", fmt.Sprintf("%q, %q", existing.Name, id.Name),
	"descriptions", fmt.Sprintf("%q, %q", existing.Description, id.Description),
	"units", fmt.Sprintf("%s, %s", existing.Unit, id.Unit),
	"numbers", fmt.Sprintf("%s, %s", existing.Number, id.Number),
	"aggregations", fmt.Sprintf("%s, %s", existing.Aggregation, id.Aggregation),
	"monotonics", fmt.Sprintf("%t, %t", existing.Monotonic, id.Monotonic),
	"temporalities", fmt.Sprintf("%s, %s", existing.Temporality.String(), id.Temporality.String()),
	)

It was added prior to Warn being added.

#4202

[MrAlias]

The emitted warning SHOULD include information for the user on how to resolve the conflict, if possible.

1. If the potential conflict involves multiple description properties, setting the description through a configured View SHOULD avoid the warning.
2. If the potential conflict involves instruments that can be distinguished by a supported View selector (e.g., instrument type) a renaming View recipe SHOULD be included in the warning.
3. Otherwise (e.g., use of multiple units), the SDK SHOULD pass through the data by reporting both Metric objects and emit a generic warning describing the duplicate instrument registration.

The first two items are not done.

opentelemetry-go/sdk/metric/pipeline.go

Lines 357 to 366 in 135c64f

	global.Info(
	"duplicate metric stream definitions",
	"names", fmt.Sprintf("%q, %q", existing.Name, id.Name),
	"descriptions", fmt.Sprintf("%q, %q", existing.Description, id.Description),
	"units", fmt.Sprintf("%s, %s", existing.Unit, id.Unit),
	"numbers", fmt.Sprintf("%s, %s", existing.Number, id.Number),
	"aggregations", fmt.Sprintf("%s, %s", existing.Aggregation, id.Aggregation),
	"monotonics", fmt.Sprintf("%t, %t", existing.Monotonic, id.Monotonic),
	"temporalities", fmt.Sprintf("%s, %s", existing.Temporality.String(), id.Temporality.String()),
	)

[MrAlias]

TODO:

• Return the same agg for identical instruments #4201
• Warn instead of Info log instrument conflict #4202
• Emit correct warning for duplicate instrument conflict #4203
• How to handle case-insensitive instrument name collisions in metric SDK opentelemetry-specification#3539
  • metric identity and aggregation for duplicate instrument registration #3835

[dashpole]

Should this be closed?

[MrAlias]

Should this be closed?

The specification issue for duplicate name conflicts looks like it is going to resolve in a different direction then we implemented. We will likely need to track that change and adjust accordingly.

[dashpole]

Thanks. I just noticed that all the TODOs were checked. :)