Keep time location in zap.Time #425
Conversation
|
|
There was a problem hiding this comment.
Interesting - I hadn't considered the need to preserve time zones. This PR can't be merged as-is, since it introduces a performance regression in zap.Time.
However, we may be able to solve this problem another way. Can you clarify for me exactly what you need here? Do you need to preserve the original time zone, or do you need to encode all times in a single (non-system) time zone?
zapcore/json_encoder_entry_test.go
Outdated
| // AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
| // LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
| // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
| // THE SOFTWARE. |
There was a problem hiding this comment.
Good catch... :(
Thanks for adding the license.
field.go
Outdated
| @@ -175,7 +175,8 @@ func Stringer(key string, val fmt.Stringer) zapcore.Field { | |||
| // Time constructs a zapcore.Field with the given key and value. The encoder | |||
| // controls how the time is serialized. | |||
| func Time(key string, val time.Time) zapcore.Field { | |||
| return zapcore.Field{Key: key, Type: zapcore.TimeType, Integer: val.UnixNano()} | |||
| // Use Interface instead of Integer here to keep location in time. | |||
| return zapcore.Field{Key: key, Type: zapcore.TimeType, Interface: val} | |||
There was a problem hiding this comment.
Unfortunately, this makes every call to Time allocate.
There was a problem hiding this comment.
I made another try: 384c2f1
This time, we don't have extra allocate.
|
Thanks for your comments. |
|
Thanks for merging! |
The
zap.Time()did convert the time value to integer usingtime.UnixNano().zap/field.go
Line 178 in 7641ffe
This means we lost the location information.
So
EncodeEntry()forJSONEncoderandISO8601TimeEncoderreturned the time string with the local timezone, not the original location specified in the time value passed tozap.Time().I add a test case to reproduce this problem at zapcore/json_encoder_entry_test.go.
Notice this test case fails only for environments whose timezone is not UTC.
This pull request fixes this problem with setting the time value to the
Interfacefield ofFieldinstead ofIntegerfield.To maintain backward compatibility, I modified the
TimeTypecase infunc (f Field) AddTo(enc ObjectEncoder)to useInterfacefield when it is set (not nil) and useIntegerfield otherwise.Could you review this pull request?
Thanks!