From 425e9ce2ff08474a5f143b2006f54c2bce325d28 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 14 Jul 2022 04:44:45 -0700 Subject: [PATCH 1/3] Use export.Aggregation instead of internal.Aggregation (#3007) * Use export.Aggregation instead of internal * Return an export.Aggregation instead of a slice --- sdk/metric/internal/aggregation.go | 61 ------------------- sdk/metric/internal/aggregator.go | 11 ++-- .../internal/aggregator_example_test.go | 12 ++-- sdk/metric/internal/histogram.go | 5 +- sdk/metric/internal/lastvalue.go | 7 ++- sdk/metric/internal/sum.go | 9 ++- 6 files changed, 27 insertions(+), 78 deletions(-) delete mode 100644 sdk/metric/internal/aggregation.go diff --git a/sdk/metric/internal/aggregation.go b/sdk/metric/internal/aggregation.go deleted file mode 100644 index f161d2debbf..00000000000 --- a/sdk/metric/internal/aggregation.go +++ /dev/null @@ -1,61 +0,0 @@ -// Copyright The OpenTelemetry Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -//go:build go1.18 -// +build go1.18 - -package internal // import "go.opentelemetry.io/otel/sdk/metric/internal" - -import ( - "go.opentelemetry.io/otel/attribute" -) - -// Aggregation is a single data point in a timeseries that summarizes -// measurements made during a time span. -type Aggregation struct { - // TODO(#2968): Replace this with the export.Aggregation type once #2961 - // is merged. - - // Timestamp defines the time the last measurement was made. If zero, no - // measurements were made for this time span. The time is represented as a - // unix timestamp with nanosecond precision. - Timestamp uint64 - - // Attributes are the unique dimensions Value describes. - Attributes attribute.Set - - // Value is the summarization of the measurements made. - Value value -} - -type value interface { - private() -} - -// SingleValue summarizes a set of measurements as a single value. -type SingleValue[N int64 | float64] struct { - Value N -} - -func (SingleValue[N]) private() {} - -// HistogramValue summarizes a set of measurements as a histogram. -type HistogramValue struct { - Bounds []float64 - Counts []uint64 - Sum float64 - Min, Max float64 -} - -func (HistogramValue) private() {} diff --git a/sdk/metric/internal/aggregator.go b/sdk/metric/internal/aggregator.go index df8b59e84bb..c81c6f5432b 100644 --- a/sdk/metric/internal/aggregator.go +++ b/sdk/metric/internal/aggregator.go @@ -17,7 +17,10 @@ package internal // import "go.opentelemetry.io/otel/sdk/metric/internal" -import "go.opentelemetry.io/otel/attribute" +import ( + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/sdk/metric/export" +) // Aggregator forms an aggregation from a collection of recorded measurements. type Aggregator[N int64 | float64] interface { @@ -25,7 +28,7 @@ type Aggregator[N int64 | float64] interface { // into an aggregation. Aggregate(measurement N, attr attribute.Set) - // Aggregations returns a slice of Aggregation, one per each attribute set - // used to scope measurement aggregation, and ends an aggregation cycle. - Aggregations() []Aggregation + // Aggregation returns an Aggregation, for all the aggregated + // measurements made and ends an aggregation cycle. + Aggregation() export.Aggregation } diff --git a/sdk/metric/internal/aggregator_example_test.go b/sdk/metric/internal/aggregator_example_test.go index 47fe2253818..c89acdd12f6 100644 --- a/sdk/metric/internal/aggregator_example_test.go +++ b/sdk/metric/internal/aggregator_example_test.go @@ -25,13 +25,13 @@ import ( "go.opentelemetry.io/otel/metric/instrument" "go.opentelemetry.io/otel/metric/instrument/syncint64" "go.opentelemetry.io/otel/sdk/metric/aggregation" + "go.opentelemetry.io/otel/sdk/metric/export" ) type meter struct { // When a reader initiates a collection, the meter would collect - // aggregations from each of these functions. In this process they will - // progress the aggregation period of each instrument's aggregator. - aggregationFuncs []func() []Aggregation + // aggregations from each of these functions. + aggregations []export.Aggregation } func (m *meter) SyncInt64() syncint64.InstrumentProvider { @@ -51,7 +51,7 @@ func (p *syncInt64Provider) Counter(string, ...instrument.Option) (syncint64.Cou aggregator := NewCumulativeSum[int64]() count := inst{aggregateFunc: aggregator.Aggregate} - p.aggregationFuncs = append(p.aggregationFuncs, aggregator.Aggregations) + p.aggregations = append(p.aggregations, aggregator.Aggregation()) fmt.Printf("using %T aggregator for counter\n", aggregator) @@ -69,7 +69,7 @@ func (p *syncInt64Provider) UpDownCounter(string, ...instrument.Option) (syncint aggregator := NewLastValue[int64]() upDownCount := inst{aggregateFunc: aggregator.Aggregate} - p.aggregationFuncs = append(p.aggregationFuncs, aggregator.Aggregations) + p.aggregations = append(p.aggregations, aggregator.Aggregation()) fmt.Printf("using %T aggregator for up-down counter\n", aggregator) @@ -89,7 +89,7 @@ func (p *syncInt64Provider) Histogram(string, ...instrument.Option) (syncint64.H }) hist := inst{aggregateFunc: aggregator.Aggregate} - p.aggregationFuncs = append(p.aggregationFuncs, aggregator.Aggregations) + p.aggregations = append(p.aggregations, aggregator.Aggregation()) fmt.Printf("using %T aggregator for histogram\n", aggregator) diff --git a/sdk/metric/internal/histogram.go b/sdk/metric/internal/histogram.go index 6931c01f344..ddebf840b69 100644 --- a/sdk/metric/internal/histogram.go +++ b/sdk/metric/internal/histogram.go @@ -20,6 +20,7 @@ package internal // import "go.opentelemetry.io/otel/sdk/metric/internal" import ( "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/sdk/metric/aggregation" + "go.opentelemetry.io/otel/sdk/metric/export" ) // histogram summarizes a set of measurements as an histogram with @@ -51,7 +52,7 @@ type deltaHistogram[N int64 | float64] struct { // TODO(#2970): implement. } -func (s *deltaHistogram[N]) Aggregations() []Aggregation { +func (s *deltaHistogram[N]) Aggregation() export.Aggregation { // TODO(#2970): implement. return nil } @@ -74,7 +75,7 @@ type cumulativeHistogram[N int64 | float64] struct { // TODO(#2970): implement. } -func (s *cumulativeHistogram[N]) Aggregations() []Aggregation { +func (s *cumulativeHistogram[N]) Aggregation() export.Aggregation { // TODO(#2970): implement. return nil } diff --git a/sdk/metric/internal/lastvalue.go b/sdk/metric/internal/lastvalue.go index 986a2313ad0..7c439d43e7d 100644 --- a/sdk/metric/internal/lastvalue.go +++ b/sdk/metric/internal/lastvalue.go @@ -17,7 +17,10 @@ package internal // import "go.opentelemetry.io/otel/sdk/metric/internal" -import "go.opentelemetry.io/otel/attribute" +import ( + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/sdk/metric/export" +) // lastValue summarizes a set of measurements as the last one made. type lastValue[N int64 | float64] struct { @@ -34,7 +37,7 @@ func (s *lastValue[N]) Aggregate(value N, attr attribute.Set) { // TODO(#2971): implement. } -func (s *lastValue[N]) Aggregations() []Aggregation { +func (s *lastValue[N]) Aggregation() export.Aggregation { // TODO(#2971): implement. return nil } diff --git a/sdk/metric/internal/sum.go b/sdk/metric/internal/sum.go index 6d01183fb03..35766e98dd3 100644 --- a/sdk/metric/internal/sum.go +++ b/sdk/metric/internal/sum.go @@ -17,7 +17,10 @@ package internal // import "go.opentelemetry.io/otel/sdk/metric/internal" -import "go.opentelemetry.io/otel/attribute" +import ( + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/sdk/metric/export" +) // sum summarizes a set of measurements as their arithmetic sum. type sum[N int64 | float64] struct { @@ -47,7 +50,7 @@ type deltaSum[N int64 | float64] struct { // TODO(#2972): implement. } -func (s *deltaSum[N]) Aggregations() []Aggregation { +func (s *deltaSum[N]) Aggregation() export.Aggregation { // TODO(#2972): implement. return nil } @@ -71,7 +74,7 @@ type cumulativeSum[N int64 | float64] struct { // TODO(#2972): implement. } -func (s *cumulativeSum[N]) Aggregations() []Aggregation { +func (s *cumulativeSum[N]) Aggregation() export.Aggregation { // TODO(#2972): implement. return nil } From a1c5b6fb6b000aa80634a51a608f2ca3f677f3e1 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 14 Jul 2022 04:49:07 -0700 Subject: [PATCH 2/3] Use attribute Sets instead of KeyValues for export data (#3012) Attribute Sets have stronger guarantees about the uniqueness of their keys and more functionality. We already ensure attributes are stored as Sets by the aggregator which will produce these data types. Instead of converting to a KeyValue slice, keep the data as a Set. Any user of the data can always call the ToSlice method to use the data as a slice of KeyValues. --- sdk/metric/export/data.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/sdk/metric/export/data.go b/sdk/metric/export/data.go index d130da3786a..196a83057a9 100644 --- a/sdk/metric/export/data.go +++ b/sdk/metric/export/data.go @@ -87,8 +87,9 @@ func (Sum) privateAggregation() {} // DataPoint is a single data point in a timeseries. type DataPoint struct { - // Attributes is the set of key value pairs that uniquely identify the timeseries. - Attributes []attribute.KeyValue + // Attributes is the set of key value pairs that uniquely identify the + // timeseries. + Attributes attribute.Set // StartTime is when the timeseries was started. (optional) StartTime time.Time // Time is the time when the timeseries was recorded. (optional) @@ -126,8 +127,9 @@ func (Histogram) privateAggregation() {} // HistogramDataPoint is a single histogram data point in a timeseries. type HistogramDataPoint struct { - // Attributes is the set of key value pairs that uniquely identify the timeseries. - Attributes []attribute.KeyValue + // Attributes is the set of key value pairs that uniquely identify the + // timeseries. + Attributes attribute.Set // StartTime is when the timeseries was started. StartTime time.Time // Time is the time when the timeseries was recorded. From 5f342475bb4aabb65f1f6e918822a4e95ea397c6 Mon Sep 17 00:00:00 2001 From: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com> Date: Thu, 14 Jul 2022 07:55:44 -0400 Subject: [PATCH 3/3] Change Instrument Library to Instrument Scope (#3016) Co-authored-by: Tyler Yahn --- sdk/metric/meter.go | 22 +++++++++++----------- sdk/metric/meter_test.go | 16 ++++++++-------- sdk/metric/provider.go | 2 +- sdk/metric/view/instrument.go | 2 +- sdk/metric/view/view.go | 24 ++++++++++++------------ sdk/metric/view/view_test.go | 22 +++++++++++----------- 6 files changed, 44 insertions(+), 44 deletions(-) diff --git a/sdk/metric/meter.go b/sdk/metric/meter.go index daf892dd1dc..c77a4fa54d8 100644 --- a/sdk/metric/meter.go +++ b/sdk/metric/meter.go @@ -31,7 +31,7 @@ import ( ) // meterRegistry keeps a record of initialized meters for instrumentation -// libraries. A meter is unique to an instrumentation library and if multiple +// scopes. A meter is unique to an instrumentation scope and if multiple // requests for that meter are made a meterRegistry ensure the same instance // is used. // @@ -43,31 +43,31 @@ import ( type meterRegistry struct { sync.Mutex - meters map[instrumentation.Library]*meter + meters map[instrumentation.Scope]*meter } -// Get returns a registered meter matching the instrumentation library if it +// Get returns a registered meter matching the instrumentation scope if it // exists in the meterRegistry. Otherwise, a new meter configured for the -// instrumentation library is registered and then returned. +// instrumentation scope is registered and then returned. // // Get is safe to call concurrently. -func (r *meterRegistry) Get(l instrumentation.Library) *meter { +func (r *meterRegistry) Get(s instrumentation.Scope) *meter { r.Lock() defer r.Unlock() if r.meters == nil { - m := &meter{Library: l} - r.meters = map[instrumentation.Library]*meter{l: m} + m := &meter{Scope: s} + r.meters = map[instrumentation.Scope]*meter{s: m} return m } - m, ok := r.meters[l] + m, ok := r.meters[s] if ok { return m } - m = &meter{Library: l} - r.meters[l] = m + m = &meter{Scope: s} + r.meters[s] = m return m } @@ -91,7 +91,7 @@ func (r *meterRegistry) Range(f func(*meter) bool) { // produced by an instrumentation scope will use metric instruments from a // single meter. type meter struct { - instrumentation.Library + instrumentation.Scope // TODO (#2815, 2814): implement. } diff --git a/sdk/metric/meter_test.go b/sdk/metric/meter_test.go index 38aa7ae28d9..54ae767b20b 100644 --- a/sdk/metric/meter_test.go +++ b/sdk/metric/meter_test.go @@ -26,24 +26,24 @@ import ( ) func TestMeterRegistry(t *testing.T) { - il0 := instrumentation.Library{Name: "zero"} - il1 := instrumentation.Library{Name: "one"} + is0 := instrumentation.Scope{Name: "zero"} + is1 := instrumentation.Scope{Name: "one"} r := meterRegistry{} var m0 *meter t.Run("ZeroValueGetDoesNotPanic", func(t *testing.T) { - assert.NotPanics(t, func() { m0 = r.Get(il0) }) - assert.Equal(t, il0, m0.Library, "uninitialized meter returned") + assert.NotPanics(t, func() { m0 = r.Get(is0) }) + assert.Equal(t, is0, m0.Scope, "uninitialized meter returned") }) - m01 := r.Get(il0) + m01 := r.Get(is0) t.Run("GetSameMeter", func(t *testing.T) { - assert.Samef(t, m0, m01, "returned different meters: %v", il0) + assert.Samef(t, m0, m01, "returned different meters: %v", is0) }) - m1 := r.Get(il1) + m1 := r.Get(is1) t.Run("GetDifferentMeter", func(t *testing.T) { - assert.NotSamef(t, m0, m1, "returned same meters: %v", il1) + assert.NotSamef(t, m0, m1, "returned same meters: %v", is1) }) t.Run("RangeComplete", func(t *testing.T) { diff --git a/sdk/metric/provider.go b/sdk/metric/provider.go index 5b939f93251..a3c12ea5808 100644 --- a/sdk/metric/provider.go +++ b/sdk/metric/provider.go @@ -76,7 +76,7 @@ func NewMeterProvider(options ...Option) *MeterProvider { // This method is safe to call concurrently. func (mp *MeterProvider) Meter(name string, options ...metric.MeterOption) metric.Meter { c := metric.NewMeterConfig(options...) - return mp.meters.Get(instrumentation.Library{ + return mp.meters.Get(instrumentation.Scope{ Name: name, Version: c.InstrumentationVersion(), SchemaURL: c.SchemaURL(), diff --git a/sdk/metric/view/instrument.go b/sdk/metric/view/instrument.go index 10d30243fa8..3df030aa4ba 100644 --- a/sdk/metric/view/instrument.go +++ b/sdk/metric/view/instrument.go @@ -24,7 +24,7 @@ import ( // Instrument uniquely identifies an instrument within a meter. type Instrument struct { - Scope instrumentation.Library + Scope instrumentation.Scope Name string Description string diff --git a/sdk/metric/view/view.go b/sdk/metric/view/view.go index 35483f59ab1..03bfd8eec73 100644 --- a/sdk/metric/view/view.go +++ b/sdk/metric/view/view.go @@ -40,7 +40,7 @@ import ( type View struct { instrumentName *regexp.Regexp hasWildcard bool - scope instrumentation.Library + scope instrumentation.Scope filter attribute.Filter name string @@ -57,9 +57,9 @@ func New(opts ...Option) (View, error) { v = opt.apply(v) } - emptyLibrary := instrumentation.Library{} + emptyScope := instrumentation.Scope{} if v.instrumentName == nil && - v.scope == emptyLibrary { + v.scope == emptyScope { return View{}, fmt.Errorf("must provide at least 1 match option") } @@ -104,23 +104,23 @@ func (v View) matchName(name string) bool { return v.instrumentName == nil || v.instrumentName.MatchString(name) } -func (v View) matchLibraryName(name string) bool { +func (v View) matchScopeName(name string) bool { return v.scope.Name == "" || name == v.scope.Name } -func (v View) matchLibraryVersion(version string) bool { +func (v View) matchScopeVersion(version string) bool { return v.scope.Version == "" || version == v.scope.Version } -func (v View) matchLibrarySchemaURL(schemaURL string) bool { +func (v View) matchScopeSchemaURL(schemaURL string) bool { return v.scope.SchemaURL == "" || schemaURL == v.scope.SchemaURL } func (v View) match(i Instrument) bool { return v.matchName(i.Name) && - v.matchLibraryName(i.Scope.Name) && - v.matchLibrarySchemaURL(i.Scope.SchemaURL) && - v.matchLibraryVersion(i.Scope.Version) + v.matchScopeName(i.Scope.Name) && + v.matchScopeSchemaURL(i.Scope.SchemaURL) && + v.matchScopeVersion(i.Scope.Version) } // Option applies a Configuration option value to a View. All options @@ -155,12 +155,12 @@ func MatchInstrumentName(name string) Option { // TODO (#2813): Implement MatchInstrumentKind when InstrumentKind is defined. // TODO (#2813): Implement MatchNumberKind when NumberKind is defined. -// MatchInstrumentationLibrary will do an exact match on any +// MatchInstrumentationScope will do an exact match on any // instrumentation.Scope field that is non-empty (""). The default is to match all // instrumentation scopes. -func MatchInstrumentationLibrary(lib instrumentation.Library) Option { +func MatchInstrumentationScope(scope instrumentation.Scope) Option { return optionFunc(func(v View) View { - v.scope = lib + v.scope = scope return v }) } diff --git a/sdk/metric/view/view_test.go b/sdk/metric/view/view_test.go index 10696ff00a6..2d95c437355 100644 --- a/sdk/metric/view/view_test.go +++ b/sdk/metric/view/view_test.go @@ -28,7 +28,7 @@ import ( ) var matchInstrument = Instrument{ - Scope: instrumentation.Library{ + Scope: instrumentation.Scope{ Name: "bar", Version: "v1.0.0", SchemaURL: "stuff.test/", @@ -38,7 +38,7 @@ var matchInstrument = Instrument{ } var noMatchInstrument = Instrument{ - Scope: instrumentation.Library{ + Scope: instrumentation.Scope{ Name: "notfoo", Version: "v0.x.0", SchemaURL: "notstuff.test/", @@ -65,9 +65,9 @@ func TestViewTransformInstrument(t *testing.T) { notMatch: emptyDescription, }, { - name: "Library name", + name: "Scope name", options: []Option{ - MatchInstrumentationLibrary(instrumentation.Library{ + MatchInstrumentationScope(instrumentation.Scope{ Name: "bar", }), }, @@ -75,9 +75,9 @@ func TestViewTransformInstrument(t *testing.T) { notMatch: emptyDescription, }, { - name: "Library version", + name: "Scope version", options: []Option{ - MatchInstrumentationLibrary(instrumentation.Library{ + MatchInstrumentationScope(instrumentation.Scope{ Version: "v1.0.0", }), }, @@ -86,9 +86,9 @@ func TestViewTransformInstrument(t *testing.T) { notMatch: emptyDescription, }, { - name: "Library SchemaURL", + name: "Scope SchemaURL", options: []Option{ - MatchInstrumentationLibrary(instrumentation.Library{ + MatchInstrumentationScope(instrumentation.Scope{ SchemaURL: "stuff.test/", }), }, @@ -107,7 +107,7 @@ func TestViewTransformInstrument(t *testing.T) { name: "composite literal name", options: []Option{ MatchInstrumentName("foo"), - MatchInstrumentationLibrary(instrumentation.Library{ + MatchInstrumentationScope(instrumentation.Scope{ Name: "bar", Version: "v1.0.0", SchemaURL: "stuff.test/", @@ -123,7 +123,7 @@ func TestViewTransformInstrument(t *testing.T) { WithRename("baz"), }, match: Instrument{ - Scope: instrumentation.Library{ + Scope: instrumentation.Scope{ Name: "bar", Version: "v1.0.0", SchemaURL: "stuff.test/", @@ -140,7 +140,7 @@ func TestViewTransformInstrument(t *testing.T) { WithSetDescription("descriptive stuff"), }, match: Instrument{ - Scope: instrumentation.Library{ + Scope: instrumentation.Scope{ Name: "bar", Version: "v1.0.0", SchemaURL: "stuff.test/",