Hi Frank,
Following on from our #pcp and other earlier discussion, I've
pulled pmdasystemd into the dev branch. I made a few tweaks,
all minor - build related, and typos.
[ This is another event metric PMDA, so I'd encourage others
to review and pass on judgements^Wopinions as well, if they
have any thoughts, since this is a, er, growth industry. Some
interesting precedents are being set by experimentation with
these new agents at the moment and the more eyes the merrier. ]
I have a few other review comments as well, things I didn't
think of earlier ... let know your thinking on these, please.
- Can this PMDA run as an unprivileged user? If the answer is
yes, the attached "unprivileged.patch" would suit. However if
the answer is no, then the DSO mode is not useful & we may as
well remove that code.
- There's an empty README file in src/pmdas/systemd - nuke it?
A man page might be handy to brain dump and issues or tidbits
for people trying it out - perhaps see pmdabash.1 as a sample.
- I noticed the metric names seemed to glop (to my eye) both
the event metrics and their parameters into one pmns subtree.
I noticed also there's a little comment to say "these ones are
one group" and "these others are event metrics" - suggests that
the parameters might warrant their own little part of the pmns.
Up to you, though. I experimented with one way (see attached
"parameters.patch") but other options might be more clear - it
might be better to use the terminology in your comment, which
could result in systemd.journal.fields.*, for example.
The code generally looks great though, hopefully it will become
a generally handy little PMDA for people once systemd has taken
over the world. ;)
cheers.
--
Nathan
unprivileged.patch
Description: Text Data
parameters.patch
Description: Text Data
|