[feat] Support periodic refresh of sampling strategies #2188

defool · 2020-04-19T11:53:36Z

PR for #2186

Signed-off-by: defool defool@foxmail.com

Which problem is this PR solving?

Add CLI option to refresh sampling strategies file.

Short description of the changes

Add --strategies-reload-interval option for updating strategies periodically.

codecov · 2020-04-19T15:57:01Z

Codecov Report

Merging #2188 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2188      +/-   ##
==========================================
+ Coverage   96.14%   96.16%   +0.01%     
==========================================
  Files         219      219              
  Lines       10586    10628      +42     
==========================================
+ Hits        10178    10220      +42     
+ Misses        353      351       -2     
- Partials       55       57       +2

Impacted Files	Coverage Δ
plugin/sampling/strategystore/static/constants.go	`100.00% <100.00%> (ø)`
plugin/sampling/strategystore/static/options.go	`100.00% <100.00%> (ø)`
...in/sampling/strategystore/static/strategy_store.go	`100.00% <100.00%> (ø)`
cmd/query/app/server.go	`91.78% <0.00%> (-2.74%)`	⬇️
plugin/storage/badger/spanstore/reader.go	`96.79% <0.00%> (ø)`
...lugin/sampling/strategystore/adaptive/processor.go	`100.00% <0.00%> (+0.79%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b99114e...b9ca39b. Read the comment docs.

yurishkuro

Thank you for the PR. Note that it does not address #2186 as that one is about an external API (e.g. JSON endpoint). But periodic reloading is a good feature.

yurishkuro · 2020-04-19T17:17:43Z

plugin/sampling/strategystore/static/factory.go

+	if err != nil {
+		return ss, err
+	}
+	if s, ok := ss.(strategystore.StrategyUpdater); f.options.ReloadInterval > 0 && ok {


strategystore.StrategyUpdater interface is unnecessary at this time. This factory is already coupled with static storage, so it already knows that it supports Update method.

Also, it is better to implement the timer loop inside that storage, not in the factory.

yurishkuro · 2020-04-19T17:18:36Z

plugin/sampling/strategystore/static/factory.go

+	for {
+		select {
+		case <-ticker.C:
+			if currBytes, err := ioutil.ReadFile(filepath.Clean(filePath)); err == nil {


Suggested change

if currBytes, err := ioutil.ReadFile(filepath.Clean(filePath)); err == nil {

currBytes, err := ioutil.ReadFile(filepath.Clean(filePath))

if err != nil {

log

continue

}

yurishkuro · 2020-04-19T17:27:54Z

plugin/sampling/strategystore/static/factory_test.go

+
+func TestAutoReload(t *testing.T) {
+	// copy from fixtures/strategies.json
+	srcFile, dstFile := "fixtures/strategies.json", "fixtures/strategies_for_reload.json"


either use ioutil.TempFile() or add strategies_for_reload.json to fixtures/.gitignore

yurishkuro · 2020-04-19T17:28:25Z

plugin/sampling/strategystore/static/factory_test.go

+
+	// Test reading strategies from a file
+	//ctx, canf := context.WithCancel(context.Background())
+	//defer canf()


yurishkuro · 2020-04-19T17:30:03Z

plugin/sampling/strategystore/static/options.go

 }

 // AddFlags adds flags for Options
 func AddFlags(flagSet *flag.FlagSet) {
 	flagSet.String(samplingStrategiesFile, "", "The path for the sampling strategies file in JSON format. See sampling documentation to see format of the file")
+	flagSet.Duration(samplingStrategiesReloadInterval, 0, "Reload interval to check and reload sampling strategies file. Zero value means no checks (default 0s)")


Suggested change

flagSet.Duration(samplingStrategiesReloadInterval, 0, "Reload interval to check and reload sampling strategies file. Zero value means no checks (default 0s)")

flagSet.Duration(samplingStrategiesReloadInterval, 0, "Reload interval to check and reload sampling strategies file. Zero value means no reloading")

the default value is printed automatically

yurishkuro · 2020-04-19T18:17:05Z

cmd/collector/app/sampling/strategystore/interface.go

@@ -23,3 +23,9 @@ type StrategyStore interface {
 	// GetSamplingStrategy retrieves the sampling strategy for the specified service.
 	GetSamplingStrategy(serviceName string) (*sampling.SamplingStrategyResponse, error)
 }
+
+// StrategyUpdater updates sampling strategies.


not needed, see comments in the factory

yurishkuro · 2020-04-19T18:18:23Z

plugin/sampling/strategystore/static/factory_test.go

+
+	f := NewFactory()
+	v, command := config.Viperize(f.AddFlags)
+	_ = command.ParseFlags([]string{"--sampling.strategies-file=" + dstFile, "--sampling.strategies-reload-interval=50ms"})


Suggested change

_ = command.ParseFlags([]string{"--sampling.strategies-file=" + dstFile, "--sampling.strategies-reload-interval=50ms"})

_ = command.ParseFlags([]string{

"--sampling.strategies-file=" + dstFile,

"--sampling.strategies-reload-interval=50ms",

})

yurishkuro · 2020-04-19T18:21:33Z

plugin/sampling/strategystore/static/factory_test.go

+	require.NoError(t, err)
+
+	// wait for reload
+	time.Sleep(time.Millisecond * 50 * 2)


Sleeps are not reliable in CI. Please use a loop

for i:=0; i < 100; i++ { // wait up to 1sec if condition { break } time.Sleep(10ms) } assert

yurishkuro · 2020-04-19T18:22:30Z

plugin/sampling/strategystore/static/factory_test.go

+	//ctx, canf := context.WithCancel(context.Background())
+	//defer canf()
+
+	store, err := f.CreateStrategyStore()


This leaks a goroutine. There needs to be a way to shut down cleanly.

if we move reloading loop into storage, the test should move there as well (factory should only test parsing the options)

yurishkuro · 2020-04-19T18:23:29Z

plugin/sampling/strategystore/static/strategy_store_test.go

@@ -79,7 +78,7 @@ func TestPerOperationSamplingStrategies(t *testing.T) {
 	os := s.OperationSampling
 	assert.EqualValues(t, os.DefaultSamplingProbability, 0.8)
 	require.Len(t, os.PerOperationStrategies, 4)
-	fmt.Println(os)
+	//fmt.Println(os)


remove altogether

Thank you for suggestions, I will remove it in the later revision.

Signed-off-by: defool <defool@foxmail.com>

plugin/sampling/strategystore/static/strategy_store.go

yurishkuro · 2020-04-20T23:46:30Z

plugin/sampling/strategystore/static/strategy_store.go

+}
+
+// StopUpdateStrategy stops updating the strategy
+func (h *strategyStore) StopUpdateStrategy() {


usually we call this Close()

yurishkuro · 2020-04-20T23:49:31Z

plugin/sampling/strategystore/static/strategy_store.go

+		return fmt.Errorf("failed to unmarshal strategies: %w", err)
+	}
+	h.parseStrategies(&strategies)
+	h.logger.Info("Updated strategy:" + string(bytes))


Suggested change

h.logger.Info("Updated strategy:" + string(bytes))

h.logger.Info("Updated sampling strategies:" + string(bytes))

yurishkuro · 2020-04-20T23:53:25Z

plugin/sampling/strategystore/static/strategy_store_test.go

+	require.NoError(t, err)
+
+	// wait for reload
+	time.Sleep(interval * 2)


as I mentioned, we cannot rely on a single sleep, it makes the tests unstable due to unpredictable pauses in the CI. Use a longer loop with short checks, in most cases it will exit quickly.

yurishkuro · 2020-04-20T23:54:31Z

plugin/sampling/strategystore/static/strategy_store_test.go

+	os.Remove(dstFile)
+
+	// wait for delete and update failed
+	time.Sleep(interval * 2)


what is this testing (and how)?

The purpose is to increase the test code coverage.

It is there any other comment on this PR?

Instead of checking the error conditions via timer updates, I recommend refactoring the logic under case <-ticker.C into a helper function reloadStrategiesFile and unit-testing that function with the desired edge cases.

Signed-off-by: defool <defool@foxmail.com>

yurishkuro · 2020-04-22T01:50:22Z

plugin/sampling/strategystore/static/strategy_store.go

+		case <-ticker.C:
+			currBytes, err := ioutil.ReadFile(filepath.Clean(filePath))
+			if err != nil {
+				h.logger.Error("ReadFile failed", zap.Error(err))


Suggested change

h.logger.Error("ReadFile failed", zap.Error(err))

h.logger.Error("failed to load sampling strategies", zap.String("file", filePath), zap.Error(err))

continue

yurishkuro · 2020-04-22T01:51:47Z

plugin/sampling/strategystore/static/strategy_store.go

+				continue
+			}
+			if err = h.updateSamplingStrategy(currBytes); err != nil {
+				h.logger.Error("UpdateSamplingStrategy failed", zap.Error(err))


Suggested change

h.logger.Error("UpdateSamplingStrategy failed", zap.Error(err))

h.logger.Error("failed to update sampling strategies from file", zap.Error(err))

yurishkuro · 2020-04-22T01:52:23Z

plugin/sampling/strategystore/static/strategy_store.go

+func (h *strategyStore) updateSamplingStrategy(bytes []byte) error {
+	var strategies strategies
+	if err := json.Unmarshal(bytes, &strategies); err != nil {
+		return fmt.Errorf("failed to unmarshal strategies: %w", err)


Suggested change

return fmt.Errorf("failed to unmarshal strategies: %w", err)

return fmt.Errorf("failed to unmarshal sampling strategies: %w", err)

yurishkuro · 2020-04-25T22:31:00Z

plugin/sampling/strategystore/static/strategy_store_test.go

+	os.Remove(dstFile)
+
+	// wait for delete and update failed
+	time.Sleep(interval * 2)


Instead of checking the error conditions via timer updates, I recommend refactoring the logic under case <-ticker.C into a helper function reloadStrategiesFile and unit-testing that function with the desired edge cases.

Signed-off-by: Yuri Shkuro <ys@uber.com>

defool requested a review from a team as a code owner April 19, 2020 11:53

defool force-pushed the master branch 5 times, most recently from 4b44892 to c8195f9 Compare April 19, 2020 15:39

yurishkuro changed the title ~~Add API for collector to refresh sampling strategies~~ Support periodic refresh of sampling strategies Apr 19, 2020

yurishkuro reviewed Apr 19, 2020

View reviewed changes

yurishkuro changed the title ~~Support periodic refresh of sampling strategies~~ [feat] Support periodic refresh of sampling strategies Apr 19, 2020

yurishkuro added the changelog:new-feature Change that should be called out as new feature in CHANGELOG label Apr 19, 2020

defool force-pushed the master branch 3 times, most recently from a53bb83 to 8353fa6 Compare April 20, 2020 12:53

Support periodic reload of sampling strategies file

f6304cf

Signed-off-by: defool <defool@foxmail.com>

defool force-pushed the master branch from 8353fa6 to f6304cf Compare April 20, 2020 13:56

yurishkuro reviewed Apr 20, 2020

View reviewed changes

Optimizing code struct and comments

b9ca39b

Signed-off-by: defool <defool@foxmail.com>

yurishkuro reviewed Apr 25, 2020

View reviewed changes

yurishkuro merged commit b687b1b into jaegertracing:master Apr 25, 2020

yurishkuro pushed a commit to yurishkuro/jaeger that referenced this pull request Apr 25, 2020

[refactor] Clean-up tests from jaegertracing#2188

662f41a

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro mentioned this pull request Apr 25, 2020

[refactor] Clean-up tests from #2188 #2200

Merged

yurishkuro added a commit that referenced this pull request Apr 25, 2020

[refactor] Clean-up tests from #2188 (#2200)

66993a2

Signed-off-by: Yuri Shkuro <ys@uber.com>

This was referenced Apr 25, 2020

Add support for dynamically reloading sampling strategies #2040

Closed

collector: reload sampling strategies on file content change #1058

Closed

yurishkuro mentioned this pull request May 13, 2020

Collector API to refresh sampling strategies #2186

Open

jpkrohling mentioned this pull request Dec 1, 2020

Sampling Strategies settings not updating in Collector without pod restart jaegertracing/jaeger-operator#1324

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat] Support periodic refresh of sampling strategies #2188

[feat] Support periodic refresh of sampling strategies #2188

defool commented Apr 19, 2020 •

edited

Loading

codecov bot commented Apr 19, 2020 •

edited

Loading

yurishkuro left a comment

yurishkuro Apr 19, 2020

yurishkuro Apr 19, 2020

yurishkuro Apr 19, 2020

yurishkuro Apr 19, 2020

yurishkuro Apr 19, 2020

yurishkuro Apr 19, 2020

yurishkuro Apr 19, 2020

yurishkuro Apr 19, 2020

yurishkuro Apr 19, 2020

yurishkuro Apr 19, 2020

yurishkuro Apr 19, 2020

defool Apr 20, 2020

yurishkuro Apr 20, 2020

yurishkuro Apr 20, 2020

yurishkuro Apr 20, 2020

yurishkuro Apr 20, 2020

defool Apr 21, 2020

defool Apr 24, 2020

yurishkuro Apr 25, 2020

yurishkuro Apr 22, 2020

yurishkuro Apr 22, 2020

yurishkuro Apr 22, 2020

yurishkuro Apr 25, 2020

	flagSet.Duration(samplingStrategiesReloadInterval, 0, "Reload interval to check and reload sampling strategies file. Zero value means no checks (default 0s)")
	flagSet.Duration(samplingStrategiesReloadInterval, 0, "Reload interval to check and reload sampling strategies file. Zero value means no reloading")

	h.logger.Info("Updated strategy:" + string(bytes))
	h.logger.Info("Updated sampling strategies:" + string(bytes))

	h.logger.Error("ReadFile failed", zap.Error(err))
	h.logger.Error("failed to load sampling strategies", zap.String("file", filePath), zap.Error(err))
	continue

	h.logger.Error("UpdateSamplingStrategy failed", zap.Error(err))
	h.logger.Error("failed to update sampling strategies from file", zap.Error(err))

	return fmt.Errorf("failed to unmarshal strategies: %w", err)
	return fmt.Errorf("failed to unmarshal sampling strategies: %w", err)

[feat] Support periodic refresh of sampling strategies #2188

[feat] Support periodic refresh of sampling strategies #2188

Conversation

defool commented Apr 19, 2020 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Apr 19, 2020 • edited Loading

Codecov Report

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

defool commented Apr 19, 2020 •

edited

Loading

codecov bot commented Apr 19, 2020 •

edited

Loading