Add WithClock Option #897
Add WithClock Option #897
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #897 +/- ##
=======================================
Coverage 98.21% 98.22%
=======================================
Files 43 44 +1
Lines 1910 1915 +5
=======================================
+ Hits 1876 1881 +5
Misses 27 27
Partials 7 7
Continue to review full report at Codecov.
|
ernado
force-pushed
the
feat/time-source
branch
from
9479a5b
to
8c20a9b
Compare
There was a problem hiding this comment.
Hello, and thanks for the contribution! This is a good
change, however, in the interest of future expansion,
perhaps the source of time should be an interface? And we
could call it Clock.
type Clock interface { Now() time.Time }
func WithClock(Clock) Option
The rationale behind that is that interfaces can be upcast,
so it lets us expand to more clock-based functionality in
the future.
For example, if we end up needing time.Sleep, we can add a
Sleeper interface with a sleep method and use that if the
Clock supports it.
// hypothetical sleep code
if sleeper, ok := logger.clock.(Sleeper); ok {
sleeper.Sleep(t)
}
time_source_test.go
Outdated
| @@ -0,0 +1,40 @@ | |||
| // Copyright (c) 2016 Uber Technologies, Inc. | |||
There was a problem hiding this comment.
| // Copyright (c) 2016 Uber Technologies, Inc. | |
| // Copyright (c) 2020 Uber Technologies, Inc. |
ernado
force-pushed
the
feat/time-source
branch
2 times, most recently
from
337a417
to
f245f69
Compare
Allow passing current time source.
ernado
force-pushed
the
feat/time-source
branch
from
f245f69
to
e1bca37
Compare
|
I like this approach even more :) |
There was a problem hiding this comment.
LGTM besides minor suggestions and doc changes.
We still call time.Now() in CheckedEntry.Write when writing Zap-internal errors to ErrorOutput, but I think that's fine because this is a source of time for logged entries only.
CC @prashantv
There was a problem hiding this comment.
Change looks good, although there's still calls to time.Now() that have not been updated,
> git grep 'time.Now' | grep '\.go:' | grep -v '_test\.go'
logger.go: Time: time.Now(),
logger.go: fmt.Fprintf(log.errorOutput, "%v Logger.check error: failed to get caller\n", time.Now().UTC())
zapcore/entry.go: fmt.Fprintf(ce.ErrorOutput, "%v Unsafe CheckedEntry re-use near Entry %+v.\n", time.Now(), ce.Entry)
zapcore/entry.go: fmt.Fprintf(ce.ErrorOutput, "%v write error: %v\n", time.Now(), err)
Can you check to see if there's any other cases of time being used that need to be updated?
We'll likely want to add a ticker for #782 as well (which we can do as a follow-up).
|
Discussed with @prashantv. For the error cases, we can use the timestamps from the entries. Incoming. |
For error cases, use the timestamp attached to the corresponding entry rather than the current time.
In #897, we added a Clock interface to allow control over the source of time for operations that require accessing the current time. In #782, we discovered that this interface also needs the ability to construct tickers so that we can use it for the buffered writer. This change adds NewTicker to the Clock interface for Zap and moves it to the zapcore package as it will be needed for #782. Note that since we have not yet tagged a release of Zap with #897, this is not a breaking change. Co-authored-by: Minho Park <minho.park@uber.com> Co-authored-by: Abhinav Gupta <abg@uber.com>
This release v1.17.0 of Zap with all changes from `master` minus the following: - #897 - #948 - #950 These changes were omitted because we're still in the process of changing the interface introduced in #897. We will include that in the next release. Release changelog: https://github.com/uber-go/zap/blob/release-1-17/CHANGELOG.md#1170-25-may-2021 Using [apidiff], the surface area of this release compared to v1.16.0 is: ``` --- go.uber.org/zap --- Compatible changes: - Inline: added --- go.uber.org/zap/zapcore --- Compatible changes: - InlineMarshalerType: added --- go.uber.org/zap/zapgrpc --- Compatible changes: - (*Logger).Error: added - (*Logger).Errorf: added - (*Logger).Errorln: added - (*Logger).Info: added - (*Logger).Infof: added - (*Logger).Infoln: added - (*Logger).V: added - (*Logger).Warning: added - (*Logger).Warningf: added - (*Logger).Warningln: added --- go.uber.org/zap/zaptest/observer --- Compatible changes: - (*ObservedLogs).FilterFieldKey: added ``` [apidiff]: https://github.com/golang/exp/blob/master/apidiff/README.md Resolves #942 Refs GO-599
In #897, we added a `zap.Clock` option to control the source of time but neglected to set this field on the logger constructed by `zap.NewNop`. This has the effect of panicking the Nop logger with a nil dereference. Fix the nil dereference and add checks for the behavior of the Nop logger. Verified that these are the only instantiations of `Logger` in this package: ``` $ rg '\bLogger\{' *.go logger_test.go 67: for _, logger := range []*Logger{grandparent, parent, child} { logger.go 71: log := &Logger{ 86: return &Logger{ ``` Refs GO-684
Add WithClock option to control the source of time for logged entries. Fixes uber-go#694 Co-authored-by: Abhinav Gupta <abg@uber.com>
Allow passing current time source.
Fix #694