Skip to content

proposal: log/slog: GroupAttrs(key, attrs....Attr) Attr #66365

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

Open
seankhliao opened this issue Mar 17, 2024 · 10 comments
Open

proposal: log/slog: GroupAttrs(key, attrs....Attr) Attr #66365

seankhliao opened this issue Mar 17, 2024 · 10 comments
Labels
Milestone

Comments

@seankhliao
Copy link
Member

seankhliao commented Mar 17, 2024

Proposal Details

func GroupAttrs(key, attrs....Attr) Attr

I propose the addition of the above function to allow for the type safe construction of Group attributes.

The existing signature works if you know the list of attributes to use upfront:

// Works
g := slog.Group("key",
        slog.String("key-1", "a"),
        slog.Int("key-2", 2),
)

but less well if you need to construct the group members beforehand.

// maybe constructed
attrs := []slog.Attr{
        slog.String("key-1", "a"),
}
if x {
        attrs = append(attrs, slog.Int("key-2", 2))
}

// Doesn't work, []slog.Attr doesn't match []any
g := slog.Group("key", attrs...)

Existing code generally appears to use one of the 3 options from below,
in ascending order of popularity.

// Option 1: Unintuitive group construction
g := slog.Any("key", slog.GroupValue(attrs...))

// Option 2: convert to []any with a helper func
g := slog.Group("key", attr2any(attrs)...)

// Option 3: just use []any
var attrs []any
...
g := slog.Group("key", attrs...)

This appears to be the only gap in slog's exposed api where any is unavoidable,
and can't be escaped by using an alternate frontend (replace slog.Logger with something else while keeping slog.Handler) because Attrs are how key-value pairs are communicated to handlers.

cc @jba

@jba
Copy link
Contributor

jba commented Mar 17, 2025

What's wrong with g := slog.Any("key", slog.GroupValue(attrs...))?

Yes, there's an any, but does this come up that much in performance-critical code?

What if we added doc slog.Group pointing to this approach?

@seankhliao
Copy link
Member Author

I think it's just unintuitive and difficult to type / not helped by any sort of autocomplete?
While you want a group, you have to start by typing Any, and the any arg means autocomplete won't suggest the appropriate values.

There are almost as many instances of attrs2any (19): https://github.com/search?q=language%3Ago+%2Ffunc+.*%5C%5B%5C%5Dslog%5C.Attr%5C%29+%5C%5B%5C%5Dany%2F&type=code
as there are Any(.., GroupValue (27): https://github.com/search?q=language%3Ago+%2Fslog.Any.*slog.GroupValue%2F&type=code

@jba
Copy link
Contributor

jba commented Mar 17, 2025

Thanks for the data. Of the 46 files in your search, 21 could benefit from GroupAttrs. It's clear that people are working around the lack of this function.

I'm in favor.

/cc @aclements

@aclements
Copy link
Member

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

@aclements aclements moved this from Incoming to Active in Proposals Apr 9, 2025
@willfaught
Copy link
Contributor

Why wouldn't this work?

attrs := []any{
        slog.String("key-1", "a"),
}
if x {
        attrs = append(attrs, slog.Int("key-2", 2))
}
g := slog.Group("key", attrs...)

@jba
Copy link
Contributor

jba commented Apr 15, 2025

@willfaught That does work. The point is that you have to use any; there should be a way to do it with slog.Attr.

@aclements
Copy link
Member

This seems pretty reasonable, but I'd like to see the proposed doc comment for GroupAttrs.

@seankhliao seankhliao changed the title proposal: log/slog: GroupAttrs(key, attrs....[]Attr) Attr proposal: log/slog: GroupAttrs(key, attrs....Attr) Attr Apr 16, 2025
@seankhliao
Copy link
Member Author

I think we can copy the LogAttrs docs and say:

// GroupAttrs is a more efficient version of [Group] that accepts only [Attr]s
func GroupAttrs(key, attrs ...Attr) Attr

@kaey
Copy link

kaey commented Apr 17, 2025

there should be a way to do it with slog.Attr.

can always use slog.Attr{Key: ..., Value: ...}.
for example:

resp := []slog.Attr{
	slog.Int("status", nw.Status()),
	slog.Int("bytes", nw.BytesWritten()),
	slog.String("duration", time.Since(start).String()),
	slog.Int64("duration_us", time.Since(start).Microseconds()),
	{Key: "data", Value: slog.GroupValue(logRecord.Response...)},
}

slog.LogAttrs(ctx, slog.LevelInfo, "http request done", slog.Attr{Key: "http_response", Value: slog.GroupValue(resp...)})

Not arguing against the proposal.

@aclements
Copy link
Member

@kaey has a good point. Just to make it more crisp, option 4 is:

// Option 4
g := slog.Attr{Key: key, Value: slog.GroupValue(attrs...)}

This is type-safe, avoids any, and is allocation-free. (It's still O(n), but only because GroupValue just a quick check for empty groups. Any option using Group is O(n) with a much larger constant factor.)

But maybe this is too obscure (the fact that it took a month for someone to point it out suggests that it is 😀 ) and is worth wrapping in a helper function anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Active
Development

No branches or pull requests

6 participants