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: make it easier to log types #63570

Closed
josharian opened this issue Oct 16, 2023 · 9 comments
Closed

proposal: log/slog: make it easier to log types #63570

josharian opened this issue Oct 16, 2023 · 9 comments
Labels
Proposal
Milestone
Proposal

Comments

@josharian
Copy link
Contributor

Translating logging including the %T format verb to slog is verbose. The clearest, most concise way also adds fmt overhead.

slog.Warn("unexpected type", "x", fmt.Sprintf("%T", x))

I propose that we add API to make it nicer and cheaper. One possibility:

slog.Warn("unexpected type", "x", slog.TypeValue(x))

This is (in my opinion) lot clearer and simpler, and definitely cheaper.

In package slog:

// TypeValue returns a Value representing x's type.
// It is similar to fmt's %T verb.
func Type(x any) slog.Value {
	rt := reflect.TypeOf(x)
	return slog.StringValue(rt.String())
}

There could (should?) be a matching Type func that returns an Attr.

I am fairly new to log/slog, so there may be some other API design that better fits the package. I don't feel strongly about any of the details.

This is, strictly speaking, all covered by the core string support, since we are just translating types to their names, which are strings. But getting the name of a type for logging is such a common need that it has a dedicated fmt verb. I would argue that the core logging package should make it similarly clear, simple, and cheap.

cc @jba
@gopherbot gopherbot added this to the Proposal milestone Oct 16, 2023
@seebs
Copy link
Contributor

seebs commented Oct 16, 2023

Argument against: It's "just a string"
Argument for: A lot of things are "just a string" or "just a number" but we like having the intended semantics encoded. A time.Duration is really just an int64, for instance.

@ianlancetaylor
Copy link
Contributor

CC @jba

@jba
Copy link
Contributor

jba commented Oct 16, 2023

I don't think logging a type is common. Zap has 77 constructors for a Field (equivalent of an Attr), none of which get the type.

@josharian
Copy link
Contributor Author

josharian commented Oct 16, 2023 •
edited

Using my (slightly old) downloaded copy of https://github.com/mvdan/corpus, some very quick and dirty stats:

$ rg "log.*%T" | wc -l
2182
$ rg "log.*%v" | wc -l
114572

That's 2% as many %Ts as %vs.

A quick random sample, by piping rg into perl and dropping almost all lines:

github.com/crowdsecurity/crowdsec@v1.5.2/pkg/acquisition/acquisition.go:		log.Debugf("starting one source %d/%d ->> %T", i, len(sources), subsrc)
github.com/benthosdev/benthos/v4@v4.18.0/internal/impl/sql/processor_sql_raw.go:				s.logger.Debugf("Mapping returned non-array result: %T", iargs)
github.com/pulumi/pulumi-terraform-bridge/v3@v3.53.0/pkg/tf2pulumi/il/graph_comments.go:				b.logf("list mismatch for key '%v': %v, %T", key, len(items), element)
github.com/hanchuanchuan/bingo2sql@v1.0.2/parserV2.go:		log.Printf("%T", v)
github.com/aws/eks-anywhere@v0.16.1/pkg/controller/handlers/capiobjects.go:			log.V(6).Info("Object not managed by an eks-a Cluster, ignoring", "type", fmt.Sprintf("%T", o), "name", o.GetName())
github.com/btcsuite/btcwallet@v0.16.9/chain/bitcoind_client.go:				log.Warnf("Received unexpected filter type %T",
github.com/openservicemesh/osm@v1.2.4/pkg/k8s/announcement_handlers.go:				log.Error().Msgf("Error casting to *corev1.Pod: got type %T", addedPodObj)
github.com/!google!cloud!platform/elcarro-oracle-operator@v0.5.0-alpha/common/pkg/monitoring/monitoring.go:	log.Info("Failed to parse uint", "type", fmt.Sprintf("%T", i), "input", i)
github.com/fairwindsops/astro@v1.6.0/pkg/handler/bound.go:		log.Warnf("Object has unknown type of %T", object)

Of note, there are multiple instances of fmt.Sprintf("%T", x) in there, apparently to work with a structured logging API.

@dsnet
Copy link
Member

dsnet commented Oct 17, 2023

If we add this, would we ever regret the possible ambiguity of reflect.Type.String which only records only the package name, and not the full package path? There have been times where I'm left scratching my heard trying to figure exactly what foo.Bar refers to.

@josharian
Copy link
Contributor Author

Maybe? But we collectively have lots of code miles on %T.

@ericlagergren
Copy link
Contributor

I don't think logging a type is common. Zap has 77 constructors for a Field (equivalent of an Attr), none of which get the type.

Which is an oversight, IMO. I usually end up writing a constructor for reflect.Type for most projects.

@ianthehat
Copy link

I don't currently understand why

slog.Warn("unexpected type", "x", slog.TypeValue(x))

would be better than the already possible

slog.Warn("unexpected type", "x", reflect.TypeOf(x))

@josharian
Copy link
Contributor Author

@ianthehat oh, that's nice. that seems like an excellent approach. funny, i asked a bunch of experienced gophers for how to do this and you're the first to suggest it. :)

Proposal withdrawn. (Maybe there's something left to do here around docs or linters...but I'm not sure what/where.)

@josharian josharian mentioned this issue Oct 17, 2023
Open
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
None yet
Milestone
Proposal
Development

No branches or pull requests
8 participants
@josharian @seebs @ianthehat @ianlancetaylor @ericlagergren @dsnet @gopherbot @jba