logger PMDA review

logger PMDA review

[David Smith]

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 Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)

Re: [pcp] logger PMDA review

[Ken McDonell]

[-- Attachment #1: Type: text/plain, Size: 3253 bytes --]

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.

[-- Attachment #2: patch.logger --]
[-- Type: text/x-patch, Size: 8463 bytes --]

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);

Re: [pcp] logger PMDA review

[David Smith]

On 04/16/2011 09:30 PM, Ken McDonell wrote:
> 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

OK, that makes sense.

> 2. usage text does not quite match code

Must have missed that one in a rewrite.

> 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)

You are correct, I need to dial back the debug logic.

> 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?

In general, there is no "catchup" logic.  Clients see events from the
time when they attach.  Keeping old events around forever just in case a
new client attaches doesn't seem feasible (and a waste of memory).  For
log files (non pipe paths), I probably need to go ahead and seek to the
end when I open the file.

As far as record delimiters go, you are correct.  Right now the events
are stored in 1K chunks, but multiple events can be returned in a
record.  I've been waiting to fix this until we get a bit further in
deciding how we want to use the PMDA, but it is probably time now to fix
this.

> 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.

As I said earlier, there is no "catchup" logic.  Since "pminfo -x" opens
and closes the client connection, it will receive current data only, not
historical data.  Two concurrent pmvent processes should receive similar
updates of new data.

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

Thanks.

> 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.

Hmm.  Originally I wanted something like:

   logger.numclients
   logger.numlogfiles
   logger.param_string
   logger.syslog.records
   logger.syslog.numclients

But then I figured out I needed to add 'perfile' for something to hang
the dynamic PMNS names from.  I guess to do something like your idea
would mean I'd have to switch to completely dynamic PMNS names (instead
of having logger.{numclients,numlogfiles,param_string} be static), but
that isn't a huge deal.

I'm not sure I like having 'config.numclients', since I don't really
consider the number of clients a configuration item like the number of
logfiles is.

I think I'll probably leave this as is, especially to be able to avoid
the issue you mention of having to guard that 'config' name.

> Hope this helps.

It does - thanks a bunch.

-- 
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)

Re: logger PMDA review

[Nathan Scott]

----- Original Message -----
> After mucking around a bit, I've got a working version of my PCP
> logger
> PMDA. You can see the result by looking at:
> ...
> I'd appreciate any comments on the code. Thanks for the help.

Here's my random notes from a quick look through, some overlap
with Ken's...

Install
pmda_interface=4 -> pmdaproc.sh doesn't handle 5?
  (can see a case wrt help text - was that it?)
check for valid PMNS names, I stupidly used xxx-yyyy.
  (looks like pmda code checks, but maybe Install
   could too?)  echo $name | grep "^[A-Za-z][A-Za-z0-9]*$"
reserve a domain number - done

Sanity check (pminfo -dfmtT logger)
- no help text (not only just not dynamic ones)
- add a metric exporting name/path mapping?
- add a counter metric with nevents seen?

logger.c
- cannot call exit(1) - in dso case will terminate pmcd
- should have a default config file?  dso cannot work atm
  (or remove DSO case)
- not sure what param_string intention is?  (WIP?)

event.c
- exit on failure of one file is a bit extreme, if
  other files ok and file temporarily / not yet available.
  (exit vs DSO issue as well).
- comment typo "the we", "which logfiles is up to date"
- use of gettimeofday() - might be better to have option of
  parsing a timestamp from the line(s) of file and using that?
- "We can't really use select() on the logfiles... if normal file,"
  "select will (continually) return EOF ... So, now we read data "
  "inside the event fetch routine."
  - pmdaweblog? (see below)
- should definately seek to end of file at the start (commented out
  at the moment), else could read gigabytes of data on startup for
  large logfiles.

util.c
- start_cmd should be popen(2)?  more portable (works on Windows)
  (maybe not, due to need to kill it? -- __pmProcessCreate?)

percontext.c
- wonder if some of this should become generic libpcp_pmda code,
  similarities with pmdasample's needs.

In general:
- refer pmdaweblog ... similar sort of thing, predates events by ~10
  years or so, and is a bit old in the tooth, but code in there might
  be useful for reference.  the select loop in particular, and also
  the regex's pulling out timestamps (IIRC).
- might want to revisit using dynamic metric names versus metric
  instances, dynamic names generally a bit more complicated to code.
  (removes need to restrict/check names in Install too, although they
  do need to be unique)


-- 
Nathan

Re: logger PMDA review

[David Smith]

On 04/17/2011 06:21 PM, Nathan Scott wrote:
> 
> ----- Original Message -----
>> After mucking around a bit, I've got a working version of my PCP
>> logger
>> PMDA. You can see the result by looking at:
>> ...
>> I'd appreciate any comments on the code. Thanks for the help.
> 
> Here's my random notes from a quick look through, some overlap
> with Ken's...
> 
> Install
> pmda_interface=4 -> pmdaproc.sh doesn't handle 5?
>   (can see a case wrt help text - was that it?)

I can't quite remember now what the problem was, but since Ken said he's
fixed it anyway it doesn't really matter...

> check for valid PMNS names, I stupidly used xxx-yyyy.
>   (looks like pmda code checks, but maybe Install
>    could too?)  echo $name | grep "^[A-Za-z][A-Za-z0-9]*$"

'Install' could certainly check for valid PMNS names, but there is also
nothing from keeping a user from editing the config file by hand so the
code will still have to check.  Isn't an '_' a valid PMNS name character
(as long as it isn't the 1st char)?

> reserve a domain number - done

Thanks.

> Sanity check (pminfo -dfmtT logger)
> - no help text (not only just not dynamic ones)

Hmm, I certainly meant for the static PMNS items to have help.  I'll
look into it.  Adding help for the dynamic PMNS items doesn't look too
hard.

> - add a metric exporting name/path mapping?

That's a good idea - I could add something like:

    logger.perfile.{NAME}.path

> - add a counter metric with nevents seen?

Hmm.  That's tricky since the number of events seen would be client
specific.

> logger.c
> - cannot call exit(1) - in dso case will terminate pmcd
> - should have a default config file?  dso cannot work atm
>   (or remove DSO case)

On Ken's recommendation, I'm removing the DSO case.

> - not sure what param_string intention is?  (WIP?)

'param_string' is an artifact of how the event stuff works.  When you
call pmdaEventAddParam(), you have to tie the data atom to an existing
PMID.  'param_string' exists just for that purpose.

> event.c
> - exit on failure of one file is a bit extreme, if
>   other files ok and file temporarily / not yet available.
>   (exit vs DSO issue as well).

I was probably being lazy there.  That is certainly fixable.

> - comment typo "the we", "which logfiles is up to date"

Thanks, I'll fix that.

> - use of gettimeofday() - might be better to have option of
>   parsing a timestamp from the line(s) of file and using that?

I'll probably leave that one for now, until we get a bit further down
the road of figuring out exactly how we will use this.

> - "We can't really use select() on the logfiles... if normal file,"
>   "select will (continually) return EOF ... So, now we read data "
>   "inside the event fetch routine."
>   - pmdaweblog? (see below)
> - should definately seek to end of file at the start (commented out
>   at the moment), else could read gigabytes of data on startup for
>   large logfiles.

I'm not sure why I put off doing the lseek().  I'll fix it.

> util.c
> - start_cmd should be popen(2)?  more portable (works on Windows)
>   (maybe not, due to need to kill it? -- __pmProcessCreate?)

Right, the need to kill the subprocess on exit is why I didn't use
popen().  I see __pmProcessCreate() now, but it also doesn't return the pid.

> percontext.c
> - wonder if some of this should become generic libpcp_pmda code,
>   similarities with pmdasample's needs.

It is possible.  I certainly started with the sample PMDA percontext.c,
and tried to make it a bit more generic.

> In general:
> - refer pmdaweblog ... similar sort of thing, predates events by ~10
>   years or so, and is a bit old in the tooth, but code in there might
>   be useful for reference.  the select loop in particular, and also
>   the regex's pulling out timestamps (IIRC).

Thanks, I'll give it a look.

> - might want to revisit using dynamic metric names versus metric
>   instances, dynamic names generally a bit more complicated to code.
>   (removes need to restrict/check names in Install too, although they
>   do need to be unique)

On the dynamic metric name issue versus metric instances issue, I looked
into that a bit.  I was considering making each logfile a separate
instance.  However, I couldn't figure out from the client point of view
how to "subscribe" to only one instance of a particular metric.  It
seemed wrong to make the client consume data for multiple files when it
was only interested in one file's data.

If I'm wrong here, I'll be happy to rethink.

-- 
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)

Re: [pcp] logger PMDA review

[Ken McDonell]

On Mon, 2011-04-18 at 13:24 -0500, David Smith wrote:
> ...
> 
> > - might want to revisit using dynamic metric names versus metric
> >   instances, dynamic names generally a bit more complicated to code.
> >   (removes need to restrict/check names in Install too, although they
> >   do need to be unique)
> 
> On the dynamic metric name issue versus metric instances issue, I looked
> into that a bit.  I was considering making each logfile a separate
> instance.  However, I couldn't figure out from the client point of view
> how to "subscribe" to only one instance of a particular metric.  It
> seemed wrong to make the client consume data for multiple files when it
> was only interested in one file's data.
> 
> If I'm wrong here, I'll be happy to rethink.
> 

Clients use pmAddProfile and pmDelProfile to allow selection of one,
some or all of the instances for each instance domain ... once made, the
profile remains "sticky" for each subsequent pmFetch ... the default is
for pmFetch to return all instances for all instance domains.  This is
how pmval -i instance or pmevent metric[instance] works.

I suspect an instance domain is going to be a cleaner long-term solution
than dynamic metrics in the PMNS for your case of multiple log files
being processed.

Re: [pcp] logger PMDA review

[Josef 'Jeff' Sipek]

On Tue, Apr 19, 2011 at 12:28:08PM +1000, Ken McDonell wrote:
> On Mon, 2011-04-18 at 13:24 -0500, David Smith wrote:
> > ...
> > 
> > > - might want to revisit using dynamic metric names versus metric
> > >   instances, dynamic names generally a bit more complicated to code.
> > >   (removes need to restrict/check names in Install too, although they
> > >   do need to be unique)
> > 
> > On the dynamic metric name issue versus metric instances issue, I looked
> > into that a bit.  I was considering making each logfile a separate
> > instance.  However, I couldn't figure out from the client point of view
> > how to "subscribe" to only one instance of a particular metric.  It
> > seemed wrong to make the client consume data for multiple files when it
> > was only interested in one file's data.
> > 
> > If I'm wrong here, I'll be happy to rethink.
> > 
> 
> Clients use pmAddProfile and pmDelProfile to allow selection of one,
> some or all of the instances for each instance domain ... once made, the
> profile remains "sticky" for each subsequent pmFetch ... the default is
> for pmFetch to return all instances for all instance domains.  This is
> how pmval -i instance or pmevent metric[instance] works.
> 
> I suspect an instance domain is going to be a cleaner long-term solution
> than dynamic metrics in the PMNS for your case of multiple log files
> being processed.

Will using instance domains become an issue if one decides to merge several
log files?  The result of a merge may end up useless if the instance ids
changed between the two log files.

Jeff.

-- 
If I have trouble installing Linux, something is wrong. Very wrong.
		- Linus Torvalds

Re: [pcp] logger PMDA review

[Ken McDonell]

On Mon, 2011-04-18 at 23:08 -0400, Josef 'Jeff' Sipek wrote:
> ...
> > I suspect an instance domain is going to be a cleaner long-term solution
> > than dynamic metrics in the PMNS for your case of multiple log files
> > being processed.
> 
> Will using instance domains become an issue if one decides to merge several
> log files?  The result of a merge may end up useless if the instance ids
> changed between the two log files.

Good point Jeff.

Either the internal instance id needs to be in the configuration file
and needs to be kept the same for each external log file ... David
already has something like this in the PMNS prefix that is held in the
configuration file, this could be replaced by a small integer to be used
as the internal instance id (the external instance id could be the
pathname to the logfile).

Or the PMDA needs some persistent state to save the mapping from
internal to external instance identifiers ... refer to man pmdacache for
a set of routines to do _just_ _this_ on behalf of a PMDA.

Re: logger PMDA review

[Nathan Scott]

----- Original Message -----
> On 04/17/2011 06:21 PM, Nathan Scott wrote:
> ...
> 'Install' could certainly check for valid PMNS names, but there is
> also
> nothing from keeping a user from editing the config file by hand so
> the code will still have to check.

*nod*

> Isn't an '_' a valid PMNS name character
> (as long as it isn't the 1st char)?

Yep, good point, sure is.

> > - add a counter metric with nevents seen?
> 
> Hmm. That's tricky since the number of events seen would be client
> specific.

I meant more the number of (e.g.) lines read in the log file, which
would be more of a global event count (count since the pmda started)
... as an example, we've had to diagnose issues recently where an app
was meant to be writing to syslog but if misconfigured was not ... it
might have been helpful to have such a counter in this scenario.  And
while at it, a counter for the number of bytes seen would help tracking
how much log traffic is being sent at any time (also would've helped us
recently, and still would be useful actually).

> 
> > - use of gettimeofday() - might be better to have option of
> >   parsing a timestamp from the line(s) of file and using that?
> 
> I'll probably leave that one for now, until we get a bit further down
> the road of figuring out exactly how we will use this.
> 

OK, no problem.  Depending on how the file-read/select loop ends up,
this timestamp could easily be several seconds after the event, and
the log file might have a high resolution timestamp just waiting to
be parsed.

> > util.c
> > - start_cmd should be popen(2)? more portable (works on Windows)
> >   (maybe not, due to need to kill it? -- __pmProcessCreate?)
> 
> Right, the need to kill the subprocess on exit is why I didn't use
> popen(). I see __pmProcessCreate() now, but it also doesn't return the
> pid.

Wow, that was short-sighted of me ... that can be fixed though using
the existing API, just nothing needed the PID so far.  Will take a look.

> > percontext.c
> > - wonder if some of this should become generic libpcp_pmda code,
> >   similarities with pmdasample's needs.
> 
> It is possible. I certainly started with the sample PMDA percontext.c,
> and tried to make it a bit more generic.

Once we get one or two more real PMDAs, might be worth revisiting - yeah,
noticed yours was a bit more generic than pmdasample & looked like a move
in the right direction.

cheers.

-- 
Nathan