Skip to content
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

fix: Update dynamic samplers to use GetSampleRateMulti #717

Merged
merged 1 commit into from
Jun 16, 2023

Conversation

kentquirk
Copy link
Contributor

@kentquirk kentquirk commented Jun 16, 2023

Which problem is this PR solving?

  • As it says in Refinery's throughput calculations are based on traces, not spans #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.

@kentquirk kentquirk requested a review from a team as a code owner June 16, 2023 19:09
@kentquirk kentquirk changed the title Update dynamic samplers to use GetSampleRateMulti fix: Update dynamic samplers to use GetSampleRateMulti Jun 16, 2023
@kentquirk kentquirk added this to the v2.0 milestone Jun 16, 2023
Copy link
Contributor

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an exciting change to help people use the windowedthroughput sampler to enforce their rate limit, but it is a significant breaking change right? We definitely want to highlight this change in the release.

Is there anything we can do in the config converter to detect users that are affected by this change and warn them to look at their rules configs closely?

@kentquirk
Copy link
Contributor Author

I believe that I can make the config converter notice that and emit a warning. It's on the list.

@kentquirk kentquirk merged commit 253dc32 into main Jun 16, 2023
@kentquirk kentquirk deleted the kent.samplerate_multi branch June 16, 2023 19:57
kentquirk added a commit that referenced this pull request Jun 20, 2023
## 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
 ```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refinery's throughput calculations are based on traces, not spans
2 participants