Add URL option for sampling strategies file by goku321 · Pull Request #2519 · jaegertracing/jaeger

goku321 · 2020-09-29T13:05:14Z

Which problem is this PR solving?

Resolve Support url for sampling.strategies file #2015

Short description of the changes

Added a URL option to support downloading a sampling strategies file from a url.

yurishkuro · 2020-09-29T15:41:06Z

go.mod

 go 1.14

 require (
-	github.com/AndreasBriese/bbloom v0.0.0-20190825152654-46b345b51c96 // indirect


I do not believe this PR requires changes to dependencies. Please exclude the go.mod/go.sum files.

I've synced my fork with the upstream master. Do I still need to exclude go.mod/go.sum files?

if this PR does not require new dependencies, it should not be updating dependency file. Please do checkout master <file> to get the originals.

yurishkuro · 2020-09-29T15:41:53Z

plugin/sampling/strategystore/static/options.go


 const (
-	// SamplingStrategiesFile contains the name of CLI opions for config file.
+	// SamplingStrategiesFile contains the name of CLI options for config file.


Suggested change

// SamplingStrategiesFile contains the name of CLI options for config file.

// SamplingStrategiesFile contains the name of CLI option for config file.

yurishkuro · 2020-09-29T15:43:25Z

plugin/sampling/strategystore/static/options.go

 	SamplingStrategiesFile           = "sampling.strategies-file"
 	samplingStrategiesReloadInterval = "sampling.strategies-reload-interval"
+	// SamplingStrategiesURL contains the name of CLI option for config file URL.
+	SamplingStrategiesURL = "sampling.strategies-url"


I think the intention of the ticket was to allow sampling.strategies-file to contain a URL. A new flag seems redundant.

yurishkuro · 2020-09-29T15:47:16Z

plugin/sampling/strategystore/static/strategy_store.go

-	if err != nil {
-		h.logger.Error("failed to load sampling strategies", zap.String("file", filePath), zap.Error(err))
-		return lastValue
+func (h *strategyStore) reloadSamplingStrategy(reloadFrom string, path string, lastValue string) string {


The caller of this function already does checks for file vs. url. I would pass a loader function here that abstracts the source of the new config, e.g. loader func () (string, error), and only deal with processing the new value.

Yes, that's better. Was going to do the same but changed my mind later.

yurishkuro · 2020-09-29T15:48:16Z

plugin/sampling/strategystore/static/strategy_store.go

 }

+func downloadStrategies(strategiesURL string) (*strategies, error) {
+	resp, err := http.Get(strategiesURL)


this duplicates code from L112

goku321 · 2020-10-01T18:57:49Z

@yurishkuro Thanks for the suggestions. I've made changes to the pull request

yurishkuro

top-level question: should there be some local file fallback option if the URL is temporarily unavailable? Because if whichever service provides the strategy for download is down the collector would not be able to start.

plugin/sampling/strategystore/static/strategy_store.go

yurishkuro · 2020-10-03T07:20:33Z

plugin/sampling/strategystore/static/strategy_store.go

 	}
-	if err = h.updateSamplingStrategy(currBytes); err != nil {
-		h.logger.Error("failed to update sampling strategies from file", zap.Error(err))
+	if err := h.updateSamplingStrategy([]byte(newValue)); err != nil {


Consider if ([]byte, error) would be more appropriate signature for the loader(), to avoid casting back and forth.

yurishkuro · 2020-10-03T07:22:26Z

plugin/sampling/strategystore/static/strategy_store.go

-	if strategiesFile == "" {
-		return nil, nil
+	// Check if it's a URL.
+	if ok := isURL(strategiesFile); ok {


there is an existing redundancy in the original source, please consolidate it. You should not need to check isURL twice in this source file.

goku321 · 2020-10-04T18:35:16Z

top-level question: should there be some local file fallback option if the URL is temporarily unavailable? Because if whichever service provides the strategy for download is down the collector would not be able to start.

How about falling back to default strategies similar to when options.StrategiesFile is empty? Also, we can have a retry mechanism for URL option.

yurishkuro · 2020-10-04T19:08:28Z

Default strategy is fine. The difference with StrategiesFile not found is in that case it's ok to fail because it likely indicates a mis-configuration, whereas in case of HTTP server not available then failing is counterproductive.

yurishkuro · 2020-10-06T23:52:13Z

plugin/sampling/strategystore/static/strategy_store.go

+}
+
+func samplingStrategyLoader(strategiesFile string) strategyLoader {
+	return func() ([]byte, error) {


nit: you only need to test strategiesFile once, and return different functors. Otherwise strategyLoader is pointless, you could've just call the function to read stuff

if strategiesFile == "" { return func() ([]byte, error) { return []byte("null"), nil } } if isURL(strategiesFile) { return func() ([]byte, error) { return downloadSamplingStrategies(strategiesFile) } }

Oops, I was being silly. I get this, thanks.

yurishkuro · 2020-10-06T23:53:16Z

plugin/sampling/strategystore/static/strategy_store.go

+	return func() ([]byte, error) {
+		if strategiesFile == "" {
+			// Using "null" so that it un-marshals to nil pointer.
+			return []byte("null"), nil


may want to declare this as a constant

yurishkuro · 2020-10-06T23:53:49Z

plugin/sampling/strategystore/static/strategy_store.go


-func (h *strategyStore) reloadSamplingStrategyFile(filePath string, lastValue string) string {
-	currBytes, err := ioutil.ReadFile(filepath.Clean(filePath))
+func (h *strategyStore) reloadSamplingStrategy(loader strategyLoader, lastValue string) string {


Suggested change

func (h *strategyStore) reloadSamplingStrategy(loader strategyLoader, lastValue string) string {

func (h *strategyStore) reloadSamplingStrategy(loadFn strategyLoader, lastValue string) string {

yurishkuro · 2020-10-06T23:55:38Z

please check why the DCO test is failing:

Commit sha: 932f6e0, Author: Deepak, Committer: Deepak; Expected "Deepak sah.sslpu@gmail.com", but got "Deepak Sah sah.sslpu@gmail.com".

You need to ensure full name in git matches full name in github

This change will let user to provide a URL to download sampling strategies. Default strategy is the fallback option if the URL is temporarily unavailable. Signed-off-by: Deepak <sah.sslpu@gmail.com>

yurishkuro · 2020-10-07T20:50:04Z

plugin/sampling/strategystore/static/strategy_store.go

+	return func() ([]byte, error) {
+		currBytes, err := ioutil.ReadFile(strategiesFile)
+		if err != nil {
+			return nil, fmt.Errorf("failed to open strategies file: %w", err)


Suggested change

return nil, fmt.Errorf("failed to open strategies file: %w", err)

return nil, fmt.Errorf("failed to read strategies file %s: %w", strategiesFile , err)

yurishkuro · 2020-10-07T20:51:09Z

plugin/sampling/strategystore/static/strategy_store.go

 )

+// null represents "null" JSON value.
+const null = "null"


I was rather thinking this:

Suggested change

const null = "null"

var nullJSON = []byte("null")

yurishkuro · 2020-10-07T20:51:26Z

plugin/sampling/strategystore/static/strategy_store.go

+	defer resp.Body.Close()
+	if resp.StatusCode != http.StatusOK {
+		if resp.StatusCode == http.StatusServiceUnavailable {
+			return []byte("null"), nil


Suggested change

return []byte("null"), nil

return nullJSON, nil

yurishkuro · 2020-10-07T20:52:38Z

plugin/sampling/strategystore/static/strategy_store.go

+
+	defer resp.Body.Close()
+	if resp.StatusCode != http.StatusOK {
+		if resp.StatusCode == http.StatusServiceUnavailable {


this does not need to be nested inside if != ok, you can do this check first, then if != OK.

plugin/sampling/strategystore/static/strategy_store.go

yurishkuro · 2020-10-07T20:54:52Z

plugin/sampling/strategystore/static/strategy_store.go

+
+	buf := new(bytes.Buffer)
+	if _, err = buf.ReadFrom(resp.Body); err != nil {
+		return nil, fmt.Errorf("failed to read sampling strategies from downloaded JSON: %w", err)


Suggested change

return nil, fmt.Errorf("failed to read sampling strategies from downloaded JSON: %w", err)

return nil, fmt.Errorf("failed to read sampling strategies HTTP response body: %w", err)

yurishkuro · 2020-10-07T20:56:04Z

plugin/sampling/strategystore/static/strategy_store.go

+			// Using null so that it un-marshals to nil pointer.
+			return []byte(null), nil


this comment should be on the constant

Suggested change

// Using null so that it un-marshals to nil pointer.

return []byte(null), nil

return nullJSON, nil

yurishkuro · 2020-10-07T20:56:45Z

plugin/sampling/strategystore/static/strategy_store.go

+}
+
+func samplingStrategyLoader(strategiesFile string) strategyLoader {
+	if strategiesFile == "" {


Shouldn't this be an error upon instantiation of strategyStore?

Didn't quite understand this one. Can you elaborate it further? 😅

There is a constructor function NewStrategyStore. This struct cannot do anything useful if the strategiesFile is empty, correct? So we could just return an error from there if the param is empty. If that's not sufficient (e.g. because it still needs to return the default strategy), then we can still do that in the constructor and just not execute all the loading/reloading functions if the file name is blank. This way we don't have to keep checking if the file path is empty or not.

I get it. Thanks for the explanation.

yurishkuro · 2020-10-07T20:59:58Z

plugin/sampling/strategystore/static/strategy_store.go

+	}
+
+	return func() ([]byte, error) {
+		currBytes, err := ioutil.ReadFile(strategiesFile)


you may get go-sec linter error here, wrap the file name:

Suggested change

currBytes, err := ioutil.ReadFile(strategiesFile)

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

While you're at it, can you fix this line too? I.e. remove nolint comment and use filepath.Clean

./cmd/query/app/static_handler.go:210: bytes, err := ioutil.ReadFile(uiConfig) /* nolint #nosec , this comes from an admin, not user */

This updates the logic for NewStrategyStore constructor to skip any loading/reloading logic to execute when StrategiesFile is empty. Signed-off-by: Deepak <sah.sslpu@gmail.com>

yurishkuro

Looks great! I left a couple comments around the tests, otherwise LGTM.

yurishkuro · 2020-10-10T19:40:28Z

plugin/sampling/strategystore/static/strategy_store.go

+}
+
+func (h *strategyStore) autoUpdateStrategies(interval time.Duration, loader strategyLoader) {
+	lastValue := string(nullJSON)


Technically, we could have already loaded something before calling this function, so lastValue would be different. But since it's a periodic check anyway, I think it's fine.

yurishkuro · 2020-10-10T19:47:25Z

plugin/sampling/strategystore/static/strategy_store_test.go

+
+	// update original strategies file with new probability of 0.9
+	newStr = strings.Replace(string(srcBytes), "0.8", "0.9", 1)
+	require.NoError(t, ioutil.WriteFile(srcFile, []byte(newStr), 0644))


I don't think it's a good idea for the test to override the fixture file. In L316 it is already copied to a temp file that can be overwritten.

Also, if you're testing with a mock HTTP server, why use file in the first place? You can simply replace the content returned by the mock server since you can control it.

yurishkuro · 2020-10-10T19:48:30Z

plugin/sampling/strategystore/static/strategy_store_test.go

 	}
 	assert.EqualValues(t, makeResponse(sampling.SamplingStrategyType_PROBABILISTIC, 0.9), *s)
+
+	// Test auto update strategy with URL option.


This seems like a pretty separate testing scenario, any reason why it cannot be moved into a new test function with appropriate name?

Yes, it makes sense to move it to a separate test function.

plugin/sampling/strategystore/static/strategy_store_test.go

yurishkuro · 2020-10-10T19:52:07Z

plugin/sampling/strategystore/static/strategy_store_test.go

 func TestStrategyStore(t *testing.T) {
 	_, err := NewStrategyStore(Options{StrategiesFile: "fileNotFound.json"}, zap.NewNop())
-	assert.EqualError(t, err, "failed to open strategies file: open fileNotFound.json: no such file or directory")
+	assert.EqualError(t, err, "failed to read strategies file fileNotFound.json: open fileNotFound.json: no such file or directory")


I recommend using assert.Contains (as in L79) to check only for the text produced by our code, not the parts produced by stdlib functions (since they can change the text between Go versions).

This refactors the strategy store tests, moving tests with URL to separate test functions. Signed-off-by: Deepak <sah.sslpu@gmail.com>

Signed-off-by: Deepak <sah.sslpu@gmail.com>

yurishkuro · 2020-10-11T23:48:09Z

plugin/sampling/strategystore/static/strategy_store_test.go

+func TestStrategyStoreWithFile(t *testing.T) {
 	_, err := NewStrategyStore(Options{StrategiesFile: "fileNotFound.json"}, zap.NewNop())
-	assert.EqualError(t, err, "failed to read strategies file fileNotFound.json: open fileNotFound.json: no such file or directory")
+	assert.Contains(t, err.Error(), "failed to read strategies file fileNotFound.json: open fileNotFound.json: no such file or directory")


Suggested change

assert.Contains(t, err.Error(), "failed to read strategies file fileNotFound.json: open fileNotFound.json: no such file or directory")

assert.Contains(t, err.Error(), "failed to read strategies file fileNotFound.json")

only include the string that we generate, not the stdlib portion

yurishkuro · 2020-10-11T23:54:51Z

plugin/sampling/strategystore/static/strategy_store_test.go

+	assert.Equal(t, string(strategiesJSON), value)
+
+	// update original strategies with new probability of 0.9
+	strategiesJSON = []byte(`


This is not a good approach to change package variable in-place. Even though it's a var, the general assumption is that package-level vars are constants (in fact I would change its type to string to make a real constant).

An even better approach is to convert it into a function that takes the probability value and returns the full JSON string via Sprintf. Then I would also change makeMockServer to return (strategy *atomic.String, server *httptest.Server) (using uber-go/atomic), so that you can manipulate the strategy externally, e.g.:

mockStrategy, server := mockStrategyServer() ... mockStrategy.Store(strategiesJSON(0.9)) ...

This updates the mockStrategyServer to return a mock strategy along with a mock server. The returned strategy can be used to update it externally. Also, changed package-level var strategiesJSON to a function to return strategies JSON with a given probability. Signed-off-by: Deepak <sah.sslpu@gmail.com>

yurishkuro · 2020-10-12T20:56:35Z

please check the linter failures

plugin/sampling/strategystore/static/strategy_store_test.go:130:9: this value of err is never used (SA4006)
plugin/sampling/strategystore/static/strategy_store_test.go:484:11: this value of err is never used (SA4006)

Signed-off-by: Deepak <sah.sslpu@gmail.com>

goku321 · 2020-10-13T14:41:22Z

please check the linter failures

plugin/sampling/strategystore/static/strategy_store_test.go:130:9: this value of err is never used (SA4006)
plugin/sampling/strategystore/static/strategy_store_test.go:484:11: this value of err is never used (SA4006)

Fixed the linter issues.

codecov · 2020-10-13T15:16:24Z

Codecov Report

❗ No coverage uploaded for pull request base (master@28873d4). Click here to learn what that means.
The diff coverage is 91.48%.

@@            Coverage Diff            @@
##             master    #2519   +/-   ##
=========================================
  Coverage          ?   95.31%           
=========================================
  Files             ?      208           
  Lines             ?     9281           
  Branches          ?        0           
=========================================
  Hits              ?     8846           
  Misses            ?      356           
  Partials          ?       79

Impacted Files	Coverage Δ
plugin/sampling/strategystore/static/options.go	`100.00% <ø> (ø)`
...in/sampling/strategystore/static/strategy_store.go	`97.53% <91.30%> (ø)`
cmd/query/app/static_handler.go	`83.03% <100.00%> (ø)`

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 28873d4...c95250b. Read the comment docs.

yurishkuro · 2020-10-13T16:43:34Z

Thank you @goku321 !

goku321 requested a review from a team as a code owner September 29, 2020 13:05

goku321 requested a review from yurishkuro September 29, 2020 13:05

goku321 force-pushed the master branch from 2a49b2c to 72bd90f Compare September 29, 2020 13:08

yurishkuro reviewed Sep 29, 2020

View reviewed changes

goku321 requested a review from yurishkuro October 1, 2020 19:10

yurishkuro reviewed Oct 3, 2020

View reviewed changes

goku321 requested a review from yurishkuro October 6, 2020 19:48

yurishkuro reviewed Oct 6, 2020

View reviewed changes

goku321 force-pushed the master branch from a589bb0 to e088453 Compare October 7, 2020 15:18

Add url option for sampling strategies

270e96d

This change will let user to provide a URL to download sampling strategies. Default strategy is the fallback option if the URL is temporarily unavailable. Signed-off-by: Deepak <sah.sslpu@gmail.com>

goku321 force-pushed the master branch from cc7d147 to 270e96d Compare October 7, 2020 19:40

goku321 requested a review from yurishkuro October 7, 2020 19:42

yurishkuro reviewed Oct 7, 2020

View reviewed changes

goku321 added 2 commits October 11, 2020 00:33

Update NewStrategyStore constructor

b50abef

This updates the logic for NewStrategyStore constructor to skip any loading/reloading logic to execute when StrategiesFile is empty. Signed-off-by: Deepak <sah.sslpu@gmail.com>

Merge remote-tracking branch 'upstream/master'

964129a

goku321 requested a review from yurishkuro October 10, 2020 19:08

yurishkuro reviewed Oct 10, 2020

View reviewed changes

Refactor strategy store tests

f517c75

This refactors the strategy store tests, moving tests with URL to separate test functions. Signed-off-by: Deepak <sah.sslpu@gmail.com>

goku321 requested a review from yurishkuro October 11, 2020 16:16

Use strings.Replace to update strategies

2593ee2

Signed-off-by: Deepak <sah.sslpu@gmail.com>

yurishkuro reviewed Oct 11, 2020

View reviewed changes

goku321 added 2 commits October 13, 2020 00:24

Merge remote-tracking branch 'upstream/master'

8e1719f

goku321 requested a review from yurishkuro October 12, 2020 19:02

yurishkuro approved these changes Oct 12, 2020

View reviewed changes

goku321 added 2 commits October 13, 2020 20:01

Fix linter failures

6673bdf

Signed-off-by: Deepak <sah.sslpu@gmail.com>

Merge remote-tracking branch 'upstream/master'

c95250b

yurishkuro merged commit 025d2f5 into jaegertracing:master Oct 13, 2020

	// SamplingStrategiesFile contains the name of CLI options for config file.
	// SamplingStrategiesFile contains the name of CLI option for config file.

	func (h *strategyStore) reloadSamplingStrategy(loader strategyLoader, lastValue string) string {
	func (h *strategyStore) reloadSamplingStrategy(loadFn strategyLoader, lastValue string) string {

	return nil, fmt.Errorf("failed to open strategies file: %w", err)
	return nil, fmt.Errorf("failed to read strategies file %s: %w", strategiesFile , err)

		// Using null so that it un-marshals to nil pointer.
		return []byte(null), nil

	// Using null so that it un-marshals to nil pointer.
	return []byte(null), nil
	return nullJSON, nil

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

	assert.Contains(t, err.Error(), "failed to read strategies file fileNotFound.json: open fileNotFound.json: no such file or directory")
	assert.Contains(t, err.Error(), "failed to read strategies file fileNotFound.json")

Conversation

goku321 commented Sep 29, 2020

Which problem is this PR solving?

Short description of the changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goku321 commented Oct 1, 2020

Uh oh!

yurishkuro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goku321 commented Oct 4, 2020

Uh oh!

yurishkuro commented Oct 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yurishkuro commented Oct 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yurishkuro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov bot commented Oct 13, 2020 •

edited

Loading