<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN">
<html><body>
<p>Hi all,</p>
<p>Here the write_mysql output plugin patch (from 5.0.2) with modifications suggested by Octo.</p>
<p>By the way, notification are handled, all memory used is freed on shutdown (thx Valgrind).</p>
<p>Regards,</p>
<p>Cyril</p>
<p> </p>
<p>Le 04.01.2012 10:49, Cyril Feraudet a écrit :</p>
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%"><!-- html ignored --><!-- head ignored --><!-- meta ignored -->
<pre>Hi Octo, Hi all,
Thanks for your time. It will permit to me to improve this plugin and
my
knowledge of C.
Le 22.12.2011 01:06, Florian Forster a écrit :</pre>
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">Hi Cyril, thank you very much for your patches! I'm very sorry, but I can't give the plugin the thorough review it deserves right now. But maybe you find the following comments helpful. On Tue, Dec 20, 2011 at 03:03:43PM +0200, Cyril Feraudet wrote:
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">- with_mysql_libs=`$with_mysql_config --libs 2>/dev/null` + with_mysql_libs=`$with_mysql_config --libs_r 2>/dev/null`</blockquote>
This looks like a bug fix. Would it make sense to add this change to the 4.10 branch? If so, would you be willing to send a separate patch for this?</blockquote>
<pre>You are right, I will submit a patch soon. write_mysql is often called
than
mysql plugin and non thread safe library make collectd crashing
quickly.</pre>
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">+CREATE TABLE `data` ( + `id` bigint(20) NOT NULL auto_increment, + `timestamp` double NOT NULL, + `host_id` int(11) NOT NULL, + `plugin_id` int(11) NOT NULL, + `plugin_instance` varchar(255) default NULL, + `type_id` int(11) NOT NULL, + `typeinstance` varchar(255) default NULL, + `dataset_id` int(11) NOT NULL, + `value` double NOT NULL,</blockquote>
One of the reasons why I didn't write a "write_dbi" or "write_mysql" plugin before is because I was uncertain about the schema to use. Why did you chose to have a separate table for host, plugin, plugin_instance, type, and type_instance? I currently think that just having an "identifier" table with five columns would be a better trade- off between normalization and complexity.</blockquote>
<pre>The idea was to save space in `data` table and improve querying in it.
For now, I've 800MB `data` table (MEMORY type) just for the last value
of
each metrics (4207 servers / 462,395 metrics) using REPLACE instead of
INSERT
statement. I think that SQL join are faster on numeric id than varchar.
But all of this things are questionable and dependent of the final use.
I think about a customizable SQL statement in plugin configuration.</pre>
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">ATE TABLE `dataset` (</blockquote>
Why do you store datasets in the database (in addition to the types.db file)? Having the definition in more than one place is bound to create inconsistencies at some point</blockquote>
<pre>This table is for readonly purpose and to make SQL query independent of
number
of metrics in a type. It also contain type added on the way by a
plugin.</pre>
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">diff --git a/src/write_mysql.c b/src/w</blockquote>
/blockquote> typedef struct host_s host_t; +struct host_s { + char
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">name[DATA_MAX_NAME_LEN]; + int id; + host_t *next_host; +};</blockquote>
You will be doing a lot of lookups in this cache(es). I think using a binary search tree (an implementation if provided in src/utils_avltree.h) would improve performance here.</blockquote>
<pre>Thanks for the tips, Il will do that.</pre>
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">int port =
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">tatic int write_mysql_config (const char *key, const char *value) { [...] + } else if (strcasecmp ("Port", key) == 0) { + port = value; You</blockquote>
a char* to an int. This won't produce what you expect. Use the function service_name_to_port_number() from src/common.h. +static int wri</blockquote>
<pre>Your'e right I will fix it.</pre>
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">t (void) { + conn = mysql_init(NULL); + if (mysql_real_connect(conn, host, user, passwd, database, port, NULL, 0) == NULL) { The conn</blockquote>
be (re-)established in the write() callback, when it is needed. Otherwise you will never recover from connection failures. + if (!mysql_th</blockquote>
<pre>Your'e right, I just issued it. I will fix it too.</pre>
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">{ + ERROR("write_mysql plugin: mysqlclient Thread Safe OFF"); I wasn't</blockquote>
s function. Good to know, thanks! +static int add_host_id (
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">me) { [...] + len = ssnprintf (query, sizeof (query), "SELECT id FROM host WHERE name = '%s'", hostname); I think we should</blockquote>
statement with bound arguments (or whatever the terminology). This way (a) MySQL doesn't have to parse and optimize the query each time we need to insert a host and (b) quoting is done for us by the mysql library. +static int add_host_id (char *</blockquote>
<pre>I will looking for. I never used it before.</pre>
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">[...] + pthread_mutex_lock(&mutex); [...] + if (row = mysql_fetch_row(result)) { + id = atoi(row[0]); + mysql_free_result(result); + pthread_mutex_unlock(&mutex); [...] + pthread_mutex_unlock(&mutex); + return id; +} Mutex is released twice</blockquote>
+static int add_plugin_id (char</blockquote>
<pre>I'll fix that.</pre>
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">{ +static int add_type_id (char *typename) { A *lot* of duplicate code</blockquote>
ite a function like: int add_string (const char *table, const char *str); /* returns id on success, less than zero on failure. */ +static int write_mysql_write(con
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">t *ds, const value_list_t *vl, + user_data_t __attribute__((unused)) *user_data) { [...] + len = ssnprintf (tmpquery, sizeof (tmpquery), "INSERT INTO data " +</blockquote>
</blockquote>
<pre>"(timestamp,host_id,plugin_id,plugin_instance,type_id,typeinstance,dataset_id,value)"</pre>
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">+ "VALUES (%.3f,%d,%d,'%s',%d,'%s',%d,%%lf)", CDTIME_T_TO_DOUBLE (vl->time), host_id, + plugin_id, vl->plugin_instance, type_id, vl->type_instance, dataset_id );</blockquote>
You really should prepare this statement. Parsing this several thousand times per second will be a major performance problem.
<blockquote type="cite" style="padding-left:5px; border-left:#1010ff 2px solid; margin-left:5px; width:100%">+if (dso->type == DS_TYPE_GAUGE) { + len = ssnprintf (query, sizeof (query), tmpquery, vl->values[i].gauge); +} else { + if (rates == NULL) { + rates = uc_get_rate (ds, vl); + } + if (isnan(rates[i])) { continue; } + len = ssnprintf (query, sizeof (query), tmpquery, rates[i]); +} +//INFO("toto: %d", toto); +pthread_mutex_lock(&mutex); +DEBUG("write_mysql plugin: %s", query); +mysql_real_query(conn, query, len); +pthread_mutex_unlock(&mutex);</blockquote>
So you're inserting each data source in a element? How can a client distinguish between the different data sources? At least, you should add a "ds_index" column or so. Thanks again and best</blockquote>
<pre>There are dataset_id and type_id in `dataset` for that. Here a query
sample to get
all metric in a type in right order :
select data.timestamp AS date, dataset.name AS dataset_name, data.value
AS value
from data, host, plugin, type, dataset
where host.id = data.host_id
and plugin.id = data.plugin_id
and type.id = data.type_id
and data.dataset_id = dataset.id
and host.name = 'eqds3pcold001'
and plugin.name = 'interface'
and data.plugin_instance = 'eth0'
and type.name = 'if_octets'
In my case, write_mysql is used to make, for example, sum of cpu used
on several thousand
of server and re-inject it in collectd to graph it.
I've made a more flexible than Ganglia application based on Collectd at
work. My compagny
allow me to share all my work on collectd in GPL like licence. I will
Release a beta of
my unamed application soon a possible.
Regards,
Cyril Feraudet
_______________________________________________
collectd mailing list
<a href="mailto:collectd@verplant.org">collectd@verplant.org</a>
<a href="http://mailman.verplant.org/listinfo/collectd">http://mailman.verplant.org/listinfo/collectd</a>
</pre>
</blockquote>
<p> </p>
<div> </div>
</body></html>