Re: logger PMDA review

[Nathan Scott <nathans at aconex dot com>]

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