Fix tail-based sampling discarding low throughput and low sample rate traces #11642
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
carsonip
commented
Sep 13, 2023
| @@ -224,7 +224,7 @@ func (g *traceGroup) finalizeSampledTraces(traceIDs []string, ingestRateDecayFac | |||
|
|
|||
| // Resize the reservoir, so that it can hold the desired fraction of | |||
| // the observed ingest rate. | |||
| newReservoirSize := int(math.Round(g.samplingFraction * g.ingestRate)) | |||
| newReservoirSize := int(math.Ceil(g.samplingFraction * g.ingestRate)) | |||
There was a problem hiding this comment.
[For reviewer] I don't think this is needed for the fix to work, and that we have a min size. But might be good to be consistent and use math.Ceil in all samplingFraction multiplication.
kruskall
approved these changes
Sep 13, 2023
mergify bot
pushed a commit
that referenced
this pull request
Sep 14, 2023
… traces (#11642) In tail-based sampling, use math.ceil instead of math.Round when computing desired total such that on low throughput and low sample rate traces e.g. throughput=1, sample rate = 0.4, desired total = math.Ceil(1 * 0.4) = 1 as opposed to 0 when using math.Round. This will fix cases where these traces were missing before the fix. (cherry picked from commit b0e8925) # Conflicts: # changelogs/head.asciidoc
mergify bot
added a commit
that referenced
this pull request
Sep 14, 2023
… traces (#11642) (#11644) In tail-based sampling, use math.ceil instead of math.Round when computing desired total such that on low throughput and low sample rate traces e.g. throughput=1, sample rate = 0.4, desired total = math.Ceil(1 * 0.4) = 1 as opposed to 0 when using math.Round. This will fix cases where these traces were missing before the fix. (cherry picked from commit b0e8925) # Conflicts: # changelogs/head.asciidoc Co-authored-by: Carson Ip <carsonip@users.noreply.github.com>
|
Tested successfully on |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation/summary
In tail-based sampling, use math.ceil instead of math.Round when computing desired total such that on low throughput and low sample rate traces e.g. throughput=1, sample rate = 0.4, desired total = math.Ceil(1 * 0.4) = 1 as opposed to 0 when using math.Round. This will fix cases where these traces were missing before the fix.
Checklist
apmpackagehave been made)For functional changes, consider:
How to test these changes
Enable TBS, set sample rate to 0.4, send 1 trace. Check if the trace shows up.
Related issues
Closes #11372