open-telemetry / opentelemetry-go

OpenTelemetry Go API and SDK
https://opentelemetry.io/docs/languages/go
Apache License 2.0
5.18k stars 1.04k forks source link

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

Closed MrAlias closed 1 year ago

MrAlias commented 1 year ago

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 https://github.com/open-telemetry/opentelemetry-go/issues/3652#issuecomment-1572597080

MrAlias commented 1 year ago

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",
    )
}