Discussion: Conventions for log keys

[sbueringer]

This issue is to discuss conventions for log keys (brought up in kubernetes-sigs/cluster-api-provider-vsphere#2352 (comment)).

Current state in Cluster API:

1. When log keys are kinds we use the kind directly, e.g. Machine, Cluster, MachineDeployment.
2. In a lot of other cases we use lower case: count, revision, timeout, ...

In my opinion in a lot of cases for 2. we should ask ourselves if these even should be k/v pairs. For me it doesn't make sense to just put all data in a k/v pair. We should always think about if the k/v pair is actually useful. E.g. does it make sense to filter logs based on a timeout or on a count or does it make the log more readable. In my opinion this sort of data should be simply part of the message. But maybe I'm missing something.

Independent of that. For kinds we have to use the kind directly as we don't always know the kind upfront and then there is no good way to automatically calculate the right lowerCamelCase format of it. Please also see this PR in controller-runtime: kubernetes-sigs/controller-runtime#1954

Now the question becomes, do we want to use CamelCase for all log keys consistently or only for kinds?

As far as I'm aware there is no upstream guidance on it:

• https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/1602-structured-logging
  • " For now we don't want to tackle they keys and preserve freedom for caller to choose what's best"
• https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/logging.md

[sbueringer]

/triage accepted

[sbueringer]

cc @fabriziopandini @killianmuldoon @chrischdi @zhanggbj @randomvariable

[sbueringer]

cc @pohly (just in case you have a take on it / I missed some upstream Kubernetes guidance, thank you!)

[killianmuldoon]

I don't really mind the mix of camelCase and lower case - there's a good justification for it and I haven't found it distracting or confusing when reading logs.

I do think the more important point is whether certain data should be in k/v pairs - we should only really include data in k/v if it's useful for filtering or extracting from the logs. Maybe we should add some language to our logging conventions doc around that and put some effort into reviewing with that in mind in future.

[chrischdi]

Ok with the above 👍

A timeout in sense of duration may not make sense (IMHO) as k/v pair. A counter may be worth having as k/v pair, so I'd say in that cases "it depends" :-) .

[pohly]

The migration guide describes best practices for keys in Kubernetes. Unfortunately it's mostly just by example, without the corresponding rationale.

[chrischdi]

So where we differ from that best practices is:

• Always use lowerCamelCase, for example use containerName and not container name or container_name.
  ...
• Use object kind when referencing Kubernetes objects, for example deployment, pod and node.

[fabriziopandini]

In my opinion in a lot of cases for 2. we should ask ourselves if these even should be k/v pairs. For me it doesn't make sense to just put all data in a k/v pair. We should always think about if the k/v pair is actually useful. E.g. does it make sense to filter logs based on a timeout or on a count or does it make the log more readable. In my opinion this sort of data should be simply part of the message.

+1, as a general rule
e.g. For some messages in KCP remediation, we are adding many k/v to help in understanding what is going on; in this case it won't be possible to add everything to the message (k/v pairs is used as a sort of info bag)

So where we differ from that best practices is: Always use lowerCamelCase...

I don't remember why we ended up with upper case Kind, but If this is the best practice we can consider to converge (but low priority)

[sbueringer]

I don't remember why we ended up with upper case Kind, but If this is the best practice we can consider to converge (but low priority)

For kinds we have to use the kind directly as we don't always know the kind upfront and then there is no good way to automatically calculate the right lowerCamelCase format of it. Please also see this PR in controller-runtime: kubernetes-sigs/controller-runtime#1954

1. controller-runtime cannot calculate lowerCamelCase correctly, but CR adds the k/v pair of the reconciled objects, e.g. MachineDeployment, MachineSet, Cluster. I initially just lower-cased the first character, but this does not work for kinds like APIServer => aPIService. So we had to use the kind directly in controller-runtime
2. We have similar cases in Cluster API where the controller itself gets a kind dynamically instead of just a hard-coded key (e.g.

   cluster-api/bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go

   Line 174 in 252ff99

   log = log.WithValues(configOwner.GetKind(), klog.KRef(configOwner.GetNamespace(), configOwner.GetName()), "resourceVersion", configOwner.GetResourceVersion())

   )

So in my opinion we can only use kind directly as a key in Cluster API. Otherwise we will end up with inconsistent kind keys in our logs.

I think this basically applies to all cases where the kind key cannot be hard-coded via a constant string (@pohly just in case that is also relevant for upstream Kubernetes)

[fabriziopandini]

/kind feature
/priority backlog