Re: logger PMDA review

[David Smith <dsmith at redhat dot com>]

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)