Add logging options to jaeger.sampler

[tttoad]

Add logging options to jaeger.sampler.
#2259

[codecov]

Codecov Report

Merging #2566 (5abca4b) into main (e97d789) will increase coverage by 0.0%.
The diff coverage is 77.7%.

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #2566   +/-   ##
=====================================
  Coverage   69.6%   69.6%           
=====================================
  Files        147     147           
  Lines       6785    6790    +5     
=====================================
+ Hits        4725    4730    +5     
  Misses      1944    1944           
  Partials     116     116           

Impacted Files	Coverage Δ
samplers/jaegerremote/sampler_remote.go	87.4% <50.0%> (ø)
samplers/jaegerremote/sampler_remote_options.go	100.0% <100.0%> (ø)

[dmathieu]

How about using the logr interface instead of a custom-made logger that will only be used here?

[tttoad]

How about using the logr interface instead of a custom-made logger that will only be used here?

While only using the Error interface...
The level parameter in logr.Info() makes me wonder.
The definition of log.level used in the current SDK may be different from the user's log.level.
Is there a specification or recommendation for the use of the level parameter?
The user may understand the meaning of "TRACE,DEBUG,INFO,WARN", but not level=[1,2,3...].
In the SDK, maybe using the "trace,debug,info,warn" interface is a better choice.

[Aneurysm9]

I second the recommendation to use Logr. I do not want to see each and every instrumentation creating and maintaining a logging interface. This is something that we should do more to support at the API level, but in the absence of specification I think that adding a WithLogger(logr.Logger) Option to this instrumentation will allow the end user to decide which logger to use and how to configure it without requiring that we take on a significant maintenance overhead.

[tttoad]

I second the recommendation to use Logr. I do not want to see each and every instrumentation creating and maintaining a logging interface. This is something that we should do more to support at the API level, but in the absence of specification I think that adding a WithLogger(logr.Logger) Option to this instrumentation will allow the end user to decide which logger to use and how to configure it without requiring that we take on a significant maintenance overhead.

I agree to use a unified logging interface...
If we choose to use logr, we need a uniform specification on how to set the level.
"log.info" is not used in jaeger.remote, so I'll try using logr`.

[Aneurysm9]

I'm not sure I understand the concern. The logr.Logger interface provides Info() and Error() methods, which should be sufficient for this use. If more granularity is required then the V() interface can be used and the OTel log data model provides a mapping of severity levels to severity names.

[tttoad]

I'm not sure I understand it correctly.
logr.Logger is a struct.When using the WithLogger(logr.Logger) option, the end user needs to implement the logr.LogSink interface.
When using the WithLogger(logr.Logger) option, the end user needs to implement the logr.LogSink interface.
The LogSink interface is as follows.

type LogSink interface {
	Init(info RuntimeInfo)
	Enabled(level int) bool
	Info(level int, msg string, keysAndValues ...interface{})
	Error(err error, msg string, keysAndValues ...interface{})
	WithValues(keysAndValues ...interface{}) LogSink
	WithName(name string) LogSink
}

The end-user needs to clearly understand the v-level. (It is possible that the user also uses logr, but the definition of v-level is different).
https://github.com/go-logr/logr#what-if-my-v-levels-differ-between-libraries
I prefer to provide the following interface:

type Logger interface {
	Info(msg string,values ...interface{})
        Error(err error,msg string,values ...interface{}) 
	Debug(msg string,values ...interface{})
        .....
}

[tttoad]

@dmathieu @Aneurysm9 PTAL. Should I add changelog or use skiplog label？

[tttoad]

@MrAlias I think I addressed all of your concerns. PTAL

[MrAlias]

Please restore the dependabot configuration: #2566 (comment)

[tttoad]

Please restore the dependabot configuration: #2566 (comment)

Sorry, I forgot to commit it. It has been restored. PTAL