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

log/slog: documentation passes nil contexts, which is discouraged #61219

Closed
dominikh opened this issue Jul 6, 2023 · 1 comment
Closed

log/slog: documentation passes nil contexts, which is discouraged #61219

dominikh opened this issue Jul 6, 2023 · 1 comment
Assignees
@jba
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.21

Comments

@dominikh
Copy link
Member

dominikh commented Jul 6, 2023

The documentation of the slog package passes nil contexts, for example:

logger.LogAttrs(nil, slog.LevelInfo, "hello", slog.Int("count", 3))

However, the context package discourages this:

Do not pass a nil Context, even if a function permits it. Pass context.TODO if you are unsure about which Context to use.

slog internally turns nils into context.Background when calling methods on slog.Handler, which is probably good for resilience, but probably isn't something users should rely on.

In particular, such calls will currently trigger Staticcheck's SA1012 and I'm not sure slog warrants an exception.

I see from golang/exp@f0f767c that this is intentional, so this issue might be contentious. However, I'm not sure the "make them easier to write" argument is as important now, considering there are helpers for pre-defined log levels that don't take context arguments, and it would be unfortunate for the standard library to ignore its own rules.

/cc @jba
@jba jba self-assigned this Jul 7, 2023
@gopherbot
Copy link

Change https://go.dev/cl/508437 mentions this issue: log/slog: replace nil contexts with context.Background()

@jba jba mentioned this issue Jul 7, 2023
Closed
@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 10, 2023
@bcmills bcmills added this to the Go1.21 milestone Jul 10, 2023
bradfitz pushed a commit to tailscale/go that referenced this issue Jul 15, 2023
@jba @bradfitz
log/slog: replace nil contexts with context.Background()
4d3101f 
Passing nil for a context is discouraged. We should avoid it.

Fixes golang#61219.

Change-Id: I664387070aacb56af580b6b0791ca40982d2a711
Reviewed-on: https://go-review.googlesource.com/c/go/+/508437
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
eric pushed a commit to fancybits/go that referenced this issue Sep 7, 2023
@jba @eric
log/slog: replace nil contexts with context.Background()
0bd80a7 
Passing nil for a context is discouraged. We should avoid it.

Fixes golang#61219.

Change-Id: I664387070aacb56af580b6b0791ca40982d2a711
Reviewed-on: https://go-review.googlesource.com/c/go/+/508437
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
eric pushed a commit to fancybits/go that referenced this issue Sep 7, 2023
@jba @eric
log/slog: replace nil contexts with context.Background()
2024480 
Passing nil for a context is discouraged. We should avoid it.

Fixes golang#61219.

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

@jba jba

Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
4 participants
@dominikh @bcmills @gopherbot @jba