slog: Support inline group fields per spec #1263

mway · 2023-03-20T19:38:47Z

Adds support for inline groups per the slog spec: if a group's attr key is empty, its fields should be merged inline rather than nested within an object namespace.
Adds some coarse optimization by pooling temporary []zap.Field storage.

Before:

# go test ./zapslog/ -count 5 -run xxx -bench .
goos: linux
goarch: amd64
pkg: go.uber.org/zap/exp/zapslog
cpu: AMD EPYC 7B13
BenchmarkSlog-96          526584              2468 ns/op             243 B/op          3 allocs/op
BenchmarkSlog-96          446736              2751 ns/op             243 B/op          3 allocs/op
BenchmarkSlog-96          538926              2636 ns/op             243 B/op          3 allocs/op
BenchmarkSlog-96          409718              2610 ns/op             243 B/op          3 allocs/op
BenchmarkSlog-96          566422              2762 ns/op             243 B/op          3 allocs/op
PASS
ok      go.uber.org/zap/exp/zapslog     8.525s

After:

# go test ./zapslog/ -count 5 -run xxx -bench .
goos: linux
goarch: amd64
pkg: go.uber.org/zap/exp/zapslog
cpu: AMD EPYC 7B13
BenchmarkSlog-96          512924              2443 ns/op              24 B/op          1 allocs/op
BenchmarkSlog-96          397900              2796 ns/op              24 B/op          1 allocs/op
BenchmarkSlog-96          414337              2967 ns/op              24 B/op          1 allocs/op
BenchmarkSlog-96          517803              2335 ns/op              24 B/op          1 allocs/op
BenchmarkSlog-96          422788              2814 ns/op              24 B/op          1 allocs/op
PASS
ok      go.uber.org/zap/exp/zapslog     8.033s

abhinav · 2023-03-20T20:34:52Z

exp/zapslog/slog.go

+		converted.Return()
 	}
 	return nil
 }

-func convertAttrToField(attr slog.Attr) zapcore.Field {
+func convertAttrToFields(attr slog.Attr) *fieldSlice {
+	fields := _fieldSlices.Get()


I think this will be less prone to use-after-free bugs if "get from pool" and "defer return to pool" are next to each other.
Consider turning convertAttrToFields into addAttrAsFields(enc, attr), moving the AddTo inside the function, which will allow a defer fields.Return() right after the Get.

If that's unavoidable, we can Get from pool up above in MarshalLogObject outside the for loop,
pass into convertAttrToFields(attr, &converted), and then reset and re-use between each iteration:

converted := _fieldSlices.Get() defer converted.Return() for _, attr := range gs { converted.Reset() convertAttrToFields(attr, &converted) converted.AddTo(enc) }

abhinav · 2023-03-20T20:39:13Z

exp/zapslog/slog.go

+	for i := 0; i < s.Len(); i++ {
+		(*s)[i].AddTo(enc)
+	}


Any reason not to use range here?

Suggested change

for i := 0; i < s.Len(); i++ {

(*s)[i].AddTo(enc)

}

for _, f := range (*s) {

f.AddTo(enc)

}

abhinav · 2023-03-20T20:41:39Z

exp/zapslog/slog.go

+		converted := convertAttrToFields(attr)
+		fields.Extend(converted)
+		converted.Return()


I think if you take my suggestion to make convert take a *fieldSlice,
this won't need to borrow a new slice at all.

Suggested change

converted := convertAttrToFields(attr)

fields.Extend(converted)

converted.Return()

convertAttrToFields(attr, &fields)

abhinav · 2023-03-20T20:42:08Z

exp/zapslog/slog.go

+		converted := convertAttrToFields(attr)
+		fields.Extend(converted)
+		converted.Return()


Same

Suggested change

converted := convertAttrToFields(attr)

fields.Extend(converted)

converted.Return()

convertAttrToFields(attr, &fields)

abhinav · 2023-03-20T20:44:06Z

exp/zapslog/slog.go

+	tmp := [...]zapcore.Field{
+		zap.Namespace(group),
+	}
+
+	cloned := *h
+	cloned.core = h.core.With(tmp[:])


I'm 75% certain that the manual allocation won't have any effect.
It will behave the same as h.core.With(zap.Namespace(group)):
if the slice escapes, it'll be heap allocated anyway, and if it doesn't escape, it was going to be stack allocated anyway.

abhinav · 2023-03-20T20:45:21Z

exp/zapslog/slog.go

 }

 // withFields returns a cloned Handler with the given fields.
-func (h *Handler) withFields(fields ...zapcore.Field) *Handler {
+func (h *Handler) withFields(fields *fieldSlice) *Handler {
+	defer fields.Return()


This is super sus.
As a reader, it's super not clear why/how this is being freed here.

abhinav · 2023-03-20T20:45:56Z

exp/zapslog/slog.go

+	_fieldSlices.Put(s)
+}
+
+var _fieldSlices = pool.New(func() *fieldSlice {


Consider adding a clarifying comment that the pooled value MUST be a pointer type to avoid allocations.

mway · 2023-03-20T20:47:46Z

Thanks for the pass, just a rough draft atm (in case it wasn't obvious). Agree with your comments, will rework some of this soon.

abhinav · 2023-08-23T12:47:48Z

@mway if you take another stab at it, please update #1334.

…oup. (#1408) This change adds a test based on testing/slogtest that verifies compliance with the slog handler contract (a draft of this was available in #1335), and fixes all resulting issues. The two remaining issues were: - `Group("", attrs)` should inline the new fields instead of creating a group with an empty name. This was fixed with the use of `zap.Inline`. - Groups without any attributes should not be created. That is, `logger.WithGroup("foo").Info("bar")` should not create an empty "foo" namespace (`"foo": {}`). This was fixed by keeping track of unapplied groups and applying them the first time a field is serialized. Following this change, slogtest passes as expected. Refs #1333 Resolves #1334, #1401, #1402 Supersedes #1263, #1335 ### TESTS - passed. arukiidou#1 - This also works in Go 1.22 --------- Signed-off-by: junya koyama <[email protected]> Co-authored-by: Abhinav Gupta <[email protected]>

abhinav · 2024-02-05T18:00:28Z

Superseded by #1408

mway added 3 commits March 20, 2023 19:31

internal: Add and use a generic pool instead of using sync.Pool directly

e2b2455

Feedback

280a9bb

slog: Support inline group fields per spec

2eb9107

mway requested review from abhinav and sywhang March 20, 2023 19:39

abhinav reviewed Mar 20, 2023

View reviewed changes

Base automatically changed from mway/generic-pool to master March 21, 2023 14:43

abhinav mentioned this pull request Aug 23, 2023

zapslog.Handler: Test with slogtest, fix all failures #1334

Closed

4 tasks

This was referenced Jan 27, 2024

zapslog: Handler should inline the Attrs of a group with an empty key #1402

Closed

zapslog: fix all with slogtest, support inline group, ignore empty group. #1408

Merged

abhinav closed this Feb 5, 2024

abhinav deleted the mway/slog-attrs branch February 5, 2024 18:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

slog: Support inline group fields per spec #1263

slog: Support inline group fields per spec #1263

mway commented Mar 20, 2023

abhinav Mar 20, 2023

abhinav Mar 20, 2023

abhinav Mar 20, 2023

abhinav Mar 20, 2023

abhinav Mar 20, 2023

abhinav Mar 20, 2023

abhinav Mar 20, 2023

mway commented Mar 20, 2023 •

edited

Loading

abhinav commented Aug 23, 2023

abhinav commented Feb 5, 2024

slog: Support inline group fields per spec #1263

slog: Support inline group fields per spec #1263

Conversation

mway commented Mar 20, 2023

abhinav Mar 20, 2023

Choose a reason for hiding this comment

abhinav Mar 20, 2023

Choose a reason for hiding this comment

abhinav Mar 20, 2023

Choose a reason for hiding this comment

abhinav Mar 20, 2023

Choose a reason for hiding this comment

abhinav Mar 20, 2023

Choose a reason for hiding this comment

abhinav Mar 20, 2023

Choose a reason for hiding this comment

abhinav Mar 20, 2023

Choose a reason for hiding this comment

mway commented Mar 20, 2023 • edited Loading

abhinav commented Aug 23, 2023

abhinav commented Feb 5, 2024

mway commented Mar 20, 2023 •

edited

Loading