Median + latency percentile calculations are invalid due to int arithmetic

[ghost]

If I make less than 100 requests, the 50%, 90%, 98%, and 99% latency values will be the same as the minimum latency value. For example:

Minimum latency:      4013.502 milliseconds
50% latency:          4013.502 milliseconds
90% latency:          4013.502 milliseconds
98% latency:          4013.502 milliseconds
99% latency:          4013.502 milliseconds

This is due to a bug in how the latency percentages are calculated. apib is doing int arithmetic, which rounds down the index of the percentile:

getLatencyPercent method in apib/src/apib_reporting.c

static unsigned long getLatencyPercent(int percent)
{
  if (latenciesCount == 0) {
    return 0;
  }
  if (percent == 100) {
    return latencies[latenciesCount - 1];
  }
  unsigned int index = 
    (latenciesCount / 100) * percent;
  return latencies[index];
}

As you can see in (latenciesCount / 100) * percent;, (latenciesCount / 100) will always be 0 if the count is less than 100. This should be changed to float arithmetic. I was interested in using this script purely to measure median/90% latency figures, so unless this is corrected, I cannot use this script.

[gbrail]

Thanks for tracking this down -- it's great to have more people using apib and looking at the code.

I'm quite happy to merge a pull request if you have the time to make the fix. Otherwise I'll find some time to improve things.

On Thu, Aug 17, 2017 at 5:32 PM, Timothy Wang notifications@github.com wrote:

If I make less than 100 requests, the 50%, 90%, 98%, and 99% latency values will be the same as the minimum latency value. For example:

Minimum latency: 4013.502 milliseconds 50% latency: 4013.502 milliseconds 90% latency: 4013.502 milliseconds 98% latency: 4013.502 milliseconds 99% latency: 4013.502 milliseconds

This is due to a bug in how the latency percentages are calculated. apib is doing int arithmetic, which rounds down the index of the percentile:

getLatencyPercent method in apib/src/apib_reporting.c

static unsigned long getLatencyPercent(int percent) { if (latenciesCount == 0) { return 0; } if (percent == 100) { return latencies[latenciesCount - 1]; } unsigned int index = (latenciesCount / 100) * percent; return latencies[index]; }

As you can see in (latenciesCount / 100) * percent;, (latenciesCount / 100) will always be 0 if the count is less than 100. This should be changed to float arithmetic. I was interested in using this script purely to measure median/90% latency figures, so unless this is corrected, I cannot use this script.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/apigee/apib/issues/13, or mute the thread https://github.com/notifications/unsubscribe-auth/AAf0azk33t_U8brYKP_LJOYuuW4NByoTks5sZNuigaJpZM4O6-A5 .

[ghost]

@gbrail - I was able to make a local fix and verified that percentages are now calculated properly. However, I'm unable to open a PR b/c I don't have access-rights to this repo.

The below is a git diff of the change I made locally:

commit d7d1669956a1040295d2e426617e9a16e02d3fcf
Author: Tim Wang <tim.wang@grandrounds.com>
Date:   Sat Aug 19 21:50:57 2017 +0000

    correct latency percentage calculations

diff --git a/src/apib_reporting.c b/src/apib_reporting.c
index dbd1e65..defb642 100644
--- a/src/apib_reporting.c
+++ b/src/apib_reporting.c
@@ -333,7 +333,7 @@ static unsigned long getLatencyPercent(int percent)
     return latencies[latenciesCount - 1];
   }
   unsigned int index =
-    (latenciesCount / 100) * percent;
+    (latenciesCount / 100.0) * percent;
   return latencies[index];
 }

[gbrail]

Thanks for the fix! I pushed your change to master. Can you try again?

[ghost]

I confirmed that the issue has been fixed. Do you want to also push the fix to homebrew? Thanks.