Skip to content

Commit

Permalink
fix: Update dynamic samplers to use GetSampleRateMulti (#717)
Browse files Browse the repository at this point in the history
## Which problem is this PR solving?

- As it says in #636, throughput and dynamic sampler calculations have
been based on the wrong numbers for some time. They've been counting
traces, not spans, and yet everything about Honeycomb is based on span
count. Most people think it works by counting spans and are then
surprised when sample rates seem incorrect for their volume, and in
fact, our documentation says that it works this way.

## Short description of the changes

- This changes Refinery's dynamic and throughput sampler requests to use
the span count, not the trace count, when calculating numbers for the
dynamic samplers and throughput samplers.

This will need to be reflected in updated documentation.

Closes #636.
  • Loading branch information
kentquirk authored Jun 16, 2023
1 parent cfa217d commit 253dc32
Show file tree
Hide file tree
Showing 5 changed files with 21 additions and 5 deletions.
4 changes: 3 additions & 1 deletion sample/dynamic.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,8 @@ func (d *DynamicSampler) Start() error {

func (d *DynamicSampler) GetSampleRate(trace *types.Trace) (rate uint, keep bool, reason string, key string) {
key = d.key.build(trace)
rate = uint(d.dynsampler.GetSampleRate(key))
count := int(trace.DescendantCount())
rate = uint(d.dynsampler.GetSampleRateMulti(key, count))
if rate < 1 { // protect against dynsampler being broken even though it shouldn't be
rate = 1
}
Expand All @@ -75,6 +76,7 @@ func (d *DynamicSampler) GetSampleRate(trace *types.Trace) (rate uint, keep bool
"sample_rate": rate,
"sample_keep": shouldKeep,
"trace_id": trace.TraceID,
"span_count": count,
}).Logf("got sample rate and decision")
if shouldKeep {
d.Metrics.Increment(d.prefix + "num_kept")
Expand Down
4 changes: 3 additions & 1 deletion sample/dynamic_ema.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ func (d *EMADynamicSampler) Start() error {

func (d *EMADynamicSampler) GetSampleRate(trace *types.Trace) (rate uint, keep bool, reason string, key string) {
key = d.key.build(trace)
rate = uint(d.dynsampler.GetSampleRate(key))
count := int(trace.DescendantCount())
rate = uint(d.dynsampler.GetSampleRateMulti(key, count))
if rate < 1 { // protect against dynsampler being broken even though it shouldn't be
rate = 1
}
Expand All @@ -83,6 +84,7 @@ func (d *EMADynamicSampler) GetSampleRate(trace *types.Trace) (rate uint, keep b
"sample_rate": rate,
"sample_keep": shouldKeep,
"trace_id": trace.TraceID,
"span_count": count,
}).Logf("got sample rate and decision")
if shouldKeep {
d.Metrics.Increment(d.prefix + "num_kept")
Expand Down
4 changes: 3 additions & 1 deletion sample/ema_throughput.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ func (d *EMAThroughputSampler) Start() error {

func (d *EMAThroughputSampler) GetSampleRate(trace *types.Trace) (rate uint, keep bool, reason string, key string) {
key = d.key.build(trace)
rate = uint(d.dynsampler.GetSampleRate(key))
count := int(trace.DescendantCount())
rate = uint(d.dynsampler.GetSampleRateMulti(key, count))
if rate < 1 { // protect against dynsampler being broken even though it shouldn't be
rate = 1
}
Expand All @@ -87,6 +88,7 @@ func (d *EMAThroughputSampler) GetSampleRate(trace *types.Trace) (rate uint, kee
"sample_rate": rate,
"sample_keep": shouldKeep,
"trace_id": trace.TraceID,
"span_count": count,
}).Logf("got sample rate and decision")
if shouldKeep {
d.Metrics.Increment(d.prefix + "num_kept")
Expand Down
4 changes: 3 additions & 1 deletion sample/totalthroughput.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,8 @@ func (d *TotalThroughputSampler) Start() error {

func (d *TotalThroughputSampler) GetSampleRate(trace *types.Trace) (rate uint, keep bool, reason string, key string) {
key = d.key.build(trace)
rate = uint(d.dynsampler.GetSampleRate(key))
count := int(trace.DescendantCount())
rate = uint(d.dynsampler.GetSampleRateMulti(key, count))
if rate < 1 { // protect against dynsampler being broken even though it shouldn't be
rate = 1
}
Expand All @@ -79,6 +80,7 @@ func (d *TotalThroughputSampler) GetSampleRate(trace *types.Trace) (rate uint, k
"sample_rate": rate,
"sample_keep": shouldKeep,
"trace_id": trace.TraceID,
"span_count": count,
}).Logf("got sample rate and decision")
if shouldKeep {
d.Metrics.Increment(d.prefix + "num_kept")
Expand Down
10 changes: 9 additions & 1 deletion sample/windowed_throughput.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,19 @@ func (d *WindowedThroughputSampler) Start() error {

func (d *WindowedThroughputSampler) GetSampleRate(trace *types.Trace) (rate uint, keep bool, reason string, key string) {
key = d.key.build(trace)
rate = uint(d.dynsampler.GetSampleRate(key))
count := int(trace.DescendantCount())
rate = uint(d.dynsampler.GetSampleRateMulti(key, count))
if rate < 1 { // protect against dynsampler being broken even though it shouldn't be
rate = 1
}
shouldKeep := rand.Intn(int(rate)) == 0
d.Logger.Debug().WithFields(map[string]interface{}{
"sample_key": key,
"sample_rate": rate,
"sample_keep": shouldKeep,
"trace_id": trace.TraceID,
"span_count": count,
}).Logf("got sample rate and decision")
if shouldKeep {
d.Metrics.Increment(d.prefix + "num_kept")
} else {
Expand Down

0 comments on commit 253dc32

Please sign in to comment.