Pointer to T Field constructors.

[twilly]

Zap has support for constructing Field from primitive types but doesn't have constructors for pointers to the primitive types (*bool, *int, etc). These types are encountered often enough that constructing through Reflect is not ideal.

Let's discuss this feature.

[abhinav]

Would we want to report null for when these fields are nil or skip the field entirely?

[dnathe4th]

zap should report null in this scenario. The developer was explicit in their interest of the logging of that key, even if the pointer they passed goes to "nothing". To follow the analogy through to the json we tend to read these logs has, there's a meaningful semantic difference here between undefined, i.e. the key does not exist, and null, i.e. it exists and is empty.

To make up user requirements to take this argument a step further, if for some reason a consumer of these zap logs enforced a schema during ingestion silently dropping the key within the logging library could have adverse effects to the rest of the message.

[abhinav]

Makes sense. In that case, it's just a matter of deciding a convention.

Initial proposal: p suffix.

func Bool(key string, value bool) Field
func Boolp(key string, value *bool) Field

func Int64(key string, value int64) Field
func Int64p(key string, value *int64) Field

...

We'll only want to do it for primitives, not interfaces, so p here is a
reference to "pointer", not the nillability of the value. Interfaces, maps,
and slices explicitly considered out-of-scope here.

Thoughts? @jcorbin @prashantv @dnathe4th @twilly

[prashantv]

I'm fine with adding pointer methods for primitives.

I think using null if the value doesn't exist also makes sense, although it's not trivial -- I don't think the encoder interfaces allow encoding a nil. It is possible in a hacky way using AddReflected though:
https://play.golang.org/p/RxZ5KahqB9E

We might want to add a special-case for obj == nil where we skip the JSON marshaler.

[jbizzle]

@abhinav -- I took a shot at feeling my way through this. LMK if this sketch seems like the right approach and if so I can try and finish it out and submit a proper pull request.

[jbizzle]

Thanks for the feedback. Will put up a pull request with what I came with.

Also for posterity, leaving a note here that, if we want to go further and be less hacky than packing nil into Reflect(k, nil) and then having the json encoder unpack it and special-case it, we could do a bit more work and add something like

type AddNiller interface {
  AddNil(key string)
}

have the JSON Encoder implement it, and then document that marshaler implementations that are performance sensitive can check whether the encoder they receive implements AddNiller when attempting to write nil.

[abhinav]

RE: ^: I just realized that we'll still have to smuggle the nil value with Reflected because we don't get access to the encoder (to do the AddNiller interface check) until we get to the Core.

[jbizzle]

Right -- The NilAdder interface would only be of value to implementations of ObjectMarshaler that are looking to directly encode their own nil values.