On 11/04/2014 11:40 PM, Nathan Scott wrote:
> Hi David,
>
> ----- Original Message -----
>> First, some background. I'm working on integrating systemtap and pcp,
>> and I wrote a python PMDA that will be similar to the mmv pmda in that
>> it will need to monitor a directory for new data. After working on it, I
>> found out that the pcp python support doesn't really allow adding new
>> metrics (or clearing metrics) after PMDA.run() has been called.
>>
>> So, commit 38983a5 in the pcpfans.git dsmith/dev branch adds a 'refresh
>> metrics' callback to the pcp python support. This allows a python pmda
>> to add/remove metrics after PMDA.run() has been called.
>
> Looking good.
>
>> Here's how you'd use it. In your PMDA.init() function, you'd set the
>> callback, like this:
>>
>> self.set_refresh_metrics(self.__refresh_metrics)
>>
>> Then your __refresh_metrics (or whatever you want to call it) function
>> will get called when needed. I based this code on the mmv pmda, so it
>> could get called during several PDUs. Then your callback function could
>> add metrics. New code in the PMDA class will update everything.
>
> diff --git a/src/python/pcp/pmda.py b/src/python/pcp/pmda.py
> index 5d4e71b..51263aa 100644
> --- a/src/python/pcp/pmda.py
> +++ b/src/python/pcp/pmda.py
> @@ -287,6 +287,7 @@ class PMDA(MetricDispatch):
> pmdaname = 'pmda' + name
> helpfile = '%s/%s/help' % (PCP.pmGetConfig('PCP_PMDAS_DIR'), name)
> MetricDispatch.__init__(self, domain, pmdaname, logfile, helpfile)
> + self.__refresh_metrics_func = None
>
> (see below for more, but I'm not sure we need to keep this
> handle to the callback routine up at this level as well as
> the cpmda callback handle? maybe we can simplify this)
>
> ##
> @@ -345,6 +346,26 @@ class PMDA(MetricDispatch):
> metrics[i] = self._metrictable[i]
> cpmda.pmda_dispatch(ibuf.raw, numindoms, mbuf.raw, nummetrics)
>
> + def __refresh_metrics(self):
>
>
> Hmm, do we need this interface? (there seem to be no callers in
> the wrapper - is the intention the PMDA would call this? If so
> a double-underscore (internal) routine is probably not right
> (for an API we are expecting PMDAs to use). If not explicitly
> needed, we should keep it simple and drop it (but, maybe I am
> missing something there?)
You have missed something here. See below.
> I was expecting this "refreshing" would happen around the time
> PDUs exchanges happen with pmcd, which delays it until the last
> moment (and prevents the situation where unnecessary duplication
> of "refresh" work from a PMDA is done). Can we set that "refresh
> needed" flag in cpmda land from the PMDA and delay this work til
> then? That'd also encapsulate a bit more of the implementation
> details by pushing that down into the lower layers of the wrapper.
>
> + # Call the user's function.
> + if self.__refresh_metrics_func:
> + self.__refresh_metrics_func()
> +
> + # Now that the user's function has been called, we need to
> + # refresh all our class internal data.
> + #
> + # FIXME: instead of using cpmda.get_need_refresh(), we could
> + # ask the user to return a '1' from the refresh metrics
> + # function if a refresh is needed.
>
> + if cpmda.get_need_refresh():
> + self.pmns_refresh()
> + cpmda.pmid_oneline_refresh(self._metric_oneline)
> + cpmda.pmid_longtext_refresh(self._metric_helptext)
> + cpmda.indom_oneline_refresh(self._indom_oneline)
> + cpmda.indom_longtext_refresh(self._indom_helptext)
> + cpmda.pmda_rehash(self._indomtable, self._metrictable)
>
> This ended up with lots of calls back down into the cpmda layer
> from the higher level python code - kinda feels like we may have
> some unnecessary up/down-calls here? (if that makes sense at all).
> IOW, can we push most of this down into the cpmda layer, hopefully
> making the interface exposed to PMDAs a little simpler/clearer?
So here's the deal with this. I probably should have explained this
better when I sent the patch.
Right now we've got 3 levels:
- low (cpmda)
- medium (PMDA class)
- high (user's class derived from the PMDA class)
Up at the high level (the user's class), a user calls add_metric() (for
instance), which updates several of the PMDA's class internal variables,
then calls the cpmda as needed.
So, when a user wants to use the new refresh functionality, he calls
PMDA.set_refresh_metrics(my_refresh_function). From the user's point of
view, here's what happens. His 'my_refresh_function' gets called when
necessary. In that function he can add new metrics or completely clear
out the existing metrics and start from scratch adding metrics.
The problem is that adding metrics updates a bunch of information at the
medium level, the PMDA class itself - all those _metric_* and _indom_*
lists above. So, if the low level (cpmda) called directly into the
user's derived class (the high level), the PMDA class data would get
updated, but neither the user's class or cpmda really knows anything
about the PMDA class' internal data.
So, the current solution wraps the user's refresh_metrics function with
its own function called PMDA.__refresh_metrics(). So, what happens is
that the cpmda really calls PMDA.__refresh_metrics() which calls the
user's my_refresh_metrics(). If when we get back from the user's
my_refresh_metrics() function something has changed that requires a
refresh, the PMDA.__refresh_metrics() function sends the cpmda all the
PMDA class internal data.
Note that the list of PMDA class internal data being sent down to the
cpmda in PMDA.__refresh_metrics() is exactly the same list as being sent
down in PMDA.run().
If you don't like the PMDA class wrapper around the users' refresh
function, another solution would be for the PMDA class to provide a
function that would need to be called at the end of the user's refresh
function to update everything (kind of like the PMDA.run() function).
>
>
> diff --git a/src/python/pmda.c b/src/python/pmda.c
> index f0c4845..3d6079c 100644
> --- a/src/python/pmda.c
> +++ b/src/python/pmda.c
> [...]
> +static PyObject *
> +pmda_rehash(PyObject *self, PyObject *args, PyObject *keywords)
> +{
> + char *keyword_list[] = {"indoms", "metrics", NULL};
> +
> + if (indom_list) {
> + Py_DECREF(indom_list);
> + indom_list = NULL;
> + }
> + if (metric_list) {
> + Py_DECREF(metric_list);
> + metric_list = NULL;
> + }
> +
> + if (!PyArg_ParseTupleAndKeywords(args, keywords, "OO", keyword_list,
> + &indom_list, &metric_list))
> + return NULL;
> +
> + if (indom_list && metric_list) {
>
> Do we require both of these (indom_list/metric_list) to be passed in?
> I guess it may happen that this code is used from PMDAs that do not
> have indoms (or the dynamic state is such that currently there are
> only metrics with no associated indoms?) - in those cases, we could
> be called here with a metric_list only I guess, so the logic in this
> branch would need some tweaking.
With the current code, calling this with an empty indom list would work
fine. Calling it with 'None' would probably fail.
> #ifdef PyBUF_SIMPLE
> + static void
> + pmda_refresh_metrics(void)
>
> ... you've got a fair bit of new code there that's only going to be
> compiled for the python 2.x series (guarded by the above ifdef) - I
> haven't tried, but I'm punting we'll need to refactor a bit here to
> make this work for python 3 as well.
Hmm, from my reading of the python devel code, PyBUF_SIMPLE is defined
in python 2.6+. I think the code in the '#else' here is dead code for
python < 2.6.
--
David Smith
dsmith@xxxxxxxxxx
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)
|