-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat(logging): migration to structure logging for main.go, pkg/ & internal/ folders #1807
Conversation
@dmpe: The label(s) In response to this:
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. |
Welcome @dmpe! |
I have tested this locally via e2e tests and it did not fail :) When starting kube state metrics before my PR:
And after my proposed changes.
|
3f8a6f1
to
19c966c
Compare
Signed-off-by: dmpe <John Malc> <[email protected]>
19c966c
to
f406c1a
Compare
Co-authored-by: JUN YANG <[email protected]>
Co-authored-by: JUN YANG <[email protected]>
Co-authored-by: JUN YANG <[email protected]>
Co-authored-by: JUN YANG <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
/lgtm
/hold
for @dgrisonnet @fpetkovski to review.
@@ -178,7 +178,7 @@ func (b *Builder) WithCustomResourceStoreFactories(fs ...customresource.Registry | |||
for i := range fs { | |||
f := fs[i] | |||
if _, ok := availableStores[f.Name()]; ok { | |||
klog.Warningf("The internal resource store named %s already exists and is overridden by a custom resource store with the same name, please make sure it meets your expectation", f.Name()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this because we don't have a way to log warn messages with structure logging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that does not exist there. See also: kubernetes/klog#184
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dmpe, fpetkovski, mrueg 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 |
/hold cancel |
What this PR does / why we need it:
/wg structured-logging
/area logging
/priority important-longterm
/kind cleanup
/cc @kubernetes/wg-structured-logging-reviews
How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)
does not change cardinality
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):part 1 of adding JSON support #1485, i.e. with this PR we introduce structure logging, by following the associated KEP https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md