From 513ff66666c65dc664244a4adf68496ec3eb6276 Mon Sep 17 00:00:00 2001 From: Gary Brown Date: Tue, 11 Dec 2018 16:56:20 +0000 Subject: [PATCH 1/3] Fix sampling strategies overwriting service entry when no sampling type is specified Signed-off-by: Gary Brown --- .../fixtures/missing-service-types.json | 33 ++++++++++++++ .../static/fixtures/operation_strategies.json | 5 +++ .../strategystore/static/strategy_store.go | 3 +- .../static/strategy_store_test.go | 43 +++++++++++++++++++ 4 files changed, 83 insertions(+), 1 deletion(-) create mode 100644 plugin/sampling/strategystore/static/fixtures/missing-service-types.json diff --git a/plugin/sampling/strategystore/static/fixtures/missing-service-types.json b/plugin/sampling/strategystore/static/fixtures/missing-service-types.json new file mode 100644 index 00000000000..0d3d5f2a3c0 --- /dev/null +++ b/plugin/sampling/strategystore/static/fixtures/missing-service-types.json @@ -0,0 +1,33 @@ +{ + "default_strategy": { + "type": "probabilistic", + "param": 0.5 + }, + "service_strategies": [ + { + "service": "foo", + "operation_strategies": [ + { + "operation": "op1", + "type": "probabilistic", + "param": 0.2 + } + ] + }, + { + "service": "bar", + "operation_strategies": [ + { + "operation": "op3", + "type": "probabilistic", + "param": 0.3 + }, + { + "operation": "op5", + "type": "probabilistic", + "param": 0.4 + } + ] + } + ] +} diff --git a/plugin/sampling/strategystore/static/fixtures/operation_strategies.json b/plugin/sampling/strategystore/static/fixtures/operation_strategies.json index f0c7525f357..d0c00fb3247 100644 --- a/plugin/sampling/strategystore/static/fixtures/operation_strategies.json +++ b/plugin/sampling/strategystore/static/fixtures/operation_strategies.json @@ -35,6 +35,11 @@ "operation": "op4", "type": "ratelimiting", "param": 100 + }, + { + "operation": "op5", + "type": "probabilistic", + "param": 0.4 } ] } diff --git a/plugin/sampling/strategystore/static/strategy_store.go b/plugin/sampling/strategystore/static/strategy_store.go index cede2d1d85c..8598117d54e 100644 --- a/plugin/sampling/strategystore/static/strategy_store.go +++ b/plugin/sampling/strategystore/static/strategy_store.go @@ -136,6 +136,7 @@ func (h *strategyStore) parseStrategy(strategy *strategy) *sampling.SamplingStra } default: h.logger.Warn("Failed to parse sampling strategy", zap.Any("strategy", strategy)) - return &defaultStrategy + copyOfDefaultStrategy := defaultStrategy + return ©OfDefaultStrategy } } diff --git a/plugin/sampling/strategystore/static/strategy_store_test.go b/plugin/sampling/strategystore/static/strategy_store_test.go index 16e05575b44..dc1e16d9a94 100644 --- a/plugin/sampling/strategystore/static/strategy_store_test.go +++ b/plugin/sampling/strategystore/static/strategy_store_test.go @@ -91,9 +91,52 @@ func TestPerOperationSamplingStrategies(t *testing.T) { require.NotNil(t, s.OperationSampling) os = s.OperationSampling assert.EqualValues(t, os.DefaultSamplingProbability, 0.001) + require.Len(t, os.PerOperationStrategies, 2) + assert.Equal(t, "op3", os.PerOperationStrategies[0].Operation) + assert.EqualValues(t, 0.3, os.PerOperationStrategies[0].ProbabilisticSampling.SamplingRate) + assert.Equal(t, "op5", os.PerOperationStrategies[1].Operation) + assert.EqualValues(t, 0.4, os.PerOperationStrategies[1].ProbabilisticSampling.SamplingRate) + + s, err = store.GetSamplingStrategy("default") + require.NoError(t, err) + assert.EqualValues(t, makeResponse(sampling.SamplingStrategyType_PROBABILISTIC, 0.5), *s) +} + +func TestMissingServiceSamplingStrategyTypes(t *testing.T) { + logger, buf := testutils.NewLogger() + store, err := NewStrategyStore(Options{StrategiesFile: "fixtures/missing-service-types.json"}, logger) + assert.Contains(t, buf.String(), "Failed to parse sampling strategy") + require.NoError(t, err) + + expected := makeResponse(sampling.SamplingStrategyType_PROBABILISTIC, defaultSamplingProbability) + + s, err := store.GetSamplingStrategy("foo") + require.NoError(t, err) + assert.Equal(t, sampling.SamplingStrategyType_PROBABILISTIC, s.StrategyType) + assert.Equal(t, *expected.ProbabilisticSampling, *s.ProbabilisticSampling) + + require.NotNil(t, s.OperationSampling) + os := s.OperationSampling + assert.EqualValues(t, os.DefaultSamplingProbability, defaultSamplingProbability) require.Len(t, os.PerOperationStrategies, 1) + assert.Equal(t, "op1", os.PerOperationStrategies[0].Operation) + assert.EqualValues(t, 0.2, os.PerOperationStrategies[0].ProbabilisticSampling.SamplingRate) + + expected = makeResponse(sampling.SamplingStrategyType_PROBABILISTIC, defaultSamplingProbability) + + s, err = store.GetSamplingStrategy("bar") + require.NoError(t, err) + assert.Equal(t, sampling.SamplingStrategyType_PROBABILISTIC, s.StrategyType) + assert.Equal(t, *expected.ProbabilisticSampling, *s.ProbabilisticSampling) + + require.NotNil(t, s.OperationSampling) + os = s.OperationSampling + assert.EqualValues(t, os.DefaultSamplingProbability, 0.001) + require.Len(t, os.PerOperationStrategies, 2) assert.Equal(t, "op3", os.PerOperationStrategies[0].Operation) assert.EqualValues(t, 0.3, os.PerOperationStrategies[0].ProbabilisticSampling.SamplingRate) + assert.Equal(t, "op5", os.PerOperationStrategies[1].Operation) + assert.EqualValues(t, 0.4, os.PerOperationStrategies[1].ProbabilisticSampling.SamplingRate) s, err = store.GetSamplingStrategy("default") require.NoError(t, err) From 7ab96e9884b89f7d0d16ac5af89fd8d0c4a1c013 Mon Sep 17 00:00:00 2001 From: Gary Brown Date: Thu, 3 Jan 2019 17:07:42 +0000 Subject: [PATCH 2/3] Add deep copy of default strategy Signed-off-by: Gary Brown --- .../strategystore/static/strategy_store.go | 21 +++++++++++++++++-- .../static/strategy_store_test.go | 12 +++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/plugin/sampling/strategystore/static/strategy_store.go b/plugin/sampling/strategystore/static/strategy_store.go index 8598117d54e..bf9e67ef7e5 100644 --- a/plugin/sampling/strategystore/static/strategy_store.go +++ b/plugin/sampling/strategystore/static/strategy_store.go @@ -15,6 +15,8 @@ package static import ( + "bytes" + "encoding/gob" "encoding/json" "fmt" "io/ioutil" @@ -136,7 +138,22 @@ func (h *strategyStore) parseStrategy(strategy *strategy) *sampling.SamplingStra } default: h.logger.Warn("Failed to parse sampling strategy", zap.Any("strategy", strategy)) - copyOfDefaultStrategy := defaultStrategy - return ©OfDefaultStrategy + return deepCopy(&defaultStrategy) } } + +func deepCopy(s *sampling.SamplingStrategyResponse) *sampling.SamplingStrategyResponse { + var buf bytes.Buffer + enc := gob.NewEncoder(&buf) + dec := gob.NewDecoder(&buf) + err := enc.Encode(*s) + if err != nil { + return nil + } + var copy sampling.SamplingStrategyResponse + err = dec.Decode(©) + if err != nil { + return nil + } + return © +} diff --git a/plugin/sampling/strategystore/static/strategy_store_test.go b/plugin/sampling/strategystore/static/strategy_store_test.go index dc1e16d9a94..fd587e1aba5 100644 --- a/plugin/sampling/strategystore/static/strategy_store_test.go +++ b/plugin/sampling/strategystore/static/strategy_store_test.go @@ -193,3 +193,15 @@ func makeResponse(samplerType sampling.SamplingStrategyType, param float64) (res } return resp } + +func TestDeepCopy(t *testing.T) { + s := &sampling.SamplingStrategyResponse{ + StrategyType: sampling.SamplingStrategyType_PROBABILISTIC, + ProbabilisticSampling: &sampling.ProbabilisticSamplingStrategy{ + SamplingRate: 0.5, + }, + } + copy := deepCopy(s) + assert.False(t, copy == s) + assert.EqualValues(t, copy, s) +} From 0514fb1c4fd1afccb4d151a7a407fb30d8cbe734 Mon Sep 17 00:00:00 2001 From: Gary Brown Date: Tue, 15 Jan 2019 12:42:29 +0000 Subject: [PATCH 3/3] Improve test coverage Signed-off-by: Gary Brown --- plugin/sampling/strategystore/static/strategy_store.go | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/plugin/sampling/strategystore/static/strategy_store.go b/plugin/sampling/strategystore/static/strategy_store.go index bf9e67ef7e5..fce2c080d10 100644 --- a/plugin/sampling/strategystore/static/strategy_store.go +++ b/plugin/sampling/strategystore/static/strategy_store.go @@ -146,14 +146,8 @@ func deepCopy(s *sampling.SamplingStrategyResponse) *sampling.SamplingStrategyRe var buf bytes.Buffer enc := gob.NewEncoder(&buf) dec := gob.NewDecoder(&buf) - err := enc.Encode(*s) - if err != nil { - return nil - } + enc.Encode(*s) var copy sampling.SamplingStrategyResponse - err = dec.Decode(©) - if err != nil { - return nil - } + dec.Decode(©) return © }