Skip to content

Commit

Permalink
Remove Labelset (#595)
Browse files Browse the repository at this point in the history 
* Remove LabelSet frmo api/metric

* SDK tests pass

* Restore benchmarks

* All tests pass

* Remove all mentions of LabelSet

* Test RecordBatch

* Batch test

* Improves benchmark (some)

* Move the benchmark to match HEAD

* Align labels for GOARCH=386

* Add alignment test

* Disable the stress test fo GOARCH=386

* Fix bug

* Move atomic fields into their own file

* Add a TODO

* Comments

* Remove metric.Labels(...)

* FTB

Co-authored-by: Liz Fong-Jones <lizf@honeycomb.io>
  • Loading branch information
@jmacd @lizthegrey
jmacd and lizthegrey committed Mar 27, 2020
1 parent e7a9ba1 commit e8546e3
Show file tree
Hide file tree
Showing 29 changed files with 435 additions and 476 deletions.
9 changes: 5 additions & 4 deletions api/global/internal/benchmark_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"strings"
"testing"

"go.opentelemetry.io/otel/api/core"
"go.opentelemetry.io/otel/api/global"
"go.opentelemetry.io/otel/api/global/internal"
"go.opentelemetry.io/otel/api/key"
Expand Down Expand Up @@ -90,13 +91,13 @@ func BenchmarkGlobalInt64CounterAddNoSDK(b *testing.B) {
internal.ResetForTest()
ctx := context.Background()
sdk := global.Meter("test")
labs := sdk.Labels(key.String("A", "B"))
labs := []core.KeyValue{key.String("A", "B")}
cnt := Must(sdk).NewInt64Counter("int64.counter")

b.ResetTimer()

for i := 0; i < b.N; i++ {
cnt.Add(ctx, 1, labs)
cnt.Add(ctx, 1, labs...)
}
}

Expand All @@ -109,13 +110,13 @@ func BenchmarkGlobalInt64CounterAddWithSDK(b *testing.B) {

global.SetMeterProvider(fix)

labs := sdk.Labels(key.String("A", "B"))
labs := []core.KeyValue{key.String("A", "B")}
cnt := Must(sdk).NewInt64Counter("int64.counter")

b.ResetTimer()

for i := 0; i < b.N; i++ {
cnt.Add(ctx, 1, labs)
cnt.Add(ctx, 1, labs...)
}
}

Expand Down
52 changes: 4 additions & 48 deletions api/global/internal/meter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,6 @@ import (
// provider. Mutexes in the Provider and Meters ensure that each
// instrument has a delegate before the global provider is set.
//
// LabelSets are implemented by delegating to the Meter instance using
// the metric.LabelSetDelegator interface.
//
// Bound instrument operations are implemented by delegating to the
// instrument after it is registered, with a sync.Once initializer to
// protect against races with Release().
Expand Down Expand Up @@ -100,28 +97,17 @@ type AsyncImpler interface {
AsyncImpl() metric.AsyncImpl
}

type labelSet struct {
delegate unsafe.Pointer // (* metric.LabelSet)

meter *meter
value []core.KeyValue

initialize sync.Once
}

type syncHandle struct {
delegate unsafe.Pointer // (*metric.HandleImpl)

inst *syncImpl
labels metric.LabelSet
labels []core.KeyValue

initialize sync.Once
}

var _ metric.Provider = &meterProvider{}
var _ metric.Meter = &meter{}
var _ metric.LabelSet = &labelSet{}
var _ metric.LabelSetDelegate = &labelSet{}
var _ metric.InstrumentImpl = &syncImpl{}
var _ metric.BoundSyncImpl = &syncHandle{}
var _ metric.AsyncImpl = &asyncImpl{}
Expand Down Expand Up @@ -254,7 +240,7 @@ func (inst *syncImpl) Implementation() interface{} {
return inst
}

func (inst *syncImpl) Bind(labels metric.LabelSet) metric.BoundSyncImpl {
func (inst *syncImpl) Bind(labels []core.KeyValue) metric.BoundSyncImpl {
if implPtr := (*metric.SyncImpl)(atomic.LoadPointer(&inst.delegate)); implPtr != nil {
return (*implPtr).Bind(labels)
}
Expand Down Expand Up @@ -340,13 +326,13 @@ func (obs *asyncImpl) setDelegate(d metric.Meter) {

// Metric updates

func (m *meter) RecordBatch(ctx context.Context, labels metric.LabelSet, measurements ...metric.Measurement) {
func (m *meter) RecordBatch(ctx context.Context, labels []core.KeyValue, measurements ...metric.Measurement) {
if delegatePtr := (*metric.Meter)(atomic.LoadPointer(&m.delegate)); delegatePtr != nil {
(*delegatePtr).RecordBatch(ctx, labels, measurements...)
}
}

func (inst *syncImpl) RecordOne(ctx context.Context, number core.Number, labels metric.LabelSet) {
func (inst *syncImpl) RecordOne(ctx context.Context, number core.Number, labels []core.KeyValue) {
if instPtr := (*metric.SyncImpl)(atomic.LoadPointer(&inst.delegate)); instPtr != nil {
(*instPtr).RecordOne(ctx, number, labels)
}
Expand Down Expand Up @@ -377,35 +363,6 @@ func (bound *syncHandle) RecordOne(ctx context.Context, number core.Number) {
(*implPtr).RecordOne(ctx, number)
}

// LabelSet initialization

func (m *meter) Labels(labels ...core.KeyValue) metric.LabelSet {
return &labelSet{
meter: m,
value: labels,
}
}

func (labels *labelSet) Delegate() metric.LabelSet {
meterPtr := (*metric.Meter)(atomic.LoadPointer(&labels.meter.delegate))
if meterPtr == nil {
// This is technically impossible, provided the global
// Meter is updated after the meters and instruments
// have been delegated.
return labels
}
var implPtr *metric.LabelSet
labels.initialize.Do(func() {
implPtr = new(metric.LabelSet)
*implPtr = (*meterPtr).Labels(labels.value...)
atomic.StorePointer(&labels.delegate, unsafe.Pointer(implPtr))
})
if implPtr == nil {
implPtr = (*metric.LabelSet)(atomic.LoadPointer(&labels.delegate))
}
return (*implPtr)
}

// Constructors

func (m *meter) withName(opts []metric.Option) []metric.Option {
Expand Down Expand Up @@ -466,7 +423,6 @@ func AtomicFieldOffsets() map[string]uintptr {
"meter.delegate": unsafe.Offsetof(meter{}.delegate),
"syncImpl.delegate": unsafe.Offsetof(syncImpl{}.delegate),
"asyncImpl.delegate": unsafe.Offsetof(asyncImpl{}.delegate),
"labelSet.delegate": unsafe.Offsetof(labelSet{}.delegate),
"syncHandle.delegate": unsafe.Offsetof(syncHandle{}.delegate),
}
}
84 changes: 39 additions & 45 deletions api/global/internal/meter_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ func asStructs(batches []metrictest.Batch) []measured {
r = append(r, measured{
Name: m.Instrument.Descriptor().Name(),
LibraryName: m.Instrument.Descriptor().LibraryName(),
Labels: batch.LabelSet.Labels,
Labels: asMap(batch.Labels...),
Number: m.Number,
})
}
Expand All @@ -72,41 +72,38 @@ func TestDirect(t *testing.T) {
ctx := context.Background()
meter1 := global.Meter("test1")
meter2 := global.Meter("test2")
lvals1 := key.String("A", "B")
labels1 := meter1.Labels(lvals1)
lvals2 := key.String("C", "D")
labels2 := meter1.Labels(lvals2)
lvals3 := key.String("E", "F")
labels3 := meter2.Labels(lvals3)
labels1 := []core.KeyValue{key.String("A", "B")}
labels2 := []core.KeyValue{key.String("C", "D")}
labels3 := []core.KeyValue{key.String("E", "F")}

counter := Must(meter1).NewInt64Counter("test.counter")
counter.Add(ctx, 1, labels1)
counter.Add(ctx, 1, labels1)
counter.Add(ctx, 1, labels1...)
counter.Add(ctx, 1, labels1...)

measure := Must(meter1).NewFloat64Measure("test.measure")
measure.Record(ctx, 1, labels1)
measure.Record(ctx, 2, labels1)
measure.Record(ctx, 1, labels1...)
measure.Record(ctx, 2, labels1...)

_ = Must(meter1).RegisterFloat64Observer("test.observer.float", func(result metric.Float64ObserverResult) {
result.Observe(1., labels1)
result.Observe(2., labels2)
result.Observe(1., labels1...)
result.Observe(2., labels2...)
})

_ = Must(meter1).RegisterInt64Observer("test.observer.int", func(result metric.Int64ObserverResult) {
result.Observe(1, labels1)
result.Observe(2, labels2)
result.Observe(1, labels1...)
result.Observe(2, labels2...)
})

second := Must(meter2).NewFloat64Measure("test.second")
second.Record(ctx, 1, labels3)
second.Record(ctx, 2, labels3)
second.Record(ctx, 1, labels3...)
second.Record(ctx, 2, labels3...)

mock, provider := metrictest.NewProvider()
global.SetMeterProvider(provider)

counter.Add(ctx, 1, labels1)
measure.Record(ctx, 3, labels1)
second.Record(ctx, 3, labels3)
counter.Add(ctx, 1, labels1...)
measure.Record(ctx, 3, labels1...)
second.Record(ctx, 3, labels3...)

mock.RunAsyncInstruments()

Expand All @@ -117,43 +114,43 @@ func TestDirect(t *testing.T) {
{
Name: "test.counter",
LibraryName: "test1",
Labels: asMap(lvals1),
Labels: asMap(labels1...),
Number: asInt(1),
},
{
Name: "test.measure",
LibraryName: "test1",
Labels: asMap(lvals1),
Labels: asMap(labels1...),
Number: asFloat(3),
},
{
Name: "test.second",
LibraryName: "test2",
Labels: asMap(lvals3),
Labels: asMap(labels3...),
Number: asFloat(3),
},
{
Name: "test.observer.float",
LibraryName: "test1",
Labels: asMap(lvals1),
Labels: asMap(labels1...),
Number: asFloat(1),
},
{
Name: "test.observer.float",
LibraryName: "test1",
Labels: asMap(lvals2),
Labels: asMap(labels2...),
Number: asFloat(2),
},
{
Name: "test.observer.int",
LibraryName: "test1",
Labels: asMap(lvals1),
Labels: asMap(labels1...),
Number: asInt(1),
},
{
Name: "test.observer.int",
LibraryName: "test1",
Labels: asMap(lvals2),
Labels: asMap(labels2...),
Number: asInt(2),
},
},
Expand All @@ -168,16 +165,15 @@ func TestBound(t *testing.T) {
// vs. the above, to cover all the instruments.
ctx := context.Background()
glob := global.Meter("test")
lvals1 := key.String("A", "B")
labels1 := glob.Labels(lvals1)
labels1 := []core.KeyValue{key.String("A", "B")}

counter := Must(glob).NewFloat64Counter("test.counter")
boundC := counter.Bind(labels1)
boundC := counter.Bind(labels1...)
boundC.Add(ctx, 1)
boundC.Add(ctx, 1)

measure := Must(glob).NewInt64Measure("test.measure")
boundM := measure.Bind(labels1)
boundM := measure.Bind(labels1...)
boundM.Record(ctx, 1)
boundM.Record(ctx, 2)

Expand All @@ -192,13 +188,13 @@ func TestBound(t *testing.T) {
{
Name: "test.counter",
LibraryName: "test",
Labels: asMap(lvals1),
Labels: asMap(labels1...),
Number: asFloat(1),
},
{
Name: "test.measure",
LibraryName: "test",
Labels: asMap(lvals1),
Labels: asMap(labels1...),
Number: asInt(3),
},
},
Expand All @@ -213,14 +209,13 @@ func TestUnbind(t *testing.T) {
internal.ResetForTest()

glob := global.Meter("test")
lvals1 := key.New("A").String("B")
labels1 := glob.Labels(lvals1)
labels1 := []core.KeyValue{key.String("A", "B")}

counter := Must(glob).NewFloat64Counter("test.counter")
boundC := counter.Bind(labels1)
boundC := counter.Bind(labels1...)

measure := Must(glob).NewInt64Measure("test.measure")
boundM := measure.Bind(labels1)
boundM := measure.Bind(labels1...)

boundC.Unbind()
boundM.Unbind()
Expand All @@ -231,12 +226,11 @@ func TestDefaultSDK(t *testing.T) {

ctx := context.Background()
meter1 := global.Meter("builtin")
lvals1 := key.String("A", "B")
labels1 := meter1.Labels(lvals1)
labels1 := []core.KeyValue{key.String("A", "B")}

counter := Must(meter1).NewInt64Counter("test.builtin")
counter.Add(ctx, 1, labels1)
counter.Add(ctx, 1, labels1)
counter.Add(ctx, 1, labels1...)
counter.Add(ctx, 1, labels1...)

in, out := io.Pipe()
pusher, err := stdout.InstallNewPipeline(stdout.Config{
Expand All @@ -247,7 +241,7 @@ func TestDefaultSDK(t *testing.T) {
panic(err)
}

counter.Add(ctx, 1, labels1)
counter.Add(ctx, 1, labels1...)

ch := make(chan string)
go func() {
Expand All @@ -270,7 +264,7 @@ func TestUnbindThenRecordOne(t *testing.T) {

meter := global.Meter("test")
counter := Must(meter).NewInt64Counter("test.counter")
boundC := counter.Bind(meter.Labels())
boundC := counter.Bind()
global.SetMeterProvider(provider)
boundC.Unbind()

Expand Down Expand Up @@ -312,8 +306,8 @@ func TestErrorInDeferredConstructor(t *testing.T) {
global.SetMeterProvider(sdk)
})

c1.Add(ctx, 1, meter.Labels())
c2.Add(ctx, 2, meter.Labels())
c1.Add(ctx, 1)
c2.Add(ctx, 2)
}

func TestImplementationIndirection(t *testing.T) {
Expand Down
11 changes: 1 addition & 10 deletions api/metric/api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,6 @@ type Provider interface {
Meter(name string) Meter
}

// LabelSet is an implementation-level interface that represents a
// []core.KeyValue for use as pre-defined labels in the metrics API.
type LabelSet interface {
}

// Config contains some options for metrics of any kind.
type Config struct {
// Description is an optional field describing the metric
Expand Down Expand Up @@ -161,12 +156,8 @@ func (d Descriptor) LibraryName() string {

// Meter is an interface to the metrics portion of the OpenTelemetry SDK.
type Meter interface {
// Labels returns a reference to a set of labels that cannot
// be read by the application.
Labels(...core.KeyValue) LabelSet

// RecordBatch atomically records a batch of measurements.
RecordBatch(context.Context, LabelSet, ...Measurement)
RecordBatch(context.Context, []core.KeyValue, ...Measurement)

// All instrument constructors may return an error for
// conditions such as:
Expand Down
Loading

0 comments on commit e8546e3

Please sign in to comment.