✨ Ensure logging is uniform #237

DirectXMan12 · 2018-12-05T19:27:38Z

This ensures that logging uses a uniform set of tags and "style" for the tags. It also adds a helper that prevents the entire object from being logged when logging kubernetes objects. Instead, only a few key identifier fields (name, namespace, api version, and kind, when relevant) are logged when in production (the full object is still logged in development).

k8s-ci-robot · 2018-12-05T19:27:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12

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:

~~OWNERS~~ [DirectXMan12]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

This commit makes sure we're specific on what packages we "require" in Gopkg.toml. Erroneously requiring the root package in the apiextensions-apiserver, was causing us to pull in main, and transitively a whole bunch of deps that we didn't actually need.

DirectXMan12 · 2018-12-05T19:30:30Z

Please take a look at the logging guidelines, and then I'll move them to the kb book as well.

DirectXMan12 · 2018-12-05T19:31:11Z

cc @droot

droot

Took a quick look, looks good to me overall.

droot · 2018-12-05T20:03:21Z

TMP-LOGGING.md

+a Kubernetes object", "pod", somePod)`.  controller-runtime provides
+a special encoder for Zap that will transform Kubernetes objects into
+`name, namespace, apiVersion, kind` objects, when available and not in
+development mode.  Other logr implementations should implement similar


This is sweet.

droot · 2018-12-05T20:05:36Z

TMP-LOGGING.md

+logic.
+
+## Logging Tags
+


It will be useful to highlight what Tags are and how they impact the log message and what is their utility ?

replaced the word "tag" with "key-value pairs" to make things more obvious.

and expanded on things a bit.

Looks good.

This adds a "wrapper" to a Zap encoder designed to nicely log Kubernetes objects when in non-development mode. When in development mode, we still log the entire object.

This overhauls log tags in CR to ensure that we have consistent style and naming across all of our log lines.

This updates the dep list with the new zap/buffer dependency.

mengqiy

Looks great overall.
I have left a few minor comments.
And I have found that there are one pair of backquote embeded in another pair of backquote, which mess up the format a bit. e.g.

`foo `embeded text` bar`

It will be helpful to use GH preview to ensure a pretty markdown format.

mengqiy · 2018-12-05T23:28:19Z

TMP-LOGGING.md

+```
+
+While it's possible to use higher log levels, it's reccomended that you
+stick with `V(1)` or V(0)` (which is equivalent to not specifying `V`),


V(0)`

nit: a missing "`"

mengqiy · 2018-12-05T23:38:08Z

TMP-LOGGING.md

+  that. Never use `fmt.Sprintf` in your message.
+
+- Try to match the terminology in your messages with your key-value pairs
+  -- for instance, if you have a key-value pairs `api version`, use the


I can't follow this example.

Is api version the key in the K-V pair?

And it does using APIVersion conflicts with a later section saying

Use lower-case, space separated keys. For example `object` for objects, `api version` for `APIVersion`

mengqiy · 2018-12-05T23:41:42Z

TMP-LOGGING.md

+  accept a generic interface.
+
+- When logging a raw type, log it using the `type` key, with a value of
+  `fmt.Sprintf("%T", typ)`


mengqiy · 2018-12-05T23:45:54Z

TMP-LOGGING.md

+- If there's specific context around a type, the key may be more specific,
+  but should end with `type` -- for instance, `OwnerType` should be logged
+  as `owner` in the context of `log.Error(err, "Could not get ObjectKinds
+  for OwnerType", `owner type`, fmt.Sprintf("%T"))`.  When possible, favor


fmt.Sprintf("%T")

Need an object here.

droot

Logging guidelines looks pretty good now.

droot · 2018-12-06T14:35:20Z

TMP-LOGGING.md

+logic.
+
+## Logging Tags
+


Looks good.

droot · 2018-12-06T14:37:49Z

/lgtm

Will wait @mengqiy to take a look and approve since he had a few comments to be addressed.

mengqiy · 2018-12-06T18:10:03Z

It seems the bot reacted too fast 😆
I'm OK to let Solly address the minor comments in a followup PR.

…ging-audit ✨ Ensure logging is uniform

Add pkg/imports for linking dependencies from projects

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 5, 2018

k8s-ci-robot requested a review from droot December 5, 2018 19:27

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 5, 2018

DirectXMan12 force-pushed the refactor/logging-audit branch from f1ecd04 to c774e2f Compare December 5, 2018 19:27

DirectXMan12 force-pushed the refactor/logging-audit branch from edc15ac to aa88474 Compare December 5, 2018 19:29

droot reviewed Dec 5, 2018

View reviewed changes

DirectXMan12 added 3 commits December 5, 2018 14:39

Add helper for nicely logging Kubernetes objects

37142fa

This adds a "wrapper" to a Zap encoder designed to nicely log Kubernetes objects when in non-development mode. When in development mode, we still log the entire object.

Overhaul/unify log tags

07d1335

This overhauls log tags in CR to ensure that we have consistent style and naming across all of our log lines.

Update dep list

027a76d

This updates the dep list with the new zap/buffer dependency.

DirectXMan12 force-pushed the refactor/logging-audit branch from aa88474 to 027a76d Compare December 5, 2018 22:39

mengqiy reviewed Dec 5, 2018

View reviewed changes

droot reviewed Dec 6, 2018

View reviewed changes

TMP-LOGGING.md

logic.

## Logging Tags

Copy link

Contributor

droot Dec 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

k8s-ci-robot assigned droot Dec 6, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 6, 2018

droot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 6, 2018

k8s-ci-robot merged commit 43351af into kubernetes-sigs:master Dec 6, 2018

justinsb pushed a commit to justinsb/controller-runtime that referenced this pull request Dec 7, 2018

Merge pull request kubernetes-sigs#237 from DirectXMan12/refactor/log…

380e34d

…ging-audit ✨ Ensure logging is uniform

DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020

Merge pull request #237 from pwittrock/imports

3824cee

Add pkg/imports for linking dependencies from projects

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Ensure logging is uniform #237

✨ Ensure logging is uniform #237

DirectXMan12 commented Dec 5, 2018

k8s-ci-robot commented Dec 5, 2018

DirectXMan12 commented Dec 5, 2018

DirectXMan12 commented Dec 5, 2018

droot left a comment

droot Dec 5, 2018

droot Dec 5, 2018

DirectXMan12 Dec 5, 2018

DirectXMan12 Dec 5, 2018

droot Dec 6, 2018

mengqiy left a comment

mengqiy Dec 5, 2018

mengqiy Dec 5, 2018

mengqiy Dec 5, 2018

mengqiy Dec 5, 2018

droot left a comment

droot Dec 6, 2018

droot commented Dec 6, 2018

mengqiy commented Dec 6, 2018

✨ Ensure logging is uniform #237

✨ Ensure logging is uniform #237

Conversation

DirectXMan12 commented Dec 5, 2018

k8s-ci-robot commented Dec 5, 2018

DirectXMan12 commented Dec 5, 2018

DirectXMan12 commented Dec 5, 2018

droot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mengqiy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droot commented Dec 6, 2018

mengqiy commented Dec 6, 2018