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

Experimental: zap backend for go-log #61

Merged
merged 12 commits into from
Sep 5, 2019
Merged

Experimental: zap backend for go-log #61

merged 12 commits into from
Sep 5, 2019

Conversation

Kubuxu
Copy link
Member

@Kubuxu Kubuxu commented Jun 24, 2019

Builds and works with go-ipfs, exposes zap functionality.

Copy link
Member

@frrist frrist left a comment

Choose a reason for hiding this comment

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

👍 This looks good to me. Excited to use the new functionality this adds if we decide to move forward with this!

log.go Outdated Show resolved Hide resolved
oldlog.go Outdated Show resolved Hide resolved
@Kubuxu
Copy link
Member Author

Kubuxu commented Jul 8, 2019

Updated and rebased on master

@frrist
Copy link
Member

frrist commented Aug 20, 2019

Hey @Kubuxu is there anything I could do to help move this forward? I have been using it on a test branch for go-filecoin. Having the capability to produce structured logs with this would be great.

@Kubuxu Kubuxu marked this pull request as ready for review August 20, 2019 21:15
@frrist frrist requested a review from Stebalien August 20, 2019 21:22
@lanzafame
Copy link
Contributor

@Kubuxu @frrist with all the projects that use go-log now moved over to using go modules, can we tag this as a new major release and move on?

oldlog.go Outdated Show resolved Hide resolved
oldlog.go Outdated Show resolved Hide resolved
log.go Outdated Show resolved Hide resolved
oldlog.go Outdated Show resolved Hide resolved
@Stebalien
Copy link
Member

As far as I can tell, the only real breaking changes are around setting log-levels, right? In that case, bumping a minor (this is pre-1.0) should be fine. Yeah, it's a breaking change but it's easy to deal with (and there's no good way to avoid it).

Well, technically, we could release a version of go-logging that uses zap.


@lanzafame this project is pre-1.0 so bumping the major to v1 doesn't really help (and we're not ready for that anyways).

Copy link
Member

@frrist frrist left a comment

Choose a reason for hiding this comment

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

Looks good, couple minor comments that I will leave up to you.
:shipit:

oldlog.go Outdated Show resolved Hide resolved
oldlog.go Outdated Show resolved Hide resolved
levels.go Outdated Show resolved Hide resolved
levels.go Outdated Show resolved Hide resolved
oldlog.go Outdated Show resolved Hide resolved
// TODO if we would like to adjust log levels at run-time. Store this event
// logger in a map (just like the util.Logger impl)
// Logger retrieves an event logger by name
func Logger(system string) *ZapEventLogger {
Copy link
Member

Choose a reason for hiding this comment

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

Is the plan to eventually switch to zap directly? It seems odd to hard-code zap into this library and expose it like this if that isn't the case.

However, if that is the case, we should have a migration plan in place to get rid of the facade.

Copy link
Member Author

Choose a reason for hiding this comment

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

Zap's interface is big but very useful for structured logging. I don't see any benefit of writing a full interface facade for it (it won't allow us to switch to something else easier).

The migration is mostly limited to stopping the use of deprecated functions.

Copy link
Member

Choose a reason for hiding this comment

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

But is there a plan to eventually drop this library and directly depend on Zap.

Copy link
Member

Choose a reason for hiding this comment

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

I too was in the camp that logging is so pervasive that we shouldn't introduce indirections. Also, changing the logging backend happens once in a blue moon -- so it's a questionable risk/reward gradient. However, there are some legit use cases where you'd want to replace the logging backend with something else, e.g. WASM runtimes.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, as Zap doesn't provide centralised log level facilities.

Copy link
Member

Choose a reason for hiding this comment

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

However, there are some legit use cases where you'd want to replace the logging backend with something else, e.g. WASM runtimes.

I guess we'd do that using build tags anyway. As long as the callers don't need to reference zap or zapcore explicitly, we should be able to mimic its public API?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can change how logging is done for WASM by changing formatters and outputs in Zap.

Problem with mimicking Zap's API is that they are at least 93 functions we would have to define.

In some cases (performance) one would use zap.Field and connected functions to achieve 0-overhead structured logging.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

LGTM (would still like to think through the rest of the upgrade path but this looks like a reasonable step forward).

@Stebalien
Copy link
Member

Stebalien commented Aug 28, 2019

I'd like to get @raulk to give a signoff as this affects go-libp2p pretty significantly. Also, @hsanjuan.

Breaking changes:

  • SetAllLoggers now takes a string level and go-logging is no longer a dep.
  • Warning has been officially deprecaed in favor of Warn (may cause linters to fail).
  • edit: it breaks custom log formats

The benefit is:

  • Structured logging support.
  • Switching to someone else's logging library (instead of our own go-logging).

@hsanjuan
Copy link
Contributor

I have customized LogFormats because I could not read things in blue. This will also break that. But ok..

oldlog.go Show resolved Hide resolved
// TODO if we would like to adjust log levels at run-time. Store this event
// logger in a map (just like the util.Logger impl)
// Logger retrieves an event logger by name
func Logger(system string) *ZapEventLogger {
Copy link
Member

Choose a reason for hiding this comment

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

I too was in the camp that logging is so pervasive that we shouldn't introduce indirections. Also, changing the logging backend happens once in a blue moon -- so it's a questionable risk/reward gradient. However, there are some legit use cases where you'd want to replace the logging backend with something else, e.g. WASM runtimes.

log.go Show resolved Hide resolved
oldlog.go Show resolved Hide resolved
oldlog.go Outdated Show resolved Hide resolved
Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

LGTM. @Kubuxu I took the liberty to add some godocs and update deps; revert if inappropriate.

@raulk
Copy link
Member

raulk commented Aug 29, 2019

Quick alignment comment

@whyrusleeping: IIRC in the "exposing zap vs. abstraction with a zap backend" discussion, you advocated for keeping an abstraction layer as a way to substitute the backend in alternative runtime environments, like WASM. Zap is pretty modular and we should be able to do that replacement within Zap itself. Are you OK going in this direction?

@albrow: since 0x is already running go-libp2p on WASM, are you happy with this change? It should bring important optimisations.

@Kubuxu: do you have bandwidth to introduce a few go benchmarks to quantify the gains under different conditions (console, file, structured logging, different levels, etc.)?


Looks like the tests provoke a deadlock and a few data races: https://travis-ci.com/ipfs/go-log/builds/125061936

@Kubuxu
Copy link
Member Author

Kubuxu commented Aug 29, 2019

Quick benchmarks:

benchmark                      old ns/op     new ns/op     delta
BenchmarkSimpleInfo-4          5598          4015          -28.28%
BenchmarkFormatInfo-4          5819          4523          -22.27%
BenchmarkFormatInfoMulti-4     4482          2633          -41.25%

benchmark                      old allocs     new allocs     delta
BenchmarkSimpleInfo-4          27             10             -62.96%
BenchmarkFormatInfo-4          27             11             -59.26%
BenchmarkFormatInfoMulti-4     28             12             -57.14%

benchmark                      old bytes     new bytes     delta
BenchmarkSimpleInfo-4          928           368           -60.34%
BenchmarkFormatInfo-4          1488          569           -61.76%
BenchmarkFormatInfoMulti-4     1464          562           -61.61%

@Kubuxu
Copy link
Member Author

Kubuxu commented Aug 29, 2019

Looks like the tests provoke a deadlock and a few data races:

Those were there, it is a race inside a test (not the event writer) and race detector causes the lockup.

@Kubuxu
Copy link
Member Author

Kubuxu commented Aug 29, 2019

I probably won't have time to benchmark it more (at least right now).

@albrow
Copy link

albrow commented Aug 29, 2019

@raulk we aren't enabling go-log except for some rare cases where we need to debug or understand libp2p internals. As long as the new changes still allow libp2p to compile to Wasm it won't really affect us.

@Kubuxu
Copy link
Member Author

Kubuxu commented Sep 4, 2019

@Stebalien @hsanjuan @raulk
If there are no objections, I will merge this tomorrow.

@Stebalien
Copy link
Member

Please do.

@raulk
Copy link
Member

raulk commented Sep 4, 2019

Benchmarks look great.

Kubuxu and others added 12 commits September 4, 2019 22:05
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants