Skip to content

kit/log/levels: Add enabled level - #322

Closed
hnakamur wants to merge 2 commits into
go-kit:masterfrom
hnakamur:add_enabled_level
Closed

kit/log/levels: Add enabled level#322
hnakamur wants to merge 2 commits into
go-kit:masterfrom
hnakamur:add_enabled_level

Conversation

@hnakamur

Copy link
Copy Markdown

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 Jul 18, 2016
@peterbourgon

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

3 participants