Re: %adapt::<last_h mess

 On Mon, 14 Mar 2011 09:26:49 -0600, Alex Rousskov wrote:
> On 03/11/2011 05:52 PM, Amos Jeffries wrote:
>> On 12/03/11 12:59, Alex Rousskov wrote:
>>> Hello,
>>>
>>> All of the following is related to the %adapt::<last_h
>>> access.log
>>> format code (possibly parameterized with the header or element
>>> name):
>>>
>>> * squid.conf says it is the "last ICAP header received",
>>> essentially.
>>>
>>> * LFT_ADAPTATION_LAST_HEADER takes ICAP headers from all ICAP
>>> messages
>>> received, merges them (last header field wins if there are two ICAP
>>> headers with a field that is named the same), and logs the matching
>>> ICAP
>>> header field value.
>>>
>>> * LFT_ADAPTATION_LAST_HEADER_ELEM is the same as the
>>> LFT_ADAPTATION_LAST_HEADER above, but logs the matching ICAP header
>>> field value element.
>>>
>>> * LFT_ADAPTATION_LAST_ALL_HEADERS logs the last ICAP header
>>> received as
>>> described in squid.conf.
>>>
>>>
>>> In other words,
>>>
>>> * squid.conf and %adapt::<last_h without parameters are about
>>> logging
>>> the last ICAP header received;
>>>
>>> * parameterized %adapt::<last_h merges all the received ICAP
>>> headers
>>> (later headers overwrite earlier ones if there are name conflicts)
>>> and
>>> logs the last matching header field value/element.
>>>
>>>
>>> I suspect %adapt::<last_h without parameters is generally useless
>>> in
>>> access.log due to the volume of mostly irrelevant information. When
>>> somebody is using %adapt::<last_h in production, they probably log
>>> specific header field values or elements.
>>>
>>> I know that folks use parameterized %adapt::<last_h to log various
>>> transaction IDs and flags delivered by ICAP and eCAP services. For
>>> example, an eCAP service may identify the detected Virus ID or an
>>> authenticated user ID.
>>>
>>> In the cases I am familiar with, the merging performed by
>>> parameterized
>>> %adapt::<last_h is the desired/correct behavior because those Squid
>>> admins care about the ID value and not about which adaptation
>>> service
>>> determined it. They cannot assume that the ID was determined by the
>>> last
>>> service Squid talked to.
>>>
>>> However, our documentation describes a different behavior (no
>>> merging,
>>> just use the last received ICAP header), and I would not be
>>> surprised if
>>> there are users who use parameterized %adapt::<last_h while relying
>>> on
>>> the documented behavior. These users probably have just one ICAP or
>>> eCAP
>>> service or their last service always sets the header they log;
>>> otherwise, they would be logging wrong values due to the current
>>> implementation.
>>>
>>>
>>> To resolve the above conflicts, we need to change documentation,
>>> implementation, or both. I propose the following:
>>>
>>> 1. Add "%adapt::<merged_h" and document the currently implemented
>>> merging algorithm where the last header wins conflicts but all
>>> headers
>>> can play. Make sure the code implements it in all cases
>>> (parameterized
>>> and not).
>>>
>>> 2. Deprecate "%icap::<last_h" and remove support for
>>> "%adapt::<last_h".
>>> If somebody complaints, we can add "%adapt::<last_h" back, this
>>> time
>>> with the right implementation.
>>>
>>> Meanwhile, "%icap::<last_h" will default to "%adapt::<merged_h" for
>>> backward compatibility. Since no released version has
>>> "%adapt::<merged_h" code yet (they have "%icap::<last_h"), now is a
>>> good
>>> time for this step.
>>
>> No need for new names IMO.
>> We have <last_h being documented as the specific "last received
>> headers" and <h being a broader "the headers".
>
> Reusing <h sounds good!
>
>> IMO:
>> * Change existing parameterized adapt::<last_h (merging) to be what
>> happens with parameterized adapt::<h
>> * Add new parameterized adapt::<last_h which only considers the
>> last
>> ICAP headers. As per the documented semantics of <last_h.
>> * Implement non-parameterized adapt::<h to output the merged ICAP
>> headers (somehow).
>
> I am not sure I understand the first bullet correctly. I also think
> that
> parameterized cases should just narrow down what is logged and not
> change the semantics of what is logged, so we should not
> consider/document them specially (except when reporting
> implementation
> bugs). Does the end-result summary bellow match your recommendation?
>
> 1. adapt::<h logs all received headers merged into one. If two or
> more
> responses containing header field(s) with the name NAME are received,
> the header field(s) from the last received response containing header
> field named NAME win, and all fields named NAME from the previously
> received responses are discarded. Parameterized cases just narrow
> down
> what is logged from those merged headers.
>
> 2. adapt::<last_h logs the header of the last received response.
> Parameterized cases just narrow down what is logged from that
> response
> header.

 Yes. Exactly.

 I went a little further and looked at how to accomplish it. My bullets
 were kind of a step-by-step todo list ending in that state.

 What I meant was the current implementation of adapt::<{Foo}last_h
 already does what we actually want adapt::<{Foo}h to be doing.
 Just shuffling the logformat tags fixes that up as a first step.
 Followed by implementing the currently missing bits for each tag.

 Amos
Received on Mon Mar 14 2011 - 21:51:54 MDT