This is the mail archive of the systemtap@sourceware.org mailing list for the systemtap project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [pcp] suitability of PCP for event tracing


Now I'm done with the core implementation, I'm returning to some of the
peripheral issues raised in the (long) discussion on this topic ...

On Thu, 2010-11-11 at 10:49 +1100, nathans@aconex.com wrote:
> ...
> | 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.

I decided not to do this.  I'd like to keep PM_TYPE_EVENT as the sole
container for event records as this simplifies a lot of the new code.
Typed strings is a concept that probably makes sense independent of the
event records stuff, e.g. a simple metric that returns a PM_TYPE_STRING
value where the producer and consumer agree the value is encoded in a
particular way, e.g. JSON.

One option would be to add more values to the sem (semantics) field of
the pmDesc, e.g. PM_SEM_XML, PM_SEM_JSON, ... these additonal semantic
settings would need to imply PM_SEM_DISCRETE or PM_SEM_INSTANT.  This
would
      * make them available for all metrics (not just the parameter(s)
        for an event record), and
      * involve NO PDU, API or archive changes
      * seem more natural as it is part of the semantic metadata for the
        metric, rather than something to be encoded in the vtype header
        of a pmValueBlock (which is where the PM_TYPE_* data for the
        composite data types are carried around in each fetch result)

Thoughts?

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

I considered er_flags but this seems to be dependent on prior agreement
of the producer and consumer, and other than transporting this in PDUs
and archives and across the APIs there is nothing PCP needs to do based
on the er_flags value.  Also, there is no obvious place to store the
value when the packed array of event records is expanded (it cannot go
in the pmResult, so needs another allocation and return parameter for
pmUnpackEventRecords (as you indicated later).

I'd prefer to see event "flags" or event "type" encoded in a specific
parameter of the event record.  This also needs prior agreement by the
consumer and the producer, enables all of the same functionality that
er_flags would, and requires no additional API or data structure
changes.

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

Agreed, the macro became PM_CLUSTER_EVENT.

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

Not sure that this is worth it, the value is used only once in the
sample PMDA so far.  None of the libraries or tools need to know or care
about the specialness of this PMID encoding.

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

Not so ... the number of metrics per PMDA is 2^10 (items) x (2^12-1)
(clusters) because I've only pinched one value for the cluster field.
> 
> | Everywhere PM_TYPE_AGGREGATE is used in the existing code ...
> 
> This table doesn't cover libqmc in pcp-gui, where theres one extra use.

I haven't done the pcp-gui changes yet.

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

Don't like this ... see above.

> Either way, I think there also needs to be a
> void pmFreeEventRecords(int count, pmResult *rec, int *flags)

I added the convenience function pmFreeEventRecords() with one pmResult
** parameter (pmUnpackEventRecords returns an array of pmResult pointers
that is NULL terminated, so count is not needed).

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

Nod and wink.

Thanks for the feedback Nathan.

I suspect we need to wait for someone to use all this new functionality
in anger before we will be in a position to consider tweaks to what has
been done so far.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]