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

Add zap logging wrapper plugin #6500

Closed
wants to merge 3 commits into from
Closed

Conversation

alxmk
Copy link

@alxmk alxmk commented Dec 20, 2023

Why the changes in this PR are needed?

OPA SDK currently uses logrus as its standard logger, but in the project I am currently working on we use zap. I produced a wrapper as part of that work, I thought it would be useful to make this available to other SDK consumers.

See #6499 for additional context.

What are the changes in this PR?

Add a new module under logging/plugins/ozap. This module contains a single package with a wrapper to allow use of a *zap.Logger as logging.Logger. Intended use is e.g.:

opa, err := sdk.New(context.Background(), sdk.Options{
	Logger: ozap.Wrap(logger, level),
})

Notes to assist PR review:

It was suggested to adopt the pattern used by franz-go for its pluggable loggers. This involves adding these plugins as separate modules to the main project.

Further comments:

I initially placed the module under plugins/logging/ozap but felt that was more confusing since this is a separate concept to the OPA plugins, I prefer the current package structure but happy to consider alternatives.

@srenatus
Copy link
Contributor

Thanks for looking into improving our logging situation. 🎈 🥳

What I'm uneasy about is that we now not only have a logrus dependency, but also a zap dependency. A recent project that nicely decoupled loggers from their code code is franz-go. If you scroll down to plugins, you'll see one adapter module per logging framework that can be imported as needed. Only those people that import it will have the dependency in their modules.

❓ What do you think -- Are dependencies not a problematic concern from your point of view? Or would you be interested in exploring this further, helping OPA get to a place where SDK users can plug in their own logging stuff without extra deps for everyone else?

@alxmk
Copy link
Author

alxmk commented Dec 20, 2023

Thanks for the quick feedback @srenatus!

I had not considered dependency pollution, the decoupling in the project you cite looks nice and elegant. Let me reimplement this as a plugin quickly and I'll update the draft.

Obviously happy to add unit tests etc once you're happy with the broad shape.

@srenatus
Copy link
Contributor

The franz-go approach obviously needs extra modules. @ashutosh-narkar what do you think, should we put them into contrib?

Signed-off-by: Alexander Mckenzie-Kelly <alexmk@uk.ibm.com>

open-policy-agent#6499
@alxmk alxmk marked this pull request as ready for review December 20, 2023 15:44
@alxmk alxmk changed the title Add proposal for zap logging wrapper Add zap logging wrapper plugin Dec 20, 2023
Add a unit test
Add godoc comments
Add READMEs

Signed-off-by: Alexander Mckenzie-Kelly <alexmk@uk.ibm.com>
No need to be ahead of the min version for opa

Signed-off-by: Alexander Mckenzie-Kelly <alexmk@uk.ibm.com>
@ashutosh-narkar
Copy link
Member

Thanks @alxmk for working on this. I agree with @srenatus about adding an external dependency. Unless we really need to it would not be ideal. Adding this to contrib seems like a good idea.

@srenatus
Copy link
Contributor

@ashutosh-narkar the way this is done now will not add an extra dep to OPA's. It's an extra go module that people can load intentionally.

@srenatus
Copy link
Contributor

@alxmk I had a chat with @ashutosh-narkar OOB, and we've come to agree that it would -- although there are no extra deps for the github.com/open-policy-agent/opa module -- still be preferable to put this into github.com/open-policy-agent/contrib. The main reason is user expectations -- we wouldn't want to end up maintaining N logger integrations, but we'd have to eventually, and users would expect it from us if the wrapper plugin was part of this repo. With the contrib repo, the maintenance expectations aren't as high, so it's easier for us to accept contributions that could come with some ongoing maintenance needs.

Hope this makes sense, and I'm sorry for the extra work involved. I very much appreciate your contribution, even more so since it sets an example for future contributions! ✨

@alxmk
Copy link
Author

alxmk commented Dec 21, 2023

@srenatus no worries, thanks for the explanation. I will close this out and re-raise in contrib.

@alxmk
Copy link
Author

alxmk commented Dec 21, 2023

open-policy-agent/contrib#240

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 this pull request may close these issues.

None yet

3 participants