Re: logger PMDA review

[Nathan Scott <nathans at aconex dot com>]

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