Hi Mark,
----- Original Message -----
> Changes committed to git://oss.sgi.com/markgw/pcp/pcp.git dev
>
> First version of the new collectl2pcp importer. No QA yet, that is
> next - before progressively adding more support for other metrics
> found in collectl raw data archives.
Looking good. Some small stuff from reviewing...
man/man1/
- man page doesn't do collectl(1) referencing right - should it be:
.BR collectl (1)
- add reference to collectl(1) at end of man page.
- just for grins, maybe add mention of pmcollectl(1) as well
- the man page explicitly says "supports XYZ metrics only currently"
which makes it brittle to future code updates to increase coverage -
the code does a good job of handling the feedback about skipped data
so perhaps the man page could just have words describing that output
(which will hopefully lessen over time).
- loving all the packaging work that has been done, thanks!!!
src/pmimport/collectl2pcp
- the -a option seems a bit wierd in collectl2pcp.c - a mandatory
option? I was expecting syntax more like:
$ collect2pcp infile outfile
- allowing multiple infiles on the cmd line opens up the problem
of having data from different hosts - is there any advantage to
supporting multiple files, rather than just doing em individually
and then running pmlogmerge? I guess its a usability thing for
when you have many days worth of data right? Fair enough then but
it does obfuscate the code a little, so...
- code could do with a bit of refactoring - everything in main(),
perhaps the inner loops (from "parse collectl header" and the code
below that) could become a new function, passed in a file pointer
and one or two other things
- the two snippets of code that begins "if (filenum == 0)" - looks
like they could be moved out of all of the loops entirely (after
pmiStart)
- the handlers seem a little brittle in the face of odd data, eg.
the cpu_handler assumes the passed buffer will be of length >3 -
might be a good idea to pass the length of the line just read in
to these callbacks so they're more inclined to validate the input
they are given?
- could we #include pmdas/linux/indom.h instead of duplicating it?
from a quick look, looks like it doesn't pull in many odd headers
or define too many things that'd make that difficult.
- re the TODOs in util.c - those seem like micro-optimisations -
if you implement 'em, great, but i wouldn't leave a TODO comment
in the code forever if not.
cheers.
--
Nathan
|