⚠️ Fix broken kind logging #1954

tatsuhiro-t · 2022-07-13T01:30:31Z

If gvk.Kind is "APIService", I expect "apiService", but the current
code produces and logs "aPIService". Because it is not easy to do
this kind of thing, it is better to log the kind as is.

Fixes #1945

This is a breaking change. Previously, the log line included a reconciled resource (for example, APIService bar/foo) like:

"aPIService"={"name":"foo","namespace":"bar"}

Now it looks like:

"APIService"={"name":"foo","namespace":"bar"}

If gvk.Kind is "APIService", I expect "apiService", but the current code produces and logs "aPIService". Because it is not easy to do this kind of thing, it is better to log the kind as is.

k8s-ci-robot · 2022-07-13T01:30:40Z

Hi @tatsuhiro-t. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

FillZpp · 2022-07-13T02:59:18Z

pkg/builder/controller.go


 		ctrlOptions.LogConstructor = func(req *reconcile.Request) logr.Logger {
 			log := log
 			if req != nil {
 				log = log.WithValues(
-					lowerCamelCaseKind, klog.KRef(req.Namespace, req.Name),
+					kind, klog.KRef(req.Namespace, req.Name),


So why not just use gvk.Kind?

Fixed via bb36890

FillZpp · 2022-07-13T03:58:05Z

/ok-to-test

FillZpp · 2022-07-13T04:55:48Z

/label tide/merge-method-squash

/lgtm

FillZpp · 2022-07-13T04:56:46Z

/cc @alvaroaleman @sbueringer

alvaroaleman · 2022-07-13T14:07:23Z

pkg/builder/controller.go

 		ctrlOptions.LogConstructor = func(req *reconcile.Request) logr.Logger {
 			log := log
 			if req != nil {
 				log = log.WithValues(
-					lowerCamelCaseKind, klog.KRef(req.Namespace, req.Name),
+					gvk.Kind, klog.KRef(req.Namespace, req.Name),


This is a breaking change for anyone who filters their logs, please mark it as such. Please also describe how this now looks like rather than only how it does not look like anymore.

Changed the icon to ⚠️ to indicate this is a breaking change, and edited the PR description to include how it looks now.

I see your point that aPIService is definitely bad. On the other side this change is directly in conflict with the conventions used by k/k: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#name-arguments

Specifically:

Always use lowerCamelCase, for example use containerName and not container name or container_name.

I wonder if there is any alternative instead of using kind directly

Looking at the example of good keys I'm not sure if APIService should be logged as APIService or apiService (especially the CIDR and PVC examples). Maybe we can ask the logging folks in the #klog Slack channel.

If we assume the first, it could be good enough to just keep the current case when the second character is also uppercase. (I'm aware it's not a great solution, but I assume it's a good heuristic)

My thoughts:

Things like PVC is just the short name of PersistentVolumeClaim, which is different with the real kind APIService.

IMHO, changing the first case to lower is a little weird. Kubernetes resources have kind like PersistentVolumeClaim and resource(plural) like persistentvolumeclaims. But things like persistentVolumeClaim are only used as golang variable names for they should have first case lower, why should we keep this format in log?

My first impression of the linked documentation is it does not take into account the CRD names which can be arbitrarily chosen by third parties and is hard to convert to camelCase in some cases.

Alternatively, we may follow the statement under exception lists:

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.

so use object as a key to log name and namespace, and log kind as is under kind key.

"object"={"name":"foo","namespace":"bar"} "kind"="APIService"

I have no idea why they picked those conventions, but I agree it's definitely hard to do if we can't just hard-code the key.

Given that, I think i also prefer just using kind directly.

The problem with using object is that it makes it very very hard to correlate log entries via a log query across controllers.
E.g. the following example:

1st controller reconciles Clusters: k/v pairs: "Cluster": <custer>

2nd controller reconciles Machines: k/v pairs: "Machine": <machine>, "Cluster": <cluster>

...

By using kind it's trivial to match logs across controllers. If the reconciled object has the "object" key it requires to calculate the actual kind before correlation between logs of different controllers are possible.

+1 to use kind without transformations, it is simpler, less verbose than object and it read more naturally

Right, so if I understand it correctly, we have a consensus on what we have now in this PR, that is log kind as a key without transformation.

sbueringer · 2022-07-15T05:56:03Z

/lgtm

k8s-ci-robot · 2022-07-15T14:16:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, tatsuhiro-t

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~~ [alvaroaleman]

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

Fix broken kind logging

a02d631

If gvk.Kind is "APIService", I expect "apiService", but the current code produces and logs "aPIService". Because it is not easy to do this kind of thing, it is better to log the kind as is.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 13, 2022

k8s-ci-robot requested review from pwittrock and shawn-hurley July 13, 2022 01:30

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 13, 2022

tatsuhiro-t mentioned this pull request Jul 13, 2022

Broken resource kind logging #1945

Closed

FillZpp reviewed Jul 13, 2022

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 13, 2022

fixup! Fix broken kind logging

bb36890

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 13, 2022

k8s-ci-robot assigned FillZpp Jul 13, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 13, 2022

k8s-ci-robot requested review from alvaroaleman and sbueringer July 13, 2022 04:56

alvaroaleman reviewed Jul 13, 2022

View reviewed changes

tatsuhiro-t changed the title ~~🐛 Fix broken kind logging~~ ⚠️ Fix broken kind logging Jul 13, 2022

k8s-ci-robot assigned sbueringer Jul 15, 2022

sbueringer mentioned this pull request Jul 15, 2022

🌱 Improve key value pairs consistency in logging kubernetes-sigs/cluster-api#6150

Merged

6 tasks

alvaroaleman approved these changes Jul 15, 2022

View reviewed changes

k8s-ci-robot assigned alvaroaleman Jul 15, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2022

k8s-ci-robot merged commit b93b5f9 into kubernetes-sigs:master Jul 15, 2022

k8s-ci-robot added this to the v0.10.x milestone Jul 15, 2022

This was referenced Aug 17, 2022

✨ Improve key value pairs consistency in logging (II) kubernetes-sigs/cluster-api#7075

Merged

🌱 Improve logging for the MachineSet scale up/down workflow kubernetes-sigs/cluster-api#7026

Merged

sbueringer mentioned this pull request Jan 13, 2023

⚠️ fix redundant values in log entries #2074

Closed

sbueringer mentioned this pull request Sep 18, 2023

Discussion: Conventions for log keys kubernetes-sigs/cluster-api#9447

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ Fix broken kind logging #1954

⚠️ Fix broken kind logging #1954

tatsuhiro-t commented Jul 13, 2022 •

edited

Loading

k8s-ci-robot commented Jul 13, 2022

FillZpp Jul 13, 2022

tatsuhiro-t Jul 13, 2022

FillZpp commented Jul 13, 2022

FillZpp commented Jul 13, 2022

FillZpp commented Jul 13, 2022

alvaroaleman Jul 13, 2022

tatsuhiro-t Jul 14, 2022

sbueringer Jul 14, 2022 •

edited

Loading

sbueringer Jul 14, 2022 •

edited

Loading

FillZpp Jul 14, 2022

tatsuhiro-t Jul 14, 2022 •

edited

Loading

sbueringer Jul 14, 2022 •

edited

Loading

fabriziopandini Jul 14, 2022

tatsuhiro-t Jul 14, 2022

sbueringer commented Jul 15, 2022

k8s-ci-robot commented Jul 15, 2022

⚠️ Fix broken kind logging #1954

⚠️ Fix broken kind logging #1954

Conversation

tatsuhiro-t commented Jul 13, 2022 • edited Loading

k8s-ci-robot commented Jul 13, 2022

FillZpp Jul 13, 2022

Choose a reason for hiding this comment

tatsuhiro-t Jul 13, 2022

Choose a reason for hiding this comment

FillZpp commented Jul 13, 2022

FillZpp commented Jul 13, 2022

FillZpp commented Jul 13, 2022

alvaroaleman Jul 13, 2022

Choose a reason for hiding this comment

tatsuhiro-t Jul 14, 2022

Choose a reason for hiding this comment

sbueringer Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

sbueringer Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

FillZpp Jul 14, 2022

Choose a reason for hiding this comment

tatsuhiro-t Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

sbueringer Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

fabriziopandini Jul 14, 2022

Choose a reason for hiding this comment

tatsuhiro-t Jul 14, 2022

Choose a reason for hiding this comment

sbueringer commented Jul 15, 2022

k8s-ci-robot commented Jul 15, 2022

tatsuhiro-t commented Jul 13, 2022 •

edited

Loading

sbueringer Jul 14, 2022 •

edited

Loading

sbueringer Jul 14, 2022 •

edited

Loading

tatsuhiro-t Jul 14, 2022 •

edited

Loading

sbueringer Jul 14, 2022 •

edited

Loading