Allow to configure allowed levels by string value

mcosta74 commented 2 years ago

This PR allows to configure the allowed log levels from a string.

Closes: #18

Use Case:

Read the allowed log levels reading environment variable

logger = level.NewFilter(logger, level.AllowByString(os.Getenv("LOG_LEVEL"), level.AllowInfo)) // <--

ChrisHines commented 2 years ago

@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?

sagikazarmark commented 2 years ago

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 AllowByString feels a bit weird. I'd go with something like Parse.

Other than these, I don't mind adding a few utility functions if they really help.

mcosta74 commented 2 years ago

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.

@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 ?

The name AllowByString feels a bit weird. I'd go with something like Parse.

I'm open to change the name if the approach is accepted

mcosta74 commented 2 years ago

I've applied the changes suggested by @sagikazarmark. Let me know about the final decision.

mcosta74 commented 2 years ago

hello @peterbourgon, I've applied all changes requested.

Proper grammar and spelling, please :) Let me know if you'd like suggestions.

Please let me know if you find better wording for the doc strings. I'm open to change.

mcosta74 commented 2 years ago

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 https://github.com/go-kit/log/pull/22#discussion_r846373837:

+// 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 https://github.com/go-kit/log/pull/22#discussion_r846373837, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4I7OKBLXWIID3Q36AU3CDVEBZYVANCNFSM5RYT3RZA . You are receiving this because you were mentioned.Message ID: @.***>

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 2145714702

Warning: 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.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

25 of 25 (100.0%) changed or added relevant lines in 1 file are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+2.5%) to 74.597%

Files with Coverage Reduction	New Missed Lines	%
level/level.go	1	99.11%
<!--	Total:	1		-->

Totals
Change from base Build 2065105133:	2.5%
Covered Lines:	555
Relevant Lines:	744

💛 - Coveralls

mcosta74 commented 2 years ago

I think I implemented the agreed API, if the code is code we can also squash the commits

ChrisHines commented 2 years ago

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.

mcosta74 commented 2 years ago

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.

ChrisHines commented 2 years ago

@peterbourgon I think we're just waiting for you to sign off on this since you previously requested changes.

mcosta74 commented 2 years ago

Any update on this ?

ChrisHines commented 2 years ago

@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.

peterbourgon commented 2 years ago

Oops, sorry! I didn't know I was blocking.

ChrisHines commented 2 years ago

Thanks for the contribution @mcosta74 🎉 .

mcosta74 commented 2 years ago

Thanks for the contribution @mcosta74 🎉 .

you're welcome

mcosta74 commented 2 years ago

Hello, are you planning to release a new version of the package ?

peterbourgon commented 2 years ago

Sure, we can do that — v0.2.1 or v0.3.0? I don't think we made any breaking changes, so my instinct is v0.2.1?

ChrisHines commented 2 years ago

My vote is v0.2.1 as well. No breaking changes, just a new feature.

peterbourgon commented 2 years ago

Great. Assuming no objections, I'll tag a release in a day or so.

mcosta74 commented 2 years ago

not in my hands to make decisions but, since we added a feature following the SemVer guidelines I'd go for a 0.3.0. I'll keep the last number for bug-fixes

ChrisHines commented 2 years ago

@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:

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

Version 1.0.0 defines the public API. The way in which the version number is incremented after this release is dependent on this public API and how it changes.

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 log package extremely stable. We haven't made any breaking changes that I recall in ~5 years and I don't want to start doing so.

But if we stay on major version 0 per https://github.com/go-kit/log/discussions/24#discussioncomment-2649183, then I think it is fair and appropriate to adapt the y semver number for "biggish" changes and the z number for "smallish" changes. I think it is more about signaling to consumers how much they should pay attention before upgrading.

mcosta74 commented 2 years ago

@ChrisHines Thanks for the explanation. Whit this in mind I agree that 0.2.1 is the right choice

mcosta74 commented 2 years ago

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

peterbourgon commented 2 years ago

My apologies for the delay. v0.2.1 is now released. Please let me know if there are any issues.

mcosta74 commented 2 years ago

@peterbourgon, no problem. Thank you again for the products you give to the community.

go-kit / log