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.

Sign up for GitHub

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

Jump to bottom

proposal: log/slog: Add Logger.WithAttrs to fill gap in attribute based API #64350

Closed
larzconwell opened this issue Nov 23, 2023 · 4 comments
Closed

proposal: log/slog: Add Logger.WithAttrs to fill gap in attribute based API #64350

larzconwell opened this issue Nov 23, 2023 · 4 comments
Labels
Proposal
Milestone
Proposal

Comments

@larzconwell
Copy link
Contributor

larzconwell commented Nov 23, 2023 •
edited

Proposal Details

I think the log/slog API has a gap in the purely attribute based functions/methods, and adding WithAttrs to the package scope and to Logger would fill this gap. I would also find this useful, I would like to enforce/lint that our logging only uses attribute based logging functionality to avoid potential issues with missing values for keys when using the more general functionality, but it's difficult to do right now since WithAttrs is missing on Logger.

Our logging behavior typically looks something like this:

logger := slog.New(slog.NewJSONHandler(os.Stdout, nil))
logger.With(
    slog.Any("error", io.ErrUnexpectedEOF),
    slog.String("user", "larzconwell"),
).Info("Error querying user")

Ideally I'd like us to be able to do the following:

logger := slog.New(slog.NewJSONHandler(os.Stdout, nil))
logger.WithAttrs(
    slog.Any("error", io.ErrUnexpectedEOF),
    slog.String("user", "larzconwell"),
).Info("Error querying user")
@gopherbot gopherbot added this to the Proposal milestone Nov 23, 2023
@gopherbot
Copy link

Change https://go.dev/cl/544715 mentions this issue: log/slog: Add WithAttrs and Logger.WithAttrs

@bcmills
Copy link
Contributor

bcmills commented Nov 27, 2023

(attn @jba)

@jba
Copy link
Contributor

jba commented Nov 28, 2023

If the only concern is enforcement, it should be straightforward to write a vet check (static analyzer) that checks that all calls to Logger.With take Attrs.

For reference, see the vet check for slog key-value pairs. That is only 234 lines of code, and yours would be much simpler:

  1. walk the AST for calls using inspect.Preorder
  2. Look for a method on *slog.Logger named With. This code looks for a top-level function with a given name; you would have to adapt it slightly to look for a method. I would browse go/analysis/passes for code that does that already.
  3. Check that the type of each argument is slog.Attr.

You can make that into a standalone CLI tool with one line of code (example).

@apparentlymart
Copy link

I suspect that the example link in the previous comment was intended to be (example), rather than a link to a Google-internal instance of Gerrit.

@larzconwell larzconwell closed this as not planned Won't fix, can't repro, duplicate, stale Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Proposal
Projects
Proposals
Status: Incoming
Milestone
Proposal
Development

No branches or pull requests
5 participants
@apparentlymart @larzconwell @bcmills @gopherbot @jba