DSType latency: Improved after PR code review

diff --git a/src/collectd.conf.pod b/src/collectd.conf.pod
index 0f99169b4055751db48eb12e3e549af2697b7d7f..74fb977e2019867ea27834fd2f7c45ca2a07ad55 100644 (file)
--- a/src/collectd.conf.pod
+++ b/src/collectd.conf.pod
@@ -7227,8 +7227,10 @@ Sets the type used to dispatch B<LatencyPercentile> values.
 =item B<LatencyRate> I<lower_latency> I<upper_latency>
 
 Calculate and dispatch rate of latency values fall within requested interval.
-Interval specified as [I<lower_latency>,I<upper_latency>] (including boundaries).
-When I<upper_latency> value is equal to 0 then interval is [lower, infinity).
+Interval is specified as [I<lower_latency>, I<upper_latency>] (including
+boundaries). Rate cannot be reported for 0.000 latency, so I<lower_latency>
+should be 0.001 or greater. When I<upper_latency> value is equal to 0 then
+interval is [lower, infinity).
 
 Rates for different intervals can be calculated by setting this option several
 times.

diff --git a/src/daemon/utils_match.h b/src/daemon/utils_match.h
index f8aba2c54a95ab9bc87bcff89662bb965baf67d8..c44bce09e8765bb6cdeabde202ab7af092ef4f8c 100644 (file)
--- a/src/daemon/utils_match.h
+++ b/src/daemon/utils_match.h
@@ -29,7 +29,6 @@
 
 #include "plugin.h"
 #include "utils_latency.h"
-#include "utils_latency_config.h"
 
 /*
  * Each type may have 12 sub-types

diff --git a/src/daemon/utils_tail_match.c b/src/daemon/utils_tail_match.c
index 0be8cfe28e26cda47cc03d173d0d1b734b1deb9e..5dfd10ac056a792705a305895cfceb7a0c9d785e 100644 (file)
--- a/src/daemon/utils_tail_match.c
+++ b/src/daemon/utils_tail_match.c
@@ -109,13 +109,12 @@ static int simple_submit_latency (cu_match_t *match, void *user_data)
   cu_tail_match_simple_t *data = (cu_tail_match_simple_t *) user_data;
   cu_match_value_t *match_value;
   value_list_t vl = VALUE_LIST_INIT;
-  value_t values[1];
 
   match_value = (cu_match_value_t *) match_get_user_data (match);
   if (match_value == NULL)
     return (-1);
 
-  vl.values = values;
+  vl.values = &(value_t) { .gauge = NAN };
   vl.values_len = 1;
   sstrncpy (vl.host, hostname_g, sizeof (vl.host));
   sstrncpy (vl.plugin, data->plugin, sizeof (vl.plugin));
@@ -128,7 +127,7 @@ static int simple_submit_latency (cu_match_t *match, void *user_data)
   if (data->latency_config.lower) {
     ssnprintf (vl.type_instance, sizeof (vl.type_instance),
         "lower");
-    values[0].gauge = (match_value->values_num != 0)
+    vl.values[0].gauge = (match_value->values_num != 0)
       ? CDTIME_T_TO_DOUBLE (latency_counter_get_min (match_value->latency))
       : NAN;
     plugin_dispatch_values (&vl);
@@ -137,7 +136,7 @@ static int simple_submit_latency (cu_match_t *match, void *user_data)
   if (data->latency_config.avg) {
     ssnprintf (vl.type_instance, sizeof (vl.type_instance),
         "average");
-    values[0].gauge = (match_value->values_num != 0)
+    vl.values[0].gauge = (match_value->values_num != 0)
       ? CDTIME_T_TO_DOUBLE (latency_counter_get_average (match_value->latency))
       : NAN;
     plugin_dispatch_values (&vl);
@@ -146,21 +145,20 @@ static int simple_submit_latency (cu_match_t *match, void *user_data)
   if (data->latency_config.upper) {
     ssnprintf (vl.type_instance, sizeof (vl.type_instance),
         "upper");
-    values[0].gauge = (match_value->values_num != 0)
+    vl.values[0].gauge = (match_value->values_num != 0)
       ? CDTIME_T_TO_DOUBLE (latency_counter_get_max (match_value->latency))
       : NAN;
     plugin_dispatch_values (&vl);
   }
 
-  size_t i;
   /* Submit percentiles */
   if (data->latency_config.percentile_type != NULL)
     sstrncpy (vl.type, data->latency_config.percentile_type, sizeof (vl.type));
-  for (i = 0; i < data->latency_config.percentile_num; i++)
+  for (size_t i = 0; i < data->latency_config.percentile_num; i++)
   {
     ssnprintf (vl.type_instance, sizeof (vl.type_instance),
         "percentile-%.0f",  data->latency_config.percentile[i]);
-    values[0].gauge = (match_value->values_num != 0)
+    vl.values[0].gauge = (match_value->values_num != 0)
       ? CDTIME_T_TO_DOUBLE (latency_counter_get_percentile (match_value->latency,
                                             data->latency_config.percentile[i]))
       : NAN;
@@ -171,13 +169,13 @@ static int simple_submit_latency (cu_match_t *match, void *user_data)
   sstrncpy (vl.type, data->type, sizeof (vl.type));
   if (data->latency_config.rates_type != NULL)
     sstrncpy (vl.type, data->latency_config.rates_type, sizeof (vl.type));
-  for (i = 0; i < data->latency_config.rates_num; i++)
+  for (size_t i = 0; i < data->latency_config.rates_num; i++)
   {
     ssnprintf (vl.type_instance, sizeof (vl.type_instance),
         "rate-%.3f-%.3f",
         CDTIME_T_TO_DOUBLE(data->latency_config.rates[i * 2]),
         CDTIME_T_TO_DOUBLE(data->latency_config.rates[i * 2 + 1]));
-    values[0].gauge = (match_value->values_num != 0) 
+    vl.values[0].gauge = (match_value->values_num != 0) 
       ? latency_counter_get_rate (match_value->latency,
                                   data->latency_config.rates[i * 2],
                                   data->latency_config.rates[i * 2 + 1],

diff --git a/src/daemon/utils_tail_match.h b/src/daemon/utils_tail_match.h
index 5601fcbd6e6c5c7f4887e59af11132dc4344087b..ffb4999b7e153092b434e057b26ec03894e1c6f1 100644 (file)
--- a/src/daemon/utils_tail_match.h
+++ b/src/daemon/utils_tail_match.h
@@ -34,6 +34,7 @@
  */
 
 #include "utils_match.h"
+#include "utils_latency_config.h"
 
 struct cu_tail_match_s;
 typedef struct cu_tail_match_s cu_tail_match_t;

diff --git a/src/tail.c b/src/tail.c
index 5326e7c18c7910e197fd97d0a6ad43c8ca879a64..022ecfd19c0eb2df6831ad2d18fdb3d6c7f94ff4 100644 (file)
--- a/src/tail.c
+++ b/src/tail.c
@@ -94,8 +94,7 @@ static int ctail_config_add_match_dstype (ctail_config_match_t *cm,
   }
   else if (strcasecmp ("Latency", ci->values[0].value.string) == 0)
   {
-    cm->flags = UTILS_MATCH_DS_TYPE_GAUGE;
-    cm->flags |= UTILS_MATCH_CF_GAUGE_LATENCY;
+    cm->flags = UTILS_MATCH_DS_TYPE_GAUGE | UTILS_MATCH_CF_GAUGE_LATENCY;
   }
   else if (strncasecmp ("Counter", ci->values[0].value.string, strlen ("Counter")) == 0)
   {

diff --git a/src/utils_latency.c b/src/utils_latency.c
index 1275f8909ca1f46f8341066daac0eac1915f3422..30f13c70d1d869cc056c70879801e1fab25bc018 100644 (file)
--- a/src/utils_latency.c
+++ b/src/utils_latency.c
@@ -152,7 +152,7 @@ void latency_counter_add (latency_counter_t *lc, cdtime_t latency) /* {{{ */
   if (lc->max < latency)
     lc->max = latency;
 
-  /* A latency of _exactly_ 1.0 ms should be stored in the buffer 0, so
+  /* A latency of _exactly_ 1.0 ms is stored in the buffer 0, so
    * subtract one from the cdtime_t value so that exactly 1.0 ms get sorted
    * accordingly. */
   bin = (latency - 1) / lc->bin_width;
@@ -317,9 +317,7 @@ double latency_counter_get_rate (const latency_counter_t *lc, /* {{{ */
 {
   cdtime_t lower_bin;
   cdtime_t upper_bin;
-  double p;
   double sum = 0;
-  size_t i;
 
   if ((lc == NULL) || (lc->num == 0))
     return (0);
@@ -333,7 +331,7 @@ double latency_counter_get_rate (const latency_counter_t *lc, /* {{{ */
   if (upper && (upper < lower))
     return (0);
 
-  /* A latency of _exactly_ 1.0 ms should be stored in the buffer 0 */
+  /* A latency of _exactly_ 1.0 ms is stored in the buffer 0 */
   lower_bin = (lower - 1) / lc->bin_width;
 
   if (upper)
@@ -350,23 +348,33 @@ double latency_counter_get_rate (const latency_counter_t *lc, /* {{{ */
   }
 
   sum = 0;
-  for (i = lower_bin; i <= upper_bin; i++)
+  for (size_t i = lower_bin; i <= upper_bin; i++)
   {
     sum += lc->histogram[i];
   }
 
-  p = ((double)lower - (double)(lower_bin + 0) * (double)lc->bin_width - (double)DOUBLE_TO_CDTIME_T(0.001)) / (double)lc->bin_width;
-  sum -= p * lc->histogram[lower_bin];
+  /* Approximate ratio of requests below "lower" */
+  cdtime_t lower_bin_boundary = lower_bin * lc->bin_width;
+
+  /* When bin width is 0.125 (for example), then bin 0 stores
+   * values for interval [0, 0.124) (excluding).
+   * With lower = 0.100, the ratio should be 0.099 / 0.125.
+   * I.e. ratio = 0.100 - 0.000 - 0.001
+   */
+  double ratio = (double)(lower - lower_bin_boundary - DOUBLE_TO_CDTIME_T(0.001))
+                  / (double)lc->bin_width;
+  sum -= ratio * lc->histogram[lower_bin];
 
-  if (upper && upper < (upper_bin + 1) * lc->bin_width)
+  /* Approximate ratio of requests above "upper" */
+  cdtime_t upper_bin_boundary = (upper_bin + 1) * lc->bin_width;
+  if (upper)
   {
-    // p = ((upper_bin + 1) * bin_width - upper ) / bin_width;
-    p = ((double)(upper_bin + 1) * (double)lc->bin_width - (double)upper) / (double)lc->bin_width;
-    sum -= p * lc->histogram[upper_bin];
+    assert (upper <= upper_bin_boundary);
+    double ratio = (double)(upper_bin_boundary - upper) / (double)lc->bin_width;
+    sum -= ratio * lc->histogram[upper_bin];
   }
-  return sum / (CDTIME_T_TO_DOUBLE (now - lc->start_time));
 
+  return sum / (CDTIME_T_TO_DOUBLE (now - lc->start_time));
 } /* }}} double latency_counter_get_rate */
 
-
 /* vim: set sw=2 sts=2 et fdm=marker : */

diff --git a/src/utils_latency_config.c b/src/utils_latency_config.c
index 7504aafb966ed49315cd3acbaab50a1bf32f2520..2121e54673cf8366588ac8343b0014e078444050 100644 (file)
--- a/src/utils_latency_config.c
+++ b/src/utils_latency_config.c
@@ -29,13 +29,13 @@
 #include "common.h"
 #include "utils_latency_config.h"
 
-int latency_config_add_percentile (const char *plugin, latency_config_t *cl,
-    oconfig_item_t *ci)
+int latency_config_add_percentile(const char *plugin, latency_config_t *cl,
+                                  oconfig_item_t *ci)
 {
   if ((ci->values_num != 1) || (ci->values[0].type != OCONFIG_TYPE_NUMBER))
   {
-    ERROR ("%s plugin: \"%s\" requires exactly one numeric argument.",
-             plugin, ci->key);
+    ERROR("%s plugin: \"%s\" requires exactly one numeric argument.", plugin,
+          ci->key);
     return (-1);
   }
 
@@ -44,16 +44,17 @@ int latency_config_add_percentile (const char *plugin, latency_config_t *cl,
 
   if ((percent <= 0.0) || (percent >= 100))
   {
-    ERROR ("%s plugin: The value for \"%s\" must be between 0 and 100, "
-        "exclusively.", plugin, ci->key);
+    ERROR("%s plugin: The value for \"%s\" must be between 0 and 100, "
+          "exclusively.",
+          plugin, ci->key);
     return (ERANGE);
   }
 
-  tmp = realloc (cl->percentile,
-      sizeof (*cl->percentile) * (cl->percentile_num + 1));
+  tmp = realloc(cl->percentile,
+                sizeof(*cl->percentile) * (cl->percentile_num + 1));
   if (tmp == NULL)
   {
-    ERROR ("%s plugin: realloc failed.", plugin);
+    ERROR("%s plugin: realloc failed.", plugin);
     return (ENOMEM);
   }
   cl->percentile = tmp;
@@ -63,31 +64,29 @@ int latency_config_add_percentile (const char *plugin, latency_config_t *cl,
   return (0);
 } /* int latency_config_add_percentile */
 
-int latency_config_add_rate (const char *plugin, latency_config_t *cl,
-    oconfig_item_t *ci)
+int latency_config_add_rate(const char *plugin, latency_config_t *cl,
+                            oconfig_item_t *ci)
 {
-
   if ((ci->values_num != 2)
-    ||(ci->values[0].type != OCONFIG_TYPE_NUMBER)
-    ||(ci->values[1].type != OCONFIG_TYPE_NUMBER))
+      || (ci->values[0].type != OCONFIG_TYPE_NUMBER)
+      || (ci->values[1].type != OCONFIG_TYPE_NUMBER))
   {
-    ERROR ("%s plugin: \"%s\" requires exactly two numeric arguments.",
-             plugin, ci->key);
+    ERROR("%s plugin: \"%s\" requires exactly two numeric arguments.", plugin,
+          ci->key);
     return (-1);
   }
 
   if (ci->values[1].value.number &&
       ci->values[1].value.number <= ci->values[0].value.number)
   {
-    ERROR ("%s plugin: MIN must be less than MAX in \"%s\".",
-             plugin, ci->key);
+    ERROR("%s plugin: MIN must be less than MAX in \"%s\".", plugin, ci->key);
     return (-1);
   }
 
   if (ci->values[0].value.number < 0.001)
   {
-    ERROR ("%s plugin: MIN must be greater or equal to 0.001 in \"%s\".",
-             plugin, ci->key);
+    ERROR("%s plugin: MIN must be greater or equal to 0.001 in \"%s\".", plugin,
+          ci->key);
     return (-1);
   }
 
@@ -95,11 +94,10 @@ int latency_config_add_rate (const char *plugin, latency_config_t *cl,
   cdtime_t upper = DOUBLE_TO_CDTIME_T(ci->values[1].value.number);
   cdtime_t *tmp;
 
-  tmp = realloc (cl->rates,
-      sizeof (*cl->rates) * (cl->rates_num + 1) * 2);
+  tmp = realloc(cl->rates, sizeof(*cl->rates) * (cl->rates_num + 1) * 2);
   if (tmp == NULL)
   {
-    ERROR ("%s plugin: realloc failed.", plugin);
+    ERROR("%s plugin: realloc failed.", plugin);
     return (ENOMEM);
   }
   cl->rates = tmp;
@@ -110,54 +108,63 @@ int latency_config_add_rate (const char *plugin, latency_config_t *cl,
   return (0);
 } /* int latency_config_add_rate */
 
-
-int latency_config_copy (latency_config_t *dst, const latency_config_t src)
+int latency_config_copy(latency_config_t *dst, const latency_config_t src)
 {
+  *dst = (latency_config_t){
+    .rates = NULL,
+    .rates_num = src.rates_num,
+    .rates_type = NULL,
+    .percentile = NULL,
+    .percentile_num = src.percentile_num,
+    .percentile_type = NULL,
+  };
+
   /* Copy percentiles configuration */
   dst->percentile_num = src.percentile_num;
-  dst->percentile = malloc(sizeof (*dst->percentile) * (src.percentile_num));
+  dst->percentile = calloc(src.percentile_num, sizeof(*dst->percentile));
   if (dst->percentile == NULL)
-    goto nomem;
+    return (-1);
 
-  memcpy (dst->percentile, src.percentile,
-          (sizeof (*dst->percentile) * (src.percentile_num)));
+  memcpy(dst->percentile, src.percentile,
+         (sizeof(*dst->percentile) * (src.percentile_num)));
 
   if (src.percentile_type != NULL)
   {
     dst->percentile_type = strdup(src.percentile_type);
     if (dst->percentile_type == NULL)
-      goto nomem;
+    {
+      latency_config_free(*dst);
+      return (-1);
+    }
   }
 
   /* Copy rates configuration */
   dst->rates_num = src.rates_num;
-  dst->rates = malloc(sizeof (*dst->rates) * (src.rates_num) * 2);
+  dst->rates = calloc(src.rates_num * 2, sizeof(*dst->rates));
   if (dst->rates == NULL)
-    goto nomem;
+  {
+    latency_config_free(*dst);
+    return (-1);
+  }
 
-  memcpy (dst->rates, src.rates,
-          (sizeof (*dst->rates) * (src.rates_num) * 2));
+  memcpy(dst->rates, src.rates, (sizeof(*dst->rates) * (src.rates_num) * 2));
 
   if (src.rates_type != NULL)
   {
     dst->rates_type = strdup(src.rates_type);
     if (dst->rates_type == NULL)
-      goto nomem;
+    {
+      latency_config_free(*dst);
+      return (-1);
+    }
   }
 
   return (0);
-nomem:
-  free (dst->rates);
-  free (dst->rates_type);
-  free (dst->percentile);
-  free (dst->percentile_type);
-  return (-1);
 } /* int latency_config_copy */
 
-void latency_config_free (latency_config_t lc)
-{
-  sfree (lc.rates);
-  sfree (lc.rates_type);
-  sfree (lc.percentile);
-  sfree (lc.percentile_type);
+void latency_config_free(latency_config_t lc) {
+  sfree(lc.rates);
+  sfree(lc.rates_type);
+  sfree(lc.percentile);
+  sfree(lc.percentile_type);
 } /* void latency_config_free */

diff --git a/src/utils_latency_config.h b/src/utils_latency_config.h
index 283ce1c4860afea1bbff81670c04b471ba9a8847..aad8914cb7bfff94b944139670484525a42cf479 100644 (file)
--- a/src/utils_latency_config.h
+++ b/src/utils_latency_config.h
@@ -33,29 +33,28 @@
 
 struct latency_config_s
 {
-  double *percentile;
-  size_t percentile_num;
-  char   *percentile_type;
+  double   *percentile;
+  size_t   percentile_num;
+  char     *percentile_type;
   cdtime_t *rates;
   size_t   rates_num;
   char     *rates_type;
-  _Bool lower;
-  _Bool upper;
+  _Bool    lower;
+  _Bool    upper;
   //_Bool sum;
-  _Bool avg;
+  _Bool    avg;
   //_Bool count;
 };
 typedef struct latency_config_s latency_config_t;
 
+int latency_config_add_percentile(const char *plugin, latency_config_t *cl,
+                                  oconfig_item_t *ci);
 
-int latency_config_add_percentile (const char *plugin, latency_config_t *cl,
-    oconfig_item_t *ci);
+int latency_config_add_rate(const char *plugin, latency_config_t *cl,
+                            oconfig_item_t *ci);
 
-int latency_config_add_rate (const char *plugin, latency_config_t *cl,
-    oconfig_item_t *ci);
+int latency_config_copy(latency_config_t *dst, const latency_config_t src);
 
-int latency_config_copy (latency_config_t *dst, const latency_config_t src);
-
-void latency_config_free (latency_config_t lc);
+void latency_config_free(latency_config_t lc);
 
 #endif /* UTILS_LATENCY_CONFIG_H */

diff --git a/src/utils_latency_test.c b/src/utils_latency_test.c
index 84fc0a29ee1a937dbf6a1e67e919180fe2550db5..2975095982bed753e9894bbde49e80e8a32233ab 100644 (file)
--- a/src/utils_latency_test.c
+++ b/src/utils_latency_test.c
@@ -105,7 +105,7 @@ DEF_TEST (rate) {
   for (i = 0; i < 125; i++) {
     latency_counter_add (l, TIME_T_TO_CDTIME_T (((time_t) i) + 1));
   }
-  //Test expects bin width equal to 0.125s
+  //Test expects bin width will be equal to 0.125s
 
   EXPECT_EQ_DOUBLE (1/125, latency_counter_get_rate (l,
       DOUBLE_TO_CDTIME_T(10),