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

Level logging difficult to work with #131

Closed
kris-runzer opened this Issue Sep 22, 2015 · 8 comments

Comments

Projects
None yet
4 participants
@kris-runzer

kris-runzer commented Sep 22, 2015

logger := log.NewLogfmtLogger(os.Stderr)
logger = log.NewContext(logger).With("caller", log.DefaultCaller)
lvlLogger := levels.New(logger)
lvlLogger.Info("hi", "there")

In the output you will get: caller=levels.go:72

This is fixed by using log.Caller(4) instead of log.DefaultCaller but makes passing and extending the logger harder than it should be.

@kris-runzer

This comment has been minimized.

Show comment
Hide comment
@kris-runzer

kris-runzer Sep 22, 2015

Additionally, an issue we are running into is we want to use levels.Levels​ everywhere... but a lot of the go-kit boilerplate works on using ​log.Logger​ (httptransport.Server/Client for example). We can't get back from a ​levels.Logger​ to a log.Logger very easily; we have to pass around multiple variations of the logger.

kris-runzer commented Sep 22, 2015

Additionally, an issue we are running into is we want to use levels.Levels​ everywhere... but a lot of the go-kit boilerplate works on using ​log.Logger​ (httptransport.Server/Client for example). We can't get back from a ​levels.Logger​ to a log.Logger very easily; we have to pass around multiple variations of the logger.

@kris-runzer kris-runzer changed the title from Level logging prints incorrect caller information to Level logging difficult to work with Sep 22, 2015

@ChrisHines

This comment has been minimized.

Show comment
Hide comment
@ChrisHines

ChrisHines Sep 22, 2015

Member

I think (but haven't tried it yet) we can resolve these tensions if we change the Levels API to return log.Loggers instead of doing the logging directly.

For example if we change:

// Debug logs a debug event along with keyvals.
func (l Levels) Debug(keyvals ...interface{}) error {
    return l.ctx.WithPrefix(l.levelKey, l.debugValue).Log(keyvals...)
}

to

// Debug returns a debug level Logger.
func (l Levels) Debug() log.Logger {
    return l.ctx.WithPrefix(l.levelKey, l.debugValue)
}

The stack depth will again be correct and you can convert any specific level into a log.Logger and pass it to code expecting a log.Logger easily.

Usages would change from:

lvls.Debug("k1", v1, "k2", v2)

to

lvls.Debug().Log("k1", v1, "k2", v2)

Which is not quite as clean, but I don't find it offensive given the power it brings. Thoughts?

Member

ChrisHines commented Sep 22, 2015

I think (but haven't tried it yet) we can resolve these tensions if we change the Levels API to return log.Loggers instead of doing the logging directly.

For example if we change:

// Debug logs a debug event along with keyvals.
func (l Levels) Debug(keyvals ...interface{}) error {
    return l.ctx.WithPrefix(l.levelKey, l.debugValue).Log(keyvals...)
}

to

// Debug returns a debug level Logger.
func (l Levels) Debug() log.Logger {
    return l.ctx.WithPrefix(l.levelKey, l.debugValue)
}

The stack depth will again be correct and you can convert any specific level into a log.Logger and pass it to code expecting a log.Logger easily.

Usages would change from:

lvls.Debug("k1", v1, "k2", v2)

to

lvls.Debug().Log("k1", v1, "k2", v2)

Which is not quite as clean, but I don't find it offensive given the power it brings. Thoughts?

@kris-runzer

This comment has been minimized.

Show comment
Hide comment
@kris-runzer

kris-runzer Sep 22, 2015

That would make this more powerful. Only thing I'd still question is that i would pass lvls.Debug() to httptransport.Server{} as an example.

The logging level may not make sense for all logging; unless there is a strict contract that only errors are logged by these instances? Then i can just pass lvls.Error()

Is it not possible to make the levels.Levels have a .Log(...) or just expose it's underlying log.Logger?

kris-runzer commented Sep 22, 2015

That would make this more powerful. Only thing I'd still question is that i would pass lvls.Debug() to httptransport.Server{} as an example.

The logging level may not make sense for all logging; unless there is a strict contract that only errors are logged by these instances? Then i can just pass lvls.Error()

Is it not possible to make the levels.Levels have a .Log(...) or just expose it's underlying log.Logger?

@ChrisHines

This comment has been minimized.

Show comment
Hide comment
@ChrisHines

ChrisHines Sep 22, 2015

Member

Perhaps, although I would first suggest that code should just use the Logger passed to levels.New in the first place? Wrapping and unwrapping smells bad, but maybe it is the most pragmatic solution.

Member

ChrisHines commented Sep 22, 2015

Perhaps, although I would first suggest that code should just use the Logger passed to levels.New in the first place? Wrapping and unwrapping smells bad, but maybe it is the most pragmatic solution.

@kris-runzer

This comment has been minimized.

Show comment
Hide comment
@kris-runzer

kris-runzer Sep 23, 2015

So I've had the night to think about this and I'm leaning towards lvls.Debug().Log("k1", v1, "k2", v2) being a nice solution to this problem.

As for the problem I'm currently facing; I did a refactor of code organization that has allowed me to easily have levels.Levels on the stuff I write and log.Logger when I'm dealing with go-kit. Since all the go-kit loggers are "error loggers" (correct me if I'm wrong) my solution to get levels there was to just use a custom .With("level", "error") on those.

I'm happy with this solution but I feel that a better approach does exists although I can't place my finger on what it should be at this time.

kris-runzer commented Sep 23, 2015

So I've had the night to think about this and I'm leaning towards lvls.Debug().Log("k1", v1, "k2", v2) being a nice solution to this problem.

As for the problem I'm currently facing; I did a refactor of code organization that has allowed me to easily have levels.Levels on the stuff I write and log.Logger when I'm dealing with go-kit. Since all the go-kit loggers are "error loggers" (correct me if I'm wrong) my solution to get levels there was to just use a custom .With("level", "error") on those.

I'm happy with this solution but I feel that a better approach does exists although I can't place my finger on what it should be at this time.

@sevein

This comment has been minimized.

Show comment
Hide comment
@sevein

sevein Oct 4, 2015

Contributor

Nothing to add other than saying that I've run into the same issues and the solution suggested would work for me.

Contributor

sevein commented Oct 4, 2015

Nothing to add other than saying that I've run into the same issues and the solution suggested would work for me.

@TommyM

This comment has been minimized.

Show comment
Hide comment
@TommyM

TommyM Nov 26, 2015

Is there any news on this? The proposed lvls.Debug().Log("k1", v1, "k2", v2) would solve the problems I'm having with the level logging nicely.

TommyM commented Nov 26, 2015

Is there any news on this? The proposed lvls.Debug().Log("k1", v1, "k2", v2) would solve the problems I'm having with the level logging nicely.

@ChrisHines

This comment has been minimized.

Show comment
Hide comment
@ChrisHines

ChrisHines Nov 28, 2015

Member

There seems to be general agreement with the proposed solution, and no objections, so I have created a PR to make the suggested change. Those interested, please review.

Member

ChrisHines commented Nov 28, 2015

There seems to be general agreement with the proposed solution, and no objections, so I have created a PR to make the suggested change. Those interested, please review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment