Re: [PATCH] log virgin HTTP request headers

Amos Jeffries wrote:
> Tsantilas Christos wrote:
>>> Tsantilas Christos wrote:
>>>> Hi all,
>>>> This patch adds a new format code which allow the user to log HTTP
>>>> request header or header fields before they are adapted.
>>>> The existing "http::>h" format code logs HTTP request headers after
>>>> adaptation.
>>>> The new format code is the "http::>hv".
>>>>
>>>> This is a Measurement Factory project.
>>>>
>>>> Regards,
>>>> Christos
>>>>
>>> Um, I would think this makes more sense done the other way around.
>>>
>>> With the default >h displaying the virgin headers received from the
>>> client and some other code ( >ha ?) for the adapted headers. Be it
>>> adaptation or headers_access doing the alteration.
>>
>> The only objection I have is that the >h is already implemented to log
>> headers after adaptation for squid3.0 and squid3.1
>
> I know. However its documented as merely "request header" with
> indication that it's adapted first. It's historic from squid-2 where no
> adaptation happened to them. So IMO the fact that it displays the
> adapted header is kind of a regression.
>
>>
>> I did not have in my mind the headers_access, so in the patch the new
>> format code activated only if the adaptation is active.
>> Should the http::>hv (or http::>ha) be always active?
>
> Yes. IMO always active.

At the end this is not so simple.
When we are doing adaptation we are replacing the old HttpRequest object
with a new one. Logging virgin and adapted HttpRequest is simple and
does not have a Huge cost, just do not release the virgin HttpRequest
object.

But the header_access just removes the requested headers from the virgin
HttpRequest object. Requires a different approach. Maybe just make a
clone of the HttpRequest is enough, but does it worth the extra cost?

I do not know...
Are they any opinions from other developers?

>
> If you can find a nice spot prior to *_header_access to save the virgin
> data that would be good. But ...
>
> (Out of scope)
> we also have bugs open to make the header_access happen after
> adaptation. Which makes more sense than the current flow order since we
> really do want the original headers to go to ICAP and header_access to
> prevent/allow stuff going outbound.

It should not be difficult to make a fix ...

>
> Amos
Received on Thu Jan 14 2010 - 22:29:07 MST