otelzap logger ignore WithOptions invocation

[ezh]

Example

l := otelzap.Ctx(ctx).WithOptions(zap.Fields(zap.String("FOO", "BAR")))
l.Info("Hello")

I expect that there will be FOO:BAR field.

The field is added to the zap encoder, but only for *zap.Logger. skipCaller is ignored:
https://github.com/uptrace/opentelemetry-go-extra/blob/main/otelzap/otelzap.go#L31

type Logger struct {
	*zap.Logger
	skipCaller *zap.Logger

After that code is using skipCaller for log output (https://github.com/uptrace/opentelemetry-go-extra/blob/main/otelzap/otelzap.go#L262).

IMO someone forgot to add to func (l *Logger) WithOptions(opts ...zap.Option) *Logger

	clone.skipCaller = l.skipCaller.WithOptions(opts...)

https://github.com/uptrace/opentelemetry-go-extra/blob/main/otelzap/otelzap.go#L63

[vmihailenco]

Yes, skipCaller must be updated - good catch.

But I guess WithOptions(zap.Fields(zap.String("FOO", "BAR")) won't fully work, because otelzap does not have any access to those fields and thus won't send it using OpenTelemetry protocol. You will only see those fields in your logs.

As a workaround, we could add an otelzap specific option so we have access to the fields:

l := otelzap.Ctx(ctx).WithFields(zap.String("FOO", "BAR"))

And then use WithOptions(zap.Fields(...)) to proxy those fields to Zap. What do you think?

[ezh]

Hey @vmihailenco . I don't think that introducing one more function to zap API would be a nice idea.

Why you wouldn't use otelzap WithOptions to collect extra fields? It is type-safe, has no reflection, and is transparent for the end-user. Please check https://github.com/ezh/opentelemetry-go-extra/commit/717c3dbeaca0b191e62cb6ff9352278f3cc6421c

func (l *Logger) WithOptions(opts ...zap.Option) *Logger {
	clone := *l
	extraFields := []zap.Field{}
	// zap.New side effect is extracting fields from .WithOptions(zap.Fields(...))
	zap.New(&fieldExtractorCore{extraFields: &extraFields}, opts...)
	print(fmt.Sprintf("%#v", extraFields))
	clone.Logger = l.Logger.WithOptions(opts...)

printf console output

DAP server listening at: 127.0.0.1:57568
[]zapcore.Field{zapcore.Field{Key:"foo", Type:0xf, Integer:0, String:"bar", Interface:interface {}(nil)}}
Detaching and terminating target process

If there will be other options, fieldExtractorCore will silently ignore them.

[vmihailenco]

Sorcery 😮 Indeed, with that trick we don't need a separate API. Very nice.

Could you open a PR with https://github.com/ezh/opentelemetry-go-extra/commit/717c3dbeaca0b191e62cb6ff9352278f3cc6421c? I can cleanup it if needed.

[ezh]

Will do

[vmihailenco]

@ezh gentle ping if you are still interested...