memcached: Fix hitratio reporting, add connections rate report #2385

rpv-tomsk · 2017-07-30T12:27:41Z

When Collectd calculates 'hitratio', it divides two continiously-grown values.
So, reported metric contains the average since 'memcached' start, which is incorrect.
Collectd should report actual hitratio, not average for weeks-months-years.

The second commit adds connections rate reporting.

The value of hitratio is calculated as get_hits / cmd_get , unlike the rest, incr_hitratio and decr_hitratio, which are calculated like incr_hits / (incr_hits + incr_misses).

The get_hits / (get_hits + get_misses) formula may be used to unify the code, if use only these variables. But new get_* stats was added in memcached: the get_expired in memcached-1.4.26 , and get_flushed in memcached-1.4.28, so that unification cannot be applied.

rpv-tomsk · 2017-09-06T07:37:54Z

Ping.

octo

Hi @rpv-tomsk, thanks for fixing this :)

octo · 2017-09-26T14:36:50Z

src/memcached.c

@@ -285,6 +297,47 @@ static void submit_gauge2(const char *type, const char *type_inst,
  plugin_dispatch_values(&vl);
 }

+static gauge_t calculate_rate(gauge_t part, gauge_t total, gauge_t *prev_part,


I realize you copied this from the previous implementation, but "rate" is incorrect and confusing. What you're calculating here is a ratio.

octo · 2017-09-26T14:39:48Z

src/memcached.c

+  if (num == 0 || denom == 0)
+    return 0;
+
+  return 100.0 * num / denom;


Either rename this function to …_ratio and return num / denom here, or use …_ratio_percent and return 100.0 * num / denom.

Renamed to ..._ratio_percent.

Renamed to .._ratio_percent.

octo · 2017-09-26T14:41:03Z

src/memcached.c

@@ -285,6 +297,47 @@ static void submit_gauge2(const char *type, const char *type_inst,
  plugin_dispatch_values(&vl);
 }

+static gauge_t calculate_rate(gauge_t part, gauge_t total, gauge_t *prev_part,


I find it a bit unintuitive that you're effectively calculating the rate of gauge_t. It looks like the raw metrics are integers, can you change these functions to take derive_t instead?

The current code has following variable declarations:

gauge_t hits = NAN; gauge_t gets = NAN; gauge_t incr_hits = NAN;

e8fdf93#diff-e842cfa6e2070dbfa4c72d77b5b2fc71R351

These variables are passed to calculate_ratio() and calculate_ratio2() functions, so they take gauge_t type as as argument. What to do?

Convert those variables to derive_t, too. I'm fairly certain they were only gauge_t so rate = 100.0 * hits / gets; could be calculated without a cast.

Updated, variables converted to derive_t.

octo · 2017-09-26T14:42:28Z

src/memcached.c

+  *prev_total = total;
+
+  if (num == 0 || denom == 0)
+    return 0;


If there were no requests, the ratio should be NAN, not zero. Otherwise this will skew e.g. averages.

Updated, thanks!

octo · 2017-09-26T14:46:33Z

src/memcached.c

@@ -285,6 +297,47 @@ static void submit_gauge2(const char *type, const char *type_inst,
  plugin_dispatch_values(&vl);
 }

+static gauge_t calculate_rate(gauge_t part, gauge_t total, gauge_t *prev_part,


Please consider using value_to_rate() to convert both, "part" and "total" to a rate and then simply calculating the ratio as part_rate / total_rate.

I prefer to leave this untouched. I think use of value_to_rate() is excessive for this task.
That adds time calculations, which will be reducted later as the numerator and denominator will have a same value.

Fair enough, SGTM

octo · 2017-09-26T14:53:51Z

FYI, I've cherry-picked 7590014 as 66ac89c.

When Collectd calculates 'hitratio', it divides two continiously-grown values of Memcached stats. As result, reported metric contains the average since Memcached start, which is incorrect.

rpv-tomsk · 2017-10-09T14:50:06Z

Hi, Florian!

I updated this. All suggestions are accepted except proposal of value_to_rate() usage (see inline comment).

Thanks for a review.

octo · 2017-10-09T15:50:45Z

src/memcached.c

+
+static gauge_t calculate_ratio_percent2(derive_t part1, derive_t part2,
+                                        derive_t *prev1, derive_t *prev2) {
+  if (isnan(*prev1) || isnan(*prev2) || (part1 < *prev1) || (part2 < *prev2)) {


src/memcached.c: In function 'calculate_ratio_percent2': src/memcached.c:417:3: error: non-floating-point argument in call to function '__builtin_isnan' if (isnan(*prev1) || isnan(*prev2) || (part1 < *prev1) || (part2 < *prev2)) { ^

I think (*prev1 == 0) || (*prev2 == 0) should do the trick.

Thanks for a notice ;-)

octo · 2017-10-09T15:53:26Z

src/memcached.c

@@ -285,6 +297,47 @@ static void submit_gauge2(const char *type, const char *type_inst,
  plugin_dispatch_values(&vl);
 }

+static gauge_t calculate_rate(gauge_t part, gauge_t total, gauge_t *prev_part,


Fair enough, SGTM

octo

CI surfaced an issue. With that fixes this is good to do. Thanks @rpv-tomsk!

If there is no requests, NAN is reported as hitratio. If there is no hits - zero is reported.

octo

LGTM, thanks @rpv-tomsk!

rpv-tomsk changed the title ~~memcached: Fix hitratio reporting, report connections rate~~ memcached: Fix hitratio reporting, add connections rate report Jul 30, 2017

octo reviewed Sep 26, 2017

View reviewed changes

octo added the Bug A genuine bug label Sep 26, 2017

octo added this to the 5.8 milestone Oct 6, 2017

Pavel Rochnyack added 3 commits October 9, 2017 17:23

memcached: Fix CPU usage reporting

c9bea88

memcached: Fix hitratio reporting

2394ebb

When Collectd calculates 'hitratio', it divides two continiously-grown values of Memcached stats. As result, reported metric contains the average since Memcached start, which is incorrect.

memcached: Renamed calculate_rate to calculate_ratio_percent

f2d99ca

rpv-tomsk force-pushed the fix-memcached branch from 7590014 to c70e4fd Compare October 9, 2017 14:35

octo reviewed Oct 9, 2017

View reviewed changes

memcached: Replace gauge_t with derive_t

e0f43d2

rpv-tomsk force-pushed the fix-memcached branch from 48af83e to cb48bb1 Compare October 9, 2017 18:36

Pavel Rochnyack added 2 commits October 10, 2017 01:37

memcached: style changes

cb48bb1

memcached: Fix hitratio calculation

60bc9b5

If there is no requests, NAN is reported as hitratio. If there is no hits - zero is reported.

octo approved these changes Oct 10, 2017

View reviewed changes

octo merged commit 60bc9b5 into collectd:master Oct 10, 2017

rpv-tomsk deleted the fix-memcached branch October 31, 2018 17:21

octo added Fix A pull request fixing a bug and removed Bug A genuine bug labels Jan 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

memcached: Fix hitratio reporting, add connections rate report #2385

memcached: Fix hitratio reporting, add connections rate report #2385

rpv-tomsk commented Jul 30, 2017 •

edited

rpv-tomsk commented Sep 6, 2017

octo left a comment

octo Sep 26, 2017

rpv-tomsk Oct 9, 2017

octo Sep 26, 2017

rpv-tomsk Oct 9, 2017

rpv-tomsk Oct 9, 2017

octo Sep 26, 2017 •

edited

rpv-tomsk Sep 26, 2017 •

edited

octo Sep 27, 2017

rpv-tomsk Oct 9, 2017

octo Sep 26, 2017

rpv-tomsk Oct 9, 2017

octo Sep 26, 2017

rpv-tomsk Oct 9, 2017

octo Oct 9, 2017

octo commented Sep 26, 2017

rpv-tomsk commented Oct 9, 2017 •

edited

octo Oct 9, 2017

rpv-tomsk Oct 9, 2017

octo Oct 9, 2017

octo left a comment

octo left a comment

memcached: Fix hitratio reporting, add connections rate report #2385

memcached: Fix hitratio reporting, add connections rate report #2385

Conversation

rpv-tomsk commented Jul 30, 2017 • edited

rpv-tomsk commented Sep 6, 2017

octo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octo Sep 26, 2017 • edited

Choose a reason for hiding this comment

rpv-tomsk Sep 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octo commented Sep 26, 2017

rpv-tomsk commented Oct 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octo left a comment

Choose a reason for hiding this comment

octo left a comment

Choose a reason for hiding this comment

rpv-tomsk commented Jul 30, 2017 •

edited

octo Sep 26, 2017 •

edited

rpv-tomsk Sep 26, 2017 •

edited

rpv-tomsk commented Oct 9, 2017 •

edited