buildpacks / libcnb

A non-opinionated language binding for the Cloud Native Buildpack Buildpack and Extension specifications
Apache License 2.0
31 stars 13 forks source link

Standardize the logging interface to use logr.Logger #130

Closed samj1912 closed 2 months ago

samj1912 commented 2 years ago

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

There are implementations for the following logging libraries:

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

It is also the interface use by OTEL -> https://github.com/open-telemetry/opentelemetry-go/issues/1068

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

samj1912 commented 2 years ago

For standard list of log levels, there is an open issue at https://github.com/open-telemetry/opentelemetry-specification/issues/2039

dmikusa commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 11 months ago

@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

dmikusa commented 2 months ago

This has been open for a while without feedback, so I'm going to close it.

We did make some changes in the logging interface, so this should be more adaptable to different user needs. If we still need to take things a step further, I'm open to that. Let's talk use cases and more details in a new issue.