On Thu, Feb 27, 2020 at 12:50:44PM +0100, Tim Düsterhus wrote:
> >> /* add unique-id if "header-unique-id" is specified */
> >> + if (sess->fe->header_unique_id &&
> >> !LIST_ISEMPTY(&sess->fe->format_unique_id)) {
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^
> > All the unique-id generation seems to be enclosed in this. Am I missing
> > something ?
>
> Yes. The `header_unique_id` check is only in `http_process_request`.
> I've merged the previous two ifs into a single one. A unique ID in
> `http_process_request` is only generated when it is actually sent to the
> upstream server. If it is not sent we don't need to generate one,
> because no one is seeing it. This might technically be a slight behavior
> change (the unique ID is generated later), but the user won't see a
> difference.
Thank for the explanation (this should be part of the commit message
so that someone bisecting to that commit later doesn't ask the question
again).
> By replacing all the ad-hoc generation of the unique IDs with a call to
> `stream_generate_unique_id` the unique ID will be generated when it is
> first used. This is happening in this order:
>
> 1. unique-id-header
> 2. unique-id sample fetch
> 3. Logging
I'm just having a doubt now. What if the unique-id-format refernces
some part of the request ? Let's say I have this :
unique-id-format %ci_%cp_%[req.hdr(host),crc32,hex]
And in my logs I have "%ID".
Does this mean that the unique-id will now be generated only when
logging ? Because if that's the case it won't find any element from
the request anymore.
> I specifically added debug code locally that printed a message with the
> current function when the unique ID is actually generated for a request.
> If neither the header is set, not the sample fetch is used then an ID
> will still be generated for the logging.
OK. I'm just concerned about the case above, to be sure the unique-id
is still built during the request processing and not too late.
> > (...)
> >
> > Other than that I have a suggestion, I've seen recently in this thread
> > a few calls to strlen() on unique_id and header_unique_id. I think they
> > should be turned to ist so that the length is always stored with them
> > and we don't need to run strlen on them anymore at runtime. And this
> > will simplify the header addition which will basically look like this:
> >
> > http_add_header(htx, sess->fe->header_unique_id, s->unique_id);
>
> I was thinking about this, but didn't make the change to keep the diff
> small and because it increases the size of a stream by an additional
> `size_t` which I'm not sure is desired. I can create a follow up patch
> if you want.
It would be nice. We're trying to progressively clean such things from
the code. There are still a lot of them, cookie names and whatever, over
which we run strlen for each and every use case, or we have a separate
length and do manual operations to keep them in sync while we already
have everything needed to manipulate them directly.
Thanks!
Willy
