Migrate apimachinery to contextual logging #115317

yanjing1104 · 2023-01-25T17:06:20Z

Signed-off-by: Jing Yan jing1.yan@intel.com

What type of PR is this?

/kind feature

What this PR does / why we need it:

Migrated apimachinery to use contextual logging.

Which issue(s) this PR fixes:

Fixes # Part of kubernetes/enhancements#3077

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/3077-contextual-logging

k8s-ci-robot · 2023-01-25T17:06:29Z

Hi @yanjing1104. Thanks for your PR.

I'm waiting for a kubernetes 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.

k8s-triage-robot · 2023-01-25T17:54:50Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

cici37 · 2023-01-26T17:43:13Z

/wg structured-logging

yanjing1104 · 2023-01-30T06:57:39Z

/assign @pohly

pohly · 2023-01-30T13:18:11Z

staging/src/k8s.io/apimachinery/pkg/api/meta/meta.go

@@ -600,15 +600,15 @@ func (a genericAccessor) GetOwnerReferences() []metav1.OwnerReference {
 	var ret []metav1.OwnerReference
 	s := a.ownerReferences
 	if s.Kind() != reflect.Pointer || s.Elem().Kind() != reflect.Slice {
-		klog.Errorf("expect %v to be a pointer to slice", s)
+		klog.ErrorS(nil, "expect the ownerReference to be a pointer to slice", "ownerReference", s)


There's one thing we need to decide here and in many other pieces of this PR: can this call here use contextual logging?

Probably not, because changing GetOwnerReference() to GetOwnerReference(logger klog.Logger) just because it might need to log in an error path is overkill.

But we also don't want to be left with a klog.ErrorS call that then will show up when we search for non-contextual calls that might have to be updated.

So here we can use:

Suggested change

klog.ErrorS(nil, "expect the ownerReference to be a pointer to slice", "ownerReference", s)

klog.Background().Error(nil, "expect the ownerReference to be a pointer to slice", "ownerReference", s)

pohly · 2023-01-30T13:21:10Z

staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/connection.go

@@ -169,7 +169,7 @@ func (c *connection) newSpdyStream(stream *spdystream.Stream) {
 	err := c.newStreamHandler(stream, replySent)
 	rejectStream := (err != nil)
 	if rejectStream {
-		klog.Warningf("Stream rejected: %v", err)
+		klog.InfoS("Stream rejected", "err", err)


Here it might make more sense to change the API. This will have implications for code, wo most likely we'll need an alternative API that accepts a logger will keeping the old one around, perhaps with a deprecation notice.

The comment is outdated, changed it to contextual logging.

- klog.Warningf("Stream rejected: %v", err) + klog.Background().Info("Stream rejected", "err", err)

klog.Background().Info is not contextual logging because it is always using the global logger, regardless of how it got called.

You need to think about ways how a caller can inject its own logger into this function, i.e. change or extend the API.

cici37 · 2023-01-31T21:04:34Z

/remove-sig api-machinery

cici37 · 2023-03-16T16:36:43Z

/triage accepted

k8s-triage-robot · 2023-06-14T16:47:50Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

pohly · 2023-06-15T06:33:18Z

/remove-lifecycle stale

k8s-ci-robot · 2023-06-29T12:33:45Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yanjing1104
Once this PR has been reviewed and has the lgtm label, please assign deads2k for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

Signed-off-by: Jing Yan <jing1.yan@intel.com>

yanjing1104 · 2023-06-29T12:38:56Z

@pohly
Hi Patrick thanks for helping with "/remove-lifecycle stale" label.
this pr has been updated, could you help to review it when available?

yanjing1104 · 2023-06-29T12:45:40Z

/assign @deads2k

pohly · 2023-07-03T14:30:06Z

/unassign @deads2k

k8s-ci-robot · 2023-10-30T00:36:15Z

PR needs rebase.

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.

yanjing1104 · 2023-12-21T10:47:27Z

Close the PR as I wont be able to follow it, thanks~

k8s-ci-robot requested review from caesarxuchao and derekwaynecarr January 25, 2023 17:07

k8s-ci-robot added the wg/structured-logging Categorizes an issue or PR as relevant to WG Structured Logging. label Jan 26, 2023

k8s-ci-robot assigned pohly Jan 30, 2023

pohly suggested changes Jan 30, 2023

View reviewed changes

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jan 31, 2023

yanjing1104 force-pushed the apimachinery_logging branch from 323ba3e to 067ccf5 Compare March 14, 2023 21:47

yanjing1104 changed the title ~~Migrate apimachinery to structured logging~~ Migrate apimachinery to contextual logging Mar 14, 2023

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Mar 14, 2023

yanjing1104 force-pushed the apimachinery_logging branch from 067ccf5 to a19031b Compare March 14, 2023 22:18

yanjing1104 requested review from pohly and removed request for caesarxuchao and derekwaynecarr March 16, 2023 13:53

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 Mar 16, 2023

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 14, 2023

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 15, 2023

yanjing1104 force-pushed the apimachinery_logging branch from a19031b to 15953f6 Compare June 29, 2023 12:33

Convert apimachinery feature to contextual logging

15953f6

Signed-off-by: Jing Yan <jing1.yan@intel.com>

k8s-ci-robot assigned deads2k Jun 29, 2023

k8s-ci-robot unassigned deads2k Jul 3, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2023

yanjing1104 closed this Dec 21, 2023

yanjing1104 mentioned this pull request Dec 21, 2023

contextual logging kubernetes/enhancements#3077

Open

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate apimachinery to contextual logging #115317

Migrate apimachinery to contextual logging #115317

yanjing1104 commented Jan 25, 2023 •

edited

k8s-ci-robot commented Jan 25, 2023

k8s-triage-robot commented Jan 25, 2023

cici37 commented Jan 26, 2023

yanjing1104 commented Jan 30, 2023

pohly Jan 30, 2023

yanjing1104 Jun 29, 2023

pohly Jan 30, 2023

yanjing1104 Jun 29, 2023

pohly Jul 3, 2023

cici37 commented Jan 31, 2023

cici37 commented Mar 16, 2023

k8s-triage-robot commented Jun 14, 2023

pohly commented Jun 15, 2023

k8s-ci-robot commented Jun 29, 2023

yanjing1104 commented Jun 29, 2023

yanjing1104 commented Jun 29, 2023

pohly commented Jul 3, 2023

k8s-ci-robot commented Oct 30, 2023

yanjing1104 commented Dec 21, 2023

	klog.ErrorS(nil, "expect the ownerReference to be a pointer to slice", "ownerReference", s)
	klog.Background().Error(nil, "expect the ownerReference to be a pointer to slice", "ownerReference", s)

Migrate apimachinery to contextual logging #115317

Migrate apimachinery to contextual logging #115317

Conversation

yanjing1104 commented Jan 25, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Jan 25, 2023

k8s-triage-robot commented Jan 25, 2023

cici37 commented Jan 26, 2023

yanjing1104 commented Jan 30, 2023

pohly Jan 30, 2023

Choose a reason for hiding this comment

yanjing1104 Jun 29, 2023

Choose a reason for hiding this comment

pohly Jan 30, 2023

Choose a reason for hiding this comment

yanjing1104 Jun 29, 2023

Choose a reason for hiding this comment

pohly Jul 3, 2023

Choose a reason for hiding this comment

cici37 commented Jan 31, 2023

cici37 commented Mar 16, 2023

k8s-triage-robot commented Jun 14, 2023

pohly commented Jun 15, 2023

k8s-ci-robot commented Jun 29, 2023

yanjing1104 commented Jun 29, 2023

yanjing1104 commented Jun 29, 2023

pohly commented Jul 3, 2023

k8s-ci-robot commented Oct 30, 2023

yanjing1104 commented Dec 21, 2023

yanjing1104 commented Jan 25, 2023 •

edited