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

Allow klog configuration from somewhere other than flag #336

Open
fire833 opened this issue Jun 28, 2022 · 15 comments
Open

Allow klog configuration from somewhere other than flag #336

fire833 opened this issue Jun 28, 2022 · 15 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@fire833
Copy link

fire833 commented Jun 28, 2022

/kind feature

Describe the solution you'd like
Currently, the main way that you can configure klog is through the registering of flags with the standard flag package. This is useful for most cases, but it would be useful to provide another configuration interface. My idea would be to provide something like InitWithValues(init *klog.Configuration) method where the config struct provides the same flag values, but through go instead.

Or at the very least, provide an external method SetVerbosity(in int) as a means for configuring the verbosity in some way other than flags. This would be useful if you are using other flag libraries and allow you to configure klog with them as well and not being tied to using the standard flag library.

Let me know what you think.

Anything else you would like to add:
[Miscellaneous information that will assist in solving the issue.]

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 28, 2022
@pohly
Copy link

pohly commented Jun 28, 2022

The problem with klog is that it is close to impossible to experiment with new APIs: as soon as they are in a release, the API has to adhere to the strict compatibility guarantee of a v2 Go module. Therefore I am not very enthusiastic about anything that will need such significant changes.

@fire833
Copy link
Author

fire833 commented Jun 28, 2022

I am thinking it would just involve adding a public method that updates the value of settings.verbosity, probably just by calling Level.set(). I am kind of new to k8s API compliance. Would a simple addition of a public API cause problems, even if no other methods are touched?

@pohly
Copy link

pohly commented Jun 29, 2022

Would a simple addition of a public API cause problems, even if no other methods are touched?

We would be bound to support that API as-is forever (well, within klog/v2, but we don't want to switch to a /v3). The API therefore becomes technical debt.

@fire833
Copy link
Author

fire833 commented Jun 29, 2022

Ah understood.

@zostay
Copy link

zostay commented Jul 5, 2022

I understand the reluctance to make this change, but the lack of configurability creates situations that are very difficult to deal with.

For example, I have an application that is dependent upon code in k8s.io/client-go/tools, which uses klog/v2 to output logs. Something has gone wrong and I would like to be able to bump the v-level up to trace what's going on in the logs. However, doing so will require calling InitFlags in the application. The application uses zap-logger as it's normal logging platform (which we tell klog to use via klog.SetLogger). As such, we already expose flags for configuring log-levels and such related to that logger. In order to configure klog, I would need two sets of logging flags exposed to control different logging systems, which would be confusing even among developers, let alone end-users. So in order to add the v-level configuration functionality, I will want just use a monkey patch and avoid a production change. Or I might attempt something bizarre like figuring out how to manipulate the command-line so klog can parse pretend args that I've injected. (I'm not even sure that's possible, but it's the sort of bizarre approach I'm forced to consider because of how this works.)

The lack of configurability here, even if it's a tech debt burden for the future, is fairly problematic on its own. Any application using code the logs via klog that does not itself use klog as its primary logger is going to have problems. Any application that wants to configure logging by any means other than the command-line at start is also going to have problems.

@pohly
Copy link

pohly commented Jul 7, 2022

I would need two sets of logging flags exposed to control different logging systems, which would be confusing even among developers, let alone end-users.

No, you don't 😁 You can create a flag set that is not the one from your program and use that to change the values:

var fs flags.FlagSet
klog.InitFlags(&fs)
err := fs.Set("v", "5")

I understand that this is perhaps not the API that you want, but at least it exists and exposes all options in a consistent way.

@pohly
Copy link

pohly commented Aug 22, 2022

Here is one potential way forward:

  • export string constants with the name of all command line flags: those are part of the API already, we would just document them in a way that apidiff would also pick up any future changes (which has some value by itself, although it is unlikely that we'll break this)
  • create a "config" submodule (i.e. a directory with its own go.mod file): this would need to be tagged separately, but that's doable
  • add new API calls inside that submodule which call flag.Set for a local flag set on which InitFlags was called: this new API does not become part of "proper" klog and therefore this new API can still be changed before committing to it in a 1.0.0 release

@dims: would that be okay?

@dims
Copy link
Member

dims commented Aug 22, 2022

@pohly works for me!

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 20, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 20, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

@k8s-ci-robot k8s-ci-robot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 19, 2023
@k8s-ci-robot
Copy link

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

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.

@pohly
Copy link

pohly commented Mar 29, 2023

/remove-lifecycle rotten
/reopen
/triage accepted

@k8s-ci-robot k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Mar 29, 2023
@k8s-ci-robot k8s-ci-robot reopened this Mar 29, 2023
@k8s-ci-robot
Copy link

@pohly: Reopened this issue.

In response to this:

/remove-lifecycle rotten
/reopen
/triage accepted

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.

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Mar 29, 2023
@k8s-triage-robot
Copy link

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

  • Confirm that this issue is still relevant with /triage accepted (org members only)
  • Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. and removed triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
None yet
Development

No branches or pull requests

6 participants