Re: [pcp] logger PMDA review

[Ken McDonell <kenj at internode dot on dot net>]

On Wed, 2011-04-13 at 14:52 -0500, David Smith wrote: 
> After mucking around a bit, I've got a working version of my PCP logger
> PMDA.  You can see the result by looking at:
> 
> <http://sources.redhat.com/git/gitweb.cgi?p=systemtap.git;a=tree;f=pcp/src/pmdas/logger;h=b69c63b346157a2a6f857ac80cbf1eda5f7be824;hb=HEAD>
> 
> Or you can do a git pull (and look at pcp/src/pmdas/logger in the
> resulting tree):
> # git clone git://sources.redhat.com/git/systemtap.git
> 
> When installed, the PMDA creates a config file containing a NAME and
> PATH for logfiles to monitor.  If PATH ends in '|', the filename is
> interpreted as a command that pipes output to the PMDA.
> 
> Each logfile's data appears under logger.perfile.NAME.  Each event
> stream can have multiple clients attached to it.
> 
> I'd appreciate any comments on the code.  Thanks for the help.

David,

Overall this looks like a worthwhile contribution, so looks good.

I've downloaded the source, built the code and taken it for a bit of a
play.  Nothing too deep, but here is my feedback

1. building and installing the DSO variant of the PMDA is not going to
work here, as you need configfile from the command line ... so just make
it a daemon-only PMDA

2. usage text does not quite match code

3. diagnostics are a bit too verbose ... can use the existing mechanisms
to include diagsnostics, disable them by default and allow -Dappl0 or
-Dappl1 or -Dappl2 or any combination thereof on the command line to
enable diagnostics (or more exotically allow 'em to be turned on and off
via pmstore and a storable metric)

4. I'm not sure how the "catchup" logic was intended to work with an
existing file ... I used /var/log/messages, and the agent delivered 4096
byte chunks from the start of the file which did not honour any sort of
newline boundaries, and does not chop the input into one event per line
(which may be by design, I'm not sure) ... similarly in non-catchup
mode, multiple new lines in the logfile are returned as a single
record ... perhaps there needs to be some concept of a record delimiter
(newline by default) for the input logfiles?

5. I don't think the multi-client logic is correct, at least in the
"catchup" case, where repeated pminfo -x invocations deliver progressive
4096 byte chunks (even though these are different clients) and two
concurrent pmevent processes do not appear to return the same data (as I
would have expected) and one of them sometimes sees "No events" event
when there is lots of catchup still to be done.

6. Install should use pmda_interface=5 ... I've fixed the bug in
pmdaproc.sh

7. My PMNS after install is ...
        logger.numclients
        logger.numlogfiles
        logger.param_string
        logger.perfile.syslog.records
        logger.perfile.syslog.numclients

and I think this might be a little more user friendly ...

        logger.config.numclients
        logger.config.numlogfiles
        logger.config.param_string
        logger.syslog.records
        logger.config.syslog.numclients
        
although this does mean guarding against the PMNS name of "config" in
the configfile ... perhaps this is not worth changing

I've attached a suggested patch for issues 1., 2., 3. and 6.

Hope this helps.

Cheers, Ken.

diff --git a/pcp/src/pmdas/logger/Install b/pcp/src/pmdas/logger/Install
index 833e8bf..f774acf 100644
--- a/pcp/src/pmdas/logger/Install
+++ b/pcp/src/pmdas/logger/Install
@@ -31,9 +31,9 @@
 #
 iam=logger
 
-# Using PMDA_INTERFACE_5.  But, pmdaproc.sh only handles 1-4.
+# Using PMDA_INTERFACE_5
 #
-pmda_interface=4
+pmda_interface=5
 
 # Do it
 #
diff --git a/pcp/src/pmdas/logger/event.c b/pcp/src/pmdas/logger/event.c
index c223bf2..281a33d 100644
--- a/pcp/src/pmdas/logger/event.c
+++ b/pcp/src/pmdas/logger/event.c
@@ -200,7 +200,10 @@ event_create(int logfile)
     e->clients = file_data_tab[logfile].numclients;
     e->buffer[c] = '\0';
     TAILQ_INSERT_TAIL(&file_data_tab[logfile].head, e, events);
-    __pmNotifyErr(LOG_INFO, "Inserted item, clients = %d.", e->clients);
+#ifdef PCP_DEBUG
+    if (pmDebug & DBG_TRACE_APPL1)
+	__pmNotifyErr(LOG_INFO, "Inserted item, clients = %d.", e->clients);
+#endif
     return 0;
 }
 
@@ -246,7 +249,10 @@ event_fetch(pmValueBlock **vbpp, unsigned int logfile)
 	/* Add the string parameter.  Note that pmdaEventAddParam()
 	 * copies the string, so we can free it soon after. */
 	atom.cp = e->buffer;
-	__pmNotifyErr(LOG_INFO, "Adding param: %s", e->buffer);
+#ifdef PCP_DEBUG
+	if (pmDebug & DBG_TRACE_APPL1)
+	    __pmNotifyErr(LOG_INFO, "Adding param: %s", e->buffer);
+#endif
 	rc = pmdaEventAddParam(eventarray,
 			       file_data_tab[logfile].logfile->pmid_string,
 			       PM_TYPE_STRING, &atom);
diff --git a/pcp/src/pmdas/logger/logger.c b/pcp/src/pmdas/logger/logger.c
index 2824b31..4bb49c1 100644
--- a/pcp/src/pmdas/logger/logger.c
+++ b/pcp/src/pmdas/logger/logger.c
@@ -17,6 +17,11 @@
  * 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.,
  * 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
+ *
+ * Debug options
+ * APPL0	configfile processing and PMNS setup
+ * APPL1	loading event data from the log files
+ * APPL2	interaction with PMCD
  */
 
 #include <pcp/pmapi.h>
@@ -98,13 +103,15 @@ static pmdaMetric static_metrictab[] = {
 static pmdaMetric *metrictab = NULL;
 
 static char	mypath[MAXPATHLEN];
-static int	isDSO = 1;		/* ==0 if I am a daemon */
 char	       *configfile = NULL;
 
 void
 logger_end_contextCallBack(int ctx)
 {
-    __pmNotifyErr(LOG_INFO, "%s: saw context %d\n", __FUNCTION__, ctx);
+#ifdef PCP_DEBUG
+    if (pmDebug & DBG_TRACE_APPL2)
+	__pmNotifyErr(LOG_INFO, "%s: saw context %d\n", __FUNCTION__, ctx);
+#endif
     ctx_end(ctx);
 }
 
@@ -112,7 +119,10 @@ static int
 logger_profile(__pmProfile *prof, pmdaExt *ep)
 {
 //    (ep->e_context)
-    __pmNotifyErr(LOG_INFO, "%s: saw context %d\n", __FUNCTION__, ep->e_context);
+#ifdef PCP_DEBUG
+    if (pmDebug & DBG_TRACE_APPL2)
+	__pmNotifyErr(LOG_INFO, "%s: saw context %d\n", __FUNCTION__, ep->e_context);
+#endif
     ctx_start(ep->e_context);
     return 0;
 }
@@ -127,7 +137,10 @@ logger_fetchCallBack(pmdaMetric *mdesc, unsigned int inst, pmAtomValue *atom)
     int		rc;
     int		status = PMDA_FETCH_STATIC;
 
-    __pmNotifyErr(LOG_INFO, "%s called\n", __FUNCTION__);
+#ifdef PCP_DEBUG
+    if (pmDebug & DBG_TRACE_APPL2)
+	__pmNotifyErr(LOG_INFO, "%s called\n", __FUNCTION__);
+#endif
     if (idp->cluster != 0 || (idp->item < 0 || idp->item > nummetrics)) {
 	__pmNotifyErr(LOG_ERR, "%s: PM_ERR_PMID (cluster = %d, item = %d)\n",
 		      __FUNCTION__, idp->cluster, idp->item);
@@ -314,8 +327,11 @@ read_config(const char *filename)
 	strncpy(data->pathname, ptr, sizeof(data->pathname));
 	/* data->pmid_string gets filled in after pmdaInit() is called. */
 
-	__pmNotifyErr(LOG_INFO, "%s: saw logfile %s (%s)\n", __FUNCTION__,
+#ifdef PCP_DEBUG
+	if (pmDebug & DBG_TRACE_APPL0)
+	    __pmNotifyErr(LOG_INFO, "%s: saw logfile %s (%s)\n", __FUNCTION__,
 		      data->pathname, data->pmns_name);
+#endif
     }
     if (rc != 0) {
 	free(logfiles);
@@ -330,11 +346,10 @@ read_config(const char *filename)
 static void
 usage(void)
 {
-    fprintf(stderr, "Usage: %s [options]\n\n", pmProgname);
+    fprintf(stderr, "Usage: %s [options] configfile\n\n", pmProgname);
     fputs("Options:\n"
 	  "  -d domain    use domain (numeric) for metrics domain of PMDA\n"
-	  "  -l logfile   write log into logfile rather than using default log name\n"
-	  "  -m logfile   logfile to monitor (required)\n",
+	  "  -l logfile   write log into logfile rather than using default log name\n",
 	      stderr);		
     exit(1);
 }
@@ -342,15 +357,21 @@ usage(void)
 static int
 logger_pmid(const char *name, pmID *pmid, pmdaExt *pmda)
 {
-    __pmNotifyErr(LOG_INFO, "%s: name %s\n", __FUNCTION__,
+#ifdef PCP_DEBUG
+    if (pmDebug & DBG_TRACE_APPL0)
+	__pmNotifyErr(LOG_INFO, "%s: name %s\n", __FUNCTION__,
 		  (name == NULL) ? "NULL" : name);
+#endif
     return pmdaTreePMID(pmns, name, pmid);
 }
 
 static int
 logger_name(pmID pmid, char ***nameset, pmdaExt *pmda)
 {
-    __pmNotifyErr(LOG_INFO, "%s: pmid 0x%x\n", __FUNCTION__, pmid);
+#ifdef PCP_DEBUG
+    if (pmDebug & DBG_TRACE_APPL0)
+	__pmNotifyErr(LOG_INFO, "%s: pmid 0x%x\n", __FUNCTION__, pmid);
+#endif
     return pmdaTreeName(pmns, pmid, nameset);
 }
 
@@ -358,13 +379,16 @@ static int
 logger_children(const char *name, int traverse, char ***kids, int **sts,
 		pmdaExt *pmda)
 {
-    __pmNotifyErr(LOG_INFO, "%s: name %s\n", __FUNCTION__,
+#ifdef PCP_DEBUG
+    if (pmDebug & DBG_TRACE_APPL0)
+	__pmNotifyErr(LOG_INFO, "%s: name %s\n", __FUNCTION__,
+#endif
 		  (name == NULL) ? "NULL" : name);
     return pmdaTreeChildren(pmns, name, traverse, kids, sts);
 }
 
 /*
- * Initialise the agent (both daemon and DSO).
+ * Initialise the agent (daemon only).
  */
 void 
 logger_init(pmdaInterface *dp)
@@ -377,13 +401,6 @@ logger_init(pmdaInterface *dp)
     char name[MAXPATHLEN * 2];
     struct dynamic_metric_info *pinfo;
 
-    if (isDSO) {
-	int sep = __pmPathSeparator();
-	snprintf(mypath, sizeof(mypath), "%s%c" "logger" "%c" "help",
-		pmGetConfig("PCP_PMDAS_DIR"), sep, sep);
-	pmdaDSO(dp, PMDA_INTERFACE_5, "logger DSO", mypath);
-    }
-
     /* Read and parse config file. */
     if (read_config(configfile) != 0) {
 	exit(1);
@@ -488,7 +505,6 @@ main(int argc, char **argv)
     int			sep = __pmPathSeparator();
     pmdaInterface	desc;
 
-    isDSO = 0;
     __pmSetProgname(argv[0]);
 
     snprintf(mypath, sizeof(mypath), "%s%c" "logger" "%c" "help",
@@ -513,14 +529,6 @@ main(int argc, char **argv)
     logger_init(&desc);
     pmdaConnect(&desc);
 
-#ifdef HAVE_SIGHUP
-    /*
-     * Non-DSO agents should ignore gratuitous SIGHUPs, e.g. from xwsh
-     * when launched by the PCP Tutorial!
-     */
-    signal(SIGHUP, SIG_IGN);
-#endif
-
     pmdaMain(&desc);
 
     event_shutdown();
diff --git a/pcp/src/pmdas/logger/percontext.c b/pcp/src/pmdas/logger/percontext.c
index 98a1a8f..ee52d3f 100644
--- a/pcp/src/pmdas/logger/percontext.c
+++ b/pcp/src/pmdas/logger/percontext.c
@@ -70,8 +70,11 @@ ctx_start(int ctx)
 	if (ctx_start_cb) {
 	    ctxtab[ctx].user_data = ctx_start_cb(ctx);
 	}
-	__pmNotifyErr(LOG_INFO, "%s: saw new context %d (num_ctx=%d)\n",
+#ifdef PCP_DEBUG
+	if (pmDebug & DBG_TRACE_APPL2)
+	    __pmNotifyErr(LOG_INFO, "%s: saw new context %d (num_ctx=%d)\n",
 		      __FUNCTION__, ctx, num_ctx);
+#endif
     }
     return 0;
 }
@@ -80,7 +83,7 @@ void
 ctx_end(int ctx)
 {
 #ifdef PCP_DEBUG
-    if (pmDebug & DBG_TRACE_APPL1) {
+    if (pmDebug & DBG_TRACE_APPL2) {
 	fprintf(stderr, "sample_ctx_end(%d) [context is ", ctx);
 	if (ctx < 0 || ctx >= num_ctx_allocated)
 	    fprintf(stderr, "unknown, num_ctx=%d", num_ctx);
diff --git a/pcp/src/pmdas/logger/root b/pcp/src/pmdas/logger/root
index 51218c4..d423d7e 100644
--- a/pcp/src/pmdas/logger/root
+++ b/pcp/src/pmdas/logger/root
@@ -3,6 +3,7 @@
  */
 
 #include <stdpmid>
+#include "domain.h"
 
 root { logger }
 
diff --git a/pcp/src/pmdas/logger/util.c b/pcp/src/pmdas/logger/util.c
index eca1bb0..594fba8 100644
--- a/pcp/src/pmdas/logger/util.c
+++ b/pcp/src/pmdas/logger/util.c
@@ -75,8 +75,11 @@ start_cmd(const char *cmd, pid_t *ppid)
      * the exec()?  Remove things like IFS, CDPATH, ENV, and BASH_ENV.
      */
 
-    __pmNotifyErr(LOG_INFO, "%s: Trying to run command: %s", __FUNCTION__,
+#ifdef PCP_DEBUG
+    if (pmDebug & DBG_TRACE_APPL0)
+	__pmNotifyErr(LOG_INFO, "%s: Trying to run command: %s", __FUNCTION__,
 		  cmd);
+#endif
 
     /* Create the pipes. */
     rc = pipe2(pipe_fds, O_CLOEXEC|O_NONBLOCK);