Repository: open-telemetry/opentelemetry-go-contrib
Pull request #2273 (2022-05-15 14:34:53)

feat: Acquire configuration from remote, when sampler is started 
feat: load the configuration from the remote as soon as the sampler is started and the type is remote, instead of waiting for the timer notification

codecov[bot]: # [Codecov](https://codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/2273?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) Report
> Merging [#2273](https://codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/2273?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) (28547ad) into [main](https://codecov.io/gh/open-telemetry/opentelemetry-go-contrib/commit/6feda8333ee4608534f51a0bcd3e2fe8b60ab900?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) (6feda83) will **increase** coverage by `0.0%`.
> The diff coverage is `100.0%`.

[![Impacted file tree graph](https://codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/2273/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/2273?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   #2273   +/-   ##
=====================================
  Coverage   73.6%   73.7%           
=====================================
  Files        141     141           
  Lines       6396    6398    +2     
=====================================
+ Hits        4712    4717    +5     
+ Misses      1542    1540    -2     
+ Partials     142     141    -1     
```


| [Impacted Files](https://codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/2273?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/2273/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% <100.0%> (+2.1%)` | :arrow_up: |

tttoad: > While this change in itself makes sense, it would be nice if it were tested as well.
I'm doing local testing, which will take a little time since jaeger-remote is not compatible with the old interface.

tttoad: > I meant adapting the unit tests to check this behavior change automatically.
Okay, I'll add the unit tests. I'll probably be able to submit it tomorrow or the next day.

tttoad: > I meant adapting the unit tests to check this behavior change automatically.

I have added the unit test,PTAL.
tttoad: @hanyuancheung  The CI error looks like the execution timed out? Maybe a retry will fix it.

Patch:
diff --git a/samplers/jaegerremote/sampler_remote.go b/samplers/jaegerremote/sampler_remote.go
index 83b62c603..763ad64bc 100644
--- a/samplers/jaegerremote/sampler_remote.go
+++ b/samplers/jaegerremote/sampler_remote.go
@@ -127,6 +127,8 @@ func (s *Sampler) pollController() {
 }
 
 func (s *Sampler) pollControllerWithTicker(ticker *time.Ticker) {
+	s.UpdateSampler()
+
 	for {
 		select {
 		case <-ticker.C:


Test patch:
diff --git a/samplers/jaegerremote/sampler_remote_test.go b/samplers/jaegerremote/sampler_remote_test.go
index ee7a0b067..528b7dad5 100644
--- a/samplers/jaegerremote/sampler_remote_test.go
+++ b/samplers/jaegerremote/sampler_remote_test.go
@@ -90,12 +90,19 @@ type testSamplingStrategyParser struct {
 func (p *testSamplingStrategyParser) Parse(response []byte) (interface{}, error) {
 	strategy := new(jaeger_api_v2.SamplingStrategyResponse)
 
-	if string(response) == "probabilistic" {
+	switch string(response) {
+	case "probabilistic":
 		strategy.StrategyType = jaeger_api_v2.SamplingStrategyType_PROBABILISTIC
 		strategy.ProbabilisticSampling = &jaeger_api_v2.ProbabilisticSamplingStrategy{
 			SamplingRate: 0.85,
 		}
 		return strategy, nil
+	case "rateLimiting":
+		strategy.StrategyType = jaeger_api_v2.SamplingStrategyType_RATE_LIMITING
+		strategy.RateLimitingSampling = &jaeger_api_v2.RateLimitingSamplingStrategy{
+			MaxTracesPerSecond: 100,
+		}
+		return strategy, nil
 	}
 
 	return nil, errors.New("unknown strategy test request")
@@ -280,6 +287,29 @@ func TestRemotelyControlledSampler_updateSampler(t *testing.T) {
 	}
 }
 
+func TestRemotelyControlledSampler_ImmediatelyUpdateOnStartup(t *testing.T) {
+	initSampler := newProbabilisticSampler(0.123)
+	fetcher := &testSamplingStrategyFetcher{response: []byte("rateLimiting")}
+	parser := new(testSamplingStrategyParser)
+	updaters := []samplerUpdater{new(probabilisticSamplerUpdater), new(rateLimitingSamplerUpdater)}
+	sampler := New(
+		"test",
+		WithMaxOperations(42),
+		WithOperationNameLateBinding(true),
+		WithInitialSampler(initSampler),
+		WithSamplingServerURL("my url"),
+		WithSamplingRefreshInterval(10*time.Minute),
+		withSamplingStrategyFetcher(fetcher),
+		withSamplingStrategyParser(parser),
+		withUpdaters(updaters...),
+	)
+	time.Sleep(100 * time.Millisecond) // waiting for s.pollController
+	sampler.Close()                    // stop pollController, avoid date race
+	s, ok := sampler.sampler.(*rateLimitingSampler)
+	assert.True(t, ok)
+	assert.Equal(t, float64(100), s.maxTracesPerSecond)
+}
+
 func TestRemotelyControlledSampler_multiStrategyResponse(t *testing.T) {
 	agent, sampler := initAgent(t)
 	defer agent.Close()