Hi Miroslav,
On Wed, Sep 08, 2021 at 08:02:35PM +0200, Miroslav Zagorac wrote:
> On 09/08/2021 07:57 PM, Miroslav Zagorac wrote:
> > On 09/08/2021 07:42 PM, Willy Tarreau wrote:
> > > No rush on this one, I'll let you think about it, just let me know if we
> > > need to temporarily disable it from the CI, or if you just want a day or
> > > two of checking before knowing if you can get a quick solution or if it
> > > will take more time.
> > >
> >
> > Thank you for understanding. For a start it would be good to exclude
> > ot filter from the compilation process so as not to interfere with the
> > development and testing of other code.
OK I can do that.
> > Next, I could throw out the part related to context transfer via
> > variables with #ifdef/#endif blocks, so that the ot filter code is
> > compiled without it.
> >
> > After that the problem with variables could be solved without
> > interfering with the rest of the coding/compilation process and the
> > ot filter will be functional (except for the part with context transfer
> > via variables, but the question is whether anyone uses it).
OK. Given that I really have no idea what it's used for (from the
user's perspective), I cannot judge. But maybe we can start with
this and ask for testers.
> > The variable 'sess.ot.uuid' is just set, it is not used anywhere (as
> > far as memory serves me). I imagined that this could be used if the
> > user needs it, but it is not necessary.
> >
>
> OK, it is still used but again only as an example that it can be used
> and not that it should be used.
>
> In our examples uuid is set as one of the data from the opentracing
> 'baggage' data group:
>
> test/ctx/ot.cfg:
> ---
> baggage "haproxy_id" var(sess.ot.uuid)
> ---
Yes, that's where I found it but didn't know what its use was. A few
points however:
- sess.ot.uuid will be shared by all requests on the same session,
which is probably not what you want (e.g. for requests coming in
H2, the last one setting it will replace all other ones' value
while they're working with it). I think you should change its
scope to be limited to the transaction only (txn.ot.uuid). It's
also used in debugging outputs where it will be per-request and
may not match what the requests will log because of this.
- I finally found its definition here in filter.h:
#define FTL_OT_VAR_UUID "sess", "ot", "uuid"
Please note the typo in the macro name (FTL instead of FLT)
- the comment in struct flt_ot_runtime_context for uuid says
"Randomly generated UUID". We already have a function to produce a
random UUID (ha_generate_uuid()) but flt_ot_runtime_context_init()
makes its own from elements that at first glance look like they're
filled from other locations (uuid.node, uuid.clock_seq etc), but in
fact are simply aliased on the random ints. In this case better use
the existing function and avoid duplicating the painful output format
generation. If you need to keep a copy of the two randoms, just
modify the existing function to pass an optional array to store
the randoms. At first glance they're not used though.
- there's quite a bunch of functions with no description at all of
their purpose, input values, return values etc. I know pretty well
how painful it is to document that, especially since every single
time we have to go through this we find bugs due to the caller and
callee disagreeing on input domain or output values (typically the
type of thing Coverity loves to look at). This is also why it's
important. Hint: at least fill the description entry and explain
limits/restrictions if any. The locations of empty descriptions
can be found with this:
$ pcregrep -nMo1 '^ \* DESCRIPTION\n \* -\n' addons/ot/src/*.c
I'm not asking you to add all of them immediately, but I'm willing
to apply cleanup patches that routinely add a few of them when you
have a bit of time to work on this. And of course bugfixes as well
if you find bugs while documenting.
Thanks,
Willy
