Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

log/slog: change slog.Group signature to ...any #59204

Closed
ianthehat opened this issue Mar 23, 2023 · 19 comments
Closed

log/slog: change slog.Group signature to ...any #59204

ianthehat opened this issue Mar 23, 2023 · 19 comments

Comments

@ianthehat
Copy link

Currently there is now way to access the standard conversion of ...any to ...Attr without building a record.
It would be nice to be able to build a group Attr this way, for patterns where you want to be able to pass around a set of attrs (like queries that return a standard set for logging, or building a set of labels before an if that adds more).
All existing calls to slog.Group would keep working as the ...any forms happily accept Attr directly anyway, they would just be a fraction slower, in cases where the performance is needed you could use GroupValue directly.
This also exposes the standard conversion to helpers that are not performance critical so they can have a signature that looks like the normal logging functions.
Performance critical helpers need to build a Record directly to avoid allocations, and could not use this approach, but exposing an API for that is a much larger surface area.

@gopherbot gopherbot added this to the Proposal milestone Mar 23, 2023
@AndrewHarrisSPU
Copy link

AndrewHarrisSPU commented Mar 23, 2023

I don't have a strong opinion on this but I agree there is some value in having the standard ("suggested", "conventional"?) conversion defined and easily available. Another situation where a common conversion might be useful is when implementing alternatives to Logger. Using something other than Logger to wrap a Handler seems entirely anticipated by slog, and many of these might want to accept variadic any - my impression from experiments along these lines is that the utility of different conversions is not very high, but the confusion resulting from different conversions is surprisingly high.

Rather than changing slog.Group, what about a func Attrs(...any) []slog.Attr, with logic invoking slog's argsToAttr?

For groups, something like this would result in more tokens than changing the signature to Group(string, ...any), but far fewer tokens than requiring per-attribute attention.

slog.Group("label", slog.Attrs("a", 1, "b", 2)...)

@seankhliao seankhliao changed the title proposal: log/slog: Change slog.Group signature to ...any proposal: log/slog: change slog.Group signature to ...any Mar 23, 2023
@ianlancetaylor
Copy link
Contributor

CC @jba

@jba
Copy link
Contributor

jba commented Mar 27, 2023

I'm generally in favor of this; I agree there should be some way to expose the logic that converts k-v pairs to Attrs. The Attrs function is a bit more general, but I think Group(string, ...any) slog.Attr is a slightly better choice for efficiency reasons, though I'd have to implement both and see. The other nice thing about Group is that we wouldn't be adding something.

Linking to the main proposal: #56345.

@ianthehat
Copy link
Author

It's worth noting that you can easily do Attrs in terms of Group anyway, slog.Group("", args...).Group(), so I would lean in the direction of no new API, especially as I think the group case will be the most common use of attr building outside of a record.
I think most new Logger "replacements" will be building records and using Record.Add rather than a custom Attr builder

@rsc
Copy link
Contributor

rsc commented Mar 28, 2023

It sounds like people are in favor of slog.Group taking ...any over adding slog.Attrs. Do I have that right?

@rsc
Copy link
Contributor

rsc commented Mar 29, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@ChrisHines
Copy link
Contributor

Suppose someone wants to write an alternative to slog.Logger that always requires ...slog.Attr signatures and does not allow ...any in its API. Would accepting this proposal create too many holes in the damn to plug?

@ianthehat
Copy link
Author

Not sure if this is what you are asking, but you can still build a group using Attr{"label", GroupValue(attrs...)} if you want to build one directly from a []Attr.

@ChrisHines
Copy link
Contributor

ChrisHines commented Mar 30, 2023

I don't think that's what I'm asking.

At the moment none of the Attr constructors accept ...any. So it is possible to write a package that provides an alternate Logger API restricted to ...Attr parameters but reusing the slog.Attr constructors and Handler unmodified. The only "hole" left is the Record.Add method, but client code doesn't normally use that so it's OK. If the slog.Group signature takes ...any it backdoors this hypothetical package's attempts to hide ...any signatures.

I understand the desire to provide a tool for people who want access to the the ...any -> []Attr logic, so I prefer adding the func Attrs(...any) []slog.Attr function as both a more orthogonal and opt-in approach.

@jba
Copy link
Contributor

jba commented Mar 30, 2023

Suppose someone wants to write an alternative to slog.Logger that always requires ...slog.Attr signatures and does not allow ...any in its API. Would accepting this proposal create too many holes in the damn to plug?

That is an interesting point. But wouldn't they just go the whole way?

package kvless_slog

type Logger ...
type Handler = slog.Handler
type Attr = slog.Attr
type Value = slog.Value
func Int(key string, val int) Attr { return slog.Int(key, val) }
...
func Group(key string, attrs ...Attr) { return slog.Group(attrs...) }

Yes, there are a lot of those shims, but it would probably take no more than an hour and since slog is in the standard library, your maintenance burden would be small—a few tweaks every six months. And then you'd be completely locked down: no chance of calling Record.Add, or slog.Logger.Info for that matter (since client packages would no longer import log/slog at all).

@ChrisHines
Copy link
Contributor

But wouldn't they just go the whole way?

Now that you've show us how, I suppose they would. That approach had not occurred to me yet, though, so it might not occur to others. ;). But it's good to know it's possible and enterprising individuals can find the path. Thanks.

@jba
Copy link
Contributor

jba commented Mar 31, 2023

Is there a use case for func Attrs(...any) []slog.Attr, other than using it to create a Group?

@ChrisHines
Copy link
Contributor

Is there a use case for func Attrs(...any) []slog.Attr, other than using it to create a Group?

I am not sure. If there is a use case for code to create variables of type []slog.Attr and we want to populate them conveniently with the same rules that slog uses to handle ...any parameters then that seems like a use case.

But when I review code that has variables like that with our internal logging package it usually seems like a code smell and should be replaced by a use of logger.With call. But there is debate about that, for example: #59126 (comment).

@AndrewHarrisSPU
Copy link

Is there a use case for func Attrs(...any) []slog.Attr, other than using it to create a Group?

I would characterize the utility as syntactic, in a language that on principle doesn't embrace syntax metaprogramming ... there's some tension in this and it seems like a sensitive question.

A dumb playground example of what I mean: it's possible to prepare any error with file:line annotation and arbitrary additional logging attributes while still passing it around as a valid error. There would be ways to write this without a func Attrs, but I feel like the utility is tangible here.

I think Group(string, ...any) slog.Attr is a slightly better choice for efficiency reasons

I did some quick benchmarking of a slog.Group(string, ...Attr), groupAny, and AttrsFunc, using test lists based on the ones from slog/benchmark, and didn't spot a difference between the latter two. Both had worse numbers than the current Group(string, ...Attr) version (slower, and sometimes allocating more).

type Attr = slog.Attr

func groupAny(name string, args ...any) Attr {
	as := make([]Attr, 0, len(args))
	var a Attr
	for len(args) > 0 {
		a, args = argsToAttr(args)
		as = append(as, a)
	}
	return Attr{name, slog.GroupValue(as...)}
}

func AttrsFunc(args ...any) []Attr {
	as := make([]Attr, 0, len(args))
	var a Attr
	for len(args) > 0 {
		a, args = argsToAttr(args)
		as = append(as, a)
	}
	return as
}

@jba
Copy link
Contributor

jba commented Apr 3, 2023

Here is another consideration, briefly mentioned in the top post:

This also exposes the standard conversion to helpers that are not performance critical so they can have a signature that looks like the normal logging functions.

To elaborate Ian's argument: we've decided that ...any is the main way to use the log output functions, with ...Attr relegated to a performance-sensitive corner. Given that decision, and the ability write groups inline, it seems natural to allow

logger.Info("msg",
    "a", 1,
    "b", 2,
    slog.Group("g",
          "c", 3,
          "d", 4,
    ),
    "e", 5)

and jarring to require that arguments to Group be Attrs.

Indeed, a developer who only needed to produce log output, and didn't care about how Handlers or LogValuers worked, wouldn't even have to learn about Attrs and Values except for Group. (There is also the fact that the package doc begins with Attrs and Values; I now think that material should be moved later.)

To me, this is a strong reason to prefer changing the signature of Group.

@rsc
Copy link
Contributor

rsc commented Apr 12, 2023

Have all remaining concerns about this proposal been addressed?

@rsc
Copy link
Contributor

rsc commented Apr 19, 2023

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@gopherbot
Copy link

Change https://go.dev/cl/487855 mentions this issue: log/slog: Group takes ...any

@rsc
Copy link
Contributor

rsc commented May 3, 2023

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: log/slog: change slog.Group signature to ...any log/slog: change slog.Group signature to ...any May 3, 2023
@rsc rsc modified the milestones: Proposal, Backlog May 3, 2023
@dmitshur dmitshur modified the milestones: Backlog, Go1.21 Jun 4, 2023
eric pushed a commit to fancybits/go that referenced this issue Sep 7, 2023
The Group function takes a key and a ...any, which is converted
into attrs.

Fixes golang#59204.

Change-Id: Ib714365dcda2eda37863ce433f3dd8cf5eeda610
Reviewed-on: https://go-review.googlesource.com/c/go/+/487855
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
eric pushed a commit to fancybits/go that referenced this issue Sep 7, 2023
The Group function takes a key and a ...any, which is converted
into attrs.

Fixes golang#59204.

Change-Id: Ib714365dcda2eda37863ce433f3dd8cf5eeda610
Reviewed-on: https://go-review.googlesource.com/c/go/+/487855
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests

8 participants