Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable service default sampling param #2230

Merged
merged 5 commits into from
May 10, 2020
Merged

Enable service default sampling param #2230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e457add
Set default sampling for service which has no per-operation strategies
defool May 8, 2020
b4bc5b4
Merge branch 'master' into hotfix/use_service_strategies
defool May 8, 2020
7d49fb6
Merge branch 'master' into hotfix/use_service_strategies
defool May 9, 2020
85b89b4
Consider sampling type of ratelimiting
defool May 10, 2020
c3e6f1a
Merge branch 'master' into hotfix/use_service_strategies
defool May 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions plugin/sampling/strategystore/static/fixtures/service_no_per_operation.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
{
"service_strategies": [
{
"service": "ServiceA",
"type": "probabilistic",
"param": 1.0
},
{
"service": "ServiceB",
"type": "ratelimiting",
"param": 3
}
],
"default_strategy": {
"type": "probabilistic",
"param": 0.2,
"operation_strategies": [
{
"operation": "/health",
"type": "probabilistic",
"param": 0.0
}
]
}
}
11 changes: 8 additions & 3 deletions plugin/sampling/strategystore/static/strategy_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,11 +164,16 @@ func (h *strategyStore) parseStrategies(strategies *strategies) {
// is not merged with and only used as a fallback).
opS := newStore.serviceStrategies[s.Service].OperationSampling
if opS == nil {
// Service has no per-operation strategies, so just reference the default settings.
newStore.serviceStrategies[s.Service].OperationSampling = newStore.defaultStrategy.OperationSampling
if newStore.defaultStrategy.OperationSampling == nil ||
newStore.serviceStrategies[s.Service].ProbabilisticSampling == nil {
continue
}
// Service has no per-operation strategies, so just reference the default settings and change default samplingRate.
newOpS := *newStore.defaultStrategy.OperationSampling
newOpS.DefaultSamplingProbability = newStore.serviceStrategies[s.Service].ProbabilisticSampling.SamplingRate
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible that newStore.serviceStrategies[s.Service].ProbabilisticSampling == nil here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, in plugin/sampling/strategystore/static/fixtures/operation_strategies.json there is

    {
      "service": "bar",
      "type": "ratelimiting",
      "param": 5,
      ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side node: this type was a mistake introduced in the past:

// operationStrategy defines an operation specific sampling strategy.
type operationStrategy struct {
        Operation string `json:"operation"`
        strategy
}

because operations can only use probabilistic strategy (example), so this generalized schema allows things that clients can't understand. Similarly, if service-level strategy contains per-operation instructions, then the top-level strategy can only be probabilistic (default sampler in the example).

newStore.serviceStrategies[s.Service].OperationSampling = &newOpS
continue
}

if merge {
opS.PerOperationStrategies = mergePerOperationSamplingStrategies(
opS.PerOperationStrategies,
Expand Down
15 changes: 15 additions & 0 deletions plugin/sampling/strategystore/static/strategy_store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,3 +321,18 @@ func TestAutoUpdateStrategyErrors(t *testing.T) {
assert.Equal(t, "blah", store.reloadSamplingStrategyFile(tempFile.Name(), "blah"))
assert.Len(t, logs.FilterMessage("failed to update sampling strategies from file").All(), 1)
}

func TestServiceNoPerOperationStrategies(t *testing.T) {
store, err := NewStrategyStore(Options{StrategiesFile: "fixtures/service_no_per_operation.json"}, zap.NewNop())
require.NoError(t, err)

s, err := store.GetSamplingStrategy("ServiceA")
require.NoError(t, err)
assert.Equal(t, 1.0, s.OperationSampling.DefaultSamplingProbability)

s, err = store.GetSamplingStrategy("ServiceB")
require.NoError(t, err)

expected := makeResponse(sampling.SamplingStrategyType_RATE_LIMITING, 3)
assert.Equal(t, *expected.RateLimitingSampling, *s.RateLimitingSampling)
}