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

funcr Feature Request: Add LogInfoLevel Option to skip logging level in the info log #239

Closed
spacewander opened this issue Nov 28, 2023 · 6 comments · Fixed by #240
Closed
Assignees

Comments

@spacewander
Copy link
Contributor

Currently, the info level is always logged:

args = append(args, "level", level, "msg", msg)
.

However, in some situations, people may not use the log level or have an alternative to the integer log level field. So logging the integer log level in these situations may be a little useless.

The zapr implementation has an option called LogInfoLevel to control if the numeric log level is logged:
https://github.com/go-logr/zapr/blob/78b8af5329abd1ba8695aad821f95fb2e7f4e651/zapr.go#L270

What about adding a similar feature to logr? I am glad to implement it if it is accepted.

@pohly pohly changed the title Feature Request: Add LogInfoLevel Option to skip logging level in the info log funcr Feature Request: Add LogInfoLevel Option to skip logging level in the info log Nov 28, 2023
@pohly
Copy link
Contributor

pohly commented Nov 28, 2023

Do you see an advantage in using funcr compared to some other backend (like zapr, which you already mentioned)? I'm not sure how much further changes will be done (or accepted) there.

Note that as of 1.3.0, you can also use slog backends, so if you are on Go 1.21 and worried about additional dependencies, then you can configure the log/slog JSON handler as backend for logr.

@thockin
Copy link
Contributor

thockin commented Nov 28, 2023

I don't think one is too bad for funcr, and I would probably accept it as a PR. Something @pohly said without saying - funcr doesn't currently support slog, whereas slog.JSONHandler can be used as a logr backend. I would also be OK to make funcr support slog better, but I have not looked at doing that.

@spacewander
Copy link
Contributor Author

Do you see an advantage in using funcr compared to some other backend (like zapr, which you already mentioned)? I'm not sure how much further changes will be done (or accepted) there.

Note that as of 1.3.0, you can also use slog backends, so if you are on Go 1.21 and worried about additional dependencies, then you can configure the log/slog JSON handler as backend for logr.

In fact, I only use the Formatter from func. I am building a framework upon the Go extension of Envoy. The formatted string will be written to Envoy's log by calling Envoy's log API.

If I use zapr or slog, there will be two headers for each log: one from Envoy, the other from Go logger. Therefore I choose to build a logger from scratch.

@spacewander
Copy link
Contributor Author

I don't think one is too bad for funcr, and I would probably accept it as a PR. Something @pohly said without saying - funcr doesn't currently support slog, whereas slog.JSONHandler can be used as a logr backend. I would also be OK to make funcr support slog better, but I have not looked at doing that.

Great! I will give it a try when I have free time.

@pohly
Copy link
Contributor

pohly commented Nov 29, 2023

In fact, I only use the Formatter from func.

There are similar building blocks for slog - see https://github.com/golang/go/wiki/Resources-for-slog. But enhancing funcr might be simpler.

@spacewander
Copy link
Contributor Author

In fact, I only use the Formatter from func.

There are similar building blocks for slog - see https://github.com/golang/go/wiki/Resources-for-slog. But enhancing funcr might be simpler.

@pohly
Thanks for pointing out that! I am using Go 1.20 so slog is not available for now, but I will investigate it once we upgrade the Go version.

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

Successfully merging a pull request may close this issue.

3 participants