Skip to content

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