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

proposal: log/slog: rename Logger.WithGroup #59151

Closed
pohly opened this issue Mar 20, 2023 · 4 comments
Closed

proposal: log/slog: rename Logger.WithGroup #59151

pohly opened this issue Mar 20, 2023 · 4 comments

Comments

@pohly
Copy link

pohly commented Mar 20, 2023

There are are currently two different usages of "group" in the API which are completely unrelated:

I remember that "namespace" was also a candidate. I personally would prefer Logger.WithNamespace to avoid the ambiguity. But if you feel that there's no need to revisit this topic, then I'm also fine with simply closing this issue without changing anything.

cc @jba

@xorkevin
Copy link

It is my understanding that these groups are actually related, and as such I believe the naming is appropriate. WithGroup will cause log attrs to go into the group. GroupValue will cause the provided attrs in the arguments to go into the group, but unlike WithGroup it won't affect other attrs logged by the Logger.

@AndrewHarrisSPU
Copy link

AndrewHarrisSPU commented Mar 20, 2023

There are are currently two different usages of "group" in the API which are completely unrelated:

I think you're right that in practice the uses of "group" are fairly distinct / distinctly motivated.

IMHO ... they are significantly related in terms of the data model. It's almost just that the following log lines should encode data equivalently into "g":{"a":1}, even while the first commits data more persistently to Handlers and the second observes data more transiently in a single record:

	log.WithGroup("g").With("a", 1).Info("")

	log.Info("", slog.Group("g", slog.Int("a", 1)))

Where it's about the structure of the data model more exclusively, and not about encoding or formatting, is for functionality like ReplaceAttr.

	log := slog.New(slog.HandlerOptions{
		ReplaceAttr: func(groups []string, a slog.Attr) slog.Attr {
			if len(groups) == 1 && groups[0] == "g" {
				a.Value = slog.IntValue(2)
			}
			return a
		},
	}.NewJSONHandler(os.Stdout))

It's maybe a little more straightforward that we expect "g":{"a":2} from both of the logging calls from this perspective - no opinion on the method name but I do think there is a real relationship here.

@pohly
Copy link
Author

pohly commented Mar 21, 2023

According to the documentation, WithGroup causes "attributes [to be] qualified by the group name". My expectation based on that was to get "g.a" as key in JSON, not an object. But I hadn't actually run an example (nor seen one, I believe). My expectation turned out to be wrong: both calls do the exact same thing.

That makes this proposal incorrect.

@pohly pohly closed this as completed Mar 21, 2023
@jba
Copy link
Contributor

jba commented Mar 21, 2023

@pohly, thanks for closing. I used the phrase "qualified by the group name" in the docs deliberately, to avoid committing to what that might actually look like. I tried to clarify that in https://go-review.googlesource.com/c/go/+/478056.

@golang golang locked and limited conversation to collaborators Mar 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants