zap.String with ~string generic

[bcspragu]

Is your feature request related to a problem? Please describe.

Frequently, we'll want to log something defined as:

// Imagine this for basically any primary key in an ecosystem, e.g. AccountID, ContactID, etc
type UserID string

And to log it presently, we'll have to do something like:

logger.Debug("user hit admin endpoint",
  zap.String("path", r.URL.Path),
  zap.String("user_id", string(u.ID)),
  zap.String("account_id", string(acct.ID)))

Which includes a lot of string(...) conversions, that are mostly unnecessary now that generics exist.

Describe the solution you'd like

The solution I'm proposing is to change the type of zap.String from:

func String(k string, v string) zap.Field

to

func String[T ~string](k string, v T) zap.Field

This is purely a convenience thing, but it removes a lot of casting that clutters up our logging code. AFAICT, this should be backwards compatible, and generics are already in use in the latest release. The same strategy could be applied to all the other primitive-type zap.Field helpers to simplify end-user code a bit.

Describe alternatives you've considered

The zap.Stringer helper is closely related, but doesn't help the type X string use case, which is a pretty common idiom in Go.

Is this a breaking change?

Nope! Should totally be backwards compatible. Existing string(x) casts will work fine, but future ones can be reduced to just x.

Additional context

This change would take about 37 seconds to implement, happy to contribute that if folks are interested. Also, apologies if this has come up before, I tried searching issues for it, but came up empty handed. Searching for "~string" didn't give me anything relevant AFAICT.

[abhinav]

Thanks for the issue, @bcspragu.
I agree that this would be useful to have, although I don't think we can change the signature of the existing String function.
While it won't break any call sites, it will break function references. e.g.

s := zap.String
log.Info("foo", s("k", "v"))

The above will fail to compile with the suggested change.

However, I still think this functionality is valuable.
We can maybe add it as a new function?
As an opening suggestion: Str for stringy things?

func Str[T ~string](k string, v T) Field {
  return String(k, string(v))
}

If desirable, we can even expand that type constraint in the future to ~string | fmt.Stringer and dispatch to zap.String or zap.Stringer dynamically. But we don't have to do that just yet. Expanding the type constraints of a function that expects a type constraint is backwards-compatible.

WDYT @prashantv @mway @sywhang?

[prashantv]

+1 on adding this functionality as a new function. I think this would be even more helpful for the slice functions like zap.Strings, since you can't cast the slice even if the underlying type is the same.

[mway]

Agreed. Str (and Strs) sound good to me. I'd consider baking it in exp/ for a bit, maybe exp/fields as a general "field sandbox".

One minor adjustment: let's parameterize both arguments. So:

package fields // go.uber.org/zap/exp/fields

func Str[T ~string](key T, value T) zap.Field {
  return zap.String(string(key), string(value))
}

for Strs, I think this should pretty much "just work":

package fields // go.uber.org/zap/exp/fields

func Strs[T ~string](key T, values []T) zap.Field {
  return zap.Array(string(key), stringArray(values))
}

type stringArray[T ~string] []T

func (a stringArray[T]) MarshalLogArray(enc zapcore.ArrayEncoder) error {
  for i := range a {
    enc.AppendString(string(a[i]))
  }
  return nil
}

In zap proper, I think we can just change stringArray to its parameterized version without any side effects and it will support both Strings and Strs (whenever the latter is "promoted").

[bcspragu]

One minor adjustment: let's parameterize both arguments. So:

One minor adjustment to the minor adjustment. If you go this route for keys, you'll want to use two different generic args, otherwise they'll need to have the same underlying type. For example:

type (
	A string
	B string
)

func main() {
	a := A("key")
	b := B("value")
	test(a, b)
}

func test[T ~string](k T, v T) {
	fmt.Println(k, v)
}

will give you an error like type B of b does not match inferred type A for T, whereas the following would work:

func test[T ~string, U ~string](k T, v U) {
	fmt.Println(k, v)
}

For the use cases I have, I've never run into something where I'd need/want a non-string key, but I could imagine doing something like:

package logkey

type Key string

const (
  UserID LogKey = "user_id"
  // ... more keys ...
)

to have consistent keys throughout a codebase. All the other details here, like using Str/Strs, sound great to me, thanks all for jumping on this!

[abhinav]

One minor adjustment to the minor adjustment. If you go this route for keys, you'll want to use two different generic args, otherwise they'll need to have the same underlying type.

Agreed. If we parameterize the key as well, it should be its own type parameter.
I wasn't sold on the necessity of parameterizing the key as well, but @bcspragu, your last example with type Key string convinced me otherwise. I haven't seen that either, but I can imagine someone doing that for well-known keys.

So just to confirm, are we going for:

func Str[K ~string, V ~string](k K, v V) Field

func Strs[K ~string, V ~[]S, S ~string](k K, v V) Field

[mway]

Thanks for catching. That'll teach me to write comments before caffeine.

[mway]

I wasn't sold on the necessity of parameterizing the key as well

Yeah, IMO there's also no reason to not parameterize it for flexibility.

So just to confirm, are we going for:

func Str[K ~string, V ~string](k K, v V) Field

func Strs[K ~string, V ~[]S, S ~string](k K, v V) Field

I had originally collapsed V,S into V but we'd run into the same problems as we do with strings now (e.g. type MyStrings []MyString). So yeah, what you wrote looks good.

[bcspragu]

This all sounds good to me! As for next steps, I'm happy to pull together a PR, seems like the main open question is to put it in zap directly, or somewhere in exp/

[sywhang]

Given the low-risk nature of the change, I think putting this in zap directly is fine.

[mway]

We just need to be willing to commit to the API if we elect for a non-exp/ package. I don't have concerns other than making sure we feel that the name works among all of the other Field- producing funcs while we're still on v1.

[abhinav]

Thanks, @bcspragu for volunteering.

I discussed this with @sywhang and @mway and we agree that it makes sense to prove it out in exp before upgrading to top-level. Would you mind placing it in zap/exp/zapfield (usage would look like zapfield.Str(k, v)). We can promote it to top-level once folks have had opportunity to play with it and any issues or extensions become evident.

[bcspragu]

Sounds like a plan to me, I've kicked this off in #1281

[sywhang]

Since this is now available in the zap/exp/zapfield package, will close this issue. We can evaluate promoting it to a top-level field in some time.