📖 Update logging guidelines #13011
📖 Update logging guidelines #13011
Conversation
k8s-ci-robot
added
area/documentation
k8s-ci-robot
added
the
size/S
|
Looks good. There are some contextual logging efforts ongoing that might define more best practices, but this is good. /lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: b982c588715762f1f5df429ad9c997b0c09b1b82
|
AndiDog
left a comment
AndiDog
left a comment
There was a problem hiding this comment.
LGTM, with comments/questions
| Notably, over time in CAPI we are also standardizing usage of other key value pairs to improve consistency when reading | ||
| logs, e.g. | ||
| - key `reason` MUST be used when adding details about WHY a change happened. | ||
| - key `diff` MUST be used when documenting the diff in an object that either lead to a chance, or that is resulting from a change. |
There was a problem hiding this comment.
This would be very helpful. Can we specify a suggested format? For example, subfields old/new, or string format (less helpful)?
There was a problem hiding this comment.
I think it's extremely hard to standardize as we have a lot of different cases where we have different data that we can use
(check the usages of the "diff" key today in CAPI)
| the message MUST contain the Kind of the controlled/referenced object and its name, for example `Created MachineSet foo-bar` | ||
| - If the controlled/referenced object is in another namespace, use namespace/name instead of name |
There was a problem hiding this comment.
Sure to repeat the object name, which should already be a structured field? I'm not against this repetition to make it more readable for humans – just asking if this was considered.
The namespace should be in the structured field automatically (via klog.KRef). If we want the human log message to only contain the namespace if it's outside the "default", then I guess this means a utility function which does that?
There was a problem hiding this comment.
Sure to repeat the object name, which should already be a structured field? I'm not against this repetition to make it more readable for humans – just asking if this was considered.
Yes, I think we should repeat it. My experience is that omitting it makes it unnecessarily hard to figure out what's going on, e.g.
Created Machine
k,v MachineSet namespace/name Machine namespace/name + a ton of other k/v pairs
Created Machine machine-1
With tools like Grafana/Kibana etc. it makes sense in my opinion to only show specific parts of the log, E.g. to only show controller-name + msg. If all the k/v pairs have to be shown for the log to make sense the view becomes extremely verbose
If we want the human log message to only contain the namespace if it's outside the "default", then I guess this means a utility function which does that?
I think we don't need a util for that. I'm not aware of a single case of this in core Cluster API. We just use %s and e.g. machine.Name. I think folks can just decide to either do that or use klog.KObj / klog.KRef as appropriate
| Notably, over time in CAPI we are also standardizing usage of other key value pairs to improve consistency when reading | ||
| logs, e.g. | ||
| - key `reason` MUST be used when adding details about WHY a change happened. | ||
| - key `diff` MUST be used when documenting the diff in an object that either lead to a chance, or that is resulting from a change. |
There was a problem hiding this comment.
I think it's extremely hard to standardize as we have a lot of different cases where we have different data that we can use
(check the usages of the "diff" key today in CAPI)
k8s-ci-robot
removed
the
lgtm
fabriziopandini
added
the
tide/merge-method-squash
|
/lgtm /hold |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 2936dfcd8f90673c0396a98cac2dd5f1cd94c58b
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
5 participants
What this PR does / why we need it:
Documenting a few practices that we are adopting in latest review/PRs
/area documentation