[patch] Re: [collectd] Adding new DS definitions to existing plugins

[Nic Bellamy]

Florian Forster wrote:
> On Wed, Jan 24, 2007 at 10:58:34AM +1300, Nic Bellamy wrote:
>> I'd like to submit these patches when they're ready, but would be 
>> interested in any advice regarding how best to handle backwards 
>> compatibility issues. I couldn't find anything on the website containing 
>> policies about this kind of thing - can anyone fill me in?
> 
> We (try to) garuantee backward-compatibility for all versions of the
> same _major_ version (i. e. between minor versions) and backward- and
> forward-compatibility for all versions of the same _minor_ version (i.
> e. between patch releases). Compatibility is defined from a user's point
> of view, i. e. the config-file and the RRD-files must still work with a
> newer version. Internal changes, e. g. the plugin interface, are _not_
> included in this definition and may (and have) change between minor
> versions.

Thanks for that - I figured it'd be along those lines, but wanted to 
confirm. Perhaps that should go into a "developers FAQ" somewhere.

>> I've whipped up a "works for me" patch for src/cpu.c that adds irq +
>> softirq DS definitions to ds_defs[], and updates cpu_read/cpu_submit
>> to handle the extra data,
> 
> I'm in the process of writing a new major version, collectd 4. It is no
> problem at all to include that change there, though I considering to
> pull the RRD-file appart and use one RRD-file for each counter type.
> This offers a lot more flexibility over the RRD-file as it is. I
> originally decided against doing that because I thought that now
> breaking backwards compatibility where it can be avoided might be a good
> idea, but if the datasources change anyway I might as well do that.
> 
> If this patch should be accepted into the collectd 3 line then it's
> necessary that the default behavior doesn't change. You could use the
> alternative RRD-file-layout when configured to do so or simply use
> seperate RRD-files and do some trickery at graph-generation-time. I
> think the second option would be preferable for most users.

Ok. I'll have a think about which branch I want to aim for and whip up 
an appropriately designed patch.

>> and am planning to do a similar thing to src/load.c to keep track of
>> IRQ counts and context switches (since it's per-system, not per-cpu
>> counts, src/load.c makes sense to me).
> 
> I'd put that in a seperate plugin. I'm unsure if it's better to put IRQ-
> counts and context switches (and possibly forkrate) into one plugin
> (since it can all be read from `/proc/stat' under Linux) or if it'd be
> preferable to use one plugin for each. The former would be easier to
> implement while the latter might be more overhead but would fit the
> overall design better. Any opinions?

For me, efficiency is key - we're (going to be) running collectd on 
small semi-embedded appliance systems, so I'd rather avoid opening files 
and processing things any more than necessary.

A patch that adds a "system" module that keeps total IRQ counts, context 
switches and forks is attached to this message. As they're all in the 
one /proc/stat file (under Linux, anyway), I've kept them in the same 
module.

It's Linux-only at present (with an ugly #error otherwise, which should 
really be taken care of by autoconf/automake instead), as I don't have 
any other *nix systems around to test with.

Cheers,
	Nic.

-- 
Nic Bellamy,
Head Of Engineering, Vadacom Ltd - http://www.vadacom.co.nz/
-------------- next part --------------
Index: configure.in
===================================================================
--- configure.in	(revision 1143)
+++ configure.in	(revision 1144)
@@ -1001,6 +1001,7 @@
 AC_COLLECTD([sensors],   [disable], [module], [lm_sensors statistics])
 AC_COLLECTD([serial],    [disable], [module], [serial statistics])
 AC_COLLECTD([swap],      [disable], [module], [swap statistics])
+AC_COLLECTD([system],    [disable], [module], [IRQ count/context switches/fork statistics])
 AC_COLLECTD([tape],      [disable], [module], [tape statistics])
 AC_COLLECTD([traffic],   [disable], [module], [system traffic statistics])
 AC_COLLECTD([users],     [disable], [module], [user count statistics])
@@ -1054,6 +1055,7 @@
     sensors . . . . . . $enable_sensors
     serial  . . . . . . $enable_serial
     swap  . . . . . . . $enable_swap
+    system  . . . . . . $enable_system
     tape  . . . . . . . $enable_tape
     traffic . . . . . . $enable_traffic
     users . . . . . . . $enable_users
Index: src/system.c
===================================================================
--- src/system.c	(revision 0)
+++ src/system.c	(revision 1144)
@@ -0,0 +1,125 @@
+/**
+ * collectd - src/system.c
+ * Copyright (C) 2005,2006  Florian octo Forster
+ * Copyright (C) 2007 Vadacom Limited (NZ)
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
+ *
+ * Authors:
+ *   Florian octo Forster <octo at verplant.org> (src/cpu.c used as template)
+ *   Nic Bellamy <nic dot bellamy at vadacom dot co dot nz>
+ **/
+
+#include <stdio.h>
+
+#include "collectd.h"
+#include "common.h"
+#include "plugin.h"
+#include "utils_debug.h"
+
+#ifndef KERNEL_LINUX
+# error "Sorry, this plugin is Linux-only at this point. Patches welcomed!"
+#endif
+
+#define MODULE_NAME "system"
+
+static char *system_filename = "system.rrd";
+
+static char *ds_def[] =
+{
+	"DS:intr:COUNTER:"COLLECTD_HEARTBEAT":0:U",
+	"DS:ctxt:COUNTER:"COLLECTD_HEARTBEAT":0:U",
+	"DS:forks:COUNTER:"COLLECTD_HEARTBEAT":0:U",
+	NULL
+};
+static int ds_num = 3;
+
+static void system_write (char *host, char *inst, char *val)
+{
+	rrd_update_file (host, system_filename, val, ds_def, ds_num);
+}
+
+static void system_submit (
+		unsigned long long interrupts,
+		unsigned long long ctxswitches,
+		unsigned long long forks)
+{
+	char buf[BUFSIZ];
+
+	if (snprintf (buf, sizeof(buf), "%u:%llu:%llu:%llu", (unsigned int) curtime,
+				interrupts, ctxswitches, forks) >= sizeof(buf))
+		return;
+
+	plugin_submit (MODULE_NAME, NULL, buf);
+}
+
+static void system_read (void)
+{
+	FILE *fh;
+	char buf[BUFSIZ];
+	char *fields[3];
+	int numfields;
+	static complain_t complain_obj;
+	unsigned long long intr = 0, ctxt = 0, forks = 0;
+
+	if ((fh = fopen ("/proc/stat", "r")) == NULL)
+	{
+		plugin_complain (LOG_ERR, &complain_obj, "%s plugin: "
+				"fopen (/proc/stat) failed: %s",
+				MODULE_NAME, strerror (errno));
+		return;
+	}
+
+	plugin_relief (LOG_NOTICE, &complain_obj, "%s plugin: "
+			"fopen (/proc/stat) succeeded.",
+			MODULE_NAME);
+
+	while (fgets (buf, sizeof(buf), fh) != NULL)
+	{
+		if (strncmp (buf, "intr ", 5) == 0) {
+			numfields = strsplit (buf, fields, 3);
+			if (numfields >= 2) {
+				intr = strtoull(fields[1], NULL, 10);
+			}
+			continue;
+		}
+		if (strncmp (buf, "ctxt ", 5) == 0) {
+			numfields = strsplit (buf, fields, 2);
+			if (numfields >= 2) {
+				ctxt = strtoull(fields[1], NULL, 10);
+			}
+			continue;
+		}
+		if (strncmp (buf, "processes ", 10) == 0) {
+			numfields = strsplit (buf, fields, 2);
+			if (numfields >= 2) {
+				forks = strtoull(fields[1], NULL, 10);
+			}
+			continue;
+		}
+	}
+	system_submit (intr, ctxt, forks);
+
+	fclose (fh);
+
+	return;
+}
+
+void module_register (void)
+{
+	plugin_register (MODULE_NAME, NULL, system_read, system_write);
+}
+
+#undef MODULE_NAME
Index: src/collectd.conf.in
===================================================================
--- src/collectd.conf.in	(revision 1143)
+++ src/collectd.conf.in	(revision 1144)
@@ -42,6 +42,7 @@
 @BUILD_MODULE_SENSORS_TRUE.trailing-username(a)leading-domain.LoadPlugin sensors
 @BUILD_MODULE_SERIAL_TRUE.trailing-username(a)leading-domain.LoadPlugin serial
 @BUILD_MODULE_SWAP_TRUE.trailing-username(a)leading-domain.LoadPlugin swap
+.trailing-username(a)leading-domain.BUILD_MODULE_SYSTEM_TRUE@LoadPlugin system
 @BUILD_MODULE_TAPE_TRUE.trailing-username(a)leading-domain.LoadPlugin tape
 @BUILD_MODULE_TRAFFIC_TRUE.trailing-username(a)leading-domain.LoadPlugin traffic
 @BUILD_MODULE_USERS_TRUE.trailing-username(a)leading-domain.LoadPlugin users
Index: src/collectd.1
===================================================================
--- src/collectd.1	(revision 1143)
+++ src/collectd.1	(revision 1144)
@@ -179,6 +179,8 @@
 .IP "\(bu" 4
 Swap usage (\fIswap\fR)
 .IP "\(bu" 4
+System IRQ/context switch/fork counts (\fIsystem\fR)
+.IP "\(bu" 4
 Tape drive usage (\fItape\fR, Solaris only)
 .IP "\(bu" 4
 Network traffic (\fItraffic\fR)
@@ -610,6 +612,13 @@
 \&  DS:cached:GAUGE:HEARTBEAT:0:1099511627776
 \&  DS:resv:GAUGE:HEARTBEAT:0:1099511627776
 .Ve
+.IP "System IRQ/context switch/fork counts (\fIsystem.rrd\fR)" 4
+.IX Item "System IRQ/context switch/fork counts (system.rrd)"
+.Vb 3
+\&  DS:intr:COUNTER:HEARTBEAT:0:U
+\&  DS:ctxt:COUNTER:HEARTBEAT:0:U
+\&  DS:forks:COUNTER:HEARTBEAT:0:U
+.Ve
 .IP "Tape drive usage (\fItape\-\fI<name>\fI.rrd\fR)" 4
 .IX Item "Tape drive usage (tape-<name>.rrd)"
 .Vb 8
Index: src/Makefile.am
===================================================================
--- src/Makefile.am	(revision 1143)
+++ src/Makefile.am	(revision 1144)
@@ -358,6 +358,14 @@
 endif
 endif
 
+if BUILD_MODULE_SYSTEM
+pkglib_LTLIBRARIES += system.la
+system_la_SOURCES = system.c
+system_la_LDFLAGS = -module -avoid-version
+collectd_LDADD += "-dlopen" system.la
+collectd_DEPENDENCIES += system.la
+endif
+
 if BUILD_MODULE_TAPE
 pkglib_LTLIBRARIES += tape.la
 tape_la_SOURCES = tape.c