From cf7e04c54a2377d991ab4f66c94b5235ea9063c9 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Wed, 18 Aug 2021 10:50:12 -0700 Subject: [PATCH 01/33] factor instrumentation library out of the instrument descriptor --- internal/metric/global/meter.go | 25 ++- internal/metric/global/meter_test.go | 136 ++++++++------- internal/metric/global/registry_test.go | 17 +- metric/config.go | 51 ++---- metric/metric.go | 37 ++-- metric/metric_sdkapi.go | 6 +- metric/metric_test.go | 220 +++++++++--------------- metric/metrictest/meter.go | 82 +++++---- metric/registry/registry.go | 100 +++++++---- metric/registry/registry_test.go | 10 +- 10 files changed, 337 insertions(+), 347 deletions(-) diff --git a/internal/metric/global/meter.go b/internal/metric/global/meter.go index d91c0b8a234..e30eb2f1c56 100644 --- a/internal/metric/global/meter.go +++ b/internal/metric/global/meter.go @@ -48,7 +48,9 @@ import ( // methods of the api/metric/registry package. type meterKey struct { - Name, Version string + InstrumentationName string + InstrumentationVersion string + SchemaURL string } type meterProvider struct { @@ -138,7 +140,7 @@ func (p *meterProvider) setDelegate(provider metric.MeterProvider) { p.delegate = provider for key, entry := range p.meters { - entry.impl.setDelegate(key.Name, key.Version, provider) + entry.impl.setDelegate(key, provider) } p.meters = nil } @@ -151,28 +153,33 @@ func (p *meterProvider) Meter(instrumentationName string, opts ...metric.MeterOp return p.delegate.Meter(instrumentationName, opts...) } + cfg := metric.NewMeterConfig(opts...) key := meterKey{ - Name: instrumentationName, - Version: metric.NewMeterConfig(opts...).InstrumentationVersion(), + InstrumentationName: instrumentationName, + InstrumentationVersion: cfg.InstrumentationVersion(), + SchemaURL: cfg.SchemaURL(), } entry, ok := p.meters[key] if !ok { entry = &meterEntry{} - entry.unique = registry.NewUniqueInstrumentMeterImpl(&entry.impl) + entry.unique = registry.NewMeterImpl(&entry.impl) p.meters[key] = entry - } - return metric.WrapMeterImpl(entry.unique, key.Name, metric.WithInstrumentationVersion(key.Version)) + return metric.WrapMeterImpl(entry.unique) } // Meter interface and delegation -func (m *meterImpl) setDelegate(name, version string, provider metric.MeterProvider) { +func (m *meterImpl) setDelegate(key meterKey, provider metric.MeterProvider) { m.lock.Lock() defer m.lock.Unlock() d := new(metric.MeterImpl) - *d = provider.Meter(name, metric.WithInstrumentationVersion(version)).MeterImpl() + *d = provider.Meter( + key.InstrumentationName, + metric.WithInstrumentationVersion(key.InstrumentationVersion), + metric.WithSchemaURL(key.SchemaURL), + ).MeterImpl() m.delegate = unsafe.Pointer(d) for _, inst := range m.syncInsts { diff --git a/internal/metric/global/meter_test.go b/internal/metric/global/meter_test.go index be3d63b717a..cca49826db5 100644 --- a/internal/metric/global/meter_test.go +++ b/internal/metric/global/meter_test.go @@ -39,7 +39,17 @@ func TestDirect(t *testing.T) { ctx := context.Background() meter1 := metricglobal.Meter("test1", metric.WithInstrumentationVersion("semver:v1.0.0")) - meter2 := metricglobal.Meter("test2") + meter2 := metricglobal.Meter("test2", metric.WithSchemaURL("hello")) + + library1 := metrictest.Library{ + InstrumentationName: "test1", + InstrumentationVersion: "semver:v1.0.0", + } + library2 := metrictest.Library{ + InstrumentationName: "test2", + SchemaURL: "hello", + } + labels1 := []attribute.KeyValue{attribute.String("A", "B")} labels2 := []attribute.KeyValue{attribute.String("C", "D")} labels3 := []attribute.KeyValue{attribute.String("E", "F")} @@ -66,66 +76,60 @@ func TestDirect(t *testing.T) { second.Record(ctx, 1, labels3...) second.Record(ctx, 2, labels3...) - mock, provider := metrictest.NewMeterProvider() + provider := metrictest.NewMeterProvider() metricglobal.SetMeterProvider(provider) counter.Add(ctx, 1, labels1...) valuerecorder.Record(ctx, 3, labels1...) second.Record(ctx, 3, labels3...) - mock.RunAsyncInstruments() + provider.RunAsyncInstruments() - measurements := metrictest.AsStructs(mock.MeasurementBatches) + measurements := metrictest.AsStructs(provider.MeasurementBatches) require.EqualValues(t, []metrictest.Measured{ { - Name: "test.counter", - InstrumentationName: "test1", - InstrumentationVersion: "semver:v1.0.0", - Labels: metrictest.LabelsToMap(labels1...), - Number: asInt(1), + Name: "test.counter", + Library: library1, + Labels: metrictest.LabelsToMap(labels1...), + Number: asInt(1), }, { - Name: "test.valuerecorder", - InstrumentationName: "test1", - InstrumentationVersion: "semver:v1.0.0", - Labels: metrictest.LabelsToMap(labels1...), - Number: asFloat(3), + Name: "test.valuerecorder", + Library: library1, + Labels: metrictest.LabelsToMap(labels1...), + Number: asFloat(3), }, { - Name: "test.second", - InstrumentationName: "test2", - Labels: metrictest.LabelsToMap(labels3...), - Number: asFloat(3), + Name: "test.second", + Library: library2, + Labels: metrictest.LabelsToMap(labels3...), + Number: asFloat(3), }, { - Name: "test.valueobserver.float", - InstrumentationName: "test1", - InstrumentationVersion: "semver:v1.0.0", - Labels: metrictest.LabelsToMap(labels1...), - Number: asFloat(1), + Name: "test.valueobserver.float", + Library: library1, + Labels: metrictest.LabelsToMap(labels1...), + Number: asFloat(1), }, { - Name: "test.valueobserver.float", - InstrumentationName: "test1", - InstrumentationVersion: "semver:v1.0.0", - Labels: metrictest.LabelsToMap(labels2...), - Number: asFloat(2), + Name: "test.valueobserver.float", + Library: library1, + Labels: metrictest.LabelsToMap(labels2...), + Number: asFloat(2), }, { - Name: "test.valueobserver.int", - InstrumentationName: "test1", - InstrumentationVersion: "semver:v1.0.0", - Labels: metrictest.LabelsToMap(labels1...), - Number: asInt(1), + Name: "test.valueobserver.int", + Library: library1, + Labels: metrictest.LabelsToMap(labels1...), + Number: asInt(1), }, { - Name: "test.valueobserver.int", - InstrumentationName: "test1", - InstrumentationVersion: "semver:v1.0.0", - Labels: metrictest.LabelsToMap(labels2...), - Number: asInt(2), + Name: "test.valueobserver.int", + Library: library1, + Labels: metrictest.LabelsToMap(labels2...), + Number: asInt(2), }, }, measurements, @@ -138,7 +142,11 @@ func TestBound(t *testing.T) { // Note: this test uses opposite Float64/Int64 number kinds // vs. the above, to cover all the instruments. ctx := context.Background() - glob := metricglobal.Meter("test") + glob := metricglobal.Meter( + "test", + metric.WithInstrumentationVersion("semver:test-1.0"), + metric.WithSchemaURL("schema://url"), + ) labels1 := []attribute.KeyValue{attribute.String("A", "B")} counter := Must(glob).NewFloat64Counter("test.counter") @@ -151,28 +159,34 @@ func TestBound(t *testing.T) { boundM.Record(ctx, 1) boundM.Record(ctx, 2) - mock, provider := metrictest.NewMeterProvider() + provider := metrictest.NewMeterProvider() metricglobal.SetMeterProvider(provider) boundC.Add(ctx, 1) boundM.Record(ctx, 3) + library := metrictest.Library{ + InstrumentationName: "test", + InstrumentationVersion: "semver:test-1.0", + SchemaURL: "schema://url", + } + require.EqualValues(t, []metrictest.Measured{ { - Name: "test.counter", - InstrumentationName: "test", - Labels: metrictest.LabelsToMap(labels1...), - Number: asFloat(1), + Name: "test.counter", + Library: library, + Labels: metrictest.LabelsToMap(labels1...), + Number: asFloat(1), }, { - Name: "test.valuerecorder", - InstrumentationName: "test", - Labels: metrictest.LabelsToMap(labels1...), - Number: asInt(3), + Name: "test.valuerecorder", + Library: library, + Labels: metrictest.LabelsToMap(labels1...), + Number: asInt(3), }, }, - metrictest.AsStructs(mock.MeasurementBatches)) + metrictest.AsStructs(provider.MeasurementBatches)) boundC.Unbind() boundM.Unbind() @@ -199,7 +213,7 @@ func TestUnbindThenRecordOne(t *testing.T) { global.ResetForTest() ctx := context.Background() - mock, provider := metrictest.NewMeterProvider() + provider := metrictest.NewMeterProvider() meter := metricglobal.Meter("test") counter := Must(meter).NewInt64Counter("test.counter") @@ -210,7 +224,7 @@ func TestUnbindThenRecordOne(t *testing.T) { require.NotPanics(t, func() { boundC.Add(ctx, 1) }) - require.Equal(t, 0, len(mock.MeasurementBatches)) + require.Equal(t, 0, len(provider.MeasurementBatches)) } type meterProviderWithConstructorError struct { @@ -222,7 +236,7 @@ type meterWithConstructorError struct { } func (m *meterProviderWithConstructorError) Meter(iName string, opts ...metric.MeterOption) metric.Meter { - return metric.WrapMeterImpl(&meterWithConstructorError{m.MeterProvider.Meter(iName, opts...).MeterImpl()}, iName, opts...) + return metric.WrapMeterImpl(&meterWithConstructorError{m.MeterProvider.Meter(iName, opts...).MeterImpl()}) } func (m *meterWithConstructorError) NewSyncInstrument(_ metric.Descriptor) (metric.SyncImpl, error) { @@ -238,7 +252,7 @@ func TestErrorInDeferredConstructor(t *testing.T) { c1 := Must(meter).NewInt64Counter("test") c2 := Must(meter).NewInt64Counter("test") - _, provider := metrictest.NewMeterProvider() + provider := metrictest.NewMeterProvider() sdk := &meterProviderWithConstructorError{provider} require.Panics(t, func() { @@ -280,7 +294,7 @@ func TestImplementationIndirection(t *testing.T) { require.False(t, ok) // Register the SDK - _, provider := metrictest.NewMeterProvider() + provider := metrictest.NewMeterProvider() metricglobal.SetMeterProvider(provider) // Repeat the above tests @@ -309,7 +323,7 @@ func TestRecordBatchMock(t *testing.T) { meter.RecordBatch(context.Background(), nil, counter.Measurement(1)) - mock, provider := metrictest.NewMeterProvider() + provider := metrictest.NewMeterProvider() metricglobal.SetMeterProvider(provider) meter.RecordBatch(context.Background(), nil, counter.Measurement(1)) @@ -317,11 +331,13 @@ func TestRecordBatchMock(t *testing.T) { require.EqualValues(t, []metrictest.Measured{ { - Name: "test.counter", - InstrumentationName: "builtin", - Labels: metrictest.LabelsToMap(), - Number: asInt(1), + Name: "test.counter", + Library: metrictest.Library{ + InstrumentationName: "builtin", + }, + Labels: metrictest.LabelsToMap(), + Number: asInt(1), }, }, - metrictest.AsStructs(mock.MeasurementBatches)) + metrictest.AsStructs(provider.MeasurementBatches)) } diff --git a/internal/metric/global/registry_test.go b/internal/metric/global/registry_test.go index 883d3fdaab1..0791a74f9dc 100644 --- a/internal/metric/global/registry_test.go +++ b/internal/metric/global/registry_test.go @@ -22,6 +22,7 @@ import ( "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/metric/metrictest" "go.opentelemetry.io/otel/metric/registry" ) @@ -78,6 +79,10 @@ func TestRegistrySameInstruments(t *testing.T) { require.NoError(t, err1) require.NoError(t, err2) require.Equal(t, inst1, inst2) + + SetMeterProvider(metrictest.NewMeterProvider()) + + require.Equal(t, inst1, inst2) } } @@ -89,6 +94,16 @@ func TestRegistryDifferentNamespace(t *testing.T) { require.NoError(t, err1) require.NoError(t, err2) + + if inst1.Descriptor().InstrumentKind().Synchronous() { + // They're equal because of a `nil` pointer at this point. + // (Only for synchronous instruments, which lack callacks.) + require.EqualValues(t, inst1, inst2) + } + + SetMeterProvider(metrictest.NewMeterProvider()) + + // They're different after the deferred setup. require.NotEqual(t, inst1, inst2) } } @@ -109,7 +124,7 @@ func TestRegistryDiffInstruments(t *testing.T) { require.Error(t, err) require.NotNil(t, other) require.True(t, errors.Is(err, registry.ErrMetricKindMismatch)) - require.Contains(t, err.Error(), "super") + require.Contains(t, err.Error(), "by this name with another kind or number type") } } } diff --git a/metric/config.go b/metric/config.go index 053d0699b7a..6f20cbfb102 100644 --- a/metric/config.go +++ b/metric/config.go @@ -20,10 +20,8 @@ import ( // InstrumentConfig contains options for metric instrument descriptors. type InstrumentConfig struct { - description string - unit unit.Unit - instrumentationName string - instrumentationVersion string + description string + unit unit.Unit } // Description describes the instrument in human-readable terms. @@ -36,18 +34,6 @@ func (cfg InstrumentConfig) Unit() unit.Unit { return cfg.unit } -// InstrumentationName is the name of the library providing -// instrumentation. -func (cfg InstrumentConfig) InstrumentationName() string { - return cfg.instrumentationName -} - -// InstrumentationVersion is the version of the library providing -// instrumentation. -func (cfg InstrumentConfig) InstrumentationVersion() string { - return cfg.instrumentationVersion -} - // InstrumentOption is an interface for applying metric instrument options. type InstrumentOption interface { // ApplyMeter is used to set a InstrumentOption value of a @@ -85,16 +71,10 @@ func WithUnit(unit unit.Unit) InstrumentOption { }) } -// WithInstrumentationName sets the instrumentation name. -func WithInstrumentationName(name string) InstrumentOption { - return instrumentOptionFunc(func(cfg *InstrumentConfig) { - cfg.instrumentationName = name - }) -} - // MeterConfig contains options for Meters. type MeterConfig struct { instrumentationVersion string + schemaURL string } // InstrumentationVersion is the version of the library providing instrumentation. @@ -102,6 +82,11 @@ func (cfg MeterConfig) InstrumentationVersion() string { return cfg.instrumentationVersion } +// SchemaURL is the schema_url of the library providing instrumentation. +func (cfg MeterConfig) SchemaURL() string { + return cfg.schemaURL +} + // MeterOption is an interface for applying Meter options. type MeterOption interface { // ApplyMeter is used to set a MeterOption value of a MeterConfig. @@ -118,15 +103,8 @@ func NewMeterConfig(opts ...MeterOption) MeterConfig { return config } -// InstrumentMeterOption are options that can be used as both an InstrumentOption -// and MeterOption -type InstrumentMeterOption interface { - InstrumentOption - MeterOption -} - // WithInstrumentationVersion sets the instrumentation version. -func WithInstrumentationVersion(version string) InstrumentMeterOption { +func WithInstrumentationVersion(version string) MeterOption { return instrumentationVersionOption(version) } @@ -136,6 +114,13 @@ func (i instrumentationVersionOption) applyMeter(config *MeterConfig) { config.instrumentationVersion = string(i) } -func (i instrumentationVersionOption) applyInstrument(config *InstrumentConfig) { - config.instrumentationVersion = string(i) +// WithSchemaURL sets the schema URL. +func WithSchemaURL(version string) MeterOption { + return schemaURLOption(version) +} + +type schemaURLOption string + +func (s schemaURLOption) applyMeter(config *MeterConfig) { + config.schemaURL = string(s) } diff --git a/metric/metric.go b/metric/metric.go index ee177a615f5..b8406e0c4b1 100644 --- a/metric/metric.go +++ b/metric/metric.go @@ -38,8 +38,7 @@ type MeterProvider interface { // // An uninitialized Meter is a no-op implementation. type Meter struct { - impl MeterImpl - name, version string + impl MeterImpl } // RecordBatch atomically records a batch of measurements. @@ -285,9 +284,8 @@ func (m Meter) newAsync( if m.impl == nil { return NoopAsync{}, nil } - desc := NewDescriptor(name, mkind, nkind, opts...) - desc.config.instrumentationName = m.name - desc.config.instrumentationVersion = m.version + cfg := NewInstrumentConfig(opts...) + desc := NewDescriptor(name, mkind, nkind, cfg.description, cfg.unit) return m.impl.NewAsyncInstrument(desc, runner) } @@ -304,9 +302,8 @@ func (m Meter) newSync( if m.impl == nil { return NoopSync{}, nil } - desc := NewDescriptor(name, metricKind, numberKind, opts...) - desc.config.instrumentationName = m.name - desc.config.instrumentationVersion = m.version + cfg := NewInstrumentConfig(opts...) + desc := NewDescriptor(name, metricKind, numberKind, cfg.description, cfg.unit) return m.impl.NewSyncInstrument(desc) } @@ -524,16 +521,18 @@ type Descriptor struct { name string instrumentKind sdkapi.InstrumentKind numberKind number.Kind - config InstrumentConfig + description string + unit unit.Unit } // NewDescriptor returns a Descriptor with the given contents. -func NewDescriptor(name string, ikind sdkapi.InstrumentKind, nkind number.Kind, opts ...InstrumentOption) Descriptor { +func NewDescriptor(name string, ikind sdkapi.InstrumentKind, nkind number.Kind, description string, unit unit.Unit) Descriptor { return Descriptor{ name: name, instrumentKind: ikind, numberKind: nkind, - config: NewInstrumentConfig(opts...), + description: description, + unit: unit, } } @@ -550,13 +549,13 @@ func (d Descriptor) InstrumentKind() sdkapi.InstrumentKind { // Description provides a human-readable description of the metric // instrument. func (d Descriptor) Description() string { - return d.config.Description() + return d.description } // Unit describes the units of the metric instrument. Unitless // metrics return the empty string. func (d Descriptor) Unit() unit.Unit { - return d.config.Unit() + return d.unit } // NumberKind returns whether this instrument is declared over int64, @@ -564,15 +563,3 @@ func (d Descriptor) Unit() unit.Unit { func (d Descriptor) NumberKind() number.Kind { return d.numberKind } - -// InstrumentationName returns the name of the library that provided -// instrumentation for this instrument. -func (d Descriptor) InstrumentationName() string { - return d.config.InstrumentationName() -} - -// InstrumentationVersion returns the version of the library that provided -// instrumentation for this instrument. -func (d Descriptor) InstrumentationVersion() string { - return d.config.InstrumentationVersion() -} diff --git a/metric/metric_sdkapi.go b/metric/metric_sdkapi.go index eb5ac4f52a6..ca8e53191a9 100644 --- a/metric/metric_sdkapi.go +++ b/metric/metric_sdkapi.go @@ -86,10 +86,8 @@ type AsyncImpl interface { // WrapMeterImpl constructs a `Meter` implementation from a // `MeterImpl` implementation. -func WrapMeterImpl(impl MeterImpl, instrumentationName string, opts ...MeterOption) Meter { +func WrapMeterImpl(impl MeterImpl) Meter { return Meter{ - impl: impl, - name: instrumentationName, - version: NewMeterConfig(opts...).InstrumentationVersion(), + impl: impl, } } diff --git a/metric/metric_test.go b/metric/metric_test.go index c95496b3a31..9d54e0c7a98 100644 --- a/metric/metric_test.go +++ b/metric/metric_test.go @@ -120,16 +120,16 @@ func TestPrecomputedSum(t *testing.T) { } } -func checkSyncBatches(ctx context.Context, t *testing.T, labels []attribute.KeyValue, mock *metrictest.MeterImpl, nkind number.Kind, mkind sdkapi.InstrumentKind, instrument metric.InstrumentImpl, expected ...float64) { +func checkSyncBatches(ctx context.Context, t *testing.T, labels []attribute.KeyValue, provider *metrictest.MeterProvider, nkind number.Kind, mkind sdkapi.InstrumentKind, instrument metric.InstrumentImpl, expected ...float64) { t.Helper() - batchesCount := len(mock.MeasurementBatches) - if len(mock.MeasurementBatches) != len(expected) { - t.Errorf("Expected %d recorded measurement batches, got %d", batchesCount, len(mock.MeasurementBatches)) + batchesCount := len(provider.MeasurementBatches) + if len(provider.MeasurementBatches) != len(expected) { + t.Errorf("Expected %d recorded measurement batches, got %d", batchesCount, len(provider.MeasurementBatches)) } - recorded := metrictest.AsStructs(mock.MeasurementBatches) + recorded := metrictest.AsStructs(provider.MeasurementBatches) - for i, batch := range mock.MeasurementBatches { + for i, batch := range provider.MeasurementBatches { if len(batch.Measurements) != 1 { t.Errorf("Expected 1 measurement in batch %d, got %d", i, len(batch.Measurements)) } @@ -138,10 +138,12 @@ func checkSyncBatches(ctx context.Context, t *testing.T, labels []attribute.KeyV descriptor := measurement.Instrument.Descriptor() expected := metrictest.Measured{ - Name: descriptor.Name(), - InstrumentationName: descriptor.InstrumentationName(), - Labels: metrictest.LabelsToMap(labels...), - Number: metrictest.ResolveNumberByKind(t, nkind, expected[i]), + Name: descriptor.Name(), + Library: metrictest.Library{ + InstrumentationName: "apitest", + }, + Labels: metrictest.LabelsToMap(labels...), + Number: metrictest.ResolveNumberByKind(t, nkind, expected[i]), } require.Equal(t, expected, recorded[i]) } @@ -149,31 +151,25 @@ func checkSyncBatches(ctx context.Context, t *testing.T, labels []attribute.KeyV func TestOptions(t *testing.T) { type testcase struct { - name string - opts []metric.InstrumentOption - desc string - unit unit.Unit - iName string - iVer string + name string + opts []metric.InstrumentOption + desc string + unit unit.Unit } testcases := []testcase{ { - name: "no opts", - opts: nil, - desc: "", - unit: "", - iName: "", - iVer: "", + name: "no opts", + opts: nil, + desc: "", + unit: "", }, { name: "description", opts: []metric.InstrumentOption{ metric.WithDescription("stuff"), }, - desc: "stuff", - unit: "", - iName: "", - iVer: "", + desc: "stuff", + unit: "", }, { name: "description override", @@ -181,20 +177,16 @@ func TestOptions(t *testing.T) { metric.WithDescription("stuff"), metric.WithDescription("things"), }, - desc: "things", - unit: "", - iName: "", - iVer: "", + desc: "things", + unit: "", }, { name: "unit", opts: []metric.InstrumentOption{ metric.WithUnit("s"), }, - desc: "", - unit: "s", - iName: "", - iVer: "", + desc: "", + unit: "s", }, { name: "description override", @@ -202,20 +194,16 @@ func TestOptions(t *testing.T) { metric.WithDescription("stuff"), metric.WithDescription("things"), }, - desc: "things", - unit: "", - iName: "", - iVer: "", + desc: "things", + unit: "", }, { name: "unit", opts: []metric.InstrumentOption{ metric.WithUnit("s"), }, - desc: "", - unit: "s", - iName: "", - iVer: "", + desc: "", + unit: "s", }, { @@ -224,67 +212,17 @@ func TestOptions(t *testing.T) { metric.WithUnit("s"), metric.WithUnit("h"), }, - desc: "", - unit: "h", - iName: "", - iVer: "", - }, - { - name: "name", - opts: []metric.InstrumentOption{ - metric.WithInstrumentationName("n"), - }, - desc: "", - unit: "", - iName: "n", - iVer: "", - }, - - { - name: "name override", - opts: []metric.InstrumentOption{ - metric.WithInstrumentationName("n"), - metric.WithInstrumentationName("o"), - }, - desc: "", - unit: "", - iName: "o", - iVer: "", - }, - { - name: "version", - opts: []metric.InstrumentOption{ - metric.WithInstrumentationVersion("v"), - }, - desc: "", - unit: "", - iName: "", - iVer: "v", - }, - - { - name: "version override", - opts: []metric.InstrumentOption{ - metric.WithInstrumentationVersion("v"), - metric.WithInstrumentationVersion("q"), - }, - desc: "", - unit: "", - iName: "", - iVer: "q", + desc: "", + unit: "h", }, { name: "all", opts: []metric.InstrumentOption{ metric.WithDescription("stuff"), metric.WithUnit("s"), - metric.WithInstrumentationName("n"), - metric.WithInstrumentationVersion("v"), }, - desc: "stuff", - unit: "s", - iName: "n", - iVer: "v", + desc: "stuff", + unit: "s", }, } for idx, tt := range testcases { @@ -296,20 +234,18 @@ func TestOptions(t *testing.T) { if diff := cmp.Diff(cfg.Unit(), tt.unit); diff != "" { t.Errorf("Compare Unit: -got +want %s", diff) } - if diff := cmp.Diff(cfg.InstrumentationName(), tt.iName); diff != "" { - t.Errorf("Compare InstrumentationNam: -got +want %s", diff) - } - if diff := cmp.Diff(cfg.InstrumentationVersion(), tt.iVer); diff != "" { - t.Errorf("Compare InstrumentationVersion: -got +want %s", diff) - } } } +func testPair() (*metrictest.MeterProvider, metric.Meter) { + provider := metrictest.NewMeterProvider() + return provider, provider.Meter("apitest") +} func TestCounter(t *testing.T) { // N.B. the API does not check for negative // values, that's the SDK's responsibility. t.Run("float64 counter", func(t *testing.T) { - mockSDK, meter := metrictest.NewMeter() + provider, meter := testPair() c := Must(meter).NewFloat64Counter("test.counter.float") ctx := context.Background() labels := []attribute.KeyValue{attribute.String("A", "B")} @@ -317,12 +253,12 @@ func TestCounter(t *testing.T) { boundInstrument := c.Bind(labels...) boundInstrument.Add(ctx, -742) meter.RecordBatch(ctx, labels, c.Measurement(42)) - checkSyncBatches(ctx, t, labels, mockSDK, number.Float64Kind, sdkapi.CounterInstrumentKind, c.SyncImpl(), + checkSyncBatches(ctx, t, labels, provider, number.Float64Kind, sdkapi.CounterInstrumentKind, c.SyncImpl(), 1994.1, -742, 42, ) }) t.Run("int64 counter", func(t *testing.T) { - mockSDK, meter := metrictest.NewMeter() + provider, meter := testPair() c := Must(meter).NewInt64Counter("test.counter.int") ctx := context.Background() labels := []attribute.KeyValue{attribute.String("A", "B"), attribute.String("C", "D")} @@ -330,13 +266,13 @@ func TestCounter(t *testing.T) { boundInstrument := c.Bind(labels...) boundInstrument.Add(ctx, 4200) meter.RecordBatch(ctx, labels, c.Measurement(420000)) - checkSyncBatches(ctx, t, labels, mockSDK, number.Int64Kind, sdkapi.CounterInstrumentKind, c.SyncImpl(), + checkSyncBatches(ctx, t, labels, provider, number.Int64Kind, sdkapi.CounterInstrumentKind, c.SyncImpl(), 42, 4200, 420000, ) }) t.Run("int64 updowncounter", func(t *testing.T) { - mockSDK, meter := metrictest.NewMeter() + provider, meter := testPair() c := Must(meter).NewInt64UpDownCounter("test.updowncounter.int") ctx := context.Background() labels := []attribute.KeyValue{attribute.String("A", "B"), attribute.String("C", "D")} @@ -344,12 +280,12 @@ func TestCounter(t *testing.T) { boundInstrument := c.Bind(labels...) boundInstrument.Add(ctx, -100) meter.RecordBatch(ctx, labels, c.Measurement(42)) - checkSyncBatches(ctx, t, labels, mockSDK, number.Int64Kind, sdkapi.UpDownCounterInstrumentKind, c.SyncImpl(), + checkSyncBatches(ctx, t, labels, provider, number.Int64Kind, sdkapi.UpDownCounterInstrumentKind, c.SyncImpl(), 100, -100, 42, ) }) t.Run("float64 updowncounter", func(t *testing.T) { - mockSDK, meter := metrictest.NewMeter() + provider, meter := testPair() c := Must(meter).NewFloat64UpDownCounter("test.updowncounter.float") ctx := context.Background() labels := []attribute.KeyValue{attribute.String("A", "B"), attribute.String("C", "D")} @@ -357,7 +293,7 @@ func TestCounter(t *testing.T) { boundInstrument := c.Bind(labels...) boundInstrument.Add(ctx, -76) meter.RecordBatch(ctx, labels, c.Measurement(-100.1)) - checkSyncBatches(ctx, t, labels, mockSDK, number.Float64Kind, sdkapi.UpDownCounterInstrumentKind, c.SyncImpl(), + checkSyncBatches(ctx, t, labels, provider, number.Float64Kind, sdkapi.UpDownCounterInstrumentKind, c.SyncImpl(), 100.1, -76, -100.1, ) }) @@ -365,7 +301,7 @@ func TestCounter(t *testing.T) { func TestValueRecorder(t *testing.T) { t.Run("float64 valuerecorder", func(t *testing.T) { - mockSDK, meter := metrictest.NewMeter() + provider, meter := testPair() m := Must(meter).NewFloat64ValueRecorder("test.valuerecorder.float") ctx := context.Background() labels := []attribute.KeyValue{} @@ -373,12 +309,12 @@ func TestValueRecorder(t *testing.T) { boundInstrument := m.Bind(labels...) boundInstrument.Record(ctx, 0) meter.RecordBatch(ctx, labels, m.Measurement(-100.5)) - checkSyncBatches(ctx, t, labels, mockSDK, number.Float64Kind, sdkapi.ValueRecorderInstrumentKind, m.SyncImpl(), + checkSyncBatches(ctx, t, labels, provider, number.Float64Kind, sdkapi.ValueRecorderInstrumentKind, m.SyncImpl(), 42, 0, -100.5, ) }) t.Run("int64 valuerecorder", func(t *testing.T) { - mockSDK, meter := metrictest.NewMeter() + provider, meter := testPair() m := Must(meter).NewInt64ValueRecorder("test.valuerecorder.int") ctx := context.Background() labels := []attribute.KeyValue{attribute.Int("I", 1)} @@ -386,7 +322,7 @@ func TestValueRecorder(t *testing.T) { boundInstrument := m.Bind(labels...) boundInstrument.Record(ctx, 80) meter.RecordBatch(ctx, labels, m.Measurement(0)) - checkSyncBatches(ctx, t, labels, mockSDK, number.Int64Kind, sdkapi.ValueRecorderInstrumentKind, m.SyncImpl(), + checkSyncBatches(ctx, t, labels, provider, number.Int64Kind, sdkapi.ValueRecorderInstrumentKind, m.SyncImpl(), 173, 80, 0, ) }) @@ -395,74 +331,74 @@ func TestValueRecorder(t *testing.T) { func TestObserverInstruments(t *testing.T) { t.Run("float valueobserver", func(t *testing.T) { labels := []attribute.KeyValue{attribute.String("O", "P")} - mockSDK, meter := metrictest.NewMeter() + provider, meter := testPair() o := Must(meter).NewFloat64ValueObserver("test.valueobserver.float", func(_ context.Context, result metric.Float64ObserverResult) { result.Observe(42.1, labels...) }) - mockSDK.RunAsyncInstruments() - checkObserverBatch(t, labels, mockSDK, number.Float64Kind, sdkapi.ValueObserverInstrumentKind, o.AsyncImpl(), + provider.RunAsyncInstruments() + checkObserverBatch(t, labels, provider, number.Float64Kind, sdkapi.ValueObserverInstrumentKind, o.AsyncImpl(), 42.1, ) }) t.Run("int valueobserver", func(t *testing.T) { labels := []attribute.KeyValue{} - mockSDK, meter := metrictest.NewMeter() + provider, meter := testPair() o := Must(meter).NewInt64ValueObserver("test.observer.int", func(_ context.Context, result metric.Int64ObserverResult) { result.Observe(-142, labels...) }) - mockSDK.RunAsyncInstruments() - checkObserverBatch(t, labels, mockSDK, number.Int64Kind, sdkapi.ValueObserverInstrumentKind, o.AsyncImpl(), + provider.RunAsyncInstruments() + checkObserverBatch(t, labels, provider, number.Int64Kind, sdkapi.ValueObserverInstrumentKind, o.AsyncImpl(), -142, ) }) t.Run("float sumobserver", func(t *testing.T) { labels := []attribute.KeyValue{attribute.String("O", "P")} - mockSDK, meter := metrictest.NewMeter() + provider, meter := testPair() o := Must(meter).NewFloat64SumObserver("test.sumobserver.float", func(_ context.Context, result metric.Float64ObserverResult) { result.Observe(42.1, labels...) }) - mockSDK.RunAsyncInstruments() - checkObserverBatch(t, labels, mockSDK, number.Float64Kind, sdkapi.SumObserverInstrumentKind, o.AsyncImpl(), + provider.RunAsyncInstruments() + checkObserverBatch(t, labels, provider, number.Float64Kind, sdkapi.SumObserverInstrumentKind, o.AsyncImpl(), 42.1, ) }) t.Run("int sumobserver", func(t *testing.T) { labels := []attribute.KeyValue{} - mockSDK, meter := metrictest.NewMeter() + provider, meter := testPair() o := Must(meter).NewInt64SumObserver("test.observer.int", func(_ context.Context, result metric.Int64ObserverResult) { result.Observe(-142, labels...) }) - mockSDK.RunAsyncInstruments() - checkObserverBatch(t, labels, mockSDK, number.Int64Kind, sdkapi.SumObserverInstrumentKind, o.AsyncImpl(), + provider.RunAsyncInstruments() + checkObserverBatch(t, labels, provider, number.Int64Kind, sdkapi.SumObserverInstrumentKind, o.AsyncImpl(), -142, ) }) t.Run("float updownsumobserver", func(t *testing.T) { labels := []attribute.KeyValue{attribute.String("O", "P")} - mockSDK, meter := metrictest.NewMeter() + provider, meter := testPair() o := Must(meter).NewFloat64UpDownSumObserver("test.updownsumobserver.float", func(_ context.Context, result metric.Float64ObserverResult) { result.Observe(42.1, labels...) }) - mockSDK.RunAsyncInstruments() - checkObserverBatch(t, labels, mockSDK, number.Float64Kind, sdkapi.UpDownSumObserverInstrumentKind, o.AsyncImpl(), + provider.RunAsyncInstruments() + checkObserverBatch(t, labels, provider, number.Float64Kind, sdkapi.UpDownSumObserverInstrumentKind, o.AsyncImpl(), 42.1, ) }) t.Run("int updownsumobserver", func(t *testing.T) { labels := []attribute.KeyValue{} - mockSDK, meter := metrictest.NewMeter() + provider, meter := testPair() o := Must(meter).NewInt64UpDownSumObserver("test.observer.int", func(_ context.Context, result metric.Int64ObserverResult) { result.Observe(-142, labels...) }) - mockSDK.RunAsyncInstruments() - checkObserverBatch(t, labels, mockSDK, number.Int64Kind, sdkapi.UpDownSumObserverInstrumentKind, o.AsyncImpl(), + provider.RunAsyncInstruments() + checkObserverBatch(t, labels, provider, number.Int64Kind, sdkapi.UpDownSumObserverInstrumentKind, o.AsyncImpl(), -142, ) }) } func TestBatchObserverInstruments(t *testing.T) { - mockSDK, meter := metrictest.NewMeter() + provider, meter := testPair() var obs1 metric.Int64ValueObserver var obs2 metric.Float64ValueObserver @@ -483,9 +419,9 @@ func TestBatchObserverInstruments(t *testing.T) { obs1 = cb.NewInt64ValueObserver("test.observer.int") obs2 = cb.NewFloat64ValueObserver("test.observer.float") - mockSDK.RunAsyncInstruments() + provider.RunAsyncInstruments() - require.Len(t, mockSDK.MeasurementBatches, 1) + require.Len(t, provider.MeasurementBatches, 1) impl1 := obs1.AsyncImpl().Implementation().(*metrictest.Async) impl2 := obs2.AsyncImpl().Implementation().(*metrictest.Async) @@ -493,7 +429,7 @@ func TestBatchObserverInstruments(t *testing.T) { require.NotNil(t, impl1) require.NotNil(t, impl2) - got := mockSDK.MeasurementBatches[0] + got := provider.MeasurementBatches[0] require.Equal(t, labels, got.Labels) require.Len(t, got.Measurements, 2) @@ -506,17 +442,17 @@ func TestBatchObserverInstruments(t *testing.T) { require.Equal(t, 0, m2.Number.CompareNumber(number.Float64Kind, metrictest.ResolveNumberByKind(t, number.Float64Kind, 42))) } -func checkObserverBatch(t *testing.T, labels []attribute.KeyValue, mock *metrictest.MeterImpl, nkind number.Kind, mkind sdkapi.InstrumentKind, observer metric.AsyncImpl, expected float64) { +func checkObserverBatch(t *testing.T, labels []attribute.KeyValue, provider *metrictest.MeterProvider, nkind number.Kind, mkind sdkapi.InstrumentKind, observer metric.AsyncImpl, expected float64) { t.Helper() - assert.Len(t, mock.MeasurementBatches, 1) - if len(mock.MeasurementBatches) < 1 { + assert.Len(t, provider.MeasurementBatches, 1) + if len(provider.MeasurementBatches) < 1 { return } o := observer.Implementation().(*metrictest.Async) if !assert.NotNil(t, o) { return } - got := mock.MeasurementBatches[0] + got := provider.MeasurementBatches[0] assert.Equal(t, labels, got.Labels) assert.Len(t, got.Measurements, 1) if len(got.Measurements) < 1 { @@ -547,7 +483,7 @@ func (testWrappedMeter) NewAsyncInstrument(_ metric.Descriptor, _ metric.AsyncRu func TestWrappedInstrumentError(t *testing.T) { impl := &testWrappedMeter{} - meter := metric.WrapMeterImpl(impl, "test") + meter := metric.WrapMeterImpl(impl) valuerecorder, err := meter.NewInt64ValueRecorder("test.valuerecorder") @@ -562,7 +498,7 @@ func TestWrappedInstrumentError(t *testing.T) { func TestNilCallbackObserverNoop(t *testing.T) { // Tests that a nil callback yields a no-op observer without error. - _, meter := metrictest.NewMeter() + _, meter := testPair() observer := Must(meter).NewInt64ValueObserver("test.observer", nil) diff --git a/metric/metrictest/meter.go b/metric/metrictest/meter.go index a70d12b0de8..764fba48f38 100644 --- a/metric/metrictest/meter.go +++ b/metric/metrictest/meter.go @@ -23,7 +23,6 @@ import ( internalmetric "go.opentelemetry.io/otel/internal/metric" "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/metric/number" - "go.opentelemetry.io/otel/metric/registry" ) type ( @@ -32,21 +31,33 @@ type ( Labels []attribute.KeyValue } + Library struct { + InstrumentationName string + InstrumentationVersion string + SchemaURL string + } + Batch struct { // Measurement needs to be aligned for 64-bit atomic operations. Measurements []Measurement Ctx context.Context Labels []attribute.KeyValue - LibraryName string + Library Library } // MeterImpl is an OpenTelemetry Meter implementation used for testing. MeterImpl struct { + library Library + provider *MeterProvider + asyncInstruments *internalmetric.AsyncInstrumentState + } + + // MeterProvider is a collection of named MeterImpls used for testing. + MeterProvider struct { lock sync.Mutex MeasurementBatches []Batch - - asyncInstruments *internalmetric.AsyncInstrumentState + impls []*MeterImpl } Measurement struct { @@ -115,21 +126,30 @@ func (m *MeterImpl) doRecordSingle(ctx context.Context, labels []attribute.KeyVa }}) } -func NewMeterProvider() (*MeterImpl, metric.MeterProvider) { +func NewMeterProvider() *MeterProvider { + return &MeterProvider{} +} + +func (p *MeterProvider) Meter(name string, opts ...metric.MeterOption) metric.Meter { + p.lock.Lock() + defer p.lock.Unlock() + cfg := metric.NewMeterConfig(opts...) impl := &MeterImpl{ + library: Library{ + InstrumentationName: name, + InstrumentationVersion: cfg.InstrumentationVersion(), + SchemaURL: cfg.SchemaURL(), + }, + provider: p, asyncInstruments: internalmetric.NewAsyncInstrumentState(), } - return impl, registry.NewMeterProvider(impl) -} - -func NewMeter() (*MeterImpl, metric.Meter) { - impl, p := NewMeterProvider() - return impl, p.Meter("mock") + p.impls = append(p.impls, impl) + return metric.WrapMeterImpl(impl) } func (m *MeterImpl) NewSyncInstrument(descriptor metric.Descriptor) (metric.SyncImpl, error) { - m.lock.Lock() - defer m.lock.Unlock() + m.provider.lock.Lock() + defer m.provider.lock.Unlock() return &Sync{ Instrument{ @@ -140,8 +160,8 @@ func (m *MeterImpl) NewSyncInstrument(descriptor metric.Descriptor) (metric.Sync } func (m *MeterImpl) NewAsyncInstrument(descriptor metric.Descriptor, runner metric.AsyncRunner) (metric.AsyncImpl, error) { - m.lock.Lock() - defer m.lock.Unlock() + m.provider.lock.Lock() + defer m.provider.lock.Unlock() a := &Async{ Instrument: Instrument{ @@ -179,28 +199,29 @@ func (m *MeterImpl) CollectAsync(labels []attribute.KeyValue, obs ...metric.Obse } func (m *MeterImpl) collect(ctx context.Context, labels []attribute.KeyValue, measurements []Measurement) { - m.lock.Lock() - defer m.lock.Unlock() - - m.MeasurementBatches = append(m.MeasurementBatches, Batch{ + m.provider.MeasurementBatches = append(m.provider.MeasurementBatches, Batch{ Ctx: ctx, Labels: labels, Measurements: measurements, + Library: m.library, }) } -func (m *MeterImpl) RunAsyncInstruments() { - m.asyncInstruments.Run(context.Background(), m) +func (p *MeterProvider) RunAsyncInstruments() { + p.lock.Lock() + defer p.lock.Unlock() + for _, impl := range p.impls { + impl.asyncInstruments.Run(context.Background(), impl) + } } // Measured is the helper struct which provides flat representation of recorded measurements // to simplify testing type Measured struct { - Name string - InstrumentationName string - InstrumentationVersion string - Labels map[attribute.Key]attribute.Value - Number number.Number + Name string + Labels map[attribute.Key]attribute.Value + Number number.Number + Library Library } // LabelsToMap converts label set to keyValue map, to be easily used in tests @@ -218,11 +239,10 @@ func AsStructs(batches []Batch) []Measured { for _, batch := range batches { for _, m := range batch.Measurements { r = append(r, Measured{ - Name: m.Instrument.Descriptor().Name(), - InstrumentationName: m.Instrument.Descriptor().InstrumentationName(), - InstrumentationVersion: m.Instrument.Descriptor().InstrumentationVersion(), - Labels: LabelsToMap(batch.Labels...), - Number: m.Number, + Name: m.Instrument.Descriptor().Name(), + Labels: LabelsToMap(batch.Labels...), + Number: m.Number, + Library: batch.Library, }) } } diff --git a/metric/registry/registry.go b/metric/registry/registry.go index 0a42a0fdf8d..f8e504aaa6f 100644 --- a/metric/registry/registry.go +++ b/metric/registry/registry.go @@ -25,52 +25,88 @@ import ( // MeterProvider is a standard MeterProvider for wrapping `MeterImpl` type MeterProvider struct { - impl metric.MeterImpl + lock sync.Mutex + impl metric.MeterProvider + meters map[uniqueMeterKey]metric.Meter } -var _ metric.MeterProvider = (*MeterProvider)(nil) - -// uniqueInstrumentMeterImpl implements the metric.MeterImpl interface, adding -// uniqueness checking for instrument descriptors. Use NewUniqueInstrumentMeter -// to wrap an implementation with uniqueness checking. -type uniqueInstrumentMeterImpl struct { - lock sync.Mutex - impl metric.MeterImpl - state map[key]metric.InstrumentImpl +type uniqueMeterKey struct { + name string + version string + schemaURL string } -var _ metric.MeterImpl = (*uniqueInstrumentMeterImpl)(nil) - -type key struct { - instrumentName string - instrumentationName string - InstrumentationVersion string +func keyOf(name string, opts ...metric.MeterOption) uniqueMeterKey { + cfg := metric.NewMeterConfig(opts...) + return uniqueMeterKey{ + name: name, + version: cfg.InstrumentationVersion(), + schemaURL: cfg.SchemaURL(), + } } -// NewMeterProvider returns a new provider that implements instrument +var _ metric.MeterProvider = (*MeterProvider)(nil) + +// NewMeterProvider returns a new provider that implements meter // name-uniqueness checking. -func NewMeterProvider(impl metric.MeterImpl) *MeterProvider { +func NewMeterProvider(impl metric.MeterProvider) *MeterProvider { return &MeterProvider{ - impl: NewUniqueInstrumentMeterImpl(impl), + impl: impl, + meters: map[uniqueMeterKey]metric.Meter{}, } } // Meter implements MeterProvider. func (p *MeterProvider) Meter(instrumentationName string, opts ...metric.MeterOption) metric.Meter { - return metric.WrapMeterImpl(p.impl, instrumentationName, opts...) + k := keyOf(instrumentationName, opts...) + p.lock.Lock() + defer p.lock.Unlock() + m, ok := p.meters[k] + if !ok { + m = metric.WrapMeterImpl( + NewMeterImpl( + p.impl.Meter(instrumentationName, opts...).MeterImpl())) + p.meters[k] = m + } + return m + } +// List provides a list of MeterImpl objects created through this +// provider. +func (p *MeterProvider) List() []metric.MeterImpl { + p.lock.Lock() + defer p.lock.Unlock() + + var r []metric.MeterImpl + for _, meter := range p.meters { + r = append(r, meter.MeterImpl().(*uniqueInstrumentMeterImpl).impl) + } + return r +} + +// uniqueInstrumentMeterImpl implements the metric.MeterImpl interface, adding +// uniqueness checking for instrument descriptors. Use NewMeter +// to wrap an implementation with uniqueness checking. +type uniqueInstrumentMeterImpl struct { + lock sync.Mutex + impl metric.MeterImpl + state map[string]metric.InstrumentImpl +} + +var _ metric.MeterImpl = (*uniqueInstrumentMeterImpl)(nil) + // ErrMetricKindMismatch is the standard error for mismatched metric // instrument definitions. var ErrMetricKindMismatch = fmt.Errorf( "a metric was already registered by this name with another kind or number type") -// NewUniqueInstrumentMeterImpl returns a wrapped metric.MeterImpl with +// NewMeterImpl returns a wrapped metric.MeterImpl with // the addition of uniqueness checking. -func NewUniqueInstrumentMeterImpl(impl metric.MeterImpl) metric.MeterImpl { +func NewMeterImpl(impl metric.MeterImpl) metric.MeterImpl { return &uniqueInstrumentMeterImpl{ impl: impl, - state: map[key]metric.InstrumentImpl{}, + state: map[string]metric.InstrumentImpl{}, } } @@ -79,21 +115,11 @@ func (u *uniqueInstrumentMeterImpl) RecordBatch(ctx context.Context, labels []at u.impl.RecordBatch(ctx, labels, ms...) } -func keyOf(descriptor metric.Descriptor) key { - return key{ - descriptor.Name(), - descriptor.InstrumentationName(), - descriptor.InstrumentationVersion(), - } -} - // NewMetricKindMismatchError formats an error that describes a // mismatched metric instrument definition. func NewMetricKindMismatchError(desc metric.Descriptor) error { - return fmt.Errorf("metric was %s (%s %s)registered as a %s %s: %w", + return fmt.Errorf("metric %s registered as %s %s: %w", desc.Name(), - desc.InstrumentationName(), - desc.InstrumentationVersion(), desc.NumberKind(), desc.InstrumentKind(), ErrMetricKindMismatch) @@ -112,7 +138,7 @@ func Compatible(candidate, existing metric.Descriptor) bool { // registration, this returns the already-registered instrument. If // there is no conflict and no prior registration, returns (nil, nil). func (u *uniqueInstrumentMeterImpl) checkUniqueness(descriptor metric.Descriptor) (metric.InstrumentImpl, error) { - impl, ok := u.state[keyOf(descriptor)] + impl, ok := u.state[descriptor.Name()] if !ok { return nil, nil } @@ -141,7 +167,7 @@ func (u *uniqueInstrumentMeterImpl) NewSyncInstrument(descriptor metric.Descript if err != nil { return nil, err } - u.state[keyOf(descriptor)] = syncInst + u.state[descriptor.Name()] = syncInst return syncInst, nil } @@ -165,6 +191,6 @@ func (u *uniqueInstrumentMeterImpl) NewAsyncInstrument( if err != nil { return nil, err } - u.state[keyOf(descriptor)] = asyncInst + u.state[descriptor.Name()] = asyncInst return asyncInst, nil } diff --git a/metric/registry/registry_test.go b/metric/registry/registry_test.go index 3e021f13cd3..df836f1ea6c 100644 --- a/metric/registry/registry_test.go +++ b/metric/registry/registry_test.go @@ -72,7 +72,7 @@ func unwrap(impl interface{}, err error) (metric.InstrumentImpl, error) { func TestRegistrySameInstruments(t *testing.T) { for _, nf := range allNew { - _, provider := metrictest.NewMeterProvider() + provider := registry.NewMeterProvider(metrictest.NewMeterProvider()) meter := provider.Meter("meter") inst1, err1 := nf(meter, "this") @@ -86,7 +86,7 @@ func TestRegistrySameInstruments(t *testing.T) { func TestRegistryDifferentNamespace(t *testing.T) { for _, nf := range allNew { - _, provider := metrictest.NewMeterProvider() + provider := metrictest.NewMeterProvider() meter1 := provider.Meter("meter1") meter2 := provider.Meter("meter2") @@ -101,7 +101,7 @@ func TestRegistryDifferentNamespace(t *testing.T) { func TestRegistryDiffInstruments(t *testing.T) { for origName, origf := range allNew { - _, provider := metrictest.NewMeterProvider() + provider := registry.NewMeterProvider(metrictest.NewMeterProvider()) meter := provider.Meter("meter") _, err := origf(meter, "this") @@ -121,8 +121,8 @@ func TestRegistryDiffInstruments(t *testing.T) { } func TestMeterProvider(t *testing.T) { - impl, _ := metrictest.NewMeter() - p := registry.NewMeterProvider(impl) + provider := metrictest.NewMeterProvider() + p := registry.NewMeterProvider(provider) m1 := p.Meter("m1") m1p := p.Meter("m1") m2 := p.Meter("m2") From b016889f10db05d1600a65607de2bfa548182521 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Wed, 18 Aug 2021 16:39:39 -0700 Subject: [PATCH 02/33] SDK tests pass --- metric/metrictest/meter.go | 6 + sdk/export/metric/metric.go | 25 ++- sdk/metric/aggregator/aggregator_test.go | 5 +- sdk/metric/aggregator/aggregatortest/test.go | 3 +- sdk/metric/benchmark_test.go | 2 +- sdk/metric/controller/basic/controller.go | 146 ++++++++++++------ .../controller/basic/controller_test.go | 30 ++-- sdk/metric/controller/basic/pull_test.go | 18 ++- sdk/metric/controller/basic/push_test.go | 18 +-- sdk/metric/controller/controllertest/test.go | 17 ++ sdk/metric/correct_test.go | 2 +- sdk/metric/histogram_stress_test.go | 4 +- sdk/metric/minmaxsumcount_stress_test.go | 4 +- sdk/metric/processor/basic/basic.go | 47 ++++-- sdk/metric/processor/basic/basic_test.go | 47 +++--- sdk/metric/processor/processortest/test.go | 74 ++++++--- .../processor/processortest/test_test.go | 21 ++- sdk/metric/processor/reducer/reducer_test.go | 10 +- sdk/metric/selector/simple/simple_test.go | 13 +- sdk/metric/stress_test.go | 4 +- 20 files changed, 331 insertions(+), 165 deletions(-) diff --git a/metric/metrictest/meter.go b/metric/metrictest/meter.go index 764fba48f38..54c1c59454c 100644 --- a/metric/metrictest/meter.go +++ b/metric/metrictest/meter.go @@ -23,6 +23,7 @@ import ( internalmetric "go.opentelemetry.io/otel/internal/metric" "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/metric/number" + "go.opentelemetry.io/otel/metric/sdkapi" ) type ( @@ -89,6 +90,11 @@ var ( _ metric.AsyncImpl = &Async{} ) +func NewDescriptor(name string, ikind sdkapi.InstrumentKind, nkind number.Kind, opts ...metric.InstrumentOption) metric.Descriptor { + cfg := metric.NewInstrumentConfig(opts...) + return metric.NewDescriptor(name, ikind, nkind, cfg.Description(), cfg.Unit()) +} + func (i Instrument) Descriptor() metric.Descriptor { return i.descriptor } diff --git a/sdk/export/metric/metric.go b/sdk/export/metric/metric.go index 77b42fa1c5d..3cd2c204ab9 100644 --- a/sdk/export/metric/metric.go +++ b/sdk/export/metric/metric.go @@ -26,6 +26,7 @@ import ( "go.opentelemetry.io/otel/metric/number" "go.opentelemetry.io/otel/metric/sdkapi" "go.opentelemetry.io/otel/sdk/export/metric/aggregation" + "go.opentelemetry.io/otel/sdk/instrumentation" "go.opentelemetry.io/otel/sdk/resource" ) @@ -111,13 +112,13 @@ type Checkpointer interface { // any time. Processor - // CheckpointSet returns the current data set. This may be + // MetricReader returns the current data set. This may be // called before and after collection. The // implementation is required to return the same value - // throughout its lifetime, since CheckpointSet exposes a + // throughout its lifetime, since MetricReader exposes a // sync.Locker interface. The caller is responsible for - // locking the CheckpointSet before initiating collection. - CheckpointSet() CheckpointSet + // locking the MetricReader before initiating collection. + MetricReader() MetricReader // StartCollection begins a collection interval. StartCollection() @@ -126,6 +127,12 @@ type Checkpointer interface { FinishCollection() error } +// CheckpointerFactory is an interface for producing configured +// Checkpointer instances. +type CheckpointerFactory interface { + NewCheckpointer() Checkpointer +} + // Aggregator implements a specific aggregation behavior, e.g., a // behavior to track a sequence of updates to an instrument. Sum-only // instruments commonly use a simple Sum aggregator, but for the @@ -211,7 +218,7 @@ type Exporter interface { // // The CheckpointSet interface refers to the Processor that just // completed collection. - Export(ctx context.Context, resource *resource.Resource, checkpointSet CheckpointSet) error + Export(ctx context.Context, resource *resource.Resource, reader InstrumentationLibraryMetricReader) error // ExportKindSelector is an interface used by the Processor // in deciding whether to compute Delta or Cumulative @@ -219,6 +226,10 @@ type Exporter interface { ExportKindSelector } +type InstrumentationLibraryMetricReader interface { + ForEach(readerFunc func(instrumentation.Library, MetricReader) error) error +} + // ExportKindSelector is a sub-interface of Exporter used to indicate // whether the Processor should compute Delta or Cumulative // Aggregations. @@ -229,11 +240,11 @@ type ExportKindSelector interface { ExportKindFor(descriptor *metric.Descriptor, aggregatorKind aggregation.Kind) ExportKind } -// CheckpointSet allows a controller to access a complete checkpoint of +// MetricReader allows a controller to access a complete checkpoint of // aggregated metrics from the Processor. This is passed to the // Exporter which may then use ForEach to iterate over the collection // of aggregated metrics. -type CheckpointSet interface { +type MetricReader interface { // ForEach iterates over aggregated checkpoints for all // metrics that were updated during the last collection // period. Each aggregated checkpoint returned by the diff --git a/sdk/metric/aggregator/aggregator_test.go b/sdk/metric/aggregator/aggregator_test.go index 7fcceeb790f..6120e7752c6 100644 --- a/sdk/metric/aggregator/aggregator_test.go +++ b/sdk/metric/aggregator/aggregator_test.go @@ -22,6 +22,7 @@ import ( "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/metric/metrictest" "go.opentelemetry.io/otel/metric/number" "go.opentelemetry.io/otel/metric/sdkapi" "go.opentelemetry.io/otel/sdk/export/metric/aggregation" @@ -74,7 +75,7 @@ func TestRangeTest(t *testing.T) { // Only Counters implement a range test. for _, nkind := range []number.Kind{number.Float64Kind, number.Int64Kind} { t.Run(nkind.String(), func(t *testing.T) { - desc := metric.NewDescriptor( + desc := metrictest.NewDescriptor( "name", sdkapi.CounterInstrumentKind, nkind, @@ -92,7 +93,7 @@ func TestNaNTest(t *testing.T) { sdkapi.ValueRecorderInstrumentKind, sdkapi.ValueObserverInstrumentKind, } { - desc := metric.NewDescriptor( + desc := metrictest.NewDescriptor( "name", mkind, nkind, diff --git a/sdk/metric/aggregator/aggregatortest/test.go b/sdk/metric/aggregator/aggregatortest/test.go index a4abd658c7a..a6b5a964977 100644 --- a/sdk/metric/aggregator/aggregatortest/test.go +++ b/sdk/metric/aggregator/aggregatortest/test.go @@ -27,6 +27,7 @@ import ( ottest "go.opentelemetry.io/otel/internal/internaltest" "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/metric/metrictest" "go.opentelemetry.io/otel/metric/number" "go.opentelemetry.io/otel/metric/sdkapi" export "go.opentelemetry.io/otel/sdk/export/metric" @@ -66,7 +67,7 @@ func newProfiles() []Profile { } func NewAggregatorTest(mkind sdkapi.InstrumentKind, nkind number.Kind) *metric.Descriptor { - desc := metric.NewDescriptor("test.name", mkind, nkind) + desc := metrictest.NewDescriptor("test.name", mkind, nkind) return &desc } diff --git a/sdk/metric/benchmark_test.go b/sdk/metric/benchmark_test.go index b29adf0a06c..4e4c76b54d0 100644 --- a/sdk/metric/benchmark_test.go +++ b/sdk/metric/benchmark_test.go @@ -43,7 +43,7 @@ func newFixture(b *testing.B) *benchFixture { } bf.accumulator = sdk.NewAccumulator(bf) - bf.meter = metric.WrapMeterImpl(bf.accumulator, "benchmarks") + bf.meter = metric.WrapMeterImpl(bf.accumulator) return bf } diff --git a/sdk/metric/controller/basic/controller.go b/sdk/metric/controller/basic/controller.go index 0f79e025087..73ea36ca4c7 100644 --- a/sdk/metric/controller/basic/controller.go +++ b/sdk/metric/controller/basic/controller.go @@ -24,6 +24,7 @@ import ( "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/metric/registry" export "go.opentelemetry.io/otel/sdk/export/metric" + "go.opentelemetry.io/otel/sdk/instrumentation" sdk "go.opentelemetry.io/otel/sdk/metric" controllerTime "go.opentelemetry.io/otel/sdk/metric/controller/time" "go.opentelemetry.io/otel/sdk/resource" @@ -55,16 +56,17 @@ var ErrControllerStarted = fmt.Errorf("controller already started") // using the export.CheckpointSet RWLock interface. Collection will // be blocked by a pull request in the basic controller. type Controller struct { - lock sync.Mutex - accumulator *sdk.Accumulator - provider *registry.MeterProvider - checkpointer export.Checkpointer - resource *resource.Resource - exporter export.Exporter - wg sync.WaitGroup - stopCh chan struct{} - clock controllerTime.Clock - ticker controllerTime.Ticker + lock sync.Mutex + provider *registry.MeterProvider + checkpointerFactory export.CheckpointerFactory + accumulatorProvider accumulatorProvider + + resource *resource.Resource + exporter export.Exporter + wg sync.WaitGroup + stopCh chan struct{} + clock controllerTime.Clock + ticker controllerTime.Ticker collectPeriod time.Duration collectTimeout time.Duration @@ -75,10 +77,37 @@ type Controller struct { collectedTime time.Time } +type accumulatorProvider struct { + controller *Controller +} + +var _ metric.MeterProvider = &accumulatorProvider{} + +func (a *accumulatorProvider) Meter(instrumentationName string, opts ...metric.MeterOption) metric.Meter { + checkpointer := a.controller.checkpointerFactory.NewCheckpointer() + accumulator := sdk.NewAccumulator(checkpointer) + cfg := metric.NewMeterConfig(opts...) + return metric.WrapMeterImpl(&accumulatorCheckpointer{ + Accumulator: accumulator, + checkpointer: checkpointer, + library: instrumentation.Library{ + Name: instrumentationName, + Version: cfg.InstrumentationVersion(), + SchemaURL: cfg.SchemaURL(), + }, + }) +} + +type accumulatorCheckpointer struct { + *sdk.Accumulator + checkpointer export.Checkpointer + library instrumentation.Library +} + // New constructs a Controller using the provided checkpointer and // options (including optional exporter) to configure a metric // export pipeline. -func New(checkpointer export.Checkpointer, opts ...Option) *Controller { +func New(checkpointerFactory export.CheckpointerFactory, opts ...Option) *Controller { c := &config{ CollectPeriod: DefaultPeriod, CollectTimeout: DefaultPeriod, @@ -96,20 +125,20 @@ func New(checkpointer export.Checkpointer, opts ...Option) *Controller { otel.Handle(err) } } - impl := sdk.NewAccumulator(checkpointer) - return &Controller{ - provider: registry.NewMeterProvider(impl), - accumulator: impl, - checkpointer: checkpointer, - resource: c.Resource, - exporter: c.Exporter, - stopCh: nil, - clock: controllerTime.RealClock{}, + cont := &Controller{ + checkpointerFactory: checkpointerFactory, + exporter: c.Exporter, + resource: c.Resource, + stopCh: nil, + clock: controllerTime.RealClock{}, collectPeriod: c.CollectPeriod, collectTimeout: c.CollectTimeout, pushTimeout: c.PushTimeout, } + cont.accumulatorProvider.controller = cont + cont.provider = registry.NewMeterProvider(&cont.accumulatorProvider) + return cont } // SetClock supports setting a mock clock for testing. This must be @@ -131,6 +160,12 @@ func (c *Controller) Resource() *resource.Resource { return c.resource } +// Reader returns an InstrumentationLibraryMetricReader for iterating +// through the metrics of each registered library, one at a time. +func (c *Controller) Reader() export.InstrumentationLibraryMetricReader { + return libraryReader{c} +} + // Start begins a ticker that periodically collects and exports // metrics with the configured interval. This is required for calling // a configured Exporter (see WithExporter) and is otherwise optional @@ -198,9 +233,7 @@ func (c *Controller) runTicker(ctx context.Context, stopCh chan struct{}) { // collect computes a checkpoint and optionally exports it. func (c *Controller) collect(ctx context.Context) error { - if err := c.checkpoint(ctx, func() bool { - return true - }); err != nil { + if err := c.checkpoint(ctx); err != nil { return err } if c.exporter == nil { @@ -216,15 +249,21 @@ func (c *Controller) collect(ctx context.Context) error { // compute the CheckpointSet. This applies the configured collection // timeout. Note that this does not try to cancel a Collect or Export // when Stop() is called. -func (c *Controller) checkpoint(ctx context.Context, cond func() bool) error { - ckpt := c.checkpointer.CheckpointSet() +func (c *Controller) checkpoint(ctx context.Context) error { + for _, impl := range c.provider.List() { + if err := c.checkpoint1(ctx, impl.(*accumulatorCheckpointer)); err != nil { + return err + } + } + return nil +} + +func (c *Controller) checkpoint1(ctx context.Context, ac *accumulatorCheckpointer) error { + ckpt := ac.checkpointer.MetricReader() ckpt.Lock() defer ckpt.Unlock() - if !cond() { - return nil - } - c.checkpointer.StartCollection() + ac.checkpointer.StartCollection() if c.collectTimeout > 0 { var cancel context.CancelFunc @@ -232,7 +271,7 @@ func (c *Controller) checkpoint(ctx context.Context, cond func() bool) error { defer cancel() } - _ = c.accumulator.Collect(ctx) + _ = ac.Accumulator.Collect(ctx) var err error select { @@ -243,7 +282,7 @@ func (c *Controller) checkpoint(ctx context.Context, cond func() bool) error { } // Finish the checkpoint whether the accumulator timed out or not. - if cerr := c.checkpointer.FinishCollection(); cerr != nil { + if cerr := ac.checkpointer.FinishCollection(); cerr != nil { if err == nil { err = cerr } else { @@ -254,34 +293,41 @@ func (c *Controller) checkpoint(ctx context.Context, cond func() bool) error { return err } -// export calls the exporter with a read lock on the CheckpointSet, +// export calls the exporter with a read lock on the MetricReader, // applying the configured export timeout. func (c *Controller) export(ctx context.Context) error { - ckpt := c.checkpointer.CheckpointSet() - ckpt.RLock() - defer ckpt.RUnlock() - if c.pushTimeout > 0 { var cancel context.CancelFunc ctx, cancel = context.WithTimeout(ctx, c.pushTimeout) defer cancel() } - return c.exporter.Export(ctx, c.resource, ckpt) + return c.exporter.Export(ctx, c.resource, c.Reader()) } -// ForEach gives the caller read-locked access to the current -// export.CheckpointSet. -func (c *Controller) ForEach(ks export.ExportKindSelector, f func(export.Record) error) error { - ckpt := c.checkpointer.CheckpointSet() - ckpt.RLock() - defer ckpt.RUnlock() +type libraryReader struct { + *Controller +} - return ckpt.ForEach(ks, f) +var _ export.InstrumentationLibraryMetricReader = libraryReader{} + +func (l libraryReader) ForEach(readerFunc func(l instrumentation.Library, r export.MetricReader) error) error { + for _, impl := range l.Controller.provider.List() { + pair := impl.(*accumulatorCheckpointer) + reader := pair.checkpointer.MetricReader() + if err := func() error { + reader.RLock() + defer reader.RUnlock() + return readerFunc(pair.library, reader) + }(); err != nil { + return err + } + } + return nil } // IsRunning returns true if the controller was started via Start(), -// indicating that the current export.CheckpointSet is being kept +// indicating that the current export.MetricReader is being kept // up-to-date. func (c *Controller) IsRunning() bool { c.lock.Lock() @@ -298,16 +344,20 @@ func (c *Controller) Collect(ctx context.Context) error { // computing checkpoints with the collection period. return ErrControllerStarted } + if !c.shouldCollect() { + return nil + } - return c.checkpoint(ctx, c.shouldCollect) + return c.checkpoint(ctx) } // shouldCollect returns true if the collector should collect now, // based on the timestamp, the last collection time, and the // configured period. func (c *Controller) shouldCollect() bool { - // This is called with the CheckpointSet exclusive - // lock held. + c.lock.Lock() + defer c.lock.Unlock() + if c.collectPeriod == 0 { return true } diff --git a/sdk/metric/controller/basic/controller_test.go b/sdk/metric/controller/basic/controller_test.go index b67ce74ee4e..c18b8f7fed1 100644 --- a/sdk/metric/controller/basic/controller_test.go +++ b/sdk/metric/controller/basic/controller_test.go @@ -28,6 +28,7 @@ import ( "go.opentelemetry.io/otel/metric" export "go.opentelemetry.io/otel/sdk/export/metric" "go.opentelemetry.io/otel/sdk/export/metric/aggregation" + "go.opentelemetry.io/otel/sdk/instrumentation" controller "go.opentelemetry.io/otel/sdk/metric/controller/basic" "go.opentelemetry.io/otel/sdk/metric/controller/controllertest" processor "go.opentelemetry.io/otel/sdk/metric/processor/basic" @@ -40,12 +41,15 @@ const envVar = "OTEL_RESOURCE_ATTRIBUTES" func getMap(t *testing.T, cont *controller.Controller) map[string]float64 { out := processortest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, cont.ForEach( - export.CumulativeExportKindSelector(), - func(record export.Record) error { - return out.AddRecord(record) - }, - )) + require.NoError(t, cont.Reader().ForEach( + func(_ instrumentation.Library, reader export.MetricReader) error { + return reader.ForEach( + export.CumulativeExportKindSelector(), + func(record export.Record) error { + return out.AddRecord(record) + }, + ) + })) return out.Map() } @@ -113,7 +117,7 @@ func TestControllerUsesResource(t *testing.T) { sel := export.CumulativeExportKindSelector() exp := processortest.New(sel, attribute.DefaultEncoder()) cont := controller.New( - processor.New( + processor.NewFactory( processortest.AggregatorSelector(), exp, ), @@ -139,7 +143,7 @@ func TestControllerUsesResource(t *testing.T) { func TestStartNoExporter(t *testing.T) { cont := controller.New( - processor.New( + processor.NewFactory( processortest.AggregatorSelector(), export.CumulativeExportKindSelector(), ), @@ -209,7 +213,7 @@ func TestStartNoExporter(t *testing.T) { func TestObserverCanceled(t *testing.T) { cont := controller.New( - processor.New( + processor.NewFactory( processortest.AggregatorSelector(), export.CumulativeExportKindSelector(), ), @@ -242,7 +246,7 @@ func TestObserverCanceled(t *testing.T) { func TestObserverContext(t *testing.T) { cont := controller.New( - processor.New( + processor.NewFactory( processortest.AggregatorSelector(), export.CumulativeExportKindSelector(), ), @@ -284,7 +288,7 @@ func newBlockingExporter() *blockingExporter { } } -func (b *blockingExporter) Export(ctx context.Context, res *resource.Resource, output export.CheckpointSet) error { +func (b *blockingExporter) Export(ctx context.Context, res *resource.Resource, output export.InstrumentationLibraryMetricReader) error { var err error _ = b.exporter.Export(ctx, res, output) if b.calls == 0 { @@ -306,7 +310,7 @@ func (*blockingExporter) ExportKindFor( func TestExportTimeout(t *testing.T) { exporter := newBlockingExporter() cont := controller.New( - processor.New( + processor.NewFactory( processortest.AggregatorSelector(), export.CumulativeExportKindSelector(), ), @@ -363,7 +367,7 @@ func TestCollectAfterStopThenStartAgain(t *testing.T) { attribute.DefaultEncoder(), ) cont := controller.New( - processor.New( + processor.NewFactory( processortest.AggregatorSelector(), exp, ), diff --git a/sdk/metric/controller/basic/pull_test.go b/sdk/metric/controller/basic/pull_test.go index b77a4fd57b4..7647db21cf1 100644 --- a/sdk/metric/controller/basic/pull_test.go +++ b/sdk/metric/controller/basic/pull_test.go @@ -34,7 +34,7 @@ import ( func TestPullNoCollect(t *testing.T) { puller := controller.New( - processor.New( + processor.NewFactory( processortest.AggregatorSelector(), export.CumulativeExportKindSelector(), processor.WithMemory(true), @@ -51,7 +51,11 @@ func TestPullNoCollect(t *testing.T) { require.NoError(t, puller.Collect(ctx)) records := processortest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, puller.ForEach(export.CumulativeExportKindSelector(), records.AddRecord)) + require.NoError(t, controllertest.ReadAll( + puller.Reader(), + export.CumulativeExportKindSelector(), + records.AddInstrumentationLibraryRecord, + )) require.EqualValues(t, map[string]float64{ "counter.sum/A=B/": 10, @@ -61,7 +65,7 @@ func TestPullNoCollect(t *testing.T) { require.NoError(t, puller.Collect(ctx)) records = processortest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, puller.ForEach(export.CumulativeExportKindSelector(), records.AddRecord)) + require.NoError(t, controllertest.ReadAll(puller.Reader(), export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) require.EqualValues(t, map[string]float64{ "counter.sum/A=B/": 20, @@ -70,7 +74,7 @@ func TestPullNoCollect(t *testing.T) { func TestPullWithCollect(t *testing.T) { puller := controller.New( - processor.New( + processor.NewFactory( processortest.AggregatorSelector(), export.CumulativeExportKindSelector(), processor.WithMemory(true), @@ -89,7 +93,7 @@ func TestPullWithCollect(t *testing.T) { require.NoError(t, puller.Collect(ctx)) records := processortest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, puller.ForEach(export.CumulativeExportKindSelector(), records.AddRecord)) + require.NoError(t, controllertest.ReadAll(puller.Reader(), export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) require.EqualValues(t, map[string]float64{ "counter.sum/A=B/": 10, @@ -100,7 +104,7 @@ func TestPullWithCollect(t *testing.T) { // Cached value! require.NoError(t, puller.Collect(ctx)) records = processortest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, puller.ForEach(export.CumulativeExportKindSelector(), records.AddRecord)) + require.NoError(t, controllertest.ReadAll(puller.Reader(), export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) require.EqualValues(t, map[string]float64{ "counter.sum/A=B/": 10, @@ -112,7 +116,7 @@ func TestPullWithCollect(t *testing.T) { // Re-computed value! require.NoError(t, puller.Collect(ctx)) records = processortest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, puller.ForEach(export.CumulativeExportKindSelector(), records.AddRecord)) + require.NoError(t, controllertest.ReadAll(puller.Reader(), export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) require.EqualValues(t, map[string]float64{ "counter.sum/A=B/": 20, diff --git a/sdk/metric/controller/basic/push_test.go b/sdk/metric/controller/basic/push_test.go index 56a623954df..e9e818de536 100644 --- a/sdk/metric/controller/basic/push_test.go +++ b/sdk/metric/controller/basic/push_test.go @@ -72,19 +72,17 @@ func newExporter() *processortest.Exporter { ) } -func newCheckpointer() export.Checkpointer { - return processortest.Checkpointer( - processortest.NewProcessor( - processortest.AggregatorSelector(), - attribute.DefaultEncoder(), - ), +func newCheckpointerFactory() export.CheckpointerFactory { + return processortest.NewCheckpointerFactory( + processortest.AggregatorSelector(), + attribute.DefaultEncoder(), ) } func TestPushDoubleStop(t *testing.T) { ctx := context.Background() exporter := newExporter() - checkpointer := newCheckpointer() + checkpointer := newCheckpointerFactory() p := controller.New(checkpointer, controller.WithExporter(exporter)) require.NoError(t, p.Start(ctx)) require.NoError(t, p.Stop(ctx)) @@ -94,7 +92,7 @@ func TestPushDoubleStop(t *testing.T) { func TestPushDoubleStart(t *testing.T) { ctx := context.Background() exporter := newExporter() - checkpointer := newCheckpointer() + checkpointer := newCheckpointerFactory() p := controller.New(checkpointer, controller.WithExporter(exporter)) require.NoError(t, p.Start(ctx)) err := p.Start(ctx) @@ -105,7 +103,7 @@ func TestPushDoubleStart(t *testing.T) { func TestPushTicker(t *testing.T) { exporter := newExporter() - checkpointer := newCheckpointer() + checkpointer := newCheckpointerFactory() p := controller.New( checkpointer, controller.WithExporter(exporter), @@ -185,7 +183,7 @@ func TestPushExportError(t *testing.T) { // This test validates the error handling // behavior of the basic Processor is honored // by the push processor. - checkpointer := processor.New(processortest.AggregatorSelector(), exporter) + checkpointer := processor.NewFactory(processortest.AggregatorSelector(), exporter) p := controller.New( checkpointer, controller.WithExporter(exporter), diff --git a/sdk/metric/controller/controllertest/test.go b/sdk/metric/controller/controllertest/test.go index d298776d734..f4293cb7f64 100644 --- a/sdk/metric/controller/controllertest/test.go +++ b/sdk/metric/controller/controllertest/test.go @@ -19,6 +19,8 @@ import ( "github.com/benbjohnson/clock" + export "go.opentelemetry.io/otel/sdk/export/metric" + "go.opentelemetry.io/otel/sdk/instrumentation" controllerTime "go.opentelemetry.io/otel/sdk/metric/controller/time" ) @@ -56,3 +58,18 @@ func (t MockTicker) Stop() { func (t MockTicker) C() <-chan time.Time { return t.ticker.C } + +// ReadAll is a helper for tests that want a flat iterator over all +// metrics instead of a two-level iterator (instrumentation library, +// metric). +func ReadAll( + reader export.InstrumentationLibraryMetricReader, + kind export.ExportKindSelector, + apply func(instrumentation.Library, export.Record) error, +) error { + return reader.ForEach(func(library instrumentation.Library, ckpt export.MetricReader) error { + return ckpt.ForEach(kind, func(record export.Record) error { + return apply(library, record) + }) + }) +} diff --git a/sdk/metric/correct_test.go b/sdk/metric/correct_test.go index 98f2512c785..c20297c2b9d 100644 --- a/sdk/metric/correct_test.go +++ b/sdk/metric/correct_test.go @@ -86,7 +86,7 @@ func newSDK(t *testing.T) (metric.Meter, *metricsdk.Accumulator, *testSelector, accum := metricsdk.NewAccumulator( processor, ) - meter := metric.WrapMeterImpl(accum, "test") + meter := metric.WrapMeterImpl(accum) return meter, accum, testSelector, processor } diff --git a/sdk/metric/histogram_stress_test.go b/sdk/metric/histogram_stress_test.go index 4c94378cb5b..2715e587c29 100644 --- a/sdk/metric/histogram_stress_test.go +++ b/sdk/metric/histogram_stress_test.go @@ -22,14 +22,14 @@ import ( "github.com/stretchr/testify/require" - "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/metric/metrictest" "go.opentelemetry.io/otel/metric/number" "go.opentelemetry.io/otel/metric/sdkapi" "go.opentelemetry.io/otel/sdk/metric/aggregator/histogram" ) func TestStressInt64Histogram(t *testing.T) { - desc := metric.NewDescriptor("some_metric", sdkapi.ValueRecorderInstrumentKind, number.Int64Kind) + desc := metrictest.NewDescriptor("some_metric", sdkapi.ValueRecorderInstrumentKind, number.Int64Kind) alloc := histogram.New(2, &desc, histogram.WithExplicitBoundaries([]float64{25, 50, 75})) h, ckpt := &alloc[0], &alloc[1] diff --git a/sdk/metric/minmaxsumcount_stress_test.go b/sdk/metric/minmaxsumcount_stress_test.go index 97163d28183..da15b23cb13 100644 --- a/sdk/metric/minmaxsumcount_stress_test.go +++ b/sdk/metric/minmaxsumcount_stress_test.go @@ -20,14 +20,14 @@ import ( "testing" "time" - "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/metric/metrictest" "go.opentelemetry.io/otel/metric/number" "go.opentelemetry.io/otel/metric/sdkapi" "go.opentelemetry.io/otel/sdk/metric/aggregator/minmaxsumcount" ) func TestStressInt64MinMaxSumCount(t *testing.T) { - desc := metric.NewDescriptor("some_metric", sdkapi.ValueRecorderInstrumentKind, number.Int64Kind) + desc := metrictest.NewDescriptor("some_metric", sdkapi.ValueRecorderInstrumentKind, number.Int64Kind) alloc := minmaxsumcount.New(2, &desc) mmsc, ckpt := &alloc[0], &alloc[1] diff --git a/sdk/metric/processor/basic/basic.go b/sdk/metric/processor/basic/basic.go index 8fb24d09195..5de936ad930 100644 --- a/sdk/metric/processor/basic/basic.go +++ b/sdk/metric/processor/basic/basic.go @@ -113,7 +113,7 @@ type ( var _ export.Processor = &Processor{} var _ export.Checkpointer = &Processor{} -var _ export.CheckpointSet = &state{} +var _ export.MetricReader = &state{} // ErrInconsistentState is returned when the sequence of collection's starts and finishes are incorrectly balanced. var ErrInconsistentState = fmt.Errorf("inconsistent processor state") @@ -127,20 +127,43 @@ var ErrInvalidExportKind = fmt.Errorf("invalid export kind") // data, so that this Processor can prepare to compute Delta or // Cumulative Aggregations as needed. func New(aselector export.AggregatorSelector, eselector export.ExportKindSelector, opts ...Option) *Processor { + return NewFactory(aselector, eselector, opts...).NewCheckpointer().(*Processor) +} + +type factory struct { + aselector export.AggregatorSelector + eselector export.ExportKindSelector + config config +} + +func NewFactory(aselector export.AggregatorSelector, eselector export.ExportKindSelector, opts ...Option) export.CheckpointerFactory { + var config config + for _, opt := range opts { + opt.applyProcessor(&config) + } + return factory{ + aselector: aselector, + eselector: eselector, + config: config, + } +} + +var _ export.CheckpointerFactory = factory{} + +func (f factory) NewCheckpointer() export.Checkpointer { now := time.Now() p := &Processor{ - AggregatorSelector: aselector, - ExportKindSelector: eselector, + AggregatorSelector: f.aselector, + ExportKindSelector: f.eselector, state: state{ values: map[stateKey]*stateValue{}, processStart: now, intervalStart: now, + config: f.config, }, } - for _, opt := range opts { - opt.applyProcessor(&p.config) - } return p + } // Process implements export.Processor. @@ -241,11 +264,11 @@ func (b *Processor) Process(accum export.Accumulation) error { return value.current.Merge(agg, desc) } -// CheckpointSet returns the associated CheckpointSet. Use the -// CheckpointSet Locker interface to synchronize access to this -// object. The CheckpointSet.ForEach() method cannot be called +// MetricReader returns the associated MetricReader. Use the +// MetricReader Locker interface to synchronize access to this +// object. The MetricReader.ForEach() method cannot be called // concurrently with Process(). -func (b *Processor) CheckpointSet() export.CheckpointSet { +func (b *Processor) MetricReader() export.MetricReader { return &b.state } @@ -260,7 +283,7 @@ func (b *Processor) StartCollection() { // FinishCollection signals to the Processor that a complete // collection has finished and that ForEach will be called to access -// the CheckpointSet. +// the MetricReader. func (b *Processor) FinishCollection() error { b.intervalEnd = time.Now() if b.startedCollection != b.finishedCollection+1 { @@ -314,7 +337,7 @@ func (b *Processor) FinishCollection() error { return nil } -// ForEach iterates through the CheckpointSet, passing an +// ForEach iterates through the MetricReader, passing an // export.Record with the appropriate Cumulative or Delta aggregation // to an exporter. func (b *state) ForEach(exporter export.ExportKindSelector, f func(export.Record) error) error { diff --git a/sdk/metric/processor/basic/basic_test.go b/sdk/metric/processor/basic/basic_test.go index 0bb3a6386c8..86d434ddc6e 100644 --- a/sdk/metric/processor/basic/basic_test.go +++ b/sdk/metric/processor/basic/basic_test.go @@ -26,10 +26,12 @@ import ( "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/metric/metrictest" "go.opentelemetry.io/otel/metric/number" "go.opentelemetry.io/otel/metric/sdkapi" export "go.opentelemetry.io/otel/sdk/export/metric" "go.opentelemetry.io/otel/sdk/export/metric/aggregation" + "go.opentelemetry.io/otel/sdk/instrumentation" sdk "go.opentelemetry.io/otel/sdk/metric" "go.opentelemetry.io/otel/sdk/metric/aggregator/aggregatortest" "go.opentelemetry.io/otel/sdk/metric/processor/basic" @@ -136,8 +138,8 @@ func testProcessor( instSuffix := fmt.Sprint(".", strings.ToLower(akind.String())) - desc1 := metric.NewDescriptor(fmt.Sprint("inst1", instSuffix), mkind, nkind) - desc2 := metric.NewDescriptor(fmt.Sprint("inst2", instSuffix), mkind, nkind) + desc1 := metrictest.NewDescriptor(fmt.Sprint("inst1", instSuffix), mkind, nkind) + desc2 := metrictest.NewDescriptor(fmt.Sprint("inst2", instSuffix), mkind, nkind) for nc := 0; nc < nCheckpoint; nc++ { @@ -174,7 +176,7 @@ func testProcessor( continue } - checkpointSet := processor.CheckpointSet() + metricReader := processor.MetricReader() for _, repetitionAfterEmptyInterval := range []bool{false, true} { if repetitionAfterEmptyInterval { @@ -188,7 +190,7 @@ func testProcessor( // Test the final checkpoint state. records1 := processorTest.NewOutput(attribute.DefaultEncoder()) - err = checkpointSet.ForEach(export.ConstantExportKindSelector(ekind), records1.AddRecord) + err = metricReader.ForEach(export.ConstantExportKindSelector(ekind), records1.AddRecord) // Test for an allowed error: if err != nil && err != aggregation.ErrNoSubtraction { @@ -267,7 +269,7 @@ func (bogusExporter) ExportKindFor(*metric.Descriptor, aggregation.Kind) export. return 1000000 } -func (bogusExporter) Export(context.Context, export.CheckpointSet) error { +func (bogusExporter) Export(context.Context, export.MetricReader) error { panic("Not called") } @@ -300,7 +302,7 @@ func TestBasicInconsistent(t *testing.T) { // Test no start b = basic.New(processorTest.AggregatorSelector(), export.StatelessExportKindSelector()) - desc := metric.NewDescriptor("inst", sdkapi.CounterInstrumentKind, number.Int64Kind) + desc := metrictest.NewDescriptor("inst", sdkapi.CounterInstrumentKind, number.Int64Kind) accum := export.NewAccumulation(&desc, attribute.EmptySet(), aggregatortest.NoopAggregator{}) require.Equal(t, basic.ErrInconsistentState, b.Process(accum)) @@ -325,7 +327,7 @@ func TestBasicTimestamps(t *testing.T) { time.Sleep(time.Nanosecond) afterNew := time.Now() - desc := metric.NewDescriptor("inst", sdkapi.CounterInstrumentKind, number.Int64Kind) + desc := metrictest.NewDescriptor("inst", sdkapi.CounterInstrumentKind, number.Int64Kind) accum := export.NewAccumulation(&desc, attribute.EmptySet(), aggregatortest.NoopAggregator{}) b.StartCollection() @@ -370,11 +372,11 @@ func TestBasicTimestamps(t *testing.T) { func TestStatefulNoMemoryCumulative(t *testing.T) { ekindSel := export.CumulativeExportKindSelector() - desc := metric.NewDescriptor("inst.sum", sdkapi.CounterInstrumentKind, number.Int64Kind) + desc := metrictest.NewDescriptor("inst.sum", sdkapi.CounterInstrumentKind, number.Int64Kind) selector := processorTest.AggregatorSelector() processor := basic.New(selector, ekindSel, basic.WithMemory(false)) - checkpointSet := processor.CheckpointSet() + metricReader := processor.MetricReader() for i := 1; i < 3; i++ { // Empty interval @@ -383,7 +385,7 @@ func TestStatefulNoMemoryCumulative(t *testing.T) { // Verify zero elements records := processorTest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, checkpointSet.ForEach(ekindSel, records.AddRecord)) + require.NoError(t, metricReader.ForEach(ekindSel, records.AddRecord)) require.EqualValues(t, map[string]float64{}, records.Map()) // Add 10 @@ -393,7 +395,7 @@ func TestStatefulNoMemoryCumulative(t *testing.T) { // Verify one element records = processorTest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, checkpointSet.ForEach(ekindSel, records.AddRecord)) + require.NoError(t, metricReader.ForEach(ekindSel, records.AddRecord)) require.EqualValues(t, map[string]float64{ "inst.sum/A=B/": float64(i * 10), }, records.Map()) @@ -403,11 +405,11 @@ func TestStatefulNoMemoryCumulative(t *testing.T) { func TestStatefulNoMemoryDelta(t *testing.T) { ekindSel := export.DeltaExportKindSelector() - desc := metric.NewDescriptor("inst.sum", sdkapi.SumObserverInstrumentKind, number.Int64Kind) + desc := metrictest.NewDescriptor("inst.sum", sdkapi.SumObserverInstrumentKind, number.Int64Kind) selector := processorTest.AggregatorSelector() processor := basic.New(selector, ekindSel, basic.WithMemory(false)) - checkpointSet := processor.CheckpointSet() + metricReader := processor.MetricReader() for i := 1; i < 3; i++ { // Empty interval @@ -416,7 +418,7 @@ func TestStatefulNoMemoryDelta(t *testing.T) { // Verify zero elements records := processorTest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, checkpointSet.ForEach(ekindSel, records.AddRecord)) + require.NoError(t, metricReader.ForEach(ekindSel, records.AddRecord)) require.EqualValues(t, map[string]float64{}, records.Map()) // Add 10 @@ -426,7 +428,7 @@ func TestStatefulNoMemoryDelta(t *testing.T) { // Verify one element records = processorTest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, checkpointSet.ForEach(ekindSel, records.AddRecord)) + require.NoError(t, metricReader.ForEach(ekindSel, records.AddRecord)) require.EqualValues(t, map[string]float64{ "inst.sum/A=B/": 10, }, records.Map()) @@ -439,11 +441,11 @@ func TestMultiObserverSum(t *testing.T) { export.DeltaExportKindSelector(), } { - desc := metric.NewDescriptor("observe.sum", sdkapi.SumObserverInstrumentKind, number.Int64Kind) + desc := metrictest.NewDescriptor("observe.sum", sdkapi.SumObserverInstrumentKind, number.Int64Kind) selector := processorTest.AggregatorSelector() processor := basic.New(selector, ekindSel, basic.WithMemory(false)) - checkpointSet := processor.CheckpointSet() + metricReader := processor.MetricReader() for i := 1; i < 3; i++ { // Add i*10*3 times @@ -461,7 +463,7 @@ func TestMultiObserverSum(t *testing.T) { // Verify one element records := processorTest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, checkpointSet.ForEach(ekindSel, records.AddRecord)) + require.NoError(t, metricReader.ForEach(ekindSel, records.AddRecord)) require.EqualValues(t, map[string]float64{ "observe.sum/A=B/": float64(3 * 10 * multiplier), }, records.Map()) @@ -477,7 +479,7 @@ func TestSumObserverEndToEnd(t *testing.T) { eselector, ) accum := sdk.NewAccumulator(proc) - meter := metric.WrapMeterImpl(accum, "testing") + meter := metric.WrapMeterImpl(accum) var calls int64 metric.Must(meter).NewInt64SumObserver("observer.sum", @@ -486,7 +488,7 @@ func TestSumObserverEndToEnd(t *testing.T) { result.Observe(calls) }, ) - data := proc.CheckpointSet() + data := proc.MetricReader() var startTime [3]time.Time var endTime [3]time.Time @@ -498,7 +500,10 @@ func TestSumObserverEndToEnd(t *testing.T) { require.NoError(t, proc.FinishCollection()) exporter := processortest.New(eselector, attribute.DefaultEncoder()) - require.NoError(t, exporter.Export(ctx, resource.Empty(), data)) + require.NoError(t, exporter.Export(ctx, resource.Empty(), processortest.TestLibraryReader( + instrumentation.Library{ + Name: "test", + }, data))) require.EqualValues(t, map[string]float64{ "observer.sum//": float64(i + 1), diff --git a/sdk/metric/processor/processortest/test.go b/sdk/metric/processor/processortest/test.go index a58eeef47de..76469021c5b 100644 --- a/sdk/metric/processor/processortest/test.go +++ b/sdk/metric/processor/processortest/test.go @@ -26,6 +26,7 @@ import ( "go.opentelemetry.io/otel/metric/number" export "go.opentelemetry.io/otel/sdk/export/metric" "go.opentelemetry.io/otel/sdk/export/metric/aggregation" + "go.opentelemetry.io/otel/sdk/instrumentation" "go.opentelemetry.io/otel/sdk/metric/aggregator/exact" "go.opentelemetry.io/otel/sdk/metric/aggregator/histogram" "go.opentelemetry.io/otel/sdk/metric/aggregator/lastvalue" @@ -45,14 +46,14 @@ type ( } // mapValue is value stored in a processor used to produce a - // CheckpointSet. + // MetricReader. mapValue struct { labels *attribute.Set resource *resource.Resource aggregator export.Aggregator } - // Output implements export.CheckpointSet. + // Output implements export.MetricReader. Output struct { m map[mapKey]mapValue labelEncoder attribute.Encoder @@ -92,6 +93,28 @@ type ( } ) +type testFactory struct { + selector export.AggregatorSelector + encoder attribute.Encoder +} + +func NewCheckpointerFactory(selector export.AggregatorSelector, encoder attribute.Encoder) export.CheckpointerFactory { + return testFactory{ + selector: selector, + encoder: encoder, + } +} + +func NewCheckpointer(p *Processor) export.Checkpointer { + return &testCheckpointer{ + Processor: p, + } +} + +func (f testFactory) NewCheckpointer() export.Checkpointer { + return NewCheckpointer(NewProcessor(f.selector, f.encoder)) +} + // NewProcessor returns a new testing Processor implementation. // Verify expected outputs using Values(), e.g.: // @@ -126,14 +149,6 @@ func (p *Processor) Reset() { p.output.Reset() } -// Checkpointer returns a checkpointer that computes a single -// interval. -func Checkpointer(p *Processor) export.Checkpointer { - return &testCheckpointer{ - Processor: p, - } -} - // StartCollection implements export.Checkpointer. func (c *testCheckpointer) StartCollection() { if c.started != c.finished { @@ -153,8 +168,8 @@ func (c *testCheckpointer) FinishCollection() error { return nil } -// CheckpointSet implements export.Checkpointer. -func (c *testCheckpointer) CheckpointSet() export.CheckpointSet { +// MetricReader implements export.Checkpointer. +func (c *testCheckpointer) MetricReader() export.MetricReader { return c.Processor.output } @@ -214,7 +229,7 @@ func NewOutput(labelEncoder attribute.Encoder) *Output { } } -// ForEach implements export.CheckpointSet. +// ForEach implements export.MetricReader. func (o *Output) ForEach(_ export.ExportKindSelector, ff func(export.Record) error) error { for key, value := range o.m { if err := ff(export.NewRecord( @@ -238,6 +253,10 @@ func (o *Output) AddRecord(rec export.Record) error { return o.AddRecordWithResource(rec, resource.Empty()) } +func (o *Output) AddInstrumentationLibraryRecord(_ instrumentation.Library, rec export.Record) error { + return o.AddRecordWithResource(rec, resource.Empty()) +} + func (o *Output) AddRecordWithResource(rec export.Record, res *resource.Resource) error { key := mapKey{ desc: rec.Descriptor(), @@ -332,17 +351,19 @@ func New(selector export.ExportKindSelector, encoder attribute.Encoder) *Exporte } } -func (e *Exporter) Export(_ context.Context, res *resource.Resource, ckpt export.CheckpointSet) error { +func (e *Exporter) Export(_ context.Context, res *resource.Resource, ckpt export.InstrumentationLibraryMetricReader) error { e.output.Lock() defer e.output.Unlock() e.exportCount++ - return ckpt.ForEach(e.ExportKindSelector, func(r export.Record) error { - if e.InjectErr != nil { - if err := e.InjectErr(r); err != nil { - return err + return ckpt.ForEach(func(library instrumentation.Library, mr export.MetricReader) error { + return mr.ForEach(e.ExportKindSelector, func(r export.Record) error { + if e.InjectErr != nil { + if err := e.InjectErr(r); err != nil { + return err + } } - } - return e.output.AddRecordWithResource(r, res) + return e.output.AddRecordWithResource(r, res) + }) }) } @@ -372,3 +393,16 @@ func (e *Exporter) Reset() { e.output.Reset() e.exportCount = 0 } + +func TestLibraryReader(l instrumentation.Library, r export.MetricReader) export.InstrumentationLibraryMetricReader { + return ilReader{l, r} +} + +type ilReader struct { + library instrumentation.Library + reader export.MetricReader +} + +func (ilr ilReader) ForEach(readerFunc func(instrumentation.Library, export.MetricReader) error) error { + return readerFunc(ilr.library, ilr.reader) +} diff --git a/sdk/metric/processor/processortest/test_test.go b/sdk/metric/processor/processortest/test_test.go index 0c0b244f37f..bd42f6772f2 100644 --- a/sdk/metric/processor/processortest/test_test.go +++ b/sdk/metric/processor/processortest/test_test.go @@ -23,7 +23,9 @@ import ( "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" export "go.opentelemetry.io/otel/sdk/export/metric" + "go.opentelemetry.io/otel/sdk/instrumentation" metricsdk "go.opentelemetry.io/otel/sdk/metric" + "go.opentelemetry.io/otel/sdk/metric/processor/processortest" processorTest "go.opentelemetry.io/otel/sdk/metric/processor/processortest" "go.opentelemetry.io/otel/sdk/resource" ) @@ -31,7 +33,7 @@ import ( func generateTestData(proc export.Processor) { ctx := context.Background() accum := metricsdk.NewAccumulator(proc) - meter := metric.WrapMeterImpl(accum, "testing") + meter := metric.WrapMeterImpl(accum) counter := metric.Must(meter).NewFloat64Counter("counter.sum") @@ -51,12 +53,12 @@ func generateTestData(proc export.Processor) { func TestProcessorTesting(t *testing.T) { // Test the Processor test helper using a real Accumulator to // generate Accumulations. - testProc := processorTest.NewProcessor( - processorTest.AggregatorSelector(), - attribute.DefaultEncoder(), + checkpointer := processorTest.NewCheckpointer( + processorTest.NewProcessor( + processorTest.AggregatorSelector(), + attribute.DefaultEncoder(), + ), ) - checkpointer := processorTest.Checkpointer(testProc) - generateTestData(checkpointer) res := resource.NewSchemaless(attribute.String("R", "V")) @@ -73,7 +75,12 @@ func TestProcessorTesting(t *testing.T) { attribute.DefaultEncoder(), ) - err := exporter.Export(context.Background(), res, checkpointer.CheckpointSet()) + err := exporter.Export(context.Background(), res, processortest.TestLibraryReader( + instrumentation.Library{ + Name: "test", + }, + checkpointer.MetricReader(), + )) require.NoError(t, err) require.EqualValues(t, expect, exporter.Values()) } diff --git a/sdk/metric/processor/reducer/reducer_test.go b/sdk/metric/processor/reducer/reducer_test.go index cd429c68c6c..b05bbf5ecd9 100644 --- a/sdk/metric/processor/reducer/reducer_test.go +++ b/sdk/metric/processor/reducer/reducer_test.go @@ -23,8 +23,10 @@ import ( "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" export "go.opentelemetry.io/otel/sdk/export/metric" + "go.opentelemetry.io/otel/sdk/instrumentation" metricsdk "go.opentelemetry.io/otel/sdk/metric" "go.opentelemetry.io/otel/sdk/metric/processor/basic" + "go.opentelemetry.io/otel/sdk/metric/processor/processortest" processorTest "go.opentelemetry.io/otel/sdk/metric/processor/processortest" "go.opentelemetry.io/otel/sdk/metric/processor/reducer" "go.opentelemetry.io/otel/sdk/resource" @@ -53,7 +55,7 @@ func (testFilter) LabelFilterFor(_ *metric.Descriptor) attribute.Filter { func generateData(impl metric.MeterImpl) { ctx := context.Background() - meter := metric.WrapMeterImpl(impl, "testing") + meter := metric.WrapMeterImpl(impl) counter := metric.Must(meter).NewFloat64Counter("counter.sum") @@ -74,7 +76,7 @@ func TestFilterProcessor(t *testing.T) { attribute.DefaultEncoder(), ) accum := metricsdk.NewAccumulator( - reducer.New(testFilter{}, processorTest.Checkpointer(testProc)), + reducer.New(testFilter{}, processorTest.NewCheckpointer(testProc)), ) generateData(accum) @@ -103,7 +105,9 @@ func TestFilterBasicProcessor(t *testing.T) { } res := resource.NewSchemaless(attribute.String("R", "V")) - require.NoError(t, exporter.Export(context.Background(), res, basicProc.CheckpointSet())) + require.NoError(t, exporter.Export(context.Background(), res, processortest.TestLibraryReader(instrumentation.Library{ + Name: "test", + }, basicProc.MetricReader()))) require.EqualValues(t, map[string]float64{ "counter.sum/A=1,C=3/R=V": 200, diff --git a/sdk/metric/selector/simple/simple_test.go b/sdk/metric/selector/simple/simple_test.go index bccf5d5ef4d..fd08008faa0 100644 --- a/sdk/metric/selector/simple/simple_test.go +++ b/sdk/metric/selector/simple/simple_test.go @@ -20,6 +20,7 @@ import ( "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/metric/metrictest" "go.opentelemetry.io/otel/metric/number" "go.opentelemetry.io/otel/metric/sdkapi" export "go.opentelemetry.io/otel/sdk/export/metric" @@ -32,12 +33,12 @@ import ( ) var ( - testCounterDesc = metric.NewDescriptor("counter", sdkapi.CounterInstrumentKind, number.Int64Kind) - testUpDownCounterDesc = metric.NewDescriptor("updowncounter", sdkapi.UpDownCounterInstrumentKind, number.Int64Kind) - testSumObserverDesc = metric.NewDescriptor("sumobserver", sdkapi.SumObserverInstrumentKind, number.Int64Kind) - testUpDownSumObserverDesc = metric.NewDescriptor("updownsumobserver", sdkapi.UpDownSumObserverInstrumentKind, number.Int64Kind) - testValueRecorderDesc = metric.NewDescriptor("valuerecorder", sdkapi.ValueRecorderInstrumentKind, number.Int64Kind) - testValueObserverDesc = metric.NewDescriptor("valueobserver", sdkapi.ValueObserverInstrumentKind, number.Int64Kind) + testCounterDesc = metrictest.NewDescriptor("counter", sdkapi.CounterInstrumentKind, number.Int64Kind) + testUpDownCounterDesc = metrictest.NewDescriptor("updowncounter", sdkapi.UpDownCounterInstrumentKind, number.Int64Kind) + testSumObserverDesc = metrictest.NewDescriptor("sumobserver", sdkapi.SumObserverInstrumentKind, number.Int64Kind) + testUpDownSumObserverDesc = metrictest.NewDescriptor("updownsumobserver", sdkapi.UpDownSumObserverInstrumentKind, number.Int64Kind) + testValueRecorderDesc = metrictest.NewDescriptor("valuerecorder", sdkapi.ValueRecorderInstrumentKind, number.Int64Kind) + testValueObserverDesc = metrictest.NewDescriptor("valueobserver", sdkapi.ValueObserverInstrumentKind, number.Int64Kind) ) func oneAgg(sel export.AggregatorSelector, desc *metric.Descriptor) export.Aggregator { diff --git a/sdk/metric/stress_test.go b/sdk/metric/stress_test.go index 9783ea0fcfa..b2d078363ce 100644 --- a/sdk/metric/stress_test.go +++ b/sdk/metric/stress_test.go @@ -246,7 +246,7 @@ func (f *testFixture) preCollect() { f.dupCheck = map[testKey]int{} } -func (*testFixture) CheckpointSet() export.CheckpointSet { +func (*testFixture) MetricReader() export.MetricReader { return nil } @@ -296,7 +296,7 @@ func stressTest(t *testing.T, impl testImpl) { cc := concurrency() sdk := NewAccumulator(fixture) - meter := metric.WrapMeterImpl(sdk, "stress_test") + meter := metric.WrapMeterImpl(sdk) fixture.wg.Add(cc + 1) for i := 0; i < cc; i++ { From 5ab49d19d834135dda514ab2ca12d5d2d4e1b148 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Thu, 19 Aug 2021 13:29:12 -0700 Subject: [PATCH 03/33] checkpoint work --- exporters/otlp/otlpmetric/exporter.go | 13 +- exporters/otlp/otlpmetric/exporter_test.go | 197 ++++++++++-------- .../internal/metrictransform/metric.go | 140 +++++++------ .../internal/metrictransform/metric_test.go | 19 +- .../internal/otlpmetrictest/data.go | 46 ++-- sdk/metric/processor/processortest/test.go | 45 +++- sdk/resource/resource.go | 3 + 7 files changed, 271 insertions(+), 192 deletions(-) diff --git a/exporters/otlp/otlpmetric/exporter.go b/exporters/otlp/otlpmetric/exporter.go index 548a4afa381..3a9f09bc2cf 100644 --- a/exporters/otlp/otlpmetric/exporter.go +++ b/exporters/otlp/otlpmetric/exporter.go @@ -24,6 +24,7 @@ import ( metricsdk "go.opentelemetry.io/otel/sdk/export/metric" "go.opentelemetry.io/otel/sdk/export/metric/aggregation" "go.opentelemetry.io/otel/sdk/resource" + metricpb "go.opentelemetry.io/proto/otlp/metrics/v1" ) var ( @@ -43,16 +44,20 @@ type Exporter struct { } // Export exports a batch of metrics. -func (e *Exporter) Export(ctx context.Context, res *resource.Resource, checkpointSet metricsdk.CheckpointSet) error { - rms, err := metrictransform.CheckpointSet(ctx, e, res, checkpointSet, 1) +func (e *Exporter) Export(ctx context.Context, res *resource.Resource, ilmr metricsdk.InstrumentationLibraryMetricReader) error { + rm, err := metrictransform.InstrumentationLibraryMetricReader(ctx, e, res, ilmr, 1) if err != nil { return err } - if len(rms) == 0 { + if rm == nil { return nil } - return e.client.UploadMetrics(ctx, rms) + // TODO: There is never more than one resource emitted by this + // call, as per the specification. We can change the + // signature of UploadMetrics correspondingly. Here create a + // singleton list to reduce the size of the current PR: + return e.client.UploadMetrics(ctx, []*metricpb.ResourceMetrics{rm}) } // Start establishes a connection to the receiving endpoint. diff --git a/exporters/otlp/otlpmetric/exporter_test.go b/exporters/otlp/otlpmetric/exporter_test.go index c98cff6ec83..9091399cbfd 100644 --- a/exporters/otlp/otlpmetric/exporter_test.go +++ b/exporters/otlp/otlpmetric/exporter_test.go @@ -17,7 +17,6 @@ package otlpmetric_test import ( "context" "fmt" - "sync" "testing" "time" @@ -30,12 +29,14 @@ import ( "go.opentelemetry.io/otel/exporters/otlp/otlpmetric" "go.opentelemetry.io/otel/exporters/otlp/otlpmetric/internal/metrictransform" "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/metric/metrictest" "go.opentelemetry.io/otel/metric/number" "go.opentelemetry.io/otel/metric/sdkapi" metricsdk "go.opentelemetry.io/otel/sdk/export/metric" - "go.opentelemetry.io/otel/sdk/export/metric/aggregation" + "go.opentelemetry.io/otel/sdk/instrumentation" "go.opentelemetry.io/otel/sdk/metric/aggregator/histogram" "go.opentelemetry.io/otel/sdk/metric/aggregator/sum" + "go.opentelemetry.io/otel/sdk/metric/processor/processortest" "go.opentelemetry.io/otel/sdk/resource" commonpb "go.opentelemetry.io/proto/otlp/common/v1" metricpb "go.opentelemetry.io/proto/otlp/metrics/v1" @@ -85,26 +86,31 @@ func pointTime() uint64 { return uint64(intervalEnd.UnixNano()) } -type checkpointSet struct { - sync.RWMutex - records []metricsdk.Record -} - -func (m *checkpointSet) ForEach(_ metricsdk.ExportKindSelector, fn func(metricsdk.Record) error) error { - for _, r := range m.records { - if err := fn(r); err != nil && err != aggregation.ErrNoData { - return err - } - } - return nil -} - -type record struct { +type testRecord struct { name string iKind sdkapi.InstrumentKind nKind number.Kind - opts []metric.InstrumentOption labels []attribute.KeyValue + + meterName string + meterOpts []metric.MeterOption +} + +func record( + name string, + iKind sdkapi.InstrumentKind, + nKind number.Kind, + labels []attribute.KeyValue, + meterName string, + meterOpts ...metric.MeterOption) testRecord { + return testRecord{ + name: name, + iKind: iKind, + nKind: nKind, + labels: labels, + meterName: meterName, + meterOpts: meterOpts, + } } var ( @@ -154,26 +160,31 @@ var ( testerAResourcePb = metrictransform.Resource(testerAResource) ) +const ( + // Most of this test uses an empty instrumentation library name. + testLibName = "" +) + func TestNoGroupingExport(t *testing.T) { runMetricExportTests( t, nil, nil, - []record{ - { + []testRecord{ + record( "int64-count", sdkapi.CounterInstrumentKind, number.Int64Kind, - nil, append(baseKeyValues, cpuKey.Int(1)), - }, - { + testLibName, + ), + record( "int64-count", sdkapi.CounterInstrumentKind, number.Int64Kind, - nil, append(baseKeyValues, cpuKey.Int(2)), - }, + testLibName, + ), }, []*metricpb.ResourceMetrics{ { @@ -213,13 +224,13 @@ func TestNoGroupingExport(t *testing.T) { } func TestValuerecorderMetricGroupingExport(t *testing.T) { - r := record{ + r := record( "valuerecorder", sdkapi.ValueRecorderInstrumentKind, number.Int64Kind, - nil, append(baseKeyValues, cpuKey.Int(1)), - } + testLibName, + ) expected := []*metricpb.ResourceMetrics{ { Resource: nil, @@ -259,22 +270,22 @@ func TestValuerecorderMetricGroupingExport(t *testing.T) { }, }, } - runMetricExportTests(t, nil, nil, []record{r, r}, expected) + runMetricExportTests(t, nil, nil, []testRecord{r, r}, expected) } func TestCountInt64MetricGroupingExport(t *testing.T) { - r := record{ + r := record( "int64-count", sdkapi.CounterInstrumentKind, number.Int64Kind, - nil, append(baseKeyValues, cpuKey.Int(1)), - } + testLibName, + ) runMetricExportTests( t, nil, nil, - []record{r, r}, + []testRecord{r, r}, []*metricpb.ResourceMetrics{ { Resource: nil, @@ -313,18 +324,18 @@ func TestCountInt64MetricGroupingExport(t *testing.T) { } func TestCountFloat64MetricGroupingExport(t *testing.T) { - r := record{ + r := record( "float64-count", sdkapi.CounterInstrumentKind, number.Float64Kind, - nil, append(baseKeyValues, cpuKey.Int(1)), - } + testLibName, + ) runMetricExportTests( t, nil, nil, - []record{r, r}, + []testRecord{r, r}, []*metricpb.ResourceMetrics{ { Resource: nil, @@ -367,35 +378,35 @@ func TestResourceMetricGroupingExport(t *testing.T) { t, nil, testerAResource, - []record{ - { + []testRecord{ + record( "int64-count", sdkapi.CounterInstrumentKind, number.Int64Kind, - nil, append(baseKeyValues, cpuKey.Int(1)), - }, - { + testLibName, + ), + record( "int64-count", sdkapi.CounterInstrumentKind, number.Int64Kind, - nil, append(baseKeyValues, cpuKey.Int(1)), - }, - { + testLibName, + ), + record( "int64-count", sdkapi.CounterInstrumentKind, number.Int64Kind, - nil, append(baseKeyValues, cpuKey.Int(2)), - }, - { + testLibName, + ), + record( "int64-count", sdkapi.CounterInstrumentKind, number.Int64Kind, - nil, append(baseKeyValues, cpuKey.Int(1)), - }, + testLibName, + ), }, []*metricpb.ResourceMetrics{ { @@ -447,57 +458,56 @@ func TestResourceMetricGroupingExport(t *testing.T) { } func TestResourceInstLibMetricGroupingExport(t *testing.T) { - countingLib1 := []metric.InstrumentOption{ - metric.WithInstrumentationName("counting-lib"), - metric.WithInstrumentationVersion("v1"), - } - countingLib2 := []metric.InstrumentOption{ - metric.WithInstrumentationName("counting-lib"), - metric.WithInstrumentationVersion("v2"), - } - summingLib := []metric.InstrumentOption{ - metric.WithInstrumentationName("summing-lib"), - } + version1 := metric.WithInstrumentationVersion("v1") + version2 := metric.WithInstrumentationVersion("v2") + specialSchema := metric.WithSchemaURL("schurl") + summingLib := "summing-lib" + countingLib := "counting-lib" runMetricExportTests( t, nil, testerAResource, - []record{ - { + []testRecord{ + record( "int64-count", sdkapi.CounterInstrumentKind, number.Int64Kind, - countingLib1, append(baseKeyValues, cpuKey.Int(1)), - }, - { + countingLib, + version1, + ), + record( "int64-count", sdkapi.CounterInstrumentKind, number.Int64Kind, - countingLib2, append(baseKeyValues, cpuKey.Int(1)), - }, - { + countingLib, + version2, + ), + record( "int64-count", sdkapi.CounterInstrumentKind, number.Int64Kind, - countingLib1, append(baseKeyValues, cpuKey.Int(1)), - }, - { + countingLib, + version1, + ), + record( "int64-count", sdkapi.CounterInstrumentKind, number.Int64Kind, - countingLib1, append(baseKeyValues, cpuKey.Int(2)), - }, - { + countingLib, + version1, + ), + record( "int64-count", sdkapi.CounterInstrumentKind, number.Int64Kind, - summingLib, append(baseKeyValues, cpuKey.Int(1)), - }, + summingLib, + specialSchema, + ), }, []*metricpb.ResourceMetrics{ { @@ -569,6 +579,7 @@ func TestResourceInstLibMetricGroupingExport(t *testing.T) { InstrumentationLibrary: &commonpb.InstrumentationLibrary{ Name: "summing-lib", }, + SchemaUrl: "schurl", Metrics: []*metricpb.Metric{ { Name: "int64-count", @@ -618,14 +629,14 @@ func TestStatelessExportKind(t *testing.T) { ), }, testerAResource, - []record{ - { + []testRecord{ + record( "instrument", k.instrumentKind, number.Int64Kind, - nil, append(baseKeyValues, cpuKey.Int(1)), - }, + testLibName, + ), }, []*metricpb.ResourceMetrics{ { @@ -660,14 +671,14 @@ func TestStatelessExportKind(t *testing.T) { } } -func runMetricExportTests(t *testing.T, opts []otlpmetric.Option, res *resource.Resource, records []record, expected []*metricpb.ResourceMetrics) { +func runMetricExportTests(t *testing.T, opts []otlpmetric.Option, res *resource.Resource, records []testRecord, expected []*metricpb.ResourceMetrics) { exp, driver := newExporter(t, opts...) - recs := []metricsdk.Record{} + libraryRecs := map[instrumentation.Library][]metricsdk.Record{} for _, r := range records { lcopy := make([]attribute.KeyValue, len(r.labels)) copy(lcopy, r.labels) - desc := metric.NewDescriptor(r.name, r.iKind, r.nKind, r.opts...) + desc := metrictest.NewDescriptor(r.name, r.iKind, r.nKind) labs := attribute.NewSet(lcopy...) var agg, ckpt metricsdk.Aggregator @@ -705,9 +716,15 @@ func runMetricExportTests(t *testing.T, opts []otlpmetric.Option, res *resource. } require.NoError(t, agg.SynchronizedMove(ckpt, &desc)) - recs = append(recs, metricsdk.NewRecord(&desc, &labs, ckpt.Aggregation(), intervalStart, intervalEnd)) + meterCfg := metric.NewMeterConfig(r.meterOpts...) + lib := instrumentation.Library{ + Name: r.meterName, + Version: meterCfg.InstrumentationVersion(), + SchemaURL: meterCfg.SchemaURL(), + } + libraryRecs[lib] = append(libraryRecs[lib], metricsdk.NewRecord(&desc, &labs, ckpt.Aggregation(), intervalStart, intervalEnd)) } - assert.NoError(t, exp.Export(context.Background(), res, &checkpointSet{records: recs})) + assert.NoError(t, exp.Export(context.Background(), res, processortest.MultiInstrumentationLibraryMetricReader(libraryRecs))) // assert.ElementsMatch does not equate nested slices of different order, // therefore this requires the top level slice to be broken down. @@ -715,7 +732,7 @@ func runMetricExportTests(t *testing.T, opts []otlpmetric.Option, res *resource. // that validate the metric elements match for all expected pairs. Finally, // make we saw all expected pairs. keyFor := func(ilm *metricpb.InstrumentationLibraryMetrics) string { - return fmt.Sprintf("%s/%s", ilm.GetInstrumentationLibrary().GetName(), ilm.GetInstrumentationLibrary().GetVersion()) + return fmt.Sprintf("%s/%s/%s", ilm.GetInstrumentationLibrary().GetName(), ilm.GetInstrumentationLibrary().GetVersion(), ilm.GetSchemaUrl()) } got := map[string][]*metricpb.Metric{} for _, rm := range driver.rm { @@ -767,7 +784,11 @@ func TestEmptyMetricExport(t *testing.T) { }, } { driver.Reset() - require.NoError(t, exp.Export(context.Background(), resource.Empty(), &checkpointSet{records: test.records})) + require.NoError(t, exp.Export(context.Background(), resource.Empty(), processortest.MultiInstrumentationLibraryMetricReader(map[instrumentation.Library][]metricsdk.Record{ + instrumentation.Library{ + Name: testLibName, + }: test.records, + }))) assert.Equal(t, test.want, driver.rm) } } diff --git a/exporters/otlp/otlpmetric/internal/metrictransform/metric.go b/exporters/otlp/otlpmetric/internal/metrictransform/metric.go index 44d9e47007c..ec8a5339bc6 100644 --- a/exporters/otlp/otlpmetric/internal/metrictransform/metric.go +++ b/exporters/otlp/otlpmetric/internal/metrictransform/metric.go @@ -58,9 +58,8 @@ var ( // result is the product of transforming Records into OTLP Metrics. type result struct { - InstrumentationLibrary instrumentation.Library - Metric *metricpb.Metric - Err error + Metric *metricpb.Metric + Err error } // toNanos returns the number of nanoseconds since the UNIX epoch. @@ -71,50 +70,78 @@ func toNanos(t time.Time) uint64 { return uint64(t.UnixNano()) } -// CheckpointSet transforms all records contained in a checkpoint into +// InstrumentationLibraryMetricReader transforms all records contained in a checkpoint into // batched OTLP ResourceMetrics. -func CheckpointSet(ctx context.Context, exportSelector export.ExportKindSelector, res *resource.Resource, cps export.CheckpointSet, numWorkers uint) ([]*metricpb.ResourceMetrics, error) { - records, errc := source(ctx, exportSelector, cps) - - // Start a fixed number of goroutines to transform records. - transformed := make(chan result) - var wg sync.WaitGroup - wg.Add(int(numWorkers)) - for i := uint(0); i < numWorkers; i++ { +func InstrumentationLibraryMetricReader(ctx context.Context, exportSelector export.ExportKindSelector, res *resource.Resource, ilmr export.InstrumentationLibraryMetricReader, numWorkers uint) (*metricpb.ResourceMetrics, error) { + var ilms []*metricpb.InstrumentationLibraryMetrics + + err := ilmr.ForEach(func(lib instrumentation.Library, mr export.MetricReader) error { + + records, errc := source(ctx, exportSelector, mr) + + // Start a fixed number of goroutines to transform records. + transformed := make(chan result) + var wg sync.WaitGroup + wg.Add(int(numWorkers)) + for i := uint(0); i < numWorkers; i++ { + go func() { + defer wg.Done() + transformer(ctx, exportSelector, records, transformed) + }() + } go func() { - defer wg.Done() - transformer(ctx, exportSelector, records, transformed) + wg.Wait() + close(transformed) }() - } - go func() { - wg.Wait() - close(transformed) - }() - // Synchronously collect the transformed records and transmit. - rms, err := sink(ctx, res, transformed) - if err != nil { + // Synchronously collect the transformed records and transmit. + ms, err := sink(ctx, transformed) + if err != nil { + return nil + } + + // source is complete, check for any errors. + if err := <-errc; err != nil { + return err + } + if len(ms) == 0 { + return nil + } + + ilms = append(ilms, &metricpb.InstrumentationLibraryMetrics{ + Metrics: ms, + SchemaUrl: lib.SchemaURL, + InstrumentationLibrary: &commonpb.InstrumentationLibrary{ + Name: lib.Name, + Version: lib.Version, + }, + }) + return nil + }) + if len(ilms) == 0 { return nil, err } - // source is complete, check for any errors. - if err := <-errc; err != nil { - return nil, err + rms := &metricpb.ResourceMetrics{ + Resource: Resource(res), + SchemaUrl: res.SchemaURL(), + InstrumentationLibraryMetrics: ilms, } - return rms, nil + + return rms, err } // source starts a goroutine that sends each one of the Records yielded by -// the CheckpointSet on the returned chan. Any error encoutered will be sent +// the MetricReader on the returned chan. Any error encoutered will be sent // on the returned error chan after seeding is complete. -func source(ctx context.Context, exportSelector export.ExportKindSelector, cps export.CheckpointSet) (<-chan export.Record, <-chan error) { +func source(ctx context.Context, exportSelector export.ExportKindSelector, mr export.MetricReader) (<-chan export.Record, <-chan error) { errc := make(chan error, 1) out := make(chan export.Record) // Seed records into process. go func() { defer close(out) // No select is needed since errc is buffered. - errc <- cps.ForEach(exportSelector, func(r export.Record) error { + errc <- mr.ForEach(exportSelector, func(r export.Record) error { select { case <-ctx.Done(): return ErrContextCanceled @@ -136,10 +163,6 @@ func transformer(ctx context.Context, exportSelector export.ExportKindSelector, continue } res := result{ - InstrumentationLibrary: instrumentation.Library{ - Name: r.Descriptor().InstrumentationName(), - Version: r.Descriptor().InstrumentationVersion(), - }, Metric: m, Err: err, } @@ -155,30 +178,32 @@ func transformer(ctx context.Context, exportSelector export.ExportKindSelector, // // Any errors encoutered transforming input will be reported with an // ErrTransforming as well as the completed ResourceMetrics. It is up to the -// caller to handle any incorrect data in these ResourceMetrics. -func sink(ctx context.Context, res *resource.Resource, in <-chan result) ([]*metricpb.ResourceMetrics, error) { +// caller to handle any incorrect data in these ResourceMetric. +func sink(ctx context.Context, in <-chan result) ([]*metricpb.Metric, error) { var errStrings []string // Group by instrumentation library name and then the MetricDescriptor. - grouped := map[instrumentation.Library]map[string]*metricpb.Metric{} + grouped := map[string]*metricpb.Metric{} for res := range in { if res.Err != nil { errStrings = append(errStrings, res.Err.Error()) continue } - mb, ok := grouped[res.InstrumentationLibrary] - if !ok { - mb = make(map[string]*metricpb.Metric) - grouped[res.InstrumentationLibrary] = mb - } - mID := res.Metric.GetName() - m, ok := mb[mID] + m, ok := grouped[mID] if !ok { - mb[mID] = res.Metric + grouped[mID] = res.Metric continue + } + // Note: There is extra work happening in this code + // that can be improved when the work described in + // #2119 is completed. The SDK has a guarantee that + // no more than one point per period per label set is + // produced, so this fallthrough should never happen. + // The final step of #2119 is to remove all the + // grouping logic here. switch res.Metric.Data.(type) { case *metricpb.Metric_Gauge: m.GetGauge().DataPoints = append(m.GetGauge().DataPoints, res.Metric.GetGauge().DataPoints...) @@ -198,33 +223,16 @@ func sink(ctx context.Context, res *resource.Resource, in <-chan result) ([]*met return nil, nil } - rm := &metricpb.ResourceMetrics{ - Resource: Resource(res), - } - if res != nil { - rm.SchemaUrl = res.SchemaURL() - } - - rms := []*metricpb.ResourceMetrics{rm} - for il, mb := range grouped { - ilm := &metricpb.InstrumentationLibraryMetrics{ - Metrics: make([]*metricpb.Metric, 0, len(mb)), - InstrumentationLibrary: &commonpb.InstrumentationLibrary{ - Name: il.Name, - Version: il.Version, - }, - } - for _, m := range mb { - ilm.Metrics = append(ilm.Metrics, m) - } - rm.InstrumentationLibraryMetrics = append(rm.InstrumentationLibraryMetrics, ilm) + ms := make([]*metricpb.Metric, 0, len(grouped)) + for _, m := range grouped { + ms = append(ms, m) } // Report any transform errors. if len(errStrings) > 0 { - return rms, fmt.Errorf("%w:\n -%s", ErrTransforming, strings.Join(errStrings, "\n -")) + return ms, fmt.Errorf("%w:\n -%s", ErrTransforming, strings.Join(errStrings, "\n -")) } - return rms, nil + return ms, nil } // Record transforms a Record into an OTLP Metric. An ErrIncompatibleAgg diff --git a/exporters/otlp/otlpmetric/internal/metrictransform/metric_test.go b/exporters/otlp/otlpmetric/internal/metrictransform/metric_test.go index 4a8ec9288fe..ee5dcdd1aef 100644 --- a/exporters/otlp/otlpmetric/internal/metrictransform/metric_test.go +++ b/exporters/otlp/otlpmetric/internal/metrictransform/metric_test.go @@ -26,6 +26,7 @@ import ( "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/metric/metrictest" "go.opentelemetry.io/otel/metric/number" "go.opentelemetry.io/otel/metric/sdkapi" export "go.opentelemetry.io/otel/sdk/export/metric" @@ -122,7 +123,7 @@ func TestMinMaxSumCountValue(t *testing.T) { } func TestMinMaxSumCountDatapoints(t *testing.T) { - desc := metric.NewDescriptor("", sdkapi.ValueRecorderInstrumentKind, number.Int64Kind) + desc := metrictest.NewDescriptor("", sdkapi.ValueRecorderInstrumentKind, number.Int64Kind) labels := attribute.NewSet(attribute.String("one", "1")) mmscs := minmaxsumcount.New(2, &metric.Descriptor{}) mmsc, ckpt := &mmscs[0], &mmscs[1] @@ -178,7 +179,7 @@ func TestMinMaxSumCountPropagatesErrors(t *testing.T) { } func TestSumIntDataPoints(t *testing.T) { - desc := metric.NewDescriptor("", sdkapi.ValueRecorderInstrumentKind, number.Int64Kind) + desc := metrictest.NewDescriptor("", sdkapi.ValueRecorderInstrumentKind, number.Int64Kind) labels := attribute.NewSet(attribute.String("one", "1")) sums := sumAgg.New(2) s, ckpt := &sums[0], &sums[1] @@ -218,7 +219,7 @@ func TestSumIntDataPoints(t *testing.T) { } func TestSumFloatDataPoints(t *testing.T) { - desc := metric.NewDescriptor("", sdkapi.ValueRecorderInstrumentKind, number.Float64Kind) + desc := metrictest.NewDescriptor("", sdkapi.ValueRecorderInstrumentKind, number.Float64Kind) labels := attribute.NewSet(attribute.String("one", "1")) sums := sumAgg.New(2) s, ckpt := &sums[0], &sums[1] @@ -256,7 +257,7 @@ func TestSumFloatDataPoints(t *testing.T) { } func TestLastValueIntDataPoints(t *testing.T) { - desc := metric.NewDescriptor("", sdkapi.ValueRecorderInstrumentKind, number.Int64Kind) + desc := metrictest.NewDescriptor("", sdkapi.ValueRecorderInstrumentKind, number.Int64Kind) labels := attribute.NewSet(attribute.String("one", "1")) lvs := lvAgg.New(2) lv, ckpt := &lvs[0], &lvs[1] @@ -291,7 +292,7 @@ func TestLastValueIntDataPoints(t *testing.T) { } func TestExactIntDataPoints(t *testing.T) { - desc := metric.NewDescriptor("", sdkapi.ValueRecorderInstrumentKind, number.Int64Kind) + desc := metrictest.NewDescriptor("", sdkapi.ValueRecorderInstrumentKind, number.Int64Kind) labels := attribute.NewSet(attribute.String("one", "1")) arrs := arrAgg.New(2) e, ckpt := &arrs[0], &arrs[1] @@ -326,7 +327,7 @@ func TestExactIntDataPoints(t *testing.T) { } func TestExactFloatDataPoints(t *testing.T) { - desc := metric.NewDescriptor("", sdkapi.ValueRecorderInstrumentKind, number.Float64Kind) + desc := metrictest.NewDescriptor("", sdkapi.ValueRecorderInstrumentKind, number.Float64Kind) labels := attribute.NewSet(attribute.String("one", "1")) arrs := arrAgg.New(2) e, ckpt := &arrs[0], &arrs[1] @@ -360,7 +361,7 @@ func TestExactFloatDataPoints(t *testing.T) { } func TestSumErrUnknownValueType(t *testing.T) { - desc := metric.NewDescriptor("", sdkapi.ValueRecorderInstrumentKind, number.Kind(-1)) + desc := metrictest.NewDescriptor("", sdkapi.ValueRecorderInstrumentKind, number.Kind(-1)) labels := attribute.NewSet() s := &sumAgg.New(1)[0] record := export.NewRecord(&desc, &labels, s, intervalStart, intervalEnd) @@ -445,7 +446,7 @@ var _ aggregation.MinMaxSumCount = &testErrMinMaxSumCount{} func TestRecordAggregatorIncompatibleErrors(t *testing.T) { makeMpb := func(kind aggregation.Kind, agg aggregation.Aggregation) (*metricpb.Metric, error) { - desc := metric.NewDescriptor("things", sdkapi.CounterInstrumentKind, number.Int64Kind) + desc := metrictest.NewDescriptor("things", sdkapi.CounterInstrumentKind, number.Int64Kind) labels := attribute.NewSet() test := &testAgg{ kind: kind, @@ -481,7 +482,7 @@ func TestRecordAggregatorIncompatibleErrors(t *testing.T) { func TestRecordAggregatorUnexpectedErrors(t *testing.T) { makeMpb := func(kind aggregation.Kind, agg aggregation.Aggregation) (*metricpb.Metric, error) { - desc := metric.NewDescriptor("things", sdkapi.CounterInstrumentKind, number.Int64Kind) + desc := metrictest.NewDescriptor("things", sdkapi.CounterInstrumentKind, number.Int64Kind) labels := attribute.NewSet() return Record(export.CumulativeExportKindSelector(), export.NewRecord(&desc, &labels, agg, intervalStart, intervalEnd)) } diff --git a/exporters/otlp/otlpmetric/internal/otlpmetrictest/data.go b/exporters/otlp/otlpmetric/internal/otlpmetrictest/data.go index 1e7144ad752..b5a30d37fdc 100644 --- a/exporters/otlp/otlpmetric/internal/otlpmetrictest/data.go +++ b/exporters/otlp/otlpmetric/internal/otlpmetrictest/data.go @@ -21,35 +21,41 @@ import ( "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/metric/metrictest" "go.opentelemetry.io/otel/metric/number" "go.opentelemetry.io/otel/metric/sdkapi" exportmetric "go.opentelemetry.io/otel/sdk/export/metric" + "go.opentelemetry.io/otel/sdk/instrumentation" "go.opentelemetry.io/otel/sdk/metric/aggregator/sum" + "go.opentelemetry.io/otel/sdk/metric/processor/processortest" ) -// Used to avoid implementing locking functions for test -// checkpointsets. -type noopLocker struct{} - -// Lock implements sync.Locker, which is needed for -// exportmetric.CheckpointSet. -func (noopLocker) Lock() {} - -// Unlock implements sync.Locker, which is needed for -// exportmetric.CheckpointSet. -func (noopLocker) Unlock() {} - -// RLock implements exportmetric.CheckpointSet. -func (noopLocker) RLock() {} - -// RUnlock implements exportmetric.CheckpointSet. -func (noopLocker) RUnlock() {} - // OneRecordCheckpointSet is a CheckpointSet that returns just one // filled record. It may be useful for testing driver's metrics // export. -type OneRecordCheckpointSet struct { - noopLocker +func OneRecordCheckpointSet() exportmetric.InstrumentationLibraryMetricReader { + desc := metrictest.NewDescriptor( + "foo", + sdkapi.CounterInstrumentKind, + number.Int64Kind, + ) + agg := sum.New(1) + if err := agg[0].Update(context.Background(), number.NewInt64Number(42), &desc); err != nil { + panic(err) + } + start := time.Date(2020, time.December, 8, 19, 15, 0, 0, time.UTC) + end := time.Date(2020, time.December, 8, 19, 16, 0, 0, time.UTC) + labels := attribute.NewSet(attribute.String("abc", "def"), attribute.Int64("one", 1)) + rec := exportmetric.NewRecord(&desc, &labels, agg[0].Aggregation(), start, end) + + return processortest.MultiInstrumentationLibraryMetricReader( + map[instrumentation.Library][]exportmetric.Record{ + instrumentation.Library{ + Name: "onelib", + }: []exportmetric.Record{ + exportmetric.NewRecord(&desc, ), + }, + }) } var _ exportmetric.CheckpointSet = OneRecordCheckpointSet{} diff --git a/sdk/metric/processor/processortest/test.go b/sdk/metric/processor/processortest/test.go index 76469021c5b..fb2688ca9a1 100644 --- a/sdk/metric/processor/processortest/test.go +++ b/sdk/metric/processor/processortest/test.go @@ -394,15 +394,50 @@ func (e *Exporter) Reset() { e.exportCount = 0 } -func TestLibraryReader(l instrumentation.Library, r export.MetricReader) export.InstrumentationLibraryMetricReader { - return ilReader{l, r} +func OneInstrumentationLibraryMetricReader(l instrumentation.Library, r export.MetricReader) export.InstrumentationLibraryMetricReader { + return oneLibraryMetricReader{l, r} } -type ilReader struct { +type oneLibraryMetricReader struct { library instrumentation.Library reader export.MetricReader } -func (ilr ilReader) ForEach(readerFunc func(instrumentation.Library, export.MetricReader) error) error { - return readerFunc(ilr.library, ilr.reader) +func (o oneLibraryMetricReader) ForEach(readerFunc func(instrumentation.Library, export.MetricReader) error) error { + return readerFunc(o.library, o.reader) +} + +func MultiInstrumentationLibraryMetricReader(records map[instrumentation.Library][]export.Record) export.InstrumentationLibraryMetricReader { + return instrumentationLibraryMetricReader{records: records} +} + +type instrumentationLibraryMetricReader struct { + records map[instrumentation.Library][]export.Record +} + +var _ export.InstrumentationLibraryMetricReader = instrumentationLibraryMetricReader{} + +func (m instrumentationLibraryMetricReader) ForEach(fn func(instrumentation.Library, export.MetricReader) error) error { + for library, records := range m.records { + if err := fn(library, &metricReader{records: records}); err != nil { + return err + } + } + return nil +} + +type metricReader struct { + sync.RWMutex + records []export.Record +} + +var _ export.MetricReader = &metricReader{} + +func (m *metricReader) ForEach(_ export.ExportKindSelector, fn func(export.Record) error) error { + for _, record := range m.records { + if err := fn(record); err != nil && err != aggregation.ErrNoData { + return err + } + } + return nil } diff --git a/sdk/resource/resource.go b/sdk/resource/resource.go index 0d442f91716..5a2eaec7230 100644 --- a/sdk/resource/resource.go +++ b/sdk/resource/resource.go @@ -132,6 +132,9 @@ func (r *Resource) Attributes() []attribute.KeyValue { } func (r *Resource) SchemaURL() string { + if r == nil { + return "" + } return r.schemaURL } From 5b9625f938f0f1e63a2d3a12c3d87b0cc9aa042e Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 20 Aug 2021 00:03:44 -0700 Subject: [PATCH 04/33] otlp and opencensus tests passing --- bridge/opencensus/exporter.go | 25 +++++++-- bridge/opencensus/exporter_test.go | 34 ++++++------ bridge/opencensus/go.mod | 1 + bridge/opencensus/go.sum | 2 + bridge/opencensus/test/go.sum | 2 + .../internal/otlpmetrictest/data.go | 55 ++++--------------- .../internal/otlpmetrictest/otlptest.go | 2 +- .../otlpmetric/otlpmetricgrpc/client_test.go | 6 +- .../otlpmetric/otlpmetricgrpc/example_test.go | 6 +- .../otlpmetric/otlpmetrichttp/client_test.go | 2 +- 10 files changed, 58 insertions(+), 77 deletions(-) diff --git a/bridge/opencensus/exporter.go b/bridge/opencensus/exporter.go index c5a0986b44b..63a78ff5c89 100644 --- a/bridge/opencensus/exporter.go +++ b/bridge/opencensus/exporter.go @@ -32,6 +32,7 @@ import ( "go.opentelemetry.io/otel/metric/unit" export "go.opentelemetry.io/otel/sdk/export/metric" "go.opentelemetry.io/otel/sdk/export/metric/aggregation" + "go.opentelemetry.io/otel/sdk/instrumentation" "go.opentelemetry.io/otel/sdk/resource" ) @@ -55,18 +56,30 @@ func (e *exporter) ExportMetrics(ctx context.Context, metrics []*metricdata.Metr if len(metrics) != 0 { res = convertResource(metrics[0].Resource) } - return e.base.Export(ctx, res, &checkpointSet{metrics: metrics}) + return e.base.Export(ctx, res, &censusLibraryMetricReader{metrics: metrics}) } -type checkpointSet struct { - // RWMutex implements locking for the `CheckpointSet` interface. +type censusLibraryMetricReader struct { + metrics []*metricdata.Metric +} + +func (r censusLibraryMetricReader) ForEach(readerFunc func(instrumentation.Library, export.MetricReader) error) error { + return readerFunc(instrumentation.Library{ + Name: "OpenCensus Bridge", + }, &metricReader{metrics: r.metrics}) +} + +type metricReader struct { + // RWMutex implements locking for the `MetricReader` interface. sync.RWMutex metrics []*metricdata.Metric } +var _ export.MetricReader = &metricReader{} + // ForEach iterates through the CheckpointSet, passing an // export.Record with the appropriate aggregation to an exporter. -func (d *checkpointSet) ForEach(exporter export.ExportKindSelector, f func(export.Record) error) error { +func (d *metricReader) ForEach(exporter export.ExportKindSelector, f func(export.Record) error) error { for _, m := range d.metrics { descriptor, err := convertDescriptor(m.Descriptor) if err != nil { @@ -158,7 +171,6 @@ func convertDescriptor(ocDescriptor metricdata.Descriptor) (metric.Descriptor, e } opts := []metric.InstrumentOption{ metric.WithDescription(ocDescriptor.Description), - metric.WithInstrumentationName("OpenCensus Bridge"), } switch ocDescriptor.Unit { case metricdata.UnitDimensionless: @@ -168,5 +180,6 @@ func convertDescriptor(ocDescriptor metricdata.Descriptor) (metric.Descriptor, e case metricdata.UnitMilliseconds: opts = append(opts, metric.WithUnit(unit.Milliseconds)) } - return metric.NewDescriptor(ocDescriptor.Name, ikind, nkind, opts...), nil + cfg := metric.NewInstrumentConfig(opts...) + return metric.NewDescriptor(ocDescriptor.Name, ikind, nkind, cfg.Description(), cfg.Unit()), nil } diff --git a/bridge/opencensus/exporter_test.go b/bridge/opencensus/exporter_test.go index eace2b0633c..3b44bb61812 100644 --- a/bridge/opencensus/exporter_test.go +++ b/bridge/opencensus/exporter_test.go @@ -28,12 +28,15 @@ import ( "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/metric/metrictest" "go.opentelemetry.io/otel/metric/number" "go.opentelemetry.io/otel/metric/sdkapi" "go.opentelemetry.io/otel/metric/unit" export "go.opentelemetry.io/otel/sdk/export/metric" exportmetric "go.opentelemetry.io/otel/sdk/export/metric" "go.opentelemetry.io/otel/sdk/export/metric/aggregation" + "go.opentelemetry.io/otel/sdk/instrumentation" + "go.opentelemetry.io/otel/sdk/metric/controller/controllertest" "go.opentelemetry.io/otel/sdk/resource" ) @@ -44,12 +47,13 @@ type fakeExporter struct { err error } -func (f *fakeExporter) Export(ctx context.Context, res *resource.Resource, cps exportmetric.CheckpointSet) error { - return cps.ForEach(f, func(record exportmetric.Record) error { - f.resource = res - f.records = append(f.records, record) - return f.err - }) +func (f *fakeExporter) Export(ctx context.Context, res *resource.Resource, ilmr exportmetric.InstrumentationLibraryMetricReader) error { + return controllertest.ReadAll(ilmr, export.StatelessExportKindSelector(), + func(_ instrumentation.Library, record exportmetric.Record) error { + f.resource = res + f.records = append(f.records, record) + return f.err + }) } type fakeErrorHandler struct { @@ -71,11 +75,10 @@ func (f *fakeErrorHandler) matches(err error) error { func TestExportMetrics(t *testing.T) { now := time.Now() - basicDesc := metric.NewDescriptor( + basicDesc := metrictest.NewDescriptor( "", sdkapi.ValueObserverInstrumentKind, number.Int64Kind, - metric.WithInstrumentationName("OpenCensus Bridge"), ) fakeErrorHandler := &fakeErrorHandler{} otel.SetErrorHandler(fakeErrorHandler) @@ -393,11 +396,10 @@ func TestConvertDescriptor(t *testing.T) { }{ { desc: "empty descriptor", - expected: metric.NewDescriptor( + expected: metrictest.NewDescriptor( "", sdkapi.ValueObserverInstrumentKind, number.Int64Kind, - metric.WithInstrumentationName("OpenCensus Bridge"), ), }, { @@ -408,11 +410,10 @@ func TestConvertDescriptor(t *testing.T) { Unit: metricdata.UnitBytes, Type: metricdata.TypeGaugeInt64, }, - expected: metric.NewDescriptor( + expected: metrictest.NewDescriptor( "foo", sdkapi.ValueObserverInstrumentKind, number.Int64Kind, - metric.WithInstrumentationName("OpenCensus Bridge"), metric.WithDescription("bar"), metric.WithUnit(unit.Bytes), ), @@ -425,11 +426,10 @@ func TestConvertDescriptor(t *testing.T) { Unit: metricdata.UnitMilliseconds, Type: metricdata.TypeGaugeFloat64, }, - expected: metric.NewDescriptor( + expected: metrictest.NewDescriptor( "foo", sdkapi.ValueObserverInstrumentKind, number.Float64Kind, - metric.WithInstrumentationName("OpenCensus Bridge"), metric.WithDescription("bar"), metric.WithUnit(unit.Milliseconds), ), @@ -442,11 +442,10 @@ func TestConvertDescriptor(t *testing.T) { Unit: metricdata.UnitDimensionless, Type: metricdata.TypeCumulativeInt64, }, - expected: metric.NewDescriptor( + expected: metrictest.NewDescriptor( "foo", sdkapi.SumObserverInstrumentKind, number.Int64Kind, - metric.WithInstrumentationName("OpenCensus Bridge"), metric.WithDescription("bar"), metric.WithUnit(unit.Dimensionless), ), @@ -459,11 +458,10 @@ func TestConvertDescriptor(t *testing.T) { Unit: metricdata.UnitDimensionless, Type: metricdata.TypeCumulativeFloat64, }, - expected: metric.NewDescriptor( + expected: metrictest.NewDescriptor( "foo", sdkapi.SumObserverInstrumentKind, number.Float64Kind, - metric.WithInstrumentationName("OpenCensus Bridge"), metric.WithDescription("bar"), metric.WithUnit(unit.Dimensionless), ), diff --git a/bridge/opencensus/go.mod b/bridge/opencensus/go.mod index 46983755edd..81cd6c32d8b 100644 --- a/bridge/opencensus/go.mod +++ b/bridge/opencensus/go.mod @@ -8,6 +8,7 @@ require ( go.opentelemetry.io/otel/metric v0.22.0 go.opentelemetry.io/otel/sdk v1.0.0-RC2 go.opentelemetry.io/otel/sdk/export/metric v0.22.0 + go.opentelemetry.io/otel/sdk/metric v0.22.0 go.opentelemetry.io/otel/trace v1.0.0-RC2 ) diff --git a/bridge/opencensus/go.sum b/bridge/opencensus/go.sum index 0a5b78fb16b..06db7855f05 100644 --- a/bridge/opencensus/go.sum +++ b/bridge/opencensus/go.sum @@ -1,5 +1,7 @@ cloud.google.com/go v0.26.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw= github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= +github.com/benbjohnson/clock v1.1.0 h1:Q92kusRqC1XV2MjkWETPvjJVqKetz1OzxZB7mHJLju8= +github.com/benbjohnson/clock v1.1.0/go.mod h1:J11/hYXuz8f4ySSvYwY0FKfm+ezbsZBKZxNJlLklBHA= github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw= github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= diff --git a/bridge/opencensus/test/go.sum b/bridge/opencensus/test/go.sum index 9cb1b023e7f..b77d6496659 100644 --- a/bridge/opencensus/test/go.sum +++ b/bridge/opencensus/test/go.sum @@ -1,5 +1,7 @@ cloud.google.com/go v0.26.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw= github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= +github.com/benbjohnson/clock v1.1.0 h1:Q92kusRqC1XV2MjkWETPvjJVqKetz1OzxZB7mHJLju8= +github.com/benbjohnson/clock v1.1.0/go.mod h1:J11/hYXuz8f4ySSvYwY0FKfm+ezbsZBKZxNJlLklBHA= github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU= github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw= github.com/cncf/udpa/go v0.0.0-20191209042840-269d4d468f6f/go.mod h1:M8M6+tZqaGXZJjfX53e64911xZQV5JYwmTeXPW+k8Sc= diff --git a/exporters/otlp/otlpmetric/internal/otlpmetrictest/data.go b/exporters/otlp/otlpmetric/internal/otlpmetrictest/data.go index b5a30d37fdc..4b15b4c7dcc 100644 --- a/exporters/otlp/otlpmetric/internal/otlpmetrictest/data.go +++ b/exporters/otlp/otlpmetric/internal/otlpmetrictest/data.go @@ -20,7 +20,6 @@ import ( "time" "go.opentelemetry.io/otel/attribute" - "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/metric/metrictest" "go.opentelemetry.io/otel/metric/number" "go.opentelemetry.io/otel/metric/sdkapi" @@ -30,10 +29,10 @@ import ( "go.opentelemetry.io/otel/sdk/metric/processor/processortest" ) -// OneRecordCheckpointSet is a CheckpointSet that returns just one +// OneRecordMetricReader is a MetricReader that returns just one // filled record. It may be useful for testing driver's metrics // export. -func OneRecordCheckpointSet() exportmetric.InstrumentationLibraryMetricReader { +func OneRecordMetricReader() exportmetric.InstrumentationLibraryMetricReader { desc := metrictest.NewDescriptor( "foo", sdkapi.CounterInstrumentKind, @@ -52,55 +51,21 @@ func OneRecordCheckpointSet() exportmetric.InstrumentationLibraryMetricReader { map[instrumentation.Library][]exportmetric.Record{ instrumentation.Library{ Name: "onelib", - }: []exportmetric.Record{ - exportmetric.NewRecord(&desc, ), - }, + }: []exportmetric.Record{rec}, }) } -var _ exportmetric.CheckpointSet = OneRecordCheckpointSet{} - -// ForEach implements exportmetric.CheckpointSet. It always invokes -// the callback once with always the same record. -func (OneRecordCheckpointSet) ForEach(kindSelector exportmetric.ExportKindSelector, recordFunc func(exportmetric.Record) error) error { - desc := metric.NewDescriptor( - "foo", - sdkapi.CounterInstrumentKind, - number.Int64Kind, - ) - agg := sum.New(1) - if err := agg[0].Update(context.Background(), number.NewInt64Number(42), &desc); err != nil { - return err - } - start := time.Date(2020, time.December, 8, 19, 15, 0, 0, time.UTC) - end := time.Date(2020, time.December, 8, 19, 16, 0, 0, time.UTC) - labels := attribute.NewSet(attribute.String("abc", "def"), attribute.Int64("one", 1)) - rec := exportmetric.NewRecord(&desc, &labels, agg[0].Aggregation(), start, end) - return recordFunc(rec) -} - -// EmptyCheckpointSet is a checkpointer that has no records at all. -type EmptyCheckpointSet struct { - noopLocker +func EmptyMetricReader() exportmetric.InstrumentationLibraryMetricReader { + return processortest.MultiInstrumentationLibraryMetricReader(nil) } -var _ exportmetric.CheckpointSet = EmptyCheckpointSet{} - -// ForEach implements exportmetric.CheckpointSet. It never invokes the -// callback. -func (EmptyCheckpointSet) ForEach(kindSelector exportmetric.ExportKindSelector, recordFunc func(exportmetric.Record) error) error { - return nil -} - -// FailCheckpointSet is a checkpointer that returns an error during +// FailMetricReader is a checkpointer that returns an error during // ForEach. -type FailCheckpointSet struct { - noopLocker -} +type FailMetricReader struct{} -var _ exportmetric.CheckpointSet = FailCheckpointSet{} +var _ exportmetric.InstrumentationLibraryMetricReader = FailMetricReader{} -// ForEach implements exportmetric.CheckpointSet. It always fails. -func (FailCheckpointSet) ForEach(kindSelector exportmetric.ExportKindSelector, recordFunc func(exportmetric.Record) error) error { +// ForEach implements exportmetric.MetricReader. It always fails. +func (FailMetricReader) ForEach(readerFunc func(instrumentation.Library, exportmetric.MetricReader) error) error { return fmt.Errorf("fail") } diff --git a/exporters/otlp/otlpmetric/internal/otlpmetrictest/otlptest.go b/exporters/otlp/otlpmetric/internal/otlpmetrictest/otlptest.go index 6785958bdc0..84d07546bd0 100644 --- a/exporters/otlp/otlpmetric/internal/otlpmetrictest/otlptest.go +++ b/exporters/otlp/otlpmetric/internal/otlpmetrictest/otlptest.go @@ -40,7 +40,7 @@ import ( // themselves. func RunEndToEndTest(ctx context.Context, t *testing.T, exp *otlpmetric.Exporter, mcMetrics Collector) { selector := simple.NewWithInexpensiveDistribution() - proc := processor.New(selector, exportmetric.StatelessExportKindSelector()) + proc := processor.NewFactory(selector, exportmetric.StatelessExportKindSelector()) cont := controller.New(proc, controller.WithExporter(exp)) require.NoError(t, cont.Start(ctx)) diff --git a/exporters/otlp/otlpmetric/otlpmetricgrpc/client_test.go b/exporters/otlp/otlpmetric/otlpmetricgrpc/client_test.go index e55d240d9eb..6b197893599 100644 --- a/exporters/otlp/otlpmetric/otlpmetricgrpc/client_test.go +++ b/exporters/otlp/otlpmetric/otlpmetricgrpc/client_test.go @@ -40,7 +40,7 @@ import ( ) var ( - oneRecord = otlpmetrictest.OneRecordCheckpointSet{} + oneRecord = otlpmetrictest.OneRecordMetricReader() testResource = resource.Empty() ) @@ -719,7 +719,7 @@ func TestEmptyData(t *testing.T) { assert.NoError(t, exp.Shutdown(ctx)) }() - assert.NoError(t, exp.Export(ctx, testResource, otlpmetrictest.EmptyCheckpointSet{})) + assert.NoError(t, exp.Export(ctx, testResource, otlpmetrictest.EmptyMetricReader())) } func TestFailedMetricTransform(t *testing.T) { @@ -737,5 +737,5 @@ func TestFailedMetricTransform(t *testing.T) { assert.NoError(t, exp.Shutdown(ctx)) }() - assert.Error(t, exp.Export(ctx, testResource, otlpmetrictest.FailCheckpointSet{})) + assert.Error(t, exp.Export(ctx, testResource, otlpmetrictest.FailMetricReader{})) } diff --git a/exporters/otlp/otlpmetric/otlpmetricgrpc/example_test.go b/exporters/otlp/otlpmetric/otlpmetricgrpc/example_test.go index 51cebd41ed2..abbe0e981d4 100644 --- a/exporters/otlp/otlpmetric/otlpmetricgrpc/example_test.go +++ b/exporters/otlp/otlpmetric/otlpmetricgrpc/example_test.go @@ -48,7 +48,7 @@ func Example_insecure() { }() pusher := controller.New( - processor.New( + processor.NewFactory( simple.NewWithExactDistribution(), exp, ), @@ -107,7 +107,7 @@ func Example_withTLS() { }() pusher := controller.New( - processor.New( + processor.NewFactory( simple.NewWithExactDistribution(), exp, ), @@ -164,7 +164,7 @@ func Example_withDifferentSignalCollectors() { }() pusher := controller.New( - processor.New( + processor.NewFactory( simple.NewWithExactDistribution(), exp, ), diff --git a/exporters/otlp/otlpmetric/otlpmetrichttp/client_test.go b/exporters/otlp/otlpmetric/otlpmetrichttp/client_test.go index 55bd67e11bc..63e7688f8f1 100644 --- a/exporters/otlp/otlpmetric/otlpmetrichttp/client_test.go +++ b/exporters/otlp/otlpmetric/otlpmetrichttp/client_test.go @@ -38,7 +38,7 @@ const ( ) var ( - oneRecord = otlpmetrictest.OneRecordCheckpointSet{} + oneRecord = otlpmetrictest.OneRecordMetricReader() testResource = resource.Empty() ) From 72e8439304edd2ea39260fcd76e90bf5b2aee275 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 20 Aug 2021 00:13:36 -0700 Subject: [PATCH 05/33] prometheus --- exporters/prometheus/prometheus.go | 74 +++++++++++++------------ exporters/prometheus/prometheus_test.go | 2 +- 2 files changed, 41 insertions(+), 35 deletions(-) diff --git a/exporters/prometheus/prometheus.go b/exporters/prometheus/prometheus.go index 1b6fd1f3ca5..6149bdfd2b6 100644 --- a/exporters/prometheus/prometheus.go +++ b/exporters/prometheus/prometheus.go @@ -32,6 +32,7 @@ import ( "go.opentelemetry.io/otel/metric/number" export "go.opentelemetry.io/otel/sdk/export/metric" "go.opentelemetry.io/otel/sdk/export/metric/aggregation" + "go.opentelemetry.io/otel/sdk/instrumentation" controller "go.opentelemetry.io/otel/sdk/metric/controller/basic" "go.opentelemetry.io/otel/sdk/resource" ) @@ -152,11 +153,13 @@ func (c *collector) Describe(ch chan<- *prometheus.Desc) { c.exp.lock.RLock() defer c.exp.lock.RUnlock() - _ = c.exp.Controller().ForEach(c.exp, func(record export.Record) error { - var labelKeys []string - mergeLabels(record, c.exp.controller.Resource(), &labelKeys, nil) - ch <- c.toDesc(record, labelKeys) - return nil + _ = c.exp.Controller().Reader().ForEach(func(_ instrumentation.Library, reader export.MetricReader) error { + return reader.ForEach(c.exp, func(record export.Record) error { + var labelKeys []string + mergeLabels(record, c.exp.controller.Resource(), &labelKeys, nil) + ch <- c.toDesc(record, labelKeys) + return nil + }) }) } @@ -173,36 +176,39 @@ func (c *collector) Collect(ch chan<- prometheus.Metric) { otel.Handle(err) } - err := ctrl.ForEach(c.exp, func(record export.Record) error { - agg := record.Aggregation() - numberKind := record.Descriptor().NumberKind() - instrumentKind := record.Descriptor().InstrumentKind() - - var labelKeys, labels []string - mergeLabels(record, c.exp.controller.Resource(), &labelKeys, &labels) - - desc := c.toDesc(record, labelKeys) - - if hist, ok := agg.(aggregation.Histogram); ok { - if err := c.exportHistogram(ch, hist, numberKind, desc, labels); err != nil { - return fmt.Errorf("exporting histogram: %w", err) - } - } else if sum, ok := agg.(aggregation.Sum); ok && instrumentKind.Monotonic() { - if err := c.exportMonotonicCounter(ch, sum, numberKind, desc, labels); err != nil { - return fmt.Errorf("exporting monotonic counter: %w", err) + err := ctrl.Reader().ForEach(func(_ instrumentation.Library, reader export.MetricReader) error { + return reader.ForEach(c.exp, func(record export.Record) error { + + agg := record.Aggregation() + numberKind := record.Descriptor().NumberKind() + instrumentKind := record.Descriptor().InstrumentKind() + + var labelKeys, labels []string + mergeLabels(record, c.exp.controller.Resource(), &labelKeys, &labels) + + desc := c.toDesc(record, labelKeys) + + if hist, ok := agg.(aggregation.Histogram); ok { + if err := c.exportHistogram(ch, hist, numberKind, desc, labels); err != nil { + return fmt.Errorf("exporting histogram: %w", err) + } + } else if sum, ok := agg.(aggregation.Sum); ok && instrumentKind.Monotonic() { + if err := c.exportMonotonicCounter(ch, sum, numberKind, desc, labels); err != nil { + return fmt.Errorf("exporting monotonic counter: %w", err) + } + } else if sum, ok := agg.(aggregation.Sum); ok && !instrumentKind.Monotonic() { + if err := c.exportNonMonotonicCounter(ch, sum, numberKind, desc, labels); err != nil { + return fmt.Errorf("exporting non monotonic counter: %w", err) + } + } else if lastValue, ok := agg.(aggregation.LastValue); ok { + if err := c.exportLastValue(ch, lastValue, numberKind, desc, labels); err != nil { + return fmt.Errorf("exporting last value: %w", err) + } + } else { + return fmt.Errorf("%w: %s", ErrUnsupportedAggregator, agg.Kind()) } - } else if sum, ok := agg.(aggregation.Sum); ok && !instrumentKind.Monotonic() { - if err := c.exportNonMonotonicCounter(ch, sum, numberKind, desc, labels); err != nil { - return fmt.Errorf("exporting non monotonic counter: %w", err) - } - } else if lastValue, ok := agg.(aggregation.LastValue); ok { - if err := c.exportLastValue(ch, lastValue, numberKind, desc, labels); err != nil { - return fmt.Errorf("exporting last value: %w", err) - } - } else { - return fmt.Errorf("%w: %s", ErrUnsupportedAggregator, agg.Kind()) - } - return nil + return nil + }) }) if err != nil { otel.Handle(err) diff --git a/exporters/prometheus/prometheus_test.go b/exporters/prometheus/prometheus_test.go index 0f3895813c7..719ca1d13c4 100644 --- a/exporters/prometheus/prometheus_test.go +++ b/exporters/prometheus/prometheus_test.go @@ -84,7 +84,7 @@ func expectHistogram(name string, values ...string) expectedMetric { func newPipeline(config prometheus.Config, options ...controller.Option) (*prometheus.Exporter, error) { c := controller.New( - processor.New( + processor.NewFactory( selector.NewWithHistogramDistribution( histogram.WithExplicitBoundaries(config.DefaultHistogramBoundaries), ), From 198f350fdbe85bb9f085d14df7979374638fd70b Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 20 Aug 2021 10:10:06 -0700 Subject: [PATCH 06/33] tests pass, working on lint --- example/prometheus/main.go | 2 +- exporters/otlp/otlpmetric/exporter_test.go | 2 +- .../internal/otlpmetrictest/data.go | 4 +- exporters/stdout/stdoutmetric/example_test.go | 2 +- exporters/stdout/stdoutmetric/metric.go | 137 +++++++++--------- exporters/stdout/stdoutmetric/metric_test.go | 4 +- sdk/export/metric/exportkind_test.go | 4 +- sdk/metric/processor/basic/basic_test.go | 2 +- .../processor/processortest/test_test.go | 2 +- sdk/metric/processor/reducer/reducer_test.go | 2 +- 10 files changed, 84 insertions(+), 77 deletions(-) diff --git a/example/prometheus/main.go b/example/prometheus/main.go index 81cce5cda0f..6826363e881 100644 --- a/example/prometheus/main.go +++ b/example/prometheus/main.go @@ -40,7 +40,7 @@ var ( func initMeter() { config := prometheus.Config{} c := controller.New( - processor.New( + processor.NewFactory( selector.NewWithHistogramDistribution( histogram.WithExplicitBoundaries(config.DefaultHistogramBoundaries), ), diff --git a/exporters/otlp/otlpmetric/exporter_test.go b/exporters/otlp/otlpmetric/exporter_test.go index 9091399cbfd..a9fe21d100c 100644 --- a/exporters/otlp/otlpmetric/exporter_test.go +++ b/exporters/otlp/otlpmetric/exporter_test.go @@ -785,7 +785,7 @@ func TestEmptyMetricExport(t *testing.T) { } { driver.Reset() require.NoError(t, exp.Export(context.Background(), resource.Empty(), processortest.MultiInstrumentationLibraryMetricReader(map[instrumentation.Library][]metricsdk.Record{ - instrumentation.Library{ + { Name: testLibName, }: test.records, }))) diff --git a/exporters/otlp/otlpmetric/internal/otlpmetrictest/data.go b/exporters/otlp/otlpmetric/internal/otlpmetrictest/data.go index 4b15b4c7dcc..07faccbde53 100644 --- a/exporters/otlp/otlpmetric/internal/otlpmetrictest/data.go +++ b/exporters/otlp/otlpmetric/internal/otlpmetrictest/data.go @@ -49,9 +49,9 @@ func OneRecordMetricReader() exportmetric.InstrumentationLibraryMetricReader { return processortest.MultiInstrumentationLibraryMetricReader( map[instrumentation.Library][]exportmetric.Record{ - instrumentation.Library{ + { Name: "onelib", - }: []exportmetric.Record{rec}, + }: {rec}, }) } diff --git a/exporters/stdout/stdoutmetric/example_test.go b/exporters/stdout/stdoutmetric/example_test.go index 7107b9fce66..9c23bef9100 100644 --- a/exporters/stdout/stdoutmetric/example_test.go +++ b/exporters/stdout/stdoutmetric/example_test.go @@ -72,7 +72,7 @@ func InstallExportPipeline(ctx context.Context) func() { } pusher := controller.New( - processor.New( + processor.NewFactory( simple.NewWithInexpensiveDistribution(), exporter, ), diff --git a/exporters/stdout/stdoutmetric/metric.go b/exporters/stdout/stdoutmetric/metric.go index 922d473f8d3..c98391ea78f 100644 --- a/exporters/stdout/stdoutmetric/metric.go +++ b/exporters/stdout/stdoutmetric/metric.go @@ -25,6 +25,7 @@ import ( "go.opentelemetry.io/otel/metric" exportmetric "go.opentelemetry.io/otel/sdk/export/metric" "go.opentelemetry.io/otel/sdk/export/metric/aggregation" + "go.opentelemetry.io/otel/sdk/instrumentation" "go.opentelemetry.io/otel/sdk/resource" ) @@ -50,93 +51,99 @@ func (e *metricExporter) ExportKindFor(desc *metric.Descriptor, kind aggregation return exportmetric.StatelessExportKindSelector().ExportKindFor(desc, kind) } -func (e *metricExporter) Export(_ context.Context, res *resource.Resource, checkpointSet exportmetric.CheckpointSet) error { +func (e *metricExporter) Export(_ context.Context, res *resource.Resource, reader exportmetric.InstrumentationLibraryMetricReader) error { var aggError error var batch []line - aggError = checkpointSet.ForEach(e, func(record exportmetric.Record) error { - desc := record.Descriptor() - agg := record.Aggregation() - kind := desc.NumberKind() - encodedResource := res.Encoded(e.config.LabelEncoder) + aggError = reader.ForEach(func(lib instrumentation.Library, mr exportmetric.MetricReader) error { var instLabels []attribute.KeyValue - if name := desc.InstrumentationName(); name != "" { + if name := lib.Name; name != "" { instLabels = append(instLabels, attribute.String("instrumentation.name", name)) - if version := desc.InstrumentationVersion(); version != "" { + if version := lib.Version; version != "" { instLabels = append(instLabels, attribute.String("instrumentation.version", version)) } + if schema := lib.SchemaURL; schema != "" { + instLabels = append(instLabels, attribute.String("instrumentation.schema_url", schema)) + } } instSet := attribute.NewSet(instLabels...) encodedInstLabels := instSet.Encoded(e.config.LabelEncoder) - var expose line + return mr.ForEach(e, func(record exportmetric.Record) error { + desc := record.Descriptor() + agg := record.Aggregation() + kind := desc.NumberKind() + encodedResource := res.Encoded(e.config.LabelEncoder) - if sum, ok := agg.(aggregation.Sum); ok { - value, err := sum.Sum() - if err != nil { - return err - } - expose.Sum = value.AsInterface(kind) - } + var expose line - if mmsc, ok := agg.(aggregation.MinMaxSumCount); ok { - count, err := mmsc.Count() - if err != nil { - return err + if sum, ok := agg.(aggregation.Sum); ok { + value, err := sum.Sum() + if err != nil { + return err + } + expose.Sum = value.AsInterface(kind) } - expose.Count = count - max, err := mmsc.Max() - if err != nil { - return err + if mmsc, ok := agg.(aggregation.MinMaxSumCount); ok { + count, err := mmsc.Count() + if err != nil { + return err + } + expose.Count = count + + max, err := mmsc.Max() + if err != nil { + return err + } + expose.Max = max.AsInterface(kind) + + min, err := mmsc.Min() + if err != nil { + return err + } + expose.Min = min.AsInterface(kind) + } else if lv, ok := agg.(aggregation.LastValue); ok { + value, timestamp, err := lv.LastValue() + if err != nil { + return err + } + expose.LastValue = value.AsInterface(kind) + + if e.config.Timestamps { + expose.Timestamp = ×tamp + } } - expose.Max = max.AsInterface(kind) - min, err := mmsc.Min() - if err != nil { - return err - } - expose.Min = min.AsInterface(kind) - } else if lv, ok := agg.(aggregation.LastValue); ok { - value, timestamp, err := lv.LastValue() - if err != nil { - return err + var encodedLabels string + iter := record.Labels().Iter() + if iter.Len() > 0 { + encodedLabels = record.Labels().Encoded(e.config.LabelEncoder) } - expose.LastValue = value.AsInterface(kind) - if e.config.Timestamps { - expose.Timestamp = ×tamp + var sb strings.Builder + + sb.WriteString(desc.Name()) + + if len(encodedLabels) > 0 || len(encodedResource) > 0 || len(encodedInstLabels) > 0 { + sb.WriteRune('{') + sb.WriteString(encodedResource) + if len(encodedInstLabels) > 0 && len(encodedResource) > 0 { + sb.WriteRune(',') + } + sb.WriteString(encodedInstLabels) + if len(encodedLabels) > 0 && (len(encodedInstLabels) > 0 || len(encodedResource) > 0) { + sb.WriteRune(',') + } + sb.WriteString(encodedLabels) + sb.WriteRune('}') } - } - - var encodedLabels string - iter := record.Labels().Iter() - if iter.Len() > 0 { - encodedLabels = record.Labels().Encoded(e.config.LabelEncoder) - } - - var sb strings.Builder - - sb.WriteString(desc.Name()) - - if len(encodedLabels) > 0 || len(encodedResource) > 0 || len(encodedInstLabels) > 0 { - sb.WriteRune('{') - sb.WriteString(encodedResource) - if len(encodedInstLabels) > 0 && len(encodedResource) > 0 { - sb.WriteRune(',') - } - sb.WriteString(encodedInstLabels) - if len(encodedLabels) > 0 && (len(encodedInstLabels) > 0 || len(encodedResource) > 0) { - sb.WriteRune(',') - } - sb.WriteString(encodedLabels) - sb.WriteRune('}') - } - expose.Name = sb.String() + expose.Name = sb.String() - batch = append(batch, expose) - return nil + batch = append(batch, expose) + return nil + }) }) if len(batch) == 0 { return aggError diff --git a/exporters/stdout/stdoutmetric/metric_test.go b/exporters/stdout/stdoutmetric/metric_test.go index d1f3c2265c8..6aaa11efa72 100644 --- a/exporters/stdout/stdoutmetric/metric_test.go +++ b/exporters/stdout/stdoutmetric/metric_test.go @@ -61,7 +61,7 @@ func newFixtureWithResource(t *testing.T, res *resource.Resource, opts ...stdout t.Fatal("Error building fixture: ", err) } aggSel := processortest.AggregatorSelector() - proc := processor.New(aggSel, export.StatelessExportKindSelector()) + proc := processor.NewFactory(aggSel, export.StatelessExportKindSelector()) cont := controller.New(proc, controller.WithExporter(exp), controller.WithResource(res), @@ -87,7 +87,7 @@ func (fix testFixture) Output() string { func TestStdoutTimestamp(t *testing.T) { var buf bytes.Buffer aggSel := processortest.AggregatorSelector() - proc := processor.New(aggSel, export.CumulativeExportKindSelector()) + proc := processor.NewFactory(aggSel, export.CumulativeExportKindSelector()) exporter, err := stdoutmetric.New( stdoutmetric.WithWriter(&buf), ) diff --git a/sdk/export/metric/exportkind_test.go b/sdk/export/metric/exportkind_test.go index c862dd3ed2f..a6df2558bf0 100644 --- a/sdk/export/metric/exportkind_test.go +++ b/sdk/export/metric/exportkind_test.go @@ -19,7 +19,7 @@ import ( "github.com/stretchr/testify/require" - "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/metric/metrictest" "go.opentelemetry.io/otel/metric/number" "go.opentelemetry.io/otel/metric/sdkapi" "go.opentelemetry.io/otel/sdk/export/metric/aggregation" @@ -60,7 +60,7 @@ func TestExportKindSelectors(t *testing.T) { seks := StatelessExportKindSelector() for _, ikind := range append(deltaMemoryKinds, cumulativeMemoryKinds...) { - desc := metric.NewDescriptor("instrument", ikind, number.Int64Kind) + desc := metrictest.NewDescriptor("instrument", ikind, number.Int64Kind) var akind aggregation.Kind if ikind.Adding() { diff --git a/sdk/metric/processor/basic/basic_test.go b/sdk/metric/processor/basic/basic_test.go index 86d434ddc6e..8804415026a 100644 --- a/sdk/metric/processor/basic/basic_test.go +++ b/sdk/metric/processor/basic/basic_test.go @@ -500,7 +500,7 @@ func TestSumObserverEndToEnd(t *testing.T) { require.NoError(t, proc.FinishCollection()) exporter := processortest.New(eselector, attribute.DefaultEncoder()) - require.NoError(t, exporter.Export(ctx, resource.Empty(), processortest.TestLibraryReader( + require.NoError(t, exporter.Export(ctx, resource.Empty(), processortest.OneInstrumentationLibraryMetricReader( instrumentation.Library{ Name: "test", }, data))) diff --git a/sdk/metric/processor/processortest/test_test.go b/sdk/metric/processor/processortest/test_test.go index bd42f6772f2..aee7a1370a6 100644 --- a/sdk/metric/processor/processortest/test_test.go +++ b/sdk/metric/processor/processortest/test_test.go @@ -75,7 +75,7 @@ func TestProcessorTesting(t *testing.T) { attribute.DefaultEncoder(), ) - err := exporter.Export(context.Background(), res, processortest.TestLibraryReader( + err := exporter.Export(context.Background(), res, processortest.OneInstrumentationLibraryMetricReader( instrumentation.Library{ Name: "test", }, diff --git a/sdk/metric/processor/reducer/reducer_test.go b/sdk/metric/processor/reducer/reducer_test.go index b05bbf5ecd9..407a5a79754 100644 --- a/sdk/metric/processor/reducer/reducer_test.go +++ b/sdk/metric/processor/reducer/reducer_test.go @@ -105,7 +105,7 @@ func TestFilterBasicProcessor(t *testing.T) { } res := resource.NewSchemaless(attribute.String("R", "V")) - require.NoError(t, exporter.Export(context.Background(), res, processortest.TestLibraryReader(instrumentation.Library{ + require.NoError(t, exporter.Export(context.Background(), res, processortest.OneInstrumentationLibraryMetricReader(instrumentation.Library{ Name: "test", }, basicProc.MetricReader()))) From e201f52ccdb0261e5f442409fb59fd069ccdbd05 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 20 Aug 2021 10:26:47 -0700 Subject: [PATCH 07/33] lint applied: MetricReader->Reader --- bridge/opencensus/exporter.go | 10 +++--- bridge/opencensus/exporter_test.go | 2 +- exporters/otlp/otlpmetric/exporter.go | 4 +-- exporters/otlp/otlpmetric/exporter_test.go | 4 +-- .../internal/metrictransform/metric.go | 10 +++--- .../internal/otlpmetrictest/data.go | 20 +++++------ .../otlpmetric/otlpmetricgrpc/client_test.go | 6 ++-- .../otlpmetric/otlpmetrichttp/client_test.go | 2 +- exporters/prometheus/prometheus.go | 4 +-- exporters/stdout/stdoutmetric/metric.go | 4 +-- sdk/export/metric/metric.go | 18 +++++----- sdk/metric/controller/basic/controller.go | 16 ++++----- .../controller/basic/controller_test.go | 4 +-- sdk/metric/controller/controllertest/test.go | 4 +-- sdk/metric/processor/basic/basic.go | 14 ++++---- sdk/metric/processor/basic/basic_test.go | 14 ++++---- sdk/metric/processor/processortest/test.go | 36 +++++++++---------- .../processor/processortest/test_test.go | 4 +-- sdk/metric/processor/reducer/reducer_test.go | 4 +-- sdk/metric/stress_test.go | 2 +- 20 files changed, 91 insertions(+), 91 deletions(-) diff --git a/bridge/opencensus/exporter.go b/bridge/opencensus/exporter.go index 63a78ff5c89..172cb13c8c6 100644 --- a/bridge/opencensus/exporter.go +++ b/bridge/opencensus/exporter.go @@ -56,26 +56,26 @@ func (e *exporter) ExportMetrics(ctx context.Context, metrics []*metricdata.Metr if len(metrics) != 0 { res = convertResource(metrics[0].Resource) } - return e.base.Export(ctx, res, &censusLibraryMetricReader{metrics: metrics}) + return e.base.Export(ctx, res, &censusLibraryReader{metrics: metrics}) } -type censusLibraryMetricReader struct { +type censusLibraryReader struct { metrics []*metricdata.Metric } -func (r censusLibraryMetricReader) ForEach(readerFunc func(instrumentation.Library, export.MetricReader) error) error { +func (r censusLibraryReader) ForEach(readerFunc func(instrumentation.Library, export.Reader) error) error { return readerFunc(instrumentation.Library{ Name: "OpenCensus Bridge", }, &metricReader{metrics: r.metrics}) } type metricReader struct { - // RWMutex implements locking for the `MetricReader` interface. + // RWMutex implements locking for the `Reader` interface. sync.RWMutex metrics []*metricdata.Metric } -var _ export.MetricReader = &metricReader{} +var _ export.Reader = &metricReader{} // ForEach iterates through the CheckpointSet, passing an // export.Record with the appropriate aggregation to an exporter. diff --git a/bridge/opencensus/exporter_test.go b/bridge/opencensus/exporter_test.go index 3b44bb61812..930e9f99137 100644 --- a/bridge/opencensus/exporter_test.go +++ b/bridge/opencensus/exporter_test.go @@ -47,7 +47,7 @@ type fakeExporter struct { err error } -func (f *fakeExporter) Export(ctx context.Context, res *resource.Resource, ilmr exportmetric.InstrumentationLibraryMetricReader) error { +func (f *fakeExporter) Export(ctx context.Context, res *resource.Resource, ilmr exportmetric.InstrumentationLibraryReader) error { return controllertest.ReadAll(ilmr, export.StatelessExportKindSelector(), func(_ instrumentation.Library, record exportmetric.Record) error { f.resource = res diff --git a/exporters/otlp/otlpmetric/exporter.go b/exporters/otlp/otlpmetric/exporter.go index 3a9f09bc2cf..9fb83d5b27e 100644 --- a/exporters/otlp/otlpmetric/exporter.go +++ b/exporters/otlp/otlpmetric/exporter.go @@ -44,8 +44,8 @@ type Exporter struct { } // Export exports a batch of metrics. -func (e *Exporter) Export(ctx context.Context, res *resource.Resource, ilmr metricsdk.InstrumentationLibraryMetricReader) error { - rm, err := metrictransform.InstrumentationLibraryMetricReader(ctx, e, res, ilmr, 1) +func (e *Exporter) Export(ctx context.Context, res *resource.Resource, ilmr metricsdk.InstrumentationLibraryReader) error { + rm, err := metrictransform.InstrumentationLibraryReader(ctx, e, res, ilmr, 1) if err != nil { return err } diff --git a/exporters/otlp/otlpmetric/exporter_test.go b/exporters/otlp/otlpmetric/exporter_test.go index a9fe21d100c..4f60233131e 100644 --- a/exporters/otlp/otlpmetric/exporter_test.go +++ b/exporters/otlp/otlpmetric/exporter_test.go @@ -724,7 +724,7 @@ func runMetricExportTests(t *testing.T, opts []otlpmetric.Option, res *resource. } libraryRecs[lib] = append(libraryRecs[lib], metricsdk.NewRecord(&desc, &labs, ckpt.Aggregation(), intervalStart, intervalEnd)) } - assert.NoError(t, exp.Export(context.Background(), res, processortest.MultiInstrumentationLibraryMetricReader(libraryRecs))) + assert.NoError(t, exp.Export(context.Background(), res, processortest.MultiInstrumentationLibraryReader(libraryRecs))) // assert.ElementsMatch does not equate nested slices of different order, // therefore this requires the top level slice to be broken down. @@ -784,7 +784,7 @@ func TestEmptyMetricExport(t *testing.T) { }, } { driver.Reset() - require.NoError(t, exp.Export(context.Background(), resource.Empty(), processortest.MultiInstrumentationLibraryMetricReader(map[instrumentation.Library][]metricsdk.Record{ + require.NoError(t, exp.Export(context.Background(), resource.Empty(), processortest.MultiInstrumentationLibraryReader(map[instrumentation.Library][]metricsdk.Record{ { Name: testLibName, }: test.records, diff --git a/exporters/otlp/otlpmetric/internal/metrictransform/metric.go b/exporters/otlp/otlpmetric/internal/metrictransform/metric.go index ec8a5339bc6..4b5933fac94 100644 --- a/exporters/otlp/otlpmetric/internal/metrictransform/metric.go +++ b/exporters/otlp/otlpmetric/internal/metrictransform/metric.go @@ -70,12 +70,12 @@ func toNanos(t time.Time) uint64 { return uint64(t.UnixNano()) } -// InstrumentationLibraryMetricReader transforms all records contained in a checkpoint into +// InstrumentationLibraryReader transforms all records contained in a checkpoint into // batched OTLP ResourceMetrics. -func InstrumentationLibraryMetricReader(ctx context.Context, exportSelector export.ExportKindSelector, res *resource.Resource, ilmr export.InstrumentationLibraryMetricReader, numWorkers uint) (*metricpb.ResourceMetrics, error) { +func InstrumentationLibraryReader(ctx context.Context, exportSelector export.ExportKindSelector, res *resource.Resource, ilmr export.InstrumentationLibraryReader, numWorkers uint) (*metricpb.ResourceMetrics, error) { var ilms []*metricpb.InstrumentationLibraryMetrics - err := ilmr.ForEach(func(lib instrumentation.Library, mr export.MetricReader) error { + err := ilmr.ForEach(func(lib instrumentation.Library, mr export.Reader) error { records, errc := source(ctx, exportSelector, mr) @@ -132,9 +132,9 @@ func InstrumentationLibraryMetricReader(ctx context.Context, exportSelector expo } // source starts a goroutine that sends each one of the Records yielded by -// the MetricReader on the returned chan. Any error encoutered will be sent +// the Reader on the returned chan. Any error encoutered will be sent // on the returned error chan after seeding is complete. -func source(ctx context.Context, exportSelector export.ExportKindSelector, mr export.MetricReader) (<-chan export.Record, <-chan error) { +func source(ctx context.Context, exportSelector export.ExportKindSelector, mr export.Reader) (<-chan export.Record, <-chan error) { errc := make(chan error, 1) out := make(chan export.Record) // Seed records into process. diff --git a/exporters/otlp/otlpmetric/internal/otlpmetrictest/data.go b/exporters/otlp/otlpmetric/internal/otlpmetrictest/data.go index 07faccbde53..523211b0cef 100644 --- a/exporters/otlp/otlpmetric/internal/otlpmetrictest/data.go +++ b/exporters/otlp/otlpmetric/internal/otlpmetrictest/data.go @@ -29,10 +29,10 @@ import ( "go.opentelemetry.io/otel/sdk/metric/processor/processortest" ) -// OneRecordMetricReader is a MetricReader that returns just one +// OneRecordReader is a Reader that returns just one // filled record. It may be useful for testing driver's metrics // export. -func OneRecordMetricReader() exportmetric.InstrumentationLibraryMetricReader { +func OneRecordReader() exportmetric.InstrumentationLibraryReader { desc := metrictest.NewDescriptor( "foo", sdkapi.CounterInstrumentKind, @@ -47,7 +47,7 @@ func OneRecordMetricReader() exportmetric.InstrumentationLibraryMetricReader { labels := attribute.NewSet(attribute.String("abc", "def"), attribute.Int64("one", 1)) rec := exportmetric.NewRecord(&desc, &labels, agg[0].Aggregation(), start, end) - return processortest.MultiInstrumentationLibraryMetricReader( + return processortest.MultiInstrumentationLibraryReader( map[instrumentation.Library][]exportmetric.Record{ { Name: "onelib", @@ -55,17 +55,17 @@ func OneRecordMetricReader() exportmetric.InstrumentationLibraryMetricReader { }) } -func EmptyMetricReader() exportmetric.InstrumentationLibraryMetricReader { - return processortest.MultiInstrumentationLibraryMetricReader(nil) +func EmptyReader() exportmetric.InstrumentationLibraryReader { + return processortest.MultiInstrumentationLibraryReader(nil) } -// FailMetricReader is a checkpointer that returns an error during +// FailReader is a checkpointer that returns an error during // ForEach. -type FailMetricReader struct{} +type FailReader struct{} -var _ exportmetric.InstrumentationLibraryMetricReader = FailMetricReader{} +var _ exportmetric.InstrumentationLibraryReader = FailReader{} -// ForEach implements exportmetric.MetricReader. It always fails. -func (FailMetricReader) ForEach(readerFunc func(instrumentation.Library, exportmetric.MetricReader) error) error { +// ForEach implements exportmetric.Reader. It always fails. +func (FailReader) ForEach(readerFunc func(instrumentation.Library, exportmetric.Reader) error) error { return fmt.Errorf("fail") } diff --git a/exporters/otlp/otlpmetric/otlpmetricgrpc/client_test.go b/exporters/otlp/otlpmetric/otlpmetricgrpc/client_test.go index 6b197893599..541723ebe2d 100644 --- a/exporters/otlp/otlpmetric/otlpmetricgrpc/client_test.go +++ b/exporters/otlp/otlpmetric/otlpmetricgrpc/client_test.go @@ -40,7 +40,7 @@ import ( ) var ( - oneRecord = otlpmetrictest.OneRecordMetricReader() + oneRecord = otlpmetrictest.OneRecordReader() testResource = resource.Empty() ) @@ -719,7 +719,7 @@ func TestEmptyData(t *testing.T) { assert.NoError(t, exp.Shutdown(ctx)) }() - assert.NoError(t, exp.Export(ctx, testResource, otlpmetrictest.EmptyMetricReader())) + assert.NoError(t, exp.Export(ctx, testResource, otlpmetrictest.EmptyReader())) } func TestFailedMetricTransform(t *testing.T) { @@ -737,5 +737,5 @@ func TestFailedMetricTransform(t *testing.T) { assert.NoError(t, exp.Shutdown(ctx)) }() - assert.Error(t, exp.Export(ctx, testResource, otlpmetrictest.FailMetricReader{})) + assert.Error(t, exp.Export(ctx, testResource, otlpmetrictest.FailReader{})) } diff --git a/exporters/otlp/otlpmetric/otlpmetrichttp/client_test.go b/exporters/otlp/otlpmetric/otlpmetrichttp/client_test.go index 63e7688f8f1..fa5c2a79e31 100644 --- a/exporters/otlp/otlpmetric/otlpmetrichttp/client_test.go +++ b/exporters/otlp/otlpmetric/otlpmetrichttp/client_test.go @@ -38,7 +38,7 @@ const ( ) var ( - oneRecord = otlpmetrictest.OneRecordMetricReader() + oneRecord = otlpmetrictest.OneRecordReader() testResource = resource.Empty() ) diff --git a/exporters/prometheus/prometheus.go b/exporters/prometheus/prometheus.go index 6149bdfd2b6..d46b1f0ccf2 100644 --- a/exporters/prometheus/prometheus.go +++ b/exporters/prometheus/prometheus.go @@ -153,7 +153,7 @@ func (c *collector) Describe(ch chan<- *prometheus.Desc) { c.exp.lock.RLock() defer c.exp.lock.RUnlock() - _ = c.exp.Controller().Reader().ForEach(func(_ instrumentation.Library, reader export.MetricReader) error { + _ = c.exp.Controller().Reader().ForEach(func(_ instrumentation.Library, reader export.Reader) error { return reader.ForEach(c.exp, func(record export.Record) error { var labelKeys []string mergeLabels(record, c.exp.controller.Resource(), &labelKeys, nil) @@ -176,7 +176,7 @@ func (c *collector) Collect(ch chan<- prometheus.Metric) { otel.Handle(err) } - err := ctrl.Reader().ForEach(func(_ instrumentation.Library, reader export.MetricReader) error { + err := ctrl.Reader().ForEach(func(_ instrumentation.Library, reader export.Reader) error { return reader.ForEach(c.exp, func(record export.Record) error { agg := record.Aggregation() diff --git a/exporters/stdout/stdoutmetric/metric.go b/exporters/stdout/stdoutmetric/metric.go index c98391ea78f..7ad8495b0e0 100644 --- a/exporters/stdout/stdoutmetric/metric.go +++ b/exporters/stdout/stdoutmetric/metric.go @@ -51,10 +51,10 @@ func (e *metricExporter) ExportKindFor(desc *metric.Descriptor, kind aggregation return exportmetric.StatelessExportKindSelector().ExportKindFor(desc, kind) } -func (e *metricExporter) Export(_ context.Context, res *resource.Resource, reader exportmetric.InstrumentationLibraryMetricReader) error { +func (e *metricExporter) Export(_ context.Context, res *resource.Resource, reader exportmetric.InstrumentationLibraryReader) error { var aggError error var batch []line - aggError = reader.ForEach(func(lib instrumentation.Library, mr exportmetric.MetricReader) error { + aggError = reader.ForEach(func(lib instrumentation.Library, mr exportmetric.Reader) error { var instLabels []attribute.KeyValue if name := lib.Name; name != "" { diff --git a/sdk/export/metric/metric.go b/sdk/export/metric/metric.go index 3cd2c204ab9..2aaf1663ca4 100644 --- a/sdk/export/metric/metric.go +++ b/sdk/export/metric/metric.go @@ -112,13 +112,13 @@ type Checkpointer interface { // any time. Processor - // MetricReader returns the current data set. This may be + // Reader returns the current data set. This may be // called before and after collection. The // implementation is required to return the same value - // throughout its lifetime, since MetricReader exposes a + // throughout its lifetime, since Reader exposes a // sync.Locker interface. The caller is responsible for - // locking the MetricReader before initiating collection. - MetricReader() MetricReader + // locking the Reader before initiating collection. + Reader() Reader // StartCollection begins a collection interval. StartCollection() @@ -218,7 +218,7 @@ type Exporter interface { // // The CheckpointSet interface refers to the Processor that just // completed collection. - Export(ctx context.Context, resource *resource.Resource, reader InstrumentationLibraryMetricReader) error + Export(ctx context.Context, resource *resource.Resource, reader InstrumentationLibraryReader) error // ExportKindSelector is an interface used by the Processor // in deciding whether to compute Delta or Cumulative @@ -226,8 +226,8 @@ type Exporter interface { ExportKindSelector } -type InstrumentationLibraryMetricReader interface { - ForEach(readerFunc func(instrumentation.Library, MetricReader) error) error +type InstrumentationLibraryReader interface { + ForEach(readerFunc func(instrumentation.Library, Reader) error) error } // ExportKindSelector is a sub-interface of Exporter used to indicate @@ -240,11 +240,11 @@ type ExportKindSelector interface { ExportKindFor(descriptor *metric.Descriptor, aggregatorKind aggregation.Kind) ExportKind } -// MetricReader allows a controller to access a complete checkpoint of +// Reader allows a controller to access a complete checkpoint of // aggregated metrics from the Processor. This is passed to the // Exporter which may then use ForEach to iterate over the collection // of aggregated metrics. -type MetricReader interface { +type Reader interface { // ForEach iterates over aggregated checkpoints for all // metrics that were updated during the last collection // period. Each aggregated checkpoint returned by the diff --git a/sdk/metric/controller/basic/controller.go b/sdk/metric/controller/basic/controller.go index 73ea36ca4c7..6bd08a73de3 100644 --- a/sdk/metric/controller/basic/controller.go +++ b/sdk/metric/controller/basic/controller.go @@ -160,9 +160,9 @@ func (c *Controller) Resource() *resource.Resource { return c.resource } -// Reader returns an InstrumentationLibraryMetricReader for iterating +// Reader returns an InstrumentationLibraryReader for iterating // through the metrics of each registered library, one at a time. -func (c *Controller) Reader() export.InstrumentationLibraryMetricReader { +func (c *Controller) Reader() export.InstrumentationLibraryReader { return libraryReader{c} } @@ -259,7 +259,7 @@ func (c *Controller) checkpoint(ctx context.Context) error { } func (c *Controller) checkpoint1(ctx context.Context, ac *accumulatorCheckpointer) error { - ckpt := ac.checkpointer.MetricReader() + ckpt := ac.checkpointer.Reader() ckpt.Lock() defer ckpt.Unlock() @@ -293,7 +293,7 @@ func (c *Controller) checkpoint1(ctx context.Context, ac *accumulatorCheckpointe return err } -// export calls the exporter with a read lock on the MetricReader, +// export calls the exporter with a read lock on the Reader, // applying the configured export timeout. func (c *Controller) export(ctx context.Context) error { if c.pushTimeout > 0 { @@ -309,12 +309,12 @@ type libraryReader struct { *Controller } -var _ export.InstrumentationLibraryMetricReader = libraryReader{} +var _ export.InstrumentationLibraryReader = libraryReader{} -func (l libraryReader) ForEach(readerFunc func(l instrumentation.Library, r export.MetricReader) error) error { +func (l libraryReader) ForEach(readerFunc func(l instrumentation.Library, r export.Reader) error) error { for _, impl := range l.Controller.provider.List() { pair := impl.(*accumulatorCheckpointer) - reader := pair.checkpointer.MetricReader() + reader := pair.checkpointer.Reader() if err := func() error { reader.RLock() defer reader.RUnlock() @@ -327,7 +327,7 @@ func (l libraryReader) ForEach(readerFunc func(l instrumentation.Library, r expo } // IsRunning returns true if the controller was started via Start(), -// indicating that the current export.MetricReader is being kept +// indicating that the current export.Reader is being kept // up-to-date. func (c *Controller) IsRunning() bool { c.lock.Lock() diff --git a/sdk/metric/controller/basic/controller_test.go b/sdk/metric/controller/basic/controller_test.go index c18b8f7fed1..919b1927b95 100644 --- a/sdk/metric/controller/basic/controller_test.go +++ b/sdk/metric/controller/basic/controller_test.go @@ -42,7 +42,7 @@ func getMap(t *testing.T, cont *controller.Controller) map[string]float64 { out := processortest.NewOutput(attribute.DefaultEncoder()) require.NoError(t, cont.Reader().ForEach( - func(_ instrumentation.Library, reader export.MetricReader) error { + func(_ instrumentation.Library, reader export.Reader) error { return reader.ForEach( export.CumulativeExportKindSelector(), func(record export.Record) error { @@ -288,7 +288,7 @@ func newBlockingExporter() *blockingExporter { } } -func (b *blockingExporter) Export(ctx context.Context, res *resource.Resource, output export.InstrumentationLibraryMetricReader) error { +func (b *blockingExporter) Export(ctx context.Context, res *resource.Resource, output export.InstrumentationLibraryReader) error { var err error _ = b.exporter.Export(ctx, res, output) if b.calls == 0 { diff --git a/sdk/metric/controller/controllertest/test.go b/sdk/metric/controller/controllertest/test.go index f4293cb7f64..f7bfa6d8429 100644 --- a/sdk/metric/controller/controllertest/test.go +++ b/sdk/metric/controller/controllertest/test.go @@ -63,11 +63,11 @@ func (t MockTicker) C() <-chan time.Time { // metrics instead of a two-level iterator (instrumentation library, // metric). func ReadAll( - reader export.InstrumentationLibraryMetricReader, + reader export.InstrumentationLibraryReader, kind export.ExportKindSelector, apply func(instrumentation.Library, export.Record) error, ) error { - return reader.ForEach(func(library instrumentation.Library, ckpt export.MetricReader) error { + return reader.ForEach(func(library instrumentation.Library, ckpt export.Reader) error { return ckpt.ForEach(kind, func(record export.Record) error { return apply(library, record) }) diff --git a/sdk/metric/processor/basic/basic.go b/sdk/metric/processor/basic/basic.go index 5de936ad930..42ad472a640 100644 --- a/sdk/metric/processor/basic/basic.go +++ b/sdk/metric/processor/basic/basic.go @@ -113,7 +113,7 @@ type ( var _ export.Processor = &Processor{} var _ export.Checkpointer = &Processor{} -var _ export.MetricReader = &state{} +var _ export.Reader = &state{} // ErrInconsistentState is returned when the sequence of collection's starts and finishes are incorrectly balanced. var ErrInconsistentState = fmt.Errorf("inconsistent processor state") @@ -264,11 +264,11 @@ func (b *Processor) Process(accum export.Accumulation) error { return value.current.Merge(agg, desc) } -// MetricReader returns the associated MetricReader. Use the -// MetricReader Locker interface to synchronize access to this -// object. The MetricReader.ForEach() method cannot be called +// Reader returns the associated Reader. Use the +// Reader Locker interface to synchronize access to this +// object. The Reader.ForEach() method cannot be called // concurrently with Process(). -func (b *Processor) MetricReader() export.MetricReader { +func (b *Processor) Reader() export.Reader { return &b.state } @@ -283,7 +283,7 @@ func (b *Processor) StartCollection() { // FinishCollection signals to the Processor that a complete // collection has finished and that ForEach will be called to access -// the MetricReader. +// the Reader. func (b *Processor) FinishCollection() error { b.intervalEnd = time.Now() if b.startedCollection != b.finishedCollection+1 { @@ -337,7 +337,7 @@ func (b *Processor) FinishCollection() error { return nil } -// ForEach iterates through the MetricReader, passing an +// ForEach iterates through the Reader, passing an // export.Record with the appropriate Cumulative or Delta aggregation // to an exporter. func (b *state) ForEach(exporter export.ExportKindSelector, f func(export.Record) error) error { diff --git a/sdk/metric/processor/basic/basic_test.go b/sdk/metric/processor/basic/basic_test.go index 8804415026a..5bf2e2a2b33 100644 --- a/sdk/metric/processor/basic/basic_test.go +++ b/sdk/metric/processor/basic/basic_test.go @@ -176,7 +176,7 @@ func testProcessor( continue } - metricReader := processor.MetricReader() + metricReader := processor.Reader() for _, repetitionAfterEmptyInterval := range []bool{false, true} { if repetitionAfterEmptyInterval { @@ -269,7 +269,7 @@ func (bogusExporter) ExportKindFor(*metric.Descriptor, aggregation.Kind) export. return 1000000 } -func (bogusExporter) Export(context.Context, export.MetricReader) error { +func (bogusExporter) Export(context.Context, export.Reader) error { panic("Not called") } @@ -376,7 +376,7 @@ func TestStatefulNoMemoryCumulative(t *testing.T) { selector := processorTest.AggregatorSelector() processor := basic.New(selector, ekindSel, basic.WithMemory(false)) - metricReader := processor.MetricReader() + metricReader := processor.Reader() for i := 1; i < 3; i++ { // Empty interval @@ -409,7 +409,7 @@ func TestStatefulNoMemoryDelta(t *testing.T) { selector := processorTest.AggregatorSelector() processor := basic.New(selector, ekindSel, basic.WithMemory(false)) - metricReader := processor.MetricReader() + metricReader := processor.Reader() for i := 1; i < 3; i++ { // Empty interval @@ -445,7 +445,7 @@ func TestMultiObserverSum(t *testing.T) { selector := processorTest.AggregatorSelector() processor := basic.New(selector, ekindSel, basic.WithMemory(false)) - metricReader := processor.MetricReader() + metricReader := processor.Reader() for i := 1; i < 3; i++ { // Add i*10*3 times @@ -488,7 +488,7 @@ func TestSumObserverEndToEnd(t *testing.T) { result.Observe(calls) }, ) - data := proc.MetricReader() + data := proc.Reader() var startTime [3]time.Time var endTime [3]time.Time @@ -500,7 +500,7 @@ func TestSumObserverEndToEnd(t *testing.T) { require.NoError(t, proc.FinishCollection()) exporter := processortest.New(eselector, attribute.DefaultEncoder()) - require.NoError(t, exporter.Export(ctx, resource.Empty(), processortest.OneInstrumentationLibraryMetricReader( + require.NoError(t, exporter.Export(ctx, resource.Empty(), processortest.OneInstrumentationLibraryReader( instrumentation.Library{ Name: "test", }, data))) diff --git a/sdk/metric/processor/processortest/test.go b/sdk/metric/processor/processortest/test.go index fb2688ca9a1..05fc7fb9b6b 100644 --- a/sdk/metric/processor/processortest/test.go +++ b/sdk/metric/processor/processortest/test.go @@ -46,14 +46,14 @@ type ( } // mapValue is value stored in a processor used to produce a - // MetricReader. + // Reader. mapValue struct { labels *attribute.Set resource *resource.Resource aggregator export.Aggregator } - // Output implements export.MetricReader. + // Output implements export.Reader. Output struct { m map[mapKey]mapValue labelEncoder attribute.Encoder @@ -168,8 +168,8 @@ func (c *testCheckpointer) FinishCollection() error { return nil } -// MetricReader implements export.Checkpointer. -func (c *testCheckpointer) MetricReader() export.MetricReader { +// Reader implements export.Checkpointer. +func (c *testCheckpointer) Reader() export.Reader { return c.Processor.output } @@ -229,7 +229,7 @@ func NewOutput(labelEncoder attribute.Encoder) *Output { } } -// ForEach implements export.MetricReader. +// ForEach implements export.Reader. func (o *Output) ForEach(_ export.ExportKindSelector, ff func(export.Record) error) error { for key, value := range o.m { if err := ff(export.NewRecord( @@ -351,11 +351,11 @@ func New(selector export.ExportKindSelector, encoder attribute.Encoder) *Exporte } } -func (e *Exporter) Export(_ context.Context, res *resource.Resource, ckpt export.InstrumentationLibraryMetricReader) error { +func (e *Exporter) Export(_ context.Context, res *resource.Resource, ckpt export.InstrumentationLibraryReader) error { e.output.Lock() defer e.output.Unlock() e.exportCount++ - return ckpt.ForEach(func(library instrumentation.Library, mr export.MetricReader) error { + return ckpt.ForEach(func(library instrumentation.Library, mr export.Reader) error { return mr.ForEach(e.ExportKindSelector, func(r export.Record) error { if e.InjectErr != nil { if err := e.InjectErr(r); err != nil { @@ -394,30 +394,30 @@ func (e *Exporter) Reset() { e.exportCount = 0 } -func OneInstrumentationLibraryMetricReader(l instrumentation.Library, r export.MetricReader) export.InstrumentationLibraryMetricReader { - return oneLibraryMetricReader{l, r} +func OneInstrumentationLibraryReader(l instrumentation.Library, r export.Reader) export.InstrumentationLibraryReader { + return oneLibraryReader{l, r} } -type oneLibraryMetricReader struct { +type oneLibraryReader struct { library instrumentation.Library - reader export.MetricReader + reader export.Reader } -func (o oneLibraryMetricReader) ForEach(readerFunc func(instrumentation.Library, export.MetricReader) error) error { +func (o oneLibraryReader) ForEach(readerFunc func(instrumentation.Library, export.Reader) error) error { return readerFunc(o.library, o.reader) } -func MultiInstrumentationLibraryMetricReader(records map[instrumentation.Library][]export.Record) export.InstrumentationLibraryMetricReader { - return instrumentationLibraryMetricReader{records: records} +func MultiInstrumentationLibraryReader(records map[instrumentation.Library][]export.Record) export.InstrumentationLibraryReader { + return instrumentationLibraryReader{records: records} } -type instrumentationLibraryMetricReader struct { +type instrumentationLibraryReader struct { records map[instrumentation.Library][]export.Record } -var _ export.InstrumentationLibraryMetricReader = instrumentationLibraryMetricReader{} +var _ export.InstrumentationLibraryReader = instrumentationLibraryReader{} -func (m instrumentationLibraryMetricReader) ForEach(fn func(instrumentation.Library, export.MetricReader) error) error { +func (m instrumentationLibraryReader) ForEach(fn func(instrumentation.Library, export.Reader) error) error { for library, records := range m.records { if err := fn(library, &metricReader{records: records}); err != nil { return err @@ -431,7 +431,7 @@ type metricReader struct { records []export.Record } -var _ export.MetricReader = &metricReader{} +var _ export.Reader = &metricReader{} func (m *metricReader) ForEach(_ export.ExportKindSelector, fn func(export.Record) error) error { for _, record := range m.records { diff --git a/sdk/metric/processor/processortest/test_test.go b/sdk/metric/processor/processortest/test_test.go index aee7a1370a6..defce19ade8 100644 --- a/sdk/metric/processor/processortest/test_test.go +++ b/sdk/metric/processor/processortest/test_test.go @@ -75,11 +75,11 @@ func TestProcessorTesting(t *testing.T) { attribute.DefaultEncoder(), ) - err := exporter.Export(context.Background(), res, processortest.OneInstrumentationLibraryMetricReader( + err := exporter.Export(context.Background(), res, processortest.OneInstrumentationLibraryReader( instrumentation.Library{ Name: "test", }, - checkpointer.MetricReader(), + checkpointer.Reader(), )) require.NoError(t, err) require.EqualValues(t, expect, exporter.Values()) diff --git a/sdk/metric/processor/reducer/reducer_test.go b/sdk/metric/processor/reducer/reducer_test.go index 407a5a79754..21c03ec52b1 100644 --- a/sdk/metric/processor/reducer/reducer_test.go +++ b/sdk/metric/processor/reducer/reducer_test.go @@ -105,9 +105,9 @@ func TestFilterBasicProcessor(t *testing.T) { } res := resource.NewSchemaless(attribute.String("R", "V")) - require.NoError(t, exporter.Export(context.Background(), res, processortest.OneInstrumentationLibraryMetricReader(instrumentation.Library{ + require.NoError(t, exporter.Export(context.Background(), res, processortest.OneInstrumentationLibraryReader(instrumentation.Library{ Name: "test", - }, basicProc.MetricReader()))) + }, basicProc.Reader()))) require.EqualValues(t, map[string]float64{ "counter.sum/A=1,C=3/R=V": 200, diff --git a/sdk/metric/stress_test.go b/sdk/metric/stress_test.go index b2d078363ce..a9c8fec6cad 100644 --- a/sdk/metric/stress_test.go +++ b/sdk/metric/stress_test.go @@ -246,7 +246,7 @@ func (f *testFixture) preCollect() { f.dupCheck = map[testKey]int{} } -func (*testFixture) MetricReader() export.MetricReader { +func (*testFixture) Reader() export.Reader { return nil } From fdbe7c3614e3a2413b99308dc578657d869aec54 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 20 Aug 2021 11:19:56 -0700 Subject: [PATCH 08/33] comments --- sdk/export/metric/metric.go | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/sdk/export/metric/metric.go b/sdk/export/metric/metric.go index 2aaf1663ca4..d685aa67bb2 100644 --- a/sdk/export/metric/metric.go +++ b/sdk/export/metric/metric.go @@ -216,8 +216,8 @@ type Exporter interface { // The Context comes from the controller that initiated // collection. // - // The CheckpointSet interface refers to the Processor that just - // completed collection. + // The InstrumentationLibraryReader interface refers to the + // Processor that just completed collection. Export(ctx context.Context, resource *resource.Resource, reader InstrumentationLibraryReader) error // ExportKindSelector is an interface used by the Processor @@ -226,10 +226,6 @@ type Exporter interface { ExportKindSelector } -type InstrumentationLibraryReader interface { - ForEach(readerFunc func(instrumentation.Library, Reader) error) error -} - // ExportKindSelector is a sub-interface of Exporter used to indicate // whether the Processor should compute Delta or Cumulative // Aggregations. @@ -240,10 +236,19 @@ type ExportKindSelector interface { ExportKindFor(descriptor *metric.Descriptor, aggregatorKind aggregation.Kind) ExportKind } +// InstrumentationLibraryReader is an interface for exporters to iterate +// over one instrumentation library of metric data at a time. +type InstrumentationLibraryReader interface { + // ForEach calls the passed function once per instrumentation library, + // allowing the caller to emit metrics grouped by the library that + // produced them. + ForEach(readerFunc func(instrumentation.Library, Reader) error) error +} + // Reader allows a controller to access a complete checkpoint of -// aggregated metrics from the Processor. This is passed to the -// Exporter which may then use ForEach to iterate over the collection -// of aggregated metrics. +// aggregated metrics from the Processor for a single library of +// metric data. This is passed to the Exporter which may then use +// ForEach to iterate over the collection of aggregated metrics. type Reader interface { // ForEach iterates over aggregated checkpoints for all // metrics that were updated during the last collection From cb9e4310dc52bb8ac47585c4ae46e25c956a6b16 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 20 Aug 2021 12:26:51 -0700 Subject: [PATCH 09/33] Changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 492e0227e5e..e0e9187990f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Metric SDK/API implementation type `InstrumentKind` moves into `sdkapi` sub-package. (#2091) - The Metrics SDK export record no longer contains a Resource pointer, the SDK `"go.opentelemetry.io/otel/sdk/trace/export/metric".Exporter.Export()` function for push-based exporters now takes a single Resource argument, pull-based exporters use `"go.opentelemetry.io/otel/sdk/metric/controller/basic".Controller.Resource()`. (#2120) +- The Metric SDK `Export()` function takes a new two-level reader interface for iterating over results one instrumentation library at a time. + - The former `"go.opentelemetry.io/otel/sdk/export/metric".CheckpointSet` is renamed `Reader`. + - The new interface is named `"go.opentelemetry.io/otel/sdk/export/metric".InstrumentationLibraryReader`. (#2197) ### Deprecated From e591fa597819b4dfe4ff3594caa52258e196ff50 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Wed, 8 Sep 2021 11:37:00 -0700 Subject: [PATCH 10/33] Apply suggestions from code review Co-authored-by: alrex --- bridge/opencensus/exporter_test.go | 4 ++-- exporters/otlp/otlpmetric/exporter.go | 4 ++-- sdk/metric/processor/basic/basic_test.go | 24 ++++++++++++------------ 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/bridge/opencensus/exporter_test.go b/bridge/opencensus/exporter_test.go index d184e914296..ee5d7607930 100644 --- a/bridge/opencensus/exporter_test.go +++ b/bridge/opencensus/exporter_test.go @@ -47,8 +47,8 @@ type fakeExporter struct { err error } -func (f *fakeExporter) Export(ctx context.Context, res *resource.Resource, ilmr exportmetric.InstrumentationLibraryReader) error { - return controllertest.ReadAll(ilmr, export.StatelessExportKindSelector(), +func (f *fakeExporter) Export(ctx context.Context, res *resource.Resource, ilr exportmetric.InstrumentationLibraryReader) error { + return controllertest.ReadAll(ilr, export.StatelessExportKindSelector(), func(_ instrumentation.Library, record exportmetric.Record) error { f.resource = res f.records = append(f.records, record) diff --git a/exporters/otlp/otlpmetric/exporter.go b/exporters/otlp/otlpmetric/exporter.go index 9fb83d5b27e..241715f3041 100644 --- a/exporters/otlp/otlpmetric/exporter.go +++ b/exporters/otlp/otlpmetric/exporter.go @@ -44,8 +44,8 @@ type Exporter struct { } // Export exports a batch of metrics. -func (e *Exporter) Export(ctx context.Context, res *resource.Resource, ilmr metricsdk.InstrumentationLibraryReader) error { - rm, err := metrictransform.InstrumentationLibraryReader(ctx, e, res, ilmr, 1) +func (e *Exporter) Export(ctx context.Context, res *resource.Resource, ilr metricsdk.InstrumentationLibraryReader) error { + rm, err := metrictransform.InstrumentationLibraryReader(ctx, e, res, ilr, 1) if err != nil { return err } diff --git a/sdk/metric/processor/basic/basic_test.go b/sdk/metric/processor/basic/basic_test.go index f3483cecae7..d57b7eca814 100644 --- a/sdk/metric/processor/basic/basic_test.go +++ b/sdk/metric/processor/basic/basic_test.go @@ -176,7 +176,7 @@ func testProcessor( continue } - metricReader := processor.Reader() + reader := processor.Reader() for _, repetitionAfterEmptyInterval := range []bool{false, true} { if repetitionAfterEmptyInterval { @@ -190,7 +190,7 @@ func testProcessor( // Test the final checkpoint state. records1 := processorTest.NewOutput(attribute.DefaultEncoder()) - err = metricReader.ForEach(export.ConstantExportKindSelector(ekind), records1.AddRecord) + err = reader.ForEach(export.ConstantExportKindSelector(ekind), records1.AddRecord) // Test for an allowed error: if err != nil && err != aggregation.ErrNoSubtraction { @@ -376,7 +376,7 @@ func TestStatefulNoMemoryCumulative(t *testing.T) { selector := processorTest.AggregatorSelector() processor := basic.New(selector, ekindSel, basic.WithMemory(false)) - metricReader := processor.Reader() + reader := processor.Reader() for i := 1; i < 3; i++ { // Empty interval @@ -385,7 +385,7 @@ func TestStatefulNoMemoryCumulative(t *testing.T) { // Verify zero elements records := processorTest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, metricReader.ForEach(ekindSel, records.AddRecord)) + require.NoError(t, reader.ForEach(ekindSel, records.AddRecord)) require.EqualValues(t, map[string]float64{}, records.Map()) // Add 10 @@ -395,7 +395,7 @@ func TestStatefulNoMemoryCumulative(t *testing.T) { // Verify one element records = processorTest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, metricReader.ForEach(ekindSel, records.AddRecord)) + require.NoError(t, reader.ForEach(ekindSel, records.AddRecord)) require.EqualValues(t, map[string]float64{ "inst.sum/A=B/": float64(i * 10), }, records.Map()) @@ -409,7 +409,7 @@ func TestStatefulNoMemoryDelta(t *testing.T) { selector := processorTest.AggregatorSelector() processor := basic.New(selector, ekindSel, basic.WithMemory(false)) - metricReader := processor.Reader() + reader := processor.Reader() for i := 1; i < 3; i++ { // Empty interval @@ -418,7 +418,7 @@ func TestStatefulNoMemoryDelta(t *testing.T) { // Verify zero elements records := processorTest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, metricReader.ForEach(ekindSel, records.AddRecord)) + require.NoError(t, reader.ForEach(ekindSel, records.AddRecord)) require.EqualValues(t, map[string]float64{}, records.Map()) // Add 10 @@ -428,7 +428,7 @@ func TestStatefulNoMemoryDelta(t *testing.T) { // Verify one element records = processorTest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, metricReader.ForEach(ekindSel, records.AddRecord)) + require.NoError(t, reader.ForEach(ekindSel, records.AddRecord)) require.EqualValues(t, map[string]float64{ "inst.sum/A=B/": 10, }, records.Map()) @@ -445,7 +445,7 @@ func TestMultiObserverSum(t *testing.T) { selector := processorTest.AggregatorSelector() processor := basic.New(selector, ekindSel, basic.WithMemory(false)) - metricReader := processor.Reader() + reader := processor.Reader() for i := 1; i < 3; i++ { // Add i*10*3 times @@ -463,7 +463,7 @@ func TestMultiObserverSum(t *testing.T) { // Verify one element records := processorTest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, metricReader.ForEach(ekindSel, records.AddRecord)) + require.NoError(t, reader.ForEach(ekindSel, records.AddRecord)) require.EqualValues(t, map[string]float64{ "observe.sum/A=B/": float64(3 * 10 * multiplier), }, records.Map()) @@ -488,7 +488,7 @@ func TestCounterObserverEndToEnd(t *testing.T) { result.Observe(calls) }, ) - data := proc.Reader() + reader := proc.Reader() var startTime [3]time.Time var endTime [3]time.Time @@ -503,7 +503,7 @@ func TestCounterObserverEndToEnd(t *testing.T) { require.NoError(t, exporter.Export(ctx, resource.Empty(), processortest.OneInstrumentationLibraryReader( instrumentation.Library{ Name: "test", - }, data))) + }, reader))) require.EqualValues(t, map[string]float64{ "observer.sum//": float64(i + 1), From 65b8f3e0ea91fa7fdbdce8caf2311e89f0dc40e1 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Wed, 8 Sep 2021 14:35:05 -0700 Subject: [PATCH 11/33] remove an interdependency --- exporters/otlp/otlpmetric/exporter_test.go | 8 ++++---- sdk/resource/resource.go | 3 --- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/exporters/otlp/otlpmetric/exporter_test.go b/exporters/otlp/otlpmetric/exporter_test.go index 0c12afd2fae..f329627d0c4 100644 --- a/exporters/otlp/otlpmetric/exporter_test.go +++ b/exporters/otlp/otlpmetric/exporter_test.go @@ -169,7 +169,7 @@ func TestNoGroupingExport(t *testing.T) { runMetricExportTests( t, nil, - nil, + resource.Empty(), []testRecord{ record( "int64-count", @@ -270,7 +270,7 @@ func TestHistogramMetricGroupingExport(t *testing.T) { }, }, } - runMetricExportTests(t, nil, nil, []testRecord{r, r}, expected) + runMetricExportTests(t, nil, resource.Empty(), []testRecord{r, r}, expected) } func TestCountInt64MetricGroupingExport(t *testing.T) { @@ -284,7 +284,7 @@ func TestCountInt64MetricGroupingExport(t *testing.T) { runMetricExportTests( t, nil, - nil, + resource.Empty(), []testRecord{r, r}, []*metricpb.ResourceMetrics{ { @@ -334,7 +334,7 @@ func TestCountFloat64MetricGroupingExport(t *testing.T) { runMetricExportTests( t, nil, - nil, + resource.Empty(), []testRecord{r, r}, []*metricpb.ResourceMetrics{ { diff --git a/sdk/resource/resource.go b/sdk/resource/resource.go index 6bb2a2805aa..2ebf5fae4b6 100644 --- a/sdk/resource/resource.go +++ b/sdk/resource/resource.go @@ -132,9 +132,6 @@ func (r *Resource) Attributes() []attribute.KeyValue { } func (r *Resource) SchemaURL() string { - if r == nil { - return "" - } return r.schemaURL } From 7871e03987934feba4a184c7f0759abd2b016c72 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Wed, 8 Sep 2021 14:47:30 -0700 Subject: [PATCH 12/33] fix build --- sdk/metric/processor/basic/basic_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/sdk/metric/processor/basic/basic_test.go b/sdk/metric/processor/basic/basic_test.go index d57b7eca814..dec7dbbeba5 100644 --- a/sdk/metric/processor/basic/basic_test.go +++ b/sdk/metric/processor/basic/basic_test.go @@ -494,6 +494,7 @@ func TestCounterObserverEndToEnd(t *testing.T) { var endTime [3]time.Time for i := range startTime { + data := proc.Reader() data.Lock() proc.StartCollection() accum.Collect(ctx) From 7f71842275c4a0bd54f1d6bd1656da1d34fdd8af Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Tue, 14 Sep 2021 15:38:50 -0700 Subject: [PATCH 13/33] re-indent one --- sdk/metric/controller/basic/pull_test.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/sdk/metric/controller/basic/pull_test.go b/sdk/metric/controller/basic/pull_test.go index 7647db21cf1..2a2b7d18f79 100644 --- a/sdk/metric/controller/basic/pull_test.go +++ b/sdk/metric/controller/basic/pull_test.go @@ -51,11 +51,7 @@ func TestPullNoCollect(t *testing.T) { require.NoError(t, puller.Collect(ctx)) records := processortest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, controllertest.ReadAll( - puller.Reader(), - export.CumulativeExportKindSelector(), - records.AddInstrumentationLibraryRecord, - )) + require.NoError(t, controllertest.ReadAll(puller.Reader(), export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) require.EqualValues(t, map[string]float64{ "counter.sum/A=B/": 10, From c18e5e72b9d5a01794a38635c7c1b2bd15326ade Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Wed, 15 Sep 2021 12:16:37 -0700 Subject: [PATCH 14/33] Apply suggestions from code review Co-authored-by: Tyler Yahn --- CHANGELOG.md | 4 ++-- bridge/opencensus/go.mod | 2 +- metric/registry/registry.go | 6 +++--- sdk/metric/controller/basic/controller.go | 4 ++-- sdk/metric/controller/controllertest/test.go | 4 ++-- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b241943af8..d27d8847480 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,9 +51,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The Metrics SDK export record no longer contains a Resource pointer, the SDK `"go.opentelemetry.io/otel/sdk/trace/export/metric".Exporter.Export()` function for push-based exporters now takes a single Resource argument, pull-based exporters use `"go.opentelemetry.io/otel/sdk/metric/controller/basic".Controller.Resource()`. (#2120) - The JSON output of the `go.opentelemetry.io/otel/exporters/stdout/stdouttrace` is harmonized now such that the output is "plain" JSON objects after each other of the form `{ ... } { ... } { ... }`. Earlier the JSON objects describing a span were wrapped in a slice for each `Exporter.ExportSpans` call, like `[ { ... } ][ { ... } { ... } ]`. Outputting JSON object directly after each other is consistent with JSON loggers, and a bit easier to parse and read. (#2196) - Update the `NewTracerConfig`, `NewSpanStartConfig`, `NewSpanEndConfig`, and `NewEventConfig` function in the `go.opentelemetry.io/otel/trace` package to return their respective configurations as structs instead of pointers to the struct. (#2212) -- The Metric SDK `Export()` function takes a new two-level reader interface for iterating over results one instrumentation library at a time. +- The Metric SDK `Export()` function takes a new two-level reader interface for iterating over results one instrumentation library at a time. (#2197) - The former `"go.opentelemetry.io/otel/sdk/export/metric".CheckpointSet` is renamed `Reader`. - - The new interface is named `"go.opentelemetry.io/otel/sdk/export/metric".InstrumentationLibraryReader`. (#2197) + - The new interface is named `"go.opentelemetry.io/otel/sdk/export/metric".InstrumentationLibraryReader`. ### Deprecated diff --git a/bridge/opencensus/go.mod b/bridge/opencensus/go.mod index 0e2c60316dc..6866aff6ee8 100644 --- a/bridge/opencensus/go.mod +++ b/bridge/opencensus/go.mod @@ -8,7 +8,7 @@ require ( go.opentelemetry.io/otel/metric v0.23.0 go.opentelemetry.io/otel/sdk v1.0.0-RC3 go.opentelemetry.io/otel/sdk/export/metric v0.23.0 - go.opentelemetry.io/otel/sdk/metric v0.0.0-00010101000000-000000000000 + go.opentelemetry.io/otel/sdk/metric v0.23.0 go.opentelemetry.io/otel/trace v1.0.0-RC3 ) diff --git a/metric/registry/registry.go b/metric/registry/registry.go index f8e504aaa6f..c48a35798f1 100644 --- a/metric/registry/registry.go +++ b/metric/registry/registry.go @@ -63,9 +63,9 @@ func (p *MeterProvider) Meter(instrumentationName string, opts ...metric.MeterOp defer p.lock.Unlock() m, ok := p.meters[k] if !ok { - m = metric.WrapMeterImpl( - NewMeterImpl( - p.impl.Meter(instrumentationName, opts...).MeterImpl())) + m = metric.WrapMeterImpl(NewMeterImpl( + p.impl.Meter(instrumentationName, opts...).MeterImpl(), + )) p.meters[k] = m } return m diff --git a/sdk/metric/controller/basic/controller.go b/sdk/metric/controller/basic/controller.go index 6bd08a73de3..94b4d5e1fe2 100644 --- a/sdk/metric/controller/basic/controller.go +++ b/sdk/metric/controller/basic/controller.go @@ -160,9 +160,9 @@ func (c *Controller) Resource() *resource.Resource { return c.resource } -// Reader returns an InstrumentationLibraryReader for iterating +// InstrumentationLibraryReader returns an InstrumentationLibraryReader for iterating // through the metrics of each registered library, one at a time. -func (c *Controller) Reader() export.InstrumentationLibraryReader { +func (c *Controller) InstrumentationLibraryReader() export.InstrumentationLibraryReader { return libraryReader{c} } diff --git a/sdk/metric/controller/controllertest/test.go b/sdk/metric/controller/controllertest/test.go index f7bfa6d8429..d4b8d3a3299 100644 --- a/sdk/metric/controller/controllertest/test.go +++ b/sdk/metric/controller/controllertest/test.go @@ -67,8 +67,8 @@ func ReadAll( kind export.ExportKindSelector, apply func(instrumentation.Library, export.Record) error, ) error { - return reader.ForEach(func(library instrumentation.Library, ckpt export.Reader) error { - return ckpt.ForEach(kind, func(record export.Record) error { + return reader.ForEach(func(library instrumentation.Library, reader export.Reader) error { + return reader.ForEach(kind, func(record export.Record) error { return apply(library, record) }) }) From 89b1fd15687bef0124703823a59c148051536713 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Wed, 15 Sep 2021 12:33:47 -0700 Subject: [PATCH 15/33] Lint&feedback --- sdk/metric/controller/basic/controller.go | 25 ++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/sdk/metric/controller/basic/controller.go b/sdk/metric/controller/basic/controller.go index 6bd08a73de3..17341b5e320 100644 --- a/sdk/metric/controller/basic/controller.go +++ b/sdk/metric/controller/basic/controller.go @@ -251,14 +251,22 @@ func (c *Controller) collect(ctx context.Context) error { // when Stop() is called. func (c *Controller) checkpoint(ctx context.Context) error { for _, impl := range c.provider.List() { - if err := c.checkpoint1(ctx, impl.(*accumulatorCheckpointer)); err != nil { + acPair, ok := impl.(*accumulatorCheckpointer) + if !ok { + return fmt.Errorf("impossible type assertion failed: %T", impl) + } + if err := c.checkpointSingleAccmulator(ctx, acPair); err != nil { return err } } return nil } -func (c *Controller) checkpoint1(ctx context.Context, ac *accumulatorCheckpointer) error { +// checkpointSingleAccmulator checkpoints a single instrumentation +// library's accumulator, which involves calling +// checkpointer.StartCollection, accumulator.Collect, and +// checkpointer.FinishCollection in sequence. +func (c *Controller) checkpointSingleAccmulator(ctx context.Context, ac *accumulatorCheckpointer) error { ckpt := ac.checkpointer.Reader() ckpt.Lock() defer ckpt.Unlock() @@ -313,12 +321,19 @@ var _ export.InstrumentationLibraryReader = libraryReader{} func (l libraryReader) ForEach(readerFunc func(l instrumentation.Library, r export.Reader) error) error { for _, impl := range l.Controller.provider.List() { - pair := impl.(*accumulatorCheckpointer) - reader := pair.checkpointer.Reader() + // Note: the Controller owns the provider, which is a registry + // that calls (accumulatorProvider).Meter() to obtain this value, + // so the following type assertion will succeed or else there is a + // bug in the registry. + acPair, ok := impl.(*accumulatorCheckpointer) + if !ok { + return fmt.Errorf("impossible type assertion failed: %T", impl) + } + reader := acPair.checkpointer.Reader() if err := func() error { reader.RLock() defer reader.RUnlock() - return readerFunc(pair.library, reader) + return readerFunc(acPair.library, reader) }(); err != nil { return err } From 5032f6341cc64954e2f10c3edcda5e1a72a27b81 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Wed, 15 Sep 2021 13:48:42 -0700 Subject: [PATCH 16/33] update after rename --- exporters/prometheus/prometheus.go | 4 ++-- sdk/metric/controller/basic/controller.go | 2 +- sdk/metric/controller/basic/controller_test.go | 2 +- sdk/metric/controller/basic/pull_test.go | 10 +++++----- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/exporters/prometheus/prometheus.go b/exporters/prometheus/prometheus.go index d46b1f0ccf2..257e1772c72 100644 --- a/exporters/prometheus/prometheus.go +++ b/exporters/prometheus/prometheus.go @@ -153,7 +153,7 @@ func (c *collector) Describe(ch chan<- *prometheus.Desc) { c.exp.lock.RLock() defer c.exp.lock.RUnlock() - _ = c.exp.Controller().Reader().ForEach(func(_ instrumentation.Library, reader export.Reader) error { + _ = c.exp.Controller().InstrumentationLibraryReader().ForEach(func(_ instrumentation.Library, reader export.Reader) error { return reader.ForEach(c.exp, func(record export.Record) error { var labelKeys []string mergeLabels(record, c.exp.controller.Resource(), &labelKeys, nil) @@ -176,7 +176,7 @@ func (c *collector) Collect(ch chan<- prometheus.Metric) { otel.Handle(err) } - err := ctrl.Reader().ForEach(func(_ instrumentation.Library, reader export.Reader) error { + err := ctrl.InstrumentationLibraryReader().ForEach(func(_ instrumentation.Library, reader export.Reader) error { return reader.ForEach(c.exp, func(record export.Record) error { agg := record.Aggregation() diff --git a/sdk/metric/controller/basic/controller.go b/sdk/metric/controller/basic/controller.go index bb761bec91a..de04356bd2c 100644 --- a/sdk/metric/controller/basic/controller.go +++ b/sdk/metric/controller/basic/controller.go @@ -310,7 +310,7 @@ func (c *Controller) export(ctx context.Context) error { defer cancel() } - return c.exporter.Export(ctx, c.resource, c.Reader()) + return c.exporter.Export(ctx, c.resource, c.InstrumentationLibraryReader()) } type libraryReader struct { diff --git a/sdk/metric/controller/basic/controller_test.go b/sdk/metric/controller/basic/controller_test.go index f2c60966fb4..8d8fd055a05 100644 --- a/sdk/metric/controller/basic/controller_test.go +++ b/sdk/metric/controller/basic/controller_test.go @@ -41,7 +41,7 @@ const envVar = "OTEL_RESOURCE_ATTRIBUTES" func getMap(t *testing.T, cont *controller.Controller) map[string]float64 { out := processortest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, cont.Reader().ForEach( + require.NoError(t, cont.InstrumentationLibraryReader().ForEach( func(_ instrumentation.Library, reader export.Reader) error { return reader.ForEach( export.CumulativeExportKindSelector(), diff --git a/sdk/metric/controller/basic/pull_test.go b/sdk/metric/controller/basic/pull_test.go index 2a2b7d18f79..c8d11cb2379 100644 --- a/sdk/metric/controller/basic/pull_test.go +++ b/sdk/metric/controller/basic/pull_test.go @@ -51,7 +51,7 @@ func TestPullNoCollect(t *testing.T) { require.NoError(t, puller.Collect(ctx)) records := processortest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, controllertest.ReadAll(puller.Reader(), export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) + require.NoError(t, controllertest.ReadAll(puller.InstrumentationLibraryReader(), export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) require.EqualValues(t, map[string]float64{ "counter.sum/A=B/": 10, @@ -61,7 +61,7 @@ func TestPullNoCollect(t *testing.T) { require.NoError(t, puller.Collect(ctx)) records = processortest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, controllertest.ReadAll(puller.Reader(), export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) + require.NoError(t, controllertest.ReadAll(puller.InstrumentationLibraryReader(), export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) require.EqualValues(t, map[string]float64{ "counter.sum/A=B/": 20, @@ -89,7 +89,7 @@ func TestPullWithCollect(t *testing.T) { require.NoError(t, puller.Collect(ctx)) records := processortest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, controllertest.ReadAll(puller.Reader(), export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) + require.NoError(t, controllertest.ReadAll(puller.InstrumentationLibraryReader(), export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) require.EqualValues(t, map[string]float64{ "counter.sum/A=B/": 10, @@ -100,7 +100,7 @@ func TestPullWithCollect(t *testing.T) { // Cached value! require.NoError(t, puller.Collect(ctx)) records = processortest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, controllertest.ReadAll(puller.Reader(), export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) + require.NoError(t, controllertest.ReadAll(puller.InstrumentationLibraryReader(), export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) require.EqualValues(t, map[string]float64{ "counter.sum/A=B/": 10, @@ -112,7 +112,7 @@ func TestPullWithCollect(t *testing.T) { // Re-computed value! require.NoError(t, puller.Collect(ctx)) records = processortest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, controllertest.ReadAll(puller.Reader(), export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) + require.NoError(t, controllertest.ReadAll(puller.InstrumentationLibraryReader(), export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) require.EqualValues(t, map[string]float64{ "counter.sum/A=B/": 20, From 472b1132d972415c0188f64e81a2f537a2385a50 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Thu, 16 Sep 2021 12:36:11 -0700 Subject: [PATCH 17/33] comment fix --- metric/registry/registry.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metric/registry/registry.go b/metric/registry/registry.go index c48a35798f1..f6e68137e70 100644 --- a/metric/registry/registry.go +++ b/metric/registry/registry.go @@ -86,7 +86,7 @@ func (p *MeterProvider) List() []metric.MeterImpl { } // uniqueInstrumentMeterImpl implements the metric.MeterImpl interface, adding -// uniqueness checking for instrument descriptors. Use NewMeter +// uniqueness checking for instrument descriptors. Use NewMeterImpl // to wrap an implementation with uniqueness checking. type uniqueInstrumentMeterImpl struct { lock sync.Mutex From 52b6ca95ac3eaf0fd12853f4556109859aa2ed9d Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 17 Sep 2021 04:11:28 -0700 Subject: [PATCH 18/33] style fix for meter options --- metric/config.go | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/metric/config.go b/metric/config.go index 6f20cbfb102..4f856e5677b 100644 --- a/metric/config.go +++ b/metric/config.go @@ -103,24 +103,22 @@ func NewMeterConfig(opts ...MeterOption) MeterConfig { return config } -// WithInstrumentationVersion sets the instrumentation version. -func WithInstrumentationVersion(version string) MeterOption { - return instrumentationVersionOption(version) -} +type meterOptionFunc func(*MeterConfig) -type instrumentationVersionOption string - -func (i instrumentationVersionOption) applyMeter(config *MeterConfig) { - config.instrumentationVersion = string(i) +func (fn meterOptionFunc) applyMeter(cfg *MeterConfig) { + fn(cfg) } -// WithSchemaURL sets the schema URL. -func WithSchemaURL(version string) MeterOption { - return schemaURLOption(version) +// WithInstrumentationVersion sets the instrumentation version. +func WithInstrumentationVersion(version string) MeterOption { + return meterOptionFunc(func(config *MeterConfig) { + config.instrumentationVersion = version + }) } -type schemaURLOption string - -func (s schemaURLOption) applyMeter(config *MeterConfig) { - config.schemaURL = string(s) +// WithSchemaURL sets the schema URL. +func WithSchemaURL(schemaURL string) MeterOption { + return meterOptionFunc(func(config *MeterConfig) { + config.schemaURL = schemaURL + }) } From 2780b454c88bb75611333de273fe85bad697ff6b Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 17 Sep 2021 04:30:13 -0700 Subject: [PATCH 19/33] remove libraryReader, let Controller implement the reader API directly --- sdk/metric/controller/basic/controller.go | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/sdk/metric/controller/basic/controller.go b/sdk/metric/controller/basic/controller.go index de04356bd2c..fc49c9e6011 100644 --- a/sdk/metric/controller/basic/controller.go +++ b/sdk/metric/controller/basic/controller.go @@ -77,6 +77,8 @@ type Controller struct { collectedTime time.Time } +var _ export.InstrumentationLibraryReader = &Controller{} + type accumulatorProvider struct { controller *Controller } @@ -163,7 +165,7 @@ func (c *Controller) Resource() *resource.Resource { // InstrumentationLibraryReader returns an InstrumentationLibraryReader for iterating // through the metrics of each registered library, one at a time. func (c *Controller) InstrumentationLibraryReader() export.InstrumentationLibraryReader { - return libraryReader{c} + return c } // Start begins a ticker that periodically collects and exports @@ -313,14 +315,9 @@ func (c *Controller) export(ctx context.Context) error { return c.exporter.Export(ctx, c.resource, c.InstrumentationLibraryReader()) } -type libraryReader struct { - *Controller -} - -var _ export.InstrumentationLibraryReader = libraryReader{} - -func (l libraryReader) ForEach(readerFunc func(l instrumentation.Library, r export.Reader) error) error { - for _, impl := range l.Controller.provider.List() { +// ForEach implements export.InstrumentationLibraryReader. +func (c *Controller) ForEach(readerFunc func(l instrumentation.Library, r export.Reader) error) error { + for _, impl := range c.provider.List() { // Note: the Controller owns the provider, which is a registry // that calls (accumulatorProvider).Meter() to obtain this value, // so the following type assertion will succeed or else there is a From 3b7c3ca5f8096fe86105a8ec49c5b0715831526c Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 17 Sep 2021 05:05:31 -0700 Subject: [PATCH 20/33] rename 'impl' field to 'provider' --- metric/registry/registry.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/metric/registry/registry.go b/metric/registry/registry.go index f6e68137e70..653a7b41106 100644 --- a/metric/registry/registry.go +++ b/metric/registry/registry.go @@ -23,10 +23,12 @@ import ( "go.opentelemetry.io/otel/metric" ) -// MeterProvider is a standard MeterProvider for wrapping `MeterImpl` +// MeterProvider is a standard MeterProvider that adds uniqueness +// checking of Meters (by InstrumentationLibrary Name/Version/Schema) type MeterProvider struct { + provider metric.MeterProvider + lock sync.Mutex - impl metric.MeterProvider meters map[uniqueMeterKey]metric.Meter } @@ -49,10 +51,10 @@ var _ metric.MeterProvider = (*MeterProvider)(nil) // NewMeterProvider returns a new provider that implements meter // name-uniqueness checking. -func NewMeterProvider(impl metric.MeterProvider) *MeterProvider { +func NewMeterProvider(provider metric.MeterProvider) *MeterProvider { return &MeterProvider{ - impl: impl, - meters: map[uniqueMeterKey]metric.Meter{}, + provider: provider, + meters: map[uniqueMeterKey]metric.Meter{}, } } @@ -64,7 +66,7 @@ func (p *MeterProvider) Meter(instrumentationName string, opts ...metric.MeterOp m, ok := p.meters[k] if !ok { m = metric.WrapMeterImpl(NewMeterImpl( - p.impl.Meter(instrumentationName, opts...).MeterImpl(), + p.provider.Meter(instrumentationName, opts...).MeterImpl(), )) p.meters[k] = m } From 23d05853e31157981bd98f7920f331a711e465b1 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 17 Sep 2021 06:45:39 -0700 Subject: [PATCH 21/33] remove a type assertion --- metric/registry/registry.go | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/metric/registry/registry.go b/metric/registry/registry.go index 653a7b41106..3fa0b95b0ec 100644 --- a/metric/registry/registry.go +++ b/metric/registry/registry.go @@ -26,10 +26,13 @@ import ( // MeterProvider is a standard MeterProvider that adds uniqueness // checking of Meters (by InstrumentationLibrary Name/Version/Schema) type MeterProvider struct { + // Note: This is a convenience for SDKs which are required to + // implemented the functionality provided here, + provider metric.MeterProvider lock sync.Mutex - meters map[uniqueMeterKey]metric.Meter + meters map[uniqueMeterKey]*uniqueInstrumentMeterImpl } type uniqueMeterKey struct { @@ -54,7 +57,7 @@ var _ metric.MeterProvider = (*MeterProvider)(nil) func NewMeterProvider(provider metric.MeterProvider) *MeterProvider { return &MeterProvider{ provider: provider, - meters: map[uniqueMeterKey]metric.Meter{}, + meters: map[uniqueMeterKey]*uniqueInstrumentMeterImpl{}, } } @@ -65,13 +68,12 @@ func (p *MeterProvider) Meter(instrumentationName string, opts ...metric.MeterOp defer p.lock.Unlock() m, ok := p.meters[k] if !ok { - m = metric.WrapMeterImpl(NewMeterImpl( + m = newMeterImpl( p.provider.Meter(instrumentationName, opts...).MeterImpl(), - )) + ) p.meters[k] = m } - return m - + return metric.WrapMeterImpl(m) } // List provides a list of MeterImpl objects created through this @@ -82,14 +84,14 @@ func (p *MeterProvider) List() []metric.MeterImpl { var r []metric.MeterImpl for _, meter := range p.meters { - r = append(r, meter.MeterImpl().(*uniqueInstrumentMeterImpl).impl) + r = append(r, meter.impl) } return r } // uniqueInstrumentMeterImpl implements the metric.MeterImpl interface, adding -// uniqueness checking for instrument descriptors. Use NewMeterImpl -// to wrap an implementation with uniqueness checking. +// uniqueness checking for instrument descriptors. Use NewMeterProvider +// to gain access to a Meter implementation with uniqueness checking. type uniqueInstrumentMeterImpl struct { lock sync.Mutex impl metric.MeterImpl @@ -104,8 +106,16 @@ var ErrMetricKindMismatch = fmt.Errorf( "a metric was already registered by this name with another kind or number type") // NewMeterImpl returns a wrapped metric.MeterImpl with -// the addition of uniqueness checking. +// the addition of instrument name uniqueness checking. func NewMeterImpl(impl metric.MeterImpl) metric.MeterImpl { + // Note: the internal/metric/global package uses a NewMeterImpl + // directly, instead of relying on the *registry.MeterProvider. + // This allows it to synchronize its calls to set the delegates + // at the moment the SDK is registered. + return newMeterImpl(impl) +} + +func newMeterImpl(impl metric.MeterImpl) *uniqueInstrumentMeterImpl { return &uniqueInstrumentMeterImpl{ impl: impl, state: map[string]metric.InstrumentImpl{}, From a927758e1d6317a3d099f141da468721dff3510e Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 17 Sep 2021 08:24:30 -0700 Subject: [PATCH 22/33] move metric/registry into internal; move registry.MeterProvider into metric controller --- internal/metric/global/meter.go | 9 +- internal/metric/global/registry_test.go | 2 +- internal/metric/go.sum | 1 + {metric => internal/metric}/registry/doc.go | 2 +- .../metric}/registry/registry.go | 105 +++------------- .../metric}/registry/registry_test.go | 28 ++--- metric/metrictest/meter.go | 2 + sdk/metric/controller/basic/controller.go | 113 ++++++++++-------- 8 files changed, 103 insertions(+), 159 deletions(-) rename {metric => internal/metric}/registry/doc.go (92%) rename {metric => internal/metric}/registry/registry.go (55%) rename {metric => internal/metric}/registry/registry_test.go (86%) diff --git a/internal/metric/global/meter.go b/internal/metric/global/meter.go index e30eb2f1c56..f8f0992b7ef 100644 --- a/internal/metric/global/meter.go +++ b/internal/metric/global/meter.go @@ -21,9 +21,9 @@ import ( "unsafe" "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/internal/metric/registry" "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/metric/number" - "go.opentelemetry.io/otel/metric/registry" ) // This file contains the forwarding implementation of MeterProvider used as @@ -162,7 +162,12 @@ func (p *meterProvider) Meter(instrumentationName string, opts ...metric.MeterOp entry, ok := p.meters[key] if !ok { entry = &meterEntry{} - entry.unique = registry.NewMeterImpl(&entry.impl) + // Note: This code implements its own MeterProvider + // name-uniqueness logic because there is + // synchronization required at the moment of + // delegation. We use the same instrument-uniqueness + // checking the real SDK uses here: + entry.unique = registry.NewUniqueInstrumentMeterImpl(&entry.impl) p.meters[key] = entry } return metric.WrapMeterImpl(entry.unique) diff --git a/internal/metric/global/registry_test.go b/internal/metric/global/registry_test.go index ec0fe88cc8b..2eac26f716f 100644 --- a/internal/metric/global/registry_test.go +++ b/internal/metric/global/registry_test.go @@ -21,9 +21,9 @@ import ( "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/internal/metric/registry" "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/metric/metrictest" - "go.opentelemetry.io/otel/metric/registry" ) type ( diff --git a/internal/metric/go.sum b/internal/metric/go.sum index f212493d586..5f0b342ef32 100644 --- a/internal/metric/go.sum +++ b/internal/metric/go.sum @@ -7,6 +7,7 @@ github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZN github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY= github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +golang.org/x/sys v0.0.0-20210423185535-09eb48e85fd7/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= diff --git a/metric/registry/doc.go b/internal/metric/registry/doc.go similarity index 92% rename from metric/registry/doc.go rename to internal/metric/registry/doc.go index a53ba455455..2f17562f0eb 100644 --- a/metric/registry/doc.go +++ b/internal/metric/registry/doc.go @@ -21,4 +21,4 @@ This package is currently in a pre-GA phase. Backwards incompatible changes may be introduced in subsequent minor version releases as we work to track the evolving OpenTelemetry specification and user feedback. */ -package registry // import "go.opentelemetry.io/otel/metric/registry" +package registry // import "go.opentelemetry.io/otel/internal/metric/registry" diff --git a/metric/registry/registry.go b/internal/metric/registry/registry.go similarity index 55% rename from metric/registry/registry.go rename to internal/metric/registry/registry.go index 3fa0b95b0ec..5eafa540d31 100644 --- a/metric/registry/registry.go +++ b/internal/metric/registry/registry.go @@ -23,107 +23,38 @@ import ( "go.opentelemetry.io/otel/metric" ) -// MeterProvider is a standard MeterProvider that adds uniqueness -// checking of Meters (by InstrumentationLibrary Name/Version/Schema) -type MeterProvider struct { - // Note: This is a convenience for SDKs which are required to - // implemented the functionality provided here, - - provider metric.MeterProvider - - lock sync.Mutex - meters map[uniqueMeterKey]*uniqueInstrumentMeterImpl -} - -type uniqueMeterKey struct { - name string - version string - schemaURL string -} - -func keyOf(name string, opts ...metric.MeterOption) uniqueMeterKey { - cfg := metric.NewMeterConfig(opts...) - return uniqueMeterKey{ - name: name, - version: cfg.InstrumentationVersion(), - schemaURL: cfg.SchemaURL(), - } -} - -var _ metric.MeterProvider = (*MeterProvider)(nil) - -// NewMeterProvider returns a new provider that implements meter -// name-uniqueness checking. -func NewMeterProvider(provider metric.MeterProvider) *MeterProvider { - return &MeterProvider{ - provider: provider, - meters: map[uniqueMeterKey]*uniqueInstrumentMeterImpl{}, - } -} - -// Meter implements MeterProvider. -func (p *MeterProvider) Meter(instrumentationName string, opts ...metric.MeterOption) metric.Meter { - k := keyOf(instrumentationName, opts...) - p.lock.Lock() - defer p.lock.Unlock() - m, ok := p.meters[k] - if !ok { - m = newMeterImpl( - p.provider.Meter(instrumentationName, opts...).MeterImpl(), - ) - p.meters[k] = m - } - return metric.WrapMeterImpl(m) -} - -// List provides a list of MeterImpl objects created through this -// provider. -func (p *MeterProvider) List() []metric.MeterImpl { - p.lock.Lock() - defer p.lock.Unlock() - - var r []metric.MeterImpl - for _, meter := range p.meters { - r = append(r, meter.impl) - } - return r -} - -// uniqueInstrumentMeterImpl implements the metric.MeterImpl interface, adding -// uniqueness checking for instrument descriptors. Use NewMeterProvider -// to gain access to a Meter implementation with uniqueness checking. -type uniqueInstrumentMeterImpl struct { +// UniqueInstrumentMeterImpl implements the metric.MeterImpl interface, adding +// uniqueness checking for instrument descriptors. +type UniqueInstrumentMeterImpl struct { lock sync.Mutex impl metric.MeterImpl state map[string]metric.InstrumentImpl } -var _ metric.MeterImpl = (*uniqueInstrumentMeterImpl)(nil) +var _ metric.MeterImpl = (*UniqueInstrumentMeterImpl)(nil) // ErrMetricKindMismatch is the standard error for mismatched metric // instrument definitions. var ErrMetricKindMismatch = fmt.Errorf( "a metric was already registered by this name with another kind or number type") -// NewMeterImpl returns a wrapped metric.MeterImpl with -// the addition of instrument name uniqueness checking. -func NewMeterImpl(impl metric.MeterImpl) metric.MeterImpl { - // Note: the internal/metric/global package uses a NewMeterImpl - // directly, instead of relying on the *registry.MeterProvider. - // This allows it to synchronize its calls to set the delegates - // at the moment the SDK is registered. - return newMeterImpl(impl) -} - -func newMeterImpl(impl metric.MeterImpl) *uniqueInstrumentMeterImpl { - return &uniqueInstrumentMeterImpl{ +// NewUniqueInstrumentMeterImpl returns a wrapped metric.MeterImpl +// with the addition of instrument name uniqueness checking. +func NewUniqueInstrumentMeterImpl(impl metric.MeterImpl) *UniqueInstrumentMeterImpl { + return &UniqueInstrumentMeterImpl{ impl: impl, state: map[string]metric.InstrumentImpl{}, } } +// MeterImpl gives the caller access to the underlying MeterImpl +// used by this UniqueInstrumentMeterImpl. +func (u *UniqueInstrumentMeterImpl) MeterImpl() metric.MeterImpl { + return u.impl +} + // RecordBatch implements metric.MeterImpl. -func (u *uniqueInstrumentMeterImpl) RecordBatch(ctx context.Context, labels []attribute.KeyValue, ms ...metric.Measurement) { +func (u *UniqueInstrumentMeterImpl) RecordBatch(ctx context.Context, labels []attribute.KeyValue, ms ...metric.Measurement) { u.impl.RecordBatch(ctx, labels, ms...) } @@ -149,7 +80,7 @@ func Compatible(candidate, existing metric.Descriptor) bool { // `descriptor` argument. If there is an existing compatible // registration, this returns the already-registered instrument. If // there is no conflict and no prior registration, returns (nil, nil). -func (u *uniqueInstrumentMeterImpl) checkUniqueness(descriptor metric.Descriptor) (metric.InstrumentImpl, error) { +func (u *UniqueInstrumentMeterImpl) checkUniqueness(descriptor metric.Descriptor) (metric.InstrumentImpl, error) { impl, ok := u.state[descriptor.Name()] if !ok { return nil, nil @@ -163,7 +94,7 @@ func (u *uniqueInstrumentMeterImpl) checkUniqueness(descriptor metric.Descriptor } // NewSyncInstrument implements metric.MeterImpl. -func (u *uniqueInstrumentMeterImpl) NewSyncInstrument(descriptor metric.Descriptor) (metric.SyncImpl, error) { +func (u *UniqueInstrumentMeterImpl) NewSyncInstrument(descriptor metric.Descriptor) (metric.SyncImpl, error) { u.lock.Lock() defer u.lock.Unlock() @@ -184,7 +115,7 @@ func (u *uniqueInstrumentMeterImpl) NewSyncInstrument(descriptor metric.Descript } // NewAsyncInstrument implements metric.MeterImpl. -func (u *uniqueInstrumentMeterImpl) NewAsyncInstrument( +func (u *UniqueInstrumentMeterImpl) NewAsyncInstrument( descriptor metric.Descriptor, runner metric.AsyncRunner, ) (metric.AsyncImpl, error) { diff --git a/metric/registry/registry_test.go b/internal/metric/registry/registry_test.go similarity index 86% rename from metric/registry/registry_test.go rename to internal/metric/registry/registry_test.go index bdd00411774..69781ae03e8 100644 --- a/metric/registry/registry_test.go +++ b/internal/metric/registry/registry_test.go @@ -21,9 +21,9 @@ import ( "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/internal/metric/registry" "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/metric/metrictest" - "go.opentelemetry.io/otel/metric/registry" ) type ( @@ -70,11 +70,17 @@ func unwrap(impl interface{}, err error) (metric.InstrumentImpl, error) { return nil, err } +func testMeterWithRegistry(name string) metric.Meter { + return metric.WrapMeterImpl( + registry.NewUniqueInstrumentMeterImpl( + metrictest.NewMeterProvider().Meter(name).MeterImpl(), + ), + ) +} + func TestRegistrySameInstruments(t *testing.T) { for _, nf := range allNew { - provider := registry.NewMeterProvider(metrictest.NewMeterProvider()) - - meter := provider.Meter("meter") + meter := testMeterWithRegistry("meter") inst1, err1 := nf(meter, "this") inst2, err2 := nf(meter, "this") @@ -101,8 +107,7 @@ func TestRegistryDifferentNamespace(t *testing.T) { func TestRegistryDiffInstruments(t *testing.T) { for origName, origf := range allNew { - provider := registry.NewMeterProvider(metrictest.NewMeterProvider()) - meter := provider.Meter("meter") + meter := testMeterWithRegistry("meter") _, err := origf(meter, "this") require.NoError(t, err) @@ -119,14 +124,3 @@ func TestRegistryDiffInstruments(t *testing.T) { } } } - -func TestMeterProvider(t *testing.T) { - provider := metrictest.NewMeterProvider() - p := registry.NewMeterProvider(provider) - m1 := p.Meter("m1") - m1p := p.Meter("m1") - m2 := p.Meter("m2") - - require.Equal(t, m1, m1p) - require.NotEqual(t, m1, m2) -} diff --git a/metric/metrictest/meter.go b/metric/metrictest/meter.go index 54c1c59454c..84f730428e2 100644 --- a/metric/metrictest/meter.go +++ b/metric/metrictest/meter.go @@ -32,6 +32,8 @@ type ( Labels []attribute.KeyValue } + // Library is the same as "sdk/instrumentation".Library but there is + // a package cycle to use it. Library struct { InstrumentationName string InstrumentationVersion string diff --git a/sdk/metric/controller/basic/controller.go b/sdk/metric/controller/basic/controller.go index fc49c9e6011..3c00fbadeb5 100644 --- a/sdk/metric/controller/basic/controller.go +++ b/sdk/metric/controller/basic/controller.go @@ -21,8 +21,8 @@ import ( "time" "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/internal/metric/registry" "go.opentelemetry.io/otel/metric" - "go.opentelemetry.io/otel/metric/registry" export "go.opentelemetry.io/otel/sdk/export/metric" "go.opentelemetry.io/otel/sdk/instrumentation" sdk "go.opentelemetry.io/otel/sdk/metric" @@ -57,9 +57,8 @@ var ErrControllerStarted = fmt.Errorf("controller already started") // be blocked by a pull request in the basic controller. type Controller struct { lock sync.Mutex - provider *registry.MeterProvider + libraries map[instrumentation.Library]*registry.UniqueInstrumentMeterImpl checkpointerFactory export.CheckpointerFactory - accumulatorProvider accumulatorProvider resource *resource.Resource exporter export.Exporter @@ -78,26 +77,31 @@ type Controller struct { } var _ export.InstrumentationLibraryReader = &Controller{} +var _ metric.MeterProvider = &Controller{} -type accumulatorProvider struct { - controller *Controller -} - -var _ metric.MeterProvider = &accumulatorProvider{} - -func (a *accumulatorProvider) Meter(instrumentationName string, opts ...metric.MeterOption) metric.Meter { - checkpointer := a.controller.checkpointerFactory.NewCheckpointer() - accumulator := sdk.NewAccumulator(checkpointer) +func (c *Controller) Meter(instrumentationName string, opts ...metric.MeterOption) metric.Meter { cfg := metric.NewMeterConfig(opts...) - return metric.WrapMeterImpl(&accumulatorCheckpointer{ - Accumulator: accumulator, - checkpointer: checkpointer, - library: instrumentation.Library{ - Name: instrumentationName, - Version: cfg.InstrumentationVersion(), - SchemaURL: cfg.SchemaURL(), - }, - }) + library := instrumentation.Library{ + Name: instrumentationName, + Version: cfg.InstrumentationVersion(), + SchemaURL: cfg.SchemaURL(), + } + + c.lock.Lock() + defer c.lock.Unlock() + m, ok := c.libraries[library] + if !ok { + checkpointer := c.checkpointerFactory.NewCheckpointer() + accumulator := sdk.NewAccumulator(checkpointer) + m = registry.NewUniqueInstrumentMeterImpl(&accumulatorCheckpointer{ + Accumulator: accumulator, + checkpointer: checkpointer, + library: library, + }) + + c.libraries[library] = m + } + return metric.WrapMeterImpl(m) } type accumulatorCheckpointer struct { @@ -127,7 +131,8 @@ func New(checkpointerFactory export.CheckpointerFactory, opts ...Option) *Contro otel.Handle(err) } } - cont := &Controller{ + return &Controller{ + libraries: map[instrumentation.Library]*registry.UniqueInstrumentMeterImpl{}, checkpointerFactory: checkpointerFactory, exporter: c.Exporter, resource: c.Resource, @@ -138,9 +143,6 @@ func New(checkpointerFactory export.CheckpointerFactory, opts ...Option) *Contro collectTimeout: c.CollectTimeout, pushTimeout: c.PushTimeout, } - cont.accumulatorProvider.controller = cont - cont.provider = registry.NewMeterProvider(&cont.accumulatorProvider) - return cont } // SetClock supports setting a mock clock for testing. This must be @@ -153,7 +155,7 @@ func (c *Controller) SetClock(clock controllerTime.Clock) { // MeterProvider returns a MeterProvider instance for this controller. func (c *Controller) MeterProvider() metric.MeterProvider { - return c.provider + return c } // Resource returns the *resource.Resource associated with this @@ -202,19 +204,23 @@ func (c *Controller) Start(ctx context.Context) error { // // Note that Stop() will not cancel an ongoing collection or export. func (c *Controller) Stop(ctx context.Context) error { - c.lock.Lock() - defer c.lock.Unlock() + if lastCollection := func() bool { + c.lock.Lock() + defer c.lock.Unlock() + + if c.stopCh == nil { + return false + } - if c.stopCh == nil { + close(c.stopCh) + c.stopCh = nil + c.wg.Wait() + c.ticker.Stop() + c.ticker = nil + return true + }(); !lastCollection { return nil } - - close(c.stopCh) - c.stopCh = nil - c.wg.Wait() - c.ticker.Stop() - c.ticker = nil - return c.collect(ctx) } @@ -247,17 +253,29 @@ func (c *Controller) collect(ctx context.Context) error { return c.export(ctx) } +// accumulatorList returns a snapshot of current accumulators +// registered to this controller. This briefly locks the controller. +func (c *Controller) accumulatorList() []*accumulatorCheckpointer { + c.lock.Lock() + defer c.lock.Unlock() + + var r []*accumulatorCheckpointer + for _, entry := range c.libraries { + acc, ok := entry.MeterImpl().(*accumulatorCheckpointer) + if ok { + r = append(r, acc) + } + } + return r +} + // checkpoint calls the Accumulator and Checkpointer interfaces to // compute the CheckpointSet. This applies the configured collection // timeout. Note that this does not try to cancel a Collect or Export // when Stop() is called. func (c *Controller) checkpoint(ctx context.Context) error { - for _, impl := range c.provider.List() { - acPair, ok := impl.(*accumulatorCheckpointer) - if !ok { - return fmt.Errorf("impossible type assertion failed: %T", impl) - } - if err := c.checkpointSingleAccmulator(ctx, acPair); err != nil { + for _, impl := range c.accumulatorList() { + if err := c.checkpointSingleAccmulator(ctx, impl); err != nil { return err } } @@ -317,16 +335,9 @@ func (c *Controller) export(ctx context.Context) error { // ForEach implements export.InstrumentationLibraryReader. func (c *Controller) ForEach(readerFunc func(l instrumentation.Library, r export.Reader) error) error { - for _, impl := range c.provider.List() { - // Note: the Controller owns the provider, which is a registry - // that calls (accumulatorProvider).Meter() to obtain this value, - // so the following type assertion will succeed or else there is a - // bug in the registry. - acPair, ok := impl.(*accumulatorCheckpointer) - if !ok { - return fmt.Errorf("impossible type assertion failed: %T", impl) - } + for _, acPair := range c.accumulatorList() { reader := acPair.checkpointer.Reader() + // TODO: We should not fail fast; instead accumulate errors. if err := func() error { reader.RLock() defer reader.RUnlock() From f1b51da4b6ab3a0ac443bbc947068d5a0d7580df Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 17 Sep 2021 08:41:37 -0700 Subject: [PATCH 23/33] add test for controller registry function --- .../controller/basic/controller_test.go | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/sdk/metric/controller/basic/controller_test.go b/sdk/metric/controller/basic/controller_test.go index 8d8fd055a05..29e25922f32 100644 --- a/sdk/metric/controller/basic/controller_test.go +++ b/sdk/metric/controller/basic/controller_test.go @@ -441,3 +441,46 @@ func TestCollectAfterStopThenStartAgain(t *testing.T) { "one.lastvalue//": 6, }, exp.Values()) } + +func TestRegistryFunction(t *testing.T) { + exp := processortest.New( + export.CumulativeExportKindSelector(), + attribute.DefaultEncoder(), + ) + cont := controller.New( + processor.NewFactory( + processortest.AggregatorSelector(), + exp, + ), + controller.WithCollectPeriod(time.Second), + controller.WithExporter(exp), + controller.WithResource(resource.Empty()), + ) + + m1 := cont.MeterProvider().Meter("test") + m2 := cont.MeterProvider().Meter("test") + + require.NotNil(t, m1) + require.Equal(t, m1, m2) + + c1, err := m1.NewInt64Counter("counter.sum") + require.NoError(t, err) + + c2, err := m1.NewInt64Counter("counter.sum") + require.NoError(t, err) + + require.Equal(t, c1, c2) + + ctx := context.Background() + + require.NoError(t, cont.Start(ctx)) + + c1.Add(ctx, 10) + c2.Add(ctx, 10) + + require.NoError(t, cont.Stop(ctx)) + + require.EqualValues(t, map[string]float64{ + "counter.sum//": 20, + }, exp.Values()) +} From d09e2e2f400c0be0e4c4d4bffbff32ba0e558dad Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 17 Sep 2021 08:56:41 -0700 Subject: [PATCH 24/33] CheckpointSet->Reader everywhere --- bridge/opencensus/exporter.go | 4 ++-- .../otlpmetric/internal/metrictransform/metric.go | 6 +++--- exporters/prometheus/prometheus.go | 2 +- sdk/metric/controller/basic/controller.go | 14 +++++++------- sdk/metric/doc.go | 10 +++++----- sdk/metric/processor/basic/basic.go | 2 +- sdk/metric/processor/basic/config.go | 2 +- 7 files changed, 20 insertions(+), 20 deletions(-) diff --git a/bridge/opencensus/exporter.go b/bridge/opencensus/exporter.go index 3314bfc0536..d96e793211c 100644 --- a/bridge/opencensus/exporter.go +++ b/bridge/opencensus/exporter.go @@ -77,8 +77,8 @@ type metricReader struct { var _ export.Reader = &metricReader{} -// ForEach iterates through the CheckpointSet, passing an -// export.Record with the appropriate aggregation to an exporter. +// ForEach iterates through the metrics data, synthesizing an +// export.Record with the appropriate aggregation for the exporter. func (d *metricReader) ForEach(exporter export.ExportKindSelector, f func(export.Record) error) error { for _, m := range d.metrics { descriptor, err := convertDescriptor(m.Descriptor) diff --git a/exporters/otlp/otlpmetric/internal/metrictransform/metric.go b/exporters/otlp/otlpmetric/internal/metrictransform/metric.go index 4b5933fac94..b794b6c3df3 100644 --- a/exporters/otlp/otlpmetric/internal/metrictransform/metric.go +++ b/exporters/otlp/otlpmetric/internal/metrictransform/metric.go @@ -52,7 +52,7 @@ var ( // transformation. ErrContextCanceled = errors.New("context canceled") - // ErrTransforming is returned when an unexected error is encoutered transforming. + // ErrTransforming is returned when an unexected error is encountered transforming. ErrTransforming = errors.New("transforming failed") ) @@ -132,7 +132,7 @@ func InstrumentationLibraryReader(ctx context.Context, exportSelector export.Exp } // source starts a goroutine that sends each one of the Records yielded by -// the Reader on the returned chan. Any error encoutered will be sent +// the Reader on the returned chan. Any error encountered will be sent // on the returned error chan after seeding is complete. func source(ctx context.Context, exportSelector export.ExportKindSelector, mr export.Reader) (<-chan export.Record, <-chan error) { errc := make(chan error, 1) @@ -176,7 +176,7 @@ func transformer(ctx context.Context, exportSelector export.ExportKindSelector, // sink collects transformed Records and batches them. // -// Any errors encoutered transforming input will be reported with an +// Any errors encountered transforming input will be reported with an // ErrTransforming as well as the completed ResourceMetrics. It is up to the // caller to handle any incorrect data in these ResourceMetric. func sink(ctx context.Context, in <-chan result) ([]*metricpb.Metric, error) { diff --git a/exporters/prometheus/prometheus.go b/exporters/prometheus/prometheus.go index 257e1772c72..aa9a42f8fa0 100644 --- a/exporters/prometheus/prometheus.go +++ b/exporters/prometheus/prometheus.go @@ -163,7 +163,7 @@ func (c *collector) Describe(ch chan<- *prometheus.Desc) { }) } -// Collect exports the last calculated CheckpointSet. +// Collect exports the last calculated Reader state. // // Collect is invoked whenever prometheus.Gatherer is also invoked. // For example, when the HTTP endpoint is invoked by Prometheus. diff --git a/sdk/metric/controller/basic/controller.go b/sdk/metric/controller/basic/controller.go index 3c00fbadeb5..4c59ee1ef26 100644 --- a/sdk/metric/controller/basic/controller.go +++ b/sdk/metric/controller/basic/controller.go @@ -53,7 +53,7 @@ var ErrControllerStarted = fmt.Errorf("controller already started") // collection // // The controller supports mixing push and pull access to metric data -// using the export.CheckpointSet RWLock interface. Collection will +// using the export.Reader RWLock interface. Collection will // be blocked by a pull request in the basic controller. type Controller struct { lock sync.Mutex @@ -110,8 +110,8 @@ type accumulatorCheckpointer struct { library instrumentation.Library } -// New constructs a Controller using the provided checkpointer and -// options (including optional exporter) to configure a metric +// New constructs a Controller using the provided checkpointer factory +// and options (including optional exporter) to configure a metric // export pipeline. func New(checkpointerFactory export.CheckpointerFactory, opts ...Option) *Controller { c := &config{ @@ -270,23 +270,23 @@ func (c *Controller) accumulatorList() []*accumulatorCheckpointer { } // checkpoint calls the Accumulator and Checkpointer interfaces to -// compute the CheckpointSet. This applies the configured collection +// compute the Reader. This applies the configured collection // timeout. Note that this does not try to cancel a Collect or Export // when Stop() is called. func (c *Controller) checkpoint(ctx context.Context) error { for _, impl := range c.accumulatorList() { - if err := c.checkpointSingleAccmulator(ctx, impl); err != nil { + if err := c.checkpointSingleAccumulator(ctx, impl); err != nil { return err } } return nil } -// checkpointSingleAccmulator checkpoints a single instrumentation +// checkpointSingleAccumulator checkpoints a single instrumentation // library's accumulator, which involves calling // checkpointer.StartCollection, accumulator.Collect, and // checkpointer.FinishCollection in sequence. -func (c *Controller) checkpointSingleAccmulator(ctx context.Context, ac *accumulatorCheckpointer) error { +func (c *Controller) checkpointSingleAccumulator(ctx context.Context, ac *accumulatorCheckpointer) error { ckpt := ac.checkpointer.Reader() ckpt.Lock() defer ckpt.Unlock() diff --git a/sdk/metric/doc.go b/sdk/metric/doc.go index c6ac21c497a..35d0fa55447 100644 --- a/sdk/metric/doc.go +++ b/sdk/metric/doc.go @@ -102,7 +102,7 @@ record has an associated aggregator. Processor is an interface which sits between the SDK and an exporter. The Processor embeds an AggregatorSelector, used by the SDK to assign new Aggregators. The Processor supports a Process() API for submitting -checkpointed aggregators to the processor, and a CheckpointSet() API +checkpointed aggregators to the processor, and a Reader() API for producing a complete checkpoint for the exporter. Two default Processor implementations are provided, the "defaultkeys" Processor groups aggregate metrics by their recommended Descriptor.Keys(), the @@ -113,9 +113,9 @@ provide the serialization logic for labels. This allows avoiding duplicate serialization of labels, once as a unique key in the SDK (or Processor) and once in the exporter. -CheckpointSet is an interface between the Processor and the Exporter. -After completing a collection pass, the Processor.CheckpointSet() method -returns a CheckpointSet, which the Exporter uses to iterate over all +Reader is an interface between the Processor and the Exporter. +After completing a collection pass, the Processor.Reader() method +returns a Reader, which the Exporter uses to iterate over all the updated metrics. Record is a struct containing the state of an individual exported @@ -126,7 +126,7 @@ Labels is a struct containing an ordered set of labels, the corresponding unique encoding, and the encoder that produced it. Exporter is the final stage of an export pipeline. It is called with -a CheckpointSet capable of enumerating all the updated metrics. +a Reader capable of enumerating all the updated metrics. Controller is not an export interface per se, but it orchestrates the export pipeline. For example, a "push" controller will establish a diff --git a/sdk/metric/processor/basic/basic.go b/sdk/metric/processor/basic/basic.go index 42ad472a640..a8340b8ecbc 100644 --- a/sdk/metric/processor/basic/basic.go +++ b/sdk/metric/processor/basic/basic.go @@ -90,7 +90,7 @@ type ( state struct { config config - // RWMutex implements locking for the `CheckpointSet` interface. + // RWMutex implements locking for the `Reader` interface. sync.RWMutex values map[stateKey]*stateValue diff --git a/sdk/metric/processor/basic/config.go b/sdk/metric/processor/basic/config.go index 736f646aaf8..f4cf440cf4c 100644 --- a/sdk/metric/processor/basic/config.go +++ b/sdk/metric/processor/basic/config.go @@ -18,7 +18,7 @@ package basic // import "go.opentelemetry.io/otel/sdk/metric/processor/basic" type config struct { // Memory controls whether the processor remembers metric // instruments and label sets that were previously reported. - // When Memory is true, CheckpointSet.ForEach() will visit + // When Memory is true, Reader.ForEach() will visit // metrics that were not updated in the most recent interval. Memory bool } From 6ad398d2d4164e03fce39355cf072b3d220d6e7d Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 17 Sep 2021 11:43:12 -0700 Subject: [PATCH 25/33] lint --- internal/metric/go.sum | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/metric/go.sum b/internal/metric/go.sum index 5f0b342ef32..f212493d586 100644 --- a/internal/metric/go.sum +++ b/internal/metric/go.sum @@ -7,7 +7,6 @@ github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZN github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY= github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= -golang.org/x/sys v0.0.0-20210423185535-09eb48e85fd7/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= From 989066bf0f190152a6f45fd98e8970bb731b2afd Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Tue, 21 Sep 2021 12:05:15 -0700 Subject: [PATCH 26/33] remove two unnecessary accessor methods; Controller implements MeterProvider and InstrumentationLibraryReader directly, no need to get these --- .../internal/otlpmetrictest/otlptest.go | 2 +- .../otlpmetric/otlpmetricgrpc/example_test.go | 6 ++--- exporters/prometheus/prometheus.go | 6 ++--- exporters/stdout/stdoutmetric/example_test.go | 2 +- exporters/stdout/stdoutmetric/metric_test.go | 4 +-- sdk/metric/controller/basic/controller.go | 13 +-------- .../controller/basic/controller_test.go | 27 +++++++------------ sdk/metric/controller/basic/pull_test.go | 14 +++++----- sdk/metric/controller/basic/push_test.go | 4 +-- 9 files changed, 29 insertions(+), 49 deletions(-) diff --git a/exporters/otlp/otlpmetric/internal/otlpmetrictest/otlptest.go b/exporters/otlp/otlpmetric/internal/otlpmetrictest/otlptest.go index cd84c0c564e..a1328312781 100644 --- a/exporters/otlp/otlpmetric/internal/otlpmetrictest/otlptest.go +++ b/exporters/otlp/otlpmetric/internal/otlpmetrictest/otlptest.go @@ -44,7 +44,7 @@ func RunEndToEndTest(ctx context.Context, t *testing.T, exp *otlpmetric.Exporter cont := controller.New(proc, controller.WithExporter(exp)) require.NoError(t, cont.Start(ctx)) - meter := cont.MeterProvider().Meter("test-meter") + meter := cont.Meter("test-meter") labels := []attribute.KeyValue{attribute.Bool("test", true)} type data struct { diff --git a/exporters/otlp/otlpmetric/otlpmetricgrpc/example_test.go b/exporters/otlp/otlpmetric/otlpmetricgrpc/example_test.go index abbe0e981d4..76a6f1ee03a 100644 --- a/exporters/otlp/otlpmetric/otlpmetricgrpc/example_test.go +++ b/exporters/otlp/otlpmetric/otlpmetricgrpc/example_test.go @@ -55,7 +55,7 @@ func Example_insecure() { controller.WithExporter(exp), controller.WithCollectPeriod(2*time.Second), ) - global.SetMeterProvider(pusher.MeterProvider()) + global.SetMeterProvider(pusher) if err := pusher.Start(ctx); err != nil { log.Fatalf("could not start metric controoler: %v", err) @@ -114,7 +114,7 @@ func Example_withTLS() { controller.WithExporter(exp), controller.WithCollectPeriod(2*time.Second), ) - global.SetMeterProvider(pusher.MeterProvider()) + global.SetMeterProvider(pusher) if err := pusher.Start(ctx); err != nil { log.Fatalf("could not start metric controoler: %v", err) @@ -171,7 +171,7 @@ func Example_withDifferentSignalCollectors() { controller.WithExporter(exp), controller.WithCollectPeriod(2*time.Second), ) - global.SetMeterProvider(pusher.MeterProvider()) + global.SetMeterProvider(pusher) if err := pusher.Start(ctx); err != nil { log.Fatalf("could not start metric controoler: %v", err) diff --git a/exporters/prometheus/prometheus.go b/exporters/prometheus/prometheus.go index aa9a42f8fa0..572feff5701 100644 --- a/exporters/prometheus/prometheus.go +++ b/exporters/prometheus/prometheus.go @@ -121,7 +121,7 @@ func New(config Config, controller *controller.Controller) (*Exporter, error) { // MeterProvider returns the MeterProvider of this exporter. func (e *Exporter) MeterProvider() metric.MeterProvider { - return e.controller.MeterProvider() + return e.controller } // Controller returns the controller object that coordinates collection for the SDK. @@ -153,7 +153,7 @@ func (c *collector) Describe(ch chan<- *prometheus.Desc) { c.exp.lock.RLock() defer c.exp.lock.RUnlock() - _ = c.exp.Controller().InstrumentationLibraryReader().ForEach(func(_ instrumentation.Library, reader export.Reader) error { + _ = c.exp.Controller().ForEach(func(_ instrumentation.Library, reader export.Reader) error { return reader.ForEach(c.exp, func(record export.Record) error { var labelKeys []string mergeLabels(record, c.exp.controller.Resource(), &labelKeys, nil) @@ -176,7 +176,7 @@ func (c *collector) Collect(ch chan<- prometheus.Metric) { otel.Handle(err) } - err := ctrl.InstrumentationLibraryReader().ForEach(func(_ instrumentation.Library, reader export.Reader) error { + err := ctrl.ForEach(func(_ instrumentation.Library, reader export.Reader) error { return reader.ForEach(c.exp, func(record export.Record) error { agg := record.Aggregation() diff --git a/exporters/stdout/stdoutmetric/example_test.go b/exporters/stdout/stdoutmetric/example_test.go index bf19844b6ff..522877e73eb 100644 --- a/exporters/stdout/stdoutmetric/example_test.go +++ b/exporters/stdout/stdoutmetric/example_test.go @@ -81,7 +81,7 @@ func InstallExportPipeline(ctx context.Context) func() { if err = pusher.Start(ctx); err != nil { log.Fatalf("starting push controller: %v", err) } - global.SetMeterProvider(pusher.MeterProvider()) + global.SetMeterProvider(pusher) return func() { if err := pusher.Stop(ctx); err != nil { diff --git a/exporters/stdout/stdoutmetric/metric_test.go b/exporters/stdout/stdoutmetric/metric_test.go index ce27ab5bbc2..85ae1f3fb8e 100644 --- a/exporters/stdout/stdoutmetric/metric_test.go +++ b/exporters/stdout/stdoutmetric/metric_test.go @@ -68,7 +68,7 @@ func newFixtureWithResource(t *testing.T, res *resource.Resource, opts ...stdout ) ctx := context.Background() require.NoError(t, cont.Start(ctx)) - meter := cont.MeterProvider().Meter("test") + meter := cont.Meter("test") return testFixture{ t: t, @@ -101,7 +101,7 @@ func TestStdoutTimestamp(t *testing.T) { ctx := context.Background() require.NoError(t, cont.Start(ctx)) - meter := cont.MeterProvider().Meter("test") + meter := cont.Meter("test") counter := metric.Must(meter).NewInt64Counter("name.lastvalue") before := time.Now() diff --git a/sdk/metric/controller/basic/controller.go b/sdk/metric/controller/basic/controller.go index 4c59ee1ef26..7621f78e59c 100644 --- a/sdk/metric/controller/basic/controller.go +++ b/sdk/metric/controller/basic/controller.go @@ -153,23 +153,12 @@ func (c *Controller) SetClock(clock controllerTime.Clock) { c.clock = clock } -// MeterProvider returns a MeterProvider instance for this controller. -func (c *Controller) MeterProvider() metric.MeterProvider { - return c -} - // Resource returns the *resource.Resource associated with this // controller. func (c *Controller) Resource() *resource.Resource { return c.resource } -// InstrumentationLibraryReader returns an InstrumentationLibraryReader for iterating -// through the metrics of each registered library, one at a time. -func (c *Controller) InstrumentationLibraryReader() export.InstrumentationLibraryReader { - return c -} - // Start begins a ticker that periodically collects and exports // metrics with the configured interval. This is required for calling // a configured Exporter (see WithExporter) and is otherwise optional @@ -330,7 +319,7 @@ func (c *Controller) export(ctx context.Context) error { defer cancel() } - return c.exporter.Export(ctx, c.resource, c.InstrumentationLibraryReader()) + return c.exporter.Export(ctx, c.resource, c) } // ForEach implements export.InstrumentationLibraryReader. diff --git a/sdk/metric/controller/basic/controller_test.go b/sdk/metric/controller/basic/controller_test.go index 29e25922f32..4cca2cc2442 100644 --- a/sdk/metric/controller/basic/controller_test.go +++ b/sdk/metric/controller/basic/controller_test.go @@ -41,7 +41,7 @@ const envVar = "OTEL_RESOURCE_ATTRIBUTES" func getMap(t *testing.T, cont *controller.Controller) map[string]float64 { out := processortest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, cont.InstrumentationLibraryReader().ForEach( + require.NoError(t, cont.ForEach( func(_ instrumentation.Library, reader export.Reader) error { return reader.ForEach( export.CumulativeExportKindSelector(), @@ -125,9 +125,8 @@ func TestControllerUsesResource(t *testing.T) { ) ctx := context.Background() require.NoError(t, cont.Start(ctx)) - prov := cont.MeterProvider() - ctr := metric.Must(prov.Meter("named")).NewFloat64Counter("calls.sum") + ctr := metric.Must(cont.Meter("named")).NewFloat64Counter("calls.sum") ctr.Add(context.Background(), 1.) // Collect once @@ -153,10 +152,9 @@ func TestStartNoExporter(t *testing.T) { mock := controllertest.NewMockClock() cont.SetClock(mock) - prov := cont.MeterProvider() calls := int64(0) - _ = metric.Must(prov.Meter("named")).NewInt64CounterObserver("calls.lastvalue", + _ = metric.Must(cont.Meter("named")).NewInt64CounterObserver("calls.lastvalue", func(ctx context.Context, result metric.Int64ObserverResult) { calls++ checkTestContext(t, ctx) @@ -222,10 +220,9 @@ func TestObserverCanceled(t *testing.T) { controller.WithResource(resource.Empty()), ) - prov := cont.MeterProvider() calls := int64(0) - _ = metric.Must(prov.Meter("named")).NewInt64CounterObserver("done.lastvalue", + _ = metric.Must(cont.Meter("named")).NewInt64CounterObserver("done.lastvalue", func(ctx context.Context, result metric.Int64ObserverResult) { <-ctx.Done() calls++ @@ -254,9 +251,7 @@ func TestObserverContext(t *testing.T) { controller.WithResource(resource.Empty()), ) - prov := cont.MeterProvider() - - _ = metric.Must(prov.Meter("named")).NewInt64CounterObserver("done.lastvalue", + _ = metric.Must(cont.Meter("named")).NewInt64CounterObserver("done.lastvalue", func(ctx context.Context, result metric.Int64ObserverResult) { time.Sleep(10 * time.Millisecond) checkTestContext(t, ctx) @@ -322,10 +317,8 @@ func TestExportTimeout(t *testing.T) { mock := controllertest.NewMockClock() cont.SetClock(mock) - prov := cont.MeterProvider() - calls := int64(0) - _ = metric.Must(prov.Meter("named")).NewInt64CounterObserver("one.lastvalue", + _ = metric.Must(cont.Meter("named")).NewInt64CounterObserver("one.lastvalue", func(ctx context.Context, result metric.Int64ObserverResult) { calls++ result.Observe(calls) @@ -378,10 +371,8 @@ func TestCollectAfterStopThenStartAgain(t *testing.T) { mock := controllertest.NewMockClock() cont.SetClock(mock) - prov := cont.MeterProvider() - calls := 0 - _ = metric.Must(prov.Meter("named")).NewInt64CounterObserver("one.lastvalue", + _ = metric.Must(cont.Meter("named")).NewInt64CounterObserver("one.lastvalue", func(ctx context.Context, result metric.Int64ObserverResult) { calls++ result.Observe(int64(calls)) @@ -457,8 +448,8 @@ func TestRegistryFunction(t *testing.T) { controller.WithResource(resource.Empty()), ) - m1 := cont.MeterProvider().Meter("test") - m2 := cont.MeterProvider().Meter("test") + m1 := cont.Meter("test") + m2 := cont.Meter("test") require.NotNil(t, m1) require.Equal(t, m1, m2) diff --git a/sdk/metric/controller/basic/pull_test.go b/sdk/metric/controller/basic/pull_test.go index c8d11cb2379..04c25c23571 100644 --- a/sdk/metric/controller/basic/pull_test.go +++ b/sdk/metric/controller/basic/pull_test.go @@ -44,14 +44,14 @@ func TestPullNoCollect(t *testing.T) { ) ctx := context.Background() - meter := puller.MeterProvider().Meter("nocache") + meter := puller.Meter("nocache") counter := metric.Must(meter).NewInt64Counter("counter.sum") counter.Add(ctx, 10, attribute.String("A", "B")) require.NoError(t, puller.Collect(ctx)) records := processortest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, controllertest.ReadAll(puller.InstrumentationLibraryReader(), export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) + require.NoError(t, controllertest.ReadAll(puller, export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) require.EqualValues(t, map[string]float64{ "counter.sum/A=B/": 10, @@ -61,7 +61,7 @@ func TestPullNoCollect(t *testing.T) { require.NoError(t, puller.Collect(ctx)) records = processortest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, controllertest.ReadAll(puller.InstrumentationLibraryReader(), export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) + require.NoError(t, controllertest.ReadAll(puller, export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) require.EqualValues(t, map[string]float64{ "counter.sum/A=B/": 20, @@ -82,14 +82,14 @@ func TestPullWithCollect(t *testing.T) { puller.SetClock(mock) ctx := context.Background() - meter := puller.MeterProvider().Meter("nocache") + meter := puller.Meter("nocache") counter := metric.Must(meter).NewInt64Counter("counter.sum") counter.Add(ctx, 10, attribute.String("A", "B")) require.NoError(t, puller.Collect(ctx)) records := processortest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, controllertest.ReadAll(puller.InstrumentationLibraryReader(), export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) + require.NoError(t, controllertest.ReadAll(puller, export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) require.EqualValues(t, map[string]float64{ "counter.sum/A=B/": 10, @@ -100,7 +100,7 @@ func TestPullWithCollect(t *testing.T) { // Cached value! require.NoError(t, puller.Collect(ctx)) records = processortest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, controllertest.ReadAll(puller.InstrumentationLibraryReader(), export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) + require.NoError(t, controllertest.ReadAll(puller, export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) require.EqualValues(t, map[string]float64{ "counter.sum/A=B/": 10, @@ -112,7 +112,7 @@ func TestPullWithCollect(t *testing.T) { // Re-computed value! require.NoError(t, puller.Collect(ctx)) records = processortest.NewOutput(attribute.DefaultEncoder()) - require.NoError(t, controllertest.ReadAll(puller.InstrumentationLibraryReader(), export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) + require.NoError(t, controllertest.ReadAll(puller, export.CumulativeExportKindSelector(), records.AddInstrumentationLibraryRecord)) require.EqualValues(t, map[string]float64{ "counter.sum/A=B/": 20, diff --git a/sdk/metric/controller/basic/push_test.go b/sdk/metric/controller/basic/push_test.go index e9e818de536..0b1b5474f3b 100644 --- a/sdk/metric/controller/basic/push_test.go +++ b/sdk/metric/controller/basic/push_test.go @@ -110,7 +110,7 @@ func TestPushTicker(t *testing.T) { controller.WithCollectPeriod(time.Second), controller.WithResource(testResource), ) - meter := p.MeterProvider().Meter("name") + meter := p.Meter("name") mock := controllertest.NewMockClock() p.SetClock(mock) @@ -196,7 +196,7 @@ func TestPushExportError(t *testing.T) { ctx := context.Background() - meter := p.MeterProvider().Meter("name") + meter := p.Meter("name") counter1 := metric.Must(meter).NewInt64Counter("counter1.sum") counter2 := metric.Must(meter).NewInt64Counter("counter2.sum") From ea7bc599bd3a24c4acb4cd9facd13f08cd702237 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Tue, 21 Sep 2021 12:37:53 -0700 Subject: [PATCH 27/33] use a sync.Map --- sdk/metric/controller/basic/controller.go | 31 +++++++++++++---------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/sdk/metric/controller/basic/controller.go b/sdk/metric/controller/basic/controller.go index 7621f78e59c..5579751be46 100644 --- a/sdk/metric/controller/basic/controller.go +++ b/sdk/metric/controller/basic/controller.go @@ -57,7 +57,7 @@ var ErrControllerStarted = fmt.Errorf("controller already started") // be blocked by a pull request in the basic controller. type Controller struct { lock sync.Mutex - libraries map[instrumentation.Library]*registry.UniqueInstrumentMeterImpl + libraries sync.Map // a map[instrumentation.Library]*initMeterOnce checkpointerFactory export.CheckpointerFactory resource *resource.Resource @@ -76,6 +76,11 @@ type Controller struct { collectedTime time.Time } +type initMeterOnce struct { + unique *registry.UniqueInstrumentMeterImpl + initOnce sync.Once +} + var _ export.InstrumentationLibraryReader = &Controller{} var _ metric.MeterProvider = &Controller{} @@ -87,21 +92,20 @@ func (c *Controller) Meter(instrumentationName string, opts ...metric.MeterOptio SchemaURL: cfg.SchemaURL(), } - c.lock.Lock() - defer c.lock.Unlock() - m, ok := c.libraries[library] - if !ok { + newTmp := &initMeterOnce{} + m, _ := c.libraries.LoadOrStore(library, newTmp) + mo := m.(*initMeterOnce) + mo.initOnce.Do(func() { checkpointer := c.checkpointerFactory.NewCheckpointer() accumulator := sdk.NewAccumulator(checkpointer) - m = registry.NewUniqueInstrumentMeterImpl(&accumulatorCheckpointer{ + mo.unique = registry.NewUniqueInstrumentMeterImpl(&accumulatorCheckpointer{ Accumulator: accumulator, checkpointer: checkpointer, library: library, }) + }) - c.libraries[library] = m - } - return metric.WrapMeterImpl(m) + return metric.WrapMeterImpl(mo.unique) } type accumulatorCheckpointer struct { @@ -132,7 +136,6 @@ func New(checkpointerFactory export.CheckpointerFactory, opts ...Option) *Contro } } return &Controller{ - libraries: map[instrumentation.Library]*registry.UniqueInstrumentMeterImpl{}, checkpointerFactory: checkpointerFactory, exporter: c.Exporter, resource: c.Resource, @@ -249,12 +252,14 @@ func (c *Controller) accumulatorList() []*accumulatorCheckpointer { defer c.lock.Unlock() var r []*accumulatorCheckpointer - for _, entry := range c.libraries { - acc, ok := entry.MeterImpl().(*accumulatorCheckpointer) + c.libraries.Range(func(_, value interface{}) bool { + mo := value.(*initMeterOnce) + acc, ok := mo.unique.MeterImpl().(*accumulatorCheckpointer) if ok { r = append(r, acc) } - } + return true + }) return r } From 3356eb5ed0c288ac3edcc2bc2e853aecda7f29b3 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Tue, 21 Sep 2021 13:02:07 -0700 Subject: [PATCH 28/33] ensure the initOnce is always called; handle multiple errors --- sdk/metric/controller/basic/controller.go | 70 ++++++++++++++--------- 1 file changed, 43 insertions(+), 27 deletions(-) diff --git a/sdk/metric/controller/basic/controller.go b/sdk/metric/controller/basic/controller.go index 5579751be46..da863fcaab3 100644 --- a/sdk/metric/controller/basic/controller.go +++ b/sdk/metric/controller/basic/controller.go @@ -95,6 +95,11 @@ func (c *Controller) Meter(instrumentationName string, opts ...metric.MeterOptio newTmp := &initMeterOnce{} m, _ := c.libraries.LoadOrStore(library, newTmp) mo := m.(*initMeterOnce) + + return metric.WrapMeterImpl(c.initializeUniqueMeter(library, mo)) +} + +func (c *Controller) initializeUniqueMeter(library instrumentation.Library, mo *initMeterOnce) *registry.UniqueInstrumentMeterImpl { mo.initOnce.Do(func() { checkpointer := c.checkpointerFactory.NewCheckpointer() accumulator := sdk.NewAccumulator(checkpointer) @@ -104,8 +109,19 @@ func (c *Controller) Meter(instrumentationName string, opts ...metric.MeterOptio library: library, }) }) + return mo.unique +} - return metric.WrapMeterImpl(mo.unique) +// syncMapKeyValueToAccuulatorCheckpointer encapsulates the invariants +// placed on the libraries sync.Map, which is a +// map[instrumentation.Library]*initMeterOnce where the +// registry.UniqueInstrumentMeter's implementation is a +// *accumulatorCheckpointer. +func (c *Controller) syncMapKeyValueToAccumulatorCheckpointer(key, value interface{}) *accumulatorCheckpointer { + return c.initializeUniqueMeter( + key.(instrumentation.Library), + value.(*initMeterOnce), + ).MeterImpl().(*accumulatorCheckpointer) } type accumulatorCheckpointer struct { @@ -245,35 +261,27 @@ func (c *Controller) collect(ctx context.Context) error { return c.export(ctx) } -// accumulatorList returns a snapshot of current accumulators -// registered to this controller. This briefly locks the controller. -func (c *Controller) accumulatorList() []*accumulatorCheckpointer { - c.lock.Lock() - defer c.lock.Unlock() - - var r []*accumulatorCheckpointer - c.libraries.Range(func(_, value interface{}) bool { - mo := value.(*initMeterOnce) - acc, ok := mo.unique.MeterImpl().(*accumulatorCheckpointer) - if ok { - r = append(r, acc) - } - return true - }) - return r -} - // checkpoint calls the Accumulator and Checkpointer interfaces to // compute the Reader. This applies the configured collection // timeout. Note that this does not try to cancel a Collect or Export // when Stop() is called. func (c *Controller) checkpoint(ctx context.Context) error { - for _, impl := range c.accumulatorList() { - if err := c.checkpointSingleAccumulator(ctx, impl); err != nil { - return err + var errs []error + c.libraries.Range(func(key, value interface{}) bool { + acPair := c.syncMapKeyValueToAccumulatorCheckpointer(key, value) + + if err := c.checkpointSingleAccumulator(ctx, acPair); err != nil { + errs = append(errs, err) } + return false + }) + if errs == nil { + return nil } - return nil + if len(errs) == 1 { + return errs[0] + } + return fmt.Errorf("multiple checkpoint errors %w %v", errs[0], errs[1:]) } // checkpointSingleAccumulator checkpoints a single instrumentation @@ -329,18 +337,26 @@ func (c *Controller) export(ctx context.Context) error { // ForEach implements export.InstrumentationLibraryReader. func (c *Controller) ForEach(readerFunc func(l instrumentation.Library, r export.Reader) error) error { - for _, acPair := range c.accumulatorList() { + var errs []error + c.libraries.Range(func(key, value interface{}) bool { + acPair := c.syncMapKeyValueToAccumulatorCheckpointer(key, value) reader := acPair.checkpointer.Reader() - // TODO: We should not fail fast; instead accumulate errors. if err := func() error { reader.RLock() defer reader.RUnlock() return readerFunc(acPair.library, reader) }(); err != nil { - return err + errs = append(errs, err) } + return false + }) + if errs == nil { + return nil } - return nil + if len(errs) == 1 { + return errs[0] + } + return fmt.Errorf("multiple ForEach errors %w %v", errs[0], errs[1:]) } // IsRunning returns true if the controller was started via Start(), From b4636f3518d700cb0cc834f379e7f204ae68ad49 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Thu, 23 Sep 2021 12:16:46 -0700 Subject: [PATCH 29/33] Apply suggestions from code review Co-authored-by: Anthony Mirabella --- exporters/otlp/otlpmetric/internal/metrictransform/metric.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exporters/otlp/otlpmetric/internal/metrictransform/metric.go b/exporters/otlp/otlpmetric/internal/metrictransform/metric.go index b794b6c3df3..8774db45f1e 100644 --- a/exporters/otlp/otlpmetric/internal/metrictransform/metric.go +++ b/exporters/otlp/otlpmetric/internal/metrictransform/metric.go @@ -182,7 +182,7 @@ func transformer(ctx context.Context, exportSelector export.ExportKindSelector, func sink(ctx context.Context, in <-chan result) ([]*metricpb.Metric, error) { var errStrings []string - // Group by instrumentation library name and then the MetricDescriptor. + // Group by the MetricDescriptor. grouped := map[string]*metricpb.Metric{} for res := range in { if res.Err != nil { From b418d3df4530dcb44a820adc1635f87650a85300 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Thu, 23 Sep 2021 12:53:57 -0700 Subject: [PATCH 30/33] cleanup locking in metrictest --- metric/metrictest/meter.go | 48 ++++++++++++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 10 deletions(-) diff --git a/metric/metrictest/meter.go b/metric/metrictest/meter.go index 84f730428e2..957f894f142 100644 --- a/metric/metrictest/meter.go +++ b/metric/metrictest/meter.go @@ -92,6 +92,8 @@ var ( _ metric.AsyncImpl = &Async{} ) +// NewDescriptor is a test helper for constructing test metric +// descriptors using standard options. func NewDescriptor(name string, ikind sdkapi.InstrumentKind, nkind number.Kind, opts ...metric.InstrumentOption) metric.Descriptor { cfg := metric.NewInstrumentConfig(opts...) return metric.NewDescriptor(name, ikind, nkind, cfg.Description(), cfg.Unit()) @@ -134,10 +136,13 @@ func (m *MeterImpl) doRecordSingle(ctx context.Context, labels []attribute.KeyVa }}) } +// NewMeterProvider returns a MeterProvider suitable for testing. +// When the test is complete, consult MeterProvider.MeasurementBatches. func NewMeterProvider() *MeterProvider { return &MeterProvider{} } +// Meter implements metric.MeterProvider. func (p *MeterProvider) Meter(name string, opts ...metric.MeterOption) metric.Meter { p.lock.Lock() defer p.lock.Unlock() @@ -155,10 +160,8 @@ func (p *MeterProvider) Meter(name string, opts ...metric.MeterOption) metric.Me return metric.WrapMeterImpl(impl) } +// NewSyncInstrument implements metric.MeterImpl. func (m *MeterImpl) NewSyncInstrument(descriptor metric.Descriptor) (metric.SyncImpl, error) { - m.provider.lock.Lock() - defer m.provider.lock.Unlock() - return &Sync{ Instrument{ descriptor: descriptor, @@ -167,10 +170,8 @@ func (m *MeterImpl) NewSyncInstrument(descriptor metric.Descriptor) (metric.Sync }, nil } +// NewAsyncInstrument implements metric.MeterImpl. func (m *MeterImpl) NewAsyncInstrument(descriptor metric.Descriptor, runner metric.AsyncRunner) (metric.AsyncImpl, error) { - m.provider.lock.Lock() - defer m.provider.lock.Unlock() - a := &Async{ Instrument: Instrument{ descriptor: descriptor, @@ -178,10 +179,11 @@ func (m *MeterImpl) NewAsyncInstrument(descriptor metric.Descriptor, runner metr }, runner: runner, } - m.asyncInstruments.Register(a, runner) + m.provider.registerAsyncInstrument(a, m, runner) return a, nil } +// RecordBatch implements metric.MeterImpl. func (m *MeterImpl) RecordBatch(ctx context.Context, labels []attribute.KeyValue, measurements ...metric.Measurement) { mm := make([]Measurement, len(measurements)) for i := 0; i < len(measurements); i++ { @@ -194,6 +196,7 @@ func (m *MeterImpl) RecordBatch(ctx context.Context, labels []attribute.KeyValue m.collect(ctx, labels, mm) } +// CollectAsync is called from asyncInstruments.Run() with the lock held. func (m *MeterImpl) CollectAsync(labels []attribute.KeyValue, obs ...metric.Observation) { mm := make([]Measurement, len(obs)) for i := 0; i < len(obs); i++ { @@ -206,8 +209,9 @@ func (m *MeterImpl) CollectAsync(labels []attribute.KeyValue, obs ...metric.Obse m.collect(context.Background(), labels, mm) } +// collect is called from CollectAsync() or RecordBatch() with the lock held. func (m *MeterImpl) collect(ctx context.Context, labels []attribute.KeyValue, measurements []Measurement) { - m.provider.MeasurementBatches = append(m.provider.MeasurementBatches, Batch{ + m.provider.addMeasurement(Batch{ Ctx: ctx, Labels: labels, Measurements: measurements, @@ -215,10 +219,34 @@ func (m *MeterImpl) collect(ctx context.Context, labels []attribute.KeyValue, me }) } -func (p *MeterProvider) RunAsyncInstruments() { +// registerAsyncInstrument locks the provider and registers the new Async instrument. +func (p *MeterProvider) registerAsyncInstrument(a *Async, m *MeterImpl, runner metric.AsyncRunner) { p.lock.Lock() defer p.lock.Unlock() - for _, impl := range p.impls { + + m.asyncInstruments.Register(a, runner) +} + +// addMeasurement locks the provider and adds the new measurement batch. +func (p *MeterProvider) addMeasurement(b Batch) { + p.lock.Lock() + defer p.lock.Unlock() + p.MeasurementBatches = append(p.MeasurementBatches, b) +} + +// copyImpls locks the provider and copies the current list of *MeterImpls. +func (p *MeterProvider) copyImpls() []*MeterImpl { + p.lock.Lock() + defer p.lock.Unlock() + cpy := make([]*MeterImpl, len(p.impls)) + copy(cpy, p.impls) + return cpy +} + +// RunAsyncInstruments is used in tests to trigger collection from +// asynchronous instruments. +func (p *MeterProvider) RunAsyncInstruments() { + for _, impl := range p.copyImpls() { impl.asyncInstruments.Run(context.Background(), impl) } } From 265232bd3576de6b2d8085beca70c848da4d7887 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 24 Sep 2021 08:55:16 -0700 Subject: [PATCH 31/33] Revert "ensure the initOnce is always called; handle multiple errors" This reverts commit 3356eb5ed0c288ac3edcc2bc2e853aecda7f29b3. --- sdk/metric/controller/basic/controller.go | 70 +++++++++-------------- 1 file changed, 27 insertions(+), 43 deletions(-) diff --git a/sdk/metric/controller/basic/controller.go b/sdk/metric/controller/basic/controller.go index da863fcaab3..5579751be46 100644 --- a/sdk/metric/controller/basic/controller.go +++ b/sdk/metric/controller/basic/controller.go @@ -95,11 +95,6 @@ func (c *Controller) Meter(instrumentationName string, opts ...metric.MeterOptio newTmp := &initMeterOnce{} m, _ := c.libraries.LoadOrStore(library, newTmp) mo := m.(*initMeterOnce) - - return metric.WrapMeterImpl(c.initializeUniqueMeter(library, mo)) -} - -func (c *Controller) initializeUniqueMeter(library instrumentation.Library, mo *initMeterOnce) *registry.UniqueInstrumentMeterImpl { mo.initOnce.Do(func() { checkpointer := c.checkpointerFactory.NewCheckpointer() accumulator := sdk.NewAccumulator(checkpointer) @@ -109,19 +104,8 @@ func (c *Controller) initializeUniqueMeter(library instrumentation.Library, mo * library: library, }) }) - return mo.unique -} -// syncMapKeyValueToAccuulatorCheckpointer encapsulates the invariants -// placed on the libraries sync.Map, which is a -// map[instrumentation.Library]*initMeterOnce where the -// registry.UniqueInstrumentMeter's implementation is a -// *accumulatorCheckpointer. -func (c *Controller) syncMapKeyValueToAccumulatorCheckpointer(key, value interface{}) *accumulatorCheckpointer { - return c.initializeUniqueMeter( - key.(instrumentation.Library), - value.(*initMeterOnce), - ).MeterImpl().(*accumulatorCheckpointer) + return metric.WrapMeterImpl(mo.unique) } type accumulatorCheckpointer struct { @@ -261,27 +245,35 @@ func (c *Controller) collect(ctx context.Context) error { return c.export(ctx) } +// accumulatorList returns a snapshot of current accumulators +// registered to this controller. This briefly locks the controller. +func (c *Controller) accumulatorList() []*accumulatorCheckpointer { + c.lock.Lock() + defer c.lock.Unlock() + + var r []*accumulatorCheckpointer + c.libraries.Range(func(_, value interface{}) bool { + mo := value.(*initMeterOnce) + acc, ok := mo.unique.MeterImpl().(*accumulatorCheckpointer) + if ok { + r = append(r, acc) + } + return true + }) + return r +} + // checkpoint calls the Accumulator and Checkpointer interfaces to // compute the Reader. This applies the configured collection // timeout. Note that this does not try to cancel a Collect or Export // when Stop() is called. func (c *Controller) checkpoint(ctx context.Context) error { - var errs []error - c.libraries.Range(func(key, value interface{}) bool { - acPair := c.syncMapKeyValueToAccumulatorCheckpointer(key, value) - - if err := c.checkpointSingleAccumulator(ctx, acPair); err != nil { - errs = append(errs, err) + for _, impl := range c.accumulatorList() { + if err := c.checkpointSingleAccumulator(ctx, impl); err != nil { + return err } - return false - }) - if errs == nil { - return nil - } - if len(errs) == 1 { - return errs[0] } - return fmt.Errorf("multiple checkpoint errors %w %v", errs[0], errs[1:]) + return nil } // checkpointSingleAccumulator checkpoints a single instrumentation @@ -337,26 +329,18 @@ func (c *Controller) export(ctx context.Context) error { // ForEach implements export.InstrumentationLibraryReader. func (c *Controller) ForEach(readerFunc func(l instrumentation.Library, r export.Reader) error) error { - var errs []error - c.libraries.Range(func(key, value interface{}) bool { - acPair := c.syncMapKeyValueToAccumulatorCheckpointer(key, value) + for _, acPair := range c.accumulatorList() { reader := acPair.checkpointer.Reader() + // TODO: We should not fail fast; instead accumulate errors. if err := func() error { reader.RLock() defer reader.RUnlock() return readerFunc(acPair.library, reader) }(); err != nil { - errs = append(errs, err) + return err } - return false - }) - if errs == nil { - return nil } - if len(errs) == 1 { - return errs[0] - } - return fmt.Errorf("multiple ForEach errors %w %v", errs[0], errs[1:]) + return nil } // IsRunning returns true if the controller was started via Start(), From 2e183e95b7b7ae88e307cf7b18e893927420e151 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 24 Sep 2021 08:55:28 -0700 Subject: [PATCH 32/33] Revert "use a sync.Map" This reverts commit ea7bc599bd3a24c4acb4cd9facd13f08cd702237. --- sdk/metric/controller/basic/controller.go | 31 ++++++++++------------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/sdk/metric/controller/basic/controller.go b/sdk/metric/controller/basic/controller.go index 5579751be46..7621f78e59c 100644 --- a/sdk/metric/controller/basic/controller.go +++ b/sdk/metric/controller/basic/controller.go @@ -57,7 +57,7 @@ var ErrControllerStarted = fmt.Errorf("controller already started") // be blocked by a pull request in the basic controller. type Controller struct { lock sync.Mutex - libraries sync.Map // a map[instrumentation.Library]*initMeterOnce + libraries map[instrumentation.Library]*registry.UniqueInstrumentMeterImpl checkpointerFactory export.CheckpointerFactory resource *resource.Resource @@ -76,11 +76,6 @@ type Controller struct { collectedTime time.Time } -type initMeterOnce struct { - unique *registry.UniqueInstrumentMeterImpl - initOnce sync.Once -} - var _ export.InstrumentationLibraryReader = &Controller{} var _ metric.MeterProvider = &Controller{} @@ -92,20 +87,21 @@ func (c *Controller) Meter(instrumentationName string, opts ...metric.MeterOptio SchemaURL: cfg.SchemaURL(), } - newTmp := &initMeterOnce{} - m, _ := c.libraries.LoadOrStore(library, newTmp) - mo := m.(*initMeterOnce) - mo.initOnce.Do(func() { + c.lock.Lock() + defer c.lock.Unlock() + m, ok := c.libraries[library] + if !ok { checkpointer := c.checkpointerFactory.NewCheckpointer() accumulator := sdk.NewAccumulator(checkpointer) - mo.unique = registry.NewUniqueInstrumentMeterImpl(&accumulatorCheckpointer{ + m = registry.NewUniqueInstrumentMeterImpl(&accumulatorCheckpointer{ Accumulator: accumulator, checkpointer: checkpointer, library: library, }) - }) - return metric.WrapMeterImpl(mo.unique) + c.libraries[library] = m + } + return metric.WrapMeterImpl(m) } type accumulatorCheckpointer struct { @@ -136,6 +132,7 @@ func New(checkpointerFactory export.CheckpointerFactory, opts ...Option) *Contro } } return &Controller{ + libraries: map[instrumentation.Library]*registry.UniqueInstrumentMeterImpl{}, checkpointerFactory: checkpointerFactory, exporter: c.Exporter, resource: c.Resource, @@ -252,14 +249,12 @@ func (c *Controller) accumulatorList() []*accumulatorCheckpointer { defer c.lock.Unlock() var r []*accumulatorCheckpointer - c.libraries.Range(func(_, value interface{}) bool { - mo := value.(*initMeterOnce) - acc, ok := mo.unique.MeterImpl().(*accumulatorCheckpointer) + for _, entry := range c.libraries { + acc, ok := entry.MeterImpl().(*accumulatorCheckpointer) if ok { r = append(r, acc) } - return true - }) + } return r } From 6e984a51e5ed08f52cdec9ed2c525fdcc78d5a46 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 24 Sep 2021 09:02:44 -0700 Subject: [PATCH 33/33] restore the TODO about sync.Map --- sdk/metric/controller/basic/controller.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/sdk/metric/controller/basic/controller.go b/sdk/metric/controller/basic/controller.go index 7621f78e59c..97ba8945f06 100644 --- a/sdk/metric/controller/basic/controller.go +++ b/sdk/metric/controller/basic/controller.go @@ -56,7 +56,13 @@ var ErrControllerStarted = fmt.Errorf("controller already started") // using the export.Reader RWLock interface. Collection will // be blocked by a pull request in the basic controller. type Controller struct { - lock sync.Mutex + // lock protects libraries and synchronizes Start() and Stop(). + lock sync.Mutex + // TODO: libraries is synchronized by lock, but could be + // accomplished using a sync.Map. The SDK specification will + // probably require this, as the draft already states that + // Stop() and MeterProvider.Meter() should not block each + // other. libraries map[instrumentation.Library]*registry.UniqueInstrumentMeterImpl checkpointerFactory export.CheckpointerFactory