[collectd] patch for email stats from postfix and amavisd-new, dns stats from powerdns
Sebastian Harl
sh at tokkee.org
Wed Nov 21 00:48:39 CET 2007
Hi Luke,
On Sun, Nov 18, 2007 at 11:23:42PM -0800, Luke Heberling wrote:
> Hello collectd. Thanks for the use of your code. Hope you can make
> use of this patch.
Thanks for your patch. Here are a few notes from a first quick glance. I'm not
going to comment the logfile parsing thing right now, as this is a different
issue...
I hope, I don't sound too picky ;-)
> I'd be happy to make any modifications you deem appropriate for
> inclusion in the distribution.
First of all, please send one patch for each changeset (i.e. one patch per
plugin in this case) - this makes reviews a lot easier.
A couple of general coding issues:
* Try to modularize things. As you said, the log parsing is basically the
same for amavis and postfix, so it should be extracted into e.g. a
utils_logparse or utils_logtail module. You could have two plugins then,
both using this module. The same probably applies for the MRU cache.
As a general rule, some utils_<foo> module should be created whenever two
or more plugins partly share the same functionality.
If you create any such module please don't forget to document its API in
the header (see utils_avltree.h for a pretty good example).
* Please try to roughly stick to the coding style used in most parts of
collectd. The following issues currently come to my mind:
- use "function_name (arg1, arg2)" instead of "function_name( arg1, ... )"
- operators like '+', '-', '*', '=', '>=', etc. should be padded with a
single space on both sides (e.g. "if (a == b)", "int a = b", etc.)
- try to limit lines to no more than 80 characters (counting tabs as eight
spaces puts you on the safe side while four spaces should be fine as
well)
Usually, none of this would prevent a patch from being accepted, but it
makes us a lot happier, if we don't have to adopt such things ourself ;-)
Anyway, it's quite important to stick to one single coding style within the
same module. I noticed that you mixed some different styles in your patch
(e.g. "foo==bar" vs. "foo == bar", "function( foo )" vs. "function(foo)") -
this makes the code pretty hard to read.
> + - amafix
> + Amavis/Postfix: messages sent/received, spam/virii blocked, bytes
> + transferred, messages rejected, etc.
As noted above: this should be split into two plugins.
> + - pdns
> + PowerDNS authoritative and recursive server statistics.
I'd personally prefer "powerdns" as the plugin name, but this definitely is no
show-stopper :-)
> --- collectd-pristine/debian/collectd.conf 2007-11-17 04:44:33.365223549 +0000
> +++ collectd/debian/collectd.conf 2007-11-18 02:34:08.677952469 +0000
This file is not part of the source distribution ;-)
> +#<Plugin amafix
... anyway, you're missing a closing '>' here ;-)
> --- collectd-pristine/src/amafix.c 1970-01-01 00:00:00.000000000 +0000
> +++ collectd/src/amafix.c 2007-11-18 02:34:08.681952461 +0000
> + * EXAMPLE
[...]
Is there a specific reason for including this in the source code as well? Imho
is perfectly fine to include it in the manpage only - this would save you/us
from keeping documentation up to date in two different places (which is kind
of error prone).
> +#define DS_PF_RECEIVED_LOG "pf_received_loc"
[...]
> +#define CONFIG_FILE_NAME "FileName"
[...]
Most (all?) of these defines are used only once in the code. Please include
the strings directly instead of using defines which don't add any more
information (the name is basically identical to the string value).
> +extern char *strndup( const char *s, size_t len );
> +extern size_t strnlen( const char *s, size_t len );
Please don't add prototypes of library functions to your source code. Simply
include the appropriate headers. If functions are only available if certain
defines have been set, rethink if you really want to use those functions.
We're not very happy about any architecture / distribution / whatever specific
extensions (especially GNU does some really strange things some times...) as
this limits portability a lot. And yes, collectd currently has been reported
to run on quite a few non-GNU platforms. If you really cannot live without any
such extensions add appropriate checks to the configure script (or ask us to
do so).
> +static const char *config_keys[] = {
> + CONFIG_FILE_NAME,
> + CONFIG_PF_MRU_SIZE,
> + CONFIG_AV_MRU_SIZE,
> + NULL,
> +};
> +static int config_keys_len = 3;
You can use the STATIC_ARRAY_SIZE() macro (defined in collectd.h) to get the
size of static arrays. This might prevent some errors in the future.
> +/* constant value inlined in module_register for
> + * benefit of debian maintainer script
> + */
> +#define PLUGIN_NAME "amafix"
Just out of curiosity: what's the benefit you're talking about? ;-)
> + for( i=1;i<sizeof(primes)/sizeof(primes[0]);++i,lastdiff=diff) {
> + diff=target-primes[i];
This is one example of what I was talking about earlier which could definitely
use some more spaces to greatly improve readability...
> + switch( *--ptr2 ) {
> + ++*counter;
Please add one more pair of brackets to expressions like that (like e.g.
"*(--ptr2)"). In this case the operator precedence should be fairly obvious
but in general it can get very confusing to figure out what's going on.
> + if( fd != NULL )
> + while( fgets( buffer, buffer_len, fd ) ) {
> + if( feof( fd ) ) {
> + len = strnlen( buffer, buffer_len );
> + if( len == 0 )
> + break;
> +
> + if( *(buffer+len-1) != '\n' ) {
> + DEBUG( "Partial Line: %s\n", buffer );
> + fseek( fd, -len, SEEK_CUR );
> + break;
> + }
> + }
> + process_line( buffer );
> + }
In general we leave out braces if a block only consists of a single
instruction. However in cases like this (if the single instruction is kind of
lengthy) it might make sense to add that additional pair of braces to improve
readability (at a first glance I was confused like hell in this case -
especially as the correct indentation somehow got lost).
> --- collectd-pristine/src/collectd.conf.in 2007-11-17 04:44:33.369223540 +0000
> +++ collectd/src/collectd.conf.in 2007-11-18 02:34:08.681952461 +0000
[...]
> +#<Plugin amafix
... the missing '>' again - in this case the file will be included in the
source distribution ;-)
> --- collectd-pristine/src/collectd.conf.pod 2007-11-17 04:44:33.369223540 +0000
> +++ collectd/src/collectd.conf.pod 2007-11-18 02:34:08.681952461 +0000
> +=item B<FileName> I<FileName>
> +
> +The amafix plugin tails a logfile gathering mail statistics. This option sets
> +the filename that will be tailed. This option is required.
Please try to set a default for each configuration option. "/var/log/mail.log"
sounds like a very reasonable default in this case...
> --- collectd-pristine/src/types.db 2007-11-17 04:44:33.421223427 +0000
> +++ collectd/src/types.db 2007-11-18 02:34:08.681952461 +0000
> +pf_received_loc value:COUNTER:0:U
> +pf_delivered_loc value:COUNTER:0:U
> +pf_received_net value:COUNTER:0:U
[...]
> +av_passed_clean value:COUNTER:0:U
> +av_passed_spammy value:COUNTER:0:U
> +av_passed_spam value:COUNTER:0:U
[...]
This does not fit into collectd's data definition concept. types.db only
contains the definition of different data types and somewhat descriptive but
general names. A closer specification of the collected data happens through
the use of the (plugin name, plugin instance, type instance) tuple. In your
case you somewhat mixed the plugin name (e.g. "av") with a type instance (e.g.
"spam") to create new data types. You should probably introduce new data types
like "email_counter" ("email_count" is available already, but unfortunately
it's defined as a GAUGE value :-/), "email_bytes" and the like and pass any
other information using appropriate type instances (and possibly plugin
instances).
Florian, maybe we should document how this tuple is supposed to be used
somewhere - especially, as the interface now is available to the user through
the perl, exec, etc. plugins.
I hope I didn't scare you off but could give you some useful hints :-)
Cheers,
Sebastian
--
Sebastian "tokkee" Harl +++ GnuPG-ID: 0x8501C7FC +++ http://tokkee.org/
Those who would give up Essential Liberty to purchase a little Temporary
Safety, deserve neither Liberty nor Safety. -- Benjamin Franklin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://mailman.verplant.org/pipermail/collectd/attachments/20071121/2e57e28f/attachment.pgp
More information about the collectd
mailing list