Use log Fields in ctxlog instead of logger

[carlosms]

Fix #427.

I couldn't find a way to use go-log for the fields merge.

[smacker]

huh? Why not just:

for k, v := range prevFields {
    newFields[k] = v
}

?

[carlosms]

Because of things like this, and any other future corner cases
sirupsen/logrus@08e8d65#diff-32f22e4789509e4288e8ee93f532fbd0

We could do the for you posted and trust that log.New will sanitize any problems when we need the logger. But it seemed safer to also store the log.Fields already sanitized. And logrus is already a dependency, so I didn't give it much thought and just delegated the merge.

[smacker]

but the log.New also runs logrus.With internally that does sanitize. So we repeat the work.

And if later go-log change the backend from logrus to something else which don't have these constraints on the field types?

[carlosms]

Right now it's very localized and the for will work fine, but maybe in the future having different log fields in the context value and the logger becomes a problem for whatever reason. For #357 and #248 we'll have to create a method to access to the ctxlog log fields directly instead of the logger.

I don't see any downside to use the available sanitizing. If/when go-log changes the backend, we can just as easily change it here.

[smacker]

I see breaking an abstraction here. Which I'm quite strongly against of.

So I have 2 points against using logrus here:

1. Breaking an abstraction. What is the point of go-log lib if we lock ourselves on particular implementation details of it (logrus). Let's use logrus directly then.
2. This code in logrus exists only because of implementation details of JSON formatter. And even with it it doesn't cover all cases. For example, if we occasionally pass struct with a function inside, code of Log fields are lost when events are sent to the queue #357 might still fail (I didn't read implementation of msgpack.Marshal inside go-queue). But default log in dev mode will work.

If you want to have the same sanitazion in this place as in logrus I would prefer if you just copy-past it then using logrus directly.

[carlosms]

We are breaking abstraction anyway. go-log does not allow access to the logger log.Fields, so what we are doing here is making assumptions of the internals in any case.

I am assuming go-log uses logrus. But you are also assuming that the fields merge is done replacing older values. Maybe tomorrow go-log or logrus makes a change and now With(fields), when it finds a repeated log field key, joins the previous and new values with a ,.

Even with the small snippet you propose:

for k, v := range prevFields {
    newFields[k] = v
}

There is a bug. In case of repeated keys go-log (via logrus) keeps the new value. With that snippet the older value would be kept instead. So we take the log fields merge problem -even if it's such a small problem-, already solved by a library, and repeat the work ourselves.

Anyway I don't agree with you but I aslo don't see the point of keeping discussing such a small thing. Change done in 0b4202b

[smacker]

log.Fields is a public interface of go-log. It's absolutely fine to use it.

There is no bug. It's our code that written in this or that way. go-log doesn't provide any public API for merging fields so it's on us. Our code doesn't have to repeat how go-log works with Fields.