Hi Frank,
overall this looks very nice, straightforward to setup and easy access
to the results. Seeing how thin the Python side is reassuring that the
approach is right. But let me still probe on a few things to see if we
could make it even more simpler..
On 2015-12-06 22:47, Frank Ch. Eigler wrote:
>
> Fresh on pcpfans.git fche/fetchgroup, comes python bindings. The
> outline checked with Marko earlier turns out to be implementable.
>
> % cat foo.py
> from pcp import pmapi
> import cpmapi as c_api
>
> ctx = pmapi.pmContext(c_api.PM_CONTEXT_HOST, "local:")
> pmfg = pmapi.fetchgroup(ctx)
> v = pmfg.extend_item("hinv.ncpu", c_api.PM_TYPE_U32)
> vv = pmfg.extend_indom("kernel.all.load", c_api.PM_TYPE_FLOAT)
- I mentioned about this earlier relating to the C API but I think it's
worth checking again with the Python side: has the type argument really
be mandatory here? If the user don't pass it, it'd require two lines to
figure it out in the Python layer on the user's behalf and the user can
later on do type(v()) or str(v()) or such as needed. This way there
wouldn't be need to expose the low level C types in the Python API. (And
I think it'd be better to use native types internally for as long as
possible rather than converting to/from PM_TYPE_STRING even if it works
now.)
- Regardless of the above, perhaps it might be better to define
extend_item(self, metric=None, mtype=None, scale=None, instance=None)
instead of
extend_item(self, metric=None, mtype=None, instance=None, scale=None)
so that the scale would follow metric (and possibly mtype) in both _item
and _indom variants and the "instance limiting" parameter would be the
last one (or perhaps vice versa if you prefer).
- I was thinking a moment for the possibility to combine the _item and
_indom variants but perhaps that's not a good idea.
> pmfg.fetch()
> print ("number of cpus: %d" % v())
> for icode, iname, value in vv():
> print ("load average %s: %f" % (iname, value()))
Here things look exceptionally elegant :)
Thanks,
--
Marko Myllynen
|