-
Notifications
You must be signed in to change notification settings - Fork 18
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 to configure allowed levels by string value #22
Allow to configure allowed levels by string value #22
Conversation
@mcosta74 Thanks for the well written PR. You added docs, examples, and all the nice touches. Much appreciated. I'm still not sure if we want to add this API, but if we do I think your approach takes the right approach. So, before I add any comments on the details I want to poll the other maintainers for their opinion. @peterbourgon, @sagikazarmark what do you think of adding this option? |
I'm not 100% sure why a fallback option is necessary. As far as I can tell, it's equivalent to the following: logger = level.NewFilter(logger, level.AllowInfo, level.AllowByString(os.Getenv("LOG_LEVEL"))) If the string doesn't match any of the log levels, it should just be noop. Personally, I'm not a huge fan of fallbacks in this case though. Consider the following: let's say you misconfigure the production instance and as a result debug logs will start flowing in, potentially containing PII without you even noticing. I usually log a warning or an error whenever the log config is invalid (which is also why I manually parse log levels and match them to the appropriate log levels). The name Other than these, I don't mind adding a few utility functions if they really help. |
@sagikazarmark Agree, the fallback mechanism may obfuscate a misconfiguration. Should I apply the changes or better to wait a final decision about adding or not that function ?
I'm open to change the name if the approach is accepted |
I've applied the changes suggested by @sagikazarmark. |
hello @peterbourgon, I've applied all changes requested.
Please let me know if you find better wording for the doc strings. I'm open to change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't totally complete but I hope my intent is clear :)
level/example_test.go
Outdated
func Example_parse() { | ||
// Set up logger with level filter. | ||
allow, err := level.ParseOption("info") | ||
if err != nil { | ||
// fallback to default value in case parse failure | ||
allow = level.AllowInfo() | ||
} | ||
logger := log.NewLogfmtLogger(os.Stdout) | ||
logger = level.NewFilter(logger, allow) | ||
logger = log.With(logger, "caller", log.DefaultCaller) | ||
|
||
// Use level helpers to log at different levels. | ||
level.Error(logger).Log("err", errors.New("bad data")) | ||
level.Info(logger).Log("event", "data saved") | ||
level.Debug(logger).Log("next item", 17) // filtered | ||
|
||
// Output: | ||
// level=error caller=example_test.go:53 err="bad data" | ||
// level=info caller=example_test.go:54 event="data saved" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func Example_parse() { | |
// Set up logger with level filter. | |
allow, err := level.ParseOption("info") | |
if err != nil { | |
// fallback to default value in case parse failure | |
allow = level.AllowInfo() | |
} | |
logger := log.NewLogfmtLogger(os.Stdout) | |
logger = level.NewFilter(logger, allow) | |
logger = log.With(logger, "caller", log.DefaultCaller) | |
// Use level helpers to log at different levels. | |
level.Error(logger).Log("err", errors.New("bad data")) | |
level.Info(logger).Log("event", "data saved") | |
level.Debug(logger).Log("next item", 17) // filtered | |
// Output: | |
// level=error caller=example_test.go:53 err="bad data" | |
// level=info caller=example_test.go:54 event="data saved" | |
} | |
func Example_parse() { | |
fs := flag.NewFlagSet("myprogram", flag.ExitOnError) | |
lv := fs.String("log", "info", "debug, info, warn, or error") | |
fs.Parse([]string{"-log", "warn"}) | |
logger := log.NewLogfmtLogger(os.Stdout) | |
logger = level.NewFilter(logger, level.MustParseLevel(*lv)) | |
level.Error(logger).Log("msg", "alpha") | |
level.Warn(logger).Log("msg", "beta") | |
level.Info(logger).Log("msg", "delta") | |
level.Debug(logger).Log("msg", "kappa") | |
// Output: | |
// level=error msg="alpha" | |
// level=warn msg="beta" | |
} |
Looks like we have an agreement.
Let me implement these API
==============
Massimo Costa
Il ven 8 apr 2022, 20:16 Chris Hines ***@***.***> ha scritto:
… ***@***.**** commented on this pull request.
------------------------------
In level/level.go
<#22 (comment)>:
> +// ParseOption creates an Option from a string value. It might be useful in case we need to configure
+// the log level using a flag, an environment variable, a configuration file, ...
+//
+// Leading and trailing spaces are ignored and the comparison is Case Insensitive:
+// "ERROR", "Error", " Error " are accepted
+//
+// If the level parameter does not match any of the available values, the function returns a not nil error.
+func ParseOption(level string) (Option, error) {
+ level = strings.TrimSpace(strings.ToLower(level))
+
+ switch level {
+ case debugValue.name:
+ return AllowDebug(), nil
+
+ case infoValue.name:
+ return AllowInfo(), nil
+
+ case warnValue.name:
+ return AllowWarn(), nil
+
+ case errorValue.name:
+ return AllowError(), nil
+
+ default:
+ // return a noop Option in case of unknown level string
+ return nil, fmt.Errorf("invalid level: %s", level)
+ }
+}
🎉 Great. 🎉 I'm on board too.
—
Reply to this email directly, view it on GitHub
<#22 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4I7OKBLXWIID3Q36AU3CDVEBZYVANCNFSM5RYT3RZA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Pull Request Test Coverage Report for Build 2145714702Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
I think I implemented the agreed API, if the code is code we can also squash the commits |
Thanks, @mcosta74. I will review the code sometime in the next few days. Also, thanks for you patience during the API debate. |
no problem at all, I think it was a very interesting and productive discussion. I would only suggest to create a CONTRIBUTION page (maybe with a discussion template) to clarify what are the principles to follow in case someone else wants to propose new API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things.
level/level.go
Outdated
@@ -66,6 +75,26 @@ func (l *logger) Log(keyvals ...interface{}) error { | |||
// Option sets a parameter for the leveled logger. | |||
type Option func(*logger) | |||
|
|||
// Allow allows to configure the log level with a Value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Allow allows to configure the log level with a Value. | |
// Allow the provided log level to pass. |
level/level_test.go
Outdated
@@ -12,11 +12,51 @@ import ( | |||
) | |||
|
|||
func TestVariousLevels(t *testing.T) { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
level/level.go
Outdated
// ParseDefault returns a Value from a level string or the default if the level is not valid. | ||
func ParseDefault(level string, def Value) Value { | ||
if v, err := Parse(level); err != nil { | ||
return def | ||
} else { | ||
return v | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// ParseDefault returns a Value from a level string or the default if the level is not valid. | |
func ParseDefault(level string, def Value) Value { | |
if v, err := Parse(level); err != nil { | |
return def | |
} else { | |
return v | |
} | |
} | |
// ParseDefault calls Parse, and returns the default Value on error. | |
func ParseDefault(level string, def Value) Value { | |
if v, err := Parse(level); err == nil { | |
return v | |
} | |
return def | |
} |
level/level.go
Outdated
// Parse returns a Value from a level string. Allowed values are "debug", "info", "warn" and "error". | ||
// Levels are normalized via strings.TrimSpace and strings.ToLower | ||
func Parse(level string) (Value, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Parse returns a Value from a level string. Allowed values are "debug", "info", "warn" and "error". | |
// Levels are normalized via strings.TrimSpace and strings.ToLower | |
func Parse(level string) (Value, error) { | |
// Parse a string to it's corresponding level value. Valid strings are "debug", | |
// "info", "warn", and "error". Strings are normalized via strings.TrimSpace and | |
// strings.ToLower. | |
func Parse(level string) (Value, error) { |
level/doc.go
Outdated
// It's also possible to configure log level from a string (for instance from | ||
// a flag, environment variable or configuration file). | ||
// | ||
// fs := flag.NewFlagSet("example") | ||
// lvl := fs.String("log-level", "", `"debug", "info", "warn" or "error"`) | ||
// | ||
// var logger log.Logger | ||
// logger = log.NewLogfmtLogger(os.Stderr) | ||
// logger = level.NewFilter(logger, level.Allow(level.ParseDefault(*lvl, level.InfoValue()))) // <-- | ||
// logger = log.With(logger, "ts", log.DefaultTimestampUTC) | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// It's also possible to configure log level from a string (for instance from | |
// a flag, environment variable or configuration file). | |
// | |
// fs := flag.NewFlagSet("example") | |
// lvl := fs.String("log-level", "", `"debug", "info", "warn" or "error"`) | |
// | |
// var logger log.Logger | |
// logger = log.NewLogfmtLogger(os.Stderr) | |
// logger = level.NewFilter(logger, level.Allow(level.ParseDefault(*lvl, level.InfoValue()))) // <-- | |
// logger = log.With(logger, "ts", log.DefaultTimestampUTC) | |
// | |
// It's also possible to configure log level from a string. For instance, from | |
// a flag, environment variable or configuration file. | |
// | |
// fs := flag.NewFlagSet("myprogram") | |
// lvl := fs.String("log", "info", "debug, info, warn, error") | |
// | |
// var logger log.Logger | |
// logger = log.NewLogfmtLogger(os.Stderr) | |
// logger = level.NewFilter(logger, level.Allow(level.ParseDefault(*lvl, level.InfoValue()))) // <-- | |
// logger = log.With(logger, "ts", log.DefaultTimestampUTC) | |
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice addition to the package. Thanks for the contribution. I have a few nits, mostly style tweaks.
// Parse a string to its corresponding level value. Valid strings are "debug", | ||
// "info", "warn", and "error". Strings are normalized via strings.TrimSpace and | ||
// strings.ToLower. | ||
func Parse(level string) (Value, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the only returned error is invalid level, then maybe the second argument could simply be a boolran?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't more idiomatic to return an error? maybe in the future we might return different types of errors and we'll not need to introduce breaking changes in the API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, error
is a bit more future proof and IMO not meaningfully different for callers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. 🎉
@peterbourgon I think we're just waiting for you to sign off on this since you previously requested changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Márk Sági-Kazár <sagikazarmark@users.noreply.github.com>
Any update on this ? |
@peterbourgon Everyone involved has approved this PR except you. I think @mcosta74 has addressed all of your feedback. Please verify and approve if you agree. Thanks. |
Oops, sorry! I didn't know I was blocking. |
Thanks for the contribution @mcosta74 🎉 . |
you're welcome |
Hello, are you planning to release a new version of the package ? |
Sure, we can do that — |
My vote is |
Great. Assuming no objections, I'll tag a release in a day or so. |
not in my hands to make decisions but, since we added a feature following the SemVer guidelines I'd go for a |
@mcosta74 That is a perfectly correct reading of the SemVer guidelines. But my interpretation is that only strictly applies for major version >= 1. From semver.org:
So I think it is also correct for us to do whatever we want. I don't want to suggest we should start making lots of breaking changes. I consider this module highly stable, and the API of the core But if we stay on major version 0 per #24 (comment), then I think it is fair and appropriate to adapt the |
@ChrisHines Thanks for the explanation. Whit this in mind I agree that |
Hi @peterbourgon any ETA about the release? I'm using the "unreleased" version in a project we're building but I'd like to use a "released" one |
My apologies for the delay. v0.2.1 is now released. Please let me know if there are any issues. |
@peterbourgon, no problem. Thank you again for the products you give to the community. |
This PR allows to configure the allowed log levels from a string.
Closes: #18
Use Case:
Read the allowed log levels reading environment variable