----- "Ken McDonell" <kenj@xxxxxxxxxxxxxxxx> wrote:
> On Mon, 2010-10-11 at 19:02 +1100, Ken McDonell wrote:
> > It has taken me a while to work through all of the issues here, not
> to
> > mention more distractions than you could hit with a big stick.
> >
> > Anyway putting an array of event records inside a pmResult is not
> going
> > to be too hard - see http://oss.sgi.com/~kenj/event-params.html
>
> Now would be a good time for feedback if you have any.
Read through it again, looks good - here's some feedback...
| There may be just one PMID and an associated PM_TYPE_STRING value
| in cases where the event subsystem produces event records as
| structured strings, e.g. XML or JSON encodings.
I think we should introduce PM_TYPE_JSON and PM_TYPE_XML so that the
clients can differentiate these from unstructured strings... (and do
the right thing, e.g. pcp-gui code can use Qt XML classes, QtScript
for JSON, etc) - now would seem like a good time, since we're adding
in PM_TYPE_EVENT to the set of types here.
| ... Similarly that we do not need the inst field ...
(typo, no need for "that" there)
| typedef struct {
| struct timeval er_timestamp;
| int er_nparams; /* number of er_param[] entries */
| pmEventParameter er_param[1];
| } pmEventRecord;
I think we need an "er_type" or perhaps an "er_flags" field above too
(there's room there for it, above nparams, so I'm getting in early!)
This might be "enum { PM_EVENT_POINT, PM_EVENT_START, PM_EVENT_END }",
which will let clients know what class of trace data this is, which is
not captured anywhere in the current proposal.
I'm thinking "er_flags" cos I think it will be useful for a client to
be able to tell in a generic way whether there are identifiers, so we
could extend that set to include PM_EVENT_TRACEID and PM_EVENT_PARENTID,
optionally signifying "1st parameter is a unique trace ID", and (also
optionally) "2nd parameter is the parent ID" ... ? (or, something like
that, anyway ...).
| typedef struct {
| int ea_nrecords; /* number of ea_record[] entries */
| int ea_nmissed; /* number of missed event records */
| pmEventRecord *ea_record[1];
| } pmEventArray;
Is ea_record being a pointer array a typo or intentional...?
| ... PM_CLUSTER_ERA ...
That name is confusing (ERA being both a word and TLA). Maybe just make
another pmid_* interface (like pmid_item, pmid_cluster, pmid_domain)...
and/or a #define alongside DYNAMIC_PMID like EVENTRECORD_PMID perhaps?
static inline int pmid_event_record_array(pmID id);
{
return __pmid_int(&id)->cluster == EVENT_RECORD_PMID;
}
Probably we should replace the open-coded uses of DYNAMIC_PMID with a new
pmid_dynamic() routine too to keep those details all together in impl.h.
Note that using -1 as the value for "ERA" limits PMDAs to 2^10 metrics...
probably OK I guess (not great), not sure there's any other better way to
attack this problem though... hmmm.
| Everywhere PM_TYPE_AGGREGATE is used in the existing code ...
This table doesn't cover libqmc in pcp-gui, where theres one extra use.
| pmUnpackEventRecords(pmValueBlock *vbp, pmResult **rec, int *nmissed)
So, if the above er_flags is introduced, I think this needs to become
more like those namespace routines (with optional status array) ...
int pmUnpackEventRecords(pmValueBlock *vbp, pmResult **rec, int **flags, int
*nmissed)
Either way, I think there also needs to be a
void pmFreeEventRecords(int count, pmResult *rec, int *flags)
And finally, pedantically, the strace example - if/when this PMDA is
written, PMID 62.0.0 should definately have string type - the numbers
change depending on Linux architecture, so this decoding should really
be done in the PMDA. OK, I did say pedantic!
cheers.
--
Nathan
|