Duplicate instrument registration generates an error instead of a valid instrument

[alanwest]

Description

Spec says:

When more than one Instrument of the same name is created for identical Meters, denoted duplicate instrument registration, the implementation MUST create a valid Instrument in every case.

The loop in the following code creates a duplicate instrument. An error is generated and the instrument is not valid.

Steps To Reproduce

module main

go 1.19

require (
	go.opentelemetry.io/otel v1.10.0
	go.opentelemetry.io/otel/exporters/stdout/stdoutmetric v0.32.1
	go.opentelemetry.io/otel/sdk v1.10.0
	go.opentelemetry.io/otel/sdk/metric v0.32.1
)

require (
	github.com/go-logr/logr v1.2.3 // indirect
	github.com/go-logr/stdr v1.2.2 // indirect
	go.opentelemetry.io/otel/metric v0.32.1 // indirect
	go.opentelemetry.io/otel/trace v1.10.0 // indirect
	golang.org/x/sys v0.0.0-20210510120138-977fb7262007 // indirect
)

package main

import (
	"context"
	"log"
	"time"

	"go.opentelemetry.io/otel/exporters/stdout/stdoutmetric"
	"go.opentelemetry.io/otel/sdk/metric"
)

func main() {
	ctx := context.Background()

	exporter, _ := stdoutmetric.New()

	reader := metric.NewPeriodicReader(
		exporter,
		metric.WithInterval(time.Second),
	)

	meterProvider := metric.NewMeterProvider(
		metric.WithReader(reader),
	)

	meter := meterProvider.Meter("my-meter")

	for i := 0; i < 10; i++ {
		counter, err := meter.SyncFloat64().Counter("my-counter")

		if err != nil {
			// Shouldn't get here.
			// Identical instrument registration should be allowed.
			// https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument-type-conflict-detection
			log.Fatalf("failed to initialize instrument: %v", err)
		}

		counter.Add(ctx, 12.0)
		time.Sleep(500 * time.Millisecond)
	}

	meterProvider.Shutdown(ctx)
}

[MrAlias]

I think returning an error here is potentially appropriate according to:

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). The warning helps to avoid the semantic error state described in the OpenTelemetry Metrics data model when more than one Metric is written for a given instrument name and Meter identity by the same MeterProvider.

I could also see this being a time when logging an info message with the global logger makes more sense.

That said, the returned counter is currently empty. That is definitely invalid behavior. Whether a non-nil error is returned or not, the returned counter needs to be valid.

@alanwest in your use of this. Do you just halt when this fails? Or do you handle the error in a different way.

cc @MadVikingGod

[alanwest]

in your use of this. Do you just halt when this fails? Or do you handle the error in a different way

Yes, in the code I shared I just halt, but I totally could have just logged out the error myself and continued. But, as you point out a non-nil error should still result in a valid counter.

I'm no Go engineer, so I can't say what would be idiomatic for y'all to meet the requirement: "the implementation SHOULD emit a warning to the user". I guess the question is whether a non-nil error in Go is the way a user would expect this to be handled.

In .NET we do not log anything when a duplicate instrument is "identical". We only log a warning when a duplicate instrument registration would result in a "distinct" instrument. So something like:

var meter = new Meter("my-meter")
meter.CreateCounter("my-instrument")

// Duplicate registration differs by instrument kind
meter.CreateHistogram("my-instrument")

[MrAlias]

In .NET we do not log anything when a duplicate instrument is "identical". We only log a warning when a duplicate instrument registration would result in a "distinct" instrument. So something like:

Ah, makes sense 👍

[MadVikingGod]

I did make a patch to change this behavior to closer match what java and .NET do: #3181

[MrAlias]

I was able to verify #3181 resolves this based on the example provided.

[MrAlias]

I have validated that #3238 resolves this issue based on the example example provided.

[MrAlias]

Sorry for the chatter and false starts, but I have validated that #3251 resolves this issue based on the example example provided.