aggregation plugin: Implement the "GroupBy" option.

This new configuration format has two benefits:
1) It is easier to understand by users, because they don't have to know
   about two types of wildcards.
2) In the future matching of Host, Plugin, Type, ... may support regular
   expressions. In that case, this configuration syntax doesn't need to
   be adapted.

diff --git a/src/aggregation.c b/src/aggregation.c
index 4922434..985fa7c 100644 (file)
--- a/src/aggregation.c
+++ b/src/aggregation.c
@@ -376,11 +376,11 @@ static void agg_lookup_free_obj_callback (void *user_obj) /* {{{ */
 /*
  * <Plugin "aggregation">
  *   <Aggregation>
- *     Host "/any/"
  *     Plugin "cpu"
- *     PluginInstance "/all/"
  *     Type "cpu"
- *     TypeInstance "/any/"
+ *
+ *     GroupBy Host
+ *     GroupBy TypeInstance
  *
  *     CalculateNum true
  *     CalculateSum true
@@ -391,6 +391,43 @@ static void agg_lookup_free_obj_callback (void *user_obj) /* {{{ */
  *   </Aggregation>
  * </Plugin>
  */
+static int agg_config_handle_group_by (oconfig_item_t const *ci, /* {{{ */
+    aggregation_t *agg)
+{
+  int i;
+
+  for (i = 0; i < ci->values_num; i++)
+  {
+    char const *value;
+
+    if (ci->values[i].type != OCONFIG_TYPE_STRING)
+    {
+      ERROR ("aggregation plugin: Argument %i of the \"GroupBy\" option "
+          "is not a string.", i + 1);
+      continue;
+    }
+
+    value = ci->values[i].value.string;
+
+    if (strcasecmp ("Host", value) == 0)
+      sstrncpy (agg->ident.host, LU_ANY, sizeof (agg->ident.host));
+    else if (strcasecmp ("Plugin", value) == 0)
+      sstrncpy (agg->ident.plugin, LU_ANY, sizeof (agg->ident.plugin));
+    else if (strcasecmp ("PluginInstance", value) == 0)
+      sstrncpy (agg->ident.plugin_instance, LU_ANY,
+          sizeof (agg->ident.plugin_instance));
+    else if (strcasecmp ("TypeInstance", value) == 0)
+      sstrncpy (agg->ident.type_instance, LU_ANY, sizeof (agg->ident.type_instance));
+    else if (strcasecmp ("Type", value) == 0)
+      ERROR ("aggregation plugin: Grouping by type is not supported.");
+    else
+      WARNING ("aggregation plugin: The \"%s\" argument to the \"GroupBy\" "
+          "option is invalid and will be ignored.", value);
+  } /* for (ci->values) */
+
+  return (0);
+} /* }}} int agg_config_handle_group_by */
+
 static int agg_config_aggregation (oconfig_item_t *ci) /* {{{ */
 {
   aggregation_t *agg;
@@ -406,6 +443,14 @@ static int agg_config_aggregation (oconfig_item_t *ci) /* {{{ */
   }
   memset (agg, 0, sizeof (*agg));
 
+  sstrncpy (agg->ident.host, LU_ALL, sizeof (agg->ident.host));
+  sstrncpy (agg->ident.plugin, LU_ALL, sizeof (agg->ident.plugin));
+  sstrncpy (agg->ident.plugin_instance, LU_ALL,
+      sizeof (agg->ident.plugin_instance));
+  sstrncpy (agg->ident.type, LU_ALL, sizeof (agg->ident.type));
+  sstrncpy (agg->ident.type_instance, LU_ALL,
+      sizeof (agg->ident.type_instance));
+
   for (i = 0; i < ci->children_num; i++)
   {
     oconfig_item_t *child = ci->children + i;
@@ -425,6 +470,8 @@ static int agg_config_aggregation (oconfig_item_t *ci) /* {{{ */
     else if (strcasecmp ("TypeInstance", child->key) == 0)
       cf_util_get_string_buffer (child, agg->ident.type_instance,
           sizeof (agg->ident.type_instance));
+    else if (strcasecmp ("GroupBy", child->key) == 0)
+      agg_config_handle_group_by (child, agg);
     else if (strcasecmp ("CalculateNum", child->key) == 0)
       cf_util_get_boolean (child, &agg->calc_num);
     else if (strcasecmp ("CalculateSum", child->key) == 0)
@@ -444,7 +491,17 @@ static int agg_config_aggregation (oconfig_item_t *ci) /* {{{ */
 
   /* Sanity checking */
   is_valid = 1;
-  if (strchr (agg->ident.type, '/') != NULL) /* {{{ */
+  if (LU_IS_ALL (agg->ident.type)) /* {{{ */
+  {
+    ERROR ("aggregation plugin: It appears you did not specify the required "
+        "\"Type\" option in this aggregation. "
+        "(Host \"%s\", Plugin \"%s\", PluginInstance \"%s\", "
+        "Type \"%s\", TypeInstance \"%s\")",
+        agg->ident.host, agg->ident.plugin, agg->ident.plugin_instance,
+        agg->ident.type, agg->ident.type_instance);
+    is_valid = 0;
+  }
+  else if (strchr (agg->ident.type, '/') != NULL)
   {
     ERROR ("aggregation plugin: The \"Type\" may not contain the '/' "
         "character. Especially, it may not be a wildcard. The current "
@@ -458,7 +515,9 @@ static int agg_config_aggregation (oconfig_item_t *ci) /* {{{ */
       && !LU_IS_ALL (agg->ident.type_instance))
   {
     ERROR ("aggregation plugin: An aggregation must contain at least one "
-        "\"/all/\" wildcard. Otherwise, nothing will be aggregated. "
+        "wildcard. This is achieved by leaving at least one of the \"Host\", "
+        "\"Plugin\", \"PluginInstance\" and \"TypeInstance\" options blank "
+        "and not grouping by that field. "
         "(Host \"%s\", Plugin \"%s\", PluginInstance \"%s\", "
         "Type \"%s\", TypeInstance \"%s\")",
         agg->ident.host, agg->ident.plugin, agg->ident.plugin_instance,
@@ -492,6 +551,11 @@ static int agg_config_aggregation (oconfig_item_t *ci) /* {{{ */
     return (-1);
   }
 
+  DEBUG ("aggregation plugin: Successfully added aggregation: "
+      "(Host \"%s\", Plugin \"%s\", PluginInstance \"%s\", "
+      "Type \"%s\", TypeInstance \"%s\")",
+      agg->ident.host, agg->ident.plugin, agg->ident.plugin_instance,
+      agg->ident.type, agg->ident.type_instance);
   return (0);
 } /* }}} int agg_config_aggregation */
 
@@ -499,6 +563,8 @@ static int agg_config (oconfig_item_t *ci) /* {{{ */
 {
   int i;
 
+  pthread_mutex_lock (&agg_instance_list_lock);
+
   if (lookup == NULL)
   {
     lookup = lookup_create (agg_lookup_class_callback,
@@ -507,6 +573,7 @@ static int agg_config (oconfig_item_t *ci) /* {{{ */
         agg_lookup_free_obj_callback);
     if (lookup == NULL)
     {
+      pthread_mutex_unlock (&agg_instance_list_lock);
       ERROR ("aggregation plugin: lookup_create failed.");
       return (-1);
     }
@@ -523,6 +590,8 @@ static int agg_config (oconfig_item_t *ci) /* {{{ */
           "<Plugin aggregation /> blocks and will be ignored.", child->key);
   }
 
+  pthread_mutex_unlock (&agg_instance_list_lock);
+
   return (0);
 } /* }}} int agg_config */
 
@@ -548,6 +617,7 @@ static int agg_read (void) /* {{{ */
     else
       success++;
   }
+
   pthread_mutex_unlock (&agg_instance_list_lock);
 
   return ((success > 0) ? 0 : -1);

diff --git a/src/collectd.conf.in b/src/collectd.conf.in
index 102e116..ebddb2b 100644 (file)
--- a/src/collectd.conf.in
+++ b/src/collectd.conf.in
@@ -164,11 +164,14 @@
 
 #<Plugin "aggregation">
 #  <Aggregation>
-#    Host "/any/"
-#    Plugin "example"
-#    PluginInstance "/all/"
-#    Type "gauge"
-#    TypeInstance "/any/"
+#    #Host "unspecified"
+#    Plugin "cpu"
+#    #PluginInstance "unspecified"
+#    Type "cpu"
+#    #TypeInstance "unspecified"
+#
+#    GroupBy "Host"
+#    GroupBy "TypeInstance"
 #
 #    CalculateNum false
 #    CalculateSum false

diff --git a/src/collectd.conf.pod b/src/collectd.conf.pod
index 6992849..284c36c 100644 (file)
--- a/src/collectd.conf.pod
+++ b/src/collectd.conf.pod
@@ -199,25 +199,41 @@ statistics for your entire fleet.
 The grouping is powerful but, as with many powerful tools, may be a bit
 difficult to wrap your head around. The grouping will therefore be
 demonstrated using an example: The average and sum of the CPU usage across
-CPUs of each client is to be calculated.
+all CPUs of each host is to be calculated.
 
-To select all the affected values for our example, set C<plugin=cpu> and
-C<type=cpu>. The other values, we set to a wildcard. There are two different
-wildcard tokens: C</all/> and C</any/>. C</any/> works like a
-C<GROUPE<nbsp>BY> clause in SQL, i.e. if host is set to C</any/>, a separate
-aggregation will be calculated for each host. In the example, we need to group
-by I<Host> and I<Type Instance> (user, system, idle, ...) but we don't group
-by I<Plugin Instance> (CPU number).
+To select all the affected values for our example, set C<Plugin cpu> and
+C<Type cpu>. The other values are left unspecified, meaning "all values". The
+I<Host>, I<Plugin>, I<PluginInstance>, I<Type> and I<TypeInstance> options
+work as if they were specified in the C<WHERE> clause of an C<SELECT> SQL
+statement.
+
+  Plugin "cpu"
+  Type "cpu"
+
+Although the I<Host>, I<PluginInstance> (CPU number, i.e. 0, 1, 2, ...)  and
+I<TypeInstance> (idle, user, system, ...) fields are left unspecified in the
+example, the intention is to have a new value for each host / type instance
+pair. This is achieved by "grouping" the values using the C<GroupBy> option.
+It can be specified multiple times to group by more than one field.
+
+  GroupBy "Host"
+  GroupBy "TypeInstance"
+
+We do neither specify nor group by I<plugin instance> (the CPU number), so all
+metrics that differ in the CPU number only will be aggregated. Each
+aggregation needs I<at least one> such field, otherwise no aggregation would
+take place.
 
 The full example configuration looks like this:
 
  <Plugin "aggregation">
    <Aggregation>
-     Host "/any/"
      Plugin "cpu"
-     PluginInstance "/all/"
      Type "cpu"
-     TypeInstance "/any/"
+     
+     GroupBy "Host"
+     GroupBy "TypeInstance"
+     
      CalculateSum true
      CalculateAverage true
    </Aggregation>
@@ -229,14 +245,14 @@ There are a couple of limitations you should be aware of:
 
 =item
 
-The I<Type> cannot be a wildcard, because it is not reasonable to add apples
-to oranges. Also, the internal lookup structure won't work if you set it to
-C</any/> or C</all/>.
+The I<Type> cannot be left unspecified, because it is not reasonable to add
+apples to oranges. Also, the internal lookup structure won't work if you try
+to group by type.
 
 =item
 
-There must be at least one C</all/> wildcard, otherwise nothing will be
-aggregated.
+There must be at least one unspecified, ungrouped field. Otherwise nothing
+will be aggregated.
 
 =back
 
@@ -257,10 +273,13 @@ aggregations. The following options are valid inside B<Aggregation> blocks:
 
 =item B<TypeInstance> I<TypeInstance>
 
-Selects the value lists to be added to this aggregation. Each field, with the
-exception of B<Type>, can hold either a fixed string, or one of the wildcard
-tokens C</all/> and C</any/>. B<Type> must be a valid data set name, see
-L<types.db(5)> for details.
+Selects the value lists to be added to this aggregation. B<Type> must be a
+valid data set name, see L<types.db(5)> for details.
+
+=item B<GroupBy> B<Host>|B<Plugin>|B<PluginInstance>|B<TypeInstance>
+
+Group valued by the specified field. The B<GroupBy> option may be repeated to
+group by multiple fields.
 
 =item B<CalculateNum> B<true>|B<false>