Skip to content

Commit

Permalink
[sampling] Fix merging of per-operation strategies into service strat…
Browse files Browse the repository at this point in the history
…egies without them (jaegertracing#5277)

## Which problem is this PR solving?
- Part 1 of [jaegertracing#5270]

## Description of the changes
- See issue for description of the bug and of the two changed behaviors
- Default `probabilistic` operation level strategies will now be
returned if service strategy is of `ratelimiting` type
- If the service itself has `probabilistic` strategy, its sampling rate
takes priority for operation-level definition

## How was this change tested?
- Via updated unit test
- By building local all-in-one executable and using it as a otel
instrumentation backend for simple java service
https://github.com/kuujis/opentelemetry-java-5504 and using the below
`sampling_strategies.json` from
open-telemetry/opentelemetry-java#5504
```
{
  "service_strategies": [
    {
      "service": "foobar",
      "type": "ratelimiting",
      "param": 2
    }
  ],
  "default_strategy": {
    "type": "probabilistic",
    "param": 0.2,
    "operation_strategies": [
      {
        "operation": "metrics",
        "type": "probabilistic",
        "param": 0.0
      }
    ]
  }
}
```

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: Kazimieras Pociunas <kazimieras.pociunas@gmail.com>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Co-authored-by: Yuri Shkuro <github@ysh.us>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
  • Loading branch information
3 people authored Apr 20, 2024
1 parent 7f99a88 commit a72dfc3
Show file tree
Hide file tree
Showing 8 changed files with 303 additions and 123 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"probabilisticSampling": {
"samplingRate": 1
},
"operationSampling": {
"defaultSamplingProbability": 1,
"perOperationStrategies": [
{
"operation": "/health",
"probabilisticSampling": {
"samplingRate": 0.1
}
}
]
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"strategyType": 1,
"rateLimitingSampling": {
"maxTracesPerSecond": 3
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"probabilisticSampling": {
"samplingRate": 1
},
"operationSampling": {
"defaultSamplingProbability": 1,
"perOperationStrategies": [
{
"operation": "/health",
"probabilisticSampling": {
"samplingRate": 0.1
}
}
]
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"strategyType": 1,
"rateLimitingSampling": {
"maxTracesPerSecond": 3
},
"operationSampling": {
"defaultSamplingProbability": 0.2,
"perOperationStrategies": [
{
"operation": "/health",
"probabilisticSampling": {
"samplingRate": 0.1
}
}
]
}
}
Original file line number Diff line number Diff line change
@@ -1,25 +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
}
]
}
}
{
"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.1
}
]
}
}
7 changes: 7 additions & 0 deletions plugin/sampling/strategystore/static/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const (
// samplingStrategiesFile contains the name of CLI option for config file.
samplingStrategiesFile = "sampling.strategies-file"
samplingStrategiesReloadInterval = "sampling.strategies-reload-interval"
samplingStrategiesBugfix5270 = "sampling.strategies.bugfix-5270"
)

// Options holds configuration for the static sampling strategy store.
Expand All @@ -33,17 +34,23 @@ type Options struct {
StrategiesFile string
// ReloadInterval is the time interval to check and reload sampling strategies file
ReloadInterval time.Duration
// Flag for enabling possibly breaking change which includes default operations level
// strategies when calculating Ratelimiting type service level strategy
// more information https://github.com/jaegertracing/jaeger/issues/5270
IncludeDefaultOpStrategies bool
}

// AddFlags adds flags for Options
func AddFlags(flagSet *flag.FlagSet) {
flagSet.Duration(samplingStrategiesReloadInterval, 0, "Reload interval to check and reload sampling strategies file. Zero value means no reloading")
flagSet.String(samplingStrategiesFile, "", "The path for the sampling strategies file in JSON format. See sampling documentation to see format of the file")
flagSet.Bool(samplingStrategiesBugfix5270, false, "Include default operation level strategies for Ratesampling type service level strategy. Cf. https://github.com/jaegertracing/jaeger/issues/5270")
}

// InitFromViper initializes Options with properties from viper
func (opts *Options) InitFromViper(v *viper.Viper) *Options {
opts.StrategiesFile = v.GetString(samplingStrategiesFile)
opts.ReloadInterval = v.GetDuration(samplingStrategiesReloadInterval)
opts.IncludeDefaultOpStrategies = v.GetBool(samplingStrategiesBugfix5270)
return opts
}
63 changes: 56 additions & 7 deletions plugin/sampling/strategystore/static/strategy_store.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ type strategyStore struct {
storedStrategies atomic.Value // holds *storedStrategies

cancelFunc context.CancelFunc

options Options
}

type storedStrategies struct {
Expand All @@ -58,20 +60,32 @@ func NewStrategyStore(options Options, logger *zap.Logger) (ss.StrategyStore, er
h := &strategyStore{
logger: logger,
cancelFunc: cancelFunc,
options: options,
}
h.storedStrategies.Store(defaultStrategies())

if options.StrategiesFile == "" {
h.parseStrategies(nil)
h.logger.Info("No sampling strategies source provided, using defaults")
return h, nil
}

loadFn := h.samplingStrategyLoader(options.StrategiesFile)
strategies, err := loadStrategies(loadFn)
if err != nil {
return nil, err
} else if strategies == nil {
h.logger.Info("No sampling strategies found or URL is unavailable, using defaults")
return h, nil
}

if !h.options.IncludeDefaultOpStrategies {
h.logger.Warn("Default operations level strategies will not be included for Ratelimiting service strategies." +
"This behavior will be changed in future releases. " +
"Cf. https://github.com/jaegertracing/jaeger/issues/5270")
h.parseStrategies_deprecated(strategies)
} else {
h.parseStrategies(strategies)
}
h.parseStrategies(strategies)

if options.ReloadInterval > 0 {
go h.autoUpdateStrategies(ctx, options.ReloadInterval, loadFn)
Expand Down Expand Up @@ -206,11 +220,7 @@ func loadStrategies(loadFn strategyLoader) (*strategies, error) {
return strategies, nil
}

func (h *strategyStore) parseStrategies(strategies *strategies) {
if strategies == nil {
h.logger.Info("No sampling strategies provided or URL is unavailable, using defaults")
return
}
func (h *strategyStore) parseStrategies_deprecated(strategies *strategies) {
newStore := defaultStrategies()
if strategies.DefaultStrategy != nil {
newStore.defaultStrategy = h.parseServiceStrategies(strategies.DefaultStrategy)
Expand Down Expand Up @@ -249,6 +259,45 @@ func (h *strategyStore) parseStrategies(strategies *strategies) {
h.storedStrategies.Store(newStore)
}

func (h *strategyStore) parseStrategies(strategies *strategies) {
newStore := defaultStrategies()
if strategies.DefaultStrategy != nil {
newStore.defaultStrategy = h.parseServiceStrategies(strategies.DefaultStrategy)
}

for _, s := range strategies.ServiceStrategies {
newStore.serviceStrategies[s.Service] = h.parseServiceStrategies(s)

// Config for this service may not have per-operation strategies,
// but if the default strategy has them they should still apply.

if newStore.defaultStrategy.OperationSampling == nil {
// Default strategy doens't have them either, nothing to do.
continue
}

opS := newStore.serviceStrategies[s.Service].OperationSampling
if opS == nil {

// Service does not have its own per-operation rules, so copy (by value) from the default strategy.
newOpS := *newStore.defaultStrategy.OperationSampling

// If the service's own default is probabilistic, then its sampling rate should take precedence.
if newStore.serviceStrategies[s.Service].ProbabilisticSampling != nil {
newOpS.DefaultSamplingProbability = newStore.serviceStrategies[s.Service].ProbabilisticSampling.SamplingRate
}
newStore.serviceStrategies[s.Service].OperationSampling = &newOpS
continue
}

// If the service did have its own per-operation strategies, then merge them with the default ones.
opS.PerOperationStrategies = mergePerOperationSamplingStrategies(
opS.PerOperationStrategies,
newStore.defaultStrategy.OperationSampling.PerOperationStrategies)
}
h.storedStrategies.Store(newStore)
}

// mergePerOperationSamplingStrategies merges two operation strategies a and b, where a takes precedence over b.
func mergePerOperationSamplingStrategies(
a, b []*api_v2.OperationSamplingStrategy,
Expand Down
Loading

0 comments on commit a72dfc3

Please sign in to comment.