From 0c857a3d602497fb83cf210cda573e8ac17efa18 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 8 Jun 2022 12:02:39 -0700 Subject: [PATCH] Implement MeterProvider's Meter method (#2945) * Implement stubbed meter create method * Rename return value to avoid comment * Encapsulate meterRegistry tests with identifying name * Run lint fix * Comment meterRegistry being concurrent safe * Remove ordered meter tracking in the meterRegistry * Test range completeness instead of order * Remove provider field from meter --- sdk/metric/meter.go | 62 +++++++++++++++++++++++++++++++++- sdk/metric/meter_test.go | 66 +++++++++++++++++++++++++++++++++++++ sdk/metric/provider.go | 12 ++++--- sdk/metric/provider_test.go | 16 +++++++++ 4 files changed, 151 insertions(+), 5 deletions(-) create mode 100644 sdk/metric/meter_test.go diff --git a/sdk/metric/meter.go b/sdk/metric/meter.go index 98eba1b15fc..3b052ae864e 100644 --- a/sdk/metric/meter.go +++ b/sdk/metric/meter.go @@ -19,6 +19,7 @@ package metric // import "go.opentelemetry.io/otel/sdk/metric" import ( "context" + "sync" "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/metric/instrument" @@ -26,14 +27,73 @@ import ( "go.opentelemetry.io/otel/metric/instrument/asyncint64" "go.opentelemetry.io/otel/metric/instrument/syncfloat64" "go.opentelemetry.io/otel/metric/instrument/syncint64" + "go.opentelemetry.io/otel/sdk/instrumentation" ) +// meterRegistry keeps a record of initialized meters for instrumentation +// libraries. A meter is unique to an instrumentation library and if multiple +// requests for that meter are made a meterRegistry ensure the same instance +// is used. +// +// The zero meterRegistry is empty and ready for use. +// +// A meterRegistry must not be copied after first use. +// +// All methods of a meterRegistry are safe to call concurrently. +type meterRegistry struct { + sync.Mutex + + meters map[instrumentation.Library]*meter +} + +// Get returns a registered meter matching the instrumentation library if it +// exists in the meterRegistry. Otherwise, a new meter configured for the +// instrumentation library is registered and then returned. +// +// Get is safe to call concurrently. +func (r *meterRegistry) Get(l instrumentation.Library) *meter { + r.Lock() + defer r.Unlock() + + if r.meters == nil { + m := &meter{Library: l} + r.meters = map[instrumentation.Library]*meter{l: m} + return m + } + + m, ok := r.meters[l] + if ok { + return m + } + + m = &meter{Library: l} + r.meters[l] = m + return m +} + +// Range calls f sequentially for each meter present in the meterRegistry. If +// f returns false, the iteration is stopped. +// +// Range is safe to call concurrently. +func (r *meterRegistry) Range(f func(*meter) bool) { + r.Lock() + defer r.Unlock() + + for _, m := range r.meters { + if !f(m) { + return + } + } +} + // meter handles the creation and coordination of all metric instruments. A // meter represents a single instrumentation scope; all metric telemetry // produced by an instrumentation scope will use metric instruments from a // single meter. type meter struct { - // TODO (#2821, #2815, 2814): implement. + instrumentation.Library + + // TODO (#2815, 2814): implement. } // Compile-time check meter implements metric.Meter. diff --git a/sdk/metric/meter_test.go b/sdk/metric/meter_test.go new file mode 100644 index 00000000000..3fdfbdcb344 --- /dev/null +++ b/sdk/metric/meter_test.go @@ -0,0 +1,66 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build go1.17 +// +build go1.17 + +package metric + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "go.opentelemetry.io/otel/sdk/instrumentation" +) + +func TestMeterRegistry(t *testing.T) { + il0 := instrumentation.Library{Name: "zero"} + il1 := instrumentation.Library{Name: "one"} + + r := meterRegistry{} + var m0 *meter + t.Run("ZeroValueGetDoesNotPanic", func(t *testing.T) { + assert.NotPanics(t, func() { m0 = r.Get(il0) }) + assert.Equal(t, il0, m0.Library, "uninitialized meter returned") + }) + + m01 := r.Get(il0) + t.Run("GetSameMeter", func(t *testing.T) { + assert.Samef(t, m0, m01, "returned different meters: %v", il0) + }) + + m1 := r.Get(il1) + t.Run("GetDifferentMeter", func(t *testing.T) { + assert.NotSamef(t, m0, m1, "returned same meters: %v", il1) + }) + + t.Run("RangeComplete", func(t *testing.T) { + var got []*meter + r.Range(func(m *meter) bool { + got = append(got, m) + return true + }) + assert.ElementsMatch(t, []*meter{m0, m1}, got) + }) + + t.Run("RangeStopIteration", func(t *testing.T) { + var i int + r.Range(func(m *meter) bool { + i++ + return false + }) + assert.Equal(t, 1, i, "iteration not stopped after first flase return") + }) +} diff --git a/sdk/metric/provider.go b/sdk/metric/provider.go index badb22de3e4..ecd15550f42 100644 --- a/sdk/metric/provider.go +++ b/sdk/metric/provider.go @@ -21,6 +21,7 @@ import ( "context" "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/sdk/instrumentation" "go.opentelemetry.io/otel/sdk/metric/view" "go.opentelemetry.io/otel/sdk/resource" ) @@ -32,6 +33,7 @@ import ( type MeterProvider struct { res *resource.Resource + meters meterRegistry //nolint:structcheck,unused //This will be used by the MP to produce for a reader, and for the Meters to do proper view filtering readers map[Reader][]view.View @@ -73,10 +75,12 @@ func NewMeterProvider(options ...Option) *MeterProvider { // // This method is safe to call concurrently. func (mp *MeterProvider) Meter(name string, options ...metric.MeterOption) metric.Meter { - // TODO (#2821): ensure this is concurrent safe. - // TODO: test this is concurrent safe. - // TODO (#2821): register and track the created Meter. - return &meter{} + c := metric.NewMeterConfig(options...) + return mp.meters.Get(instrumentation.Library{ + Name: name, + Version: c.InstrumentationVersion(), + SchemaURL: c.SchemaURL(), + }) } // ForceFlush flushes all pending telemetry. diff --git a/sdk/metric/provider_test.go b/sdk/metric/provider_test.go index dbcc43f4949..dca401fe3b8 100644 --- a/sdk/metric/provider_test.go +++ b/sdk/metric/provider_test.go @@ -24,6 +24,17 @@ import ( "github.com/stretchr/testify/assert" ) +func TestMeterConcurrentSafe(t *testing.T) { + const name = "TestMeterConcurrentSafe meter" + mp := NewMeterProvider() + + go func() { + _ = mp.Meter(name) + }() + + _ = mp.Meter(name) +} + func TestForceFlushConcurrentSafe(t *testing.T) { mp := NewMeterProvider() @@ -44,6 +55,11 @@ func TestShutdownConcurrentSafe(t *testing.T) { _ = mp.Shutdown(context.Background()) } +func TestMeterDoesNotPanicForEmptyMeterProvider(t *testing.T) { + mp := MeterProvider{} + assert.NotPanics(t, func() { _ = mp.Meter("") }) +} + func TestForceFlushDoesNotPanicForEmptyMeterProvider(t *testing.T) { mp := MeterProvider{} assert.NotPanics(t, func() { _ = mp.ForceFlush(context.Background()) })