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

feat: Warn about samplers that might need adjustment #718

Merged
merged 1 commit into from
Jun 20, 2023

Conversation

kentquirk
Copy link
Contributor

@kentquirk kentquirk commented Jun 17, 2023

Which problem is this PR solving?

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

@kentquirk kentquirk requested a review from a team as a code owner June 17, 2023 15:21
@kentquirk kentquirk added this to the v2.0 milestone Jun 17, 2023
@kentquirk kentquirk force-pushed the kent.sample_rate_warning branch from cfc28d6 to 300e7bf Compare June 19, 2023 15:29
@kentquirk kentquirk merged commit dae269d into main Jun 20, 2023
@kentquirk kentquirk deleted the kent.sample_rate_warning branch June 20, 2023 16:41
kentquirk added a commit that referenced this pull request Jun 20, 2023
Based on #718, so ready to review independently.

## Which problem is this PR solving?

- Up to now, setting MaxAlloc requires typing or copy-pasting large
numbers that are hard to read and visually parse. This PR allows using
standard suffixes like "Gb" and "MiB". This responds to a request from
Honeycomb field engineering.

## Short description of the changes

- Import the docker library for parsing memory sizes
- Create a MemorySize type with marshal and unmarshal methods, and use
it as appropriate
- Update the metadata and templates
- Regenerate all the things
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.

2 participants