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

Standardize the logging interface to use logr.Logger #130

Open
samj1912 opened this issue Apr 2, 2022 · 5 comments
Open

Standardize the logging interface to use logr.Logger #130

samj1912 opened this issue Apr 2, 2022 · 5 comments
Labels
type:enhancement A general enhancement

Comments

@samj1912
Copy link
Member

samj1912 commented Apr 2, 2022

This is a standard interface in golang for logging and contains implementations in this interface with -

There are implementations for the following logging libraries:

  • a function (can bridge to non-structured libraries): funcr
  • a testing.T (for use in Go tests, with JSON-like output): testr
  • github.com/google/glog: glogr
  • k8s.io/klog (for Kubernetes): klogr
  • a testing.T (with klog-like text output): ktesting
  • go.uber.org/zap: zapr
  • log (the Go standard library logger): stdr
  • github.com/sirupsen/logrus: logrusr
  • github.com/wojas/genericr: genericr (makes it easy to implement your own backend)
  • logfmt (Heroku style logging): logfmtr
  • github.com/rs/zerolog: zerologr
  • github.com/go-kit/log: gokitlogr (also compatible with github.com/go-kit/kit/log since v0.12.0)
  • bytes.Buffer (writing to a buffer): bufrlogr (useful for ensuring values were logged, like during testing)

More details at https://github.com/go-logr/logr

It is also the interface use by OTEL -> open-telemetry/opentelemetry-go#1068

Instead of re-inventing the wheel, we should just use this in libcnb v2.

@samj1912
Copy link
Member Author

samj1912 commented Apr 2, 2022

For standard list of log levels, there is an open issue at open-telemetry/opentelemetry-specification#2039

@dmikusa
Copy link
Contributor

dmikusa commented Apr 2, 2022

This is more in line with what I had originally been thinking but after looking at the libcnb code base and what we actually use, and talking with @ekcasey, I think this is still too much API. We only ever log stuff at debug, with one exception, and that was a warning which @ekcasey convinced me should just be an error. So I think what we need sits between fmt.Println and the logr.Logger API.

In the interest of keeping the API as slim as possible, only what we need, and not taking on external dependencies, I would prefer to keep our own API. Unless there are some killer use cases that this library would enable that I'm missing.

@samj1912
Copy link
Member Author

samj1912 commented Apr 2, 2022

In terms of the actual dependencies, the library does not import any external package -> https://github.com/go-logr/logr/blob/master/go.mod

Even in terms of stdlib it only imports the context package https://pkg.go.dev/github.com/go-logr/logr?tab=imports

As for killer features, it lets buildpack authors use a logger of their choice and has implementation for all popular logging libraries.

It is also the default logging interface used in OTEL -> https://pkg.go.dev/go.opentelemetry.io/otel#SetLogger

which means that if buildpack authors using libcnb want integration with OTEL, they can reuse the same interface.

I think the value add is fairly large and we can just use the stdlib implementation of logr in libcnb by default if users don't specify one (https://github.com/go-logr/stdr). This is again the default logr implementation used by OTEL https://github.com/open-telemetry/opentelemetry-go/blob/b7a5c1a4e5205760415024017c20c4188193576f/go.mod#L7

@dmikusa
Copy link
Contributor

dmikusa commented Apr 2, 2022

In terms of the actual dependencies, the library does not import any external package -> https://github.com/go-logr/logr/blob/master/go.mod
Even in terms of stdlib it only imports the context package https://pkg.go.dev/github.com/go-logr/logr?tab=imports

Right, it's small but libcnb has to take a dependency on github.com/go-logr/logr, or am I misunderstanding something?

As for killer features, it lets buildpack authors use a logger of their choice and has implementation for all popular logging libraries.
It is also the default logging interface used in OTEL -> https://pkg.go.dev/go.opentelemetry.io/otel#SetLogger

This was along the lines of what I had originally been thinking until @ekcasey and I talked. Part of the conversation was do buildpack authors really need a full-blown logging/telemetry library?

I can't speak for others, but in Paketo Java I think the answer is no. What we really want is to be able to control the text format. To make the output of libcnb blend with the output that we write from our buildpacks. I don't need my output prefixed with the log level, a time stamp, line number, etc.., and I don't need the ability to configure logging on the fly or change output sinks. I almost think calling the interface a Logger is not right. What I need is really just an output formatter.

I respect that others might, so I'm genuinely curious if folks want all the logging goodies.

I do think that with our basic interface we could still allow folks to plug in their own logging framework of choice, if they want that, by having them write an adapter to our Logger interface for their logging framework of choice. It is additional code, but it's not complex code.

@dmikusa
Copy link
Contributor

dmikusa commented Oct 3, 2023

@samj1912 Is this something you still want to see in libcnb?

We did revamp the logger a bit to make it extendable. I believe that you could create an adapter to send logs to another logging framework. I've used the adapter in Paketo's libpak v2 to integrate it with the logger we have in libpak, and that has been working well.

Does this work for you or would you like to keep this open? Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants