Re: [PATCH] trace: take live traces via telemetry

[David Marchand <david.marchand@redhat.com>]

Hello Morten, Bruce,

On Tue, Oct 18, 2022 at 4:34 PM Morten Brørup <mb@smartsharesystems.com> wrote:
> > --> /trace/list,lib.ethdev.*
> > {"/trace/list": {"lib.ethdev.configure": "Disabled",
> >     "lib.ethdev.rxq.setup": "Disabled",
> >     "lib.ethdev.txq.setup": "Disabled",
> >     "lib.ethdev.start": "Disabled",
> >     "lib.ethdev.stop": "Disabled",
> >     "lib.ethdev.close": "Disabled",
> >     "lib.ethdev.rx.burst": "Disabled",
> >     "lib.ethdev.tx.burst": "Disabled"}}
>
> Jerin commented that "Disabled"/"Enabled" are a bit verbose, and suggested shortening them.

I forgot to handle this part (EINTR...), thanks for reminding.


> It seems to me that these values are Boolean, and should be true or false (not surrounded by quotation marks), instead of some string representing a Boolean value. Note: This would require expanding the telemetry library with a Boolean type.

Indeed.

>
> Alternatively, use integer values 0 or 1.
>
> If we want to represent Boolean values as strings, I vote for "TRUE" and "FALSE", using all upper case to indicate that they are magic strings - and also to help avoid confusion with the JSON true/false Boolean values, which are all lower case.

Introducing those strings is confusing, especially if we later
introduce the boolean type.
I had a quick try and adding the boolean type is relatively easy (I
just posted a patch, see
https://patchwork.dpdk.org/project/dpdk/patch/20221019073702.3948624-1-david.marchand@redhat.com/).

I would either go with adding this new type (though we are past rc1,
this addition is self contained and low risk), or use simple integers.


-- 
David Marchand