Skip to content

⚠️ Fix broken kind logging #1954

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 15, 2022
Merged

⚠️ Fix broken kind logging #1954

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a02d631
Fix broken kind logging
Jul 13, 2022
bb36890
fixup! Fix broken kind logging
Jul 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions pkg/builder/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,13 +315,11 @@ func (blder *Builder) doController(r reconcile.Reconciler) error {
"controllerKind", gvk.Kind,
)

lowerCamelCaseKind := strings.ToLower(gvk.Kind[:1]) + gvk.Kind[1:]

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),
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@sbueringer sbueringer Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@sbueringer sbueringer Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

My thoughts:

  1. Things like PVC is just the short name of PersistentVolumeClaim, which is different with the real kind APIService.
  2. 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?

Copy link
Contributor Author

@tatsuhiro-t tatsuhiro-t Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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"

Copy link
Member

@sbueringer sbueringer Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

"namespace", req.Namespace, "name", req.Name,
)
}
Expand Down