From d8e1728e5198087a77ecae83fcd8d63f6d4aad54 Mon Sep 17 00:00:00 2001 From: Florian Forster Date: Sat, 10 Nov 2012 21:36:17 +0100 Subject: [PATCH] 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. --- src/aggregation.c | 80 ++++++++++++++++++++++++++++++++++++++++--- src/collectd.conf.in | 13 ++++--- src/collectd.conf.pod | 59 ++++++++++++++++++++----------- 3 files changed, 122 insertions(+), 30 deletions(-) diff --git a/src/aggregation.c b/src/aggregation.c index 49224345..985fa7c3 100644 --- a/src/aggregation.c +++ b/src/aggregation.c @@ -376,11 +376,11 @@ static void agg_lookup_free_obj_callback (void *user_obj) /* {{{ */ /* * * - * 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) /* {{{ */ * * */ +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) /* {{{ */ " 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 102e116d..ebddb2b8 100644 --- a/src/collectd.conf.in +++ b/src/collectd.conf.in @@ -164,11 +164,14 @@ # # -# 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 69928494..284c36c4 100644 --- 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 and -C. The other values, we set to a wildcard. There are two different -wildcard tokens: C and C. C works like a -CBY> clause in SQL, i.e. if host is set to C, a separate -aggregation will be calculated for each host. In the example, we need to group -by I and I (user, system, idle, ...) but we don't group -by I (CPU number). +To select all the affected values for our example, set C and +C. The other values are left unspecified, meaning "all values". The +I, I, I, I and I options +work as if they were specified in the C clause of an C