zitadel / oidc

Easy to use OpenID Connect client and server library written for Go and certified by the OpenID Foundation
https://zitadel.com
Apache License 2.0
1.4k stars 147 forks source link

proposal: improve logging #218

Closed muir closed 1 year ago

muir commented 2 years ago

Is your feature request related to a problem? Please describe. Implementing op and rp integrations I had lots of bugs. I only found those bugs by adding 100s of prints to the code.

Describe the solution you'd like After xop is released (should be in a couple weeks or less), I would like to add logging everywhere. This will require changing many function signatures because either a *xop.Log or a context.Context would need to be passed to every function that wants to log and many functions do not currently have a context parameter.

Xop is the right choice because:

Describe alternatives you've considered

Xop is not the right choice because:

Additional context

muhlemmer commented 1 year ago

We can already look into adding context where required, while the exact way of implementing logging is being investigated. #309 is meant to split the change of context and logging.

muhlemmer commented 1 year ago

The Go ecosystem is full of different loggers. Some better than others. Developers often have different opinions on this subject. I feel that it is not the place of a library to make a fixed decision on a log implementation.

Yes, xop looks like a good choice for the above reasons, but our zitadel product already build on logrus for example. There might be reasons to change that, but that could take some time after any decision is taken. Meanwhile, we (urgently) need to improve the situation around logging.

Inspired by jackc/pgx, which offers a general logging interface and adapters towards the major logging libraries that are available, I've looked around to the possibility if we can provide something similar.

https://github.com/go-logr/logr seems to be a very valid option to achieve a similar solution:

By using logr we give power to the package users to make their own decisions on a logger. Also, I shouldn't be much work to create an adapter for xop.

muhlemmer commented 1 year ago

CC @zitadel/engineers, @fforootd

muhlemmer commented 1 year ago

logr proposal was accepted during internal meeting. This issue is replaced by user story #379.