Proposal: Use klog as kubebuilder default logger
#3036
Comments
astraw99
added
the
kind/feature
|
Hi @astraw99, By looking at your example using klog: (proposed PR changes #3033 )
Am I right? Note that the logs done by controller-runtime by default (as it is today) adopt the same k8s format. Regards the time format not parse for humans it will be solved in the next controller-runtime release and when we bump it on Kubebuilder, see: kubernetes-sigs/controller-runtime#2029 |
We can see the log format (full-time, no file and line info) is not consistent with the klog format, as in the upper leaderelection line, the desired format should like this: |
|
Hi @astraw99, Thank you for the clarifications. I am OK with the change proposed https://github.com/kubernetes-sigs/kubebuilder/pull/3033/files. You are not the first one looking for klog but I think would be nice to get others' opinions as well. @jmrodri @varshaprasad96 @everettraven wdyt? Also, I am not sure why we use In the default scaffold, however, it seems that would be nice to check it out and see if we should not need to ensure the same behaviour with klog. |
|
@astraw99 for human readability you can adjust |
|
In my personal opinion klog is way less readable than zap 🙈. And zap is way more configurable. Usually I use zap with json format for logging and redirect klog to zap logger using klog.SetOutput + zapio.Writer/os.Discard, while exposing the klog level indirectly as env var (we dont use flags in my org). It's not ideal 😞.I understand that being consistent with other k8s components would be beneficial though. Also, anecdotally, in my org we spend way too much time discussing what should be logged at what level, and exposing only |
|
Currently in go ecosystem Full list - https://github.com/go-logr/logr#implementations-non-exhaustive Among the choices, IMO in scaffolding all places generic EDIT: I think for scaffolding, using |
@camilamacedo86 reading through some of the comments here I really like the idea proposed by @grzesuav where we potentially change all logging instances in the default scaffold to use a logging interface (as @grzesuav mentioned @astraw99 I think this approach would satisfy your needs by making the logging much more configurable as long as the logger that will be used satisfies the |
|
Hi @grzesuav and @everettraven,
WDYT about creating a PR with the suggestions so that we can have a better idea and discuss the proposed solution? |
|
/triage-accepted |
|
@grzesuav @camilamacedo86 @varshaprasad96 kubebuilder/pkg/plugins/golang/v3/scaffolds/internal/templates/main.go Lines 232 to 238 in 15bd05e We can see the In this proposal, we just want to use the |
|
looking at what you wrote
the log format should be machine readable, to be able to be parsed by logstash/kibana/etc In case you need to change it to human readable in dev env, just run you code with flag
Not sure why it is a pros, k8s komponent are in major refactoring to use structured logging, which isn't easy considering the codebase. Among logr compatibile loggers zap is more perfomant than klog
The same for zap - https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/log/zap#Options.BindFlags |
|
@varshaprasad96 @camilamacedo86 sure, will try to do it over the weekend |
|
@camilamacedo86 @varshaprasad96 after looking onto code it seems there is no much to extract as as @astraw99 noticed the only place which require change is logger initialization and import. However looking onto code it seems it can be parameterized (i.e. by default generate I can add it, the changes in the template will be (I put var mainTemplate = `{{ .Boilerplate }}
package main
import (
"flag"
"os"
// Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.)
// to ensure that exec-entrypoint and run can make use of them.
_ "k8s.io/client-go/plugin/pkg/client/auth"
"k8s.io/apimachinery/pkg/runtime"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/log/zap"
%s - NEW MARKER FOR LOGGER IMPORT
"sigs.k8s.io/controller-runtime/pkg/healthz"
%s
)
var (
scheme = runtime.NewScheme()
setupLog = ctrl.Log.WithName("setup")
)
func init() {
utilruntime.Must(clientgoscheme.AddToScheme(scheme))
%s
}
func main() {
{{- if not .ComponentConfig }}
var metricsAddr string
var enableLeaderElection bool
var probeAddr string
flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.")
flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")
flag.BoolVar(&enableLeaderElection, "leader-elect", false,
"Enable leader election for controller manager. " +
"Enabling this will ensure there is only one active controller manager.")
{{- else }}
var configFile string
flag.StringVar(&configFile, "config", "",
"The controller will load its initial configuration from this file. " +
"Omit this flag to use the default configuration values. " +
"Command-line flags override configuration from this file.")
{{- end }}
%s - NEW MARKER FOR LOGGER INIT
flag.Parse()
ctrl.SetLogger(%s - NEW MARKER FOR LOGGER SETTING)want to ask before going forward though |
|
alternative would be to use |
|
@grzesuav Thanks for your analysis! Thinking about this a bit more and seeing what @astraw99 commented I personally feel like this should be a no-op. With the default logging scaffold in the opts := zap.Options{
Development: true,
}
opts.BindFlags(flag.CommandLine)
flag.Parse()
ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts)))It seems like it would pretty trivial to manually convert this to: opts.BindFlags(flag.CommandLine)
flag.Parse()
ctrl.SetLogger(klog.New())Since the default scaffolding has been My suggestion would be that if it isn't made clear in the Kubebuilder documentation how to change the logger that we add a new section to the FAQ about how to do this. |
|
I agree with @everettraven +1000.Also:
So, (ihmo) I think we could close this one as not accept. PS.: However, if you folks see that community/users would prefer we scaffold klog by default instead of zap then, ihmo this change should be done only for go-v4/alpha. |
|
sure thing. IMO changing to Regarding maturity, as I mentioned earlier zap is faster , so changing to klog feels to me like going into wrong direction. |
|
OK, after discussion, we prefer to use |
What do you want to happen?
Using the latest kubebuilder cli, quick start a new project based on the official document.
After
make deploy IMG=xxx/xxx:tag, the controller-pod log time is just long numbers (1.6656692709539523e+09):Should make it human readable like:
Related issue: kubernetes-sigs/controller-runtime#2024
With the latest PR merged: kubernetes-sigs/controller-runtime#2029, tried using it got the log:
But the result log format (no file and line info) is not consistent with the klog format, as in the upper
leaderelectionline.So propose to use
klogto make it consistent with the K8s core components, like:Pros
klog.InitFlags(klogFlags)Cons
Extra Labels
No response
The text was updated successfully, but these errors were encountered: