Replacing "level" field

[maxatome]

Hi,

logger := log.NewLogfmtLogger(log.NewSyncWriter(os.Stdout))
logger.Log("msg", "foo")

logger = level.Error(logger)
logger.Log("msg", "foo")

logger = level.Info(logger)
logger.Log("msg", "foo")

produces the following output playground:

msg=foo
level=error msg=foo
level=info level=error msg=foo

so level appears twice in last log.

What do you think about adding a new function in log:

func Replace(logger Logger, k, v interface{})

which works as log.With but replace key k if it already exists, in logger, in the new returned Logger instance?

So level.{Debug,Error,Info,Warn} can use it instead of log.WithPrefix as now.

Replace could be variadic as With* functions, but is there a real need?

If you are interested (variadic or not), I can provide a PR.

[ChrisHines]

This is a curious request. What is the use case?

In my experience it is not typical to store the leveled loggers in variables, but rather to treat them as one time use loggers like this:

level.Error(logger).Log("msg", "foo")

...

level.Info(logger).Log("msg", "foo")

Mutating an existing contextual logger is a non-starter. By design, the keyvals carried by contextual loggers are immutable. So Replace would need to return a copy of the provided logger with one or more keys modified.

If the idea is to save memory allocations I am not sure there are big gains to be had here since both log.WithPrefix and log.Replace would be allocating and copying a similar amount of data.

I am not excited about this idea, but could perhaps be persuaded by a good use case or some benchmarks showing a worthwhile improvement over the existing code.

I am willing to review a PR if you want to explore this idea, but will not promise it will be accepted. Fair warning that the bar will be high.

[peterbourgon]

The solution here may be to clarify this expectation in the docs.

[maxatome]

The goal here is not to mutate an existing logger, but just to avoid "level" key to be present several times in the resulting logger when we call level.{Debug,Error,Info,Warn}.

Take a library accepting a log.Logger as input to produce all its logging. Most of its logging messages are errors, and a very few can be issued from other levels. So we can consider Error is the default level in this case.

In such case and knowing this context, using logger.Log(…) most of the time, and level.Xxx(logger).Log(…) a few times to change the level, is lighter than always using level.Error(logger).Log(…) and never logger.Log(…) as all of our logs are "leveled".

That is why it could be useful to have such a Replace function to replace a field in the resulting logger (but not modifying the logger in-place.)

Note that it is a bit counterintuitive that a logger issued from level.Info(logger) cannot be passed to another level.Xxx() to produce a new logger with a different level.

[peterbourgon]

Note that it is a bit counterintuitive that a logger issued from level.Info(logger) cannot be passed to another level.Xxx() to produce a new logger with a different level.

Yes, we should make this intuitive. The output of e.g. level.Info should not in general be assigned to anything. If you have some code that does that, and consequently produces multiple level fields in the output, the code should be fixed.

In other words,

Take a library accepting a log.Logger as input to produce all its logging. Most of its logging messages are errors, and a very few can be issued from other levels. So we can consider Error is the default level in this case.

Package log is not intended to be used this way. It is expected that every log statement begin with level.Foo in the code.

[maxatome]

OK.