log/slog: inconsistent handling of empty groups #61067

seankhliao · 2023-06-29T15:20:03Z

What version of Go are you using (`go version`)?

$ go version
go version go1.21rc2 linux/amd64

Does this issue reproduce with the latest release?

slog is in the next release

What operating system and processor architecture are you using (`go env`)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/user/.cache/go-build"
GOENV="/home/user/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS="-trimpath "-ldflags=-s -w""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/user/.data/go/pkg/mod"
GONOPROXY="github.com/snyk"
GONOSUMDB="github.com/snyk"
GOOS="linux"
GOPATH="/home/user/.data/go"
GOPRIVATE="github.com/snyk"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/golang"
GOSUMDB="off"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/golang/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20.5"
GCCGO="gccgo"
GOAMD64="v3"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="0"
GOMOD="/home/user/tmp/testrepo0123/go.mod"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build3289887255=/tmp/go-build -gno-record-gcc-switches"

What did you do?

package main

import (
	"log/slog"
	"os"
)

func main() {
	h := slog.NewJSONHandler(os.Stdout, nil)
	l := slog.New(h)

	l.WithGroup("x").WithGroup("y").Info("foo")
	l.Info("foo", slog.Group("x", slog.Group("y")))
	l.Info("foo", slog.Group("x"))
}

What did you expect to see?

The documentation for slog.Handler.Handle states:

// Handle methods that produce output should observe the following rules:
// ...
// - If a group has no Attrs (even if it has a non-empty key),
// ignore it.

I expected for there to be no empty groups anywhere.

What did you see instead?

It doesn't seem to apply recursively, or apply to groups set with WithGroup:

{"time":"2023-06-29T16:13:32.030631211+01:00","level":"INFO","msg":"foo","x":{"y":{}}}
{"time":"2023-06-29T16:13:32.030684844+01:00","level":"INFO","msg":"foo","x":{}}
{"time":"2023-06-29T16:13:32.030687407+01:00","level":"INFO","msg":"foo"}

cc @jba

The text was updated successfully, but these errors were encountered:

jba · 2023-07-06T12:58:09Z

Thanks, I never thought about this case.

I guess you could argue that a group containing another empty group isn't really empty, just like a directory containing an empty directory isn't empty. But I think your expectation is more reasonable. I'll prepare a fix.

gopherbot · 2023-07-07T12:55:02Z

Change https://go.dev/cl/508436 mentions this issue: log/slog: handle recursively empty groups

gopherbot · 2023-07-07T14:00:18Z

Change https://go.dev/cl/508438 mentions this issue: testing/slogtest: check for no group with empty record

As #61067 pointed out, slog did not properly handle empty groups. https://go.dev/cl/508436 dealt with most cases inside slog itself, but handlers must still do a check on their own. Namely, a handler must not output a group created by WithGroup unless the Record has attributes. This change adds a test to slogtest to check that case. Fixes #61227. Change-Id: Ibc065b6e5f6e199a41bce8332ea8c7f9d8373392 Reviewed-on: https://go-review.googlesource.com/c/go/+/508438 Reviewed-by: Alan Donovan <adonovan@google.com> Run-TryBot: Jonathan Amsterdam <jba@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>

Handlers should not display empty groups. A group with no attributes is certainly empty. But we also want to consider a group to be empty if all its attributes are empty groups. The built-in handlers did not handle this second case properly. This CL fixes that. There are two places in the implementation that we need to consider. For Values of KindGroup, we change the GroupValue constructor to omit Attrs that are empty groups. A Group is then empty if and only if it has no Attrs. This avoids a recursive check for emptiness. It does require allocation, but that doesn't worry us because Group values should be relatively rare. For groups established by WithGroup, we avoid opening such groups unless the Record contains non-empty groups. As we did for values, we avoid adding empty groups to records in the first place, so we only need to check that the record has at least one Attr. We are doing extra work, so we need to make sure we aren't slowing things down unduly. Benchmarks before and after this change show minimal differences. Fixes golang#61067. Change-Id: I684c7ca834bbf69210516faecae04ee548846fb7 Reviewed-on: https://go-review.googlesource.com/c/go/+/508436 Run-TryBot: Jonathan Amsterdam <jba@google.com> Reviewed-by: Alan Donovan <adonovan@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>

As golang#61067 pointed out, slog did not properly handle empty groups. https://go.dev/cl/508436 dealt with most cases inside slog itself, but handlers must still do a check on their own. Namely, a handler must not output a group created by WithGroup unless the Record has attributes. This change adds a test to slogtest to check that case. Fixes golang#61227. Change-Id: Ibc065b6e5f6e199a41bce8332ea8c7f9d8373392 Reviewed-on: https://go-review.googlesource.com/c/go/+/508438 Reviewed-by: Alan Donovan <adonovan@google.com> Run-TryBot: Jonathan Amsterdam <jba@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>

Handlers should not display empty groups. A group with no attributes is certainly empty. But we also want to consider a group to be empty if all its attributes are empty groups. The built-in handlers did not handle this second case properly. This CL fixes that. There are two places in the implementation that we need to consider. For Values of KindGroup, we change the GroupValue constructor to omit Attrs that are empty groups. A Group is then empty if and only if it has no Attrs. This avoids a recursive check for emptiness. It does require allocation, but that doesn't worry us because Group values should be relatively rare. For groups established by WithGroup, we avoid opening such groups unless the Record contains non-empty groups. As we did for values, we avoid adding empty groups to records in the first place, so we only need to check that the record has at least one Attr. We are doing extra work, so we need to make sure we aren't slowing things down unduly. Benchmarks before and after this change show minimal differences. Fixes golang#61067. Change-Id: I684c7ca834bbf69210516faecae04ee548846fb7 Reviewed-on: https://go-review.googlesource.com/c/go/+/508436 Run-TryBot: Jonathan Amsterdam <jba@google.com> Reviewed-by: Alan Donovan <adonovan@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>

As golang#61067 pointed out, slog did not properly handle empty groups. https://go.dev/cl/508436 dealt with most cases inside slog itself, but handlers must still do a check on their own. Namely, a handler must not output a group created by WithGroup unless the Record has attributes. This change adds a test to slogtest to check that case. Fixes golang#61227. Change-Id: Ibc065b6e5f6e199a41bce8332ea8c7f9d8373392 Reviewed-on: https://go-review.googlesource.com/c/go/+/508438 Reviewed-by: Alan Donovan <adonovan@google.com> Run-TryBot: Jonathan Amsterdam <jba@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>

Handlers should not display empty groups. A group with no attributes is certainly empty. But we also want to consider a group to be empty if all its attributes are empty groups. The built-in handlers did not handle this second case properly. This CL fixes that. There are two places in the implementation that we need to consider. For Values of KindGroup, we change the GroupValue constructor to omit Attrs that are empty groups. A Group is then empty if and only if it has no Attrs. This avoids a recursive check for emptiness. It does require allocation, but that doesn't worry us because Group values should be relatively rare. For groups established by WithGroup, we avoid opening such groups unless the Record contains non-empty groups. As we did for values, we avoid adding empty groups to records in the first place, so we only need to check that the record has at least one Attr. We are doing extra work, so we need to make sure we aren't slowing things down unduly. Benchmarks before and after this change show minimal differences. Fixes golang#61067. Change-Id: I684c7ca834bbf69210516faecae04ee548846fb7 Reviewed-on: https://go-review.googlesource.com/c/go/+/508436 Run-TryBot: Jonathan Amsterdam <jba@google.com> Reviewed-by: Alan Donovan <adonovan@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>

As golang#61067 pointed out, slog did not properly handle empty groups. https://go.dev/cl/508436 dealt with most cases inside slog itself, but handlers must still do a check on their own. Namely, a handler must not output a group created by WithGroup unless the Record has attributes. This change adds a test to slogtest to check that case. Fixes golang#61227. Change-Id: Ibc065b6e5f6e199a41bce8332ea8c7f9d8373392 Reviewed-on: https://go-review.googlesource.com/c/go/+/508438 Reviewed-by: Alan Donovan <adonovan@google.com> Run-TryBot: Jonathan Amsterdam <jba@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>

seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 29, 2023

jba self-assigned this Jul 6, 2023

jba removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 6, 2023

gopherbot closed this as completed in d3d78b4 Jul 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

log/slog: inconsistent handling of empty groups #61067

log/slog: inconsistent handling of empty groups #61067

seankhliao commented Jun 29, 2023

jba commented Jul 6, 2023

gopherbot commented Jul 7, 2023

gopherbot commented Jul 7, 2023

log/slog: inconsistent handling of empty groups #61067

log/slog: inconsistent handling of empty groups #61067

Comments

seankhliao commented Jun 29, 2023

What version of Go are you using (go version)?

Does this issue reproduce with the latest release?

What operating system and processor architecture are you using (go env)?

What did you do?

What did you expect to see?

What did you see instead?

jba commented Jul 6, 2023

gopherbot commented Jul 7, 2023

gopherbot commented Jul 7, 2023

What version of Go are you using (`go version`)?

What operating system and processor architecture are you using (`go env`)?