Add logging developer guideline #5175
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gardener-robot
added
needs/review
area/dev-productivity
|
/assign |
76 tasks
timebertt
force-pushed
the
logging-guideline
branch
from
b44a7b4
to
b1a300d
Compare
|
Fixed the unit tests by adapting them to the changes in the last commit. |
timebertt
force-pushed
the
logging-guideline
branch
from
b1a300d
to
4e5fa93
Compare
|
Thanks for all your feedback! |
stoyanr
suggested changes
Dec 21, 2021
There was a problem hiding this comment.
I also like it very much. I also "tested" it by switching to logr.Logger in #5150, and it "worked" fine (but I still needed some advice from @timebertt, see below). Still, I have a few comments that I consider relatively important.
timebertt
force-pushed
the
logging-guideline
branch
from
e7da935
to
1ba9356
Compare
|
@stoyanr thanks for your feedback as well. I addressed your comments in new commits. PTAL :) |
|
/retest |
stoyanr
approved these changes
Dec 22, 2021
krgostev
pushed a commit
to krgostev/gardener
that referenced
this pull request
Apr 21, 2022
* Add logging developer guideline * Follow logging guideline in already refactored controllers * Consistently capitalize error logs * Consistently capitalize info logs * Address review comments * Address more feedback * Clarify the level mapping * Discourage use of kutil.ObjectName * Add some notes about the migration
krgostev
pushed a commit
to krgostev/gardener
that referenced
this pull request
Jul 5, 2022
* Add logging developer guideline * Follow logging guideline in already refactored controllers * Consistently capitalize error logs * Consistently capitalize info logs * Address review comments * Address more feedback * Clarify the level mapping * Discourage use of kutil.ObjectName * Add some notes about the migration
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
How to categorize this PR?
/area documentation dev-productivity logging
/kind enhancement
What this PR does / why we need it:
In the context of #4251, we are migrating our components from
logrustologr(see #5057 for part 1).As migrating to a new logging library and also a new logging style (structured logging) brings up a lot of questions and confusion, I proactively try to clarify them in a developer guideline around logging in our components.
Also, this guideline will hopefully bring more consistency into logging in our codebase.
This should be consulted by developers adding new logs or refactoring existing code (like me), as well as reviewers.
I hope that it will be useful for them, so that they don't have to read up on so many things and dig through logging code to figure out how to best write a simple log statement in some controller.
Which issue(s) this PR fixes:
Part of #4251
Special notes for your reviewer:
After "formalizing" the guideline, I went over the code touched by #5057 again and applied it to it. That's also why I'm adding this guideline now, so that I have something to follow along the way as well :)
Also, I cleaned up some "obvious" inconsistencies in the codebase with the guideline already (mainly regarding the capitalization part).
I only touched
logrstatements for now, as the rest will be touched later on anyways.Further inconsistencies like e.g.
fmt.Sprintfusage in log messages can be cleaned up later on in dedicated PRs./squash
Release note:
A developer guideline on logging in Gardener components has been added. Please consult this document as a developer or reviewer to ensure consistency in our logs across the codebase. You can find the document [here](https://github.com/gardener/gardener/blob/master/docs/development/logging.md).