Update instruction for naming arguments (#5537)

* Update instruction for naming arguments

* Apply suggestions from code review

Co-authored-by: Elana Hashman <[email protected]>

* Update contributors/devel/sig-instrumentation/migration-to-structured-logging.md

Co-authored-by: Elana Hashman <[email protected]>

* Update contributors/devel/sig-instrumentation/migration-to-structured-logging.md

Co-authored-by: Elana Hashman <[email protected]>

Co-authored-by: Marek Siarkowicz <[email protected]>
Co-authored-by: Elana Hashman <[email protected]>

Author: serathius

contributors/devel/sig-instrumentation/migration-to-structured-logging.md

@@ -201,6 +201,12 @@ has different meaning for variadic arguments. Instead of just passing arguments,
 argument name and argument value. This means when migrating a log call we need to add an additional string before each
 argument, that will be used as it's name.
 
+How variable arguments should be used:
+
+```go
+klog.InfoS("message", "key1", value1, "key2", "value2")
+```
+
 For example
 ```go
 func LogHTTP(r *http.Request) {
@@ -214,37 +220,85 @@ func LogHTTP(r *http.Request) {
 }
 ```
 
-Names of arguments should use [lowerCamelCase] and be alphanumeric. Arguments names in one log call should be unique.
-Names should be picked based on semantic meaning of value itself, not the context in which is used (log message should
-imply the context). For example names like `status` should be used over (`desiredStatus`, `oldStatus`, `badStatus`) thus
-allowing to query and join different log lines of the `status` field.
-
-Kubernetes objects should be referenced using only their kind, no matter their api group or version. Example argument
-names: `deployment`, `pod`, `node`, `replicaSet`. For objects of unknown type, is ok to log them under `object` key with
-addition of `apiVersion` and `kind` fields describing the k8s object type.
-
-In situations when we want to the log value of the same meaning twice (e.g. transition between state) it is ok to name
-an additional argument based on context, but leaving one most current/correct value with canonical name.
-
-Examples of keys (strongly suggested, will be extended when pattern emerge, no standard schema yet):
-* `err` - error when using `klog.InfoS`. Used for expected errors that are not `klog.ErrorS`.
-* `object` - reference to k8s objects of unknown type. Should be used with `kind` and `apiVersion`.
-* `kind` - kind of k8s object of unknown type.
-* `apiVersion` -  API version of k8s object of unknown type.
-
-Example:
+When deciding on names of arguments you should:
+* Always use [lowerCamelCase], for example use `containerName` and not `container name` or `container_name`.
+* Use [alphanumeric] characters: no special characters like `%$*`, non-latin, or unicode characters.
+* Use object kind when referencing Kubernetes objects, for example `deployment`, `pod` and `node`.
+* Describe the type of value stored under the key and use normalized labels:
+  * Don't include implementation-specific details in the labels. Don't use `directory`, do use `path`.
+  * Do not provide additional context for how value is used. Don't use `podIP`, do use `IP`.
+  * With the exception of acronyms like "IP" and the standard "err", don't shorten names. Don't use `addr`, do use `address`.
+  * When names are very ambiguous, try to include context in the label. For example, instead of
+    `key` use `cacheKey` or instead of `version` use `dockerVersion`.
+* Be consistent, for example when logging file path we should always use `path` and not switch between
+  `hostPath`, `path`, `file`.
+
+Here are a few exceptions to the rules above---some cases are temporary workarounds that may change if we settle on better solution:
+* Do use `err` rather than `error` to match the key used by `klog.ErrorS`
+* Context in name is acceptable to distinguish between values that normally go under same key. For example using both
+  `status` and `oldStatus` in log that needs to show the change between statuses.
+* When Kubernetes object kind is unknown without runtime checking we should use `object` key. To provide information
+  about kind we should add separate `apiVersion` and `kind` fields.
+* If we cannot use `klog.KObj` nor `klog.KRef` for Kubernetes object, like in cases when we only have access to name or UID,
+  then we should fallback to using object kind with suffix based on value type. For example `podName`, `podUID`.
+* When providing multiple indistinguishable values (for example list of evicted pods), then we can use plural version of
+  argument name. For example we should use `pods` and not `podList`.
+
+Examples of **good keys** (strongly suggested, will be extended when pattern emerge, no standard schema yet):
+* `cacheKey`
+* `cacheValue`
+* `CIDR`
+* `containerID`
+* `containerName`
+* `controller`
+* `cronJob`
+* `deployment`
+* `dockerVersion`
+* `duration`
+* `err`
+* `job`
+* `object`
+* `pod`
+* `podName`
+* `podUID`
+* `PVC`
+* `PV`
+* `volumeName`
+* `replicaSet`
+
+Examples of **bad** keys:
+* `addr` - replace with `address`
+* `container` - replace with `containerName` or `containerID` depending on value
+* `currentNode` - replace with `node`
+* `directory` - replace with `path`
+* `elapsed` - replace with `duration`
+* `externalIP` - replace  with `IP`
+* `file` - replace with `path`
+* `hostPath` - replace with `path`
+* `ip` - replace with `IP`
+* `key` -  replace with key describing what kind of key it is, for example `cacheKey`
+* `loadBalancerIP` - replace with `IP`
+* `podFullName` - try to rewrite code so that pod name or pod object can be used with `pod` or `podName` keys
+* `podIP` - replace with `IP`
+* `podList` - replace with `pods`
+* `version` - replace with key describing what it belongs to so that it can be compared, for example `dockerVersion`
+* `servicePortName` - replace with `portName`
+* `svc` - replace with `service`
+
+Example of using context in to distinguish between two same keys:
 
 ```go
-func ChangeStatus(newStatus, currentStatus string) {
-  err := changeStatus(newStatus)
-  if err != nil {
-    klog.ErrorS(err, "Failed changing status", "desiredStatus", newStatus, "status", currentStatus)
-  }
-  klog.InfoS("Changed status", "previousStatus", currentStatus, "status", newStatus)
+func ChangePodStatus(newStatus, currentStatus string) {
+  klog.InfoS("PodStatusController found pod with status", "status", currentStatus)
+  ...
+  // Logic that changes status
+  ...
+  klog.InfoS("PodStatusController changed pod status", "oldStatus", currentStatus, "status", newStatus)
 }
 ```
 
 [lowerCamelCase]: https://en.wiktionary.org/wiki/lowerCamelCase
+[alphanumeric]: https://en.wikipedia.org/wiki/Alphanumeric
 
 ### Use `klog.KObj` and `klog.KRef` for Kubernetes objects