This is the mail archive of the systemtap@sourceware.org mailing list for the systemtap project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [pcp] logger PMDA review


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)


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]