Re: [dpdk-dev] [PATCH 00/12] update and simplify telemetry library.

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, Apr 01, 2020 at 05:42:21PM +0200, David Marchand wrote:
> Hello,
> 
> 
> On Thu, Mar 19, 2020 at 6:35 PM Ciara Power <ciara.power@intel.com> wrote:
> >
> > This patchset extensively reworks the telemetry library adding new
> > functionality and simplifying much of the existing code, while
> > maintaining backward compatibility.
> >
> > This work is based on the previously sent RFC for a "process info"
> > library: https://patchwork.dpdk.org/project/dpdk/list/?series=7741
> > However, rather than creating a new library, this patchset takes
> > that work and merges it into the existing telemetry library, as
> > mentioned above.
> >
> > The telemetry library as shipped in 19.11 is based upon the metrics
> > library and outputs all statistics based on that as a source. However,
> > this limits the telemetry output to only port-level statistics
> > information, rather than allowing it to be used as a general scheme for
> > telemetry information across all DPDK libraries.
> >
> > With this patchset applied, rather than the telemetry library being
> > responsible for pulling ethdev stats and pushing them into the metrics
> > library for retrieval later, each library e.g. ethdev, rawdev, and even
> > the metrics library itself (for backwards compatiblity) now handle their
> > own stats.  Any library or app can register a callback function with
> > telemetry, which will be called if requested by the client connected via
> > the telemetry socket. The callback function in the library/app then
> > formats its stats, or other data, into a JSON string, and returns it to
> > telemetry to be sent to the client.
> >
> > To maintain backward compatibility, e.g. to allow the dpdk telemetry
> > collectd plugin to continue to work, some of the existing telemetry
> > code is kept, but is moved into the metrics library, and callbacks are
> > registered with telemetry for the legacy commands that were supported
> > previously.
> >
> > The new version of the library, apart from the legacy interface support
> > for backward compatibility, does not have an external dependency on the
> > Jansson library, allowing the library to be enabled by default.
> >
> > Note: In this version of the patchset, telemetry output is provided by
> > the ethdev, rawdev and eal libraries, but this may be expanded further
> > in later versions which are planned ahead of the merge deadline for
> > 20.05
> 
> This patchset does not apply on current master.
> Could you rebase it?
> 
> CI reported a build failure for Windows, please have a look.
> 
Yep, we spotted that and will fix in V2 (which naturally will also be
rebased).

> Is there a reason to keep a separate telemetry library rather than
> integrate this framework into EAL?
> 
No reason this could not be done, however, since telemetry library is
already separate, and EAL is already pretty crowded, I think keeping this
separate might lead to easier maintenance.

However, if people generally prefer it just merged into EAL, that can be
done also.

> This series removes the only user of the experimental rte_option API,
> which can be removed afaiu.
> 
Yep, that's something we spotted and intended to flag for discussion on the
V2 patchset.

My slight concern would be that having extra args for particular additional
libraries is something we might want in the future, so we should think
carefully before removing it. However, since overall I like shorter code,
I'd personally vote in favour of removal. :-)

Regards,
/Bruce