Integration with zap logger

[madhuravi]

The jaeger tracer defines a jaeger logger interface here https://github.com/uber/jaeger-client-go/blob/master/logger.go#L29.

Is there a plan to replace this with zap logger directly? For UberFx, I have a wrapper for jaeger logger that calls zap under the hood https://github.com/uber-go/fx/blob/master/tracing/tracer.go#L69. This also gets a bit messy because zap expects even arguments to the log message so I ended up appending a key string just to make it even.

@akshayjshah @sectioneight @anuptalwalkar @yurishkuro

[yurishkuro]

We do not plan on replacing Jaeger logger with Zap logger. The logging requirements of Jaeger client are very minimal, while zap is a relatively large library that jaeger client does not need to depend on. We can create an adapter either in some internal repo, or possibly in jaeger-lib.

[akshayjshah]

@madhuravi The wrapper you're using is unusual, and probably results in nonsensical output. Infof-style logs aren't structured, so there's little point in trying to fit them into a key-value interface. I suggest that you fmt.Sprintf the template and args and pass those as the zap message.

The sugared logger in zap itself supports Infof natively; as soon as the dev branch lands on master, you'll be able to remove this little bit of indirection.

[anuptalwalkar]

Thats nice to hear Akshay. we will definitely look forward to integrate sugar logger when it is ready in master. The purpose of wanting zap integration was to maintain and improve performance with Jaeger client. Since jaeger will be called on almost all requests, it will have benifits to services looking forward for high performance. From fx point of view, having to implement jaeger logger seems a bit unnecessary. What do you think?

[akshayjshah]

Let's wait and see if this is actually a problem. Have you benchmarked the Jaeger client integration in UberFX? Without benchmarks showing a significant problem, I don't think it's worth taking another dependency here.

[yurishkuro]

Also, Jaeger only logs when there are serious errors. Normal service operation has no logging from tracing.

[madhuravi]

Make sense. Thanks Yuri and Akshay. With jaeger's error logging, perf should not matter too much. To clarify the original ask, jaeger doesn't need to take a direct dependency on zap, if the jaeger.Logger interface matches the SugaredLogger/ZapLogger, we can directly pass in the logger instead of having to write wrappers around it.

Looking at SugaredLogger https://sourcegraph.com/github.com/uber-go/zap@2dfe9d24f8835055021d17ab3b9f869bfa503942/-/blob/sugar.go#L125:1-126:1

would it be possible to change the jaeger Logger "Error" method signature to match "Errorf" in the SugaredLogger? It would be more consistent to have Infof and Errorf even within the jaeger logger. What do you think Yuri?

[yurishkuro]

I prefer not to change the API of the logger, it's going to be just an annoying backwards incompatible change for people, without real benefits. But we can add this wrapper instead:

func WrapFormattingLogger(
    infof func(format string, v ...interface{}),
    errorf func(format string, v ...interface{})) jaeger.Logger

[akshayjshah]

Agreed with Yuri - there's no need to change Jaeger. As the integration point for multiple libraries, UberFX will naturally have a few small type adapters like this.

[madhuravi]

I understand. +1 for a WrapLogger on jaeger that everyone can use instead of writing their own adapters. @akshayjshah UberFx having adapters is a micro issue, it's only a few lines of code on our end. The bigger ideal is right now our set of tools and infra is pretty messy. As we converge and establish standards for standard libraries for each component., it's a great experience for engineers to have all libraries follow the same pattern. For eg. going forward if zap is the recommended logging library, this is reinforced as other libraries either directly use zap or follow the same pattern set by it so an adapter is not required. This also reduces the mental burden of having to learn and know many different types of loggers. This is idealistic and different libraries will always be in different levels of maturity but taking small steps towards that goal will just deliver a much better experience if we think about them as a cohesive suite of libraries that just work when used together.

[yurishkuro]

@black-adder given that we did make jaeger-lib a dependency of jaeger-client, we can now add the wrapper around zap to jaeger-lib.

[akshayjshah]

I'm refactoring the uberFX logging to work with zap 1.0; the difference between the two APIs is quite small, and I think the wrapper is actually better in UberFX. Up to you though.

[black-adder]

we'll do it in jaeger-lib, it makes more sense to have the zap wrapper specifically for jaeger to be in the jaeger repo, that way other people can use it without the uberfx depedency

[black-adder]

the zap.Logger wrapper is ready: https://github.com/uber/jaeger-client-go/releases/tag/v2.1.0

[akshayjshah]

Thanks for the quick turnaround, @black-adder. Left some comments on that diff; it will benefit from some small changes.