Improvement: Add gologr svclog wrapper #114

etherandrius · 2021-01-13T11:47:13Z

After this PR

svc1log wrapper for the go-logr logger.

This is needed to get proper logs when using k8s sigs controller-runtime. https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/log/log.go#L42-L46
==COMMIT_MSG==
==COMMIT_MSG==

Possible downsides?

This change is

gologr-wlog/gologr.go

gologr-wlog/gologr_test.go

splucs · 2021-01-13T12:05:52Z

gologr-wlog/gologr_test.go

+	_ "github.com/palantir/witchcraft-go-logging/wlog-zap"
+)
+
+type logEntry struct {


This is fragile as the API might break in case the wlog-zap logEntry implementation changes.
Can you implement a mock logger that sends logs to a channel and assert on these messages?

Instead of using a struct I am now using a JSONMatcher

following the internal tests: https://github.com/etherandrius/witchcraft-go-logging/blob/develop/wlog/svclog/svc1log/svc1logtests/tests.go#L106-L125

In terms of fragility it's the same, but I think it's acceptable

gologr-wlog/gologr.go

etherandrius · 2021-01-14T12:27:32Z

Verify succeeded on the latest commit, but not showing up below.

bmoylan

Reviewed 10 of 12 files at r2.
Reviewable status: 9 of 11 files reviewed, 9 unresolved discussions (waiting on @etherandrius and @splucs)

wlog/svclog/svc1log/svc1logr/svc1logr.go, line 15 at r2 (raw file):

// limitations under the License.

package svc1logr

So I'd actually argue that because this is ancillary to the core functionality and products import it directly, it should be a top-level package if it's going to be in this repository.

svc1logr feels a little too succinct to indicate that this implements a different api and use case than the svc1log library. Don't want to spend a bunch of energy bikeshedding but something like wlog-logr would make more sense to me.

We should also add some thorough package-level documentation that describes what this is and why one might need to use it given that it will be relatively visible in this repo but only used in a handful of specific cases where we want compatibility.

nmiyake · 2021-01-20T17:15:32Z

Similar to @bmoylan's feedback, can you provide more context around how this will be used? I see the initial comment around "k8s sigs controller-runtime", but was hoping to get more context.

Is the idea that the top-level main package that uses those packages will import this logger implementation so that all logging goes through the same logging mechanism? Want to understand if this is a straightforward case of adding another logger implementation (like zapper or zerolog etc.) or if this is attempting to do something different.

etherandrius · 2021-01-26T11:31:08Z

Similar to @bmoylan's feedback, can you provide more context around how this will be used? I see the initial comment around "k8s sigs controller-runtime", but was hoping to get more context.

In general this should be used when you are using a library that uses the gologr log interface and you want to get logs from the library. For example we will use this to get logs from a conversion webhook implementation inside k8s sigs links.

Is the idea that the top-level main package that uses those packages will import this logger implementation so that all logging goes through the same logging mechanism?

Yes.

Want to understand if this is a straightforward case of adding another logger implementation (like zapper or zerolog etc.) or if this is attempting to do something different.

The main difference is that the returned logger is not a wlog logger but gologr logger. So in a sense it's the opposite of what the other packages do.

svc1logr feels a little too succinct to indicate that this implements a different api and use case than the svc1log library. Don't want to spend a bunch of energy bikeshedding but something like wlog-logr would make more sense to me.

I like making it a top level package. However, I would prefer to name it svc1logr (or logr-svc1log).

I don't want the name to have wlog in it since the implementation is specific to svc1log (not evt2log, etc..).
I don't want the name to be of the form wlog-* since the package is "opposite" of the other packages with this naming convention.

generate update readme rename package add package readme rename receiver svc1logr nit

nmiyake

Discussed further offline and synthesizing summary here.

My main question/hesitation is around whether it makes sense to have adapters (functions that wrap an svc1log in another logger interface for the purpose of interoperability with other libraries) as part of this main project. Unlike the logger implementation providers, this use case seemed more specialized.

However, it does make sense that the service logger does map most closely to a "regular" logger, and in scenarios like this where an external library/package expects a particular logger interface and we want the ability to delegate the logging to service logs, centralizing the translation here could make sense.

Currently I'm leaning towards being okay with having this implementation here, but would like to change the layout/organization to make it more general and make it more straightforward in a case where we might want to add another logger adapter of this form. For example, @etherandrius flagged that a similar pattern could be used to create an adapter for the go-acme logger (https://github.com/go-acme/lego/blob/master/log/logger.go) for projects that expect that logger but where we want to log to a service logger.

I'm going to examine a few other repositories that perform this kind of conversion and will then respond with concrete feedback in terms of where to move code and implementation.

Reviewable status: 9 of 13 files reviewed, 9 unresolved discussions (waiting on @etherandrius and @splucs)

nmiyake

Ah interesting -- taking a look at https://github.com/go-logr, the org itself has a repository per implementation for things like "zapr", "stdr", etc.

Ideally I think the best approach would be to have a wlogr or svc1logr implementation repo there. However, I think the probability of getting that done is probably low (even though this repo is OSS, it's not widely used so not sure that the maintainer would accept a repository there, and presumably we would also have less control).

Based on this, wondering if it might make more sense to have a svc1logr module in palantir/pkg rather than including it here -- that would fit better with my intuition that this adapter is a bit more of a separate client-type thing than the logger implementations themselves, which we do package with this repository (although I suppose an argument could be made that even those may make sense as separate modules).

@bmoylan do you have any thoughts on the above? Concretely, what do you think between having something like svclog/svc1log/adpaters with svc1logr as one of the directories, vs. having a separate github.com/palantir/pkg/svc1logr module?

Reviewable status: 9 of 13 files reviewed, 9 unresolved discussions (waiting on @etherandrius and @splucs)

nmiyake

Provided review on the content. Besides the content, the high-level question is the one posed above as to whether it makes sense to proceed with this in this repository or as a separate module.

Reviewed 10 of 12 files at r2, 4 of 4 files at r3.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @etherandrius and @splucs)

README.md, line 13 at r3 (raw file):

libraries. `witchcraft-go-logging` includes implementations that use [zap](https://github.com/uber-go/zap), 
[zerolog](https://github.com/rs/zerolog) and [glog](https://github.com/golang/glog). We also provide an implementation
for [go-logr](https://github.com/go-logr/logr) that uses svc1log internally.

If we proceed with this PR in this repository, then this should be expanded a bit -- I think it would be worth explaining that the repository also provides adapters that will return a particular logger implementation using svc1log underneath, and that an implementation for logr is provided.

gologr-wlog/gologr.go, line 52 at r1 (raw file):

Previously, etherandrius (Andrius Grabauskas) wrote…

I've read this https://dave.cheney.net/2015/11/05/lets-talk-about-logging and decided to bump these up from Warn -> Error

When this condition is hit, is it clear that the error is in the logging layer? Agree that it's good to message this in some way, but want to be sure that the source of the error is clear.

svc1logr/svc1logr.go, line 31 at r3 (raw file):

// New returns a go-logr interface implementation that uses svc1log internally.
func New(logger svc1log.Logger, origin string) logr.Logger {

Not sure if including origin string here is preferable from an API standpoint, especially given that svc1log itself supports setting the origin in ways besides using a hard-coded string. Is the motivation to do this to support the WithName function?

svc1logr/svc1logr.go, line 51 at r3 (raw file):

}

func (s *svc1logr) V(level int) logr.InfoLogger {

Believe the returned interface should be logr.Logger.

svc1logr/svc1logr.go, line 52 at r3 (raw file):

func (s *svc1logr) V(level int) logr.InfoLogger {
	return New(s.logger, s.origin)

This doesn't seem to honor the intent of the API -- the intent would be to return a new logger where the service logger level is set based on the provided value. Even if it's not possible to do with the service logger given our API, it should be possible to do this by tracking the level in the svc1logr struct and gating output on that.

svc1logr/svc1logr.go, line 61 at r3 (raw file):

func (s *svc1logr) WithName(name string) logr.Logger {
	return New(s.logger, filepath.Join(s.origin, name))

This should probably be path.Join rather than filepath.Join, as I don't think the separator used for the logger should be platform-dependent (don't think that using \ as the separator on Windows makes sense)

etherandrius

Reviewable status: 9 of 13 files reviewed, 14 unresolved discussions (waiting on @etherandrius, @nmiyake, and @splucs)

README.md, line 13 at r3 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

If we proceed with this PR in this repository, then this should be expanded a bit -- I think it would be worth explaining that the repository also provides adapters that will return a particular logger implementation using svc1log underneath, and that an implementation for logr is provided.

Done.

gologr-wlog/gologr_test.go, line 71 at r1 (raw file):

Previously, splucs (Lucas França de Oliveira) wrote…

Remove debug statements.

Done.

gologr-wlog/gologr.go, line 11 at r1 (raw file):

Previously, splucs (Lucas França de Oliveira) wrote…

Just name this logger.

I prefer to keep this svc1logr. It follows logr convention and doesn't clash with the logger sub-value.

gologr-wlog/gologr.go, line 17 at r1 (raw file):

Previously, splucs (Lucas França de Oliveira) wrote…

Package name already tells what it is, just name this New.

Done.

gologr-wlog/gologr.go, line 25 at r1 (raw file):

Previously, splucs (Lucas França de Oliveira) wrote…

Name the receiver the initial letter of the struct.

Done.

gologr-wlog/gologr.go, line 52 at r1 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

When this condition is hit, is it clear that the error is in the logging layer? Agree that it's good to message this in some way, but want to be sure that the source of the error is clear.

I think origin is enough to signify that this is from the external library.
I've prepended the logs with Logging layer:

gologr-wlog/gologr.go, line 57 at r1 (raw file):

Previously, splucs (Lucas França de Oliveira) wrote…

make(map[string]interface{}, len(keysAndValues)/2)

Done.

svc1logr/svc1logr.go, line 31 at r3 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

Not sure if including origin string here is preferable from an API standpoint, especially given that svc1log itself supports setting the origin in ways besides using a hard-coded string. Is the motivation to do this to support the WithName function?

Correct WithName only appends string to the name, hence we need to store/extract the origin value. I did not find a nice way to extract and change the origin already present in svc1log.Logger.

svc1logr/svc1logr.go, line 51 at r3 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

Believe the returned interface should be logr.Logger.

Done.

svc1logr/svc1logr.go, line 52 at r3 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

This doesn't seem to honor the intent of the API -- the intent would be to return a new logger where the service logger level is set based on the provided value. Even if it's not possible to do with the service logger given our API, it should be possible to do this by tracking the level in the svc1logr struct and gating output on that.

Done.

I've added two values to the struct level int and enabled bool.

level - signifies up to what verbosity we should log the output out
enabled - signifies if the current logging level is below or above our preferred verbosity level.

added a check to Info(..) function to check if logger is enabled, omitted the same from Error() function since errors are by definition verbosity 0. (It's illegal to have verbosity <0, If a user want to have no logs they should not use this package.)

svc1logr/svc1logr.go, line 61 at r3 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

This should probably be path.Join rather than filepath.Join, as I don't think the separator used for the logger should be platform-dependent (don't think that using \ as the separator on Windows makes sense)

Done.

bmoylan

// sorry, meant to publish this yesterday

Thanks @nmiyake for doing a thorough review here.

I actually like the idea of adapters -- I've written a few throwaway/internal versions of these over the years and it would be nice to put them in one place.

One downside is if it bloats the dependency tree. We strive to do a good job keeping the wlog impls isolated, and I'd want to do the same thing here either with modules or just copying the interface and duck-typing it. My gut feeling is that it feels a little heavy and specific to go in palantir/pkg and would prefer to keep them in this repo if manageable.

Reviewed 1 of 4 files at r3.
Reviewable status: 9 of 13 files reviewed, 15 unresolved discussions (waiting on @bmoylan, @etherandrius, @nmiyake, and @splucs)

svc1logr/svc1logr.go, line 30 at r3 (raw file):

}

// New returns a go-logr interface implementation that uses svc1log internally.

Maybe a better doc is something like

// New provides an implementation of [logr.Logger](https://pkg.go.dev/github.com/go-logr/logr#Logger) which delegates to a witchcraft svc1log logger.
// Use this package for software which expects a logr.Logger but where the calling code has access to an svc1log.Logger or wants the output to be Witchcraft-compatible.
// V levels are ignored: Info() is logged as INFO and Error() as ERROR. All values are converted to safe parameters.

etherandrius

I think I've addressed all the changes for now.

The only thing that's up in the air is where this should live:

In a top level package adapters
OR; In a svc1log subpackage svclog/svc1log/adapters/svc1logr

For now I am keeping it as a top level package.

As for duck-typing: I do not love the idea. For me it seems that it will only bring confusion and obfuscate the code. But It's up to you guys to decide I won't fight on the decision much.

Reviewable status: 9 of 12 files reviewed, 15 unresolved discussions (waiting on @bmoylan, @etherandrius, @nmiyake, and @splucs)

svc1logr/svc1logr.go, line 30 at r3 (raw file):

Previously, bmoylan (Brad Moylan) wrote…

Maybe a better doc is something like

// New provides an implementation of [logr.Logger](https://pkg.go.dev/github.com/go-logr/logr#Logger) which delegates to a witchcraft svc1log logger.
// Use this package for software which expects a logr.Logger but where the calling code has access to an svc1log.Logger or wants the output to be Witchcraft-compatible.
// V levels are ignored: Info() is logged as INFO and Error() as ERROR. All values are converted to safe parameters.

Done. Modified the comment slightly since we now support V levels, to be more inline with the API.

nmiyake

Sorry for the long delay.

Reviewed again, and am okay with the overall concept/approach. I would prefer the current location in the top-level "adapters" directory over mingling individual adapters as subpackages.

Left feedback on content.

Reviewed 3 of 4 files at r4.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @bmoylan, @etherandrius, @nmiyake, and @splucs)

README.md, line 9 at r4 (raw file):

[![](https://godoc.org/github.com/palantir/witchcraft-go-logging?status.svg)](http://godoc.org/github.com/palantir/witchcraft-go-logging)

`witchcraft-go-logging` is a Go implementationof the Witchcraft logging specification. It provides an API that can be

Would prefer this for this section:

`witchcraft-go-logging` is a Go implementation of the Witchcraft logging specification. It provides an API that can be
used for logging and some default implementations of the logging API using different existing popular Go logging
libraries.

Currently, implementations that use the following libraries are provided:
* [zap](https://github.com/uber-go/zap)
* [zerolog](https://github.com/rs/zerolog)
* [glog](https://github.com/golang/glog)

This module also includes adapters that allow Witchcraft loggers to satisfy a different logging interface. The most
common use case is provide compatibility between the `svc1log.Logger` interface and other Go logging interfaces.

Currently, adapters for the following interfaces are provided:
* [go-logr](https://github.com/go-logr/logr)

adapters/svc1logr/svc1logr.go, line 42 at r4 (raw file):

		logger:  logger,
		level:   level,
		enabled: true,

This results in incorrect behavior if the provided level is >0, since the logger will behave as if it is enabled.

adapters/svc1logr/svc1logr.go, line 60 at r4 (raw file):

}

func (s *svc1logr) V(level int) logr.Logger {

Would add comment to both constructor and to this function that explains that the level being set here only controls whether or not this interface will perform the logging, and is independent from the underlying logger (if the svc1log.Logger being wrapped is at the ERROR level, then it won't output anything at INFO level regardless of the level set here)

adapters/svc1logr/svc1logr.go, line 68 at r4 (raw file):

		origin:  s.origin,
		logger:  s.logger,
		level:   s.level,

This implementation still doesn't match the interface API -- based on the documentation and examples of existing implementations, I think this is meant to be strictly additive:

svc1logr/svc1logr.go, line 52 at r3 (raw file):

Previously, etherandrius (Andrius Grabauskas) wrote…

Done.

I've added two values to the struct level int and enabled bool.

level - signifies up to what verbosity we should log the output out
enabled - signifies if the current logging level is below or above our preferred verbosity level.

added a check to Info(..) function to check if logger is enabled, omitted the same from Error() function since errors are by definition verbosity 0. (It's illegal to have verbosity <0, If a user want to have no logs they should not use this package.)

Thanks for this information -- would add this as a comment in the implementation of the Error function so that this context is captured in code.

etherandrius commented Jan 13, 2021

View reviewed changes

gologr-wlog/gologr.go Outdated Show resolved Hide resolved

splucs requested changes Jan 13, 2021

View reviewed changes

gologr-wlog/gologr.go Outdated Show resolved Hide resolved

gologr-wlog/gologr.go Outdated Show resolved Hide resolved

agrabauskas added 5 commits January 14, 2021 14:10

Improvement: Add gologr svclog wrapper

131a76a

bump warnings to errors

8bc6536

Address CR

75992be

more consistent naming

9d62d40

tests

3ad0b76

etherandrius force-pushed the ag/gologrwrapper branch from 91a201d to 3ad0b76 Compare January 14, 2021 14:13

changelog

3ae166d

splucs approved these changes Jan 15, 2021

View reviewed changes

etherandrius added autorelease merge when ready labels Jan 18, 2021

bmoylan requested changes Jan 20, 2021

View reviewed changes

etherandrius force-pushed the ag/gologrwrapper branch 2 times, most recently from 5248c1e to a7223c5 Compare January 26, 2021 12:06

move package to top level

2b396e7

generate update readme rename package add package readme rename receiver svc1logr nit

etherandrius force-pushed the ag/gologrwrapper branch from a7223c5 to 2b396e7 Compare January 26, 2021 12:07

nmiyake reviewed Jan 28, 2021

View reviewed changes

nmiyake requested changes Jan 28, 2021

View reviewed changes

agrabauskas added 4 commits January 29, 2021 09:26

logr.Logger

baaf302

verbosity, error log changes, filepath->path

a254909

better readme

9cbe3c5

move svc1logr -> adapters/svc1logr

6f9adb7

etherandrius commented Jan 29, 2021

View reviewed changes

fix testss

0abd129

bmoylan reviewed Jan 29, 2021

View reviewed changes

better package docs

75996bb

etherandrius commented Feb 1, 2021

View reviewed changes

nmiyake requested changes Feb 25, 2021

View reviewed changes

bmoylan mentioned this pull request Apr 21, 2022

feature: Add adapters/svc1zap for code that accepts *zap.Logger #198

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvement: Add gologr svclog wrapper #114

Improvement: Add gologr svclog wrapper #114

etherandrius commented Jan 13, 2021 •

edited by bmoylan

splucs Jan 13, 2021

etherandrius Jan 14, 2021

etherandrius commented Jan 14, 2021

bmoylan left a comment

nmiyake commented Jan 20, 2021

etherandrius commented Jan 26, 2021 •

edited

nmiyake left a comment

nmiyake left a comment

nmiyake left a comment

etherandrius left a comment

bmoylan left a comment

etherandrius left a comment

nmiyake left a comment

Improvement: Add gologr svclog wrapper #114

Are you sure you want to change the base?

Improvement: Add gologr svclog wrapper #114

Conversation

etherandrius commented Jan 13, 2021 • edited by bmoylan

After this PR

Possible downsides?

splucs Jan 13, 2021

Choose a reason for hiding this comment

etherandrius Jan 14, 2021

Choose a reason for hiding this comment

etherandrius commented Jan 14, 2021

bmoylan left a comment

Choose a reason for hiding this comment

nmiyake commented Jan 20, 2021

etherandrius commented Jan 26, 2021 • edited

nmiyake left a comment

Choose a reason for hiding this comment

nmiyake left a comment

Choose a reason for hiding this comment

nmiyake left a comment

Choose a reason for hiding this comment

etherandrius left a comment

Choose a reason for hiding this comment

bmoylan left a comment

Choose a reason for hiding this comment

etherandrius left a comment

Choose a reason for hiding this comment

nmiyake left a comment

Choose a reason for hiding this comment

etherandrius commented Jan 13, 2021 •

edited by bmoylan

etherandrius commented Jan 26, 2021 •

edited