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

What level does klog.Warning map to? #151

Closed
justinsb opened this issue Oct 6, 2022 · 3 comments
Closed

What level does klog.Warning map to? #151

justinsb opened this issue Oct 6, 2022 · 3 comments
Assignees

Comments

@justinsb
Copy link

justinsb commented Oct 6, 2022

I'd like to use klog.FromContext, but that returns a logr.Logger, so now I have to map klog concepts in logr. That then exposes me to the info vs warning challenge.

As Info maps to V(0), then presumably Warning maps to V(-1). However logr currently disallows negative V values, mapping them to 0.

I think the choices we have are:
a) to deprecate Warning in klog (a huge task)
b) to replace logr in klog with something that maps more directly to klog functionality
c) to relax logr restrictions around negative V values and treat Warning as V(-1). We could still disallow explicit calls to log.V(-1), and say that the only way to get the negative level is to call log.Warning.

My preference would be option (c), but I'd like to hear the maintainers views here. Or are we thinking option (b) is more likely, given the recent proposals to enhance logging in the (broader) go stdlib?

@pohly
Copy link
Contributor

pohly commented Oct 6, 2022

logger.V(0) is equivalent to logger, and logger.Info should always emit the log message. Therefore the Kubernetes migration guidelines are to replace klog.Warning with logger.Info, perhaps with "Warning" inside the message text. Would that work for you?

@justinsb
Copy link
Author

justinsb commented Oct 7, 2022

The reason that's tricky is that now I have to make bulk changes to all the callsites that call Warning, including changing the messages, so this is no longer something that we can consider a refactoring or a no-op. Users that were filtering only for warning messages also now have to change their behaviour/tooling. I don't see a ton of upside, so it goes into the "not worth it" bucket.

I think if I better understood why removing warning was important, rather than just a nice-to-have, this would be an easier pill to swallow. Do we have such a document / FAQ? If we end up figuring out a reason here, I'd be happy to contribute such a document!

@thockin
Copy link
Contributor

thockin commented Oct 7, 2022

The dogmatic answer is to force clarity - is this something that needs attention or not? In my own experience, the use of Warning is all over the map.

Practically, you have to change most callsites anyway because logr doesn't have any printf-style calls, so you at least need to change args to key-value pairs. Right?

Lastly, we have always said that logs are not an API, or else we can't ever evolve them. I wouldn't change them wildly for no reason, but I also wouldn't feel terrible about changing them, especially if the main text remains.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants