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

✨ Add JSON log format and deprecate klog flags #6072

Merged
merged 1 commit into from Feb 17, 2022

Conversation

sbueringer
Copy link
Member

@sbueringer sbueringer commented Feb 7, 2022

What this PR does / why we need it:
This PR starts using components-base/logs. Through that we get the following changes:

I think it makes sense to deprecate the klog flags in ClusterAPI too as the same reasons managed in the KEP also apply to us. I also think it makes sense to use component-base/logs as we get JSON logging for free, without having to implement it ourselves. Downside is that we depend on upstream k/component-base, but I think this should be fine.

Note: This PR does not enable JSON logging per default, which makes sense given that it's currently alpha upstream.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #5571
Related #6069
Related #3889

Some links:
* doc: klog
* doc: JSON log format
* KEP-1602: Structured Logging
* KEP-2845: Deprecate klog specific flags in Kubernetes Components

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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 Feb 7, 2022
@sbueringer sbueringer force-pushed the pr-json-log branch 2 times, most recently from 4aebce0 to fbd58a3 Compare February 7, 2022 19:11
@sbueringer sbueringer changed the title [WIP] Add JSON log format [WIP] Add JSON log format and deprecate klog flags Feb 7, 2022
@sbueringer
Copy link
Member Author

sbueringer commented Feb 7, 2022

@dims Do you think it makes sense to use component-base/logs in CAPI instead of using the klog flags directly?

The main reasons for it in my opinion are to profit from upstream Kubernetes developments:

  • decoupling & deprecation from klog flags
  • JSON logging implementation

Not sure if it's problematic to depend on component-base/logs.

@sbueringer sbueringer changed the title [WIP] Add JSON log format and deprecate klog flags ✨ Add JSON log format and deprecate klog flags Feb 7, 2022
@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 Feb 7, 2022
@enxebre
Copy link
Member

enxebre commented Feb 9, 2022

+1, this seems a step in good direction towards converging with kube ecosystem common pattern.

@sbueringer
Copy link
Member Author

/test pull-cluster-api-e2e-informing-ipv6-main
(just need feedback if this job works with main, we have at least issues on release-1.1)

@sbueringer
Copy link
Member Author

sbueringer commented Feb 9, 2022

+1, this seems a step in good direction towards converging with kube ecosystem common pattern.

I agree.

Just fyi, I moved further discussion to the issue (#5571 (comment)) and tried to summarize there what it would mean to adopt component-base/logs.

@vincepri
Copy link
Member

vincepri commented Feb 9, 2022

Should we mark this as a breaking change?

@sbueringer
Copy link
Member Author

sbueringer commented Feb 9, 2022

Should we mark this as a breaking change?

I think there are no breaking changes in this PR. From a user perspective:

  • There is an additional logging-format flag (and a few more, details in the issue))
  • The default logging-format is still text
  • The klog flags will be deprecated, but they are still there (k/k plans to drop them with v1.26)
    • fyi, they are added in logs.AddFlags(fs, logs.SkipLoggingConfigurationFlags())

I think what we should definitely do, if we decide to go ahead with this change, is document this as a recommendation (?) for providers in the v1.1=>v1.2 migration guide

@vincepri
Copy link
Member

vincepri commented Feb 9, 2022

fyi, they are added in logs.AddFlags(fs, logs.SkipLoggingConfigurationFlags())

Got it, this was missed as it wasn't clear that this new line adds the same flags

@sbueringer
Copy link
Member Author

/test pull-cluster-api-e2e-informing-ipv6-main

Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

I'm definitely +1 to keep up with Kubernetes' new KEPs in this area.
/lgtm

Giving a little bit of time for people to review new dependency, but if no one raises concerns I'm going to get this merged sometimes next week

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2022
Copy link
Contributor

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 16, 2022
@sbueringer
Copy link
Member Author

Just rebased onto current main to pull in latest changes on main.

Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 17, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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 Feb 17, 2022
@k8s-ci-robot k8s-ci-robot merged commit 294917f into kubernetes-sigs:main Feb 17, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.2 milestone Feb 17, 2022
@sbueringer sbueringer deleted the pr-json-log branch February 17, 2022 05:30
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support JSON log format
6 participants