From 792ffa22a45b197676837772e29348abd104d925 Mon Sep 17 00:00:00 2001 From: Pavel Rochnyack Date: Mon, 19 Sep 2016 23:32:54 +0600 Subject: [PATCH] DSType latency: Improved after PR code review --- src/collectd.conf.pod | 6 ++- src/daemon/utils_match.h | 1 - src/daemon/utils_tail_match.c | 18 ++++---- src/daemon/utils_tail_match.h | 1 + src/tail.c | 3 +- src/utils_latency.c | 34 ++++++++------ src/utils_latency_config.c | 101 ++++++++++++++++++++++-------------------- src/utils_latency_config.h | 25 +++++------ src/utils_latency_test.c | 2 +- 9 files changed, 102 insertions(+), 89 deletions(-) diff --git a/src/collectd.conf.pod b/src/collectd.conf.pod index cb18c6b6..a592ed32 100644 --- a/src/collectd.conf.pod +++ b/src/collectd.conf.pod @@ -7088,8 +7088,10 @@ Sets the type used to dispatch B values. =item B I I Calculate and dispatch rate of latency values fall within requested interval. -Interval specified as [I,I] (including boundaries). -When I value is equal to 0 then interval is [lower, infinity). +Interval is specified as [I, I] (including +boundaries). Rate cannot be reported for 0.000 latency, so I +should be 0.001 or greater. When I 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 318c676c..f6c57f8a 100644 --- 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 87754e18..90a2dab2 100644 --- a/src/daemon/utils_tail_match.c +++ b/src/daemon/utils_tail_match.c @@ -115,13 +115,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)); @@ -134,7 +133,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); @@ -143,7 +142,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); @@ -152,21 +151,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; @@ -177,13 +175,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 5601fcbd..ffb4999b 100644 --- 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 3c8d62a0..68689cdf 100644 --- a/src/tail.c +++ b/src/tail.c @@ -92,8 +92,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 1275f890..30f13c70 100644 --- 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 7504aafb..2121e546 100644 --- 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 283ce1c4..aad8914c 100644 --- 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 84fc0a29..29750959 100644 --- 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), -- 2.11.0