Fix sampling strategies overwriting service entry when no sampling ty… #1244
Conversation
objectiser
requested review from
black-adder,
jpkrohling,
pavolloffay,
tiffon,
vprithvi and
yurishkuro
as code owners
ghost
assigned 
what do you mean by "sampling type"? Taking a copy seems fine, but what does it fix? Do we modify the returned struct somehow, so that the default is affected for every service? |
Codecov Report
@@ Coverage Diff @@
## master #1244 +/- ##
======================================
Coverage 100% 100%
======================================
Files 161 161
Lines 7185 7193 +8
======================================
+ Hits 7185 7193 +8
Continue to review full report at Codecov.
|
|
For example, as shown here. The service level sampling type. Yes the defaultStrategy is being modified. |
|
Maybe not in this PR, but I think in general it would be better to enable the service specific sampling strategy to be optional, and if not defined, then the default defined in the config should be used. Only if that is not defined, should the hardcoded default be used. |
| @@ -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 | |||
There was a problem hiding this comment.
I am not sure this way of copying is correct. It only works for primitive strategies, anything nested would still have the same issue. I think it needs a deep copy.
|
@yurishkuro What is the preferred approach for doing deep copy in Jaeger? |
|
We haven't done it anywhere afaik. One option is to use |
objectiser
force-pushed
the
sampling
branch
2 times, most recently
from
4d36bf8
to
c35e894
Compare
…pe is specified Signed-off-by: Gary Brown <[email protected]>
Signed-off-by: Gary Brown <[email protected]>
Signed-off-by: Gary Brown <[email protected]>
|
@yurishkuro Updated. |
…pe is specified
Signed-off-by: Gary Brown [email protected]
Which problem is this PR solving?
Resolves #1205
If the service strategy does not define a sampling type, it reuses the default sampling strategy but applies the operation sampling strategies. End result is that all services (that don't define a sampling type) will share a single entry with identical details (i.e. the ones associated with the last initialised entry).
Short description of the changes
Although it is an error state, if no sampling type is defined and the default sampling strategy is to be used, then it should return a copy.