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

kit/log/levels: Add enabled level #322

Closed
wants to merge 2 commits into from

Conversation

hnakamur
Copy link

I read #252 and #269 and I think I prefer simpler approach for users to check a log level is enabled. So here it is.

The example code:

    logger, err := enabled.New(levels.New(log.NewLogfmtLogger(os.Stdout)), "warn")
    if err != nil {
        // This happens only when the level is invalid.
        // In this example, this never happens as the valid level "warn" is used.
        // In a real usage like reading a level from a command line option or a
        // config file, you should check this error.
        panic(err)
    }
    if logger.DebugEnabled() {
        logger.Debug().Log("msg", "hello")
    }
    if logger.WarnEnabled() {
        logger.With("context", "foo").Warn().Log("err", "error")
    }

The con is that it takes two more lines for if logger.XxxEnabled() { and }.
The pro is that it avoids the execution cost of the block for if when the level is disabled. I think this is important.

@hnakamur hnakamur changed the title Add enabled level kit/log/levels: Add enabled level Jul 18, 2016
@peterbourgon
Copy link
Member

Thanks for the contribution. Doing explicit checks before every error invocation is very verbose, I think a non-starter from a usability perspective. @ChrisHines any other thought?

@ChrisHines
Copy link
Member

I agree that this approach is not what I want to put in the kit/log/levels package. As I explained in #269 (comment), the performance gain of the if statement is relatively small.

I do think that this is a viable approach for applications that require optimal performance when logging is disabled. There is no reason an application or organization can't use the technique in their own code, but I don't think the need is common enough to promote this pattern in Go kit.

@peterbourgon
Copy link
Member

Thanks, Chris. And thanks, Hiroaki, for the submission! We won't be accepting it in the current form but I really appreciate the attention and effort. If you wanted to contribute to Go kit logging, here are some open PRs and issues that need attention:

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 this pull request may close these issues.

3 participants