append some context to logs

[chuckha]

/kind feature

Describe the solution you'd like
I'd like to know if logs are coming from the machine actuator or the controller actuator. would be happy with [controller] or [machine] in front of the log.

Anything else you would like to add:
This is up for discussion, but it would be useful in the effort to make this easier to debug

Environment:

• Cluster-api-provider-aws version:
• Kubernetes version: (use kubectl version):
• OS (e.g. from /etc/os-release):

[randomvariable]

Makes sense to do with #305 too

[vincepri]

Do the current logs have enough context or should we add more? The way klog is structured is going to be difficult and repetitive to add more context to the current logs. We could adopt https://github.com/go-logr/logr and zap logger, which is already included in controller runtime, but that means to switch to json logging, which I'm not sure if that makes things better or worse.

[chuckha]

The context I was referring to was knowing if the logs were coming from the machine actuator or cluster actuator.

I'm not sure it's worth adding more context as I've been able to debug issues fine without it, I think it's a nice to have and would be fine moving this to the backlog if others are also happy with the logging.

[vincepri]

/priority backlog
/unassign

[vincepri]

/good-first-issue
/help-wanted

[usahai30]

If available .. I want to take up this one and get started :) Can I a take up this one? @chuckha

[chuckha]

yes, please @usahai30

/lifecycle active

join us in #cluster-api-aws on the k8s slack if you're not already if you have any questions!

[usahai30]

Thanks @chuckha
I have joined the slack channel too.

Please confirm my understanding for this issue -

1. In order to differ the logs coming from Machine Actuator or Cluster Actuator, we should print some context for same.
2. These logs are klogs and just appending context like "[MACHINE]" or "[MACHINE-ACTUATOR]" will do the job.
   For Eg -
   Current Logs -

klog.Infof("Setting machine %q nodeRef to %q", scope.Name(), nodeRef.Name)

To-Be -

klog.Infof("[MACHINE] Setting machine %q nodeRef to %q", scope.Name(), nodeRef.Name)

3. Also, we require the context only for actuators or should we do for other components also? controllers ?

[chuckha]

It would be nice if there was a way to set this up in the instantiation of the klog object instead of changing every logging instance

But yes, you're on the right path!

The actuators are the important part, but if you end up having to add context to the manager's logger, that would be fine too.

[usahai30]

Yeah sure, I agree with that ... Will look around what best I can come up with.

[dennisme]

Semi related klog issue / feature here: kubernetes/klog#22

[usahai30]

@chuckha I looked at the possibilities and my understandings are -

1. klog doesn't have the structure logging to carry the context with log messages.
2. However, I could see 'klogr' being introduced for structured based logging -
   https://github.com/kubernetes/klog/tree/master/klogr
   but this is still a BETA implementation and currently doesn't solve our requirement also as formatted printing Infof method is still not here.
3. Right now, I could see hardcoding log lines with context as the only possible solution here.

Please suggest, what should we do here? Should we wait for structured logging to be implemented first in klog?

[chuckha]

@usahai30 Thanks so much for digging into this. I think we'll want to tackle: #641 first over the course of a release and then make progress on this. So I'm going to remove good first issue and make 641 block this

[chuckha]

/remove-lifecycle active
/remove-help
/remove-good-first-issue

[chuckha]

/remove-priority longterm-important

[k8s-ci-robot]

@chuckha: Those labels are not set on the issue: priority/longterm-important

In response to this:

/remove-priority longterm-important

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[chuckha]

/remove-priority important-longterm