Skip to content
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.

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

Support structured logging #149

Merged

Conversation

bells17
Copy link
Contributor

@bells17 bells17 commented Sep 12, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:

I've updated the klog functions used within csi-lib-utils to structured logging functions, following the guidelines below:
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md

Several CSI Sidecars like external-snapshotter rely on csi-lib-utils, and to fully support structured logging in these Sidecars, it's necessary for csi-lib-utils to adopt structured logging as well.

In addition, I've modified the log messages based on the following guideline:
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#remove-string-formatting-from-log-message

I’ve updated the klog functions in use according to the guidelines provided below, and I've confirmed that they pass the logcheck tests:
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#change-log-functions

Which issue(s) this PR fixes:

Fixes #150

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Added support for structured logging (the log messages have been changed due to the activation of structured logging)

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. labels Sep 12, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 12, 2023
@bells17 bells17 marked this pull request as ready for review September 12, 2023 18:39
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 12, 2023
Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

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

Instead of "just" doing structured logging, would it be possible to also do contextual logging, i.e. remove the dependency on the global logger?

This makes libraries more versatile.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 13, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 13, 2023
@bells17
Copy link
Contributor Author

bells17 commented Sep 13, 2023

@pohly Thank you for your review.

Instead of "just" doing structured logging, would it be possible to also do contextual logging, i.e. remove the dependency on the global logger?

This makes libraries more versatile.

I've made an attempt to incorporate contextual logging as well. Could you please review it again?
(Sorry for the inconvenience, I had to force push due to a failed conflict resolution.)

I have also confirmed that the following tests with logcheck pass:

$ logcheck -check-contextual ./accessmodes/
$ logcheck -check-contextual ./connection/
$ logcheck -check-contextual ./deprecatedflags/
$ logcheck -check-contextual ./leaderelection/
$ logcheck -check-contextual ./metrics/
$ logcheck -check-contextual ./protosanitizer/
$ logcheck -check-contextual ./rpc/

connection/connection.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 16, 2023
@bells17 bells17 force-pushed the support-structured-logging branch 2 times, most recently from 1b7a924 to 5767ec6 Compare September 17, 2023 17:10
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 17, 2023
@bells17 bells17 requested a review from pohly September 17, 2023 17:22
connection/connection.go Show resolved Hide resolved
connection/connection.go Outdated Show resolved Hide resolved
connection/connection.go Outdated Show resolved Hide resolved
deprecatedflags/deprecatedflags.go Outdated Show resolved Hide resolved
leaderelection/leader_election.go Outdated Show resolved Hide resolved
rpc/common.go Outdated Show resolved Hide resolved
@bells17
Copy link
Contributor Author

bells17 commented Apr 24, 2024

@pohly Thank you for the review.
I've made revisions following the comments you provided. I would appreciate it if you could confirm the changes.

rpc/common.go Show resolved Hide resolved
Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

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

Some small nits, but overall this looks good.

@bells17
Copy link
Contributor Author

bells17 commented Apr 30, 2024

@pohly Thank you for the improvement suggestions. I've updated the implementation accordingly.
Could you please review the changes when you have a moment?

@bells17 bells17 requested a review from pohly April 30, 2024 14:55
@pohly
Copy link
Contributor

pohly commented May 2, 2024

/lgtm

@bells17: can you squash into one commit?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 2, 2024
@bells17 bells17 force-pushed the support-structured-logging branch from 6e6f23e to ca88d98 Compare May 2, 2024 14:32
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 2, 2024
@bells17
Copy link
Contributor Author

bells17 commented May 2, 2024

@pohly Thank you for the review and lgtm!
I've squashed the commits into one.

@pohly
Copy link
Contributor

pohly commented May 2, 2024

/lgtm
/assign @jsafrane

For approval.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 2, 2024
@jsafrane
Copy link
Contributor

jsafrane commented May 3, 2024

/approve

@k8s-ci-robot
Copy link
Contributor

[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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 3, 2024
@k8s-ci-robot k8s-ci-robot merged commit 5a06050 into kubernetes-csi:master May 3, 2024
3 checks passed
@jsafrane jsafrane mentioned this pull request May 3, 2024
@bells17 bells17 deleted the support-structured-logging branch May 3, 2024 12:08
xing-yang pushed a commit to xing-yang/csi-lib-utils that referenced this pull request May 6, 2024
iPraveenParihar added a commit to csi-addons/kubernetes-csi-addons that referenced this pull request May 14, 2024
Version 0.18.0 of github.com/kubernetes-csi/csi-lib-utils
added support for structured logging.
This commit includes passing the context parameter for the
necessary function.

ref: kubernetes-csi/csi-lib-utils#149

Signed-off-by: Praveen M <m.praveen@ibm.com>
iPraveenParihar added a commit to csi-addons/kubernetes-csi-addons that referenced this pull request May 15, 2024
Version 0.18.0 of github.com/kubernetes-csi/csi-lib-utils
added support for structured logging.
This commit includes passing the context parameter for the
necessary function.

ref: kubernetes-csi/csi-lib-utils#149

Signed-off-by: Praveen M <m.praveen@ibm.com>
mergify bot pushed a commit to csi-addons/kubernetes-csi-addons that referenced this pull request May 15, 2024
Version 0.18.0 of github.com/kubernetes-csi/csi-lib-utils
added support for structured logging.
This commit includes passing the context parameter for the
necessary function.

ref: kubernetes-csi/csi-lib-utils#149

Signed-off-by: Praveen M <m.praveen@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support structured logging
4 participants