Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add WithClock Option #897

Merged
merged 6 commits into from Jan 8, 2021
Merged

Add WithClock Option #897

merged 6 commits into from Jan 8, 2021

Conversation

ernado
Copy link
Contributor

@ernado ernado commented Dec 24, 2020

Allow passing current time source.

Fix #694

@CLAassistant
Copy link

@CLAassistant CLAassistant commented Dec 24, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

@codecov codecov bot commented Dec 24, 2020

Codecov Report

Merging #897 (a0150fa) into master (a68efdb) will increase coverage by 0.00%.
The diff coverage is 88.88%.

Impacted file tree graph

@@           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           
Impacted Files Coverage Δ
zapcore/entry.go 93.82% <50.00%> (ø)
clock.go 100.00% <100.00%> (ø)
logger.go 96.19% <100.00%> (+0.03%) ⬆️
options.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a68efdb...a0150fa. Read the comment docs.

Copy link
Contributor

@abhinav abhinav left a 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)
}

@@ -0,0 +1,40 @@
// Copyright (c) 2016 Uber Technologies, Inc.
Copy link
Contributor

@abhinav abhinav Dec 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Copyright (c) 2016 Uber Technologies, Inc.
// Copyright (c) 2020 Uber Technologies, Inc.

@ernado ernado force-pushed the feat/time-source branch 2 times, most recently from 337a417 to f245f69 Compare Dec 24, 2020
Allow passing current time source.
@ernado ernado changed the title Add WithTimeSource Option Add WithClock Option Dec 24, 2020
@ernado
Copy link
Contributor Author

@ernado ernado commented Dec 24, 2020

I like this approach even more :)
I've updated pull request to use Clock interface, thank you!
Not sure about naming in comments, hope they are fine too.

@ernado ernado requested a review from abhinav Dec 24, 2020
Copy link
Contributor

@abhinav abhinav left a 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

clock.go Outdated Show resolved Hide resolved
logger.go Outdated Show resolved Hide resolved
clock.go Outdated Show resolved Hide resolved
options.go Outdated Show resolved Hide resolved
clock_test.go Outdated Show resolved Hide resolved
logger.go Outdated Show resolved Hide resolved
logger.go Show resolved Hide resolved
logger.go Show resolved Hide resolved
Copy link
Contributor

@prashantv prashantv left a 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).

@abhinav
Copy link
Contributor

@abhinav abhinav commented Jan 7, 2021

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.
@abhinav abhinav merged commit f8ef926 into uber-go:master Jan 8, 2021
3 of 4 checks passed
@ernado ernado deleted the feat/time-source branch Jan 8, 2021
@abhinav abhinav mentioned this pull request May 18, 2021
abhinav added a commit that referenced this issue May 25, 2021
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>
@abhinav abhinav mentioned this pull request May 25, 2021
abhinav added a commit that referenced this issue May 25, 2021
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
abhinav added a commit that referenced this issue Jun 28, 2021
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
cgxxv pushed a commit to cgxxv/zap that referenced this issue Mar 25, 2022
Add WithClock option to control the source of time for logged entries.

Fixes uber-go#694

Co-authored-by: Abhinav Gupta <abg@uber.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants