Migrate client-go/metadata to contextual logging #122225

ricardoapl · 2023-12-07T19:43:04Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Migrate client-go/metadata to contextual logging

Which issue(s) this PR fixes:

Part of kubernetes/enhancements#3077

Special notes for your reviewer:

Log output before migration

I1202 17:13:16.944886 1320000 metadata.go:194] Unable to retrieve PartialObjectMetadata: &runtime.notRegisteredErr{schemeName:"k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/register.go:27", gvk:schema.GroupVersionKind{Group:"", Version:"v1", Kind:"Pod"}, target:runtime.GroupVersioner(nil), t:reflect.Type(nil)}
I1202 17:25:19.610064 1472026 metadata.go:230] Unable to retrieve PartialObjectMetadataList: &runtime.notRegisteredErr{schemeName:"k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/register.go:27", gvk:schema.GroupVersionKind{Group:"", Version:"v1", Kind:"PodList"}, target:runtime.GroupVersioner(nil), t:reflect.Type(nil)}

Log output after migration

I1202 17:10:46.625898 1286529 metadata.go:194] "Could not retrieve PartialObjectMetadata" err="no kind \"Pod\" is registered for version \"v1\" in scheme \"k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/register.go:27\""
I1202 17:27:14.864645 1496151 metadata.go:230] "Could not retrieve PartialObjectMetadataList" err="no kind \"PodList\" is registered for version \"v1\" in scheme \"k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/register.go:27\""

Does this PR introduce a user-facing change?

Migrate client-go/metadata to contextual logging

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

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

/wg structured-logging
/area logging
/priority important-longterm
/cc @kubernetes/wg-structured-logging-reviews

Signed-off-by: Ricardo Lopes <ricardoapl.dev@gmail.com>

k8s-ci-robot · 2023-12-07T19:43:07Z

@ricardoapl: GitHub didn't allow me to request PR reviews from the following users: kubernetes/wg-structured-logging-reviews.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Migrate client-go/metadata to contextual logging

Which issue(s) this PR fixes:

Part of kubernetes/enhancements#3077

Special notes for your reviewer:

Log output before migration

I1202 17:13:16.944886 1320000 metadata.go:194] Unable to retrieve PartialObjectMetadata: &runtime.notRegisteredErr{schemeName:"k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/register.go:27", gvk:schema.GroupVersionKind{Group:"", Version:"v1", Kind:"Pod"}, target:runtime.GroupVersioner(nil), t:reflect.Type(nil)}
I1202 17:25:19.610064 1472026 metadata.go:230] Unable to retrieve PartialObjectMetadataList: &runtime.notRegisteredErr{schemeName:"k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/register.go:27", gvk:schema.GroupVersionKind{Group:"", Version:"v1", Kind:"PodList"}, target:runtime.GroupVersioner(nil), t:reflect.Type(nil)}

Log output after migration

I1202 17:10:46.625898 1286529 metadata.go:194] "Could not retrieve PartialObjectMetadata" err="no kind \"Pod\" is registered for version \"v1\" in scheme \"k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/register.go:27\""
I1202 17:27:14.864645 1496151 metadata.go:230] "Could not retrieve PartialObjectMetadataList" err="no kind \"PodList\" is registered for version \"v1\" in scheme \"k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/register.go:27\""

Does this PR introduce a user-facing change?

Migrate client-go/metadata to contextual logging

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

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

/wg structured-logging
/area logging
/priority important-longterm
/cc @kubernetes/wg-structured-logging-reviews

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-ci-robot · 2023-12-07T19:43:09Z

Please note that we're already in Test Freeze for the release-1.29 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.29.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Thu Dec 7 16:15:00 UTC 2023.

k8s-ci-robot · 2023-12-07T19:43:13Z

Welcome @ricardoapl!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-12-07T19:43:15Z

Hi @ricardoapl. 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.

mengjiao-liu · 2023-12-08T07:13:04Z

/triage accepted

mengjiao-liu · 2023-12-08T07:31:27Z

staging/src/k8s.io/client-go/metadata/metadata.go

@@ -191,7 +191,7 @@ func (c *client) Get(ctx context.Context, name string, opts metav1.GetOptions, s
 	}
 	obj, err := result.Get()
 	if runtime.IsNotRegisteredError(err) {
-		klog.V(5).Infof("Unable to retrieve PartialObjectMetadata: %#v", err)
+		klog.FromContext(ctx).V(5).Info("Could not retrieve PartialObjectMetadata", "err", err)


The unit test(staging/src/k8s.io/client-go/metadata/metadata_test.go) corresponding to the Get method can use _, ctx := ktesting.NewTestContext(t) to pass context to test the contextual logging.

Thank you, should be resolved with 04e5522 and fb20f8f

Please take another look

make test WHAT=./staging/src/k8s.io/client-go/metadata/ GOFLAGS="-v" +++ [1208 10:21:40] Set GOMAXPROCS automatically to 12 +++ [1208 10:21:40] Running tests without code coverage and with -race === RUN TestClient === RUN TestClient/GET_is_able_to_convert_a_JSON_object_to_PartialObjectMetadata metadata.go:194: I1208 10:05:11.876937] Could not retrieve PartialObjectMetadata err="no kind \"Pod\" is registered for version \"v1\" in scheme \"k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/register.go:27\"" === RUN TestClient/LIST_is_able_to_convert_a_JSON_object_to_PartialObjectMetadata metadata.go:230: I1208 10:05:11.878102] Could not retrieve PartialObjectMetadataList err="no kind \"PodList\" is registered for version \"v1\" in scheme \"k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/register.go:27\"" === RUN TestClient/GET_fails_if_the_object_is_JSON_and_has_no_kind === RUN TestClient/GET_fails_if_the_object_is_JSON_and_has_no_apiVersion === RUN TestClient/GET_fails_if_the_object_is_JSON_and_not_clearly_metadata metadata.go:194: I1208 10:05:11.880156] Could not retrieve PartialObjectMetadata err="no kind \"Pod\" is registered for version \"v1\" in scheme \"k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme/register.go:27\"" === RUN TestClient/Delete_fails_if_DeleteOptions_cannot_be_serialized_to_JSON === RUN TestClient/DeleteCollection_fails_if_DeleteOptions_cannot_be_serialized_to_JSON --- PASS: TestClient (0.01s) --- PASS: TestClient/GET_is_able_to_convert_a_JSON_object_to_PartialObjectMetadata (0.01s) --- PASS: TestClient/LIST_is_able_to_convert_a_JSON_object_to_PartialObjectMetadata (0.00s) --- PASS: TestClient/GET_fails_if_the_object_is_JSON_and_has_no_kind (0.00s) --- PASS: TestClient/GET_fails_if_the_object_is_JSON_and_has_no_apiVersion (0.00s) --- PASS: TestClient/GET_fails_if_the_object_is_JSON_and_not_clearly_metadata (0.00s) --- PASS: TestClient/Delete_fails_if_DeleteOptions_cannot_be_serialized_to_JSON (0.00s) --- PASS: TestClient/DeleteCollection_fails_if_DeleteOptions_cannot_be_serialized_to_JSON (0.00s) PASS ok k8s.io/kubernetes/staging/src/k8s.io/client-go/metadata (cached)

mengjiao-liu · 2023-12-08T07:31:39Z

staging/src/k8s.io/client-go/metadata/metadata.go

@@ -227,7 +227,7 @@ func (c *client) List(ctx context.Context, opts metav1.ListOptions) (*metav1.Par
 	}
 	obj, err := result.Get()
 	if runtime.IsNotRegisteredError(err) {
-		klog.V(5).Infof("Unable to retrieve PartialObjectMetadataList: %#v", err)
+		klog.FromContext(ctx).V(5).Info("Could not retrieve PartialObjectMetadataList", "err", err)


same above.

Should also be resolved with 04e5522 and fb20f8f

Signed-off-by: Ricardo Lopes <ricardoapl.dev@gmail.com>

pohly · 2023-12-08T17:38:12Z

staging/src/k8s.io/client-go/metadata/metadata_test.go

@@ -55,7 +56,7 @@ func TestClient(t *testing.T) {
 	testCases := []struct {
 		name    string
 		handler func(t *testing.T, w http.ResponseWriter, req *http.Request)
-		want    func(t *testing.T, client *Client)
+		want    func(t *testing.T, ctx context.Context, client *Client)


Typically ctx is the first parameter in a function. I prefer to follow that also here and below.

Typically ctx is the first parameter in a function.

Indeed. I wasn't sure about it, because I found a few parts of the codebase doing it the other way around.

Should be resolved with 372ecf8

Please take another look

Thank you

I agree, it's a bit subjective. I got annoyed by the separate parameters and proposed a solution here (still to be implemented, comments welcome): #122148 (comment)

Signed-off-by: Ricardo Lopes <ricardoapl.dev@gmail.com>

mengjiao-liu · 2023-12-11T09:16:02Z

/ok-to-test

mengjiao-liu · 2023-12-12T03:12:12Z

/lgtm
in my side

Wait for confirmation from the hack & client-go reviewer
/assign @sttts

k8s-ci-robot · 2023-12-12T03:12:21Z

LGTM label has been added.

Git tree hash: a763fed9a91a11250086033eeb84cf592da4e225

mengjiao-liu · 2023-12-12T03:13:45Z

/label tide/merge-method-squash

sttts · 2023-12-12T07:53:07Z

/approve

k8s-ci-robot · 2023-12-12T07:53:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ricardoapl, sttts

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:

~~hack/OWNERS~~ [sttts]
~~staging/src/k8s.io/client-go/OWNERS~~ [sttts]

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

* client-go: migrate metadata to contextual logging Signed-off-by: Ricardo Lopes <ricardoapl.dev@gmail.com> * client-go: test for metadata contextual logs Signed-off-by: Ricardo Lopes <ricardoapl.dev@gmail.com> * refactor: extract context for table driven testing Signed-off-by: Ricardo Lopes <ricardoapl.dev@gmail.com> * refactor: pass context as first parameter Signed-off-by: Ricardo Lopes <ricardoapl.dev@gmail.com> --------- Signed-off-by: Ricardo Lopes <ricardoapl.dev@gmail.com>

client-go: migrate metadata to contextual logging

8a9ee4b

Signed-off-by: Ricardo Lopes <ricardoapl.dev@gmail.com>

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 7, 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 Dec 8, 2023

mengjiao-liu reviewed Dec 8, 2023

View reviewed changes

client-go: test for metadata contextual logs

04e5522

Signed-off-by: Ricardo Lopes <ricardoapl.dev@gmail.com>

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 8, 2023

refactor: extract context for table driven testing

fb20f8f

Signed-off-by: Ricardo Lopes <ricardoapl.dev@gmail.com>

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 8, 2023

pohly reviewed Dec 8, 2023

View reviewed changes

refactor: pass context as first parameter

372ecf8

Signed-off-by: Ricardo Lopes <ricardoapl.dev@gmail.com>

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 Dec 11, 2023

k8s-ci-robot assigned sttts and mengjiao-liu Dec 12, 2023

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

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

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

k8s-ci-robot merged commit 86ab185 into kubernetes:master Dec 14, 2023
14 checks passed

k8s-ci-robot added this to the v1.30 milestone Dec 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate client-go/metadata to contextual logging #122225

Migrate client-go/metadata to contextual logging #122225

ricardoapl commented Dec 7, 2023

k8s-ci-robot commented Dec 7, 2023

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 Dec 7, 2023

k8s-ci-robot commented Dec 7, 2023

k8s-ci-robot commented Dec 7, 2023

mengjiao-liu commented Dec 8, 2023

mengjiao-liu Dec 8, 2023

ricardoapl Dec 8, 2023 •

edited

mengjiao-liu Dec 8, 2023

ricardoapl Dec 8, 2023 •

edited

pohly Dec 8, 2023

ricardoapl Dec 8, 2023

pohly Dec 12, 2023

mengjiao-liu commented Dec 11, 2023

mengjiao-liu commented Dec 12, 2023

k8s-ci-robot commented Dec 12, 2023

mengjiao-liu commented Dec 12, 2023

sttts commented Dec 12, 2023

k8s-ci-robot commented Dec 12, 2023

Migrate client-go/metadata to contextual logging #122225

Migrate client-go/metadata to contextual logging #122225

Conversation

ricardoapl commented Dec 7, 2023

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 Dec 7, 2023

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 Dec 7, 2023

k8s-ci-robot commented Dec 7, 2023

k8s-ci-robot commented Dec 7, 2023

mengjiao-liu commented Dec 8, 2023

mengjiao-liu Dec 8, 2023

Choose a reason for hiding this comment

ricardoapl Dec 8, 2023 • edited

Choose a reason for hiding this comment

mengjiao-liu Dec 8, 2023

Choose a reason for hiding this comment

ricardoapl Dec 8, 2023 • edited

Choose a reason for hiding this comment

pohly Dec 8, 2023

Choose a reason for hiding this comment

ricardoapl Dec 8, 2023

Choose a reason for hiding this comment

pohly Dec 12, 2023

Choose a reason for hiding this comment

mengjiao-liu commented Dec 11, 2023

mengjiao-liu commented Dec 12, 2023

k8s-ci-robot commented Dec 12, 2023

mengjiao-liu commented Dec 12, 2023

sttts commented Dec 12, 2023

k8s-ci-robot commented Dec 12, 2023

ricardoapl Dec 8, 2023 •

edited

ricardoapl Dec 8, 2023 •

edited