Skip to content

Commit

Permalink
Optimize and benchmark handling of invalid keys (#276)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jmacd
jmacd authored Sep 12, 2022
1 parent 5e6c4ed commit 3f456dc
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 28 deletions.
74 changes: 59 additions & 15 deletions lightstep/sdk/metric/benchmark_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,25 @@ import (
"testing"

"github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/data"
"github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/view"
"go.opentelemetry.io/otel/attribute"
)

// Tested prior to 0.11.0 release
// goos: darwin
// goarch: arm64
// pkg: github.com/lightstep/otel-launcher-go/lightstep/sdk/metric
// BenchmarkCounterAddNoAttrs-10 35354023 33.79 ns/op 0 B/op 0 allocs/op
// BenchmarkCounterAddOneAttr-10 14354538 82.77 ns/op 64 B/op 1 allocs/op
// BenchmarkCounterAddOneInvalidAttr-10 9307794 128.4 ns/op 128 B/op 1 allocs/op
// BenchmarkCounterAddManyAttrs-10 1000000 1075 ns/op 569 B/op 6 allocs/op
// BenchmarkCounterAddManyInvalidAttrs-10 832549 1654 ns/op 1080 B/op 10 allocs/op
// BenchmarkCounterAddManyFilteredAttrs-10 1000000 1304 ns/op 953 B/op 8 allocs/op
// BenchmarkCounterCollectOneAttrNoReuse-10 2537348 468.0 ns/op 400 B/op 7 allocs/op
// BenchmarkCounterCollectOneAttrWithReuse-10 3679694 328.2 ns/op 136 B/op 3 allocs/op
// BenchmarkCounterCollectTenAttrs-10 715490 1635 ns/op 712 B/op 12 allocs/op
// BenchmarkCounterCollectTenAttrsTenTimes-10 72478 16475 ns/op 7128 B/op 120 allocs/op

func BenchmarkCounterAddNoAttrs(b *testing.B) {
ctx := context.Background()
rdr := NewManualReader("bench")
Expand All @@ -35,10 +51,6 @@ func BenchmarkCounterAddNoAttrs(b *testing.B) {
}
}

// 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")
Expand All @@ -52,17 +64,19 @@ func BenchmarkCounterAddOneAttr(b *testing.B) {
}
}

// 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 BenchmarkCounterAddOneInvalidAttr(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("", "V"), attribute.String("K", "V"))
}
}

func BenchmarkCounterAddManyAttrs(b *testing.B) {
ctx := context.Background()
rdr := NewManualReader("bench")
Expand All @@ -76,6 +90,36 @@ func BenchmarkCounterAddManyAttrs(b *testing.B) {
}
}

func BenchmarkCounterAddManyInvalidAttrs(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("", i), attribute.Int("K", i))
}
}

func BenchmarkCounterAddManyFilteredAttrs(b *testing.B) {
ctx := context.Background()
rdr := NewManualReader("bench")
provider := NewMeterProvider(
WithReader(rdr,
view.WithClause(view.WithKeys([]attribute.Key{"K"})),
),
)
b.ReportAllocs()

cntr, _ := provider.Meter("test").SyncInt64().Counter("hello")

for i := 0; i < b.N; i++ {
cntr.Add(ctx, 1, attribute.Int("L", i), attribute.Int("K", i))
}
}

func BenchmarkCounterCollectOneAttrNoReuse(b *testing.B) {
ctx := context.Background()
rdr := NewManualReader("bench")
Expand Down
40 changes: 30 additions & 10 deletions lightstep/sdk/metric/internal/viewstate/base_instrument.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,22 +96,42 @@ func (metric *instrumentBase[N, Storage, Auxiliary, Methods]) mergeDescription(d
}
}

func attributeFilter(viewf *attribute.Filter) attribute.Filter {
return func(kv attribute.KeyValue) bool {
if kv.Key == "" {
// isValidAttribute supports filtering invalid attributes. Note, this
// should be fast, trye not to allocate! Note: the specification is
// somewhat ambiguous about empty strings, see
// https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/common/attribute-naming.md
// which just says "valid Unicode sequence".
func isValidAttribute(kv attribute.KeyValue) bool {
return kv.Key != ""
}

func (metric *instrumentBase[N, Storage, Auxiliary, Methods]) invalidAttributeFilter(kv attribute.KeyValue) bool {
return isValidAttribute(kv) && (metric.keysFilter == nil || (*metric.keysFilter)(kv))
}

func (metric *instrumentBase[N, Storage, Auxiliary, Methods]) applyKeysFilter(kvs attribute.Set) attribute.Set {
invalidFilter := false
for iter := kvs.Iter(); iter.Next(); {
kv := iter.Attribute()
if !isValidAttribute(kv) {
doevery.TimePeriod(time.Minute, func() {
otel.Handle(fmt.Errorf("use of empty attribute key, e.g., with value %q", kv.Value.Emit()))
})
return false
invalidFilter = true
break
}

return viewf == nil || (*viewf)(kv)
}
}

func (metric *instrumentBase[N, Storage, Auxiliary, Methods]) applyKeysFilter(kvs attribute.Set) attribute.Set {
kvs, _ = attribute.NewSetWithFiltered(kvs.ToSlice(), attributeFilter(metric.keysFilter))
return kvs
if !invalidFilter && metric.keysFilter == nil {
return kvs
}
var res attribute.Set
if !invalidFilter {
res, _ = kvs.Filter(*metric.keysFilter)
} else {
res, _ = kvs.Filter(metric.invalidAttributeFilter)
}
return res
}

func (metric *instrumentBase[N, Storage, Auxiliary, Methods]) getOrCreateEntry(kvs attribute.Set) *storageHolder[Storage, Auxiliary] {
Expand Down
7 changes: 4 additions & 3 deletions lightstep/sdk/metric/internal/viewstate/viewstate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1703,11 +1703,11 @@ func TestEmptyKeyFilter(t *testing.T) {
inst, err := testCompile(vc, "foo", sdkinstrument.SyncCounter, number.Float64Kind)
require.NoError(t, err)

acc1 := inst.NewAccumulator(attribute.NewSet(attribute.String("", "1value")))
acc1 := inst.NewAccumulator(attribute.NewSet(attribute.String("", "1value"), attribute.String("K", "V")))
acc1.(Updater[float64]).Update(1)
acc1.SnapshotAndProcess(false)

acc2 := inst.NewAccumulator(attribute.NewSet(attribute.String("", "2value")))
acc2 := inst.NewAccumulator(attribute.NewSet(attribute.String("", "2value"), attribute.String("K", "V")))
acc2.(Updater[float64]).Update(1)
acc2.SnapshotAndProcess(false)

Expand All @@ -1718,6 +1718,7 @@ func TestEmptyKeyFilter(t *testing.T) {
test.Descriptor("foo", sdkinstrument.SyncCounter, number.Float64Kind),
test.Point(
startTime, endTime, sum.NewMonotonicFloat64(2), cumulative,
attribute.String("K", "V"),
),
),
)
Expand All @@ -1739,7 +1740,7 @@ func TestEmptyKeyFilterAndView(t *testing.T) {
inst, err := testCompile(vc, "foo", sdkinstrument.SyncCounter, number.Float64Kind)
require.NoError(t, err)

acc1 := inst.NewAccumulator(attribute.NewSet(attribute.String("a", "1"), attribute.String("", "empty")))
acc1 := inst.NewAccumulator(attribute.NewSet(attribute.String("a", "1"), attribute.String("", "empty"), attribute.String("b", "ignored")))
acc1.(Updater[float64]).Update(1)
acc1.SnapshotAndProcess(false)

Expand Down

0 comments on commit 3f456dc

Please sign in to comment.