diff --git a/bridges/otelzap/core.go b/bridges/otelzap/core.go index 59914e125c3..b3c9a48f9b4 100644 --- a/bridges/otelzap/core.go +++ b/bridges/otelzap/core.go @@ -172,9 +172,9 @@ func (o *Core) Write(ent zapcore.Entry, fields []zapcore.Field) error { } func convertField(fields []zapcore.Field) (context.Context, []log.KeyValue) { - // TODO: Use objectEncoder from a pool instead of newObjectEncoder. var ctx context.Context - enc := newObjectEncoder(len(fields)) + enc, free := getObjectEncoder() + defer free() for _, field := range fields { if ctxFld, ok := field.Interface.(context.Context); ok { ctx = ctxFld diff --git a/bridges/otelzap/encoder.go b/bridges/otelzap/encoder.go index 8b962bd309c..0481ca19c79 100644 --- a/bridges/otelzap/encoder.go +++ b/bridges/otelzap/encoder.go @@ -6,6 +6,7 @@ package otelzap // import "go.opentelemetry.io/contrib/bridges/otelzap" import ( "fmt" "reflect" + "sync" "time" "go.uber.org/zap/zapcore" @@ -13,6 +14,47 @@ import ( "go.opentelemetry.io/otel/log" ) +var arrayEncoderPool = sync.Pool{ + New: func() interface{} { + // Similar to console_encoder which uses capacity of 2 + return &arrayEncoder{elems: make([]log.Value, 0, 2)} + }, +} + +func getArrayEncoder() (arr *arrayEncoder, free func()) { + arr = arrayEncoderPool.Get().(*arrayEncoder) + return arr, func() { + arr.elems = arr.elems[:0] + // TODO: limit returned size so the pool doesn't hold on to very large + // buffers. Idea is based on + // https://cs.opensource.google/go/x/exp/+/814bf88c:slog/internal/buffer/buffer.go;l=27-34 + + arrayEncoderPool.Put(arr) + } +} + +var objectEncoderPool = sync.Pool{ + New: func() interface{} { + // Similar to console_encoder which uses capacity of 2 + return newObjectEncoder(2) + }, +} + +func getObjectEncoder() (obj *objectEncoder, free func()) { + obj = objectEncoderPool.Get().(*objectEncoder) + return obj, func() { + // TODO: limit returned size so the pool doesn't hold on to very large + // buffers. Idea is based on + // https://cs.opensource.google/go/x/exp/+/814bf88c:slog/internal/buffer/buffer.go;l=27-34 + + obj.root.attrs = obj.root.attrs[:0] + obj.root.name = "" + obj.root.next = nil + obj.cur = obj.root + objectEncoderPool.Put(obj) + } +} + var ( _ zapcore.ObjectEncoder = (*objectEncoder)(nil) _ zapcore.ArrayEncoder = (*arrayEncoder)(nil) @@ -55,16 +97,16 @@ func (m *objectEncoder) calculate(o *namespace) { } func (m *objectEncoder) AddArray(key string, v zapcore.ArrayMarshaler) error { - // TODO: Use arrayEncoder from a pool. - arr := &arrayEncoder{} + arr, free := getArrayEncoder() + defer free() err := v.MarshalLogArray(arr) m.cur.attrs = append(m.cur.attrs, log.Slice(key, arr.elems...)) return err } func (m *objectEncoder) AddObject(k string, v zapcore.ObjectMarshaler) error { - // TODO: Use objectEncoder from a pool. - newobj := newObjectEncoder(2) + newobj, free := getObjectEncoder() + defer free() err := v.MarshalLogObject(newobj) newobj.calculate(newobj.root) m.cur.attrs = append(m.cur.attrs, log.Map(k, newobj.root.attrs...)) @@ -196,7 +238,6 @@ type arrayEncoder struct { } func (a *arrayEncoder) AppendArray(v zapcore.ArrayMarshaler) error { - // TODO: Use arrayEncoder from a pool. arr := &arrayEncoder{} err := v.MarshalLogArray(arr) a.elems = append(a.elems, log.SliceValue(arr.elems...)) @@ -204,7 +245,6 @@ func (a *arrayEncoder) AppendArray(v zapcore.ArrayMarshaler) error { } func (a *arrayEncoder) AppendObject(v zapcore.ObjectMarshaler) error { - // TODO: Use objectEncoder from a pool. m := newObjectEncoder(2) err := v.MarshalLogObject(m) m.calculate(m.root) diff --git a/bridges/otelzap/encoder_test.go b/bridges/otelzap/encoder_test.go index 4e284b82d35..67c2fae4816 100644 --- a/bridges/otelzap/encoder_test.go +++ b/bridges/otelzap/encoder_test.go @@ -7,6 +7,7 @@ package otelzap import ( "errors" + "fmt" "testing" "time" @@ -17,15 +18,16 @@ import ( "go.opentelemetry.io/otel/log" ) +var wantTurducken = map[string]interface{}{ + "ducks": []interface{}{ + map[string]interface{}{"in": "chicken"}, + map[string]interface{}{"in": "chicken"}, + }, +} + // Copied from https://github.com/uber-go/zap/blob/b39f8b6b6a44d8371a87610be50cce58eeeaabcb/zapcore/memory_encoder_test.go. func TestObjectEncoder(t *testing.T) { // Expected output of a turducken. - wantTurducken := map[string]interface{}{ - "ducks": []interface{}{ - map[string]interface{}{"in": "chicken"}, - map[string]interface{}{"in": "chicken"}, - }, - } tests := []struct { desc string @@ -65,6 +67,13 @@ func TestObjectEncoder(t *testing.T) { }, expected: []interface{}{wantTurducken, wantTurducken}, }, + { + desc: "AddArray-with AppendArray", + f: func(e zapcore.ObjectEncoder) { + assert.NoError(t, e.AddArray("k", number(2)), "Expected AddArray to succeed.") + }, + expected: []interface{}{[]interface{}{"1"}, []interface{}{"2"}}, + }, { desc: "AddReflected", f: func(e zapcore.ObjectEncoder) { @@ -232,6 +241,51 @@ func TestObjectEncoder(t *testing.T) { } } +// This test checks when multiple fields are added to the objectEncoder +// It is also a check for any possible shared memory issue on using pools. +func TestObjectEncoderMultiple(t *testing.T) { + tests := []struct { + desc string + f func(zapcore.ObjectEncoder) + expected []interface{} + }{ + { + desc: "AddArray-with AppendArray", + f: func(e zapcore.ObjectEncoder) { + assert.NoError(t, e.AddArray("k", number(2)), "Expected AddArray to succeed.") + assert.NoError(t, e.AddArray("k", number(3)), "Expected AddArray to succeed.") + }, + expected: []interface{}{ + []interface{}{[]interface{}{"1"}, []interface{}{"2"}}, + []interface{}{[]interface{}{"1"}, []interface{}{"2"}, []interface{}{"3"}}, + }, + }, + { + desc: "AddObject (nested)", + f: func(e zapcore.ObjectEncoder) { + assert.NoError(t, e.AddObject("k", turducken{}), "Expected AddObject to succeed.") + assert.NoError(t, e.AddObject("k", turducken{}), "Expected AddObject to succeed.") + }, + expected: []interface{}{ + wantTurducken, + wantTurducken, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + enc := newObjectEncoder(1) + tt.f(enc) + enc.calculate(enc.root) + require.Len(t, enc.root.attrs, len(tt.expected)) + for i, outer := range tt.expected { + assert.Equal(t, outer, value2Result((enc.root.attrs[i].Value)), "Unexpected encoder output.") + } + }) + } +} + // Copied from https://github.com/uber-go/zap/blob/b39f8b6b6a44d8371a87610be50cce58eeeaabcb/zapcore/memory_encoder_test.go. func TestArrayEncoder(t *testing.T) { tests := []struct { @@ -361,6 +415,23 @@ func (t turduckens) MarshalLogArray(enc zapcore.ArrayEncoder) error { return err } +type number int + +func (t number) MarshalLogArray(enc zapcore.ArrayEncoder) error { + var err error + for i := 0; i < int(t); i++ { + err = errors.Join(err, enc.AppendArray(numberString(fmt.Sprint(i+1)))) + } + return err +} + +type numberString string + +func (t numberString) MarshalLogArray(enc zapcore.ArrayEncoder) error { + enc.AppendString(string(t)) + return nil +} + type loggable struct{ bool } func (l loggable) MarshalLogObject(enc zapcore.ObjectEncoder) error {