Implement Logging Guidelines

[danehans]

#182 defined logging conventions and guidelines for the project. These guidelines should be implemented throughout the project. We should also consider tooling to ensure PRs follow logging guidelines.

[tchap]

I don't mind working on this. It seems to me it would be good to refactor the code a bit as well to get rid of all the Fatal calls and just return errors. I can just replace the calls, but honestly, it's not that great and also a bit ugly. What do you think?

[tchap]

Rewrote error handling not to use Fatal in the executable code: tchap@2d0108b . Let me know whether this is acceptable, I just played around this optimistically and I don't want to intrude, so I can as well just discard it, but it will have to be done, eventually.

[danehans]

I'm good with ErrorS instead of Fatal. Update the logging guidelines if needed. Since we do t have CI setup yet, can you run the e2e test locally and post a snip of your results?

[tchap]

/assign

[tchap]

Also let me know how far you want to push this. Reading up on the current Kubernetes standards, the trend is to move to logr.Logger and contextual logging, i.e. basically drop using klog interface altogether and remove global logging calls as in klog.Info. So these are actually two separate issues:

• Use contextual logging.
• Remove klog.

I think that contextual logging is pretty nice, it allows you to attach additional information to log entries. It obviously comes with some extra cost like replacing loggers in ctx unless you are passing it around as an arg, but overall it is a clear and simple approach. You can do contextual logging without dropping klog as there are compatible calls, but then there is really no reason to keep klog.

[danehans]

Reading up on the current Kubernetes standards

@tchap can you please provide a link(s) to these current standards? I think we should include this link(s) within our logging docs.

the trend is to move to logr.Logger and contextual logging, i.e. basically drop using klog interface

I'm in favor of moving to logr.Logger with contextual logging if this follows the k8s guidelines. Let's get input from @ahg-g @liu-cong @kfswain @robscott before proceeding.

[tchap]

can you please provide a link(s) to these current standards? I think we should include this link(s) within our logging docs.

Sure, it's right in the doc that is linked from dev.md.

[tchap]

To be clear, the doc is 2 years old, but I haven't found anything more up to date. It's in sync with what kubebuilder generates when starting a new controller project. But keep me in check, I am pretty new to this stuff.

It seems you can use both klog and logr, but if there is any substantial work being done on logging, I would vote for moving away from using global package loggers like klog.* calls. Not a great pattern. Just instantiate your logger, pass it around as an arg or in the context. It is also set "globally" in controller-runtime. It just seems way cleaner to me.

[tchap]

I think that this is implemented now, except perhaps the tooling to check incoming PRs.

When implementing this issue, I used https://pkg.go.dev/k8s.io/klog/hack/tools/logcheck . Not sure it can be used for PRs, but just linking in here just in case.

[kfswain]

I agree. I'm happy to consider this closed, I'll let @danehans make the final call however