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

log.With does not compose well with log.Levels #63

Closed
ChrisHines opened this Issue Jun 17, 2015 · 8 comments

Comments

Projects
None yet
2 participants
@ChrisHines
Member

ChrisHines commented Jun 17, 2015

Context

I would like the ability to do something like this:

var Log log.Logger

func HandleTask(taskID int) error {
    logger := log.With(Log, "task", taskID)
    logger.Info("event", "start")
    ...
    logger.Debug("event", "query", "query", query)
    if err := doQuery(query); err != nil {
        logger.Error("event", "query", "err", err)
        return err
    }
    ...
    logger.Info("event", "done")
}

The important aspect of my example is the re-use of logging context across multiple severity levels.

The problem

As it stands now, package log encourages us to implement log levels via multiple Loggers each with a different level context created by a call to log.With. Unfortunately this approach makes it cumbersome to create additional contextual loggers on top of the leveled loggers. For example:

var lvls = log.NewLevels()

func HandleTask(taskID int) error {
    dlog := log.With(lvls.Debug, "task", taskID)
    ilog := log.With(lvls.Info, "task", taskID)
    elog := log.With(lvls.Error, "task", taskID)

    ilog.Log("event", "start")
    ...
    dlog.Log("event", "query", "query", query)
    if err := doQuery(query); err != nil {
        elog.Log("event", "query", "err", err)
        return err
    }
    ...
    ilog.Log("event", "done")
}

In addition to being cumbersome, each call to log.With costs allocations.

Challenge

Can we find a good way to avoid the need for multiple calls to log.With when doing leveled logging?

@ChrisHines ChrisHines changed the title from `log.With` does not compose well with `log.Levels` to log.With does not compose well with log.Levels Jun 17, 2015

@peterbourgon

This comment has been minimized.

Show comment
Hide comment
@peterbourgon

peterbourgon Jun 17, 2015

Member

The obvious way is to canonize some levels into the interface definition, but I'd like to avoid that...

Member

peterbourgon commented Jun 17, 2015

The obvious way is to canonize some levels into the interface definition, but I'd like to avoid that...

@peterbourgon

This comment has been minimized.

Show comment
Hide comment
@peterbourgon

peterbourgon Jun 21, 2015

Member

We could define a new interface with canonized levels, with constructors that can wrap a logger...

Member

peterbourgon commented Jun 21, 2015

We could define a new interface with canonized levels, with constructors that can wrap a logger...

@ChrisHines

This comment has been minimized.

Show comment
Hide comment
@ChrisHines

ChrisHines Jun 21, 2015

Member

How would log.With compose with the new interface you propose?

Member

ChrisHines commented Jun 21, 2015

How would log.With compose with the new interface you propose?

@peterbourgon

This comment has been minimized.

Show comment
Hide comment
@peterbourgon

peterbourgon Jun 21, 2015

Member

Not well :) Could probably dream up a special-case solution, like

type Foo interface {
    Info(keyvals ...interface{})
    Warn(keyvals ...interface{})
    Error(keyvals ...interface{})

    With(keyvals ...interface{}) Foo
}

But that feels awkward, opposite of the existing API.

Member

peterbourgon commented Jun 21, 2015

Not well :) Could probably dream up a special-case solution, like

type Foo interface {
    Info(keyvals ...interface{})
    Warn(keyvals ...interface{})
    Error(keyvals ...interface{})

    With(keyvals ...interface{}) Foo
}

But that feels awkward, opposite of the existing API.

@peterbourgon

This comment has been minimized.

Show comment
Hide comment
@peterbourgon

peterbourgon Jun 21, 2015

Member

In general I'm struggling to embrace and extend log.With semantics...

Member

peterbourgon commented Jun 21, 2015

In general I'm struggling to embrace and extend log.With semantics...

@peterbourgon

This comment has been minimized.

Show comment
Hide comment
@peterbourgon

peterbourgon Jun 29, 2015

Member

@ChrisHines, what do you think about the approach described in this README?

Member

peterbourgon commented Jun 29, 2015

@ChrisHines, what do you think about the approach described in this README?

@ChrisHines

This comment has been minimized.

Show comment
Hide comment
@ChrisHines

ChrisHines Jul 3, 2015

Member

@peterbourgon What do you think of this?

func With(keyvals ...interface{}) Context {
    c := Context{}
    return c.With(keyvals...)
}

type Context []interface{}

func (c Context) With(keyvals ...interface{}) Context {
    n := len(c)
    return append(c[:n:n], keyvals...)
}

func (c Context) LogTo(logger Logger) error {
    ctx := c
    if containsValuer(ctx) {
        ctx = append(Context{}, ctx...)
        bindValues(ctx)
    }
    return logger.Log(ctx...)
}

The above code would get used like this:

ctx := log.With("a", 123)
...
ctx = ctx.With("b", "c")
...
ctx.With("msg", "message").LogTo(logger)
Member

ChrisHines commented Jul 3, 2015

@peterbourgon What do you think of this?

func With(keyvals ...interface{}) Context {
    c := Context{}
    return c.With(keyvals...)
}

type Context []interface{}

func (c Context) With(keyvals ...interface{}) Context {
    n := len(c)
    return append(c[:n:n], keyvals...)
}

func (c Context) LogTo(logger Logger) error {
    ctx := c
    if containsValuer(ctx) {
        ctx = append(Context{}, ctx...)
        bindValues(ctx)
    }
    return logger.Log(ctx...)
}

The above code would get used like this:

ctx := log.With("a", 123)
...
ctx = ctx.With("b", "c")
...
ctx.With("msg", "message").LogTo(logger)
@peterbourgon

This comment has been minimized.

Show comment
Hide comment
@peterbourgon

peterbourgon Jul 4, 2015

Member

On first read I resisted the idea of manipulating a first-order Context object and logging it "to" a destination, but after rolling it around a little bit, it feels like it could reasonable. Any reason not to s/LogTo/Log/? Thinking out loud a little bit: we're flying pretty close to an encoder or writer API, I wonder if it doesn't make sense to make that more explicit, with something like

type Logger interface {
    Log(Context) error
}

func (c Context) Log(dst Logger) error {
    return dst.Log(c)
}

...which raises the question of where best to put the valuer stuff. It feels strange to have it only occur when you context.Log(logger), and get ignored if you logger.Log(context). Maybe your version, having the logger take a keyvals, makes that more explicit.

Member

peterbourgon commented Jul 4, 2015

On first read I resisted the idea of manipulating a first-order Context object and logging it "to" a destination, but after rolling it around a little bit, it feels like it could reasonable. Any reason not to s/LogTo/Log/? Thinking out loud a little bit: we're flying pretty close to an encoder or writer API, I wonder if it doesn't make sense to make that more explicit, with something like

type Logger interface {
    Log(Context) error
}

func (c Context) Log(dst Logger) error {
    return dst.Log(c)
}

...which raises the question of where best to put the valuer stuff. It feels strange to have it only occur when you context.Log(logger), and get ignored if you logger.Log(context). Maybe your version, having the logger take a keyvals, makes that more explicit.

guycook pushed a commit to codelingo/kit that referenced this issue Oct 12, 2016

Merge pull request #76 from go-kit/gophercon-log-api
New API for package log (post-GopherCon)

Fixes #63 and some usability concerns.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment