-
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
Make operation_strategies part also be part of default_strategy #1749
Make operation_strategies part also be part of default_strategy #1749
Conversation
Makes it possible to use operation_strategies in default_strategy in the sampling strategy configuration file. Signed-off-by: Rutger Broekhoff <rutger@viasalix.nl>
Signed-off-by: Rutger Broekhoff <rutger@viasalix.nl>
Signed-off-by: Rutger Broekhoff <rutger@viasalix.nl>
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.
Thanks for the PR.
I have concerns with the modeling. I also think there there should be a code change that interprets this top-level per-operation defaults and includes them in the response for GetStrategy(service) endpoint, so that this PR actually has effect on the clients (without changing the clients themselves).
Param float64 `json:"param"` | ||
Type string `json:"type"` | ||
Param float64 `json:"param"` | ||
OperationStrategies []*operationStrategy `json:"operation_strategies"` |
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.
This is not a good encapsulation, a strategy is a "simple" struct. If necessary, the overall config can have a separate service-less per-operation section, but we should not break the notion of a simple strategy.
btw, this might be of interest #365 (comment) |
Signed-off-by: Rutger Broekhoff <rutger@viasalix.nl>
Codecov Report
@@ Coverage Diff @@
## master #1749 +/- ##
==========================================
- Coverage 96.94% 96.89% -0.05%
==========================================
Files 202 203 +1
Lines 10068 10100 +32
==========================================
+ Hits 9760 9786 +26
- Misses 269 273 +4
- Partials 39 41 +2
Continue to review full report at Codecov.
|
Signed-off-by: Rutger Broekhoff <rutger@viasalix.nl>
Signed-off-by: Rutger Broekhoff <rutger@viasalix.nl>
Signed-off-by: Rutger Broekhoff <rutger@viasalix.nl>
@rutgerbrf thanks for the PR! PTAL at the respective docs change: jaegertracing/documentation#346 |
Which problem is this PR solving?
Resolves #1748
Short description of the changes
Makes operation strategies a default part of strategy so operation strategies can be defined for the default sampling strategy.