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: add Value.MarshalJSON #63204

Open
jba opened this issue Sep 25, 2023 · 3 comments
Open

proposal: log/slog: add Value.MarshalJSON #63204

jba opened this issue Sep 25, 2023 · 3 comments
Labels
Milestone

Comments

@jba
Copy link
Contributor

jba commented Sep 25, 2023

The encoding/json package marshals a slog.Value as {}. That means if a slog.Value appears inside something that is logged, slog.JSONHandler (and any other handler that outputs JSON and doesn't treat slog.Value specially) will lose its information. (See #62699.)

To fix that, I propose adding

package slog

func (v Value) MarshalJSON() ([]byte, error) {
    return json.Marshal(v.Any())
}
@jba jba added the Proposal label Sep 25, 2023
@gopherbot gopherbot added this to the Proposal milestone Sep 25, 2023
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Sep 26, 2023
@rliebz
Copy link

rliebz commented Nov 21, 2023

While this would handle a slog.Value in a slice correctly, it unfortunately does not handle a slog.LogValuer in a slice.

Take this demonstration based on the example for using slog.LogValuer to hide secrets in the slog docs:

package main

import (
	"log/slog"
	"os"
)

// A token is a secret value that grants permissions.
type Token string

// LogValue implements slog.LogValuer.
// It avoids revealing the token.
func (Token) LogValue() slog.Value {
	return slog.StringValue("REDACTED_TOKEN")
}

// This example demonstrates a Value that replaces itself
// with an alternative representation to avoid revealing secrets.
func main() {
	t := Token("shhhh!")
	logger := slog.New(slog.NewTextHandler(os.Stdout, nil))
	logger.Info("permission granted", "user", "Perry", "token", t)

	tokens := []Token{"keep it secret", "keep it safe"}
	logger.Info("permission revoked", "tokens", tokens)
}

Playground link: https://go.dev/play/p/FBcVYD0patm

I would like to see the secrets staying secret, but the design today does not respect slog.LogValuer in those contexts, leading to unfortunate consequences (a less disastrous version of which I have run into in real application code).

Because of that behavior, I've found that for custom types that need to reliably serialize, implementing slog.LogValuer is not actually sufficient—you also need to implement MarshalJSON. Which in some cases is unfortunate, and in others where there is already a canonical JSON representation that is for whatever reason undesirable for logging, a separate type or manual serialization is required.

I'm not opposed to this proposal, but I'm wondering if there isn't a solution that solves the problem more generally. The ideal behavior IMO would be that for slog-serialized JSON, LogValue() takes precedence over MarshalJSON() at any level of nesting in slices, structs, etc. I'm not sure if there's a simple way to make something like that work.

@nesty92
Copy link

nesty92 commented Oct 22, 2024

@jba , I believe the issue may lie in how the LogValuer interface is not automatically invoked for elements within slices or arrays. I have put together an example to demonstrate this behavior and a workaround that involves creating a custom slice type to ensure consistent logging output: https://go.dev/play/p/zK1-50eJPkC

The output looks like this:

{"time":"2009-11-10T23:00:00Z","level":"DEBUG","msg":"","valid":"test","null":"null"}
{"time":"2009-11-10T23:00:00Z","level":"DEBUG","msg":"","slice":[{"V":"string","Valid":true},{"V":"","Valid":false}]}
{"time":"2009-11-10T23:00:00Z","level":"DEBUG","msg":"","nullSlice":{"0":"string","1":"null"}}
time=2009-11-10T23:00:00.000Z level=DEBUG msg="" valid=test null=null
time=2009-11-10T23:00:00.000Z level=DEBUG msg="" slice="[{V:string Valid:true} {V: Valid:false}]"
time=2009-11-10T23:00:00.000Z level=DEBUG msg="" nullSlice.0=string nullSlice.1=null

Issue

When logging a single Null[T] value, the LogValue() method is called correctly, and "null" is properly rendered for invalid values. However, when dealing with slices, LogValue() is not invoked for each element, resulting in an unexpected direct representation of the struct.

Workaround

To work around this limitation, I created a custom slice type NullSlice[T] and implemented the LogValue() method for it. This ensures that each element in the slice is properly processed according to its LogValue() method.

It's important to note that this workaround is not limited to Null[T] slices; it can be applied to any custom type slice that implements the LogValuer interface. By creating a new type for the slice and implementing LogValue(), you can ensure that each element is correctly processed, regardless of its type.

It would be great if slog could natively handle slices or arrays by automatically applying the LogValuer interface to each element, similar to how single elements are handled. This enhancement would make logging more intuitive and consistent across different data structures.

@jba
Copy link
Contributor Author

jba commented Oct 22, 2024

It would be great if slog could natively handle slices or arrays by automatically applying the LogValuer interface to each element

I understand the attraction of this idea, but it's infeasible for slog to walk every data structure it is handed looking for LogValuer. That would be too expensive. I think nested LogValuers have to be handled with user-defined types, as @nesty92 has done.

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

No branches or pull requests

4 participants