From 5758d9aeecd56cd106d9ecbe55808d1a1edae13a Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Mon, 23 May 2022 07:47:54 -0700 Subject: [PATCH 1/9] Add the Asynchronous and Synchronous state packages for review --- .../sdk/metric/internal/asyncstate/async.go | 142 +++++++++ .../metric/internal/asyncstate/async_test.go | 298 ++++++++++++++++++ .../metric/internal/asyncstate/callback.go | 102 ++++++ .../metric/internal/asyncstate/observer.go | 66 ++++ .../sdk/metric/internal/syncstate/counter.go | 50 +++ .../metric/internal/syncstate/histogram.go | 48 +++ .../internal/syncstate/refcount_mapped.go | 59 ++++ .../sdk/metric/internal/syncstate/sync.go | 189 +++++++++++ 8 files changed, 954 insertions(+) create mode 100644 lightstep/sdk/metric/internal/asyncstate/async.go create mode 100644 lightstep/sdk/metric/internal/asyncstate/async_test.go create mode 100644 lightstep/sdk/metric/internal/asyncstate/callback.go create mode 100644 lightstep/sdk/metric/internal/asyncstate/observer.go create mode 100644 lightstep/sdk/metric/internal/syncstate/counter.go create mode 100644 lightstep/sdk/metric/internal/syncstate/histogram.go create mode 100644 lightstep/sdk/metric/internal/syncstate/refcount_mapped.go create mode 100644 lightstep/sdk/metric/internal/syncstate/sync.go diff --git a/lightstep/sdk/metric/internal/asyncstate/async.go b/lightstep/sdk/metric/internal/asyncstate/async.go new file mode 100644 index 00000000..d5103324 --- /dev/null +++ b/lightstep/sdk/metric/internal/asyncstate/async.go @@ -0,0 +1,142 @@ +// 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. + +package asyncstate // import "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/asyncstate" + +import ( + "context" + "fmt" + "sync" + + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/attribute" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/pipeline" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/viewstate" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/number" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/sdkinstrument" +) + +type ( + // State is the object used to maintain independent collection + // state for each asynchronous meter. + State struct { + // pipe is the pipeline.Register number of this state. + pipe int + + // lock protects against errant use of the instrument + // w/ copied context after the callback returns. + lock sync.Mutex + + // store is a map from instrument to set of values + // observed during one collection. + store map[*Instrument]map[attribute.Set]viewstate.Accumulator + } + + // Instrument is the implementation object associated with one + // asynchronous instrument. + Instrument struct { + // opaque is used to ensure that callbacks are + // registered with instruments from the same provider. + opaque interface{} + + // compiled is the per-pipeline compiled instrument. + compiled pipeline.Register[viewstate.Instrument] + + // descriptor describes the API-level instrument. + // + // Note: Not clear why we need this. It's used for a + // range test, but shouldn't the range test be + // performed by the aggregator? If a View is allowed + // to reconfigure the aggregation in ways that change + // semantics, should the range test be based on the + // aggregation, not the original instrument? + descriptor sdkinstrument.Descriptor + } + + // contextKey is used with context.WithValue() to lookup + // per-reader state from within an executing callback + // function. + contextKey struct{} +) + +func NewState(pipe int) *State { + return &State{ + pipe: pipe, + store: map[*Instrument]map[attribute.Set]viewstate.Accumulator{}, + } +} + +// NewInstrument returns a new Instrument; this compiles individual +// instruments for each reader. +func NewInstrument(desc sdkinstrument.Descriptor, opaque interface{}, compiled pipeline.Register[viewstate.Instrument]) *Instrument { + return &Instrument{ + opaque: opaque, + descriptor: desc, + compiled: compiled, + } +} + +// SnapshotAndProcess calls SnapshotAndProcess() on each of the pending +// aggregations for a given reader. +func (inst *Instrument) SnapshotAndProcess(state *State) { + state.lock.Lock() + defer state.lock.Unlock() + + for _, acc := range state.store[inst] { + acc.SnapshotAndProcess() + } +} + +func (inst *Instrument) get(cs *callbackState, attrs []attribute.KeyValue) viewstate.Accumulator { + comp := inst.compiled[cs.state.pipe] + + cs.state.lock.Lock() + defer cs.state.lock.Unlock() + + aset := attribute.NewSet(attrs...) + imap, has := cs.state.store[inst] + + if !has { + imap = map[attribute.Set]viewstate.Accumulator{} + cs.state.store[inst] = imap + } + + se, has := imap[aset] + if !has { + se = comp.NewAccumulator(aset) + imap[aset] = se + } + return se +} + +func capture[N number.Any, Traits number.Traits[N]](ctx context.Context, inst *Instrument, value N, attrs []attribute.KeyValue) { + lookup := ctx.Value(contextKey{}) + if lookup == nil { + otel.Handle(fmt.Errorf("async instrument used outside of callback")) + return + } + + cs := lookup.(*callbackState) + cb := cs.getCallback() + if cb == nil { + otel.Handle(fmt.Errorf("async instrument used after callback return")) + return + } + if _, ok := cb.instruments[inst]; !ok { + otel.Handle(fmt.Errorf("async instrument not declared for use in callback")) + return + } + + inst.get(cs, attrs).(viewstate.Updater[N]).Update(value) +} diff --git a/lightstep/sdk/metric/internal/asyncstate/async_test.go b/lightstep/sdk/metric/internal/asyncstate/async_test.go new file mode 100644 index 00000000..cf66158e --- /dev/null +++ b/lightstep/sdk/metric/internal/asyncstate/async_test.go @@ -0,0 +1,298 @@ +// 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. + +package asyncstate // import "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/asyncstate" + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/require" + + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/aggregation" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/sum" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/data" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/pipeline" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/test" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/viewstate" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/number" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/sdkinstrument" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/view" + "go.opentelemetry.io/otel/metric/instrument" + "go.opentelemetry.io/otel/sdk/instrumentation" +) + +var ( + testLibrary = instrumentation.Library{ + Name: "test", + } + + endTime = time.Unix(100, 0) + middleTime = endTime.Add(-time.Millisecond) + startTime = endTime.Add(-2 * time.Millisecond) + + testSequence = data.Sequence{ + Start: startTime, + Last: middleTime, + Now: endTime, + } +) + +type testSDK struct { + compiler *viewstate.Compiler +} + +func (tsdk *testSDK) compile(desc sdkinstrument.Descriptor) pipeline.Register[viewstate.Instrument] { + comp, err := tsdk.compiler.Compile(desc) + if err != nil { + panic(err) + } + reg := pipeline.NewRegister[viewstate.Instrument](1) + reg[0] = comp + return reg +} + +func testAsync(name string, opts ...view.Option) *testSDK { + return &testSDK{ + compiler: viewstate.New(testLibrary, view.New(name, opts...)), + } +} + +func testState() *State { + return NewState(0) +} + +func testObserver[N number.Any, Traits number.Traits[N]](tsdk *testSDK, name string, ik sdkinstrument.Kind, opts ...instrument.Option) Observer[N, Traits] { + var t Traits + desc := test.Descriptor(name, ik, t.Kind(), opts...) + impl := NewInstrument(desc, tsdk, tsdk.compile(desc)) + return NewObserver[N, Traits](impl) +} + +func TestNewCallbackError(t *testing.T) { + tsdk := testAsync("test") + + // no instruments error + cb, err := NewCallback(nil, tsdk, nil) + require.Error(t, err) + require.Nil(t, cb) + + // nil callback error + cntr := testObserver[int64, number.Int64Traits](tsdk, "counter", sdkinstrument.CounterObserverKind) + cb, err = NewCallback([]instrument.Asynchronous{cntr}, tsdk, nil) + require.Error(t, err) + require.Nil(t, cb) +} + +func TestNewCallbackProviderMismatch(t *testing.T) { + test0 := testAsync("test0") + test1 := testAsync("test1") + + instA0 := testObserver[int64, number.Int64Traits](test0, "A", sdkinstrument.CounterObserverKind) + instB1 := testObserver[float64, number.Float64Traits](test1, "A", sdkinstrument.CounterObserverKind) + + cb, err := NewCallback([]instrument.Asynchronous{instA0, instB1}, test0, func(context.Context) {}) + require.Error(t, err) + require.Contains(t, err.Error(), "asynchronous instrument belongs to a different meter") + require.Nil(t, cb) + + cb, err = NewCallback([]instrument.Asynchronous{instA0, instB1}, test1, func(context.Context) {}) + require.Error(t, err) + require.Contains(t, err.Error(), "asynchronous instrument belongs to a different meter") + require.Nil(t, cb) + + cb, err = NewCallback([]instrument.Asynchronous{instA0}, test0, func(context.Context) {}) + require.NoError(t, err) + require.NotNil(t, cb) + + cb, err = NewCallback([]instrument.Asynchronous{instB1}, test1, func(context.Context) {}) + require.NoError(t, err) + require.NotNil(t, cb) + + // nil value not of this SDK + var fake0 instrument.Asynchronous + cb, err = NewCallback([]instrument.Asynchronous{fake0}, test0, func(context.Context) {}) + require.Error(t, err) + require.Contains(t, err.Error(), "asynchronous instrument does not belong to this SDK") + require.Nil(t, cb) + + // non-nil value not of this SDK + var fake1 struct { + instrument.Asynchronous + } + cb, err = NewCallback([]instrument.Asynchronous{fake1}, test0, func(context.Context) {}) + require.Error(t, err) + require.Contains(t, err.Error(), "asynchronous instrument does not belong to this SDK") + require.Nil(t, cb) +} + +func TestCallbackInvalidation(t *testing.T) { + errors := test.OTelErrors() + + tsdk := testAsync("test") + + var called int64 + var saveCtx context.Context + + cntr := testObserver[int64, number.Int64Traits](tsdk, "counter", sdkinstrument.CounterObserverKind) + cb, err := NewCallback([]instrument.Asynchronous{cntr}, tsdk, func(ctx context.Context) { + cntr.Observe(ctx, called) + saveCtx = ctx + called++ + }) + require.NoError(t, err) + + state := testState() + + // run the callback once legitimately + cb.Run(context.Background(), state) + + // simulate use after callback return + cntr.Observe(saveCtx, 10000000) + + cntr.inst.SnapshotAndProcess(state) + + require.Equal(t, int64(1), called) + require.Equal(t, 1, len(*errors)) + require.Contains(t, (*errors)[0].Error(), "used after callback return") + + test.RequireEqualMetrics( + t, + test.CollectScope( + t, + tsdk.compiler.Collectors(), + testSequence, + ), + test.Instrument( + cntr.inst.descriptor, + test.Point(startTime, endTime, sum.NewMonotonicInt64(0), aggregation.CumulativeTemporality), + ), + ) +} + +func TestCallbackUndeclaredInstrument(t *testing.T) { + errors := test.OTelErrors() + + tt := testAsync("test") + + var called int64 + + cntr1 := testObserver[int64, number.Int64Traits](tt, "counter1", sdkinstrument.CounterObserverKind) + cntr2 := testObserver[int64, number.Int64Traits](tt, "counter2", sdkinstrument.CounterObserverKind) + + cb, err := NewCallback([]instrument.Asynchronous{cntr1}, tt, func(ctx context.Context) { + cntr2.Observe(ctx, called) + called++ + }) + require.NoError(t, err) + + state := testState() + + // run the callback once legitimately + cb.Run(context.Background(), state) + + cntr1.inst.SnapshotAndProcess(state) + cntr2.inst.SnapshotAndProcess(state) + + require.Equal(t, int64(1), called) + require.Equal(t, 1, len(*errors)) + require.Contains(t, (*errors)[0].Error(), "instrument not declared for use in callback") + + test.RequireEqualMetrics( + t, + test.CollectScope( + t, + tt.compiler.Collectors(), + testSequence, + ), + test.Instrument( + cntr1.inst.descriptor, + ), + test.Instrument( + cntr2.inst.descriptor, + ), + ) +} + +func TestCallbackDroppedInstrument(t *testing.T) { + errors := test.OTelErrors() + + tt := testAsync("test", + view.WithClause( + view.MatchInstrumentName("drop"), + view.WithAggregation(aggregation.DropKind), + ), + ) + + cntrDrop := testObserver[float64, number.Float64Traits](tt, "drop", sdkinstrument.CounterObserverKind) + cntrKeep := testObserver[float64, number.Float64Traits](tt, "keep", sdkinstrument.CounterObserverKind) + + cb, _ := NewCallback([]instrument.Asynchronous{cntrKeep}, tt, func(ctx context.Context) { + cntrDrop.Observe(ctx, 1000) + cntrKeep.Observe(ctx, 1000) + }) + + state := testState() + + cb.Run(context.Background(), state) + + cntrKeep.inst.SnapshotAndProcess(state) + cntrDrop.inst.SnapshotAndProcess(state) + + require.Equal(t, 1, len(*errors)) + require.Contains(t, (*errors)[0].Error(), "instrument not declared for use in callback") + + test.RequireEqualMetrics( + t, + test.CollectScope( + t, + tt.compiler.Collectors(), + testSequence, + ), + test.Instrument( + cntrKeep.inst.descriptor, + test.Point(startTime, endTime, sum.NewMonotonicFloat64(1000), aggregation.CumulativeTemporality), + ), + ) +} + +func TestInstrumentUseOutsideCallback(t *testing.T) { + errors := test.OTelErrors() + + tt := testAsync("test") + + cntr := testObserver[float64, number.Float64Traits](tt, "cntr", sdkinstrument.CounterObserverKind) + + cntr.Observe(context.Background(), 1000) + + state := testState() + + cntr.inst.SnapshotAndProcess(state) + + require.Equal(t, 1, len(*errors)) + require.Contains(t, (*errors)[0].Error(), "async instrument used outside of callback") + + test.RequireEqualMetrics( + t, + test.CollectScope( + t, + tt.compiler.Collectors(), + testSequence, + ), + test.Instrument( + cntr.inst.descriptor, + ), + ) +} diff --git a/lightstep/sdk/metric/internal/asyncstate/callback.go b/lightstep/sdk/metric/internal/asyncstate/callback.go new file mode 100644 index 00000000..046ed29f --- /dev/null +++ b/lightstep/sdk/metric/internal/asyncstate/callback.go @@ -0,0 +1,102 @@ +// 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. + +package asyncstate // import "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/asyncstate" + +import ( + "context" + "fmt" + "sync" + + "go.opentelemetry.io/otel/metric/instrument" +) + +// Callback is the implementation object associated with one +// asynchronous callback. +type Callback struct { + // function is the user-provided callback function. + function func(context.Context) + + // instruments are the instruments permitted to be + // used inside this callback. + instruments map[*Instrument]struct{} +} + +// NewCallback returns a new Callback; this checks that each of the +// provided instruments belongs to the same meter provider. +func NewCallback(instruments []instrument.Asynchronous, opaque interface{}, function func(context.Context)) (*Callback, error) { + if len(instruments) == 0 { + return nil, fmt.Errorf("asynchronous callback without instruments") + } + if function == nil { + return nil, fmt.Errorf("asynchronous callback with nil function") + } + + cb := &Callback{ + function: function, + instruments: map[*Instrument]struct{}{}, + } + + for _, inst := range instruments { + ai, ok := inst.(memberInstrument) + if !ok { + return nil, fmt.Errorf("asynchronous instrument does not belong to this SDK: %T", inst) + } + thisInst := ai.instrument() + if thisInst.opaque != opaque { + return nil, fmt.Errorf("asynchronous instrument belongs to a different meter") + } + + cb.instruments[thisInst] = struct{}{} + } + + return cb, nil +} + +// Run executes the callback after setting up the appropriate context +// for a specific reader. +func (c *Callback) Run(ctx context.Context, state *State) { + cp := &callbackState{ + callback: c, + state: state, + } + c.function(context.WithValue(ctx, contextKey{}, cp)) + cp.invalidate() +} + +// callbackState is used to lookup the current callback and +// pipeline from within an executing callback function. +type callbackState struct { + // lock protects callback, see invalidate() and getCallback() + lock sync.Mutex + + // callback is the currently running callback; this is set to nil + // after the associated callback function returns. + callback *Callback + + // state is a single collection of data. + state *State +} + +func (cp *callbackState) invalidate() { + cp.lock.Lock() + defer cp.lock.Unlock() + cp.callback = nil +} + +func (cp *callbackState) getCallback() *Callback { + cp.lock.Lock() + defer cp.lock.Unlock() + return cp.callback +} diff --git a/lightstep/sdk/metric/internal/asyncstate/observer.go b/lightstep/sdk/metric/internal/asyncstate/observer.go new file mode 100644 index 00000000..a746a6df --- /dev/null +++ b/lightstep/sdk/metric/internal/asyncstate/observer.go @@ -0,0 +1,66 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package asyncstate // import "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/asyncstate" + +import ( + "context" + + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/number" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/metric/instrument" + "go.opentelemetry.io/otel/metric/instrument/asyncfloat64" + "go.opentelemetry.io/otel/metric/instrument/asyncint64" +) + +// Observer is a generic (int64 or float64) instrument which +// satisfies any of the asynchronous instrument API interfaces. +type Observer[N number.Any, Traits number.Traits[N]] struct { + instrument.Asynchronous // Note: wasted space + + inst *Instrument +} + +// Observer implements 6 instruments and memberInstrument. +var ( + _ asyncint64.Counter = Observer[int64, number.Int64Traits]{} + _ asyncint64.UpDownCounter = Observer[int64, number.Int64Traits]{} + _ asyncint64.Gauge = Observer[int64, number.Int64Traits]{} + _ memberInstrument = Observer[int64, number.Int64Traits]{} + + _ asyncfloat64.Counter = Observer[float64, number.Float64Traits]{} + _ asyncfloat64.UpDownCounter = Observer[float64, number.Float64Traits]{} + _ asyncfloat64.Gauge = Observer[float64, number.Float64Traits]{} + _ memberInstrument = Observer[float64, number.Float64Traits]{} +) + +// memberInstrument indicates whether a user-provided +// instrument was returned by this SDK. +type memberInstrument interface { + instrument() *Instrument +} + +// NewObserver returns an generic value suitable for use as any of the +// asynchronous instrument APIs. +func NewObserver[N number.Any, Traits number.Traits[N]](inst *Instrument) Observer[N, Traits] { + return Observer[N, Traits]{inst: inst} +} + +func (o Observer[N, Traits]) instrument() *Instrument { + return o.inst +} + +func (o Observer[N, Traits]) Observe(ctx context.Context, value N, attrs ...attribute.KeyValue) { + capture[N, Traits](ctx, o.inst, value, attrs) +} diff --git a/lightstep/sdk/metric/internal/syncstate/counter.go b/lightstep/sdk/metric/internal/syncstate/counter.go new file mode 100644 index 00000000..66fcfb84 --- /dev/null +++ b/lightstep/sdk/metric/internal/syncstate/counter.go @@ -0,0 +1,50 @@ +// 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. + +package syncstate // import "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/syncstate" + +import ( + "context" + + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/number" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/metric/instrument" + "go.opentelemetry.io/otel/metric/instrument/syncfloat64" + "go.opentelemetry.io/otel/metric/instrument/syncint64" +) + +// Counter is a synchronous instrument having an Add() method. +type Counter[N number.Any, Traits number.Traits[N]] struct { + instrument.Synchronous // Note: wasted space + + inst *Instrument +} + +// Counter satisfies 4 instrument APIs. +var ( + _ syncint64.Counter = Counter[int64, number.Int64Traits]{} + _ syncint64.UpDownCounter = Counter[int64, number.Int64Traits]{} + _ syncfloat64.Counter = Counter[float64, number.Float64Traits]{} + _ syncfloat64.UpDownCounter = Counter[float64, number.Float64Traits]{} +) + +// NewCounter returns a value that implements the Counter and UpDownCounter APIs. +func NewCounter[N number.Any, Traits number.Traits[N]](inst *Instrument) Counter[N, Traits] { + return Counter[N, Traits]{inst: inst} +} + +// Add increments a Counter or UpDownCounter. +func (c Counter[N, Traits]) Add(ctx context.Context, incr N, attrs ...attribute.KeyValue) { + capture[N, Traits](ctx, c.inst, incr, attrs) +} diff --git a/lightstep/sdk/metric/internal/syncstate/histogram.go b/lightstep/sdk/metric/internal/syncstate/histogram.go new file mode 100644 index 00000000..a35b90e3 --- /dev/null +++ b/lightstep/sdk/metric/internal/syncstate/histogram.go @@ -0,0 +1,48 @@ +// 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. + +package syncstate // import "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/syncstate" + +import ( + "context" + + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/number" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/metric/instrument" + "go.opentelemetry.io/otel/metric/instrument/syncfloat64" + "go.opentelemetry.io/otel/metric/instrument/syncint64" +) + +// Histogram is a synchronous instrument having a Record() method. +type Histogram[N number.Any, Traits number.Traits[N]] struct { + instrument.Synchronous // Note: wasted space + + inst *Instrument +} + +// Histogram satisfies 2 instrument APIs. +var ( + _ syncint64.Histogram = Histogram[int64, number.Int64Traits]{} + _ syncfloat64.Histogram = Histogram[float64, number.Float64Traits]{} +) + +// NewCounter returns a value that implements the Histogram API. +func NewHistogram[N number.Any, Traits number.Traits[N]](inst *Instrument) Histogram[N, Traits] { + return Histogram[N, Traits]{inst: inst} +} + +// Record records a Histogram observation. +func (h Histogram[N, Traits]) Record(ctx context.Context, incr N, attrs ...attribute.KeyValue) { + capture[N, Traits](ctx, h.inst, incr, attrs) +} diff --git a/lightstep/sdk/metric/internal/syncstate/refcount_mapped.go b/lightstep/sdk/metric/internal/syncstate/refcount_mapped.go new file mode 100644 index 00000000..d8b2dcaa --- /dev/null +++ b/lightstep/sdk/metric/internal/syncstate/refcount_mapped.go @@ -0,0 +1,59 @@ +// 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. + +package syncstate + +import ( + "sync/atomic" +) + +// refcountMapped atomically counts the number of references (usages) of an entry +// while also keeping a state of mapped/unmapped into a different data structure +// (an external map or list for example). +// +// refcountMapped uses an atomic value where the least significant bit is used to +// keep the state of mapping ('1' is used for unmapped and '0' is for mapped) and +// the rest of the bits are used for refcounting. +type refcountMapped struct { + // refcount has to be aligned for 64-bit atomic operations. + value int64 +} + +// ref returns true if the entry is still mapped and increases the +// reference usages, if unmapped returns false. +func (rm *refcountMapped) ref() bool { + // Check if this entry was marked as unmapped between the moment + // we got a reference to it (or will be removed very soon) and here. + return atomic.AddInt64(&rm.value, 2)&1 == 0 +} + +func (rm *refcountMapped) unref() { + atomic.AddInt64(&rm.value, -2) +} + +// tryUnmap flips the mapped bit to "unmapped" state and returns true if both of the +// following conditions are true upon entry to this function: +// * There are no active references; +// * The mapped bit is in "mapped" state. +// Otherwise no changes are done to mapped bit and false is returned. +func (rm *refcountMapped) tryUnmap() bool { + if atomic.LoadInt64(&rm.value) != 0 { + return false + } + return atomic.CompareAndSwapInt64( + &rm.value, + 0, + 1, + ) +} diff --git a/lightstep/sdk/metric/internal/syncstate/sync.go b/lightstep/sdk/metric/internal/syncstate/sync.go new file mode 100644 index 00000000..b7fdc752 --- /dev/null +++ b/lightstep/sdk/metric/internal/syncstate/sync.go @@ -0,0 +1,189 @@ +// 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. + +package syncstate + +import ( + "context" + "runtime" + "sync" + "sync/atomic" + + "go.opentelemetry.io/otel/attribute" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/pipeline" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/viewstate" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/number" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/sdkinstrument" +) + +// Instrument maintains a mapping from attribute.Set to an internal +// record type for a single API-level instrument. This type is +// organized so that a single attribute.Set lookup is performed +// regardless of the number of reader and instrument-view behaviors. +// Entries in the map have their accumulator's SnapshotAndProcess() +// method called whenever they are removed from the map, which can +// happen when any reader collects the instrument. +type Instrument struct { + // descriptor is the API-provided descriptor for the + // instrument, unmodified by views. + descriptor sdkinstrument.Descriptor + + // compiled will be a single compiled instrument or a + // multi-instrument in case of multiple view behaviors + // and/or readers; these distinctions do not matter + // for synchronous aggregation. + compiled viewstate.Instrument + + // current is a synchronous form of map[attribute.Set]*record. + current sync.Map +} + +// NewInstruments builds a new synchronous instrument given the +// per-pipeline instrument-views compiled. Note that the unused +// second parameter is an opaque value used in the asyncstate package, +// passed here to make these two packages generalize. +func NewInstrument(desc sdkinstrument.Descriptor, _ interface{}, compiled pipeline.Register[viewstate.Instrument]) *Instrument { + return &Instrument{ + descriptor: desc, + + // Note that viewstate.Combine is used to eliminate + // the per-pipeline distinction that is useful in the + // asyncstate package. Here, in the common case there + // will be one pipeline and one view, such that + // viewstate.Combine produces a single concrete + // viewstate.Instrument. Only when there are multiple + // views or multiple pipelines will the combination + // produce a viewstate.multiInstrment here. + compiled: viewstate.Combine(desc, compiled...), + } +} + +// SnapshotAndProcess calls SnapshotAndProcess() for all live +// accumulators of this instrument. Inactive accumulators will be +// subsequently removed from the map. +func (inst *Instrument) SnapshotAndProcess() { + inst.current.Range(func(key interface{}, value interface{}) bool { + rec := value.(*record) + if rec.snapshotAndProcess() { + return true + } + // Having no updates since last collection, try to unmap: + if unmapped := rec.refMapped.tryUnmap(); !unmapped { + // The record is still referenced, continue. + return true + } + + // If any other goroutines are now trying to re-insert this + // entry in the map, they are busy calling Gosched() awaiting + // this deletion: + inst.current.Delete(key) + + // Last we'll see of this. + _ = rec.snapshotAndProcess() + return true + }) +} + +// record consists of an accumulator, a reference count, the number of +// updates, and the number of collected updates. +type record struct { + refMapped refcountMapped + + // updateCount is incremented on every Update. + updateCount int64 + + // collectedCount is set to updateCount on collection, + // supports checking for no updates during a round. + collectedCount int64 + + // accumulator is can be a multi-accumulator if there + // are multiple behaviors or multiple readers, but + // these distinctions are not relevant for synchronous + // instruments. + accumulator viewstate.Accumulator +} + +// snapshotAndProcessRecord checks whether the accumulator has been +// modified since the last collection (by any reader), returns a +// boolean indicating whether the record is active. If active, calls +// SnapshotAndProcess on the associated accumulator and returns true. +// If updates happened since the last collection (by any reader), +// returns false. +func (rec *record) snapshotAndProcess() bool { + mods := atomic.LoadInt64(&rec.updateCount) + coll := rec.collectedCount + + if mods == coll { + return false + } + // Updates happened in this interval, collect and continue. + rec.collectedCount = mods + + rec.accumulator.SnapshotAndProcess() + return true +} + +// capture performs a single update for any synchronous instrument. +func capture[N number.Any, Traits number.Traits[N]](_ context.Context, inst *Instrument, num N, attrs []attribute.KeyValue) { + if inst.compiled == nil { + return + } + + // Note: Here, this is the place to use context, e.g., extract baggage. + + rec, updater := acquireRecord[N](inst, attrs) + defer rec.refMapped.unref() + + updater.Update(num) + + // Record was modified. + atomic.AddInt64(&rec.updateCount, 1) +} + +// acquireRecord gets or creates a `*record` corresponding to `attrs`, +// the input attributes. +func acquireRecord[N number.Any](inst *Instrument, attrs []attribute.KeyValue) (*record, viewstate.Updater[N]) { + aset := attribute.NewSet(attrs...) + if lookup, ok := inst.current.Load(aset); ok { + // Existing record case. + rec := lookup.(*record) + + if rec.refMapped.ref() { + // At this moment it is guaranteed that the + // record is in the map and will not be removed. + return rec, rec.accumulator.(viewstate.Updater[N]) + } + // This record is no longer mapped, try to add a new + // record below. + } + + newRec := &record{ + refMapped: refcountMapped{value: 2}, + } + + for { + if found, loaded := inst.current.LoadOrStore(aset, newRec); loaded { + oldRec := found.(*record) + if oldRec.refMapped.ref() { + return oldRec, oldRec.accumulator.(viewstate.Updater[N]) + } + runtime.Gosched() + continue + } + break + } + + newRec.accumulator = inst.compiled.NewAccumulator(aset) + return newRec, newRec.accumulator.(viewstate.Updater[N]) +} From 75124167cda440a1a58ef392d089f7b2f5d0edc6 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Thu, 2 Jun 2022 23:52:27 -0700 Subject: [PATCH 2/9] more testing; a bug identified --- lightstep/sdk/metric/asyncinst.go | 59 ++++ lightstep/sdk/metric/asyncinst_test.go | 92 ++++++ lightstep/sdk/metric/benchmark_test.go | 147 +++++++++ lightstep/sdk/metric/config.go | 65 ++++ lightstep/sdk/metric/instrument.go | 103 ++++++ .../sdk/metric/internal/asyncstate/async.go | 6 +- .../sdk/metric/internal/syncstate/sync.go | 16 +- .../metric/internal/syncstate/sync_test.go | 178 ++++++++++ lightstep/sdk/metric/manual.go | 66 ++++ lightstep/sdk/metric/meter.go | 85 +++++ lightstep/sdk/metric/periodic.go | 121 +++++++ lightstep/sdk/metric/producer.go | 120 +++++++ lightstep/sdk/metric/provider.go | 169 ++++++++++ lightstep/sdk/metric/provider_test.go | 304 ++++++++++++++++++ lightstep/sdk/metric/reader.go | 65 ++++ lightstep/sdk/metric/syncinst.go | 59 ++++ lightstep/sdk/metric/syncinst_test.go | 110 +++++++ 17 files changed, 1757 insertions(+), 8 deletions(-) create mode 100644 lightstep/sdk/metric/asyncinst.go create mode 100644 lightstep/sdk/metric/asyncinst_test.go create mode 100644 lightstep/sdk/metric/benchmark_test.go create mode 100644 lightstep/sdk/metric/config.go create mode 100644 lightstep/sdk/metric/instrument.go create mode 100644 lightstep/sdk/metric/internal/syncstate/sync_test.go create mode 100644 lightstep/sdk/metric/manual.go create mode 100644 lightstep/sdk/metric/meter.go create mode 100644 lightstep/sdk/metric/periodic.go create mode 100644 lightstep/sdk/metric/producer.go create mode 100644 lightstep/sdk/metric/provider.go create mode 100644 lightstep/sdk/metric/provider_test.go create mode 100644 lightstep/sdk/metric/reader.go create mode 100644 lightstep/sdk/metric/syncinst.go create mode 100644 lightstep/sdk/metric/syncinst_test.go diff --git a/lightstep/sdk/metric/asyncinst.go b/lightstep/sdk/metric/asyncinst.go new file mode 100644 index 00000000..a59fcf8d --- /dev/null +++ b/lightstep/sdk/metric/asyncinst.go @@ -0,0 +1,59 @@ +// 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. + +package metric // import "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric" + +import ( + "go.opentelemetry.io/otel/metric/instrument" + "go.opentelemetry.io/otel/metric/instrument/asyncfloat64" + "go.opentelemetry.io/otel/metric/instrument/asyncint64" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/asyncstate" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/number" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/sdkinstrument" +) + +type ( + asyncint64Instruments struct{ *meter } + asyncfloat64Instruments struct{ *meter } +) + +func (i asyncint64Instruments) Counter(name string, opts ...instrument.Option) (asyncint64.Counter, error) { + inst, err := i.asynchronousInstrument(name, opts, number.Int64Kind, sdkinstrument.CounterObserverKind) + return asyncstate.NewObserver[int64, number.Int64Traits](inst), err +} + +func (i asyncint64Instruments) UpDownCounter(name string, opts ...instrument.Option) (asyncint64.UpDownCounter, error) { + inst, err := i.asynchronousInstrument(name, opts, number.Int64Kind, sdkinstrument.UpDownCounterObserverKind) + return asyncstate.NewObserver[int64, number.Int64Traits](inst), err +} + +func (i asyncint64Instruments) Gauge(name string, opts ...instrument.Option) (asyncint64.Gauge, error) { + inst, err := i.asynchronousInstrument(name, opts, number.Int64Kind, sdkinstrument.GaugeObserverKind) + return asyncstate.NewObserver[int64, number.Int64Traits](inst), err +} + +func (f asyncfloat64Instruments) Counter(name string, opts ...instrument.Option) (asyncfloat64.Counter, error) { + inst, err := f.asynchronousInstrument(name, opts, number.Float64Kind, sdkinstrument.CounterObserverKind) + return asyncstate.NewObserver[float64, number.Float64Traits](inst), err +} + +func (f asyncfloat64Instruments) UpDownCounter(name string, opts ...instrument.Option) (asyncfloat64.UpDownCounter, error) { + inst, err := f.asynchronousInstrument(name, opts, number.Float64Kind, sdkinstrument.UpDownCounterObserverKind) + return asyncstate.NewObserver[float64, number.Float64Traits](inst), err +} + +func (f asyncfloat64Instruments) Gauge(name string, opts ...instrument.Option) (asyncfloat64.Gauge, error) { + inst, err := f.asynchronousInstrument(name, opts, number.Float64Kind, sdkinstrument.GaugeObserverKind) + return asyncstate.NewObserver[float64, number.Float64Traits](inst), err +} diff --git a/lightstep/sdk/metric/asyncinst_test.go b/lightstep/sdk/metric/asyncinst_test.go new file mode 100644 index 00000000..8b7b62d6 --- /dev/null +++ b/lightstep/sdk/metric/asyncinst_test.go @@ -0,0 +1,92 @@ +// 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. + +package metric + +import ( + "context" + "testing" + "time" + + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/aggregation" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/gauge" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/sum" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/test" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/number" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/sdkinstrument" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/metric/instrument" + "go.opentelemetry.io/otel/sdk/resource" +) + +func TestAsyncInsts(t *testing.T) { + rdr := NewManualReader("test") + res := resource.Empty() + provider := NewMeterProvider(WithReader(rdr), WithResource(res)) + + ci := must(provider.Meter("test").AsyncInt64().Counter("icount")) + cf := must(provider.Meter("test").AsyncFloat64().Counter("fcount")) + ui := must(provider.Meter("test").AsyncInt64().UpDownCounter("iupcount")) + uf := must(provider.Meter("test").AsyncFloat64().UpDownCounter("fupcount")) + gi := must(provider.Meter("test").AsyncInt64().Gauge("igauge")) + gf := must(provider.Meter("test").AsyncFloat64().Gauge("fgauge")) + + attr := attribute.String("a", "B") + + _ = provider.Meter("test").RegisterCallback([]instrument.Asynchronous{ + ci, cf, ui, uf, gi, gf, + }, func(ctx context.Context) { + ci.Observe(ctx, 2, attr) + cf.Observe(ctx, 3, attr) + ui.Observe(ctx, 4, attr) + uf.Observe(ctx, 5, attr) + gi.Observe(ctx, 6, attr) + gf.Observe(ctx, 7, attr) + }) + + data := rdr.Produce(nil) + notime := time.Time{} + cumulative := aggregation.CumulativeTemporality + + test.RequireEqualResourceMetrics( + t, data, res, + test.Scope( + test.Library("test"), + test.Instrument( + test.Descriptor("icount", sdkinstrument.CounterObserverKind, number.Int64Kind), + test.Point(notime, notime, sum.NewMonotonicInt64(2), cumulative, attr), + ), + test.Instrument( + test.Descriptor("fcount", sdkinstrument.CounterObserverKind, number.Float64Kind), + test.Point(notime, notime, sum.NewMonotonicFloat64(3), cumulative, attr), + ), + test.Instrument( + test.Descriptor("iupcount", sdkinstrument.UpDownCounterObserverKind, number.Int64Kind), + test.Point(notime, notime, sum.NewNonMonotonicInt64(4), cumulative, attr), + ), + test.Instrument( + test.Descriptor("fupcount", sdkinstrument.UpDownCounterObserverKind, number.Float64Kind), + test.Point(notime, notime, sum.NewNonMonotonicFloat64(5), cumulative, attr), + ), + test.Instrument( + test.Descriptor("igauge", sdkinstrument.GaugeObserverKind, number.Int64Kind), + test.Point(notime, notime, gauge.NewInt64(6), cumulative, attr), + ), + test.Instrument( + test.Descriptor("fgauge", sdkinstrument.GaugeObserverKind, number.Float64Kind), + test.Point(notime, notime, gauge.NewFloat64(7), cumulative, attr), + ), + ), + ) +} diff --git a/lightstep/sdk/metric/benchmark_test.go b/lightstep/sdk/metric/benchmark_test.go new file mode 100644 index 00000000..e7b039a8 --- /dev/null +++ b/lightstep/sdk/metric/benchmark_test.go @@ -0,0 +1,147 @@ +// 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. + +package metric // import "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric" + +import ( + "context" + "testing" + + "go.opentelemetry.io/otel/attribute" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/data" +) + +func BenchmarkCounterAddNoAttrs(b *testing.B) { + ctx := context.Background() + rdr := NewManualReader("bench") + provider := NewMeterProvider(WithReader(rdr)) + b.ReportAllocs() + + cntr, _ := provider.Meter("test").SyncInt64().Counter("hello") + + for i := 0; i < b.N; i++ { + cntr.Add(ctx, 1) + } +} + +// Benchmark prints 3 allocs per Add(): +// 1. new []attribute.KeyValue for the list of attributes +// 2. interface{} wrapper around attribute.Set +// 3. an attribute array (map key) +func BenchmarkCounterAddOneAttr(b *testing.B) { + ctx := context.Background() + rdr := NewManualReader("bench") + provider := NewMeterProvider(WithReader(rdr)) + b.ReportAllocs() + + cntr, _ := provider.Meter("test").SyncInt64().Counter("hello") + + for i := 0; i < b.N; i++ { + cntr.Add(ctx, 1, attribute.String("K", "V")) + } +} + +// Benchmark prints 11 allocs per Add(), I see 10 in the profile: +// 1. new []attribute.KeyValue for the list of attributes +// 2. an attribute.Sortable (acquireRecord) +// 3. the attribute.Set underlying array +// 4. interface{} wrapper around attribute.Set value +// 5. internal to sync.Map +// 6. internal sync.Map +// 7. new syncstate.record +// 8. new viewstate.syncAccumulator +// 9. an attribute.Sortable (findOutput) +// 10. an output Aggregator +func BenchmarkCounterAddManyAttrs(b *testing.B) { + ctx := context.Background() + rdr := NewManualReader("bench") + provider := NewMeterProvider(WithReader(rdr)) + b.ReportAllocs() + + cntr, _ := provider.Meter("test").SyncInt64().Counter("hello") + + for i := 0; i < b.N; i++ { + cntr.Add(ctx, 1, attribute.Int("K", i)) + } +} + +func BenchmarkCounterCollectOneAttrNoReuse(b *testing.B) { + ctx := context.Background() + rdr := NewManualReader("bench") + provider := NewMeterProvider(WithReader(rdr)) + b.ReportAllocs() + + cntr, _ := provider.Meter("test").SyncInt64().Counter("hello") + + for i := 0; i < b.N; i++ { + cntr.Add(ctx, 1, attribute.Int("K", 1)) + + _ = rdr.Produce(nil) + } +} + +func BenchmarkCounterCollectOneAttrWithReuse(b *testing.B) { + ctx := context.Background() + rdr := NewManualReader("bench") + provider := NewMeterProvider(WithReader(rdr)) + b.ReportAllocs() + + cntr, _ := provider.Meter("test").SyncInt64().Counter("hello") + + var reuse data.Metrics + + for i := 0; i < b.N; i++ { + cntr.Add(ctx, 1, attribute.Int("K", 1)) + + reuse = rdr.Produce(&reuse) + } +} + +func BenchmarkCounterCollectTenAttrs(b *testing.B) { + ctx := context.Background() + rdr := NewManualReader("bench") + provider := NewMeterProvider(WithReader(rdr)) + b.ReportAllocs() + + cntr, _ := provider.Meter("test").SyncInt64().Counter("hello") + + var reuse data.Metrics + + for i := 0; i < b.N; i++ { + for j := 0; j < 10; j++ { + cntr.Add(ctx, 1, attribute.Int("K", j)) + } + reuse = rdr.Produce(&reuse) + } +} + +func BenchmarkCounterCollectTenAttrsTenTimes(b *testing.B) { + ctx := context.Background() + rdr := NewManualReader("bench") + provider := NewMeterProvider(WithReader(rdr)) + b.ReportAllocs() + + cntr, _ := provider.Meter("test").SyncInt64().Counter("hello") + + var reuse data.Metrics + + for i := 0; i < b.N; i++ { + for k := 0; k < 10; k++ { + for j := 0; j < 10; j++ { + cntr.Add(ctx, 1, attribute.Int("K", j)) + } + reuse = rdr.Produce(&reuse) + } + } +} diff --git a/lightstep/sdk/metric/config.go b/lightstep/sdk/metric/config.go new file mode 100644 index 00000000..8ce4203c --- /dev/null +++ b/lightstep/sdk/metric/config.go @@ -0,0 +1,65 @@ +// 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. + +package metric // import "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric" + +import ( + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/view" + "go.opentelemetry.io/otel/sdk/resource" +) + +// config contains configuration options for a MeterProvider. +type config struct { + // res is the resource for this MeterProvider. + res *resource.Resource + + // readers is a slice of Reader instances corresponding with views. + // the i'th reader uses the i'th entry in views. + readers []Reader + + // views is a slice of *Views instances corresponding with readers. + // the i'th views applies to the i'th reader. + views []*view.Views +} + +// Option applies a configuration option value to a MeterProvider. +type Option interface { + apply(config) config +} + +// optionFunction makes a functional Option out of a function object. +type optionFunction func(cfg config) config + +// apply implements Option. +func (of optionFunction) apply(in config) config { + return of(in) +} + +// WithResource associates a Resource with a new MeterProvider. +func WithResource(res *resource.Resource) Option { + return optionFunction(func(cfg config) config { + cfg.res = res + return cfg + }) +} + +// WithReader associates a new Reader and associated View options with +// a new MeterProvider +func WithReader(r Reader, opts ...view.Option) Option { + return optionFunction(func(cfg config) config { + cfg.readers = append(cfg.readers, r) + cfg.views = append(cfg.views, view.New(r.String(), opts...)) + return cfg + }) +} diff --git a/lightstep/sdk/metric/instrument.go b/lightstep/sdk/metric/instrument.go new file mode 100644 index 00000000..f66bf99f --- /dev/null +++ b/lightstep/sdk/metric/instrument.go @@ -0,0 +1,103 @@ +// 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. + +package metric // import "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric" + +import ( + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/metric/instrument" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/asyncstate" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/pipeline" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/syncstate" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/viewstate" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/number" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/sdkinstrument" +) + +// instrumentConstructor refers to either the syncstate or asyncstate +// NewInstrument method. Although both receive an opaque interface{} +// to distinguish providers, only the asyncstate package needs to know +// this information. The unused parameter is passed to the syncstate +// package for the generalization used here to work. +type instrumentConstructor[T any] func( + instrument sdkinstrument.Descriptor, + opaque interface{}, + compiled pipeline.Register[viewstate.Instrument], +) T + +// configureInstrument applies the instrument configuration, checks +// for an existing definition for the same descriptor, and compiles +// and constructs the instrument if necessary. +func configureInstrument[T any]( + m *meter, + name string, + opts []instrument.Option, + nk number.Kind, + ik sdkinstrument.Kind, + listPtr *[]T, + ctor instrumentConstructor[T], +) (T, error) { + // Compute the instrument descriptor + cfg := instrument.NewConfig(opts...) + desc := sdkinstrument.NewDescriptor(name, ik, nk, cfg.Description(), cfg.Unit()) + + m.lock.Lock() + defer m.lock.Unlock() + + // Lookup a pre-existing instrument by descriptor. + if lookup, has := m.byDesc[desc]; has { + // Recompute conflicts since they may have changed. + var conflicts viewstate.ViewConflictsBuilder + + for _, compiler := range m.compilers { + _, err := compiler.Compile(desc) + conflicts.Combine(err) + } + + return lookup.(T), conflicts.AsError() + } + + // Compile the instrument for each pipeline. the first time. + var conflicts viewstate.ViewConflictsBuilder + compiled := pipeline.NewRegister[viewstate.Instrument](len(m.compilers)) + + for pipe, compiler := range m.compilers { + comp, err := compiler.Compile(desc) + compiled[pipe] = comp + conflicts.Combine(err) + } + + // Build the new instrument, cache it, append to the list. + inst := ctor(desc, m, compiled) + err := conflicts.AsError() + + m.byDesc[desc] = inst + *listPtr = append(*listPtr, inst) + if err != nil { + // Handle instrument creation errors when they're new, + // not for repeat entries above. + otel.Handle(err) + } + return inst, err +} + +// synchronousInstrument configures a synchronous instrument. +func (m *meter) synchronousInstrument(name string, opts []instrument.Option, nk number.Kind, ik sdkinstrument.Kind) (*syncstate.Instrument, error) { + return configureInstrument(m, name, opts, nk, ik, &m.syncInsts, syncstate.NewInstrument) +} + +// synchronousInstrument configures an asynchronous instrument. +func (m *meter) asynchronousInstrument(name string, opts []instrument.Option, nk number.Kind, ik sdkinstrument.Kind) (*asyncstate.Instrument, error) { + return configureInstrument(m, name, opts, nk, ik, &m.asyncInsts, asyncstate.NewInstrument) +} diff --git a/lightstep/sdk/metric/internal/asyncstate/async.go b/lightstep/sdk/metric/internal/asyncstate/async.go index d5103324..9ef58e9d 100644 --- a/lightstep/sdk/metric/internal/asyncstate/async.go +++ b/lightstep/sdk/metric/internal/asyncstate/async.go @@ -19,12 +19,12 @@ import ( "fmt" "sync" - "go.opentelemetry.io/otel" - "go.opentelemetry.io/otel/attribute" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/pipeline" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/viewstate" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/number" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/sdkinstrument" + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/attribute" ) type ( @@ -101,6 +101,8 @@ func (inst *Instrument) SnapshotAndProcess(state *State) { func (inst *Instrument) get(cs *callbackState, attrs []attribute.KeyValue) viewstate.Accumulator { comp := inst.compiled[cs.state.pipe] + // @@@ nil check? + cs.state.lock.Lock() defer cs.state.lock.Unlock() diff --git a/lightstep/sdk/metric/internal/syncstate/sync.go b/lightstep/sdk/metric/internal/syncstate/sync.go index b7fdc752..1bda1a30 100644 --- a/lightstep/sdk/metric/internal/syncstate/sync.go +++ b/lightstep/sdk/metric/internal/syncstate/sync.go @@ -20,11 +20,11 @@ import ( "sync" "sync/atomic" - "go.opentelemetry.io/otel/attribute" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/pipeline" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/viewstate" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/number" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/sdkinstrument" + "go.opentelemetry.io/otel/attribute" ) // Instrument maintains a mapping from attribute.Set to an internal @@ -107,7 +107,7 @@ type record struct { // supports checking for no updates during a round. collectedCount int64 - // accumulator is can be a multi-accumulator if there + // accumulator can be a multi-accumulator if there // are multiple behaviors or multiple readers, but // these distinctions are not relevant for synchronous // instruments. @@ -122,13 +122,13 @@ type record struct { // returns false. func (rec *record) snapshotAndProcess() bool { mods := atomic.LoadInt64(&rec.updateCount) - coll := rec.collectedCount + coll := atomic.LoadInt64(&rec.collectedCount) if mods == coll { return false } // Updates happened in this interval, collect and continue. - rec.collectedCount = mods + atomic.StoreInt64(&rec.collectedCount, mods) rec.accumulator.SnapshotAndProcess() return true @@ -155,6 +155,7 @@ func capture[N number.Any, Traits number.Traits[N]](_ context.Context, inst *Ins // the input attributes. func acquireRecord[N number.Any](inst *Instrument, attrs []attribute.KeyValue) (*record, viewstate.Updater[N]) { aset := attribute.NewSet(attrs...) + if lookup, ok := inst.current.Load(aset); ok { // Existing record case. rec := lookup.(*record) @@ -168,8 +169,12 @@ func acquireRecord[N number.Any](inst *Instrument, attrs []attribute.KeyValue) ( // record below. } + // Note: the accumulator set below is created speculatively; + // if it is never returned, it will not be updated and can be + // safely discarded. newRec := &record{ - refMapped: refcountMapped{value: 2}, + refMapped: refcountMapped{value: 2}, + accumulator: inst.compiled.NewAccumulator(aset), } for { @@ -184,6 +189,5 @@ func acquireRecord[N number.Any](inst *Instrument, attrs []attribute.KeyValue) ( break } - newRec.accumulator = inst.compiled.NewAccumulator(aset) return newRec, newRec.accumulator.(viewstate.Updater[N]) } diff --git a/lightstep/sdk/metric/internal/syncstate/sync_test.go b/lightstep/sdk/metric/internal/syncstate/sync_test.go new file mode 100644 index 00000000..ecd8c40c --- /dev/null +++ b/lightstep/sdk/metric/internal/syncstate/sync_test.go @@ -0,0 +1,178 @@ +// 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. + +package syncstate + +import ( + "context" + "math/rand" + "sync" + "testing" + "time" + + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/aggregation" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/sum" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/data" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/pipeline" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/test" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/viewstate" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/number" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/sdkinstrument" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/view" + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/sdk/instrumentation" +) + +func deltaUpdate(old, new int64) int64 { + return old + new +} + +func cumulativeUpdate(_, new int64) int64 { + return new +} + +func TestSyncStateDeltaConcurrency1(t *testing.T) { + testSyncStateConcurrency(t, 1, aggregation.DeltaTemporality, deltaUpdate) +} + +func TestSyncStateDeltaConcurrency2(t *testing.T) { + testSyncStateConcurrency(t, 2, aggregation.DeltaTemporality, deltaUpdate) +} + +func TestSyncStateCumulativeConcurrency1(t *testing.T) { + testSyncStateConcurrency(t, 1, aggregation.CumulativeTemporality, cumulativeUpdate) +} + +func TestSyncStateCumulativeConcurrency2(t *testing.T) { + testSyncStateConcurrency(t, 2, aggregation.CumulativeTemporality, cumulativeUpdate) +} + +// TODO: Add float64, Histogram tests. +func testSyncStateConcurrency(t *testing.T, numReaders int, tempo aggregation.Temporality, update func(old, new int64) int64) { + const ( + numRoutines = 10 + numAttrs = 10 + numUpdates = 1e6 + ) + + var writers sync.WaitGroup + var readers sync.WaitGroup + readers.Add(numReaders) + writers.Add(numRoutines) + + lib := instrumentation.Library{ + Name: "testlib", + } + vopts := []view.Option{ + view.WithDefaultAggregationTemporalitySelector(func(_ sdkinstrument.Kind) aggregation.Temporality { + return tempo + }), + } + vcs := make([]*viewstate.Compiler, numReaders) + for vci := range vcs { + vcs[vci] = viewstate.New(lib, view.New("test", vopts...)) + } + attrs := make([]attribute.KeyValue, numAttrs) + for i := range attrs { + attrs[i] = attribute.Int("i", i) + } + + desc := test.Descriptor("tester", sdkinstrument.CounterKind, number.Int64Kind) + + pipes := make(pipeline.Register[viewstate.Instrument], numReaders) + for vci := range vcs { + pipes[vci], _ = vcs[vci].Compile(desc) + } + + inst := NewInstrument(desc, nil, pipes) + require.NotNil(t, inst) + + cntr := NewCounter[int64, number.Int64Traits](inst) + require.NotNil(t, cntr) + + ctx, cancel := context.WithCancel(context.Background()) + + partialCounts := make([]map[attribute.Set]int64, numReaders) + for vci := range vcs { + partialCounts[vci] = map[attribute.Set]int64{} + } + + // Reader loops + for vci := range vcs { + go func(vci int, partial map[attribute.Set]int64, vc *viewstate.Compiler) { + defer readers.Done() + + // scope will be reused by this reader + var scope data.Scope + seq := data.Sequence{ + Start: time.Now(), + } + seq.Now = seq.Start + + collect := func() { + seq.Last = seq.Now + seq.Now = time.Now() + + inst.SnapshotAndProcess() + + scope.Reset() + + vc.Collectors()[0].Collect(seq, &scope.Instruments) + + for _, pt := range scope.Instruments[0].Points { + partial[pt.Attributes] = update(partial[pt.Attributes], pt.Aggregation.(*sum.MonotonicInt64).Sum().AsInt64()) + } + } + + for { + select { + case <-ctx.Done(): + collect() + return + default: + collect() + } + } + }(vci, partialCounts[vci], vcs[vci]) + } + + // Writer loops + for i := 0; i < numRoutines; i++ { + go func() { + defer writers.Done() + rnd := rand.New(rand.NewSource(rand.Int63())) + + for j := 0; j < numUpdates/numRoutines; j++ { + cntr.Add(ctx, 1, attrs[rnd.Intn(len(attrs))]) + } + }() + } + + writers.Wait() + cancel() + readers.Wait() + + for vci := range vcs { + var sum int64 + for _, count := range partialCounts[vci] { + sum += count + } + require.Equal(t, int64(numUpdates), sum, "vci==%d", vci) + } +} + +func TestSyncStateNoopInstrument(t *testing.T) { + // TODO: test with disabled instrument +} diff --git a/lightstep/sdk/metric/manual.go b/lightstep/sdk/metric/manual.go new file mode 100644 index 00000000..f838378f --- /dev/null +++ b/lightstep/sdk/metric/manual.go @@ -0,0 +1,66 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package metric // import "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric" + +import ( + "context" + "fmt" + + "go.opentelemetry.io/otel" +) + +var ErrMultipleReaderRegistration = fmt.Errorf("reader has multiple registrations") + +// ManualReader is a a simple Reader that allows an application to +// read metrics on demand. It simply stores the Producer interface +// provided through registration. Flush and Shutdown are no-ops. +type ManualReader struct { + Name string + Producer +} + +var _ Reader = &ManualReader{} + +// NewManualReader returns an Reader that stores the Producer for +// manual use and returns a configurable `name` as its String(), +func NewManualReader(name string) *ManualReader { + return &ManualReader{ + Name: name, + } +} + +// String returns the name of this ManualReader. +func (mr *ManualReader) String() string { + return mr.Name +} + +// Register stores the Producer which enables the caller to read +// metrics on demand. +func (mr *ManualReader) Register(p Producer) { + if mr.Producer != nil { + otel.Handle(fmt.Errorf("%v: %w", mr.Name, ErrMultipleReaderRegistration)) + } + mr.Producer = p +} + +// ForceFlush is a no-op, always returns nil. +func (mr *ManualReader) ForceFlush(context.Context) error { + return nil +} + +// Shutdown is a no-op, always returns nil. +func (mr *ManualReader) Shutdown(context.Context) error { + return nil +} diff --git a/lightstep/sdk/metric/meter.go b/lightstep/sdk/metric/meter.go new file mode 100644 index 00000000..6831ef44 --- /dev/null +++ b/lightstep/sdk/metric/meter.go @@ -0,0 +1,85 @@ +// 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. + +package metric // import "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric" + +import ( + "context" + "sync" + + "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/metric/instrument" + "go.opentelemetry.io/otel/metric/instrument/asyncfloat64" + "go.opentelemetry.io/otel/metric/instrument/asyncint64" + "go.opentelemetry.io/otel/metric/instrument/syncfloat64" + "go.opentelemetry.io/otel/metric/instrument/syncint64" + "go.opentelemetry.io/otel/sdk/instrumentation" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/asyncstate" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/pipeline" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/syncstate" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/viewstate" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/sdkinstrument" +) + +// meter handles the creation and coordination of all metric instruments. A +// meter represents a single instrumentation scope; all metric telemetry +// produced by an instrumentation scope will use metric instruments from a +// single meter. +type meter struct { + library instrumentation.Library + provider *MeterProvider + compilers pipeline.Register[*viewstate.Compiler] + + lock sync.Mutex + byDesc map[sdkinstrument.Descriptor]interface{} + syncInsts []*syncstate.Instrument + asyncInsts []*asyncstate.Instrument + callbacks []*asyncstate.Callback +} + +// Compile-time check meter implements metric.Meter. +var _ metric.Meter = (*meter)(nil) + +// AsyncInt64 returns the asynchronous integer instrument provider. +func (m *meter) AsyncInt64() asyncint64.InstrumentProvider { + return asyncint64Instruments{m} +} + +// AsyncFloat64 returns the asynchronous floating-point instrument provider. +func (m *meter) AsyncFloat64() asyncfloat64.InstrumentProvider { + return asyncfloat64Instruments{m} +} + +// RegisterCallback registers the function f to be called when any of the +// insts Collect method is called. +func (m *meter) RegisterCallback(insts []instrument.Asynchronous, f func(context.Context)) error { + cb, err := asyncstate.NewCallback(insts, m, f) + + if err == nil { + m.lock.Lock() + defer m.lock.Unlock() + m.callbacks = append(m.callbacks, cb) + } + return err +} + +// SyncInt64 returns the synchronous integer instrument provider. +func (m *meter) SyncInt64() syncint64.InstrumentProvider { + return syncint64Instruments{m} +} + +// SyncFloat64 returns the synchronous floating-point instrument provider. +func (m *meter) SyncFloat64() syncfloat64.InstrumentProvider { + return syncfloat64Instruments{m} +} diff --git a/lightstep/sdk/metric/periodic.go b/lightstep/sdk/metric/periodic.go new file mode 100644 index 00000000..473b44df --- /dev/null +++ b/lightstep/sdk/metric/periodic.go @@ -0,0 +1,121 @@ +// 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. + +package metric // import "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric" + +import ( + "context" + "fmt" + "sync" + "time" + + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/data" + "go.opentelemetry.io/otel" +) + +// PushExporter is an interface for push-based exporters. +type PushExporter interface { + String() string + ExportMetrics(context.Context, data.Metrics) error + ShutdownMetrics(context.Context, data.Metrics) error + ForceFlushMetrics(context.Context, data.Metrics) error +} + +// PeriodicReader is an implementation of Reader that manages periodic +// exporter, flush, and shutdown. This implementation re-uses data +// from one collection to the next, to lower memory costs. +type PeriodicReader struct { + lock sync.Mutex + data data.Metrics + interval time.Duration + exporter PushExporter + producer Producer + stop context.CancelFunc + wait sync.WaitGroup +} + +// NewPeriodicReader constructs a PeriodicReader from a push-based +// exporter given an interval (TODO: and options). +func NewPeriodicReader(exporter PushExporter, interval time.Duration /* opts ...Option*/) Reader { + return &PeriodicReader{ + interval: interval, + exporter: exporter, + } +} + +// String returns the exporter name and the configured interval. +func (pr *PeriodicReader) String() string { + return fmt.Sprintf("%v interval %v", pr.exporter.String(), pr.interval) +} + +// Register starts the periodic export loop. +func (pr *PeriodicReader) Register(producer Producer) { + ctx, cancel := context.WithCancel(context.Background()) + + pr.producer = producer + pr.stop = cancel + pr.wait.Add(1) + + go pr.start(ctx) +} + +// start runs the export loop. +func (pr *PeriodicReader) start(ctx context.Context) { + defer pr.wait.Done() + ticker := time.NewTicker(pr.interval) + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + if err := pr.collect(ctx, pr.exporter.ExportMetrics); err != nil { + otel.Handle(err) + } + } + } +} + +// Shutdown stops the stops the export loop, canceling its Context, +// and waits for it to return. Then it issues a ShutdownMetrics with +// final data. +func (pr *PeriodicReader) Shutdown(ctx context.Context) error { + pr.stop() + pr.wait.Wait() + + return pr.collect(ctx, pr.exporter.ShutdownMetrics) +} + +// ForceFlush immediately waits for an existing collection, otherwise +// immediately begins collection without regards to timing and calls +// ForceFlush with current data. +func (pr *PeriodicReader) ForceFlush(ctx context.Context) error { + return pr.collect(ctx, pr.exporter.ForceFlushMetrics) +} + +// collect serializes access to re-usable metrics data, in each case +// calling through to an underlying PushExporter method with current +// data. +func (pr *PeriodicReader) collect(ctx context.Context, method func(context.Context, data.Metrics) error) error { + pr.lock.Lock() + defer pr.lock.Unlock() + + // The lock ensures that re-use of `pr.data` is successful, it + // means that shutdown, flush, and ordinary collection are + // exclusive. Note that shutdown will cancel a concurrent + // (ordinary) export, while flush will wait for a concurrent + // export. + pr.data = pr.producer.Produce(&pr.data) + + return method(ctx, pr.data) +} diff --git a/lightstep/sdk/metric/producer.go b/lightstep/sdk/metric/producer.go new file mode 100644 index 00000000..9334afa7 --- /dev/null +++ b/lightstep/sdk/metric/producer.go @@ -0,0 +1,120 @@ +// 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. + +package metric // import "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric" + +import ( + "context" + "sync" + "time" + + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/data" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/asyncstate" +) + +// providerProducer is the binding between the MeterProvider and the +// Reader. This is the Producer instance that is passed to Register() +// for each Reader. +type providerProducer struct { + lock sync.Mutex + provider *MeterProvider + pipe int + lastCollect time.Time +} + +// producerFor returns the new Producer for calling Register. +func (mp *MeterProvider) producerFor(pipe int) Producer { + return &providerProducer{ + provider: mp, + pipe: pipe, + lastCollect: mp.startTime, + } +} + +// Produce runs collection and produces a new metrics data object. +func (pp *providerProducer) Produce(inout *data.Metrics) data.Metrics { + ordered := pp.provider.getOrdered() + + // Note: the Last time is only used in delta-temporality + // scenarios. This lock protects the only stateful change in + // `pp` but does not prevent concurrent collection. If a + // delta-temporality reader were to call Produce + // concurrently, the results would be be recorded with + // non-overlapping timestamps but would have been collected in + // an overlapping way. + pp.lock.Lock() + lastTime := pp.lastCollect + nowTime := time.Now() + pp.lastCollect = nowTime + pp.lock.Unlock() + + var output data.Metrics + if inout != nil { + inout.Reset() + output = *inout + } + + output.Resource = pp.provider.cfg.res + + sequence := data.Sequence{ + Start: pp.provider.startTime, + Last: lastTime, + Now: nowTime, + } + + // TODO: Add a timeout to the context. + ctx := context.Background() + + for _, meter := range ordered { + meter.collectFor( + ctx, + pp.pipe, + sequence, + &output, + ) + } + + return output +} + +// collectFor collects from a single meter. +func (m *meter) collectFor(ctx context.Context, pipe int, seq data.Sequence, output *data.Metrics) { + // Use m.lock to briefly access the current lists: syncInsts, asyncInsts, callbacks + m.lock.Lock() + syncInsts := m.syncInsts + asyncInsts := m.asyncInsts + callbacks := m.callbacks + m.lock.Unlock() + + asyncState := asyncstate.NewState(pipe) + + for _, cb := range callbacks { + cb.Run(ctx, asyncState) + } + + for _, inst := range syncInsts { + inst.SnapshotAndProcess() + } + + for _, inst := range asyncInsts { + inst.SnapshotAndProcess(asyncState) + } + + scope := data.ReallocateFrom(&output.Scopes) + scope.Library = m.library + + for _, coll := range m.compilers[pipe].Collectors() { + coll.Collect(seq, &scope.Instruments) + } +} diff --git a/lightstep/sdk/metric/provider.go b/lightstep/sdk/metric/provider.go new file mode 100644 index 00000000..63389dc9 --- /dev/null +++ b/lightstep/sdk/metric/provider.go @@ -0,0 +1,169 @@ +// 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. + +package metric // import "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric" + +import ( + "context" + "fmt" + "sync" + "time" + + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/pipeline" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/viewstate" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/sdkinstrument" + "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/sdk/instrumentation" + "go.opentelemetry.io/otel/sdk/resource" + "go.uber.org/multierr" +) + +// MeterProvider handles the creation and coordination of Meters. All Meters +// created by a MeterProvider will be associated with the same Resource, have +// the same Views applied to them, and have their produced metric telemetry +// passed to the configured Readers. +type MeterProvider struct { + cfg config + startTime time.Time + lock sync.Mutex + ordered []*meter + meters map[instrumentation.Library]*meter +} + +// Compile-time check MeterProvider implements metric.MeterProvider. +var _ metric.MeterProvider = (*MeterProvider)(nil) + +var ErrAlreadyShutdown = fmt.Errorf("provider was already shut down") + +// NewMeterProvider returns a new and configured MeterProvider. +// +// By default, the returned MeterProvider is configured with the default +// Resource and no Readers. Readers cannot be added after a MeterProvider is +// created. This means the returned MeterProvider, one created with no +// Readers, will be perform no operations. +func NewMeterProvider(options ...Option) *MeterProvider { + cfg := config{ + res: resource.Default(), + } + for _, option := range options { + cfg = option.apply(cfg) + } + p := &MeterProvider{ + cfg: cfg, + startTime: time.Now(), + meters: map[instrumentation.Library]*meter{}, + } + for pipe := 0; pipe < len(cfg.readers); pipe++ { + cfg.readers[pipe].Register(p.producerFor(pipe)) + } + return p +} + +// Meter returns a Meter with the given name and configured with options. +// +// The name should be the name of the instrumentation scope creating +// telemetry. This name may be the same as the instrumented code only if that +// code provides built-in instrumentation. +// +// If name is empty, the default (go.opentelemetry.io/otel/sdk/meter) will be +// used. +// +// Calls to the Meter method after Shutdown has been called will return Meters +// that perform no operations. +// +// This method is safe to call concurrently. +func (mp *MeterProvider) Meter(name string, options ...metric.MeterOption) metric.Meter { + cfg := metric.NewMeterConfig(options...) + lib := instrumentation.Library{ + Name: name, + Version: cfg.InstrumentationVersion(), + SchemaURL: cfg.SchemaURL(), + } + + mp.lock.Lock() + defer mp.lock.Unlock() + + m := mp.meters[lib] + if m != nil { + return m + } + m = &meter{ + provider: mp, + library: lib, + byDesc: map[sdkinstrument.Descriptor]interface{}{}, + compilers: pipeline.NewRegister[*viewstate.Compiler](len(mp.cfg.readers)), + } + for pipe := range m.compilers { + m.compilers[pipe] = viewstate.New(lib, mp.cfg.views[pipe]) + } + mp.ordered = append(mp.ordered, m) + mp.meters[lib] = m + return m +} + +// ForceFlush flushes all pending telemetry. +// +// This method honors the deadline or cancellation of ctx. An appropriate +// error will be returned in these situations. There is no guaranteed that all +// telemetry be flushed or all resources have been released in these +// situations. +// +// This method is safe to call concurrently. +func (mp *MeterProvider) ForceFlush(ctx context.Context) error { + var err error + for _, r := range mp.cfg.readers { + err = multierr.Append(err, r.ForceFlush(ctx)) + } + return err +} + +// Shutdown shuts down the MeterProvider flushing all pending telemetry and +// releasing any held computational resources. +// +// This call is idempotent. The first call will perform all flush and +// releasing operations. Subsequent calls will perform no action. +// +// Measurements made by instruments from meters this MeterProvider created +// will not be exported after Shutdown is called. +// +// This method honors the deadline or cancellation of ctx. An appropriate +// error will be returned in these situations. There is no guaranteed that all +// telemetry be flushed or all resources have been released in these +// situations. +// +// This method is safe to call concurrently. +func (mp *MeterProvider) Shutdown(ctx context.Context) error { + var err error + + mp.lock.Lock() + defer mp.lock.Unlock() + + if mp.meters == nil { + return ErrAlreadyShutdown + } + + for _, r := range mp.cfg.readers { + err = multierr.Append(err, r.Shutdown(ctx)) + } + + mp.meters = nil + return err +} + +// getOrdered returns meters in the order they were registered. +func (mp *MeterProvider) getOrdered() []*meter { + mp.lock.Lock() + defer mp.lock.Unlock() + return mp.ordered +} diff --git a/lightstep/sdk/metric/provider_test.go b/lightstep/sdk/metric/provider_test.go new file mode 100644 index 00000000..b6e0f329 --- /dev/null +++ b/lightstep/sdk/metric/provider_test.go @@ -0,0 +1,304 @@ +// 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. + +package metric // import "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric" + +import ( + "context" + "errors" + "fmt" + "testing" + "time" + + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/aggregation" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/sum" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/data" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/test" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/viewstate" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/number" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/sdkinstrument" + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/sdk/resource" +) + +func must[T any](t T, err error) T { + if err != nil { + panic(err) + } + return t +} + +// Tests that calls to Produce() will re-use the underlying memory up to their capacity. +func TestOutputReuse(t *testing.T) { + ctx := context.Background() + + rdr := NewManualReader("test") + res := resource.Empty() + provider := NewMeterProvider(WithReader(rdr), WithResource(res)) + + cntr := must(provider.Meter("test").SyncInt64().Counter("hello")) + + var reuse data.Metrics + var notime time.Time + const cumulative = aggregation.CumulativeTemporality + attr := attribute.Int("K", 1) + attr2 := attribute.Int("L", 1) + + cntr.Add(ctx, 1, attr) + + // With a single point, check correct initial result. + reuse = rdr.Produce(&reuse) + + test.RequireEqualResourceMetrics( + t, reuse, res, + test.Scope( + test.Library("test"), + test.Instrument( + test.Descriptor("hello", sdkinstrument.CounterKind, number.Int64Kind), + test.Point(notime, notime, sum.NewMonotonicInt64(1), cumulative, attr), + ), + ), + ) + + // Now ensure that repeated Produce gives the same point + // address, new result. + pointAddrBefore := &reuse.Scopes[0].Instruments[0].Points[0] + + cntr.Add(ctx, 1, attribute.Int("K", 1)) + + reuse = rdr.Produce(&reuse) + + test.RequireEqualResourceMetrics( + t, reuse, res, + test.Scope( + test.Library("test"), + test.Instrument( + test.Descriptor("hello", sdkinstrument.CounterKind, number.Int64Kind), + test.Point(notime, notime, sum.NewMonotonicInt64(2), cumulative, attr), + ), + ), + ) + + pointAddrAfter := &reuse.Scopes[0].Instruments[0].Points[0] + + require.Equal(t, pointAddrBefore, pointAddrAfter) + + // Give the points list a cap of 17 and make a second point, ensure + reuse.Scopes[0].Instruments[0].Points = make([]data.Point, 0, 17) + + cntr.Add(ctx, 1, attr2) + + reuse = rdr.Produce(&reuse) + + require.Equal(t, 17, cap(reuse.Scopes[0].Instruments[0].Points)) + + test.RequireEqualResourceMetrics( + t, reuse, res, + test.Scope( + test.Library("test"), + test.Instrument( + test.Descriptor("hello", sdkinstrument.CounterKind, number.Int64Kind), + test.Point(notime, notime, sum.NewMonotonicInt64(2), cumulative, attr), + test.Point(notime, notime, sum.NewMonotonicInt64(1), cumulative, attr2), + ), + ), + ) + + // Make 20 more points, ensure capacity is greater than 17 + for i := 0; i < 20; i++ { + cntr.Add(ctx, 1, attribute.Int("I", i)) + } + + reuse = rdr.Produce(&reuse) + + require.Less(t, 17, cap(reuse.Scopes[0].Instruments[0].Points)) + + // Get the new address of scope-0, instrument-0, point-0: + pointAddrBefore = &reuse.Scopes[0].Instruments[0].Points[0] + + // Register more meters; expecting a capacity of 1 before and + // >1 after (but still the same point addresses). + require.Equal(t, 1, cap(reuse.Scopes)) + + fcntr := must(provider.Meter("real").SyncFloat64().Counter("goodbye")) + + fcntr.Add(ctx, 2, attr) + + reuse = rdr.Produce(&reuse) + + pointAddrAfter = &reuse.Scopes[0].Instruments[0].Points[0] + + require.Equal(t, pointAddrBefore, pointAddrAfter) + require.Less(t, 1, cap(reuse.Scopes)) + + require.Equal(t, "test", reuse.Scopes[0].Library.Name) + require.Equal(t, "real", reuse.Scopes[1].Library.Name) +} + +type testReader struct { + flushes int + shutdowns int + retval error +} + +func (t *testReader) String() string { + return "testreader" +} + +func (t *testReader) Register(_ Producer) { +} + +func (t *testReader) ForceFlush(_ context.Context) error { + t.flushes++ + return t.retval +} + +func (t *testReader) Shutdown(_ context.Context) error { + t.shutdowns++ + return t.retval +} + +func TestForceFlush(t *testing.T) { + // Test with 0 through 2 readers, ForceFlush() with and without errors. + ctx := context.Background() + provider := NewMeterProvider() + + require.NoError(t, provider.ForceFlush(ctx)) + + rdr1 := &testReader{} + provider = NewMeterProvider(WithReader(rdr1)) + + require.NoError(t, provider.ForceFlush(ctx)) + require.Equal(t, 1, rdr1.flushes) + + rdr1.retval = fmt.Errorf("flush fail") + err := provider.ForceFlush(ctx) + require.Error(t, err) + require.Equal(t, "flush fail", err.Error()) + require.Equal(t, 2, rdr1.flushes) + + rdr2 := &testReader{} + provider = NewMeterProvider( + WithReader(rdr2), + WithReader(rdr1), + WithReader(NewManualReader("also_tested")), // ManualReader.ForceFlush cannot error + ) + + err = provider.ForceFlush(ctx) + require.Error(t, err) + require.Equal(t, "flush fail", err.Error()) + require.Equal(t, 3, rdr1.flushes) + require.Equal(t, 1, rdr2.flushes) + + rdr1.retval = nil + + err = provider.ForceFlush(ctx) + require.NoError(t, err) + require.Equal(t, 4, rdr1.flushes) + require.Equal(t, 2, rdr2.flushes) + + require.Equal(t, 0, rdr1.shutdowns) + require.Equal(t, 0, rdr2.shutdowns) +} + +func TestShutdown(t *testing.T) { + ctx := context.Background() + + // Shutdown with 0 meters; repeat shutdown causes error. + provider := NewMeterProvider() + + require.NoError(t, provider.Shutdown(ctx)) + err := provider.Shutdown(ctx) + require.Error(t, err) + require.True(t, errors.Is(err, ErrAlreadyShutdown)) + + // Shutdown with 1 meters + rdr1 := &testReader{} + provider = NewMeterProvider(WithReader(rdr1)) + + require.NoError(t, provider.Shutdown(ctx)) + require.Equal(t, 1, rdr1.shutdowns) + require.Equal(t, 0, rdr1.flushes) + + err = provider.Shutdown(ctx) + require.Error(t, err) + require.True(t, errors.Is(err, ErrAlreadyShutdown)) + + // Shutdown with 3 meters, 2 errors + rdr1.retval = fmt.Errorf("first error") + + rdr2 := &testReader{} + rdr2.retval = fmt.Errorf("second error") + + provider = NewMeterProvider( + WithReader(rdr1), + WithReader(rdr2), + WithReader(NewManualReader("also_tested")), // ManualReader.Shutdown cannot error + ) + + err = provider.Shutdown(ctx) + + require.Error(t, err) + require.Contains(t, err.Error(), "first error") + require.Contains(t, err.Error(), "second error") + require.Equal(t, 2, rdr1.shutdowns) + require.Equal(t, 1, rdr2.shutdowns) + require.Equal(t, 0, rdr1.flushes) + require.Equal(t, 0, rdr2.flushes) +} + +func TestManualReaderRegisteredTwice(t *testing.T) { + rdr := NewManualReader("tester") + errs := test.OTelErrors() + + _ = NewMeterProvider(WithReader(rdr)) + _ = NewMeterProvider(WithReader(rdr)) + + require.Equal(t, 1, len(*errs)) + require.True(t, errors.Is((*errs)[0], ErrMultipleReaderRegistration)) +} + +func TestDuplicateInstrumentConflict(t *testing.T) { + rdr := NewManualReader("test") + res := resource.Empty() + errs := test.OTelErrors() + + provider := NewMeterProvider(WithReader(rdr), WithResource(res)) + + // int64/float64 conflicting registration + icntr := must(provider.Meter("test").SyncInt64().Counter("counter")) + fcntr, err := provider.Meter("test").SyncFloat64().Counter("counter") + + require.NotNil(t, icntr) + require.NotNil(t, fcntr) + require.NotEqual(t, icntr, fcntr) + require.Error(t, err) + + expected := "Counter-Int64-MonotonicSum, Counter-Float64-MonotonicSum" + + require.Contains(t, err.Error(), expected) + + // re-register the first instrument, get a failure + icntr2, err := provider.Meter("test").SyncInt64().Counter("counter") + require.Error(t, err) + require.Contains(t, err.Error(), expected) + require.Equal(t, icntr, icntr2) + + // there is only one conflict passed to otel.Handle()--only + // the first error is handled, even though it happened twice. + require.Equal(t, 1, len(*errs)) + fmt.Println((*errs)[0]) + require.True(t, errors.Is((*errs)[0], viewstate.ViewConflictsError{})) +} diff --git a/lightstep/sdk/metric/reader.go b/lightstep/sdk/metric/reader.go new file mode 100644 index 00000000..b801de60 --- /dev/null +++ b/lightstep/sdk/metric/reader.go @@ -0,0 +1,65 @@ +// 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. + +package metric // import "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric" + +import ( + "context" + + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/data" +) + +// Reader is the interface used between the SDK and an +// exporter. Control flow is bi-directional through the +// Reader, since the SDK initiates ForceFlush and Shutdown +// while the initiates collection. The Register() method here +// informs the Reader that it can begin reading, signaling the +// start of bi-directional control flow. +// +// Typically, push-based exporters that are periodic will +// implement PeroidicExporter themselves and construct a +// PeriodicReader to satisfy this interface. +// +// Pull-based exporters will typically implement Register +// themselves, since they read on demand. +type Reader interface { + // String describes this reader. + String() string + + // Register is called when the SDK is fully + // configured. The Producer passed allows the + // Reader to begin collecting metrics using its + // Produce() method. + Register(Producer) + + // ForceFlush is called when MeterProvider.ForceFlush() is called. + ForceFlush(context.Context) error + + // Shutdown is called when MeterProvider.Shutdown() is called. + Shutdown(context.Context) error +} + +// Producer is the interface used to perform collection by the reader. +type Producer interface { + // Produce returns metrics from a single collection. + // + // Produce may be called concurrently, + // + // The `in` parameter supports re-use of memory from + // one collection to the next. Callers that pass `in` + // will write metrics into the same slices and structs. + // + // When `in` is nil, a new Metrics object is returned. + Produce(in *data.Metrics) data.Metrics +} diff --git a/lightstep/sdk/metric/syncinst.go b/lightstep/sdk/metric/syncinst.go new file mode 100644 index 00000000..2d54dac1 --- /dev/null +++ b/lightstep/sdk/metric/syncinst.go @@ -0,0 +1,59 @@ +// 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. + +package metric // import "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric" + +import ( + "go.opentelemetry.io/otel/metric/instrument" + "go.opentelemetry.io/otel/metric/instrument/syncfloat64" + "go.opentelemetry.io/otel/metric/instrument/syncint64" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/syncstate" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/number" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/sdkinstrument" +) + +type ( + syncint64Instruments struct{ *meter } + syncfloat64Instruments struct{ *meter } +) + +func (i syncint64Instruments) Counter(name string, opts ...instrument.Option) (syncint64.Counter, error) { + inst, err := i.synchronousInstrument(name, opts, number.Int64Kind, sdkinstrument.CounterKind) + return syncstate.NewCounter[int64, number.Int64Traits](inst), err +} + +func (i syncint64Instruments) UpDownCounter(name string, opts ...instrument.Option) (syncint64.UpDownCounter, error) { + inst, err := i.synchronousInstrument(name, opts, number.Int64Kind, sdkinstrument.UpDownCounterKind) + return syncstate.NewCounter[int64, number.Int64Traits](inst), err +} + +func (i syncint64Instruments) Histogram(name string, opts ...instrument.Option) (syncint64.Histogram, error) { + inst, err := i.synchronousInstrument(name, opts, number.Int64Kind, sdkinstrument.HistogramKind) + return syncstate.NewHistogram[int64, number.Int64Traits](inst), err +} + +func (f syncfloat64Instruments) Counter(name string, opts ...instrument.Option) (syncfloat64.Counter, error) { + inst, err := f.synchronousInstrument(name, opts, number.Float64Kind, sdkinstrument.CounterKind) + return syncstate.NewCounter[float64, number.Float64Traits](inst), err +} + +func (f syncfloat64Instruments) UpDownCounter(name string, opts ...instrument.Option) (syncfloat64.UpDownCounter, error) { + inst, err := f.synchronousInstrument(name, opts, number.Float64Kind, sdkinstrument.UpDownCounterKind) + return syncstate.NewCounter[float64, number.Float64Traits](inst), err +} + +func (f syncfloat64Instruments) Histogram(name string, opts ...instrument.Option) (syncfloat64.Histogram, error) { + inst, err := f.synchronousInstrument(name, opts, number.Float64Kind, sdkinstrument.HistogramKind) + return syncstate.NewHistogram[float64, number.Float64Traits](inst), err +} diff --git a/lightstep/sdk/metric/syncinst_test.go b/lightstep/sdk/metric/syncinst_test.go new file mode 100644 index 00000000..ec390b66 --- /dev/null +++ b/lightstep/sdk/metric/syncinst_test.go @@ -0,0 +1,110 @@ +// 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. + +package metric + +import ( + "context" + "testing" + "time" + + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/aggregation" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/histogram" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/sum" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/test" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/number" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/sdkinstrument" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/view" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/sdk/resource" +) + +func TestSyncInsts(t *testing.T) { + cfg := aggregator.Config{} + // A histogram with size 4 ensures the test data has 3 buckets @ scale 0 + cfg.Histogram.MaxSize = 4 + + ctx := context.Background() + rdr := NewManualReader("test") + res := resource.Empty() + provider := NewMeterProvider( + WithResource(res), + WithReader( + rdr, + view.WithDefaultAggregationConfigSelector( + func(sdkinstrument.Kind) (int64Config, float64Config aggregator.Config) { + return cfg, cfg + }, + ), + ), + ) + + ci := must(provider.Meter("test").SyncInt64().Counter("icount")) + cf := must(provider.Meter("test").SyncFloat64().Counter("fcount")) + ui := must(provider.Meter("test").SyncInt64().UpDownCounter("iupcount")) + uf := must(provider.Meter("test").SyncFloat64().UpDownCounter("fupcount")) + hi := must(provider.Meter("test").SyncInt64().Histogram("ihistogram")) + hf := must(provider.Meter("test").SyncFloat64().Histogram("fhistogram")) + + attr := attribute.String("a", "B") + + ci.Add(ctx, 2, attr) + cf.Add(ctx, 3, attr) + ui.Add(ctx, 4, attr) + uf.Add(ctx, 5, attr) + + hi.Record(ctx, 2, attr) + hi.Record(ctx, 4, attr) + hi.Record(ctx, 8, attr) + + hf.Record(ctx, 8, attr) + hf.Record(ctx, 16, attr) + hf.Record(ctx, 32, attr) + + data := rdr.Produce(nil) + notime := time.Time{} + cumulative := aggregation.CumulativeTemporality + + test.RequireEqualResourceMetrics( + t, data, res, + test.Scope( + test.Library("test"), + test.Instrument( + test.Descriptor("icount", sdkinstrument.CounterKind, number.Int64Kind), + test.Point(notime, notime, sum.NewMonotonicInt64(2), cumulative, attr), + ), + test.Instrument( + test.Descriptor("fcount", sdkinstrument.CounterKind, number.Float64Kind), + test.Point(notime, notime, sum.NewMonotonicFloat64(3), cumulative, attr), + ), + test.Instrument( + test.Descriptor("iupcount", sdkinstrument.UpDownCounterKind, number.Int64Kind), + test.Point(notime, notime, sum.NewNonMonotonicInt64(4), cumulative, attr), + ), + test.Instrument( + test.Descriptor("fupcount", sdkinstrument.UpDownCounterKind, number.Float64Kind), + test.Point(notime, notime, sum.NewNonMonotonicFloat64(5), cumulative, attr), + ), + test.Instrument( + test.Descriptor("ihistogram", sdkinstrument.HistogramKind, number.Int64Kind), + test.Point(notime, notime, histogram.NewInt64(cfg.Histogram, 2, 4, 8), cumulative, attr), + ), + test.Instrument( + test.Descriptor("fhistogram", sdkinstrument.HistogramKind, number.Float64Kind), + test.Point(notime, notime, histogram.NewFloat64(cfg.Histogram, 8, 16, 32), cumulative, attr), + ), + ), + ) +} From 4f8235f9fa4459dd0de761dc83a576aa4a368ed0 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 3 Jun 2022 10:01:58 -0700 Subject: [PATCH 3/9] WIP concurrency testing + trialing comments from 172 --- .../metric/internal/viewstate/accumulators.go | 19 +++--- .../internal/viewstate/base_instrument.go | 37 +++++++---- .../metric/internal/viewstate/collectors.go | 63 ++++++------------- .../metric/internal/viewstate/viewstate.go | 10 +-- lightstep/sdk/metric/number/traits.go | 17 ++++- lightstep/sdk/metric/producer.go | 10 ++- lightstep/sdk/metric/sdkinstrument/kind.go | 3 + 7 files changed, 88 insertions(+), 71 deletions(-) diff --git a/lightstep/sdk/metric/internal/viewstate/accumulators.go b/lightstep/sdk/metric/internal/viewstate/accumulators.go index f6705612..3f0d1b17 100644 --- a/lightstep/sdk/metric/internal/viewstate/accumulators.go +++ b/lightstep/sdk/metric/internal/viewstate/accumulators.go @@ -38,6 +38,9 @@ func (acc multiAccumulator[N]) Update(value N) { // syncAccumulator type syncAccumulator[N number.Any, Storage any, Methods aggregator.Methods[N, Storage]] struct { + // syncLock prevents two readers from calling + // SnapshotAndProcess at the same moment. + syncLock sync.Mutex current Storage snapshot Storage findStorage func() *Storage @@ -50,26 +53,28 @@ func (acc *syncAccumulator[N, Storage, Methods]) Update(number N) { func (acc *syncAccumulator[N, Storage, Methods]) SnapshotAndProcess() { var methods Methods - methods.SynchronizedMove(&acc.current, &acc.snapshot) - methods.Merge(acc.findStorage(), &acc.snapshot) + acc.syncLock.Lock() + defer acc.syncLock.Unlock() + methods.Move(&acc.current, &acc.snapshot) + methods.Merge(&acc.snapshot, acc.findStorage()) } // asyncAccumulator type asyncAccumulator[N number.Any, Storage any, Methods aggregator.Methods[N, Storage]] struct { - lock sync.Mutex + asyncLock sync.Mutex current N findStorage func() *Storage } func (acc *asyncAccumulator[N, Storage, Methods]) Update(number N) { - acc.lock.Lock() - defer acc.lock.Unlock() + acc.asyncLock.Lock() + defer acc.asyncLock.Unlock() acc.current = number } func (acc *asyncAccumulator[N, Storage, Methods]) SnapshotAndProcess() { - acc.lock.Lock() - defer acc.lock.Unlock() + acc.asyncLock.Lock() + defer acc.asyncLock.Unlock() var methods Methods methods.Update(acc.findStorage(), acc.current) diff --git a/lightstep/sdk/metric/internal/viewstate/base_instrument.go b/lightstep/sdk/metric/internal/viewstate/base_instrument.go index 7ed0babc..9f437cd9 100644 --- a/lightstep/sdk/metric/internal/viewstate/base_instrument.go +++ b/lightstep/sdk/metric/internal/viewstate/base_instrument.go @@ -28,7 +28,7 @@ import ( // instrumentBase is the common type embedded in any of the compiled instrument views. type instrumentBase[N number.Any, Storage any, Methods aggregator.Methods[N, Storage]] struct { - lock sync.Mutex + instLock sync.Mutex fromName string desc sdkinstrument.Descriptor acfg aggregator.Config @@ -65,8 +65,8 @@ func (metric *instrumentBase[N, Storage, Methods]) initStorage(s *Storage) { } func (metric *instrumentBase[N, Storage, Methods]) mergeDescription(d string) { - metric.lock.Lock() - defer metric.lock.Unlock() + metric.instLock.Lock() + defer metric.instLock.Unlock() if len(d) > len(metric.desc.Description) { metric.desc.Description = d } @@ -83,8 +83,8 @@ func (metric *instrumentBase[N, Storage, Methods]) storageFinder( } return func() *Storage { - metric.lock.Lock() - defer metric.lock.Unlock() + metric.instLock.Lock() + defer metric.instLock.Unlock() storage, has := metric.data[kvs] if has { @@ -116,16 +116,27 @@ func (metric *instrumentBase[N, Storage, Methods]) appendInstrument(output *[]da return inst } -// appendPoint is used in cases where the output Aggregation is the -// stored object; use appendOrReusePoint in the case where the output -// Aggregation is a copy of the stored object (in case the stored -// object will be reset on collection, as opposed to a second pass to -// reset delta temporality outputs before the next accumulation. -func (metric *instrumentBase[N, Storage, Methods]) appendPoint(inst *data.Instrument, set attribute.Set, agg aggregation.Aggregation, tempo aggregation.Temporality, start, end time.Time) { - point := data.ReallocateFrom(&inst.Points) +// copyPoint is used in cases where the output Aggregation is a copy +// of the stored object. +func (metric *instrumentBase[N, Storage, Methods]) appendPoint(inst *data.Instrument, set attribute.Set, storage *Storage, tempo aggregation.Temporality, start, end time.Time, reset bool) { + var methods Methods + + // Possibly re-use the underlying storage. + point, out := metric.appendOrReusePoint(inst) + if out == nil { + out = metric.newStorage() + } + + if reset { + // Note: synchronized move uses swap for expensive + // copies, like histogram. + methods.Move(storage, out) + } else { + methods.Copy(storage, out) + } point.Attributes = set - point.Aggregation = agg + point.Aggregation = methods.ToAggregation(out) point.Temporality = tempo point.Start = start point.End = end diff --git a/lightstep/sdk/metric/internal/viewstate/collectors.go b/lightstep/sdk/metric/internal/viewstate/collectors.go index 3c026abe..e9cd54a6 100644 --- a/lightstep/sdk/metric/internal/viewstate/collectors.go +++ b/lightstep/sdk/metric/internal/viewstate/collectors.go @@ -59,15 +59,13 @@ type statefulSyncInstrument[N number.Any, Storage any, Methods aggregator.Method // Collect for synchronous cumulative temporality. func (p *statefulSyncInstrument[N, Storage, Methods]) Collect(seq data.Sequence, output *[]data.Instrument) { - var methods Methods - - p.lock.Lock() - defer p.lock.Unlock() + p.instLock.Lock() + defer p.instLock.Unlock() ioutput := p.appendInstrument(output) for set, storage := range p.data { - p.appendPoint(ioutput, set, methods.ToAggregation(storage), aggregation.CumulativeTemporality, seq.Start, seq.Now) + p.appendPoint(ioutput, set, storage, aggregation.CumulativeTemporality, seq.Start, seq.Now, false) } } @@ -78,43 +76,23 @@ type statelessSyncInstrument[N number.Any, Storage any, Methods aggregator.Metho // Collect for synchronous delta temporality. func (p *statelessSyncInstrument[N, Storage, Methods]) Collect(seq data.Sequence, output *[]data.Instrument) { - var methods Methods + //var methods Methods - p.lock.Lock() - defer p.lock.Unlock() + p.instLock.Lock() + defer p.instLock.Unlock() ioutput := p.appendInstrument(output) for set, storage := range p.data { - if !methods.HasChange(storage) { - delete(p.data, set) - continue - } + // TODO: Race condition here. Somehow refcount the + // storage? Use a refcount_mapped object? - // Possibly re-use the underlying storage. For - // synchronous instruments, where accumulation happens - // between collection events (e.g., due to other - // readers collecting), we must reset the storage now - // or completely clear the map. - point, exists := p.appendOrReusePoint(ioutput) - if exists == nil { - exists = p.newStorage() - } else { - methods.Reset(exists) - } + // if !methods.HasChange(storage) { + // delete(p.data, set) + // continue + // } - // Note: This can be improved with a Copy() or Swap() - // operation in the Methods, since Merge() may be - // relatively expensive by comparison. - methods.Merge(exists, storage) - - point.Attributes = set - point.Aggregation = methods.ToAggregation(exists) - point.Temporality = aggregation.DeltaTemporality - point.Start = seq.Last - point.End = seq.Now - - methods.Reset(storage) + p.appendPoint(ioutput, set, storage, aggregation.DeltaTemporality, seq.Last, seq.Now, true) } } @@ -126,16 +104,13 @@ type statelessAsyncInstrument[N number.Any, Storage any, Methods aggregator.Meth // Collect for asynchronous cumulative temporality. func (p *statelessAsyncInstrument[N, Storage, Methods]) Collect(seq data.Sequence, output *[]data.Instrument) { - var methods Methods - - p.lock.Lock() - defer p.lock.Unlock() + p.instLock.Lock() + defer p.instLock.Unlock() ioutput := p.appendInstrument(output) for set, storage := range p.data { - // Copy the underlying storage. - p.appendPoint(ioutput, set, methods.ToAggregation(storage), aggregation.CumulativeTemporality, seq.Start, seq.Now) + p.appendPoint(ioutput, set, storage, aggregation.CumulativeTemporality, seq.Start, seq.Now, false) } // Reset the entire map. @@ -153,8 +128,8 @@ type statefulAsyncInstrument[N number.Any, Storage any, Methods aggregator.Metho func (p *statefulAsyncInstrument[N, Storage, Methods]) Collect(seq data.Sequence, output *[]data.Instrument) { var methods Methods - p.lock.Lock() - defer p.lock.Unlock() + p.instLock.Lock() + defer p.instLock.Unlock() ioutput := p.appendInstrument(output) @@ -174,7 +149,7 @@ func (p *statefulAsyncInstrument[N, Storage, Methods]) Collect(seq data.Sequence storage = pval } } - p.appendPoint(ioutput, set, methods.ToAggregation(storage), aggregation.DeltaTemporality, seq.Last, seq.Now) + p.appendPoint(ioutput, set, storage, aggregation.DeltaTemporality, seq.Last, seq.Now, false) } // Copy the current to the prior and reset. p.prior = p.data diff --git a/lightstep/sdk/metric/internal/viewstate/viewstate.go b/lightstep/sdk/metric/internal/viewstate/viewstate.go index bea0843e..1a561e39 100644 --- a/lightstep/sdk/metric/internal/viewstate/viewstate.go +++ b/lightstep/sdk/metric/internal/viewstate/viewstate.go @@ -60,7 +60,7 @@ type Compiler struct { library instrumentation.Library // lock protects collectors and names. - lock sync.Mutex + compilerLock sync.Mutex // collectors is the de-duplicated list of metric outputs, which may // contain conflicting identities. @@ -170,8 +170,8 @@ func New(library instrumentation.Library, views *view.Views) *Compiler { } func (v *Compiler) Collectors() []data.Collector { - v.lock.Lock() - defer v.lock.Unlock() + v.compilerLock.Lock() + defer v.compilerLock.Unlock() return v.collectors } @@ -228,8 +228,8 @@ func (v *Compiler) Compile(instrument sdkinstrument.Descriptor) (Instrument, Vie } } - v.lock.Lock() - defer v.lock.Unlock() + v.compilerLock.Lock() + defer v.compilerLock.Unlock() var conflicts ViewConflictsBuilder var compiled []Instrument diff --git a/lightstep/sdk/metric/number/traits.go b/lightstep/sdk/metric/number/traits.go index 3921c54b..6a6615a3 100644 --- a/lightstep/sdk/metric/number/traits.go +++ b/lightstep/sdk/metric/number/traits.go @@ -21,7 +21,7 @@ import ( ) // Traits is the generic traits interface for numbers used in the SDK. -type Traits[N int64 | float64] interface { +type Traits[N Any] interface { // FromNumber turns a generic 64bits into the correct machine type. FromNumber(Number) N @@ -31,6 +31,9 @@ type Traits[N int64 | float64] interface { // SetAtomic sets `ptr` to `value`. SetAtomic(ptr *N, value N) + // GetAtomic reads `ptr`. + GetAtomic(ptr *N) N + // AddAtomic sets `ptr` to `value+*ptr`. AddAtomic(ptr *N, value N) @@ -50,6 +53,8 @@ type Traits[N int64 | float64] interface { // Int64Traits implements Traits[int64]. type Int64Traits struct{} +var _ Traits[int64] = Int64Traits{} + func (Int64Traits) ToNumber(x int64) Number { return Number(x) } @@ -58,6 +63,10 @@ func (Int64Traits) FromNumber(n Number) int64 { return int64(n) } +func (Int64Traits) GetAtomic(ptr *int64) int64 { + return atomic.LoadInt64(ptr) +} + func (Int64Traits) SetAtomic(ptr *int64, value int64) { atomic.StoreInt64(ptr, value) } @@ -85,6 +94,8 @@ func (Int64Traits) Kind() Kind { // Float64Traits implements Traits[float64]. type Float64Traits struct{} +var _ Traits[float64] = Float64Traits{} + func (Float64Traits) ToNumber(x float64) Number { return Number(math.Float64bits(x)) } @@ -93,6 +104,10 @@ func (Float64Traits) FromNumber(n Number) float64 { return math.Float64frombits(uint64(n)) } +func (Float64Traits) GetAtomic(ptr *float64) float64 { + return math.Float64frombits(atomic.LoadUint64((*uint64)(unsafe.Pointer(ptr)))) +} + func (Float64Traits) SetAtomic(ptr *float64, value float64) { atomic.StoreUint64((*uint64)(unsafe.Pointer(ptr)), math.Float64bits(value)) } diff --git a/lightstep/sdk/metric/producer.go b/lightstep/sdk/metric/producer.go index 9334afa7..5cc00841 100644 --- a/lightstep/sdk/metric/producer.go +++ b/lightstep/sdk/metric/producer.go @@ -90,7 +90,15 @@ func (pp *providerProducer) Produce(inout *data.Metrics) data.Metrics { // collectFor collects from a single meter. func (m *meter) collectFor(ctx context.Context, pipe int, seq data.Sequence, output *data.Metrics) { - // Use m.lock to briefly access the current lists: syncInsts, asyncInsts, callbacks + // Use m.lock to briefly access the current lists: syncInsts, + // asyncInsts, callbacks. By releasing these locks, we allow + // new instruments and callbacks to be registered while + // collection happens. The items themselves are synchronized, + // and the slices are only appended to, so shallow copies are + // safe. If new instruments and callbacks are registered while + // this collection happens, they simply will not collect and + // any activity they experience concurrently will be + // registered on the next collection by this reader. m.lock.Lock() syncInsts := m.syncInsts asyncInsts := m.asyncInsts diff --git a/lightstep/sdk/metric/sdkinstrument/kind.go b/lightstep/sdk/metric/sdkinstrument/kind.go index dfe761d8..b59d45b5 100644 --- a/lightstep/sdk/metric/sdkinstrument/kind.go +++ b/lightstep/sdk/metric/sdkinstrument/kind.go @@ -27,6 +27,9 @@ const ( // HistogramKind indicates a Histogram instrument. HistogramKind + // TODO: replace "Observer" with "Asynchronous" or "Observable", + // both of which are recommended in the API spec. + // CounterObserverKind indicates a CounterObserver instrument. CounterObserverKind // UpDownCounterObserverKind indicates a UpDownCounterObserver From 9f472987d29afc16b48d5013c20b83094b4ed111 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 3 Jun 2022 11:23:01 -0700 Subject: [PATCH 4/9] from review --- lightstep/sdk/metric/internal/syncstate/sync.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lightstep/sdk/metric/internal/syncstate/sync.go b/lightstep/sdk/metric/internal/syncstate/sync.go index 1bda1a30..a637559e 100644 --- a/lightstep/sdk/metric/internal/syncstate/sync.go +++ b/lightstep/sdk/metric/internal/syncstate/sync.go @@ -89,7 +89,10 @@ func (inst *Instrument) SnapshotAndProcess() { // this deletion: inst.current.Delete(key) - // Last we'll see of this. + // We have an exclusive reference to this accumulator + // now, but there could have been an update between + // the snapshotAndProcess() and tryUnmap() above, so + // snapshotAndProcess one last time. _ = rec.snapshotAndProcess() return true }) @@ -114,7 +117,7 @@ type record struct { accumulator viewstate.Accumulator } -// snapshotAndProcessRecord checks whether the accumulator has been +// snapshotAndProcess checks whether the accumulator has been // modified since the last collection (by any reader), returns a // boolean indicating whether the record is active. If active, calls // SnapshotAndProcess on the associated accumulator and returns true. From 9c70ced3b80a9aad5dcbc59f23dc308c59bec734 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 3 Jun 2022 16:14:27 -0700 Subject: [PATCH 5/9] Updates from development branch --- lightstep/sdk/metric/instrument.go | 14 +- .../sdk/metric/internal/asyncstate/async.go | 14 +- .../metric/internal/asyncstate/async_test.go | 141 ++++++++++++------ .../sdk/metric/internal/syncstate/sync.go | 17 ++- .../metric/internal/syncstate/sync_test.go | 123 ++++++++++++++- lightstep/sdk/metric/internal/test/test.go | 2 +- .../metric/internal/viewstate/viewstate.go | 1 - lightstep/sdk/metric/provider_test.go | 1 - 8 files changed, 249 insertions(+), 64 deletions(-) diff --git a/lightstep/sdk/metric/instrument.go b/lightstep/sdk/metric/instrument.go index f66bf99f..ae316cf1 100644 --- a/lightstep/sdk/metric/instrument.go +++ b/lightstep/sdk/metric/instrument.go @@ -15,14 +15,14 @@ package metric // import "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric" import ( - "go.opentelemetry.io/otel" - "go.opentelemetry.io/otel/metric/instrument" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/asyncstate" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/pipeline" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/syncstate" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/viewstate" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/number" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/sdkinstrument" + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/metric/instrument" ) // instrumentConstructor refers to either the syncstate or asyncstate @@ -45,9 +45,9 @@ func configureInstrument[T any]( opts []instrument.Option, nk number.Kind, ik sdkinstrument.Kind, - listPtr *[]T, - ctor instrumentConstructor[T], -) (T, error) { + listPtr *[]*T, + ctor instrumentConstructor[*T], +) (*T, error) { // Compute the instrument descriptor cfg := instrument.NewConfig(opts...) desc := sdkinstrument.NewDescriptor(name, ik, nk, cfg.Description(), cfg.Unit()) @@ -82,7 +82,9 @@ func configureInstrument[T any]( inst := ctor(desc, m, compiled) err := conflicts.AsError() - m.byDesc[desc] = inst + if inst != nil { + m.byDesc[desc] = inst + } *listPtr = append(*listPtr, inst) if err != nil { // Handle instrument creation errors when they're new, diff --git a/lightstep/sdk/metric/internal/asyncstate/async.go b/lightstep/sdk/metric/internal/asyncstate/async.go index 9ef58e9d..5126502e 100644 --- a/lightstep/sdk/metric/internal/asyncstate/async.go +++ b/lightstep/sdk/metric/internal/asyncstate/async.go @@ -80,6 +80,9 @@ func NewState(pipe int) *State { // NewInstrument returns a new Instrument; this compiles individual // instruments for each reader. func NewInstrument(desc sdkinstrument.Descriptor, opaque interface{}, compiled pipeline.Register[viewstate.Instrument]) *Instrument { + // Note: we return a non-nil instrument even when all readers + // disabled the instrument. This ensures that certain error + // checks still work (wrong meter, wrong callback, etc). return &Instrument{ opaque: opaque, descriptor: desc, @@ -101,12 +104,14 @@ func (inst *Instrument) SnapshotAndProcess(state *State) { func (inst *Instrument) get(cs *callbackState, attrs []attribute.KeyValue) viewstate.Accumulator { comp := inst.compiled[cs.state.pipe] - // @@@ nil check? + if comp == nil { + // The view disabled the instrument. + return nil + } cs.state.lock.Lock() defer cs.state.lock.Unlock() - aset := attribute.NewSet(attrs...) imap, has := cs.state.store[inst] if !has { @@ -114,6 +119,7 @@ func (inst *Instrument) get(cs *callbackState, attrs []attribute.KeyValue) views cs.state.store[inst] = imap } + aset := attribute.NewSet(attrs...) se, has := imap[aset] if !has { se = comp.NewAccumulator(aset) @@ -140,5 +146,7 @@ func capture[N number.Any, Traits number.Traits[N]](ctx context.Context, inst *I return } - inst.get(cs, attrs).(viewstate.Updater[N]).Update(value) + if acc := inst.get(cs, attrs); acc != nil { + acc.(viewstate.Updater[N]).Update(value) + } } diff --git a/lightstep/sdk/metric/internal/asyncstate/async_test.go b/lightstep/sdk/metric/internal/asyncstate/async_test.go index cf66158e..77ada26d 100644 --- a/lightstep/sdk/metric/internal/asyncstate/async_test.go +++ b/lightstep/sdk/metric/internal/asyncstate/async_test.go @@ -51,27 +51,42 @@ var ( ) type testSDK struct { - compiler *viewstate.Compiler + compilers []*viewstate.Compiler } func (tsdk *testSDK) compile(desc sdkinstrument.Descriptor) pipeline.Register[viewstate.Instrument] { - comp, err := tsdk.compiler.Compile(desc) - if err != nil { - panic(err) + reg := pipeline.NewRegister[viewstate.Instrument](len(tsdk.compilers)) + + for i, comp := range tsdk.compilers { + inst, err := comp.Compile(desc) + if err != nil { + panic(err) + } + reg[i] = inst } - reg := pipeline.NewRegister[viewstate.Instrument](1) - reg[0] = comp return reg } func testAsync(name string, opts ...view.Option) *testSDK { return &testSDK{ - compiler: viewstate.New(testLibrary, view.New(name, opts...)), + compilers: []*viewstate.Compiler{ + viewstate.New(testLibrary, view.New(name, opts...)), + viewstate.New(testLibrary, view.New(name, opts...)), + }, } } -func testState() *State { - return NewState(0) +func testAsync2(name string, opts1, opts2 []view.Option) *testSDK { + return &testSDK{ + compilers: []*viewstate.Compiler{ + viewstate.New(testLibrary, view.New(name, opts1...)), + viewstate.New(testLibrary, view.New(name, opts2...)), + }, + } +} + +func testState(num int) *State { + return NewState(num) } func testObserver[N number.Any, Traits number.Traits[N]](tsdk *testSDK, name string, ik sdkinstrument.Kind, opts ...instrument.Option) Observer[N, Traits] { @@ -154,7 +169,7 @@ func TestCallbackInvalidation(t *testing.T) { }) require.NoError(t, err) - state := testState() + state := testState(0) // run the callback once legitimately cb.Run(context.Background(), state) @@ -172,7 +187,7 @@ func TestCallbackInvalidation(t *testing.T) { t, test.CollectScope( t, - tsdk.compiler.Collectors(), + tsdk.compilers[0].Collectors(), testSequence, ), test.Instrument( @@ -182,7 +197,7 @@ func TestCallbackInvalidation(t *testing.T) { ) } -func TestCallbackUndeclaredInstrument(t *testing.T) { +func TestCallbackInstrumentUndeclaredForCalback(t *testing.T) { errors := test.OTelErrors() tt := testAsync("test") @@ -198,7 +213,7 @@ func TestCallbackUndeclaredInstrument(t *testing.T) { }) require.NoError(t, err) - state := testState() + state := testState(0) // run the callback once legitimately cb.Run(context.Background(), state) @@ -214,7 +229,7 @@ func TestCallbackUndeclaredInstrument(t *testing.T) { t, test.CollectScope( t, - tt.compiler.Collectors(), + tt.compilers[0].Collectors(), testSequence, ), test.Instrument( @@ -226,73 +241,105 @@ func TestCallbackUndeclaredInstrument(t *testing.T) { ) } -func TestCallbackDroppedInstrument(t *testing.T) { +func TestInstrumentUseOutsideCallback(t *testing.T) { errors := test.OTelErrors() - tt := testAsync("test", - view.WithClause( - view.MatchInstrumentName("drop"), - view.WithAggregation(aggregation.DropKind), - ), - ) - - cntrDrop := testObserver[float64, number.Float64Traits](tt, "drop", sdkinstrument.CounterObserverKind) - cntrKeep := testObserver[float64, number.Float64Traits](tt, "keep", sdkinstrument.CounterObserverKind) + tt := testAsync("test") - cb, _ := NewCallback([]instrument.Asynchronous{cntrKeep}, tt, func(ctx context.Context) { - cntrDrop.Observe(ctx, 1000) - cntrKeep.Observe(ctx, 1000) - }) + cntr := testObserver[float64, number.Float64Traits](tt, "cntr", sdkinstrument.CounterObserverKind) - state := testState() + cntr.Observe(context.Background(), 1000) - cb.Run(context.Background(), state) + state := testState(0) - cntrKeep.inst.SnapshotAndProcess(state) - cntrDrop.inst.SnapshotAndProcess(state) + cntr.inst.SnapshotAndProcess(state) require.Equal(t, 1, len(*errors)) - require.Contains(t, (*errors)[0].Error(), "instrument not declared for use in callback") + require.Contains(t, (*errors)[0].Error(), "async instrument used outside of callback") test.RequireEqualMetrics( t, test.CollectScope( t, - tt.compiler.Collectors(), + tt.compilers[0].Collectors(), testSequence, ), test.Instrument( - cntrKeep.inst.descriptor, - test.Point(startTime, endTime, sum.NewMonotonicFloat64(1000), aggregation.CumulativeTemporality), + cntr.inst.descriptor, ), ) } -func TestInstrumentUseOutsideCallback(t *testing.T) { - errors := test.OTelErrors() +func TestCallbackDisabledInstrument(t *testing.T) { + tt := testAsync2( + "test", + []view.Option{ + view.WithClause( + view.MatchInstrumentName("drop1"), + view.WithAggregation(aggregation.DropKind), + ), + view.WithClause( + view.MatchInstrumentName("drop2"), + view.WithAggregation(aggregation.DropKind), + ), + }, + []view.Option{ + view.WithClause( + view.MatchInstrumentName("drop2"), + view.WithAggregation(aggregation.DropKind), + ), + }, + ) - tt := testAsync("test") + cntrDrop1 := testObserver[float64, number.Float64Traits](tt, "drop1", sdkinstrument.CounterObserverKind) + cntrDrop2 := testObserver[float64, number.Float64Traits](tt, "drop2", sdkinstrument.CounterObserverKind) + cntrKeep := testObserver[float64, number.Float64Traits](tt, "keep", sdkinstrument.CounterObserverKind) - cntr := testObserver[float64, number.Float64Traits](tt, "cntr", sdkinstrument.CounterObserverKind) + cb, _ := NewCallback([]instrument.Asynchronous{cntrDrop1, cntrDrop2, cntrKeep}, tt, func(ctx context.Context) { + cntrKeep.Observe(ctx, 1000) + cntrDrop1.Observe(ctx, 1001) + cntrDrop2.Observe(ctx, 1002) + }) - cntr.Observe(context.Background(), 1000) + runFor := func(num int) { + state := testState(num) - state := testState() + cb.Run(context.Background(), state) - cntr.inst.SnapshotAndProcess(state) + cntrKeep.inst.SnapshotAndProcess(state) + cntrDrop1.inst.SnapshotAndProcess(state) + cntrDrop2.inst.SnapshotAndProcess(state) + } - require.Equal(t, 1, len(*errors)) - require.Contains(t, (*errors)[0].Error(), "async instrument used outside of callback") + runFor(0) + runFor(1) test.RequireEqualMetrics( t, test.CollectScope( t, - tt.compiler.Collectors(), + tt.compilers[0].Collectors(), testSequence, ), test.Instrument( - cntr.inst.descriptor, + cntrKeep.inst.descriptor, + test.Point(startTime, endTime, sum.NewMonotonicFloat64(1000), aggregation.CumulativeTemporality), + ), + ) + test.RequireEqualMetrics( + t, + test.CollectScope( + t, + tt.compilers[1].Collectors(), + testSequence, + ), + test.Instrument( + cntrDrop1.inst.descriptor, + test.Point(startTime, endTime, sum.NewMonotonicFloat64(1001), aggregation.CumulativeTemporality), + ), + test.Instrument( + cntrKeep.inst.descriptor, + test.Point(startTime, endTime, sum.NewMonotonicFloat64(1000), aggregation.CumulativeTemporality), ), ) } diff --git a/lightstep/sdk/metric/internal/syncstate/sync.go b/lightstep/sdk/metric/internal/syncstate/sync.go index a637559e..78777fb7 100644 --- a/lightstep/sdk/metric/internal/syncstate/sync.go +++ b/lightstep/sdk/metric/internal/syncstate/sync.go @@ -54,6 +54,16 @@ type Instrument struct { // second parameter is an opaque value used in the asyncstate package, // passed here to make these two packages generalize. func NewInstrument(desc sdkinstrument.Descriptor, _ interface{}, compiled pipeline.Register[viewstate.Instrument]) *Instrument { + var nonnil []viewstate.Instrument + for _, comp := range compiled { + if comp != nil { + nonnil = append(nonnil, comp) + } + } + if nonnil == nil { + // When no readers enable the instrument, no need for an instrument. + return nil + } return &Instrument{ descriptor: desc, @@ -64,8 +74,8 @@ func NewInstrument(desc sdkinstrument.Descriptor, _ interface{}, compiled pipeli // viewstate.Combine produces a single concrete // viewstate.Instrument. Only when there are multiple // views or multiple pipelines will the combination - // produce a viewstate.multiInstrment here. - compiled: viewstate.Combine(desc, compiled...), + // produce a viewstate.multiInstrument here. + compiled: viewstate.Combine(desc, nonnil...), } } @@ -139,7 +149,8 @@ func (rec *record) snapshotAndProcess() bool { // capture performs a single update for any synchronous instrument. func capture[N number.Any, Traits number.Traits[N]](_ context.Context, inst *Instrument, num N, attrs []attribute.KeyValue) { - if inst.compiled == nil { + if inst == nil { + // Instrument was completely disabled by the view. return } diff --git a/lightstep/sdk/metric/internal/syncstate/sync_test.go b/lightstep/sdk/metric/internal/syncstate/sync_test.go index ecd8c40c..1c53060c 100644 --- a/lightstep/sdk/metric/internal/syncstate/sync_test.go +++ b/lightstep/sdk/metric/internal/syncstate/sync_test.go @@ -21,7 +21,9 @@ import ( "testing" "time" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/aggregation" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/histogram" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/sum" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/data" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/pipeline" @@ -35,6 +37,18 @@ import ( "go.opentelemetry.io/otel/sdk/instrumentation" ) +var ( + endTime = time.Unix(100, 0) + middleTime = endTime.Add(-time.Millisecond) + startTime = endTime.Add(-2 * time.Millisecond) + + testSequence = data.Sequence{ + Start: startTime, + Last: middleTime, + Now: endTime, + } +) + func deltaUpdate(old, new int64) int64 { return old + new } @@ -173,6 +187,111 @@ func testSyncStateConcurrency(t *testing.T, numReaders int, tempo aggregation.Te } } -func TestSyncStateNoopInstrument(t *testing.T) { - // TODO: test with disabled instrument +func TestSyncStatePartialNoopInstrument(t *testing.T) { + ctx := context.Background() + vopts := []view.Option{ + view.WithClause( + view.MatchInstrumentName("dropme"), + view.WithAggregation(aggregation.DropKind), + ), + } + lib := instrumentation.Library{ + Name: "testlib", + } + vcs := make([]*viewstate.Compiler, 2) + vcs[0] = viewstate.New(lib, view.New("dropper", vopts...)) + vcs[1] = viewstate.New(lib, view.New("keeper")) + + desc := test.Descriptor("dropme", sdkinstrument.HistogramKind, number.Float64Kind) + + pipes := make(pipeline.Register[viewstate.Instrument], 2) + pipes[0], _ = vcs[0].Compile(desc) + pipes[1], _ = vcs[1].Compile(desc) + + require.Nil(t, pipes[0]) + require.NotNil(t, pipes[1]) + + inst := NewInstrument(desc, nil, pipes) + require.NotNil(t, inst) + + hist := NewHistogram[float64, number.Float64Traits](inst) + require.NotNil(t, hist) + + hist.Record(ctx, 1) + hist.Record(ctx, 2) + hist.Record(ctx, 3) + + inst.SnapshotAndProcess() + + test.RequireEqualMetrics( + t, + test.CollectScope( + t, + vcs[0].Collectors(), + testSequence, + ), + ) + + // Note: Create a merged histogram that is exactly equal to + // the one we expect. Merging creates a slightly different + // struct, despite identical value, so we merge to create the + // expected value: + expectHist := histogram.NewFloat64(aggregator.HistogramConfig{}) + mergeIn := histogram.NewFloat64(aggregator.HistogramConfig{}, 1, 2, 3) + expectHist.Merge(mergeIn) + + test.RequireEqualMetrics( + t, + test.CollectScope( + t, + vcs[1].Collectors(), + testSequence, + ), + test.Instrument( + desc, + test.Point(startTime, endTime, + expectHist, + aggregation.CumulativeTemporality, + ), + ), + ) +} + +func TestSyncStateFullNoopInstrument(t *testing.T) { + ctx := context.Background() + vopts := []view.Option{ + view.WithClause( + view.MatchInstrumentName("dropme"), + view.WithAggregation(aggregation.DropKind), + ), + } + lib := instrumentation.Library{ + Name: "testlib", + } + vcs := make([]*viewstate.Compiler, 2) + vcs[0] = viewstate.New(lib, view.New("dropper", vopts...)) + vcs[1] = viewstate.New(lib, view.New("keeper", vopts...)) + + desc := test.Descriptor("dropme", sdkinstrument.HistogramKind, number.Float64Kind) + + pipes := make(pipeline.Register[viewstate.Instrument], 2) + pipes[0], _ = vcs[0].Compile(desc) + pipes[1], _ = vcs[1].Compile(desc) + + require.Nil(t, pipes[0]) + require.Nil(t, pipes[1]) + + inst := NewInstrument(desc, nil, pipes) + require.Nil(t, inst) + + hist := NewHistogram[float64, number.Float64Traits](inst) + require.NotNil(t, hist) + + hist.Record(ctx, 1) + hist.Record(ctx, 2) + hist.Record(ctx, 3) + + // There's no instrument, nothing to Snapshot + require.Equal(t, 0, len(vcs[0].Collectors())) + require.Equal(t, 0, len(vcs[1].Collectors())) } diff --git a/lightstep/sdk/metric/internal/test/test.go b/lightstep/sdk/metric/internal/test/test.go index 7fe01390..ebf13ee1 100644 --- a/lightstep/sdk/metric/internal/test/test.go +++ b/lightstep/sdk/metric/internal/test/test.go @@ -90,7 +90,7 @@ func CollectScopeReuse(t *testing.T, collectors []data.Collector, seq data.Seque func RequireEqualPoints(t *testing.T, output []data.Point, expected ...data.Point) { t.Helper() - require.Equal(t, len(output), len(expected)) + require.Equal(t, len(output), len(expected), "points have different length") cpy := make([]data.Point, len(expected)) copy(cpy, expected) diff --git a/lightstep/sdk/metric/internal/viewstate/viewstate.go b/lightstep/sdk/metric/internal/viewstate/viewstate.go index 1a561e39..bb12d761 100644 --- a/lightstep/sdk/metric/internal/viewstate/viewstate.go +++ b/lightstep/sdk/metric/internal/viewstate/viewstate.go @@ -294,7 +294,6 @@ func (v *Compiler) Compile(instrument sdkinstrument.Descriptor) (Instrument, Vie v.collectors = append(v.collectors, leaf) existingInsts = append(existingInsts, leaf) v.names[behavior.desc.Name] = existingInsts - } if len(existingInsts) > 1 || semanticErr != nil { c := Conflict{ diff --git a/lightstep/sdk/metric/provider_test.go b/lightstep/sdk/metric/provider_test.go index b6e0f329..17f937c7 100644 --- a/lightstep/sdk/metric/provider_test.go +++ b/lightstep/sdk/metric/provider_test.go @@ -299,6 +299,5 @@ func TestDuplicateInstrumentConflict(t *testing.T) { // there is only one conflict passed to otel.Handle()--only // the first error is handled, even though it happened twice. require.Equal(t, 1, len(*errs)) - fmt.Println((*errs)[0]) require.True(t, errors.Is((*errs)[0], viewstate.ViewConflictsError{})) } From dfefd1e9f0e84955c185ca671e4741bb57403a30 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Fri, 3 Jun 2022 23:27:10 -0700 Subject: [PATCH 6/9] fix race condition; add test --- .../sdk/metric/internal/syncstate/sync.go | 4 +- .../metric/internal/syncstate/sync_test.go | 74 ++++++++++++------- 2 files changed, 50 insertions(+), 28 deletions(-) diff --git a/lightstep/sdk/metric/internal/syncstate/sync.go b/lightstep/sdk/metric/internal/syncstate/sync.go index 78777fb7..24be7fe1 100644 --- a/lightstep/sdk/metric/internal/syncstate/sync.go +++ b/lightstep/sdk/metric/internal/syncstate/sync.go @@ -140,10 +140,10 @@ func (rec *record) snapshotAndProcess() bool { if mods == coll { return false } + rec.accumulator.SnapshotAndProcess() + // Updates happened in this interval, collect and continue. atomic.StoreInt64(&rec.collectedCount, mods) - - rec.accumulator.SnapshotAndProcess() return true } diff --git a/lightstep/sdk/metric/internal/syncstate/sync_test.go b/lightstep/sdk/metric/internal/syncstate/sync_test.go index 1c53060c..e6672021 100644 --- a/lightstep/sdk/metric/internal/syncstate/sync_test.go +++ b/lightstep/sdk/metric/internal/syncstate/sync_test.go @@ -49,61 +49,82 @@ var ( } ) -func deltaUpdate(old, new int64) int64 { +func deltaUpdate[N number.Any](old, new N) N { return old + new } -func cumulativeUpdate(_, new int64) int64 { +func cumulativeUpdate[N number.Any](_, new N) N { return new } -func TestSyncStateDeltaConcurrency1(t *testing.T) { - testSyncStateConcurrency(t, 1, aggregation.DeltaTemporality, deltaUpdate) +const testAttr = attribute.Key("key") + +var ( + deltaSelector = view.WithDefaultAggregationTemporalitySelector(func(_ sdkinstrument.Kind) aggregation.Temporality { + return aggregation.DeltaTemporality + }) + + cumulativeSelector = view.WithDefaultAggregationTemporalitySelector(func(_ sdkinstrument.Kind) aggregation.Temporality { + return aggregation.CumulativeTemporality + }) + + keyFilter = view.WithClause( + view.WithKeys([]attribute.Key{}), + ) +) + +func TestSyncStateDeltaConcurrencyInt(t *testing.T) { + testSyncStateConcurrency[int64, number.Int64Traits](t, deltaUpdate[int64], deltaSelector) } -func TestSyncStateDeltaConcurrency2(t *testing.T) { - testSyncStateConcurrency(t, 2, aggregation.DeltaTemporality, deltaUpdate) +func TestSyncStateCumulativeConcurrencyInt(t *testing.T) { + testSyncStateConcurrency[int64, number.Int64Traits](t, cumulativeUpdate[int64], cumulativeSelector) } -func TestSyncStateCumulativeConcurrency1(t *testing.T) { - testSyncStateConcurrency(t, 1, aggregation.CumulativeTemporality, cumulativeUpdate) +func TestSyncStateCumulativeConcurrencyIntFiltered(t *testing.T) { + testSyncStateConcurrency[int64, number.Int64Traits](t, cumulativeUpdate[int64], cumulativeSelector, keyFilter) } -func TestSyncStateCumulativeConcurrency2(t *testing.T) { - testSyncStateConcurrency(t, 2, aggregation.CumulativeTemporality, cumulativeUpdate) +func TestSyncStateDeltaConcurrencyFloat(t *testing.T) { + testSyncStateConcurrency[float64, number.Float64Traits](t, deltaUpdate[float64], deltaSelector) } -// TODO: Add float64, Histogram tests. -func testSyncStateConcurrency(t *testing.T, numReaders int, tempo aggregation.Temporality, update func(old, new int64) int64) { +func TestSyncStateCumulativeConcurrencyFloat(t *testing.T) { + testSyncStateConcurrency[float64, number.Float64Traits](t, cumulativeUpdate[float64], cumulativeSelector) +} + +func TestSyncStateCumulativeConcurrencyFloatFiltered(t *testing.T) { + testSyncStateConcurrency[float64, number.Float64Traits](t, cumulativeUpdate[float64], cumulativeSelector, keyFilter) +} + +func testSyncStateConcurrency[N number.Any, Traits number.Traits[N]](t *testing.T, update func(old, new N) N, vopts ...view.Option) { const ( + numReaders = 2 numRoutines = 10 numAttrs = 10 numUpdates = 1e6 ) + var traits Traits var writers sync.WaitGroup var readers sync.WaitGroup + readers.Add(numReaders) writers.Add(numRoutines) lib := instrumentation.Library{ Name: "testlib", } - vopts := []view.Option{ - view.WithDefaultAggregationTemporalitySelector(func(_ sdkinstrument.Kind) aggregation.Temporality { - return tempo - }), - } vcs := make([]*viewstate.Compiler, numReaders) for vci := range vcs { vcs[vci] = viewstate.New(lib, view.New("test", vopts...)) } attrs := make([]attribute.KeyValue, numAttrs) for i := range attrs { - attrs[i] = attribute.Int("i", i) + attrs[i] = testAttr.Int(i) } - desc := test.Descriptor("tester", sdkinstrument.CounterKind, number.Int64Kind) + desc := test.Descriptor("tester", sdkinstrument.CounterKind, traits.Kind()) pipes := make(pipeline.Register[viewstate.Instrument], numReaders) for vci := range vcs { @@ -113,19 +134,20 @@ func testSyncStateConcurrency(t *testing.T, numReaders int, tempo aggregation.Te inst := NewInstrument(desc, nil, pipes) require.NotNil(t, inst) - cntr := NewCounter[int64, number.Int64Traits](inst) + cntr := NewCounter[N, Traits](inst) require.NotNil(t, cntr) ctx, cancel := context.WithCancel(context.Background()) - partialCounts := make([]map[attribute.Set]int64, numReaders) + partialCounts := make([]map[attribute.Set]N, numReaders) + for vci := range vcs { - partialCounts[vci] = map[attribute.Set]int64{} + partialCounts[vci] = map[attribute.Set]N{} } // Reader loops for vci := range vcs { - go func(vci int, partial map[attribute.Set]int64, vc *viewstate.Compiler) { + go func(vci int, partial map[attribute.Set]N, vc *viewstate.Compiler) { defer readers.Done() // scope will be reused by this reader @@ -146,7 +168,7 @@ func testSyncStateConcurrency(t *testing.T, numReaders int, tempo aggregation.Te vc.Collectors()[0].Collect(seq, &scope.Instruments) for _, pt := range scope.Instruments[0].Points { - partial[pt.Attributes] = update(partial[pt.Attributes], pt.Aggregation.(*sum.MonotonicInt64).Sum().AsInt64()) + partial[pt.Attributes] = update(partial[pt.Attributes], traits.FromNumber(pt.Aggregation.(*sum.State[N, Traits, sum.Monotonic]).Sum())) } } @@ -179,11 +201,11 @@ func testSyncStateConcurrency(t *testing.T, numReaders int, tempo aggregation.Te readers.Wait() for vci := range vcs { - var sum int64 + var sum N for _, count := range partialCounts[vci] { sum += count } - require.Equal(t, int64(numUpdates), sum, "vci==%d", vci) + require.Equal(t, N(numUpdates), sum, "vci==%d", vci) } } From 54ac1b868dbdf00fd7bd64a3404f6c6348c7557f Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Mon, 6 Jun 2022 10:35:51 -0700 Subject: [PATCH 7/9] synchronize the sync-delta collector --- lightstep/sdk/metric/instrument.go | 6 +- .../sdk/metric/internal/asyncstate/async.go | 2 +- .../internal/syncstate/refcount_mapped.go | 6 ++ .../sdk/metric/internal/syncstate/sync.go | 10 +-- .../metric/internal/viewstate/accumulators.go | 90 ++++++++++++++++--- .../internal/viewstate/base_instrument.go | 66 +++++++------- .../metric/internal/viewstate/collectors.go | 90 ++++++++----------- .../metric/internal/viewstate/viewstate.go | 10 +-- .../internal/viewstate/viewstate_test.go | 36 ++++---- 9 files changed, 187 insertions(+), 129 deletions(-) diff --git a/lightstep/sdk/metric/instrument.go b/lightstep/sdk/metric/instrument.go index ae316cf1..68f7bda3 100644 --- a/lightstep/sdk/metric/instrument.go +++ b/lightstep/sdk/metric/instrument.go @@ -34,7 +34,7 @@ type instrumentConstructor[T any] func( instrument sdkinstrument.Descriptor, opaque interface{}, compiled pipeline.Register[viewstate.Instrument], -) T +) *T // configureInstrument applies the instrument configuration, checks // for an existing definition for the same descriptor, and compiles @@ -46,7 +46,7 @@ func configureInstrument[T any]( nk number.Kind, ik sdkinstrument.Kind, listPtr *[]*T, - ctor instrumentConstructor[*T], + ctor instrumentConstructor[T], ) (*T, error) { // Compute the instrument descriptor cfg := instrument.NewConfig(opts...) @@ -65,7 +65,7 @@ func configureInstrument[T any]( conflicts.Combine(err) } - return lookup.(T), conflicts.AsError() + return lookup.(*T), conflicts.AsError() } // Compile the instrument for each pipeline. the first time. diff --git a/lightstep/sdk/metric/internal/asyncstate/async.go b/lightstep/sdk/metric/internal/asyncstate/async.go index 5126502e..7b8ec12f 100644 --- a/lightstep/sdk/metric/internal/asyncstate/async.go +++ b/lightstep/sdk/metric/internal/asyncstate/async.go @@ -97,7 +97,7 @@ func (inst *Instrument) SnapshotAndProcess(state *State) { defer state.lock.Unlock() for _, acc := range state.store[inst] { - acc.SnapshotAndProcess() + acc.SnapshotAndProcess(false) } } diff --git a/lightstep/sdk/metric/internal/syncstate/refcount_mapped.go b/lightstep/sdk/metric/internal/syncstate/refcount_mapped.go index d8b2dcaa..cbf60a22 100644 --- a/lightstep/sdk/metric/internal/syncstate/refcount_mapped.go +++ b/lightstep/sdk/metric/internal/syncstate/refcount_mapped.go @@ -30,6 +30,12 @@ type refcountMapped struct { value int64 } +func newRefcountMapped() refcountMapped { + return refcountMapped{ + value: 2, + } +} + // ref returns true if the entry is still mapped and increases the // reference usages, if unmapped returns false. func (rm *refcountMapped) ref() bool { diff --git a/lightstep/sdk/metric/internal/syncstate/sync.go b/lightstep/sdk/metric/internal/syncstate/sync.go index 24be7fe1..cf85f5c3 100644 --- a/lightstep/sdk/metric/internal/syncstate/sync.go +++ b/lightstep/sdk/metric/internal/syncstate/sync.go @@ -85,7 +85,7 @@ func NewInstrument(desc sdkinstrument.Descriptor, _ interface{}, compiled pipeli func (inst *Instrument) SnapshotAndProcess() { inst.current.Range(func(key interface{}, value interface{}) bool { rec := value.(*record) - if rec.snapshotAndProcess() { + if rec.snapshotAndProcess(false) { return true } // Having no updates since last collection, try to unmap: @@ -103,7 +103,7 @@ func (inst *Instrument) SnapshotAndProcess() { // now, but there could have been an update between // the snapshotAndProcess() and tryUnmap() above, so // snapshotAndProcess one last time. - _ = rec.snapshotAndProcess() + _ = rec.snapshotAndProcess(true) return true }) } @@ -133,14 +133,14 @@ type record struct { // SnapshotAndProcess on the associated accumulator and returns true. // If updates happened since the last collection (by any reader), // returns false. -func (rec *record) snapshotAndProcess() bool { +func (rec *record) snapshotAndProcess(final bool) bool { mods := atomic.LoadInt64(&rec.updateCount) coll := atomic.LoadInt64(&rec.collectedCount) if mods == coll { return false } - rec.accumulator.SnapshotAndProcess() + rec.accumulator.SnapshotAndProcess(final) // Updates happened in this interval, collect and continue. atomic.StoreInt64(&rec.collectedCount, mods) @@ -187,7 +187,7 @@ func acquireRecord[N number.Any](inst *Instrument, attrs []attribute.KeyValue) ( // if it is never returned, it will not be updated and can be // safely discarded. newRec := &record{ - refMapped: refcountMapped{value: 2}, + refMapped: newRefcountMapped(), accumulator: inst.compiled.NewAccumulator(aset), } diff --git a/lightstep/sdk/metric/internal/viewstate/accumulators.go b/lightstep/sdk/metric/internal/viewstate/accumulators.go index 3f0d1b17..0558e2ce 100644 --- a/lightstep/sdk/metric/internal/viewstate/accumulators.go +++ b/lightstep/sdk/metric/internal/viewstate/accumulators.go @@ -16,17 +16,76 @@ package viewstate // import "github.com/lightstep/otel-launcher-go/lightstep/sdk import ( "sync" + "sync/atomic" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/number" + "go.opentelemetry.io/otel/attribute" ) +// compiledSyncBase is any synchronous instrument view. +type compiledSyncBase[N number.Any, Storage any, Methods aggregator.Methods[N, Storage]] struct { + instrumentBase[N, Storage, int64, Methods] +} + +// NewAccumulator returns a Accumulator for a synchronous instrument view. +func (csv *compiledSyncBase[N, Storage, Methods]) NewAccumulator(kvs attribute.Set) Accumulator { + sc := &syncAccumulator[N, Storage, Methods]{} + csv.initStorage(&sc.current) + csv.initStorage(&sc.snapshot) + + holder := csv.findStorage(kvs) + + sc.holder = holder + + return sc +} + +func (csv *compiledSyncBase[N, Storage, Methods]) findStorage( + kvs attribute.Set, +) *storageHolder[Storage, int64] { + kvs = csv.applyKeysFilter(kvs) + + csv.instLock.Lock() + defer csv.instLock.Unlock() + + entry := csv.getOrCreateEntry(kvs) + atomic.AddInt64(&entry.auxiliary, 1) + return entry +} + +// compiledAsyncBase is any asynchronous instrument view. +type compiledAsyncBase[N number.Any, Storage any, Methods aggregator.Methods[N, Storage]] struct { + instrumentBase[N, Storage, notUsed, Methods] +} + +// NewAccumulator returns a Accumulator for an asynchronous instrument view. +func (cav *compiledAsyncBase[N, Storage, Methods]) NewAccumulator(kvs attribute.Set) Accumulator { + ac := &asyncAccumulator[N, Storage, Methods]{} + + ac.holder = cav.findStorage(kvs) + + return ac +} + +func (cav *compiledAsyncBase[N, Storage, Methods]) findStorage( + kvs attribute.Set, +) *storageHolder[Storage, notUsed] { + kvs = cav.applyKeysFilter(kvs) + + cav.instLock.Lock() + defer cav.instLock.Unlock() + + entry := cav.getOrCreateEntry(kvs) + return entry +} + // multiAccumulator type multiAccumulator[N number.Any] []Accumulator -func (acc multiAccumulator[N]) SnapshotAndProcess() { +func (acc multiAccumulator[N]) SnapshotAndProcess(final bool) { for _, coll := range acc { - coll.SnapshotAndProcess() + coll.SnapshotAndProcess(final) } } @@ -40,10 +99,10 @@ func (acc multiAccumulator[N]) Update(value N) { type syncAccumulator[N number.Any, Storage any, Methods aggregator.Methods[N, Storage]] struct { // syncLock prevents two readers from calling // SnapshotAndProcess at the same moment. - syncLock sync.Mutex - current Storage - snapshot Storage - findStorage func() *Storage + syncLock sync.Mutex + current Storage + snapshot Storage + holder *storageHolder[Storage, int64] } func (acc *syncAccumulator[N, Storage, Methods]) Update(number N) { @@ -51,19 +110,24 @@ func (acc *syncAccumulator[N, Storage, Methods]) Update(number N) { methods.Update(&acc.current, number) } -func (acc *syncAccumulator[N, Storage, Methods]) SnapshotAndProcess() { +func (acc *syncAccumulator[N, Storage, Methods]) SnapshotAndProcess(final bool) { var methods Methods acc.syncLock.Lock() defer acc.syncLock.Unlock() methods.Move(&acc.current, &acc.snapshot) - methods.Merge(&acc.snapshot, acc.findStorage()) + methods.Merge(&acc.snapshot, &acc.holder.storage) + if final { + atomic.AddInt64(&acc.holder.auxiliary, -1) + } } +type notUsed struct{} + // asyncAccumulator type asyncAccumulator[N number.Any, Storage any, Methods aggregator.Methods[N, Storage]] struct { - asyncLock sync.Mutex - current N - findStorage func() *Storage + asyncLock sync.Mutex + current N + holder *storageHolder[Storage, notUsed] } func (acc *asyncAccumulator[N, Storage, Methods]) Update(number N) { @@ -72,10 +136,10 @@ func (acc *asyncAccumulator[N, Storage, Methods]) Update(number N) { acc.current = number } -func (acc *asyncAccumulator[N, Storage, Methods]) SnapshotAndProcess() { +func (acc *asyncAccumulator[N, Storage, Methods]) SnapshotAndProcess(_ bool) { acc.asyncLock.Lock() defer acc.asyncLock.Unlock() var methods Methods - methods.Update(acc.findStorage(), acc.current) + methods.Update(&acc.holder.storage, acc.current) } diff --git a/lightstep/sdk/metric/internal/viewstate/base_instrument.go b/lightstep/sdk/metric/internal/viewstate/base_instrument.go index 9f437cd9..4806db05 100644 --- a/lightstep/sdk/metric/internal/viewstate/base_instrument.go +++ b/lightstep/sdk/metric/internal/viewstate/base_instrument.go @@ -26,45 +26,54 @@ import ( "go.opentelemetry.io/otel/attribute" ) +type syncAuxiliary int64 + +type asyncAuxiliary struct{} + +type storageHolder[Storage, Auxiliary any] struct { + auxiliary Auxiliary + storage Storage +} + // instrumentBase is the common type embedded in any of the compiled instrument views. -type instrumentBase[N number.Any, Storage any, Methods aggregator.Methods[N, Storage]] struct { +type instrumentBase[N number.Any, Storage, Auxiliary any, Methods aggregator.Methods[N, Storage]] struct { instLock sync.Mutex fromName string desc sdkinstrument.Descriptor acfg aggregator.Config - data map[attribute.Set]*Storage + data map[attribute.Set]*storageHolder[Storage, Auxiliary] keysSet *attribute.Set keysFilter *attribute.Filter } -func (metric *instrumentBase[N, Storage, Methods]) Aggregation() aggregation.Kind { +func (metric *instrumentBase[N, Storage, Auxiliary, Methods]) Aggregation() aggregation.Kind { var methods Methods return methods.Kind() } -func (metric *instrumentBase[N, Storage, Methods]) OriginalName() string { +func (metric *instrumentBase[N, Storage, Auxiliary, Methods]) OriginalName() string { return metric.fromName } -func (metric *instrumentBase[N, Storage, Methods]) Descriptor() sdkinstrument.Descriptor { +func (metric *instrumentBase[N, Storage, Auxiliary, Methods]) Descriptor() sdkinstrument.Descriptor { return metric.desc } -func (metric *instrumentBase[N, Storage, Methods]) Keys() *attribute.Set { +func (metric *instrumentBase[N, Storage, Auxiliary, Methods]) Keys() *attribute.Set { return metric.keysSet } -func (metric *instrumentBase[N, Storage, Methods]) Config() aggregator.Config { +func (metric *instrumentBase[N, Storage, Auxiliary, Methods]) Config() aggregator.Config { return metric.acfg } -func (metric *instrumentBase[N, Storage, Methods]) initStorage(s *Storage) { +func (metric *instrumentBase[N, Storage, Auxiliary, Methods]) initStorage(s *Storage) { var methods Methods methods.Init(s, metric.acfg) } -func (metric *instrumentBase[N, Storage, Methods]) mergeDescription(d string) { +func (metric *instrumentBase[N, Storage, Auxiliary, Methods]) mergeDescription(d string) { metric.instLock.Lock() defer metric.instLock.Unlock() if len(d) > len(metric.desc.Description) { @@ -72,33 +81,28 @@ func (metric *instrumentBase[N, Storage, Methods]) mergeDescription(d string) { } } -// storageFinder searches for and possibly allocates an output Storage -// for this metric. Filtered keys, if a filter is provided, will be -// computed once. -func (metric *instrumentBase[N, Storage, Methods]) storageFinder( - kvs attribute.Set, -) func() *Storage { +func (metric *instrumentBase[N, Storage, Auxiliary, Methods]) applyKeysFilter(kvs attribute.Set) attribute.Set { if metric.keysFilter != nil { kvs, _ = attribute.NewSetWithFiltered(kvs.ToSlice(), *metric.keysFilter) } + return kvs +} - return func() *Storage { - metric.instLock.Lock() - defer metric.instLock.Unlock() - - storage, has := metric.data[kvs] - if has { - return storage - } - - ns := metric.newStorage() - metric.data[kvs] = ns - return ns +func (metric *instrumentBase[N, Storage, Auxiliary, Methods]) getOrCreateEntry(kvs attribute.Set) *storageHolder[Storage, Auxiliary] { + entry, has := metric.data[kvs] + if has { + return entry } + + var methods Methods + entry = &storageHolder[Storage, Auxiliary]{} + methods.Init(&entry.storage, metric.acfg) + metric.data[kvs] = entry + return entry } // newStorage allocates and initializes a new Storage. -func (metric *instrumentBase[N, Storage, Methods]) newStorage() *Storage { +func (metric *instrumentBase[N, Storage, Auxiliary, Methods]) newStorage() *Storage { ns := new(Storage) metric.initStorage(ns) return ns @@ -110,7 +114,7 @@ func (metric *instrumentBase[N, Storage, Methods]) newStorage() *Storage { // be produced (in the same order); consumers of delta temporality // should expect to see empty instruments in the output for metric // data that is unchanged. -func (metric *instrumentBase[N, Storage, Methods]) appendInstrument(output *[]data.Instrument) *data.Instrument { +func (metric *instrumentBase[N, Storage, Auxiliary, Methods]) appendInstrument(output *[]data.Instrument) *data.Instrument { inst := data.ReallocateFrom(output) inst.Descriptor = metric.desc return inst @@ -118,7 +122,7 @@ func (metric *instrumentBase[N, Storage, Methods]) appendInstrument(output *[]da // copyPoint is used in cases where the output Aggregation is a copy // of the stored object. -func (metric *instrumentBase[N, Storage, Methods]) appendPoint(inst *data.Instrument, set attribute.Set, storage *Storage, tempo aggregation.Temporality, start, end time.Time, reset bool) { +func (metric *instrumentBase[N, Storage, Auxiliary, Methods]) appendPoint(inst *data.Instrument, set attribute.Set, storage *Storage, tempo aggregation.Temporality, start, end time.Time, reset bool) { var methods Methods // Possibly re-use the underlying storage. @@ -144,7 +148,7 @@ func (metric *instrumentBase[N, Storage, Methods]) appendPoint(inst *data.Instru // appendOrReusePoint is an alternate to appendPoint; this form is used when // the storage will be reset on collection. -func (metric *instrumentBase[N, Storage, Methods]) appendOrReusePoint(inst *data.Instrument) (*data.Point, *Storage) { +func (metric *instrumentBase[N, Storage, Auxiliary, Methods]) appendOrReusePoint(inst *data.Instrument) (*data.Point, *Storage) { point := data.ReallocateFrom(&inst.Points) var methods Methods diff --git a/lightstep/sdk/metric/internal/viewstate/collectors.go b/lightstep/sdk/metric/internal/viewstate/collectors.go index e9cd54a6..54afbf06 100644 --- a/lightstep/sdk/metric/internal/viewstate/collectors.go +++ b/lightstep/sdk/metric/internal/viewstate/collectors.go @@ -15,6 +15,8 @@ package viewstate // import "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/viewstate" import ( + "sync/atomic" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/aggregation" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/data" @@ -22,36 +24,6 @@ import ( "go.opentelemetry.io/otel/attribute" ) -// compiledSyncBase is any synchronous instrument view. -type compiledSyncBase[N number.Any, Storage any, Methods aggregator.Methods[N, Storage]] struct { - instrumentBase[N, Storage, Methods] -} - -// NewAccumulator returns a Accumulator for a synchronous instrument view. -func (csv *compiledSyncBase[N, Storage, Methods]) NewAccumulator(kvs attribute.Set) Accumulator { - sc := &syncAccumulator[N, Storage, Methods]{} - csv.initStorage(&sc.current) - csv.initStorage(&sc.snapshot) - - sc.findStorage = csv.storageFinder(kvs) - - return sc -} - -// compiledSyncBase is any asynchronous instrument view. -type compiledAsyncBase[N number.Any, Storage any, Methods aggregator.Methods[N, Storage]] struct { - instrumentBase[N, Storage, Methods] -} - -// NewAccumulator returns a Accumulator for an asynchronous instrument view. -func (cav *compiledAsyncBase[N, Storage, Methods]) NewAccumulator(kvs attribute.Set) Accumulator { - ac := &asyncAccumulator[N, Storage, Methods]{} - - ac.findStorage = cav.storageFinder(kvs) - - return ac -} - // statefulSyncInstrument is a synchronous instrument that maintains cumulative state. type statefulSyncInstrument[N number.Any, Storage any, Methods aggregator.Methods[N, Storage]] struct { compiledSyncBase[N, Storage, Methods] @@ -64,8 +36,8 @@ func (p *statefulSyncInstrument[N, Storage, Methods]) Collect(seq data.Sequence, ioutput := p.appendInstrument(output) - for set, storage := range p.data { - p.appendPoint(ioutput, set, storage, aggregation.CumulativeTemporality, seq.Start, seq.Now, false) + for set, entry := range p.data { + p.appendPoint(ioutput, set, &entry.storage, aggregation.CumulativeTemporality, seq.Start, seq.Now, false) } } @@ -76,23 +48,35 @@ type statelessSyncInstrument[N number.Any, Storage any, Methods aggregator.Metho // Collect for synchronous delta temporality. func (p *statelessSyncInstrument[N, Storage, Methods]) Collect(seq data.Sequence, output *[]data.Instrument) { - //var methods Methods + var methods Methods p.instLock.Lock() defer p.instLock.Unlock() ioutput := p.appendInstrument(output) - for set, storage := range p.data { - // TODO: Race condition here. Somehow refcount the - // storage? Use a refcount_mapped object? - - // if !methods.HasChange(storage) { - // delete(p.data, set) - // continue - // } - - p.appendPoint(ioutput, set, storage, aggregation.DeltaTemporality, seq.Last, seq.Now, true) + for set, entry := range p.data { + p.appendPoint(ioutput, set, &entry.storage, aggregation.DeltaTemporality, seq.Last, seq.Now, true) + + // By passing reset=true above, the aggregator data in + // entry.storage has been moved into the last index of + // ioutput.Points. + ptsArr := ioutput.Points + point := &ptsArr[len(ptsArr)-1] + + cpy, _ := methods.ToStorage(point.Aggregation) + if !methods.HasChange(cpy) { + // Now If the data is unchanged, truncate. + ioutput.Points = ptsArr[0 : len(ptsArr)-1 : cap(ptsArr)] + // If there are no more accumulators, remove from the map. + if atomic.LoadInt64(&entry.auxiliary) == 0 { + delete(p.data, set) + } + } + // Another design here would avoid the point before + // appending/truncating. This choice uses the slice + // of points to store the extra allocator used, even + // until the next collection. } } @@ -109,19 +93,19 @@ func (p *statelessAsyncInstrument[N, Storage, Methods]) Collect(seq data.Sequenc ioutput := p.appendInstrument(output) - for set, storage := range p.data { - p.appendPoint(ioutput, set, storage, aggregation.CumulativeTemporality, seq.Start, seq.Now, false) + for set, entry := range p.data { + p.appendPoint(ioutput, set, &entry.storage, aggregation.CumulativeTemporality, seq.Start, seq.Now, false) } // Reset the entire map. - p.data = map[attribute.Set]*Storage{} + p.data = map[attribute.Set]*storageHolder[Storage, notUsed]{} } // statefulAsyncInstrument is an instrument that keeps asynchronous instrument state // in order to perform cumulative to delta translation. type statefulAsyncInstrument[N number.Any, Storage any, Methods aggregator.Methods[N, Storage]] struct { compiledAsyncBase[N, Storage, Methods] - prior map[attribute.Set]*Storage + prior map[attribute.Set]*storageHolder[Storage, notUsed] } // Collect for asynchronous delta temporality. @@ -133,25 +117,25 @@ func (p *statefulAsyncInstrument[N, Storage, Methods]) Collect(seq data.Sequence ioutput := p.appendInstrument(output) - for set, storage := range p.data { + for set, entry := range p.data { pval, has := p.prior[set] if has { // This does `*pval := *storage - *pval` - methods.SubtractSwap(storage, pval) + methods.SubtractSwap(&entry.storage, &pval.storage) // Skip the series if it has not changed. - if !methods.HasChange(pval) { + if !methods.HasChange(&pval.storage) { continue } // Output the difference except for Gauge, in // which case output the new value. if p.desc.Kind.HasTemporality() { - storage = pval + entry = pval } } - p.appendPoint(ioutput, set, storage, aggregation.DeltaTemporality, seq.Last, seq.Now, false) + p.appendPoint(ioutput, set, &entry.storage, aggregation.DeltaTemporality, seq.Last, seq.Now, false) } // Copy the current to the prior and reset. p.prior = p.data - p.data = map[attribute.Set]*Storage{} + p.data = map[attribute.Set]*storageHolder[Storage, notUsed]{} } diff --git a/lightstep/sdk/metric/internal/viewstate/viewstate.go b/lightstep/sdk/metric/internal/viewstate/viewstate.go index bb12d761..d6a02f81 100644 --- a/lightstep/sdk/metric/internal/viewstate/viewstate.go +++ b/lightstep/sdk/metric/internal/viewstate/viewstate.go @@ -110,7 +110,7 @@ type Accumulator interface { // There is no return value from this method; the caller can // safely forget an Accumulator after this method is called, // provided Update is not used again. - SnapshotAndProcess() + SnapshotAndProcess(final bool) } // leafInstrument is one of the (synchronous or asynchronous), @@ -332,11 +332,11 @@ func newSyncView[ // is being copied before the new object is returned to the // user, and the extra allocation cost here would be // noticeable. - metric := instrumentBase[N, Storage, Methods]{ + metric := instrumentBase[N, Storage, int64, Methods]{ fromName: behavior.fromName, desc: behavior.desc, acfg: behavior.acfg, - data: map[attribute.Set]*Storage{}, + data: map[attribute.Set]*storageHolder[Storage, int64]{}, keysSet: behavior.keysSet, keysFilter: behavior.keysFilter, } @@ -393,11 +393,11 @@ func newAsyncView[ // is being copied before the new object is returned to the // user, and the extra allocation cost here would be // noticeable. - metric := instrumentBase[N, Storage, Methods]{ + metric := instrumentBase[N, Storage, notUsed, Methods]{ fromName: behavior.fromName, desc: behavior.desc, acfg: behavior.acfg, - data: map[attribute.Set]*Storage{}, + data: map[attribute.Set]*storageHolder[Storage, notUsed]{}, keysSet: behavior.keysSet, keysFilter: behavior.keysFilter, } diff --git a/lightstep/sdk/metric/internal/viewstate/viewstate_test.go b/lightstep/sdk/metric/internal/viewstate/viewstate_test.go index b9c43780..4a606fd8 100644 --- a/lightstep/sdk/metric/internal/viewstate/viewstate_test.go +++ b/lightstep/sdk/metric/internal/viewstate/viewstate_test.go @@ -432,7 +432,7 @@ func TestDuplicatesMergeDescriptor(t *testing.T) { accUpp := inst1.NewAccumulator(attribute.NewSet()) accUpp.(Updater[int64]).Update(1) - accUpp.SnapshotAndProcess() + accUpp.SnapshotAndProcess(false) output := testCollect(t, vc) @@ -468,7 +468,7 @@ func TestViewDescription(t *testing.T) { accUpp := inst1.NewAccumulator(attribute.NewSet(attrs...)) accUpp.(Updater[int64]).Update(1) - accUpp.SnapshotAndProcess() + accUpp.SnapshotAndProcess(false) output := testCollect(t, vc) @@ -507,8 +507,8 @@ func TestKeyFilters(t *testing.T) { accUpp1.(Updater[int64]).Update(1) accUpp2.(Updater[int64]).Update(1) - accUpp1.SnapshotAndProcess() - accUpp2.SnapshotAndProcess() + accUpp1.SnapshotAndProcess(false) + accUpp2.SnapshotAndProcess(false) output := testCollect(t, vc) @@ -557,7 +557,7 @@ func TestTwoViewsOneInt64Instrument(t *testing.T) { inst.NewAccumulator(attribute.NewSet(attribute.String("a", "2"), attribute.String("b", "2"))), } { acc.(Updater[int64]).Update(1) - acc.SnapshotAndProcess() + acc.SnapshotAndProcess(false) } output := testCollect(t, vc) @@ -619,7 +619,7 @@ func TestHistogramTwoAggregations(t *testing.T) { acc.(Updater[float64]).Update(2) acc.(Updater[float64]).Update(3) acc.(Updater[float64]).Update(4) - acc.SnapshotAndProcess() + acc.SnapshotAndProcess(false) output := testCollect(t, vc) @@ -654,11 +654,11 @@ func TestAllKeysFilter(t *testing.T) { acc1 := inst.NewAccumulator(attribute.NewSet(attribute.String("a", "1"))) acc1.(Updater[float64]).Update(1) - acc1.SnapshotAndProcess() + acc1.SnapshotAndProcess(false) acc2 := inst.NewAccumulator(attribute.NewSet(attribute.String("b", "2"))) acc2.(Updater[float64]).Update(1) - acc2.SnapshotAndProcess() + acc2.SnapshotAndProcess(false) output := testCollect(t, vc) @@ -705,7 +705,7 @@ func TestAnySumAggregation(t *testing.T) { acc := inst.NewAccumulator(attribute.NewSet()) acc.(Updater[float64]).Update(1) - acc.SnapshotAndProcess() + acc.SnapshotAndProcess(false) } output := testCollect(t, vc) @@ -758,7 +758,7 @@ func TestDuplicateAsyncMeasurementsIngored(t *testing.T) { acc.(Updater[float64]).Update(1000) acc.(Updater[float64]).Update(10000) acc.(Updater[float64]).Update(100000) - acc.SnapshotAndProcess() + acc.SnapshotAndProcess(false) } output := testCollect(t, vc) @@ -810,7 +810,7 @@ func TestCumulativeTemporality(t *testing.T) { inst2.NewAccumulator(setB), } { acc.(Updater[float64]).Update(1) - acc.SnapshotAndProcess() + acc.SnapshotAndProcess(false) } test.RequireEqualMetrics(t, testCollect(t, vc), @@ -865,7 +865,7 @@ func TestDeltaTemporalityCounter(t *testing.T) { inst2.NewAccumulator(setB), } { acc.(Updater[float64]).Update(float64(rounds)) - acc.SnapshotAndProcess() + acc.SnapshotAndProcess(false) } test.RequireEqualMetrics(t, testCollectSequence(t, vc, seq), @@ -912,11 +912,11 @@ func TestDeltaTemporalityGauge(t *testing.T) { observe := func(x int) { accI := instI.NewAccumulator(set) accI.(Updater[int64]).Update(int64(x)) - accI.SnapshotAndProcess() + accI.SnapshotAndProcess(false) accF := instF.NewAccumulator(set) accF.(Updater[float64]).Update(float64(x)) - accF.SnapshotAndProcess() + accF.SnapshotAndProcess(false) } expectValues := func(x int, seq data.Sequence) { @@ -1007,19 +1007,19 @@ func TestSyncDeltaTemporalityCounter(t *testing.T) { observe := func(mono, nonMono int) { accCI := instCI.NewAccumulator(set) accCI.(Updater[int64]).Update(int64(mono)) - accCI.SnapshotAndProcess() + accCI.SnapshotAndProcess(false) accCF := instCF.NewAccumulator(set) accCF.(Updater[float64]).Update(float64(mono)) - accCF.SnapshotAndProcess() + accCF.SnapshotAndProcess(false) accUI := instUI.NewAccumulator(set) accUI.(Updater[int64]).Update(int64(nonMono)) - accUI.SnapshotAndProcess() + accUI.SnapshotAndProcess(false) accUF := instUF.NewAccumulator(set) accUF.(Updater[float64]).Update(float64(nonMono)) - accUF.SnapshotAndProcess() + accUF.SnapshotAndProcess(false) } expectValues := func(mono, nonMono int, seq data.Sequence) { From a5af7059c1ce7a4013f12cf944d5e62f7562dafd Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Mon, 6 Jun 2022 15:12:34 -0700 Subject: [PATCH 8/9] updates & edits --- lightstep/sdk/metric/instrument.go | 105 ------------------ .../sdk/metric/internal/asyncstate/async.go | 9 +- .../metric/internal/asyncstate/async_test.go | 63 +++++++++++ .../sdk/metric/internal/syncstate/sync.go | 5 + .../metric/internal/syncstate/sync_test.go | 67 +++++++++++ .../metric/internal/viewstate/accumulators.go | 15 +-- .../internal/viewstate/base_instrument.go | 13 ++- .../metric/internal/viewstate/viewstate.go | 4 + .../internal/viewstate/viewstate_test.go | 56 +++++++++- lightstep/sdk/metric/meter.go | 91 ++++++++++++++- lightstep/sdk/metric/number/number.go | 6 +- lightstep/sdk/metric/number/traits.go | 4 +- lightstep/sdk/metric/reader.go | 8 +- lightstep/sdk/metric/view/standard.go | 15 +-- 14 files changed, 320 insertions(+), 141 deletions(-) delete mode 100644 lightstep/sdk/metric/instrument.go diff --git a/lightstep/sdk/metric/instrument.go b/lightstep/sdk/metric/instrument.go deleted file mode 100644 index 68f7bda3..00000000 --- a/lightstep/sdk/metric/instrument.go +++ /dev/null @@ -1,105 +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. - -package metric // import "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric" - -import ( - "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/asyncstate" - "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/pipeline" - "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/syncstate" - "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/viewstate" - "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/number" - "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/sdkinstrument" - "go.opentelemetry.io/otel" - "go.opentelemetry.io/otel/metric/instrument" -) - -// instrumentConstructor refers to either the syncstate or asyncstate -// NewInstrument method. Although both receive an opaque interface{} -// to distinguish providers, only the asyncstate package needs to know -// this information. The unused parameter is passed to the syncstate -// package for the generalization used here to work. -type instrumentConstructor[T any] func( - instrument sdkinstrument.Descriptor, - opaque interface{}, - compiled pipeline.Register[viewstate.Instrument], -) *T - -// configureInstrument applies the instrument configuration, checks -// for an existing definition for the same descriptor, and compiles -// and constructs the instrument if necessary. -func configureInstrument[T any]( - m *meter, - name string, - opts []instrument.Option, - nk number.Kind, - ik sdkinstrument.Kind, - listPtr *[]*T, - ctor instrumentConstructor[T], -) (*T, error) { - // Compute the instrument descriptor - cfg := instrument.NewConfig(opts...) - desc := sdkinstrument.NewDescriptor(name, ik, nk, cfg.Description(), cfg.Unit()) - - m.lock.Lock() - defer m.lock.Unlock() - - // Lookup a pre-existing instrument by descriptor. - if lookup, has := m.byDesc[desc]; has { - // Recompute conflicts since they may have changed. - var conflicts viewstate.ViewConflictsBuilder - - for _, compiler := range m.compilers { - _, err := compiler.Compile(desc) - conflicts.Combine(err) - } - - return lookup.(*T), conflicts.AsError() - } - - // Compile the instrument for each pipeline. the first time. - var conflicts viewstate.ViewConflictsBuilder - compiled := pipeline.NewRegister[viewstate.Instrument](len(m.compilers)) - - for pipe, compiler := range m.compilers { - comp, err := compiler.Compile(desc) - compiled[pipe] = comp - conflicts.Combine(err) - } - - // Build the new instrument, cache it, append to the list. - inst := ctor(desc, m, compiled) - err := conflicts.AsError() - - if inst != nil { - m.byDesc[desc] = inst - } - *listPtr = append(*listPtr, inst) - if err != nil { - // Handle instrument creation errors when they're new, - // not for repeat entries above. - otel.Handle(err) - } - return inst, err -} - -// synchronousInstrument configures a synchronous instrument. -func (m *meter) synchronousInstrument(name string, opts []instrument.Option, nk number.Kind, ik sdkinstrument.Kind) (*syncstate.Instrument, error) { - return configureInstrument(m, name, opts, nk, ik, &m.syncInsts, syncstate.NewInstrument) -} - -// synchronousInstrument configures an asynchronous instrument. -func (m *meter) asynchronousInstrument(name string, opts []instrument.Option, nk number.Kind, ik sdkinstrument.Kind) (*asyncstate.Instrument, error) { - return configureInstrument(m, name, opts, nk, ik, &m.asyncInsts, asyncstate.NewInstrument) -} diff --git a/lightstep/sdk/metric/internal/asyncstate/async.go b/lightstep/sdk/metric/internal/asyncstate/async.go index 7b8ec12f..a30c821b 100644 --- a/lightstep/sdk/metric/internal/asyncstate/async.go +++ b/lightstep/sdk/metric/internal/asyncstate/async.go @@ -19,6 +19,7 @@ import ( "fmt" "sync" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/pipeline" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/viewstate" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/number" @@ -97,7 +98,9 @@ func (inst *Instrument) SnapshotAndProcess(state *State) { defer state.lock.Unlock() for _, acc := range state.store[inst] { - acc.SnapshotAndProcess(false) + // SnapshotAndProcess is always final for asynchronous state, since + // the map is built anew for each collection. + acc.SnapshotAndProcess(true) } } @@ -146,6 +149,10 @@ func capture[N number.Any, Traits number.Traits[N]](ctx context.Context, inst *I return } + if !aggregator.RangeTest[N, Traits](value, inst.descriptor.Kind) { + return + } + if acc := inst.get(cs, attrs); acc != nil { acc.(viewstate.Updater[N]).Update(value) } diff --git a/lightstep/sdk/metric/internal/asyncstate/async_test.go b/lightstep/sdk/metric/internal/asyncstate/async_test.go index 77ada26d..086ae127 100644 --- a/lightstep/sdk/metric/internal/asyncstate/async_test.go +++ b/lightstep/sdk/metric/internal/asyncstate/async_test.go @@ -16,11 +16,13 @@ package asyncstate // import "github.com/lightstep/otel-launcher-go/lightstep/sd import ( "context" + "math" "testing" "time" "github.com/stretchr/testify/require" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/aggregation" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/sum" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/data" @@ -343,3 +345,64 @@ func TestCallbackDisabledInstrument(t *testing.T) { ), ) } + +func TestOutOfRangeValues(t *testing.T) { + errors := test.OTelErrors() + + tt := testAsync("test") + + c := testObserver[float64, number.Float64Traits](tt, "c", sdkinstrument.CounterObserverKind) + u := testObserver[float64, number.Float64Traits](tt, "u", sdkinstrument.UpDownCounterObserverKind) + g := testObserver[float64, number.Float64Traits](tt, "g", sdkinstrument.GaugeObserverKind) + + cb, _ := NewCallback([]instrument.Asynchronous{ + c, u, g, + }, tt, func(ctx context.Context) { + c.Observe(ctx, math.NaN()) + c.Observe(ctx, math.Inf(+1)) + c.Observe(ctx, math.Inf(-1)) + u.Observe(ctx, math.NaN()) + u.Observe(ctx, math.Inf(+1)) + u.Observe(ctx, math.Inf(-1)) + g.Observe(ctx, math.NaN()) + g.Observe(ctx, math.Inf(+1)) + g.Observe(ctx, math.Inf(-1)) + }) + + runFor := func(num int) { + state := testState(num) + + cb.Run(context.Background(), state) + + c.inst.SnapshotAndProcess(state) + u.inst.SnapshotAndProcess(state) + g.inst.SnapshotAndProcess(state) + } + + for i := 0; i < 2; i++ { + runFor(i) + + test.RequireEqualMetrics( + t, + test.CollectScope( + t, + tt.compilers[i].Collectors(), + testSequence, + ), + test.Instrument( + c.inst.descriptor, + ), + test.Instrument( + u.inst.descriptor, + ), + test.Instrument( + g.inst.descriptor, + ), + ) + } + + // 2 readers x 3 error conditions x 3 instruments + require.Equal(t, 2*3*3, len(*errors)) + require.Contains(t, (*errors), aggregator.ErrNaNInput) + require.Contains(t, (*errors), aggregator.ErrInfInput) +} diff --git a/lightstep/sdk/metric/internal/syncstate/sync.go b/lightstep/sdk/metric/internal/syncstate/sync.go index cf85f5c3..16d77ed2 100644 --- a/lightstep/sdk/metric/internal/syncstate/sync.go +++ b/lightstep/sdk/metric/internal/syncstate/sync.go @@ -20,6 +20,7 @@ import ( "sync" "sync/atomic" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/pipeline" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/viewstate" "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/number" @@ -156,6 +157,10 @@ func capture[N number.Any, Traits number.Traits[N]](_ context.Context, inst *Ins // Note: Here, this is the place to use context, e.g., extract baggage. + if !aggregator.RangeTest[N, Traits](num, inst.descriptor.Kind) { + return + } + rec, updater := acquireRecord[N](inst, attrs) defer rec.refMapped.unref() diff --git a/lightstep/sdk/metric/internal/syncstate/sync_test.go b/lightstep/sdk/metric/internal/syncstate/sync_test.go index e6672021..11f92ff7 100644 --- a/lightstep/sdk/metric/internal/syncstate/sync_test.go +++ b/lightstep/sdk/metric/internal/syncstate/sync_test.go @@ -16,6 +16,7 @@ package syncstate import ( "context" + "math" "math/rand" "sync" "testing" @@ -317,3 +318,69 @@ func TestSyncStateFullNoopInstrument(t *testing.T) { require.Equal(t, 0, len(vcs[0].Collectors())) require.Equal(t, 0, len(vcs[1].Collectors())) } + +func TestOutOfRangeValues(t *testing.T) { + for _, desc := range []sdkinstrument.Descriptor{ + test.Descriptor("cf", sdkinstrument.CounterKind, number.Float64Kind), + test.Descriptor("uf", sdkinstrument.UpDownCounterKind, number.Float64Kind), + test.Descriptor("hf", sdkinstrument.HistogramKind, number.Float64Kind), + test.Descriptor("ci", sdkinstrument.CounterKind, number.Int64Kind), + test.Descriptor("ui", sdkinstrument.UpDownCounterKind, number.Int64Kind), + test.Descriptor("hi", sdkinstrument.HistogramKind, number.Int64Kind), + } { + ctx := context.Background() + lib := instrumentation.Library{ + Name: "testlib", + } + vcs := make([]*viewstate.Compiler, 1) + vcs[0] = viewstate.New(lib, view.New("test")) + + pipes := make(pipeline.Register[viewstate.Instrument], 1) + pipes[0], _ = vcs[0].Compile(desc) + + inst := NewInstrument(desc, nil, pipes) + require.NotNil(t, inst) + + var negOne aggregation.Aggregation + + if desc.NumberKind == number.Float64Kind { + cntr := NewCounter[float64, number.Float64Traits](inst) + + cntr.Add(ctx, -1) + cntr.Add(ctx, math.NaN()) + cntr.Add(ctx, math.Inf(+1)) + cntr.Add(ctx, math.Inf(-1)) + negOne = sum.NewNonMonotonicFloat64(-1) + } else { + cntr := NewCounter[int64, number.Int64Traits](inst) + + cntr.Add(ctx, -1) + negOne = sum.NewNonMonotonicInt64(-1) + } + + inst.SnapshotAndProcess() + + var expectPoints []data.Point + + if desc.Kind == sdkinstrument.UpDownCounterKind { + expectPoints = append(expectPoints, test.Point( + startTime, endTime, + negOne, + aggregation.CumulativeTemporality, + )) + } + + test.RequireEqualMetrics( + t, + test.CollectScope( + t, + vcs[0].Collectors(), + testSequence, + ), + test.Instrument( + desc, + expectPoints..., + ), + ) + } +} diff --git a/lightstep/sdk/metric/internal/viewstate/accumulators.go b/lightstep/sdk/metric/internal/viewstate/accumulators.go index 0558e2ce..e94a2289 100644 --- a/lightstep/sdk/metric/internal/viewstate/accumulators.go +++ b/lightstep/sdk/metric/internal/viewstate/accumulators.go @@ -34,13 +34,12 @@ func (csv *compiledSyncBase[N, Storage, Methods]) NewAccumulator(kvs attribute.S csv.initStorage(&sc.current) csv.initStorage(&sc.snapshot) - holder := csv.findStorage(kvs) - - sc.holder = holder - + sc.holder = csv.findStorage(kvs) return sc } +// findStorage locates the output Storage and adds to the auxiliary +// reference count for synchronous instruments. func (csv *compiledSyncBase[N, Storage, Methods]) findStorage( kvs attribute.Set, ) *storageHolder[Storage, int64] { @@ -64,10 +63,10 @@ func (cav *compiledAsyncBase[N, Storage, Methods]) NewAccumulator(kvs attribute. ac := &asyncAccumulator[N, Storage, Methods]{} ac.holder = cav.findStorage(kvs) - return ac } +// findStorage locates the output Storage for asynchronous instruments. func (cav *compiledAsyncBase[N, Storage, Methods]) findStorage( kvs attribute.Set, ) *storageHolder[Storage, notUsed] { @@ -76,8 +75,7 @@ func (cav *compiledAsyncBase[N, Storage, Methods]) findStorage( cav.instLock.Lock() defer cav.instLock.Unlock() - entry := cav.getOrCreateEntry(kvs) - return entry + return cav.getOrCreateEntry(kvs) } // multiAccumulator @@ -117,12 +115,11 @@ func (acc *syncAccumulator[N, Storage, Methods]) SnapshotAndProcess(final bool) methods.Move(&acc.current, &acc.snapshot) methods.Merge(&acc.snapshot, &acc.holder.storage) if final { + // On the final snapshot-and-process, decrement the auxiliary reference count. atomic.AddInt64(&acc.holder.auxiliary, -1) } } -type notUsed struct{} - // asyncAccumulator type asyncAccumulator[N number.Any, Storage any, Methods aggregator.Methods[N, Storage]] struct { asyncLock sync.Mutex diff --git a/lightstep/sdk/metric/internal/viewstate/base_instrument.go b/lightstep/sdk/metric/internal/viewstate/base_instrument.go index 4806db05..e8668378 100644 --- a/lightstep/sdk/metric/internal/viewstate/base_instrument.go +++ b/lightstep/sdk/metric/internal/viewstate/base_instrument.go @@ -26,15 +26,20 @@ import ( "go.opentelemetry.io/otel/attribute" ) -type syncAuxiliary int64 - -type asyncAuxiliary struct{} - +// storageHolder is a generic struct for holding one storage and one +// auxiliary field. Storage will be one of the aggregators. The +// auxiliary type depends on whether synchronous or asynchronous. +// +// Auxiliary is an int64 reference count for synchronous instruments +// and notUsed for asynchronous instruments. type storageHolder[Storage, Auxiliary any] struct { auxiliary Auxiliary storage Storage } +// notUsed is the Auxiliary type for asynchronous instruments. +type notUsed struct{} + // instrumentBase is the common type embedded in any of the compiled instrument views. type instrumentBase[N number.Any, Storage, Auxiliary any, Methods aggregator.Methods[N, Storage]] struct { instLock sync.Mutex diff --git a/lightstep/sdk/metric/internal/viewstate/viewstate.go b/lightstep/sdk/metric/internal/viewstate/viewstate.go index d6a02f81..55be61d3 100644 --- a/lightstep/sdk/metric/internal/viewstate/viewstate.go +++ b/lightstep/sdk/metric/internal/viewstate/viewstate.go @@ -110,6 +110,10 @@ type Accumulator interface { // There is no return value from this method; the caller can // safely forget an Accumulator after this method is called, // provided Update is not used again. + // + // When `final` is true, this is the last time the Accumulator + // will be snapshot and processed (according to the caller's + // reference counting). SnapshotAndProcess(final bool) } diff --git a/lightstep/sdk/metric/internal/viewstate/viewstate_test.go b/lightstep/sdk/metric/internal/viewstate/viewstate_test.go index 4a606fd8..b4d10d15 100644 --- a/lightstep/sdk/metric/internal/viewstate/viewstate_test.go +++ b/lightstep/sdk/metric/internal/viewstate/viewstate_test.go @@ -976,7 +976,7 @@ func TestDeltaTemporalityGauge(t *testing.T) { } // TestSyncDeltaTemporalityCounter ensures that counter and updowncounter -// are skip points with delta temporality and no change. +// skip points with delta temporality and no change. func TestSyncDeltaTemporalityCounter(t *testing.T) { views := view.New( "test", @@ -1080,3 +1080,57 @@ func TestSyncDeltaTemporalityCounter(t *testing.T) { expectValues(100, 100, seq) tick() } + +func TestSyncDeltaTemporalityMapDeletion(t *testing.T) { + views := view.New( + "test", + view.WithDefaultAggregationTemporalitySelector( + func(ik sdkinstrument.Kind) aggregation.Temporality { + return aggregation.DeltaTemporality // Always delta + }), + ) + + vc := New(testLib, views) + + inst, err := testCompile(vc, "counter", sdkinstrument.CounterKind, number.Float64Kind) + require.NoError(t, err) + + attr := attribute.String("A", "1") + set := attribute.NewSet(attr) + + acc1 := inst.NewAccumulator(set) + acc2 := inst.NewAccumulator(set) + + acc1.(Updater[float64]).Update(1) + acc2.(Updater[float64]).Update(1) + + // There are two references to one entry in the map. + require.Equal(t, 1, len(inst.(*statelessSyncInstrument[float64, sum.MonotonicFloat64, sum.MonotonicFloat64Methods]).data)) + + acc1.SnapshotAndProcess(false) + acc2.SnapshotAndProcess(true) + + var output data.Scope + + test.RequireEqualMetrics(t, + testCollectSequenceReuse(t, vc, testSequence, &output), + test.Instrument( + test.Descriptor("counter", sdkinstrument.CounterKind, number.Float64Kind), + test.Point(middleTime, endTime, sum.NewMonotonicFloat64(2), delta, attr), + ), + ) + + require.Equal(t, 1, len(inst.(*statelessSyncInstrument[float64, sum.MonotonicFloat64, sum.MonotonicFloat64Methods]).data)) + + acc1.SnapshotAndProcess(true) + + test.RequireEqualMetrics(t, + testCollectSequenceReuse(t, vc, testSequence, &output), + test.Instrument( + test.Descriptor("counter", sdkinstrument.CounterKind, number.Float64Kind), + ), + ) + + require.Equal(t, 0, len(inst.(*statelessSyncInstrument[float64, sum.MonotonicFloat64, sum.MonotonicFloat64Methods]).data)) + +} diff --git a/lightstep/sdk/metric/meter.go b/lightstep/sdk/metric/meter.go index 6831ef44..bdc9d91d 100644 --- a/lightstep/sdk/metric/meter.go +++ b/lightstep/sdk/metric/meter.go @@ -18,6 +18,13 @@ import ( "context" "sync" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/asyncstate" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/pipeline" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/syncstate" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/viewstate" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/number" + "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/sdkinstrument" + "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/metric/instrument" "go.opentelemetry.io/otel/metric/instrument/asyncfloat64" @@ -25,11 +32,6 @@ import ( "go.opentelemetry.io/otel/metric/instrument/syncfloat64" "go.opentelemetry.io/otel/metric/instrument/syncint64" "go.opentelemetry.io/otel/sdk/instrumentation" - "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/asyncstate" - "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/pipeline" - "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/syncstate" - "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/viewstate" - "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/sdkinstrument" ) // meter handles the creation and coordination of all metric instruments. A @@ -83,3 +85,82 @@ func (m *meter) SyncInt64() syncint64.InstrumentProvider { func (m *meter) SyncFloat64() syncfloat64.InstrumentProvider { return syncfloat64Instruments{m} } + +// instrumentConstructor refers to either the syncstate or asyncstate +// NewInstrument method. Although both receive an opaque interface{} +// to distinguish providers, only the asyncstate package needs to know +// this information. The unused parameter is passed to the syncstate +// package for the generalization used here to work. +type instrumentConstructor[T any] func( + instrument sdkinstrument.Descriptor, + opaque interface{}, + compiled pipeline.Register[viewstate.Instrument], +) *T + +// configureInstrument applies the instrument configuration, checks +// for an existing definition for the same descriptor, and compiles +// and constructs the instrument if necessary. +func configureInstrument[T any]( + m *meter, + name string, + opts []instrument.Option, + nk number.Kind, + ik sdkinstrument.Kind, + listPtr *[]*T, + ctor instrumentConstructor[T], +) (*T, error) { + // Compute the instrument descriptor + cfg := instrument.NewConfig(opts...) + desc := sdkinstrument.NewDescriptor(name, ik, nk, cfg.Description(), cfg.Unit()) + + m.lock.Lock() + defer m.lock.Unlock() + + // Lookup a pre-existing instrument by descriptor. + if lookup, has := m.byDesc[desc]; has { + // Recompute conflicts since they may have changed. + var conflicts viewstate.ViewConflictsBuilder + + for _, compiler := range m.compilers { + _, err := compiler.Compile(desc) + conflicts.Combine(err) + } + + return lookup.(*T), conflicts.AsError() + } + + // Compile the instrument for each pipeline. the first time. + var conflicts viewstate.ViewConflictsBuilder + compiled := pipeline.NewRegister[viewstate.Instrument](len(m.compilers)) + + for pipe, compiler := range m.compilers { + comp, err := compiler.Compile(desc) + compiled[pipe] = comp + conflicts.Combine(err) + } + + // Build the new instrument, cache it, append to the list. + inst := ctor(desc, m, compiled) + err := conflicts.AsError() + + if inst != nil { + m.byDesc[desc] = inst + } + *listPtr = append(*listPtr, inst) + if err != nil { + // Handle instrument creation errors when they're new, + // not for repeat entries above. + otel.Handle(err) + } + return inst, err +} + +// synchronousInstrument configures a synchronous instrument. +func (m *meter) synchronousInstrument(name string, opts []instrument.Option, nk number.Kind, ik sdkinstrument.Kind) (*syncstate.Instrument, error) { + return configureInstrument(m, name, opts, nk, ik, &m.syncInsts, syncstate.NewInstrument) +} + +// synchronousInstrument configures an asynchronous instrument. +func (m *meter) asynchronousInstrument(name string, opts []instrument.Option, nk number.Kind, ik sdkinstrument.Kind) (*asyncstate.Instrument, error) { + return configureInstrument(m, name, opts, nk, ik, &m.asyncInsts, asyncstate.NewInstrument) +} diff --git a/lightstep/sdk/metric/number/number.go b/lightstep/sdk/metric/number/number.go index 4d8ad69e..01540436 100644 --- a/lightstep/sdk/metric/number/number.go +++ b/lightstep/sdk/metric/number/number.go @@ -22,14 +22,14 @@ import "math" type Kind int8 const ( - // Int64Kind means that the Number stores int64. + // Int64Kind indicates int64. Int64Kind Kind = iota - // Float64Kind means that the Number stores float64. + // Float64Kind indicates float64. Float64Kind ) -// Number is a generic 64bit numeric value. +// Number is a 64bit numeric value, one of the Any interface types. type Number uint64 // Any is any of the supported generic Number types. diff --git a/lightstep/sdk/metric/number/traits.go b/lightstep/sdk/metric/number/traits.go index 6a6615a3..e23b708a 100644 --- a/lightstep/sdk/metric/number/traits.go +++ b/lightstep/sdk/metric/number/traits.go @@ -37,7 +37,7 @@ type Traits[N Any] interface { // AddAtomic sets `ptr` to `value+*ptr`. AddAtomic(ptr *N, value N) - // AddAtomic sets `ptr` to `value` and returns the former value. + // SwapAtomic sets `ptr` to `value` and returns the former value. SwapAtomic(ptr *N, value N) N // IsNaN indicates whether `math.IsNaN()` is true (impossible for int64). @@ -46,7 +46,7 @@ type Traits[N Any] interface { // IsInf indicates whether `math.IsInf()` is true (impossible for int64). IsInf(value N) bool - // Kind of + // Kind returns the number kind of these Traits. Kind() Kind } diff --git a/lightstep/sdk/metric/reader.go b/lightstep/sdk/metric/reader.go index b801de60..ac45f6a4 100644 --- a/lightstep/sdk/metric/reader.go +++ b/lightstep/sdk/metric/reader.go @@ -23,9 +23,9 @@ import ( // Reader is the interface used between the SDK and an // exporter. Control flow is bi-directional through the // Reader, since the SDK initiates ForceFlush and Shutdown -// while the initiates collection. The Register() method here -// informs the Reader that it can begin reading, signaling the -// start of bi-directional control flow. +// while the Reader initiates collection. The Register() +// method here informs the Reader that it can begin reading, +// signaling the start of bi-directional control flow. // // Typically, push-based exporters that are periodic will // implement PeroidicExporter themselves and construct a @@ -54,7 +54,7 @@ type Reader interface { type Producer interface { // Produce returns metrics from a single collection. // - // Produce may be called concurrently, + // Produce may be called concurrently. // // The `in` parameter supports re-use of memory from // one collection to the next. Callers that pass `in` diff --git a/lightstep/sdk/metric/view/standard.go b/lightstep/sdk/metric/view/standard.go index 48df963a..ce8ffc5d 100644 --- a/lightstep/sdk/metric/view/standard.go +++ b/lightstep/sdk/metric/view/standard.go @@ -20,8 +20,8 @@ import ( "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/sdkinstrument" ) -// StandardAggregation is the specified default aggregation Kind for -// each instrument Kind. +// StandardAggregationKind returns a function that configures the +// specified default aggregation Kind for each instrument Kind. func StandardAggregationKind(ik sdkinstrument.Kind) aggregation.Kind { switch ik { case sdkinstrument.HistogramKind: @@ -35,14 +35,15 @@ func StandardAggregationKind(ik sdkinstrument.Kind) aggregation.Kind { } } -// StandardTemporality returns the specified default Cumulative -// temporality for all instrument kinds. +// StandardTemporality returns a function that conifigures the +// specified default Cumulative temporality for all instrument kinds. func StandardTemporality(ik sdkinstrument.Kind) aggregation.Temporality { return aggregation.CumulativeTemporality } -// DeltaPreferredTemporality returns the specified Delta temporality -// for all instrument kinds except UpDownCounter, which remain Cumulative. +// DeltaPreferredTemporality returns a function that configures a +// preference for Delta temporality for all instrument kinds except +// UpDownCounter, which remain Cumulative. func DeltaPreferredTemporality(ik sdkinstrument.Kind) aggregation.Temporality { switch ik { case sdkinstrument.UpDownCounterKind, sdkinstrument.UpDownCounterObserverKind: @@ -52,7 +53,7 @@ func DeltaPreferredTemporality(ik sdkinstrument.Kind) aggregation.Temporality { } } -// StandardConfig returns two default-initialized aggregator.Configs. +// StandardConfig returns a function that configures two default aggregator.Configs. func StandardConfig(ik sdkinstrument.Kind) (ints, floats aggregator.Config) { return aggregator.Config{}, aggregator.Config{} } From b92304d9fa1a03f7fb31a907d95d3ff5cff04028 Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Tue, 7 Jun 2022 12:29:42 -0700 Subject: [PATCH 9/9] comments --- .../sdk/metric/internal/asyncstate/async.go | 4 ++-- .../metric/internal/asyncstate/callback.go | 4 ++-- .../sdk/metric/internal/syncstate/sync.go | 3 +++ .../internal/viewstate/base_instrument.go | 8 ++++++-- .../sdk/metric/internal/viewstate/doc.go | 19 +++++++++++++++++++ lightstep/sdk/metric/periodic.go | 6 +++--- 6 files changed, 35 insertions(+), 9 deletions(-) create mode 100644 lightstep/sdk/metric/internal/viewstate/doc.go diff --git a/lightstep/sdk/metric/internal/asyncstate/async.go b/lightstep/sdk/metric/internal/asyncstate/async.go index a30c821b..324c779f 100644 --- a/lightstep/sdk/metric/internal/asyncstate/async.go +++ b/lightstep/sdk/metric/internal/asyncstate/async.go @@ -104,7 +104,7 @@ func (inst *Instrument) SnapshotAndProcess(state *State) { } } -func (inst *Instrument) get(cs *callbackState, attrs []attribute.KeyValue) viewstate.Accumulator { +func (inst *Instrument) getOrCreate(cs *callbackState, attrs []attribute.KeyValue) viewstate.Accumulator { comp := inst.compiled[cs.state.pipe] if comp == nil { @@ -153,7 +153,7 @@ func capture[N number.Any, Traits number.Traits[N]](ctx context.Context, inst *I return } - if acc := inst.get(cs, attrs); acc != nil { + if acc := inst.getOrCreate(cs, attrs); acc != nil { acc.(viewstate.Updater[N]).Update(value) } } diff --git a/lightstep/sdk/metric/internal/asyncstate/callback.go b/lightstep/sdk/metric/internal/asyncstate/callback.go index 046ed29f..22fd7b79 100644 --- a/lightstep/sdk/metric/internal/asyncstate/callback.go +++ b/lightstep/sdk/metric/internal/asyncstate/callback.go @@ -28,8 +28,8 @@ type Callback struct { // function is the user-provided callback function. function func(context.Context) - // instruments are the instruments permitted to be - // used inside this callback. + // instruments are the set of instruments permitted to be used + // inside this callback. instruments map[*Instrument]struct{} } diff --git a/lightstep/sdk/metric/internal/syncstate/sync.go b/lightstep/sdk/metric/internal/syncstate/sync.go index 16d77ed2..b481197b 100644 --- a/lightstep/sdk/metric/internal/syncstate/sync.go +++ b/lightstep/sdk/metric/internal/syncstate/sync.go @@ -202,6 +202,9 @@ func acquireRecord[N number.Any](inst *Instrument, attrs []attribute.KeyValue) ( if oldRec.refMapped.ref() { return oldRec, oldRec.accumulator.(viewstate.Updater[N]) } + // When this happens, we are waiting for the call to Delete() + // inside SnapshotAndProcess() to complete before inserting + // a new record. This avoids busy-waiting. runtime.Gosched() continue } diff --git a/lightstep/sdk/metric/internal/viewstate/base_instrument.go b/lightstep/sdk/metric/internal/viewstate/base_instrument.go index e8668378..47eec43d 100644 --- a/lightstep/sdk/metric/internal/viewstate/base_instrument.go +++ b/lightstep/sdk/metric/internal/viewstate/base_instrument.go @@ -125,8 +125,12 @@ func (metric *instrumentBase[N, Storage, Auxiliary, Methods]) appendInstrument(o return inst } -// copyPoint is used in cases where the output Aggregation is a copy -// of the stored object. +// appendPoint adds a new point to the output. Note that the existing +// slice will be extended, if possible, and the existing Aggregation +// is potentially re-used. The variable `reset` determines whether +// Move() or Copy() is used. Note that both Move and Copy are +// synchronized with respect to Update() and Merge(), necesary for the +// synchronous code path which may see concurrent collection. func (metric *instrumentBase[N, Storage, Auxiliary, Methods]) appendPoint(inst *data.Instrument, set attribute.Set, storage *Storage, tempo aggregation.Temporality, start, end time.Time, reset bool) { var methods Methods diff --git a/lightstep/sdk/metric/internal/viewstate/doc.go b/lightstep/sdk/metric/internal/viewstate/doc.go new file mode 100644 index 00000000..f4c3e156 --- /dev/null +++ b/lightstep/sdk/metric/internal/viewstate/doc.go @@ -0,0 +1,19 @@ +// 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. + +// package viewstate implements a View compiler, which combines +// information about the instrument kind (especially synchronous +// vs. asynchronous), the configured view(s) and reader defaults, and +// conflicting instruments in the same namespace. +package viewstate // import "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/viewstate" diff --git a/lightstep/sdk/metric/periodic.go b/lightstep/sdk/metric/periodic.go index 473b44df..6ef7fda2 100644 --- a/lightstep/sdk/metric/periodic.go +++ b/lightstep/sdk/metric/periodic.go @@ -86,9 +86,9 @@ func (pr *PeriodicReader) start(ctx context.Context) { } } -// Shutdown stops the stops the export loop, canceling its Context, -// and waits for it to return. Then it issues a ShutdownMetrics with -// final data. +// Shutdown stops the export loop, canceling its Context, and waits +// for it to return. Then it issues a ShutdownMetrics with final +// data. func (pr *PeriodicReader) Shutdown(ctx context.Context) error { pr.stop() pr.wait.Wait()