pburm commented on a change in pull request #908: Fixes some edge cases around
sampler resetting
URL:
https://github.com/apache/incubator-zipkin-brave/pull/908#discussion_r283418212
##########
File path: brave/src/main/java/brave/sampler/RateLimitingSampler.java
##########
@@ -55,14 +55,17 @@ public static Sampler create(int tracesPerSecond) {
long updateAt = nextReset.get();
long nanosUntilReset = -(now - updateAt); // because nanoTime can be
negative
- boolean shouldReset = nanosUntilReset <= 0;
- if (shouldReset) {
- if (nextReset.compareAndSet(updateAt, updateAt + NANOS_PER_SECOND)) {
- usage.set(0);
- }
+ if (nanosUntilReset <= 0) {
+ // Attempt to move into the next sampling interval.
+ // nanosUntilReset is now invalid regardless of race winner, so we can't
sample based on it.
+ if (nextReset.compareAndSet(updateAt, updateAt + NANOS_PER_SECOND))
usage.set(0);
Review comment:
Only hole that I see here is that since we are ONLY adding one second to
updateAt, when the sampler is long lived and there have not been any sampling
decisions in several seconds......it will sample at 100% until updateAt has
caught up. We forked at added + -(nanosUntilReset) in addition to
NANOS_PER_SECOND when we update updateAt. That might push it too far for
somebody's taste, but it cured the 100% sampling in our implementation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
