Support structured and contextual logging #154

bells17 · 2023-09-17T20:07:19Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Migrate to use structured logging and contextual logging.
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Contextual logging added. Some function arguments have been modified.

pohly · 2023-09-18T06:00:36Z

controller/controller.go

 	}
 	// add a uniquifier so that two processes on the same host don't accidentally both become active
 	id = id + "_" + string(uuid.NewUUID())
 	component := provisionerName + "_" + id

 	v1.AddToScheme(scheme.Scheme)
 	broadcaster := record.NewBroadcaster()
-	broadcaster.StartLogging(klog.Infof)
+	broadcaster.StartStructuredLogging(0)


Here's a problem: we need a new API such that broadcaster accepts a logger instead of using the global one.

Let's not block the PR because of that, but a TODO would be good.

You can link to kubernetes/kubernetes#120729.

@pohly, I have a couple of questions regarding this:

Since no release tag has been created following the merge of the PR that added NewEventBroadcasterAdapterWithContext, is the adaptation to contextual logging for the event broadcaster unnecessary in this PR?

The interface of ResourceLockConfig.EventRecorder seems different from the new EventRecorder's interface. Would it be okay to change the interface of ResourceLockConfig.EventRecorder in a separate PR?

Since no release tag has been created following the merge of the kubernetes/kubernetes#122142, is the adaptation to contextual logging for the event broadcaster unnecessary in this PR?

Perhaps wait until this repo is updated to Kubernetes 1.30, then use the WithContext call?

The interface of ResourceLockConfig.EventRecorder seems different from the new EventRecorder's interface. Would it be okay to change the interface of ResourceLockConfig.EventRecorder in a separate PR?

I would update it together with updating Kubernetes 1.30 and adapting the code.

pohly · 2023-09-18T06:02:19Z

controller/controller.go

@@ -857,13 +862,14 @@ func (ctrl *ProvisionController) Run(ctx context.Context) {
 			go wait.Until(func() { ctrl.runVolumeWorker(ctx) }, time.Second, ctx.Done())
 		}

-		klog.Infof("Started provisioner controller %s!", ctrl.component)
+		logger.Info("Started provisioner controller!", "component", ctrl.component)


The convention is to not end the message string with punctuation because the key/value pairs follow.

pohly · 2023-09-18T06:06:53Z

controller/controller.go

-				klog.Errorf("Giving up syncing claim %q because failures %v >= threshold %v", key, ctrl.claimQueue.NumRequeues(obj), ctrl.failedProvisionThreshold)
-				klog.V(2).Infof("Removing PVC %s from claims in progress", key)
+				logger.Error(nil, "Giving up syncing claim because failures >= threshold", "claim", key, "failures", ctrl.claimQueue.NumRequeues(obj), "threshold", ctrl.failedProvisionThreshold)
+				logger.V(2).Info("Removing PVC from claims in progress", "pvc", key)


"pvc" is usually spelled "PVC" and refers to an object, not a string.

Let's use "key" as key here whenever a work queue refers to the work queue key.

Can you do a full pass over the PR and try to make the key names consistent?

pohly · 2023-09-18T06:07:24Z

controller/controller.go

 				ctrl.volumeQueue.AddRateLimited(obj)
 			} else {
-				klog.Errorf("Giving up syncing volume %q because failures %v >= threshold %v", key, ctrl.volumeQueue.NumRequeues(obj), ctrl.failedDeleteThreshold)
+				logger.Info("Giving up syncing volume because failures >= threshold", "volume", key, "failure", ctrl.volumeQueue.NumRequeues(obj), "threshold", ctrl.failedDeleteThreshold)


"failure" -> "failures" (consistency!)

bells17 · 2023-09-18T16:11:51Z

@pohly Thank you for your review. I've addressed the issues you pointed out.
Could you please review this PR again?

bells17 · 2023-09-18T17:16:22Z

I've added checks for Contextual Logging using both golangci-lint and logcheck.

I wasn't sure about the best way to add logcheck, so I've set it up in the Makefile based on the script below. If there's a more appropriate method, I'd appreciate your guidance.
https://github.com/kubernetes/kubernetes/blob/76a22d3b32a632f6bb8f7b38011f7c028d507222/hack/verify-golangci-lint.sh

Also, if testing with logcheck is not necessary, please let me know.

pohly · 2023-10-25T11:31:42Z

Makefile

+install-tools: $(GOBIN)
+	cd hack/tools \
+		&& GOBIN=$(GOBIN) go install github.com/golangci/golangci-lint/cmd/golangci-lint \
+		&& go build -o "${GOBIN}/logcheck.so" -buildmode=plugin sigs.k8s.io/logtools/logcheck/plugin


You don't need golangci-lint to run logcheck.

Instead you can do:

go install sigs.k8s.io/logtools/[email protected] logcheck -check-contextual ./...

This can go into verify below.

@bells17 can you please update the makefile with the suggestion?

bells17 · 2024-01-26T02:57:01Z

@pohly @jsafrane I apologize for the delay in addressing this. I have now made the necessary revisions based on your comments. I would greatly appreciate it if you could review the changes.

jsafrane · 2024-04-23T08:38:40Z

controller/controller.go

+	// TODO: Once the following PR is merged, change to use StartLogging and StartRecordingToSinkWithContext
+	// https://github.com/kubernetes/kubernetes/pull/120729


The PR is merged now.

jsafrane · 2024-04-24T13:18:12Z

/lgtm
/approve
We can solve TODO comment in a subsequent PR.

k8s-ci-robot · 2024-04-24T13:18:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bells17, jsafrane

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

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

bells17 · 2024-04-24T13:56:20Z

/retest

jsafrane · 2024-04-24T14:11:06Z

New go cannot install gometalinter, I'll solve it somehow in a separate PR.

bells17 · 2024-04-24T19:26:47Z

@jsafrane Regarding your comment about the new Go version not being able to install gometalinter, are you considering switching to use golangci-lint as an alternative? If this understanding is correct, may I undertake the task of integrating golangci-lint?

If I am allowed to proceed, I would appreciate your advice on whether I should update the repo-infra repository itself and reflect those changes in the sig-storage-lib-external-provisioner repository, or should I just introduce golangci-lint independently in the sig-storage-lib-external-provisioner repository?

jsafrane · 2024-04-25T15:40:30Z

@bells17 can you please rebase? I am sorry, I updated k8s to 1.30 and removed gometalinter in a single PR, now I see they should be separate.

bells17 · 2024-04-25T16:07:24Z

hack/verify-logcheck.sh

@@ -0,0 +1,37 @@
+#!/usr/bin/env bash


https://github.com/kubernetes-csi/csi-lib-utils/pull/149/files#diff-2f76c847ffab450d5e5be791a39758261423800a823839cff7dcace87e135b26

bells17 · 2024-04-25T16:17:42Z

@jsafrane Thank you. I've rebased. Additionally, after rebasing, the logcheck started to fail, so I am making the necessary corrections.
Please check when you have a moment.

jsafrane · 2024-04-26T07:30:06Z

Thanks!
/lgtm

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 17, 2023

k8s-ci-robot requested review from pohly and wongma7 September 17, 2023 20:07

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 17, 2023

bells17 force-pushed the structured-and-contextual-logging branch from 7abe4a2 to 50eae47 Compare September 17, 2023 20:10

pohly suggested changes Sep 18, 2023

View reviewed changes

pohly mentioned this pull request Sep 18, 2023

KEP-3077: contextual logging: promotion to beta in 1.30 kubernetes/enhancements#4219

Merged

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 18, 2023

bells17 requested a review from pohly September 18, 2023 17:20

bells17 force-pushed the structured-and-contextual-logging branch from 10f85a9 to fb3c86a Compare September 19, 2023 00:37

pohly reviewed Oct 25, 2023

View reviewed changes

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

bells17 force-pushed the structured-and-contextual-logging branch from fb3c86a to f07d971 Compare January 25, 2024 02:34

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 25, 2024

bells17 force-pushed the structured-and-contextual-logging branch 3 times, most recently from 4afa277 to 521d380 Compare January 25, 2024 03:15

bells17 requested review from pohly and jsafrane January 26, 2024 02:57

bells17 mentioned this pull request Jan 26, 2024

Support structured logging kubernetes-csi/csi-lib-utils#149

Merged

jsafrane reviewed Apr 23, 2024

View reviewed changes

k8s-ci-robot assigned jsafrane Apr 24, 2024

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 24, 2024

bells17 added 6 commits April 26, 2024 00:52

Support structured and contextual logging

99965bc

Add TODO comment

eeaf9cf

Fixed klog messages and key names

4313461

Added Contextual Logging check by logcheck

3c1f221

Switch to using logcheck directory instead of golangci-lint

d266edf

Change argument from context to logger where logger is sufficient

52e8ed4

bells17 force-pushed the structured-and-contextual-logging branch from 521d380 to 52e8ed4 Compare April 25, 2024 15:52

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 25, 2024

Fix logcheck error

2a46229

bells17 commented Apr 25, 2024

View reviewed changes

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

k8s-ci-robot merged commit 9e6b821 into kubernetes-sigs:master Apr 26, 2024
4 checks passed

bells17 deleted the structured-and-contextual-logging branch April 26, 2024 07:45

bells17 mentioned this pull request May 3, 2024

Support structured logging: broadcaster & eventRecorder #171

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support structured and contextual logging #154

Support structured and contextual logging #154

bells17 commented Sep 17, 2023 •

edited by jsafrane

Loading

pohly Sep 18, 2023

pohly Sep 18, 2023

pohly Sep 18, 2023

bells17 Jan 26, 2024

pohly Mar 28, 2024

pohly Sep 18, 2023

pohly Sep 18, 2023

pohly Sep 18, 2023

bells17 commented Sep 18, 2023

bells17 commented Sep 18, 2023 •

edited

Loading

pohly Oct 25, 2023 •

edited

Loading

jsafrane Dec 5, 2023

bells17 commented Jan 26, 2024

jsafrane Apr 23, 2024

jsafrane commented Apr 24, 2024

k8s-ci-robot commented Apr 24, 2024

bells17 commented Apr 24, 2024

jsafrane commented Apr 24, 2024

bells17 commented Apr 24, 2024

jsafrane commented Apr 25, 2024

bells17 Apr 25, 2024

bells17 commented Apr 25, 2024

jsafrane commented Apr 26, 2024

		// TODO: Once the following PR is merged, change to use StartLogging and StartRecordingToSinkWithContext
		// https://github.com/kubernetes/kubernetes/pull/120729

Support structured and contextual logging #154

Support structured and contextual logging #154

Conversation

bells17 commented Sep 17, 2023 • edited by jsafrane Loading

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?

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

bells17 commented Sep 18, 2023

bells17 commented Sep 18, 2023 • edited Loading

pohly Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bells17 commented Jan 26, 2024

Choose a reason for hiding this comment

jsafrane commented Apr 24, 2024

k8s-ci-robot commented Apr 24, 2024

bells17 commented Apr 24, 2024

jsafrane commented Apr 24, 2024

bells17 commented Apr 24, 2024

jsafrane commented Apr 25, 2024

Choose a reason for hiding this comment

bells17 commented Apr 25, 2024

jsafrane commented Apr 26, 2024

bells17 commented Sep 17, 2023 •

edited by jsafrane

Loading

bells17 commented Sep 18, 2023 •

edited

Loading

pohly Oct 25, 2023 •

edited

Loading