Skip to content

Commit

Permalink
Revert "Remove Entry.With method"
Browse files Browse the repository at this point in the history
This reverts commit 80d8c30

It turns out that this change is unsafe from concurrency perspective.
  • Loading branch information
elgopher committed Feb 3, 2022
1 parent e015a82 commit b575382
Show file tree
Hide file tree
Showing 8 changed files with 50 additions and 19 deletions.
3 changes: 1 addition & 2 deletions adapter/glogadapter/glog_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,7 @@ func TestAdapter_Log(t *testing.T) {
entry := logger.Entry{
Level: logger.ErrorLevel,
Message: message,
}
entry.Fields = append(entry.Fields, logger.Field{
}.With(logger.Field{
Key: "k",
Value: "v",
})
Expand Down
11 changes: 4 additions & 7 deletions adapter/internal/adaptertest/adaptertest.go
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,7 @@ func Run(t *testing.T, subject Subject) { // nolint
for name, field := range fields {
var builder strings.Builder
adapter := subject.NewAdapter(&builder)
entryWithField := entry
entryWithField.Fields = append(entryWithField.Fields,
entryWithField := entry.With(
logger.Field{
Key: name,
Value: field.value,
Expand All @@ -176,14 +175,13 @@ func Run(t *testing.T, subject Subject) { // nolint
stringFieldValue = "string"
intFieldValue = 2
)
entryWithFields := entry
entryWithFields.Fields = append(entryWithFields.Fields,
entryWithFields := entry.With(
logger.Field{
Key: "StringField",
Value: stringFieldValue,
},
)
entryWithFields.Fields = append(entryWithFields.Fields,
entryWithFields = entryWithFields.With(
logger.Field{
Key: "IntField",
Value: intFieldValue,
Expand All @@ -204,8 +202,7 @@ func Run(t *testing.T, subject Subject) { // nolint
stringFieldValue = "string"
err = "err"
)
entryWithFieldAndError := entry
entryWithFieldAndError.Fields = append(entryWithFieldAndError.Fields,
entryWithFieldAndError := entry.With(
logger.Field{
Key: "StringField",
Value: stringFieldValue,
Expand Down
4 changes: 2 additions & 2 deletions logger/_examples/caller/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ func (a ReportCallerAdapter) Log(ctx context.Context, entry logger.Entry) {
entry.SkippedCallerFrames++ // each middleware adapter must additionally skip one frame (at least)

if _, file, line, ok := runtime.Caller(entry.SkippedCallerFrames); ok {
entry.Fields = append(entry.Fields, logger.Field{Key: "file", Value: file})
entry.Fields = append(entry.Fields, logger.Field{Key: "line", Value: line})
entry = entry.With(logger.Field{Key: "file", Value: file})
entry = entry.With(logger.Field{Key: "line", Value: line})
}

a.NextAdapter.Log(ctx, entry)
Expand Down
7 changes: 4 additions & 3 deletions logger/_examples/tags/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,13 @@ type AddFieldFromContextAdapter struct {
}

func (a AddFieldFromContextAdapter) Log(ctx context.Context, entry logger.Entry) {
entry.Fields = append(entry.Fields,
// entry.With creates an entry and adds a new field to it
newEntry := entry.With(
logger.Field{
Key: tag,
Value: ctx.Value(tag),
},
)
entry.SkippedCallerFrames++ // each middleware adapter must additionally skip one frame
a.NextAdapter.Log(ctx, entry)
newEntry.SkippedCallerFrames++ // each middleware adapter must additionally skip one frame
a.NextAdapter.Log(ctx, newEntry)
}
16 changes: 14 additions & 2 deletions logger/adapter.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@ type Entry struct {

// Fields contains all accumulated fields, in the order they were appended.
//
// Please do not update fields in the slice as other go-routines may still read them. To add a new field
// please use append built-in function. To remove a field, please create a new slice and copy remaining fields.
// Please do not modify the slice directly as other go-routines may still read it. To append a new field
// please use With method. It will create a new entry with copy of all fields, plus the new one. To remove a field,
// please create a new slice and copy remaining fields.
// To update fields you can rewrite entire slice.
//
// Fields can be nil.
Expand All @@ -32,6 +33,17 @@ type Entry struct {
SkippedCallerFrames int
}

// With creates a new entry with additional field.
func (e Entry) With(field Field) Entry {
newLen := len(e.Fields) + 1
fields := make([]Field, newLen)
copy(fields, e.Fields)
e.Fields = fields
e.Fields[newLen-1] = field

return e
}

// Level is a severity level of message. Use Level.MoreSevereThan to compare two levels.
type Level int8

Expand Down
3 changes: 1 addition & 2 deletions logger/global.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,7 @@ func (g *Global) Error(ctx context.Context, msg string) {

// With creates a new child logger with field.
func (g *Global) With(key string, value interface{}) *Global {
newEntry := g.entry
newEntry.Fields = append(newEntry.Fields, Field{Key: key, Value: value})
newEntry := g.entry.With(Field{Key: key, Value: value})

return &Global{
entry: newEntry,
Expand Down
2 changes: 1 addition & 1 deletion logger/logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func (l Logger) Error(ctx context.Context, msg string) {

// With creates a new logger with field.
func (l Logger) With(key string, value interface{}) Logger {
l.entry.Fields = append(l.entry.Fields, Field{Key: key, Value: value})
l.entry = l.entry.With(Field{key, value})

return l
}
Expand Down
23 changes: 23 additions & 0 deletions logger/logger_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,29 @@ func TestWithError(t *testing.T) {
}
}

func TestEntry_With(t *testing.T) {
field1 := logger.Field{Key: "k1", Value: "v1"}
field2 := logger.Field{Key: "k2", Value: "v2"}

t.Run("should add field to empty entry", func(t *testing.T) {
entry := logger.Entry{}
newEntry := entry.With(field1)
assert.Empty(t, entry.Fields)
require.Len(t, newEntry.Fields, 1)
assert.Equal(t, newEntry.Fields[0], field1)
})

t.Run("should add field to entry with one field", func(t *testing.T) {
entry := logger.Entry{}.With(field1)
newEntry := entry.With(field2)
require.Len(t, entry.Fields, 1)
require.Len(t, newEntry.Fields, 2)
assert.Equal(t, entry.Fields[0], field1)
assert.Equal(t, newEntry.Fields[0], field1)
assert.Equal(t, newEntry.Fields[1], field2)
})
}

func TestLevel_MoreSevereThan(t *testing.T) {
t.Run("should return true", func(t *testing.T) {
assert.True(t, logger.InfoLevel.MoreSevereThan(logger.DebugLevel))
Expand Down

0 comments on commit b575382

Please sign in to comment.