[collectd] [PATCH] mysql plugin: add support for multiple databases

[Sebastian Harl]

Hi Doug,

On Mon, Mar 16, 2009 at 06:29:23PM -0400, Doug MacEachern wrote:
> Similar to the recent apache plugin thread, this patch adds config
> support to monitor multiple databases from the same collectd instance.
> Most of the logic was borrowed from the dbi plugin.

Thanks for the patch - this was long overdue ;-)

> To support legacy mode, the plugin_instance is only set if there are > 1
> database configured.. should probably have an option to always set the
> plugin_instance?

Hrm ... I'd simply add a legacy-flag to the mysql_database_t object and
set that accordingly when parsing the config. Having that depend on the
number of configured databases is unintuitive and a config option is not
very nice imho.

In fact, that "flag" could be a char-pointer, pointing to the actual
plugin_instance - then you'd only have to check for that being NULL when
dispatching values.

> mysql_config () supports a legacy mode for the original style config -
> not sure this is the best way to do it?

Imho, that's the way to go in this case. I'd also add a legacy-flag to
mysql_config_add_database() to handle old style config (plugin_instance,
hostname [see below], etc.).

> Still uses hostname_g - might want an option to change this?

I'd also let that depend on legacy-mode. New style config should go for
using the specified hostname of the database. Please use the same
semantics as in the PostgreSQL plugin (and, I hope, the dbi plugin): if
the hostname is NULL, the empty string or "localhost, then use
hostname_g, else the specified hostname.

> Let me know what you think and I can follow up with .conf/.pod/etc
> updates.

See a few minor comments about the patch below ...

> +static int mysql_config (oconfig_item_t *ci) /* {{{ */
[...]
> +		if (strcasecmp ("Database", child->key) == 0 && child->children_num > 0)

Please use additional brackets in such cases, i.e.:

  if ((<cond1>) && (<cond2>))

> +			/* legacy mode - convert to <Database ...> config */
> +			if (lci == NULL)
> +			{
> +				lci = malloc (sizeof(*lci));

Just a really minor comment: You could use an automatic variable for
that as well, i.e. allocate the memory on the stack.

> +static MYSQL *getconnection (mysql_database_t *db)
>  {
>  	static MYSQL *con;
>  	static int    state;

Uhm ... that's not going to work. The connection and state information
have to be stored in the mysql_database_t object as well.

Else, your patch looks fine to me. Thanks again!

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/20090317/879d19c9/attachment.pgp