Kubebuilder: OSDK->KB: Logging Flags

[DirectXMan12]

Optional utilities for adding flags to customize log level.

/kind feature

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[DirectXMan12]

/remove-lifecycle stale

[camilamacedo86]

Description:

It is basically to get [0] into controller-runtime. Haseeb has a PR that would make this simpler [1].

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[camilamacedo86]

/remove-lifecycle stale

[Adirio]

A pattern I really like about log levels is choosing a default log level (usually Info, Warning or Error) and then providing two flags (-v or --verbose and -q or --quiet) that increase/decrease the log level. These flags can be provided several times.

For example, if the default level is Warning:

• kubebuilder -vv ... will set the level to Debug
• kubebuilder -q ... will set the level to Error

I already have this pattern implemented in several projects so I can contribute it if you like it.

[camilamacedo86]

Hi @Adirio,

Sure. Feel free to contribute. It shows great 👍 . Just beware that we need to is as described https://github.com/kubernetes-sigs/kubebuilder/issues/885#issuecomment-552171696

[Adirio]

Do we need to use the flag names and behavior in that file? Or do we just need to provide all the functionality of those flags?

For example, kubebuilder does a good job not showing which logging framework it uses, but the flag names mention zap directly.

[camilamacedo86]

Hi @Adirio,

Note that we need to merge the PR https://github.com/kubernetes-sigs/kubebuilder/pull/1116 first.

Do we need to use the flag names and behavior in that file? Or do we just need to provide all the functionality of those flags?

For example, kubebuilder does a good job not showing which logging framework it uses, but the flag names mention zap directly.

Good question. The reason this task is the integration with SDK. So, I would say that we should keep the same names. However, I think that I would also prefer the common usage as -vv as well. @estroz @hasbro17 wdyt?

[Adirio]

The following list describes the flags in Operator SDK and how they can be implemented with [email protected]:

• --zap-devel: development (true) or production (false) mode.
  sigs.k8s.io/controller-runtime/pkg/log/zap.UseDevMode
• --zap-encoder: "json" (no spaces after , and :) or "console" (spaces after , and :).
  sigs.k8s.io/controller-runtime/pkg/log/zap.Encoder
• --zap-time-encoding: time formating in logs, valid values are "iso8601", "ISO8601", "millis" (milliseconds since epoch), "nanos" (nanoseconds since epoch), "epoch" (seconds since epoch).
  sigs.k8s.io/controller-runtime/pkg/log/zap.Encoder
• --zap-level: "debug", "info", "error", or any positive integer.
  sigs.k8s.io/controller-runtime/pkg/log/zap.Level
• --zap-stacktrace-level: "debug", "info", "error", or any positive integer.
  sigs.k8s.io/controller-runtime/pkg/log/zap.StacktraceLevel
• --zap-sample: allows to disable log sampling in production mode (in development mode it is always disabled)
  NOTE: [email protected] always samples in production mode

My suggestion would be:

• --development / -d instead of --zap-devel
• --verbose / -v and --quiet / -q instead of --zap-level with the behavior described in https://github.com/kubernetes-sigs/kubebuilder/issues/885#issuecomment-584628521
• --log-stacktrace-level instead of --zap-stacktrace-level extending the possible values to include warnings, panics and fatals as levels.
• Do not implement --zap-encoder. Notice that --development/--zap-devel already modifies the encoding format and the difference from "json" to "console" are just a couple spaces.
• --log-time-format instead of --zap-time-encoding extending the options to include all zap supported ones.
• Do not implement --zap-sample as [email protected] doesn't allow to disable sampling in production mode.

[shawn-hurley]

https://github.com/kubernetes-sigs/controller-runtime/pull/767/files

this PR has merged will the current flags being:

zap-devel
zap-encoder
zap-log-level
zap-stacktrace-level

I believe that these names are important to have the zap prefix to make sure that we describe what we are manipulating. There are many systems that one could use behind logr interface, and we are using zap and I think that needs to be exposed in the flags. if we added a new package to configure klog, and use the klogr interface I would suggest that this has a different set of flags that you could bind to.

I think we can add a follow-up's to:

1. change the name of zap-devel to zap-development. As we don't have short flags I personally feel zap-devel gets the same point across with less typing, but not going to get hung up either way.

1. change zap-log-level to zap-verbosity. I would vote against this because it is not how zap works but if we had something like klog flags then I would be very much in favor of that style.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[camilamacedo86]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[camilamacedo86]

/remove-lifecycle rotten

[StevenACoffman]

Given that kubernetes-sigs/controller-runtime#767 is merged, what more is remaining on this?

Is it just this?

• --zap-time-encoding: time formating in logs, valid values are "iso8601", "ISO8601", "millis" (milliseconds since epoch), "nanos" (nanoseconds since epoch), "epoch" (seconds since epoch).
  sigs.k8s.io/controller-runtime/pkg/log/zap.Encoder
• --zap-sample: allows to disable log sampling in production mode (in development mode it is always disabled)
  NOTE: [email protected] always samples in production mode

[camilamacedo86]

Hi @StevenACoffman,

We need to address the comment https://github.com/kubernetes-sigs/kubebuilder/issues/885#issuecomment-597872422 in https://github.com/kubernetes-sigs/controller-runtime/blob/0e1344145ac34e036199f4d5abc8c71b59b7d7e5/pkg/log/zap/zap.go#L237 and the flags that you raise as well. Also, see that controller-runtime has the issue https://github.com/kubernetes-sigs/controller-runtime/issues/1035.

IHMO we ough to track this need in controller-runtime and work on it there and then close this issue with the PR #1721. WDYT @joelanford ?

@StevenACoffman, Would you like to help with this one?

[camilamacedo86]

• To address the comment #885 (comment) the issue https://github.com/kubernetes-sigs/controller-runtime/issues/1226 was raised.
• To address the option to disable the logs controller-runtime has one issue already: https://github.com/kubernetes-sigs/controller-runtime/issues/1035
• regards to encoding time shows that it was addressed here; https://github.com/kubernetes-sigs/controller-runtime/pull/915/files

So, I think we can close this one with the https://github.com/kubernetes-sigs/kubebuilder/pull/1721 and then, if we need we can raise a new issue with a scope more defined to know what exactly still missing and should get done in KB. However, please feel free also to re-open if you think that it is required.