-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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.
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
Enable service default sampling param #2230
Conversation
Signed-off-by: defool <[email protected]>
6922f1b
to
e457add
Compare
Codecov Report
@@ Coverage Diff @@
## master #2230 +/- ##
==========================================
- Coverage 96.18% 96.16% -0.02%
==========================================
Files 219 219
Lines 10635 10640 +5
==========================================
+ Hits 10229 10232 +3
- Misses 351 352 +1
- Partials 55 56 +1
Continue to review full report at Codecov.
|
} | ||
// 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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,
...
There was a problem hiding this comment.
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).
Signed-off-by: defool [email protected]
Which problem is this PR solving?
Short description of the changes
service
's sampling param event though it has no per-operation strategies.