inconsistent display of zap.Time field with zero time struct

[gwik]

When logging a zero initialized time.Time struct the output is not consistent:

zap.L().With("time", time.Time{}).Debug("zero time")

Will output the time: 1754-08-30T22:43:41.128654848Z because calling UnixNano() then Unix(0, nsec) is not symetric in this case.
The correct output would be 0001-01-01 00:00:00 +0000 UTC (with the same layout as above).

see https://gist.github.com/patdowney/b64a4fee790ac66dd1a2 that exhibit the issue.

Looking at zap.Time I was wondering why you don't pass the Time struct in the Interface field instead of converting to an integer. Maybe this is to avoid making a new reference to traverse for the GC ?

zap/field.go

Lines 180 to 184 in 4252145

	// Time constructs a Field with the given key and value. The encoder
	// controls how the time is serialized.
	func Time(key string, val time.Time) Field {
	return Field{Key: key, Type: zapcore.TimeType, Integer: val.UnixNano(), Interface: val.Location()}
	}

zap/zapcore/field.go

Lines 141 to 147 in badef73

	case TimeType:
	if f.Interface != nil {
	enc.AddTime(f.Key, time.Unix(0, f.Integer).In(f.Interface.(*time.Location)))
	} else {
	// Fall back to UTC if location is nil.
	enc.AddTime(f.Key, time.Unix(0, f.Integer))
	}

[prashantv]

Storing a time.Time directly in the Interface field would require an additional allocation, while storing the integer + the *time.Location avoids the allocation (since *time.Location is already a pointer).

Since zap is focused on performance, we made the choice to avoid allocations here.

I think we have a couple of options to fix this though:

• If we care specifically about the zero case, we can create a new field type specifically for zero time, or store a special string value.
• We can create a new field type that stores time.Time if the value is outside of the range of what UnixNano supports,
  https://golang.org/pkg/time/#Time.UnixNano

[gwik]

Of course, storing in interface{} would make it escape to the heap.

If we care specifically about the zero case, we can create a new field type specifically for zero time, or store a special string value.

I think storing a special string makes sense. I'm not sure this requires a new field type as the added cost would be very low if you're in the int64 range.

[prashantv]

After thinking about this more, I think it might actually be cleaner to just have 2 types of time fields:

• The current type which uses UnixNano() and stores the int64 field
  *A new type which uses an interface{} and stores the *time.Time.

The latter type should only be used if the field cannot be stored as an int64.