Skip to content

Commit

Permalink
feat: Warn about samplers that might need adjustment (#718)
Browse files Browse the repository at this point in the history
## Which problem is this PR solving?

- Since we adjusted sampler behavior to count spans, not traces, users
may need to tweak sample rates and throughput values. This change has
the converter issue a warning in those cases.
- Resolves a concern @TylerHelmuth expressed in #717 

## Short description of the changes

- Add test for which samplers might need to be fixed
- Check it on rule conversion

The warning looks like this for the demo rules file:
```
WARNING: Version 2 of Refinery has changed the way that sample rates are calculated for
the dynamic and throughput samplers. Refinery v1.x was documented as counting the number
of spans, but in fact it was counting traces. Version 2 samplers correctly count the
number of spans when doing these calculations.

This means that you may need to adjust the target throughput or sample rates to get the
same behavior as before.

When using v1, if you have had difficulty reaching your target throughput or sample rates,
or if different parts of your key space vary widely in their span counts, then there is a
good chance you should lower the target values in v2.

The following parts of your configuration should be examined:
 *  __default__: DeterministicSampler
 *  dataset1: DynamicSampler
 *  dataset2: EMADynamicSampler
 *  dataset3: DeterministicSampler
 *  dataset4: RulesBasedSampler(dynamically sample 200 responses, downstream Sampler EMADynamicSampler)
 *  dataset4: RulesBasedSampler(dynamically sample 200 string responses, downstream Sampler EMADynamicSampler)
 *  dataset5: TotalThroughputSampler
 ```
  • Loading branch information
kentquirk authored Jun 20, 2023
1 parent c193b04 commit dae269d
Show file tree
Hide file tree
Showing 2 changed files with 103 additions and 0 deletions.
67 changes: 67 additions & 0 deletions config/sampler_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,73 @@ func (v *V2SamplerChoice) Sampler() (any, string) {
}
}

// This looks through the list of samplers and finds samplers that have
// sample rates that are nontrivial. The point of this is to determine which
// samplers are meaningful to report as ones the user might want to adjust
// because the way that sample rates are computed has changed. It's used
// by the rules converter.
func (v *V2SamplerChoice) NameMeaningfulSamplers() []string {
names := make(map[string]struct{})
switch {
case v.DeterministicSampler != nil:
names["DeterministicSampler"] = struct{}{}
case v.RulesBasedSampler != nil:
for _, rule := range v.RulesBasedSampler.Rules {
if rule.Sampler != nil {
if rule.SampleRate > 1 {
names[fmt.Sprintf("RulesBasedSampler(%s)", rule.Name)] = struct{}{}
} else if rule.Sampler.NameMeaningfulRate() != "" {
names[fmt.Sprintf("RulesBasedSampler(%s, downstream Sampler %s)", rule.Name, rule.Sampler.NameMeaningfulRate())] = struct{}{}
}
}
}
case v.DynamicSampler != nil:
names["DynamicSampler"] = struct{}{}
case v.EMADynamicSampler != nil:
names["EMADynamicSampler"] = struct{}{}
case v.EMAThroughputSampler != nil:
names["EMAThroughputSampler"] = struct{}{}
case v.WindowedThroughputSampler != nil:
names["WindowedThroughputSampler"] = struct{}{}
case v.TotalThroughputSampler != nil:
names["TotalThroughputSampler"] = struct{}{}
default:
return nil
}

r := make([]string, 0, len(names))
for name := range names {
r = append(r, name)
}
return r
}

func (v *RulesBasedDownstreamSampler) NameMeaningfulRate() string {
switch {
case v.DynamicSampler != nil:
if v.DynamicSampler.SampleRate > 1 {
return "DynamicSampler"
}
case v.EMADynamicSampler != nil:
if v.EMADynamicSampler.GoalSampleRate > 1 {
return "EMADynamicSampler"
}
case v.EMAThroughputSampler != nil:
if v.EMAThroughputSampler.GoalThroughputPerSec > 1 {
return "EMAThroughputSampler"
}
case v.WindowedThroughputSampler != nil:
if v.WindowedThroughputSampler.GoalThroughputPerSec > 0 {
return "WindowedThroughputSampler"
}
case v.TotalThroughputSampler != nil:
if v.TotalThroughputSampler.GoalThroughputPerSec > 0 {
return "TotalThroughputSampler"
}
}
return ""
}

type V2SamplerConfig struct {
RulesVersion int `json:"rulesversion" yaml:"RulesVersion" validate:"required,ge=2"`
Samplers map[string]*V2SamplerChoice `json:"samplers" yaml:"Samplers,omitempty" validate:"required"`
Expand Down
36 changes: 36 additions & 0 deletions tools/convert/ruleconvert.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,17 @@ func getValueForCaseInsensitiveKey[T any](m map[string]any, key string, def T) (
return def, false
}

func CheckConfigForSampleRateChanges(cfg *config.V2SamplerConfig) map[string][]string {
warnings := make(map[string][]string)
for name, v := range cfg.Samplers {
meaningfuls := v.NameMeaningfulSamplers()
if len(meaningfuls) > 0 {
warnings[name] = meaningfuls
}
}
return warnings
}

func ConvertRules(rules map[string]any, w io.Writer) {
// get the sampler type for the default rule
defaultSamplerType, _ := getValueForCaseInsensitiveKey(rules, "sampler", "DeterministicSampler")
Expand Down Expand Up @@ -159,4 +170,29 @@ func ConvertRules(rules map[string]any, w io.Writer) {

w.Write([]byte(fmt.Sprintf("# Automatically generated on %s\n", time.Now().Format(time.RFC3339))))
yaml.NewEncoder(w).Encode(newConfig)

warningText := `
WARNING: Version 2 of Refinery has changed the way that sample rates are calculated for
the dynamic and throughput samplers. Refinery v1.x was documented as counting the number
of spans, but in fact it was counting traces. Version 2 samplers correctly count the
number of spans when doing these calculations.
This means that you may need to adjust the target throughput or sample rates to get the
same behavior as before.
When using v1, if you have had difficulty reaching your target throughput or sample rates,
or if different parts of your key space vary widely in their span counts, then there is a
good chance you should lower the target values in v2.
The following parts of your configuration should be checked:`

thingsToWarnAbout := CheckConfigForSampleRateChanges(newConfig)
if len(thingsToWarnAbout) > 0 {
fmt.Println(warningText)
for k, v := range thingsToWarnAbout {
for _, s := range v {
fmt.Printf(" * %s: %s\n", k, s)
}
}
}
}

0 comments on commit dae269d

Please sign in to comment.