Re: [PATCH] Enhanced access logging, added ICAP logging, chunked requests, bug #2495 fix.

On 06/28/2009 12:45 AM, Amos Jeffries wrote:

> I note that there are a lot of different fixes going on here. Is it easy
> to pull out these ones for separate merges?
>
> - Bug 2495
> - chunked requests
> - DNS timers and logging
> - ICAP logging changes

Chunked requests are relatively easy to pull.

Bug 2495 can probably be pulled out, but DNS logging stuff is
essentially a part of it because the new DnsLookupDetail class caries
the wait time info which gets updated in the right places, all for
logging. The two were developed together, at the same time. It would
take a relatively long time to fully separate the two but it can be done.

Most logging is too tightly integrated to be separated (and it probably
makes no sense to do that anyway because it should be reviewed and used
together).

Please note that if this patch is approved, separately developed
features will be merged (by bzr) separately. My understanding is that
bzr will replay the original commit sequence so we will have separate
commit messages for separate (but dependent) features.

With the bzr magic caveat in mind, do you want me to spend time on
separating chunked requests hack and possibly bug 2495 fix?

> I'm taking a closer look at this one in light of the logging alterations
> that I have coming up.
>
>
> Your comments suggest ICAP/eCAP are going to be done outside access.log
> so the bits for them do not belong in AccessLogEntry. I know ALE is a
> mix of uses already, but no need to add even more garbage there before
> we fix that bug. Is it easy to make IcapLogEntry a main object in its
> own files, possibly inheriting from AccessLogEntry if you need the ALE
> fields in the icap.log? If not we shall have to do it after the fact in
> the logging updates, but I would rather have it clean beforehand.

There are two groups of logging features, some go into access.log and
some go into icap.log. Moreover, icap.log can log almost everything
access.log can (but in a different scope).

I doubt I can pull out IcapLogEntry and make it an ALE kid without
redesigning old ALE-dependent code that assumes everything is in ALE. I
think somebody already looked at that and the conclusion was that either
we have to redesign the whole logging mess or keep adding to it. I
wanted to save resources for more important stuff so I said "add to the
mess". This is also related to the namespace question below.

Please note that ALE is currently organized as a collection of
scope-specific subclasses. The \todo I added was specifically to
emphasize that I know that this is the wrong style, but I am not going
to change it for now. Note the plural in "Inner class declarations" :-)

> When splitting an existing code (Hs in this case) for inbound and
> outbound. Please update them to use both directional < and > format
> codes.

OK.

> The non-directional then needs to default to the old behavior
> with a configu parse WARNING: about the change.
>
> We appear to be fallign into a predictable pattern for LFT. May as well
> keep that up...

Are you sure about the warnings? Every squid.conf that used a custom (or
copied default) format will generate these warnings. I think this may
annoy too many folks. I am not against adding support for both "<*" and
"*" versions if you think I must, but I wonder if warnings should wait
until most folks switch or start using to the documented/recommended fields?

> These don't fit however. What is the difference now?
>
> + LFT_SENT_HTTP_CODE,
> + LFT_RECEIVED_HTTP_CODE,

One is sent by Squid to the client. The other one is received by Squid
from the server. Or are you asking about some other difference?

>
> These are ICAP headers right? but not with names 'Last-Matched-Icap:'
> etc.? So can you make the ICAP ones all LFT_ICAP_*
> - LFT_LAST_MATCHED_ICAP_HEADER,
> - LFT_LAST_MATCHED_ICAP_HEADER_ELEM,
> - LFT_LAST_MATCHED_ICAP_ALL_HEADERS,
> + LFT_ICAP_LAST_MATCHED_HEADER,
> + LFT_ICAP_LAST_MATCHED_HEADER_ELEM,
> + LFT_ICAP_LAST_MATCHED_ALL_HEADERS,

There are ICAP headers logged to access.log. I think this is why they do
not start with LFT_ICAP_. I am happy to change the enums if you do not
think it would be better the other way around.

I am not sure what you mean by "with names 'Last-Matched-Icap:" but
these will log actual ICAP header fields. There could be many header
fields with the same name across multiple ICAP transactions (e.g., in a
chain) so we use the "last matched" trick.

> Is the tag form: %adaptation_ and %icap_* set in stone? There was a very
> nice 'namespacing' proposition a while back which never seems to appear.
> Using the ':' separator syntax like so --> %icap::<field>, or
> %adapt::icap::<field>
> The HTTP tags are all reserved char-codes. So if we are adding a new tag
> format on top it might as well be a nice extensible one.

AFAIK, the person who was going to implement
http://wiki.squid-cache.org/Features/AclNamespaces
is no longer working on Squid. I still stand behind that proposal in
principle, but will not have the time to implement it myself in the
foreseeable future :-(.

Also, do we support line \
   continuation in squid.conf? We are approaching the limit of \
   what can be managed on one line and namespaces will only make \
   things worse

Thank you,

Alex.
Received on Sun Jun 28 2009 - 07:46:29 MDT