fix: Prevent filtered traces from biasing the sample rate #1018
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
The `URLFilter` and `MethodsFilter` implementations are side-effect free, and are safe to run in any order. However, `sampler.shouldTrace` is not, a result of `true` from it has the side effect of altering the trace window. Expected behavior ================= RPCs to a filtered URL or method should not affect sample rates of unfiltered URLs or methods. Actual behavior =============== A high rate of RPCs relative to the sample rate that are filtered causes the actual sampling to be biased and below the expected rate. Suppose that external RPCs occur at the same frequency as interval as health checks: the resulting sample rate will be approximately 1/2 the specified rate, because it will be (about) a coin flip whether the first request in an interval is a healthcheck or a valid RPC. If the first request received within a sampling window has a filtered url, `shouldTrace` will return false, because: 1. `this.sampler.shouldTrace` will return true 2. `this.urlFilter.shouldTrace` will return false 3. the && operation short-circuits and will not call `this.methodsFilter.shouldTrace` Then if the second request received is to `/foobar` immediately afterward, `shouldTrace` will return false! 1. `this.sampler.shouldTrace` returns false because the prior call changed the sampling window 2. the operation short-circuits Fix === Call the side-effecting sampler method last.
Codecov Report
@@ Coverage Diff @@
## master #1018 +/- ##
=======================================
Coverage 94.81% 94.81%
=======================================
Files 97 97
Lines 6207 6207
Branches 486 486
=======================================
Hits 5885 5885
Misses 183 183
Partials 139 139
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #1018 +/- ##
=======================================
Coverage 94.81% 94.81%
=======================================
Files 97 97
Lines 6207 6207
Branches 486 486
=======================================
Hits 5885 5885
Misses 183 183
Partials 139 139
Continue to review full report at Codecov.
|
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.
The
URLFilterandMethodsFilterimplementations are side-effectfree, and are safe to run in any order. However,
sampler.shouldTraceis not, a result of
truefrom it has the side effect of altering thetrace window.
Expected behavior
RPCs to a filtered URL or method should not affect sample rates of
unfiltered URLs or methods.
Actual behavior
A high rate of RPCs relative to the sample rate that are filtered causes
the actual sampling to be biased and below the expected rate.
Suppose that external RPCs occur at the same frequency as interval as
health checks: the resulting sample rate will be approximately 1/2 the
specified rate, because it will be (about) a coin flip whether the first
request in an interval is a healthcheck or a valid RPC.
If the first request received within a sampling window has a filtered
url,
shouldTracewill return false, because:this.sampler.shouldTracewill return truethis.urlFilter.shouldTracewill return falsethis.methodsFilter.shouldTraceThen if the second request received is to
/foobarimmediatelyafterward,
shouldTracewill return false!this.sampler.shouldTracereturns false because the prior callchanged the sampling window
Fix
Call the side-effecting sampler method last.