doc: add deprecation for restrictions in telemetry naming

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

On 7/7/22 16:39, Bruce Richardson wrote:
> 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 telemetry commands will
> +  be similarly limited.
> +
>   * raw/ioat: The ``ioat`` rawdev driver has been deprecated, since it's
>     functionality is provided through the new ``dmadev`` infrastructure.
>     To continue to use hardware previously supported by the ``ioat`` rawdev driver,

Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
  

> 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

> telemetry commands will
> +  be similarly limited.

Perhaps also add a comment about parameters to telemetry commands, for completeness.

> +
>  * raw/ioat: The ``ioat`` rawdev driver has been deprecated, since it's
>    functionality is provided through the new ``dmadev`` infrastructure.
>    To continue to use hardware previously supported by the ``ioat``
> rawdev driver,
> --
> 2.34.1
> 

Acked-by: Morten Brørup <mb@smartsharesystems.com>
  

> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Thursday 7 July 2022 14:40
> To: dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richardson@intel.com>;
> mb@smartsharesystems.com; stephen@networkplumber.org; Power, Ciara
> <ciara.power@intel.com>
> Subject: [PATCH] doc: add deprecation for restrictions in telemetry naming
> 
> 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
> +telemetry commands will
> +  be similarly limited.
> +
>  * raw/ioat: The ``ioat`` rawdev driver has been deprecated, since it's
>    functionality is provided through the new ``dmadev`` infrastructure.
>    To continue to use hardware previously supported by the ``ioat`` rawdev
> driver,
> --
> 2.34.1

 Acked-by: Ciara Power <ciara.power@intel.com>
  

On Thu, Jul 7, 2022 at 3:39 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> 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>
Acked-by: David Marchand <david.marchand@redhat.com>
  

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

/Bruce
  

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