Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Callbacks of instruments from other MeterProviders must not be collected #4164

Closed
MrAlias opened this issue Jun 1, 2023 · 1 comment · Fixed by #4333
Closed

Callbacks of instruments from other MeterProviders must not be collected #4164

MrAlias opened this issue Jun 1, 2023 · 1 comment · Fixed by #4333
Assignees
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Jun 1, 2023

From the specification:

Callback functions MUST be invoked for the specific MetricReader performing collection, such that observations made or produced by executing callbacks only apply to the intended MetricReader during collection.

The SDK does not look compliant with this:

func TestMeterProviderMixingOnRegisterErrors(t *testing.T) {
	otel.SetLogger(testr.New(t))

	rdr0 := NewManualReader()
	mp0 := NewMeterProvider(WithReader(rdr0))

	rdr1 := NewManualReader()
	mp1 := NewMeterProvider(WithReader(rdr1))

	// Meters with the same scope but different MeterProviders.
	m0 := mp0.Meter("TestMeterProviderMixingOnRegisterErrors")
	m0Ctr, err := m1.Float64ObservableCounter("float64 ctr")
	require.NoError(t, err)

	m1 := mp1.Meter("TestMeterProviderMixingOnRegisterErrors")
	m1Ctr, err := m1.Int64ObservableCounter("int64 ctr")
	require.NoError(t, err)

	_, err = m0.RegisterCallback(
		func(_ context.Context, o metric.Observer) error {
			o.ObserveFloat64(m0Ctr, 2)
			// Observe an instrument from a differnt MeterProvider.
			o.ObserveInt64(m1Ctr, 1)

			return nil
		},
		m0Ctr, m1Ctr,
	)
	assert.Error(
		t,
		err,
		"Instrument registered with Meter from different MeterProvider",
	)

	var data metricdata.ResourceMetrics
	_ = rdr0.Collect(context.Background(), &data)
	// Only the metrics from mp0 should be produced.
	assert.Len(t, data.ScopeMetrics, 1)

	err = rdr0.Collect(context.Background(), &data)
	assert.NoError(t, err, "Errored when collect should be a noop")
	assert.Len(
		t, data.ScopeMetrics, 0,
		"Metrics produced for instrument collected by different MeterProvider",
	)
}
go test ./...
?   	go.opentelemetry.io/otel/sdk/metric/metricdata	[no test files]
--- FAIL: TestMeterProviderMixingOnRegisterErrors (0.00s)
    provider_test.go:111:
        	Error Trace:	/home/tyler/go/src/go.opentelemetry.io/otel/sdk/metric/provider_test.go:111
        	Error:      	An error is expected but got nil.
        	Test:       	TestMeterProviderMixingOnRegisterErrors
        	Messages:   	Instrument registered with Meter from different MeterProvider
    provider_test.go:124:
        	Error Trace:	/home/tyler/go/src/go.opentelemetry.io/otel/sdk/metric/provider_test.go:124
        	Error:      	"[{{TestMeterProviderMixingOnRegisterErrors  } [{float64 ctr   {[{{{[]}} 2023-06-01 13:29:57.873249248 -0700 PDT m=+0.021975681 2023-06-01 13:29:57.873306387 -0700 PDT m=+0.022032821 %!s(float64=2) []}] CumulativeTemporality %!s(bool=true)}}]}]" should have 0 item(s), but has 1
        	Test:       	TestMeterProviderMixingOnRegisterErrors
        	Messages:   	Metrics produced for instrument collected by different MeterProvider
FAIL
FAIL	go.opentelemetry.io/otel/sdk/metric	0.026s
ok  	go.opentelemetry.io/otel/sdk/metric/aggregation	(cached)
ok  	go.opentelemetry.io/otel/sdk/metric/internal	(cached)
ok  	go.opentelemetry.io/otel/sdk/metric/metricdata/metricdatatest	(cached)
FAIL

Originally posted by @MrAlias in #3652 (comment)

@MrAlias
Copy link
Contributor Author

MrAlias commented Jul 14, 2023

Updated provider test:

func TestMeterProviderMixingOnRegisterErrors(t *testing.T) {
	otel.SetLogger(testr.New(t))

	rdr0 := NewManualReader()
	mp0 := NewMeterProvider(WithReader(rdr0))

	rdr1 := NewManualReader()
	mp1 := NewMeterProvider(WithReader(rdr1))

	// Meters with the same scope but different MeterProviders.
	m0 := mp0.Meter("TestMeterProviderMixingOnRegisterErrors")
	m1 := mp1.Meter("TestMeterProviderMixingOnRegisterErrors")

	m0Gauge, err := m0.Float64ObservableGauge("float64Gauge")
	require.NoError(t, err)

	m1Gauge, err := m1.Int64ObservableGauge("int64Gauge")
	require.NoError(t, err)

	_, err = m0.RegisterCallback(
		func(_ context.Context, o api.Observer) error {
			o.ObserveFloat64(m0Gauge, 2)
			// Observe an instrument from a differnt MeterProvider.
			o.ObserveInt64(m1Gauge, 1)

			return nil
		},
		m0Gauge, m1Gauge,
	)
	assert.Error(
		t,
		err,
		"Instrument registered with Meter from different MeterProvider",
	)

	var data metricdata.ResourceMetrics
	_ = rdr0.Collect(context.Background(), &data)
	// Only the metrics from mp0 should be produced.
	assert.Len(t, data.ScopeMetrics, 1)

	err = rdr1.Collect(context.Background(), &data)
	assert.NoError(t, err, "Errored when collect should be a noop")
	assert.Len(
		t, data.ScopeMetrics, 0,
		"Metrics produced for instrument collected by different MeterProvider",
	)
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

1 participant