Repository: open-telemetry/opentelemetry-go-contrib
Pull request #2566 (2022-07-09 19:27:59)

Add logging options to jaeger.sampler
Add logging options to jaeger.sampler.
#2259 

codecov[bot]: # [Codecov](https://codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/2566?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) Report
> Merging [#2566](https://codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/2566?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) (056fced) into [main](https://codecov.io/gh/open-telemetry/opentelemetry-go-contrib/commit/e18a4d6796680c2e6937f52cc863c53d1d1541e1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) (e18a4d6) will **increase** coverage by `0.2%`.
> The diff coverage is `96.8%`.

[![Impacted file tree graph](https://codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/2566/graphs/tree.svg?width=650&height=150&src=pr&token=P6F3W9WA7Q&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)](https://codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/2566?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)

```diff
@@           Coverage Diff           @@
##            main   #2566     +/-   ##
=======================================
+ Coverage   74.3%   74.5%   +0.2%     
=======================================
  Files        144     145      +1     
  Lines       6569    6629     +60     
=======================================
+ Hits        4883    4943     +60     
  Misses      1543    1543             
  Partials     143     143             
```


| [Impacted Files](https://codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/2566?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | Coverage Δ | |
|---|---|---|
| [samplers/jaegerremote/sampler\_remote.go](https://codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/2566/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-c2FtcGxlcnMvamFlZ2VycmVtb3RlL3NhbXBsZXJfcmVtb3RlLmdv) | `87.4% <33.3%> (ø)` | |
| [samplers/jaegerremote/logger.go](https://codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/2566/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-c2FtcGxlcnMvamFlZ2VycmVtb3RlL2xvZ2dlci5nbw==) | `100.0% <100.0%> (ø)` | |
| [samplers/jaegerremote/sampler\_remote\_options.go](https://codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/2566/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-c2FtcGxlcnMvamFlZ2VycmVtb3RlL3NhbXBsZXJfcmVtb3RlX29wdGlvbnMuZ28=) | `100.0% <100.0%> (ø)` | |

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](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#displaying-severity).
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: https://github.com/open-telemetry/opentelemetry-go-contrib/pull/2566#discussion_r1013026356
tttoad: > Please restore the dependabot configuration: [#2566 (comment)](https://github.com/open-telemetry/opentelemetry-go-contrib/pull/2566#discussion_r1013026356)

Sorry, I forgot to commit it. It has been restored.  PTAL

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4576559b9..95d8c15a9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -10,6 +10,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ### Added
 
+- The `WithLogger` option to `go.opentelemetry.io/contrib/samplers/jaegerremote` to allow users to pass a `logr.Logger` and have operations logged. (#2566)
 - Add the `messaging.url` & `messaging.system` attributes to all appropriate SQS operations in the `go.opentelemetry.io/contrib/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws` package. (#2879)
 
 ## [1.11.1/0.36.4/0.5.2]
diff --git a/samplers/jaegerremote/go.mod b/samplers/jaegerremote/go.mod
index b0d77e8d8..96440823c 100644
--- a/samplers/jaegerremote/go.mod
+++ b/samplers/jaegerremote/go.mod
@@ -3,9 +3,9 @@ module go.opentelemetry.io/contrib/samplers/jaegerremote
 go 1.18
 
 require (
+	github.com/go-logr/logr v1.2.3
 	github.com/gogo/protobuf v1.3.2
 	github.com/stretchr/testify v1.8.1
-	go.opentelemetry.io/otel v1.11.1
 	go.opentelemetry.io/otel/sdk v1.11.1
 	go.opentelemetry.io/otel/trace v1.11.1
 	google.golang.org/genproto v0.0.0-20211208223120-3a66f561d7aa
@@ -13,9 +13,9 @@ require (
 
 require (
 	github.com/davecgh/go-spew v1.1.1 // indirect
-	github.com/go-logr/logr v1.2.3 // indirect
 	github.com/go-logr/stdr v1.2.2 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
+	go.opentelemetry.io/otel v1.11.1 // indirect
 	golang.org/x/sys v0.0.0-20220919091848-fb04ddd9f9c8 // indirect
 	google.golang.org/protobuf v1.27.1 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
diff --git a/samplers/jaegerremote/sampler_remote.go b/samplers/jaegerremote/sampler_remote.go
index b1138d311..c6aed2311 100644
--- a/samplers/jaegerremote/sampler_remote.go
+++ b/samplers/jaegerremote/sampler_remote.go
@@ -27,7 +27,6 @@ import (
 	"time"
 
 	jaeger_api_v2 "go.opentelemetry.io/contrib/samplers/jaegerremote/internal/proto-gen/jaeger-idl/proto/api_v2"
-	"go.opentelemetry.io/otel"
 	"go.opentelemetry.io/otel/sdk/trace"
 )
 
@@ -105,7 +104,7 @@ func (s *Sampler) ShouldSample(p trace.SamplingParameters) trace.SamplingResult
 // go-routines it may have started.
 func (s *Sampler) Close() {
 	if swapped := atomic.CompareAndSwapInt64(&s.closed, 0, 1); !swapped {
-		otel.Handle(fmt.Errorf("repeated attempt to close the sampler is ignored"))
+		s.logger.Info("repeated attempt to close the sampler is ignored")
 		return
 	}
 
@@ -151,12 +150,12 @@ func (s *Sampler) setSampler(sampler trace.Sampler) {
 func (s *Sampler) UpdateSampler() {
 	res, err := s.samplingFetcher.Fetch(s.serviceName)
 	if err != nil {
-		// log.Printf("failed to fetch sampling strategy: %v", err)
+		s.logger.Error(err, "failed to fetch sampling strategy")
 		return
 	}
 	strategy, err := s.samplingParser.Parse(res)
 	if err != nil {
-		// log.Printf("failed to parse sampling strategy response: %v", err)
+		s.logger.Error(err, "failed to parse sampling strategy response")
 		return
 	}
 
@@ -164,7 +163,7 @@ func (s *Sampler) UpdateSampler() {
 	defer s.Unlock()
 
 	if err := s.updateSamplerViaUpdaters(strategy); err != nil {
-		// c.logger.Infof("failed to handle sampling strategy response %+v. Got error: %v", res, err)
+		s.logger.Error(err, "failed to handle sampling strategy response", "response", res)
 		return
 	}
 }
diff --git a/samplers/jaegerremote/sampler_remote_options.go b/samplers/jaegerremote/sampler_remote_options.go
index 2fb293646..31f95fb5e 100644
--- a/samplers/jaegerremote/sampler_remote_options.go
+++ b/samplers/jaegerremote/sampler_remote_options.go
@@ -19,6 +19,8 @@ package jaegerremote // import "go.opentelemetry.io/contrib/samplers/jaegerremot
 import (
 	"time"
 
+	"github.com/go-logr/logr"
+
 	"go.opentelemetry.io/otel/sdk/trace"
 )
 
@@ -30,6 +32,7 @@ type config struct {
 	samplingParser          samplingStrategyParser
 	updaters                []samplerUpdater
 	posParams               perOperationSamplerParams
+	logger                  logr.Logger
 }
 
 // newConfig returns an appropriately configured config.
@@ -48,6 +51,7 @@ func newConfig(options ...Option) config {
 			MaxOperations:            defaultSamplingMaxOperations,
 			OperationNameLateBinding: defaultSamplingOperationNameLateBinding,
 		},
+		logger: logr.Discard(),
 	}
 	for _, option := range options {
 		option.apply(&c)
@@ -112,6 +116,13 @@ func WithSamplingRefreshInterval(samplingRefreshInterval time.Duration) Option {
 	})
 }
 
+// WithLogger configures the sampler to log operation and debug information with logger.
+func WithLogger(logger logr.Logger) Option {
+	return optionFunc(func(c *config) {
+		c.logger = logger
+	})
+}
+
 // samplingStrategyFetcher creates a Option that initializes sampling strategy fetcher.
 func withSamplingStrategyFetcher(fetcher samplingStrategyFetcher) Option {
 	return optionFunc(func(c *config) {


Test patch:
diff --git a/samplers/jaegerremote/sampler_remote_test.go b/samplers/jaegerremote/sampler_remote_test.go
index 528b7dad5..7748b751f 100644
--- a/samplers/jaegerremote/sampler_remote_test.go
+++ b/samplers/jaegerremote/sampler_remote_test.go
@@ -23,6 +23,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/go-logr/logr/testr"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
@@ -112,6 +113,7 @@ func TestRemoteSamplerOptions(t *testing.T) {
 	initSampler := newProbabilisticSampler(0.123)
 	fetcher := new(fakeSamplingFetcher)
 	parser := new(samplingStrategyParserImpl)
+	logger := testr.New(t)
 	updaters := []samplerUpdater{new(probabilisticSamplerUpdater)}
 	sampler := New(
 		"test",
@@ -123,6 +125,7 @@ func TestRemoteSamplerOptions(t *testing.T) {
 		withSamplingStrategyFetcher(fetcher),
 		withSamplingStrategyParser(parser),
 		withUpdaters(updaters...),
+		WithLogger(logger),
 	)
 	assert.Equal(t, 42, sampler.posParams.MaxOperations)
 	assert.True(t, sampler.posParams.OperationNameLateBinding)
@@ -132,6 +135,7 @@ func TestRemoteSamplerOptions(t *testing.T) {
 	assert.Same(t, fetcher, sampler.samplingFetcher)
 	assert.Same(t, parser, sampler.samplingParser)
 	assert.EqualValues(t, sampler.updaters[0], &perOperationSamplerUpdater{MaxOperations: 42, OperationNameLateBinding: true})
+	assert.Equal(t, logger, sampler.logger)
 }
 
 func TestRemoteSamplerOptionsDefaults(t *testing.T) {