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

Consider deprecating use of third-party logging libraries in favour of stdlib (log/slog) ? #609

Closed
udf2457 opened this issue Feb 11, 2024 · 9 comments · Fixed by #617
Closed

Comments

@udf2457
Copy link
Contributor

udf2457 commented Feb 11, 2024

log/slog introduced in Go 1.21 brings structured logging that was previously the main reason to use a third-party library.

Reducing dependencies on third-party libraries is always a good thing IMHO. 😉

@rdimitrov
Copy link
Contributor

I agree 👍 Pinging @MDr164 as he was the one who mainly helped abstract this

@MDr164
Copy link
Contributor

MDr164 commented Feb 28, 2024

Technically go-logr, which I implemented before for structured logging purposes, is just an interface definition which tries to implement a common standard for logger (e.g. logrus, which was used before go-logr). We can now go two routes. Keep go-logr and ditch logrus, as go-logr also allows using log/slog as one possible implementation or go full log/slog.

Benefits of using go-logr: People can plug in their own custom logger with minimal effort as long as it implements the interface defined by go-logr.

Benefits of using log/slog: We fully rely on stdlib but other will have to accept the fact that we use log/slog or write custom handlers for it if they need extra functionality.

I personally don't mind either route here but being a go-logr user quite some time, before log/slog existed, I do have my own preference of course.

@udf2457
Copy link
Contributor Author

udf2457 commented Feb 28, 2024

I would say there is an increasing evidence from recent years in preferring stdlib to third-party.

This is perhaps particularly the case with Go where we are blessed with the good fortune of such a comprehensive stdlib unlike, say Rust where you have to farm out to third-parties for everything.

First there is the supply-chain security aspect which in recent years has become a very real concern. The less moving parts the better.

Second, we again know from recent years, third-party repos have habit of becoming "yesterday's story", either becoming explicit abandonware through project archiving, or implicit through core maintainers loosing interest and just letting it fester. It is the unfortunate side of OSS that continued maintenance is effectively wholly reliant on the goodwill of others.

@rdimitrov via #485 and aided by others has done a lot of work bring together a more sensible and simplified approach to go-tuf. One of the fundamental aspects mentioned in the opening post of #485 was "regarding the overall maintainability and usability of the current implementation". Thus I would argue that moving as much as possible to stdlib is very much in keeping with the maintainability vision.

@MDr164
Copy link
Contributor

MDr164 commented Feb 28, 2024

I agree with reducing technical debt as much as possible but given that go-logr is still not an implementation but rather just a common interface I would treat it differently here. The idea was to be able to provide your own logger if you for example include go-tuf as a library in a bigger project and we didn't want to make assumptions about other peoples choice of a logger. Given that go-logr is mostly an interface, there is not much technical debt going on here. Don't get me wrong, I don't have any objections but I also do not see much benefit of replacing go-logr. What we could do however is to ditch the logrus implementation and opt for log/slog as our default.

As I am not the sole decision maker I would recommend getting more opinions from the other maintainers on this matter.

@rdimitrov
Copy link
Contributor

I don't have a very strong preference on this one, but I prefer more to keep the interface and switch to using the standard library instead of logrus.

I like that it's a nice balance between allowing users to tailor the library to their existing logging choice but also dropping the 3rd part dependency we have with logrus (not that it's bad, but I do relate to what @udf2457 is saying about reusing as much as possible from the std library).

Of course we can always revisit this if we get enough signals for it, but for the time being the suggestion from @MDr164 seems good enough.

@kommendorkapten - do you have any thoughts on that in relation to how it is used in sigstore-go?

@kommendorkapten
Copy link
Member

I would first like to say that go-tuf does not depend on go-logr. We define our own logging interface. This interface is a sub-set of the interface that go-logr defines.

The go.mod containes references to go-logr yes, but those are all comming from either tests programs or unit tests.

As the interface is very small, it's easy to provide whatever logger the client is using, possibly by wrapping it in a small adapter.

@rdimitrov sigstore-go does not use any logging, it's all done by returning an error.

@rdimitrov
Copy link
Contributor

@kommendorkapten - thanks for your feedback 👍 I completely missed to check where we actually use the loggers and you're right - we already have a pretty good go.mod file where:

  • go-logr is used only as part of the examples and tests
  • logrus is used only as part of the tests.

Perhaps as an action item from this issue, I'll be happy if we move from logrus to log/slog for the tests so we can trim this even more, but other than that it seems that what we have now is pretty lightweight 👍

In any case @udf2457 thanks for bringing this up. I think we all agree that having less 3rd dependencies is always better, so such discussions are quite helpful.

@MDr164
Copy link
Contributor

MDr164 commented Feb 29, 2024

Oh right, totally forgot that we switched to a subset of go-logr defined directly in this repo. In this case I agree and we can use the existing log/slog implementation of go-logr (which also satisfies the subset of course). Should I take care of switching this around or is someone already looking into it?

@kommendorkapten
Copy link
Member

yeah, please do that @MDr164 I don't think anyone is looking into this.

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 a pull request may close this issue.

4 participants