output: handle WithName like zapr does #391

pohly · 2023-10-23T19:10:23Z

What this PR does / why we need it:

Previously, all names set via WithNames were concatenated with / and added to the main message string as a <names>: <msg> prefix. Now klog in all of its incarnations (internal logger in klog, klogr, textlogger) behaves like zapr and adds a "logger" key with the names concatenated by a dot as value.

The main advantage, besides consistency, is that code which does a full string match against some message string still works even when contextual logging is enabled and WithName is used, at least as long as that code doesn't also match all key/value pairs.

Which issue(s) this PR fixes:
Related-to: promoting kubernetes/enhancements#3077 to beta

Special notes for your reviewer:

https://github.com/go-logr/logr/blob/ebabbb9f842ece8e071e7bd5f3aad24ef2c5caa0/slogr/slogsink.go#L44-L45 also uses this key. @thockin prefers that approach.

Release note:

contextual logging: WithName now handles the names like zapr ("logger" as key, names concatenated by a dot as value).

/cc @mengjiao-liu

mengjiao-liu

WithName builds a special key-value pair, with logger as the special key and name as the value, which avoids the need to modify the msg string each time.

Same concern: go-logr/logr#196 (comment)
But maybe this can be done in groups in the future.

/lgtm

mengjiao-liu · 2023-10-24T07:24:33Z

/triage accepted

pohly · 2023-10-24T08:43:39Z

/assign @dims

For approval and a second opinion whether this change makes sense. We could also ask Tim, but he's busy and I already know that he prefers the separate logger key.

harshanarayana · 2023-10-26T11:13:20Z

klogr.go

+	// havePrefix is true if the first entry in values is the special
+	// nameKey key/value. Such an entry gets added and later updated in
+	// WithName.
+	havePrefix bool


Would it be better to name this as hasPrefix ?

Depends: I was thinking of it as "we have a prefix", but "logger has a prefix" also makes sense.

"has" seems to more popular in k/k/, although both occur - I'll switch to "has".

harshanarayana · 2023-10-26T11:15:46Z

klogr.go

+		prefix = prefix + "." + name
+		v[1] = prefix


Suggested change

prefix = prefix + "." + name

v[1] = prefix

v[1] = prefix + "." + name

Can we do it like this to avoid one more override of the prefix ?

harshanarayana · 2023-10-26T11:19:24Z

klogr/klogr_test.go

@@ -41,7 +41,7 @@ func testOutput(t *testing.T, format string) {
 			klogr:         new().V(0),
 			text:          "test",
 			keysAndValues: []interface{}{"akey", "avalue"},
-			expectedOutput: ` "msg"="test" "akey"="avalue"
+			expectedOutput: `"msg"="test" "akey"="avalue"


I like that this empty space at the start is going away.

Yes. That space was the result of some overly simplistic code for injecting the prefix, which is now gone.

harshanarayana · 2023-10-26T11:24:15Z

klogr/klogr.go

+		v = append(v, nameKey, name)
+		v = append(v, l.values...)


Suggested change

v = append(v, nameKey, name)

v = append(v, l.values...)

v = append(v, nameKey, name, l.values...)

I might be nitpicking, but Would the above one make it more easier ?

It would, but Go doesn't support this: one can either append multiple values or provide those values through a slice, but not both.

Ah ! Sorry, never mind. my thought about reviewing something on python slipped into this thread as well. You are right. Thanks for clarifying.

pohly · 2023-10-26T11:40:54Z

@harshanarayana: updated, please take another look.

harshanarayana · 2023-10-26T12:13:14Z

/lgtm
/cc @dims

dims · 2023-10-26T14:27:21Z

/approve

Previously, all names set via `WithNames` were concatenated with / and added to the main message string as a `<names>: <msg>` prefix. Now klog in all of its incarnations (internal logger in klog, klogr, textlogger) behaves like zapr and adds a "logger" key with the names concatenated by a dot as value. The main advantage, besides consistency, is that code which does a full string match against some message string still works even when contextual logging is enabled and `WithName` is used, at least as long as that code doesn't also match all key/value pairs.

pohly · 2023-10-26T16:26:02Z

@harshanarayana: I had to rebase, please lgtm again.

harshanarayana · 2023-10-26T16:52:56Z

/lgtm

harshanarayana · 2023-10-26T16:53:01Z

@pohly Done.

k8s-ci-robot · 2023-10-26T16:53:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, harshanarayana, mengjiao-liu, pohly

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

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

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [k8s.io/klog/v2](https://togithub.com/kubernetes/klog) | require | minor | `v2.100.1` -> `v2.110.1` | --- ### Release Notes <details> <summary>kubernetes/klog (k8s.io/klog/v2)</summary> ### [`v2.110.1`](https://togithub.com/kubernetes/klog/releases/tag/v2.110.1): Prepare klog release for Kubernetes v1.29 (Take 1) [Compare Source](https://togithub.com/kubernetes/klog/compare/v2.100.1...v2.110.1) #### What's Changed - fix: SetLogger via klog.SetLogger will output an unexpected newline by [@aimuz](https://togithub.com/aimuz) in [https://github.com/kubernetes/klog/pull/378](https://togithub.com/kubernetes/klog/pull/378) - resolve comments warning by [@lowang-bh](https://togithub.com/lowang-bh) in [https://github.com/kubernetes/klog/pull/379](https://togithub.com/kubernetes/klog/pull/379) - stderrthreshold: fix flag comment by [@pohly](https://togithub.com/pohly) in [https://github.com/kubernetes/klog/pull/376](https://togithub.com/kubernetes/klog/pull/376) - enable "go vet" checks for parameters by [@pohly](https://togithub.com/pohly) in [https://github.com/kubernetes/klog/pull/390](https://togithub.com/kubernetes/klog/pull/390) - promote experimental code to stable by [@pohly](https://togithub.com/pohly) in [https://github.com/kubernetes/klog/pull/392](https://togithub.com/kubernetes/klog/pull/392) - golangci-lint action by [@pohly](https://togithub.com/pohly) in [https://github.com/kubernetes/klog/pull/380](https://togithub.com/kubernetes/klog/pull/380) - output: handle WithName like zapr does by [@pohly](https://togithub.com/pohly) in [https://github.com/kubernetes/klog/pull/391](https://togithub.com/kubernetes/klog/pull/391) - slog support + logr 1.3.0 update by [@pohly](https://togithub.com/pohly) in [https://github.com/kubernetes/klog/pull/384](https://togithub.com/kubernetes/klog/pull/384) #### New Contributors - [@aimuz](https://togithub.com/aimuz) made their first contribution in [https://github.com/kubernetes/klog/pull/378](https://togithub.com/kubernetes/klog/pull/378) - [@lowang-bh](https://togithub.com/lowang-bh) made their first contribution in [https://github.com/kubernetes/klog/pull/379](https://togithub.com/kubernetes/klog/pull/379) **Full Changelog**: kubernetes/klog@v2.100.1...v2.110.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/hetznercloud/hcloud-cloud-controller-manager).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

k8s-ci-robot requested a review from mengjiao-liu October 23, 2023 19:10

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 23, 2023

mengjiao-liu reviewed Oct 24, 2023

View reviewed changes

k8s-ci-robot assigned mengjiao-liu Oct 24, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2023

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 24, 2023

k8s-ci-robot assigned dims Oct 24, 2023

harshanarayana reviewed Oct 26, 2023

View reviewed changes

pohly force-pushed the with-name-output branch from 0cd7e41 to f947f41 Compare October 26, 2023 11:40

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2023

k8s-ci-robot requested a review from dims October 26, 2023 12:13

k8s-ci-robot assigned harshanarayana Oct 26, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2023

pohly force-pushed the with-name-output branch from f947f41 to 009a04a Compare October 26, 2023 16:25

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2023

k8s-ci-robot merged commit 02e7b69 into kubernetes:main Oct 26, 2023
15 checks passed

platform-engineering-bot mentioned this pull request Dec 14, 2023

Update go updates arcalot/arcaflow-engine-deployer-kubernetes#31

Merged

1 task

zxh326 mentioned this pull request Nov 8, 2024

chore: update deps juicedata/juicefs-csi-driver#1168

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

output: handle WithName like zapr does #391

output: handle WithName like zapr does #391

pohly commented Oct 23, 2023 •

edited

Loading

mengjiao-liu left a comment •

edited

Loading

mengjiao-liu commented Oct 24, 2023

pohly commented Oct 24, 2023

harshanarayana Oct 26, 2023

pohly Oct 26, 2023

harshanarayana Oct 26, 2023

pohly Oct 26, 2023

harshanarayana Oct 26, 2023

pohly Oct 26, 2023

harshanarayana Oct 26, 2023

pohly Oct 26, 2023

harshanarayana Oct 26, 2023

pohly commented Oct 26, 2023

harshanarayana commented Oct 26, 2023

dims commented Oct 26, 2023

pohly commented Oct 26, 2023

harshanarayana commented Oct 26, 2023

harshanarayana commented Oct 26, 2023

k8s-ci-robot commented Oct 26, 2023

	prefix = prefix + "." + name
	v[1] = prefix
	v[1] = prefix + "." + name

	v = append(v, nameKey, name)
	v = append(v, l.values...)
	v = append(v, nameKey, name, l.values...)

output: handle WithName like zapr does #391

output: handle WithName like zapr does #391

Conversation

pohly commented Oct 23, 2023 • edited Loading

mengjiao-liu left a comment • edited Loading

Choose a reason for hiding this comment

mengjiao-liu commented Oct 24, 2023

pohly commented Oct 24, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly commented Oct 26, 2023

harshanarayana commented Oct 26, 2023

dims commented Oct 26, 2023

pohly commented Oct 26, 2023

harshanarayana commented Oct 26, 2023

harshanarayana commented Oct 26, 2023

k8s-ci-robot commented Oct 26, 2023

pohly commented Oct 23, 2023 •

edited

Loading

mengjiao-liu left a comment •

edited

Loading