Merge pull request #4266 from octo/6/fpcounter

[collectd 6] Introduce a floating point counter type.

src/daemon/metric.c

@@ -52,7 +52,7 @@
 int value_marshal_text(strbuf_t *buf, value_t v, metric_type_t type) {
   switch (type) {
   case METRIC_TYPE_GAUGE:
-  case METRIC_TYPE_UNTYPED:
+  case METRIC_TYPE_FPCOUNTER:
     return strbuf_printf(buf, GAUGE_FORMAT, v.gauge);
   case METRIC_TYPE_COUNTER:
     return strbuf_printf(buf, "%" PRIu64, v.counter);

src/daemon/metric.h

@@ -32,21 +32,26 @@
 #include "utils/strbuf/strbuf.h"
 #include "utils_time.h"
 
-#define VALUE_TYPE_GAUGE 1
-#define VALUE_TYPE_DERIVE 2
+#define METRIC_ATTR_DOUBLE 0x01
+#define METRIC_ATTR_CUMULATIVE 0x02
 
 typedef enum {
-  METRIC_TYPE_COUNTER = 0,
-  METRIC_TYPE_GAUGE = 1,
-  METRIC_TYPE_UNTYPED = 2,
+  METRIC_TYPE_UNTYPED = 0,
+  METRIC_TYPE_GAUGE = METRIC_ATTR_DOUBLE,
+  METRIC_TYPE_COUNTER = METRIC_ATTR_CUMULATIVE,
+  METRIC_TYPE_FPCOUNTER = METRIC_ATTR_DOUBLE | METRIC_ATTR_CUMULATIVE,
 } metric_type_t;
 
+#define IS_CUMULATIVE(t) ((t)&METRIC_ATTR_CUMULATIVE)
+
 typedef uint64_t counter_t;
+typedef double fpcounter_t;
 typedef double gauge_t;
 typedef int64_t derive_t;
 
 union value_u {
   counter_t counter;
+  fpcounter_t fpcounter;
   gauge_t gauge;
   derive_t derive;
 };

src/daemon/plugin.h

@@ -40,16 +40,6 @@
 #include <inttypes.h>
 #include <pthread.h>
 
-#define DS_TYPE_COUNTER 0
-#define DS_TYPE_GAUGE VALUE_TYPE_GAUGE
-#define DS_TYPE_DERIVE VALUE_TYPE_DERIVE
-
-#define DS_TYPE_TO_STRING(t)                                                   \
-  (t == DS_TYPE_COUNTER)  ? "counter"                                          \
-  : (t == DS_TYPE_GAUGE)  ? "gauge"                                            \
-  : (t == DS_TYPE_DERIVE) ? "derive"                                           \
-                          : "unknown"
-
 #ifndef LOG_ERR
 #define LOG_ERR 3
 #endif

src/daemon/utils_cache.c

@@ -150,27 +150,12 @@ static int uc_insert(metric_t const *m, char const *key) {
       .last_update = cdtime(),
       .interval = m->interval,
       .state = STATE_UNKNOWN,
+      .values_gauge = NAN,
   };
   sstrncpy(ce->name, key, sizeof(ce->name));
 
-  switch (m->family->type) {
-  case DS_TYPE_COUNTER:
-  case DS_TYPE_DERIVE:
-    // Non-gauge types will store the rate in values_gauge when the second data
-    // point is available. Initially, NAN signifies "not enough data".
-    ce->values_gauge = NAN;
-    break;
-
-  case DS_TYPE_GAUGE:
+  if (m->family->type == METRIC_TYPE_GAUGE) {
     ce->values_gauge = m->value.gauge;
-    break;
-
-  default:
-    /* This shouldn't happen. */
-    ERROR("uc_insert: Unexpected metric type %d.", m->family->type);
-    sfree(key_copy);
-    cache_free(ce);
-    return -1;
   }
 
   if (c_avl_insert(cache_tree, key_copy, ce) != 0) {
@@ -294,6 +279,54 @@ int uc_check_timeout(void) {
   return 0;
 } /* int uc_check_timeout */
 
+static int uc_update_rate(metric_t const *m, cache_entry_t *ce) {
+  switch (m->family->type) {
+  case METRIC_TYPE_COUNTER: {
+    // Counter overflows and counter resets are signaled to plugins by resetting
+    // "first_time". Since we can't distinguish between an overflow and a
+    // reset, we still provide a non-NAN rate value. In case of a counter
+    // reset, the rate value will likely be unreasonably huge.
+    if (ce->last_value.counter > m->value.counter) {
+      // counter reset
+      ce->first_time = m->time;
+      ce->first_value = m->value;
+    }
+    counter_t diff = counter_diff(ce->last_value.counter, m->value.counter);
+    ce->values_gauge =
+        ((double)diff) / CDTIME_T_TO_DOUBLE(m->time - ce->last_time);
+    return 0;
+  }
+
+  case METRIC_TYPE_FPCOUNTER: {
+    // For floating point counters, the logic is slightly different from
+    // integer counters. Floating point counters don't have a (meaningful)
+    // overflow, and we will always assume a counter reset.
+    if (ce->last_value.fpcounter > m->value.fpcounter) {
+      // counter reset
+      ce->first_time = m->time;
+      ce->first_value = m->value;
+      ce->values_gauge = NAN;
+      return 0;
+    }
+    gauge_t diff = m->value.fpcounter - ce->last_value.fpcounter;
+    ce->values_gauge = diff / CDTIME_T_TO_DOUBLE(m->time - ce->last_time);
+    return 0;
+  }
+
+  case METRIC_TYPE_GAUGE: {
+    ce->values_gauge = m->value.gauge;
+    return 0;
+  }
+
+  case METRIC_TYPE_UNTYPED:
+    break;
+  } /* switch (m->family->type) */
+
+  /* This shouldn't happen. */
+  ERROR("uc_update: invalid metric type: %d", m->family->type);
+  return EINVAL;
+}
+
 static int uc_update_metric(metric_t const *m) {
   strbuf_t buf = STRBUF_CREATE;
   int status = metric_identity(&buf, m);
@@ -332,48 +365,12 @@ static int uc_update_metric(metric_t const *m) {
     return -1;
   }
 
-  switch (m->family->type) {
-  case METRIC_TYPE_COUNTER: {
-    // Counter overflows and counter resets are signaled to plugins by resetting
-    // "first_time". Since we can't distinguish between an overflow and a
-    // reset, we still provide a non-NAN rate value. In case of a counter
-    // reset, the rate value will likely be unreasonably huge.
-    if (ce->last_value.counter > m->value.counter) {
-      // counter reset
-      ce->first_time = m->time;
-      ce->first_value = m->value;
-    }
-    counter_t diff = counter_diff(ce->last_value.counter, m->value.counter);
-    ce->values_gauge =
-        ((double)diff) / CDTIME_T_TO_DOUBLE(m->time - ce->last_time);
-    break;
-  }
-
-  case METRIC_TYPE_UNTYPED:
-  case METRIC_TYPE_GAUGE: {
-    ce->values_gauge = m->value.gauge;
-    break;
-  }
-
-#if 0
-  case DS_TYPE_DERIVE: { /* TODO(octo): add support for DERIVE */
-    derive_t diff = m->value.derive - ce->values_raw.derive;
-    ce->values_gauge =
-        ((double)diff) / (CDTIME_T_TO_DOUBLE(m->time - ce->last_time));
-    ce->values_raw.derive = m->value.derive;
-    break;
-  }
-#endif
-
-  default: {
-    /* This shouldn't happen. */
+  int err = uc_update_rate(m, ce);
+  if (err) {
     pthread_mutex_unlock(&cache_lock);
-    ERROR("uc_update: Don't know how to handle data source type %i.",
-          m->family->type);
     STRBUF_DESTROY(buf);
-    return -1;
+    return err;
   }
-  } /* switch (m->family->type) */
 
   DEBUG("uc_update: %s = %f", buf.ptr, ce->values_gauge);
 

src/daemon/utils_cache_test.c

@@ -86,6 +86,24 @@ DEF_TEST(uc_get_rate) {
           .type = METRIC_TYPE_COUNTER,
           .want = (23. + 18. + 1.) / (110. - 100.),
       },
+      {
+          .name = "fpcounter",
+          .first_value = (value_t){.fpcounter = 4.2},
+          .second_value = (value_t){.fpcounter = 10.2},
+          .first_time = TIME_T_TO_CDTIME_T(100),
+          .second_time = TIME_T_TO_CDTIME_T(110),
+          .type = METRIC_TYPE_FPCOUNTER,
+          .want = (10.2 - 4.2) / (110 - 100),
+      },
+      {
+          .name = "fpcounter with reset",
+          .first_value = (value_t){.fpcounter = 100000.0},
+          .second_value = (value_t){.fpcounter = 0.2},
+          .first_time = TIME_T_TO_CDTIME_T(100),
+          .second_time = TIME_T_TO_CDTIME_T(110),
+          .type = METRIC_TYPE_FPCOUNTER,
+          .want = NAN,
+      },
   };
 
   for (size_t i = 0; i < STATIC_ARRAY_SIZE(cases); i++) {

src/utils/cmds/cmds_test.c

@@ -314,8 +314,8 @@ static struct {
     {
         "PUTMETRIC untyped type=untyped 42",
         NULL,
-        CMD_OK,
-        CMD_PUTMETRIC,
+        CMD_ERROR,
+        CMD_UNKNOWN,
     },
     {
         "PUTMETRIC quoted_gauge type=\"GAUGE\" 42",
@@ -428,7 +428,7 @@ DEF_TEST(format_putmetric) {
                       },
                   .value.gauge = 42,
               },
-          .want = "PUTMETRIC test 42",
+          .want_err = EINVAL,
       },
       {
           .m =
@@ -460,33 +460,33 @@ DEF_TEST(format_putmetric) {
                   .family =
                       &(metric_family_t){
                           .name = "test",
-                          .type = METRIC_TYPE_UNTYPED,
+                          .type = METRIC_TYPE_GAUGE,
                       },
                   .value.gauge = 42,
                   .time = TIME_T_TO_CDTIME_T(1594809888),
               },
-          .want = "PUTMETRIC test time=1594809888.000 42",
+          .want = "PUTMETRIC test type=GAUGE time=1594809888.000 42",
       },
       {
           .m =
               {
                   .family =
                       &(metric_family_t){
                           .name = "test",
-                          .type = METRIC_TYPE_UNTYPED,
+                          .type = METRIC_TYPE_GAUGE,
                       },
                   .value.gauge = 42,
                   .interval = TIME_T_TO_CDTIME_T(10),
               },
-          .want = "PUTMETRIC test interval=10.000 42",
+          .want = "PUTMETRIC test type=GAUGE interval=10.000 42",
       },
       {
           .m =
               {
                   .family =
                       &(metric_family_t){
                           .name = "test",
-                          .type = METRIC_TYPE_UNTYPED,
+                          .type = METRIC_TYPE_GAUGE,
                       },
                   .value.gauge = 42,
                   .label.ptr =
@@ -496,7 +496,8 @@ DEF_TEST(format_putmetric) {
                       },
                   .label.num = 1,
               },
-          .want = "PUTMETRIC test label:foo=\"with \\\"quotes\\\"\" 42",
+          .want =
+              "PUTMETRIC test type=GAUGE label:foo=\"with \\\"quotes\\\"\" 42",
       },
   };
 

src/utils/cmds/putmetric.c

@@ -46,8 +46,8 @@ static int set_option(metric_t *m, char const *key, char const *value,
       m->family->type = METRIC_TYPE_GAUGE;
     } else if (strcasecmp("COUNTER", value) == 0) {
       m->family->type = METRIC_TYPE_COUNTER;
-    } else if (strcasecmp("UNTYPED", value) == 0) {
-      m->family->type = METRIC_TYPE_UNTYPED;
+    } else if (strcasecmp("FPCOUNTER", value) == 0) {
+      m->family->type = METRIC_TYPE_FPCOUNTER;
     } else {
       return CMD_ERROR;
     }
@@ -104,7 +104,7 @@ cmd_status_t cmd_parse_putmetric(size_t argc, char **argv,
     cmd_error(CMD_ERROR, errhndl, "calloc failed");
     return CMD_ERROR;
   }
-  fam->type = METRIC_TYPE_UNTYPED;
+  fam->type = METRIC_TYPE_GAUGE;
 
   int status = metric_family_metric_append(fam, (metric_t){0});
   if (status != 0) {
@@ -244,17 +244,18 @@ int cmd_format_putmetric(strbuf_t *buf, metric_t const *m) { /* {{{ */
   strbuf_print(buf, "PUTMETRIC ");
   strbuf_print(buf, m->family->name);
   switch (m->family->type) {
-  case METRIC_TYPE_UNTYPED:
-    /* no op */
-    break;
   case METRIC_TYPE_COUNTER:
     strbuf_print(buf, " type=COUNTER");
     break;
+  case METRIC_TYPE_FPCOUNTER:
+    strbuf_print(buf, " type=FPCOUNTER");
+    break;
   case METRIC_TYPE_GAUGE:
     strbuf_print(buf, " type=GAUGE");
     break;
-  default:
-    return EINVAL;
+  case METRIC_TYPE_UNTYPED:
+    /* no op */
+    break;
   }
 
   if (m->time != 0) {

src/utils/common/common.c

@@ -957,6 +957,8 @@ int format_values(strbuf_t *buf, metric_t const *m, bool store_rates) {
     }
   } else if (m->family->type == METRIC_TYPE_COUNTER) {
     strbuf_printf(buf, ":%" PRIu64, m->value.counter);
+  } else if (m->family->type == METRIC_TYPE_FPCOUNTER) {
+    strbuf_printf(buf, ":" GAUGE_FORMAT, m->value.fpcounter);
   } else if (m->family->type == DS_TYPE_DERIVE) {
     strbuf_printf(buf, ":%" PRIi64, m->value.derive);
   } else {

src/utils/format_graphite/format_graphite.c

@@ -35,22 +35,32 @@
 /* Utils functions to format data sets in graphite format.
  * Largely taken from write_graphite.c as it remains the same formatting */
 
+static int format_double(strbuf_t *buf, double d) {
+  if (isnan(d)) {
+    return strbuf_print(buf, "nan");
+  }
+  return strbuf_printf(buf, GAUGE_FORMAT, d);
+}
+
 static int gr_format_values(strbuf_t *buf, metric_t const *m, gauge_t rate,
                             bool store_rate) {
-  if (!store_rate && ((m->family->type == METRIC_TYPE_GAUGE) ||
-                      (m->family->type == METRIC_TYPE_UNTYPED))) {
+  if (m->family->type == METRIC_TYPE_GAUGE) {
     rate = m->value.gauge;
-    store_rate = true;
   }
 
   if (store_rate) {
-    if (isnan(rate)) {
-      return strbuf_print(buf, "nan");
-    } else {
-      return strbuf_printf(buf, GAUGE_FORMAT, m->value.gauge);
-    }
-  } else if (m->family->type == METRIC_TYPE_COUNTER) {
-    return strbuf_printf(buf, "%" PRIu64, (uint64_t)m->value.counter);
+    return format_double(buf, rate);
+  }
+
+  switch (m->family->type) {
+  case METRIC_TYPE_COUNTER:
+    return strbuf_printf(buf, "%" PRIu64, m->value.counter);
+  case METRIC_TYPE_FPCOUNTER:
+    return format_double(buf, m->value.fpcounter);
+  case METRIC_TYPE_GAUGE:
+    return format_double(buf, m->value.gauge);
+  case METRIC_TYPE_UNTYPED:
+    break;
   }
 
   P_ERROR("gr_format_values: Unknown data source type: %d", m->family->type);