----- Original Message -----
>
> nathans wrote:
>
> > [...] If you have time available, please have a read and send back
> > any feedback you have [...]
>
> It looks good. Some nits:
Thanks!!!
> * page viii: as we discussed on IRC, "man -k performance" is too broad;
> perhaps we should just link to "man PCPIntro".
Done, with associated changes to surrounding text (remove reference to
"all" pcp man pages, added note about SEE ALSO sections).
> * page 2, need motivation/mention for the archive folio concept
*nod*, done.
> * page 7, pmdasystemd: This does not extract performance metrics, but
> turns journal records into pcp events.
Ah we overlooked the pmdaEventQueueCounter & pmdaEventQueueBytes
interfaces there - the library is counting these important things
(syslog IOPS/throughput), we're just not exporting 'em yet. We
totally should, these metrics proved very useful for production
rsyslog deployments.
> * page 7, mention pmdalogger: extracting lines of text from a log file
> into pcp events
(which does make the above calls & export that data, for reference).
Doc updated.
> * page 7, pmwebd: Might mention HTTP beside JSON, just to spell out
> the transport protocol.
*nod*
> * page 4...9: suggest moving list of tools after the 'conceptual
> foundations' stuff
Yep - that'll flow alot better - done.
> * page 11: "there may be at most one pmcd process": not technically true,
> as people can run their own (on private ports, etc.); how about "normally"?
Done.
> * page 14: "PMCS" - is there any need for this term? How about saying that
> the pcp libraries provide utility functions for PMDAs? Or just use "PCP"
> instead of "the PMCS" throughout the book?
I think this is one of Ken's historical terms, I'll wait to hear if he's
particularly attached or still feels it adds explanatory value. I tend
to think its use could be replaced by a more generic "PCP" reference as
well, and fewer-is-best when it comes to acronyms.
> * page 17: ${PCP_RC_DIR} appears unannounced. Refer to /etc/pcp.conf
> earlier?
It is always /etc/pcp.conf (that path cannot be changed, although strictly
speaking should be ${PCP_DIR}/etc/pcp.conf - will fix). There is reference
very early on (in "Conventions" section) explaining these ${PCP_VARIABLE}
annotations. The "PCP Environment Variables" section also goes into detail
and describes the lay of the land, access from different languages, refers
to pmGetConfig(5) and so on.
> * page 23: the "store" operation reminds me, we should take that permission
> out of default pmcd.conf's.
Hmm, why? I tend to disagree ... that will break things and removes useful
functionality. What were you thinking it'll protect to disable that? We're
currently doing (which seems fair and good to me) ...
[access]
disallow * : store;
allow localhost : all;
There's sneaky things like pmcd.client.whoami and the event metric filters
that are hobbled without store capabilities, and no metrics I can think of
that we ship are questionable re store access in general. It's more what
other people (users/admins customising pcp) might do that the remote store
entry in the [access] section is protecting, I think.
> * page 24, the "collector or monitor or both" question could be clearer.
> I must admit I don't understand what exactly we're asking.
Yeah, the reason for that is a bit historical. The programmers guide goes
into it a bit more, after giving some background. Will see if I can craft
a shorter explanation here.
> * page 27, "cannot connect to remote pmcd". Note firewalls.
Done.
> * page 28, "netstat -a | grep 44321" presumes pmcd missing in /etc/services;
> use "netstat -atn" instead.
Done.
> * page 31, "dkvis" - whatchewtalkingaboutwillis?
Hehe. Yes, probably not the best place to introduce this tool, which only
exists in experimental branch of an experimental tree, in open source.
> * page 32, "The -h and -a options are mutually exclusive in all cases"
> That is kind of sad actually; we should be willing to display a mixture
> of historical and live data throughout.
Its not quite true too - pmchart does allow both (although at the time the
book was written, it didn't).
For alot of tools though, it really does make no sense (pminfo, pmprobe,
pmie, pmval, pmdumptext...) but anything that uses pmtime is a candidate
for doing a better job of handling both modes simultaneously, I guess.
> * page 32. "/etc/pcp.*": those files should be listed far earlier, to explain
> the use of ${PCP_FOOBAR} variables throughout the book. But actually,
You may have skipped over the Conventions section where this is documented
very early on (as early on as I sensibly could).
> is there some reason we don't autoconf-generate some fragment of this book,
> so that actual per-installation path names are shown in the book? Maybe
> everywhere, maybe in an appendix about This Very Installation?
Thought about that too, but in practice I don't think it'll work. Its likely
the vast majority of people reading this will be from web-downloads from the
front page of the website or google or wherever, in which case the This Very
Installation situation may not even exist.
I also think its worth showing this variable parameterisation concept in the
books, since its pervasive throughout anyway; once you start using PCP, you'll
need to know about it (useful to know its how platform-independence is handled
in PCP).
> * page 35, just a note, some neat libraries exist that take freeform english
> to specify time points / intervals, like "last thursday, 3pm"; perhaps we
> should pull them into libpcp for use with -S etc.
Sounds neat - well worth further investigation.
> * page 36, just a note, PCP_COUNTER_WRAP seems like a hacky hammer; if
> anything, it should be per-metric or something. If it's only used
> in diagnostic emergencies, maybe not document it here?
IIRC, this is (was?) intended as a complete list of env vars someone might
ever need to set. I think this was introduced as a back-compatible measure
when the default counter reporting semantics were changed many years ago as
well.
> * page 37, just a note, PMLOGGER_PORT, pmlc<->pmlogger really need some
> authentication
*nod* (and secure/encrypting connections need to precede that).
> * page 38, our mysterious superhero dkvis makes another appearance
Gone.
> * page 38, just a note, pmproxy and similar need outbound ACLs;
> bug already filed
*nod*
> * page 39, the "-t interval" mention at the bottom looks weird word-wrapped
Fixed.
> * page 49, the teaser use cases for pmie sound good; if those are actually
> implementable today,
? re "actually implementable" - its fairly arbitrary what one might run
& under what conditions (you can run anything and detect almost anything
which is what those short examples at the start of the chapter are trying
to convey I guess) ... there's many examples shipped, documented here,
man pages and so on. So, I'd be inclined to just plant the seed there &
move right along (as it does now). Section 5.4 later on does go into the
location of more examples and references into the tutorial as well.
> we should provide a link to the pmie configuration
>
> * page 54, top, backslash looks like \ not / :-)
Hah! Fixed.
> * page 82, mysterious new caped superhero "pmview" makes an appearance
... and exits stage left.
> * page 92, add-on products shping / dbping being sweaty sidekicks to the
> heros;
> as they're in the base package, why not drop "add-on products" vice
> "included non-default PMDAs"?
*nod*, yeah, those words were historical baggage.
> * page 103, PMNS syntax, I wonder whether this part needs documenting. How
> much do you expect a pcp user/administrator to read, never mind write such
> a file?
I believe its documented here (in 8.3) because PMNS customisation is required
in using the summary PMDA (8.1) - which forward-links to it.
> * page 107, do we need to document the PDU concept?
> (I above suggested dropping PMCS also.)
Yes, theres references earlier on to "PDU timeouts" and such (also a reference
in pmcd.conf earlier on), so this Acronyms section reference does indeed seem
warranted.
Will wait for Kens input on removal of PMCS, but thinking it would be good to
ditch too, myself. I understand why its been introduced, but we have so many
acronyms (PCP, PMDA, PMCD, PMID, PMINDOM, PMAPI) that I'm not really sure it's
adding enough value to keep it.
Thanks for the comprehensive review!!!
cheers.
--
Nathan
|