Controllers should use consistent logging about crossplane HOT 12 CLOSED

I'm of two minds. klog in my opinion embraces and encourages poor logging practices, but is the path of least resistance. It's well understood within the Kubernetes community, and some libraries (that should not be concerned with logging in the first place) straight up assume you're using it as demonstrated by the tweet @ichekrygin linked above.

If we were to deviate from klog my preference would be to use Zap and establish and document some conventions around its use. I have opinions about what said conventions should be, but won't go into them at this stage as I'm conscious that this is a highly bike-sheddable topic.

from crossplane.

i too remember some annoying ways that glog infiltrated the command line parsing of the Rook project. I am OK with not using klog either since it's based on that and it sounds like there is a lot of agreement here that it's not a great idea.

Sounds like perhaps we should move forward with Zap as @venuchitta originally suggested, but not implement anything for log rotation. @ichekrygin let me know if you have any issues with that, otherwise hopefully you'll get a chance to tackle this one soon @venuchitta. Let me know if you have any questions during the implementation phase! :)

from crossplane.

I can work on this feature. I think we can implement the way Istio(one of the famous open source project) implemented their logging library. We can also use Zap which is logging library developed by Uber combined with log rotation library Lumberjack as mentioned here. Zap claims that their library is fast and use memory efficiently. What do you guys think?

from crossplane.

What’s the rationale behind supporting log rotation and file handling in binary? That seems to go against typical “12 factor”/ container best practices.

from crossplane.

Thanks for the recommendations @venuchitta and @negz. I feel a bit differently about log rotation now than when I originally opened this issue. I think that log rotation shouldn't be addressed within the application itself, but instead be managed by the cluster execution environment. This is in alignment with what @negz was saying for 12-factor apps and in the Kubernetes docs.

A twelve-factor app never concerns itself with routing or storage of its output stream. It should not attempt to write to or manage logfiles. Instead, each running process writes its event stream, unbuffered, to stdout.

So I think we should not address log rotation within Crossplane.

Reading up on k8s logging conventions, I see that klog is now recommended in the community guidelines. What do you think about incorporating klog into Crossplane? It'll probably be good to follow the upstream Kubernetes practices. Thoughts?

from crossplane.

my $0.02 - klog is a fork of glog, and I was never a big fan of glog
Just a sample: https://twitter.com/jbeda/status/994408277415940096

controller-runtime uses:

• github.com/go-logr/logr
• github.com/go-logr/zapr

Check out logr/README.md, very good points from both Dave and Tim.

from crossplane.

Thanks for this discussion @jbw976 @negz @ichekrygin. After going through Dave's blog, logr's implementation, controller-runtime's use of logr/zapr and @negz's comments. I like the way logr is structured but with some complaints which I will address in MR. I will work in next couple days and will update this space. Thanks.

from crossplane.

Cool, that sounds good to me to use logr since it's a minimalized interface and can just use Zap underneath via zapr. The logr programmatic interface remains consistently used throughout the application and it's a top level main.go decision to use Zap or not.

from crossplane.

Are we confident that using logr as opposed to plain zap brings us value relative to its added complexity? My recollection was that logr was closer to a proof of concept - an experimental logging interface. It doesn't seem like it's particularly widely adopted.

My understanding is that a lot of the thought around a standardised Go logging interface related more to libraries. The idea being you could provide a library that logs without being opinionated about what logging implementation the library's caller uses. I'm not sure how relevant this is to a binary distribution like Crossplane. How likely are we to want to change the logging implementation (i.e zap) without changing the logging interface (i.e. logr)?

from crossplane.

Hi @negz I believe we are using logr because we want to follow Dave's post of logging guidelines and this interface helps us achieve that.
I do agree that its not either widely adopted or easy to use. But in the future if we want to use other logging library, logr is easily enough to adapt to that.
I am not sure about binary distribution part you are mentioning.
If we further decided not to use logr, we can work on having some other intuitive interface than that.

from crossplane.

@venuchitta just messaged me on Slack:

Hey negz, I been busy with one of the projects at my company. Can u pls take over #7. I dont want to keep you guys waiting. I will reach you out after I am free

This is still a good candidate for a community member to tackle. If no one is excited to do so I can take a pass.

from crossplane.

I'm going to reopen this to track the fact that the GCP bucket controller PR landed before I merged #354, so we need to switch controller that over to the new logging pattern.

from crossplane.