RE: [PATCH] doc: add deprecation for restrictions in telemetry naming

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Monday, 11 July 2022 12.54
> 
> On Fri, Jul 08, 2022 at 12:06:31AM +0200, Morten Brørup wrote:
> > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > Sent: Thursday, 7 July 2022 15.40
> > >
> > > Following discussion on-list [1], we will look to limited the
> allowed
> > > characters in names for items in telemetry. This will simplify the
> > > escaping needed for json output, or any future output formats. The
> > > lists
> > > will initially be minimal, since expansion to allow more characters
> can
> > > be done without affecting compatibility, while reducing the set
> cannot.
> > >
> > > Cc: mb@smartsharesystems.com
> > > Cc: stephen@networkplumber.org
> > > Cc: ciara.power@intel.com
> > >
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > >
> > > [1] http://inbox.dpdk.org/dev/20220623164245.561371-1-
> > > bruce.richardson@intel.com/#r
> > > ---
> > >  doc/guides/rel_notes/deprecation.rst | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/doc/guides/rel_notes/deprecation.rst
> > > b/doc/guides/rel_notes/deprecation.rst
> > > index 4e5b23c53d..9366690ec5 100644
> > > --- a/doc/guides/rel_notes/deprecation.rst
> > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > @@ -119,6 +119,12 @@ Deprecation Notices
> > >  * metrics: The function ``rte_metrics_init`` will have a non-void
> > > return
> > >    in order to notify errors instead of calling ``rte_exit``.
> > >
> > > +* telemetry: The allowed characters in names for dictionary values
> > > will be limited to
> > > +  alphanumeric characters and a small subset of additional
> printable
> > > characters.
> > > +  This will ensure that all dictionary parameter names can be
> output
> > > without escaping
> > > +  in json - or in any future output format used. Names for the
> >
> > json -> JSON
> >
> Capital idea! (pun very much intended :-) )
> 
> > > telemetry commands will
> > > +  be similarly limited.
> >
> > Perhaps also add a comment about parameters to telemetry commands,
> for completeness.
> >

I intentionally phrased this comment vaguely, to see what you had been thinking about this. And it certainly had the desired effect. :-)

> 
> I was not intending to impose restrictions on the parameters themselves
> since currently they are not output as part of any json. However, now
> you
> have got me thinking that perhaps we should look to scan parameters for
> invalid characters before we hand them over to the individual
> functions.
> That would allow the possibility of including parameters in any replies
> in
> a future format.
> 
> Was this what you had in mind, or any other thoughts?

Not exactly what I had in mind...

Your patch adds that "Names for the telemetry commands will be similarly limited.". This is input, not output. So you need to describe what restrictions are imposed on input.

The input commands and format don't follow any structured standard; command names, hierarchy and parameter names are individually chosen by each developer, and parameters are just a bunch of param=value with no types or limits to the values.

Also, the input is not JSON formatted, but - without looking deeply into the telemetry library - I suppose it might be URL encoded, where e.g. space is encoded as "%20" and '&' is encoded as "%26".

I think we should just leave the input without restrictions. Changing it would require a major overhaul to provide any significant improvement, e.g. attaching types to the parameters, so their values are not just BLOBs.

I don't strongly oppose to limiting the input command names; but we shouldn't impose any limit on what follows the command. So I'm proposing to explicitly mention that we don't impose any input limits beyond the command names.

Or we could provide input restrictions and parsing/formatting in a separate patch set.