From dev-return-693-archive-asf-public=cust-asf.ponee.io@zipkin.apache.org Mon May 13 15:59:07 2019 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by mx-eu-01.ponee.io (Postfix) with SMTP id 34F5B180671 for ; Mon, 13 May 2019 17:59:07 +0200 (CEST) Received: (qmail 71419 invoked by uid 500); 13 May 2019 15:59:06 -0000 Mailing-List: contact dev-help@zipkin.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@zipkin.apache.org Delivered-To: mailing list dev@zipkin.apache.org Received: (qmail 71407 invoked by uid 99); 13 May 2019 15:59:06 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 13 May 2019 15:59:06 +0000 From: GitBox To: dev@zipkin.apache.org Subject: [GitHub] [incubator-zipkin-brave] pburm commented on a change in pull request #908: Fixes some edge cases around sampler resetting Message-ID: <155776314648.26731.4078017264089479001.gitbox@gitbox.apache.org> Date: Mon, 13 May 2019 15:59:06 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit 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 and 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: users@infra.apache.org With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscribe@zipkin.apache.org For additional commands, e-mail: dev-help@zipkin.apache.org