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 out-of-the-box zap logger #6499

Closed
alxmk opened this issue Dec 20, 2023 · 3 comments · Fixed by open-policy-agent/contrib#240
Closed

Add out-of-the-box zap logger #6499

alxmk opened this issue Dec 20, 2023 · 3 comments · Fixed by open-policy-agent/contrib#240

Comments

@alxmk
Copy link

alxmk commented Dec 20, 2023

What is the underlying problem you're trying to solve?

Embedding OPA as an SDK in a service currently means either:

  • You get lucky and you use logrus
  • You have both logrus and whatever log implementation your service uses
  • You have to implement logging.Logger for your log implementation

This is all surmountable but it may be a nice quality of life improvement to have common log implementation wrappers in the core OPA library.

Describe the ideal solution

Ideally I could just do something like:

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

and get on with my day.

Describe a "Good Enough" solution

Unfortunately the SetLevel function means you also need to pass a zap.AtomicLevel:

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

A better solution would be very welcome.

Additional Context

I have a proposed implementation which I will pull request shortly.

@srenatus
Copy link
Contributor

💭 Now what "log/slog" has appeared, would that provide a no-dependencies common ground for multiple logging implementations to plug in? Honest question, I haven't played with it much yet.

alxmk added a commit to alxmk/opa that referenced this issue Dec 20, 2023
alxmk added a commit to alxmk/opa that referenced this issue Dec 20, 2023
Signed-off-by: Alexander Mckenzie-Kelly <alexmk@uk.ibm.com>

open-policy-agent#6499
alxmk added a commit to alxmk/opa that referenced this issue Dec 20, 2023
Add a unit test
Add godoc comments
Add READMEs

Signed-off-by: Alexander Mckenzie-Kelly <alexmk@uk.ibm.com>
open-policy-agent#6499
alxmk added a commit to alxmk/opa that referenced this issue Dec 20, 2023
No need to be ahead of the min version for opa

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

open-policy-agent#6499
@alxmk
Copy link
Author

alxmk commented Dec 21, 2023

log/slog seems like it could be a good integration point (similar to you I haven't had much hands on time with it yet). I think it would certainly be worth looking at that once the module minimum Go version is up to 1.21.

@srenatus
Copy link
Contributor

Ah, minimal go version. Yeah, thanks for checking that.

alxmk added a commit to alxmk/contrib that referenced this issue Dec 21, 2023
open-policy-agent/opa#6499
Signed-off-by: Alexander Mckenzie-Kelly <alexmk@uk.ibm.com>
srenatus pushed a commit to open-policy-agent/contrib that referenced this issue Dec 22, 2023
open-policy-agent/opa#6499

Signed-off-by: Alexander Mckenzie-Kelly <alexmk@uk.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants