Creating repeated Float64ObservableGauge with the same name, but different callbacks the results in only one callback being used

[Quinn-With-Two-Ns]

Description

When creating multiple Float64ObservableGauge with the same name, but different callbacks, it appears like only the first callback is used. This didn't used to be the case I tested in v1.21.0 and in that version both callbacks are called, the behaviour is also different if the callbacks are given as options when creating the gauge vs registering later which is very confusing.

I would like to understand:

1. Was this an intentional breaking change? I couldn't find anything in the release notes, but maybe I missed it.
2. Can I rely on the 2nd test behaviour or could that change as well?

Environment

• OS: Mac OS
• Architecture: ARM
• Go Version: 1.21.1
• opentelemetry-go version:v1.21.0 and v1.26.0

Steps To Reproduce

I created two tests in v1.21.0 both these test pass, in version v1.23.0 and v1.26.0 the first test fails since only the first callback is registered

Note: it is possible the behaviour changes in v1.22.0, I couldn't get this version using go get for some reason

package oteltest

import (
	"context"
	"testing"

	"go.opentelemetry.io/otel/attribute"
	sdkmetric "go.opentelemetry.io/otel/sdk/metric"
	"go.opentelemetry.io/otel/sdk/metric/metricdata"
	"go.opentelemetry.io/otel/sdk/metric/metricdata/metricdatatest"

	"github.com/stretchr/testify/assert"
	"go.opentelemetry.io/otel/metric"
)

func TestRepeatedObservableGaugesWithTheSameName(t *testing.T) {
	ctx := context.Background()
	metricReader := sdkmetric.NewManualReader()
	meterProvider := sdkmetric.NewMeterProvider(sdkmetric.WithReader(metricReader))
	meter := meterProvider.Meter("test")

	_, err := meter.Float64ObservableGauge("test-metric-name",
		metric.WithFloat64Callback(func(ctx context.Context, o metric.Float64Observer) error {
			o.Observe(1.0, metric.WithAttributeSet(attribute.NewSet(attribute.String("tag1", "value1"))))
			return nil
		}))
	assert.NoError(t, err)

	_, err = meter.Float64ObservableGauge("test-metric-name",
		metric.WithFloat64Callback(func(ctx context.Context, o metric.Float64Observer) error {
			o.Observe(5.0, metric.WithAttributeSet(attribute.NewSet(attribute.String("tag1", "value2"))))
			return nil
		}))
	assert.NoError(t, err)

	// Assert result
	var rm metricdata.ResourceMetrics
	metricReader.Collect(ctx, &rm)
	assert.Len(t, rm.ScopeMetrics, 1)
	metrics := rm.ScopeMetrics[0].Metrics
	assert.Len(t, metrics, 1)
	want := metricdata.Metrics{
		Name: "test-metric-name",
		Data: metricdata.Gauge[float64]{
			DataPoints: []metricdata.DataPoint[float64]{
				{
					Value:      1.0,
					Attributes: attribute.NewSet(attribute.String("tag1", "value1")),
				},
				{
					Value:      5.0,
					Attributes: attribute.NewSet(attribute.String("tag1", "value2")),
				},
			},
		},
	}
	metricdatatest.AssertEqual(t, want, metrics[0], metricdatatest.IgnoreTimestamp())
}

func TestRepeatedObservableGaugesWithTheSameNameAndCallbacks(t *testing.T) {
	ctx := context.Background()
	metricReader := sdkmetric.NewManualReader()
	meterProvider := sdkmetric.NewMeterProvider(sdkmetric.WithReader(metricReader))
	meter := meterProvider.Meter("test")

	og1, err := meter.Float64ObservableGauge("test-metric-name")
	assert.NoError(t, err)

	_, err = meter.RegisterCallback(func(ctx context.Context, o metric.Observer) error {
		o.ObserveFloat64(og1, 1.0, metric.WithAttributeSet(attribute.NewSet(attribute.String("tag1", "value1"))))
		return nil
	}, og1)
	assert.NoError(t, err)

	og2, err := meter.Float64ObservableGauge("test-metric-name")
	assert.NoError(t, err)

	_, err = meter.RegisterCallback(func(ctx context.Context, o metric.Observer) error {
		o.ObserveFloat64(og2, 5.0, metric.WithAttributeSet(attribute.NewSet(attribute.String("tag1", "value2"))))
		return nil
	}, og2)
	assert.NoError(t, err)

	// Assert result
	var rm metricdata.ResourceMetrics
	metricReader.Collect(ctx, &rm)
	assert.Len(t, rm.ScopeMetrics, 1)
	metrics := rm.ScopeMetrics[0].Metrics
	assert.Len(t, metrics, 1)
	want := metricdata.Metrics{
		Name: "test-metric-name",
		Data: metricdata.Gauge[float64]{
			DataPoints: []metricdata.DataPoint[float64]{
				{
					Value:      1.0,
					Attributes: attribute.NewSet(attribute.String("tag1", "value1")),
				},
				{
					Value:      5.0,
					Attributes: attribute.NewSet(attribute.String("tag1", "value2")),
				},
			},
		},
	}
	metricdatatest.AssertEqual(t, want, metrics[0], metricdatatest.IgnoreTimestamp())
}

Expected behavior

I would expect both tests to pass. Since the spec says

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.

And I don't see anything in the spec as to why the 2nd callback should not be registered just because a gauge with the same name already exists. Did I miss something?

[terinjokes]

I'm running into this same issue with Int64ObservableUpDownCounters. I want to link #4884 as being related as it seems to have been introduced at the same time, and was resolved in February. /cc @MrAlias

In my use case I have an instrumentation that wraps different instances of a component. Each instance of my instrumentation creates new meters, and I'd expect the callbacks for each to be called. Instead, only the callbacks of the first instance are called.

[sirianni]

We are running into the same issue. @MrAlias any guidance here? We'd appreciate a quick look since this is a regression and causing teams in my organization to lose confidence in the OTel library. Thanks!

[pellared]

Hi. First of all, I want to say that I feel sorry that you had to wait so long for any feedback.

I am leaning towards saying that this is an incorrect use of the API.

The created instruments are identified by https://pkg.go.dev/go.opentelemetry.io/otel/sdk/instrumentation#Scope. This comes from https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument:

The term identical applied to an Instrument describes instances where all identifying fields are equal.

Because both meters have the same name test-metric-name (and no other parameters) the second call returns the previous meter without adding a callback.

However, maybe in such case we still should add the callback to the existing meter?

Can I rely on the 2nd test behavior or could that change as well?

This one is for sure fine and IMO the idiomatic way to add more than one callback.

@open-telemetry/go-maintainers, do you think that we should still register the additional callback when Meter is called with an already registered meter? Personally, I feel that the current behavior is "safer" for the users as if they create a Meter in the hot-path (for whatever reason) then we would not add infinitely a callback. I would be afraid that changing the behavior now can cause harm to the current users. Therefore, I would say that it is an incorrect usage of the API. At this moment, I have no idea how to nicely clarify it in the docs (this is what should be probably be done to fix this issue).

[dashpole]

I think this is WAI. If we added the callback, it would result in a memory leak over time. We explicitly document this in the SDK readme: https://pkg.go.dev/go.opentelemetry.io/otel/sdk/metric#pkg-overview.

The correct way to repeatedly add callbacks to an instrument is to use meter.RegisterCallback. This ensures users are able to properly unregister them when the callback no longer becomes relevant, and avoids leaking memory.

[MrAlias]

Possible duplicate of https://github.com/open-telemetry/opentelemetry-go/issues/3808