Re: [PATCH] log virgin HTTP request headers
On 01/29/2010 05:04 AM, Tsantilas Christos wrote: >> On Thu, 2010-01-28 at 09:09 -0700, Alex Rousskov wrote: >>> On 01/28/2010 06:07 AM, Robert Collins wrote: Just a small thing: can I suggest s/virgin/pristine/ ? Or s/virgin/received/ ? >>> Pristine may work, but we (and other adaptation related documents) use >>> virgin in many places already, including APIs. >>> >>> Received is a bad idea because an adapted message is also received. >>> virgin has a sexual connotation in some cultures, and can be confusing in a way that is avoidable. >>> Tell that to the Virgin Islands folks. :-) >> It was a passing thought, I'm not like 'omg must be done' for this: I >> certainly knew what it meant, but as its (AFAIK) the first config option >> in squid to specify virgins, I see potential for confusion :) Other >> possibilities: >> - source >> - external >> - unaltered >> - original > > Yep, I think we should use "original" for documentation. FWIW, this naming issue was discussed when IETF OPES working group was working on the ICAP replacement protocol. We started with the "original" term. Some reviewers of the protocol specs did not like "original" for several reasons. There were several replacement suggestions, including virgin. At the end, RFC 4037 still said "original" (possibly because I was too lazy to change the text!). Thus, if we use "original", we will be at least consistent with OPES terminology... I am fine with replacing virgin with original if others think it is a good idea. Just do not tell Virgin Airlines or the virgin olive oil guys that they are using the wrong word :-). Cheers, Alex.
Re: [PATCH] log virgin HTTP request headers
> On Thu, 2010-01-28 at 09:09 -0700, Alex Rousskov wrote: >> On 01/28/2010 06:07 AM, Robert Collins wrote: >> > Just a small thing: can I suggest s/virgin/pristine/ ? Or >> > s/virgin/received/ ? >> >> Pristine may work, but we (and other adaptation related documents) use >> virgin in many places already, including APIs. >> >> Received is a bad idea because an adapted message is also received. >> >> > virgin has a sexual connotation in some cultures, and can be confusing >> > in a way that is avoidable. >> >> Tell that to the Virgin Islands folks. :-) > > It was a passing thought, I'm not like 'omg must be done' for this: I > certainly knew what it meant, but as its (AFAIK) the first config option > in squid to specify virgins, I see potential for confusion :) Other > possibilities: > - source > - external > - unaltered > - original Yep, I think we should use "original" for documentation. But for code requires many changes in many places... > > I don't know that the config option has to meet the code, though > obviously it is nice if it does. > > -Rob >
Re: [PATCH] log virgin HTTP request headers
Robert Collins wrote: On Thu, 2010-01-28 at 09:09 -0700, Alex Rousskov wrote: On 01/28/2010 06:07 AM, Robert Collins wrote: Just a small thing: can I suggest s/virgin/pristine/ ? Or s/virgin/received/ ? Pristine may work, but we (and other adaptation related documents) use virgin in many places already, including APIs. Received is a bad idea because an adapted message is also received. virgin has a sexual connotation in some cultures, and can be confusing in a way that is avoidable. Tell that to the Virgin Islands folks. :-) It was a passing thought, I'm not like 'omg must be done' for this: I certainly knew what it meant, but as its (AFAIK) the first config option in squid to specify virgins, I see potential for confusion :) Other possibilities: - source - external - unaltered - original I don't know that the config option has to meet the code, though obviously it is nice if it does. -Rob I've been of a similar "is it a good idea" bent since I started seeing these in the adaptation code. At least thats hidden away from regular view. Now that Rob brings it up this could cause some people problems in our middle-eastern and christian user bases. I recall something about the word being a big issue for some large international organisations. "original" sounds good for the docs/squid.conf side of things. Code: no problems. Amos -- Please be using Current Stable Squid 2.7.STABLE7 or 3.0.STABLE21 Current Beta Squid 3.1.0.15
Re: [PATCH] log virgin HTTP request headers
On Thu, 2010-01-28 at 09:09 -0700, Alex Rousskov wrote: > On 01/28/2010 06:07 AM, Robert Collins wrote: > > Just a small thing: can I suggest s/virgin/pristine/ ? Or > > s/virgin/received/ ? > > Pristine may work, but we (and other adaptation related documents) use > virgin in many places already, including APIs. > > Received is a bad idea because an adapted message is also received. > > > virgin has a sexual connotation in some cultures, and can be confusing > > in a way that is avoidable. > > Tell that to the Virgin Islands folks. :-) It was a passing thought, I'm not like 'omg must be done' for this: I certainly knew what it meant, but as its (AFAIK) the first config option in squid to specify virgins, I see potential for confusion :) Other possibilities: - source - external - unaltered - original I don't know that the config option has to meet the code, though obviously it is nice if it does. -Rob signature.asc Description: This is a digitally signed message part
Re: [PATCH] log virgin HTTP request headers
On 01/28/2010 06:07 AM, Robert Collins wrote: > Just a small thing: can I suggest s/virgin/pristine/ ? Or > s/virgin/received/ ? Pristine may work, but we (and other adaptation related documents) use virgin in many places already, including APIs. Received is a bad idea because an adapted message is also received. > virgin has a sexual connotation in some cultures, and can be confusing > in a way that is avoidable. Tell that to the Virgin Islands folks. :-) Cheers, Alex.
Re: [PATCH] log virgin HTTP request headers
> Just a small thing: can I suggest s/virgin/pristine/ ? Or > s/virgin/received/ ? > > virgin has a sexual connotation in some cultures, and can be confusing > in a way that is avoidable. The "virgin" is a term which used extensively in squid3 specially in adaptation code. But id required I can make this change... Christos > > -Rob >
Re: [PATCH] log virgin HTTP request headers
Just a small thing: can I suggest s/virgin/pristine/ ? Or s/virgin/received/ ? virgin has a sexual connotation in some cultures, and can be confusing in a way that is avoidable. -Rob signature.asc Description: This is a digitally signed message part
Re: [PATCH] log virgin HTTP request headers
Alex Rousskov wrote:
> ... //< HTTP request headers after adaptation and redirection
There are some magic words here "adaptation and redirection"
OK I did not count the redirection.
So I am posting a new patch which, I hope, satisfy all Amos and Alex
requirements:
- http::>h always logs the virgin request headers (before
adaptation/redirections)
- http::>ha logs the request headers after adaptation and redirection.
- http::>ha is always enabled (even if adaptation is off)
Alex Rousskov wrote:
..
* Should we add "virgin" to >h definition in src/cf.data.pre?
Yep
* I would polish src/cf.data.pre text a little:
[http::]>ha The adapted HTTP request headers.
Optional header name argument as for >h
OK.
* In the first hunk of src/client_side.cc, I would use a single "#if
USE_ADAPTATION" block to set both aLogEntry->headers.adapted_request and
aLogEntry->headers.request instead of testing USE_ADAPTATION twice. If
this is not possible for some reason, ignore me.
There is no USE_ADAPTATION block now
* Please add comments to document the new adapted_request fields in the
header files. Something like
... //< HTTP request headers after adaptation and redirection
may work well.
OK I add some comments
Please commit when you are comfortable with the code.
Because I change again the patch I will wait today and if there are no
any objections I will commit it tomorrow.
Regards,
Christos
Thank you,
Alex.
=== modified file 'src/AccessLogEntry.h'
--- src/AccessLogEntry.h 2009-12-22 01:12:53 +
+++ src/AccessLogEntry.h 2010-01-27 21:36:21 +
@@ -47,7 +47,9 @@
{
public:
-AccessLogEntry() : url(NULL) , reply(NULL), request(NULL) {}
+AccessLogEntry() : url(NULL) , reply(NULL), request(NULL),
+ adapted_request(NULL)
+{}
const char *url;
@@ -134,12 +136,17 @@
public:
Headers() : request(NULL),
+ adapted_request(NULL),
+
#if ICAP_CLIENT
icap(NULL),
#endif
reply(NULL) {}
-char *request;
+char *request; //< virgin HTTP request headers
+
+ char *adapted_request; //< HTTP request headers after adaptation and redirection
+
#if ICAP_CLIENT
char * icap;///< last matching ICAP response header.
@@ -159,7 +166,9 @@
} _private;
HierarchyLogEntry hier;
HttpReply *reply;
-HttpRequest *request;
+HttpRequest *request; //< virgin HTTP request
+HttpRequest *adapted_request; //< HTTP request after adaptation and redirection
+
#if ICAP_CLIENT
/** \brief This subclass holds log info for ICAP part of request
=== modified file 'src/cf.data.pre'
--- src/cf.data.pre 2010-01-21 12:48:36 +
+++ src/cf.data.pre 2010-01-27 21:41:41 +
@@ -2495,8 +2495,10 @@
HTTP cache related format codes:
- [http::]>h Request header. Optional header name argument
+ [http::]>h Virgin request header. Optional header name argument
on the format header[:[separator]element]
+ [http::]>ha The HTTP request headers after adaptation and redirection.
+Optional header name argument as for >h
[http::]h
[http::]un User name
=== modified file 'src/client_side.cc'
--- src/client_side.cc 2010-01-13 01:13:17 +
+++ src/client_side.cc 2010-01-27 21:00:28 +
@@ -461,7 +461,17 @@
mb.init();
packerToMemInit(&p, &mb);
request->header.packInto(&p);
-aLogEntry->headers.request = xstrdup(mb.buf);
+ //This is the request after adaptation or redirection
+ aLogEntry->headers.adapted_request = xstrdup(mb.buf);
+
+ // the virgin request is saved to aLogEntry->request
+ if (aLogEntry->request) {
+ packerClean(&p);
+ mb.reset();
+ packerToMemInit(&p, &mb);
+ aLogEntry->request->header.packInto(&p);
+ aLogEntry->headers.request = xstrdup(mb.buf);
+ }
#if ICAP_CLIENT
packerClean(&p);
@@ -559,7 +569,7 @@
if (!Config.accessList.log || checklist->fastCheck()) {
if (request)
-al.request = HTTPMSGLOCK(request);
+al.adapted_request = HTTPMSGLOCK(request);
accessLogLog(&al, checklist);
updateCounters();
=== modified file 'src/client_side_request.cc'
--- src/client_side_request.cc 2010-01-22 01:13:11 +
+++ src/client_side_request.cc 2010-01-27 21:02:01 +
@@ -1256,6 +1256,10 @@
{
assert(calloutContext);
+/*Save the original request for logging purposes*/
+if (!calloutContext->http->al.request)
+ calloutContext->http->al.request = HTTPMSGLOCK(request);
+
if (!calloutContext->http_access_done) {
debugs(83, 3, HERE << "Doing calloutContext->clientAccessCheck()");
calloutContext->http_access_done = true;
=== modified file 'src/log/access_log.cc'
--- src/log/access_log.cc 2009-12-22 01:12:53 +
+++ src/log/access_log.cc 2010-01-27 21:09:18 +
@@ -359,6 +359,10 @@
LFT_REQUEST_HEADER_ELEM,
LFT_REQUEST_ALL_HEADERS,
+LFT_ADAPTED_REQUEST_HEADER,
+
Re: [PATCH] log virgin HTTP request headers
On 01/24/2010 08:20 PM, Amos Jeffries wrote: > Tsantilas Christos wrote: >> Hi all, >> First of all sorry for late answer. >> I am sending a second version of the patch. >> The new patch uses the format code "http::>ha" to log the adapted >> headers. The already existing format code http::>h logs the original >> request headers. >> This patch still does not handle the use of the request_header_access. >> Please read below. >> >> >> Amos Jeffries 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 >> >> OK Amos this patch does it. >> >>> adaptation or headers_access doing the alteration. >> >> The request_header_access does not modify the request send by the >> client but has to do with the http request will be send to the server >> by squid. >> Also currently does not have any effect to request headers logging. >> >> The request_header_access implemented inside http.cc file. If we want >> to consider it as request adaptation mechanism maybe we should move it >> to client_side* code. >> >> Regards, >> Christos >> >>> >>> Likewise on replies. >>> >>> Amos > > Thats fine. +1 from me. * Should we add "virgin" to >h definition in src/cf.data.pre? * I would polish src/cf.data.pre text a little: [http::]>ha The adapted HTTP request headers. Optional header name argument as for >h * In the first hunk of src/client_side.cc, I would use a single "#if USE_ADAPTATION" block to set both aLogEntry->headers.adapted_request and aLogEntry->headers.request instead of testing USE_ADAPTATION twice. If this is not possible for some reason, ignore me. * Please add comments to document the new adapted_request fields in the header files. Something like ... //< HTTP request headers after adaptation and redirection may work well. Please commit when you are comfortable with the code. Thank you, Alex.
Re: [PATCH] log virgin HTTP request headers
Tsantilas Christos wrote: Hi all, First of all sorry for late answer. I am sending a second version of the patch. The new patch uses the format code "http::>ha" to log the adapted headers. The already existing format code http::>h logs the original request headers. This patch still does not handle the use of the request_header_access. Please read below. Amos Jeffries 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 OK Amos this patch does it. adaptation or headers_access doing the alteration. The request_header_access does not modify the request send by the client but has to do with the http request will be send to the server by squid. Also currently does not have any effect to request headers logging. The request_header_access implemented inside http.cc file. If we want to consider it as request adaptation mechanism maybe we should move it to client_side* code. Regards, Christos Likewise on replies. Amos Thats fine. +1 from me. Amos -- Please be using Current Stable Squid 2.7.STABLE7 or 3.0.STABLE21 Current Beta Squid 3.1.0.15
Re: [PATCH] log virgin HTTP request headers
Hi all,
First of all sorry for late answer.
I am sending a second version of the patch.
The new patch uses the format code "http::>ha" to log the adapted
headers. The already existing format code http::>h logs the original
request headers.
This patch still does not handle the use of the request_header_access.
Please read below.
Amos Jeffries 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
OK Amos this patch does it.
adaptation or headers_access doing the alteration.
The request_header_access does not modify the request send by the client
but has to do with the http request will be send to the server by squid.
Also currently does not have any effect to request headers logging.
The request_header_access implemented inside http.cc file. If we want
to consider it as request adaptation mechanism maybe we should move it
to client_side* code.
Regards,
Christos
Likewise on replies.
Amos
=== modified file 'src/AccessLogEntry.h'
--- src/AccessLogEntry.h 2009-12-22 01:12:53 +
+++ src/AccessLogEntry.h 2010-01-24 18:45:24 +
@@ -47,7 +47,11 @@
{
public:
-AccessLogEntry() : url(NULL) , reply(NULL), request(NULL) {}
+AccessLogEntry() : url(NULL) , reply(NULL), request(NULL),
+#if USE_ADAPTATION
+ adapted_request(NULL)
+#endif
+{}
const char *url;
@@ -134,6 +138,10 @@
public:
Headers() : request(NULL),
+#if USE_ADAPTATION
+ adapted_request(NULL),
+#endif
+
#if ICAP_CLIENT
icap(NULL),
#endif
@@ -141,6 +149,11 @@
char *request;
+#if USE_ADAPTATION
+ char *adapted_request;
+#endif
+
+
#if ICAP_CLIENT
char * icap;///< last matching ICAP response header.
#endif
@@ -160,6 +173,10 @@
HierarchyLogEntry hier;
HttpReply *reply;
HttpRequest *request;
+#if USE_ADAPTATION
+HttpRequest *adapted_request;
+#endif
+
#if ICAP_CLIENT
/** \brief This subclass holds log info for ICAP part of request
=== modified file 'src/cf.data.pre'
--- src/cf.data.pre 2010-01-21 12:48:36 +
+++ src/cf.data.pre 2010-01-24 21:53:21 +
@@ -2547,7 +2547,7 @@
when multiple ICAP transactions per HTTP
transaction are supported.
- If adaptation is enabled the following two codes become available:
+ If adaptation is enabled the following codes become available:
adapt::sum_trs Summed adaptation transaction response
times recorded as a comma-separated list in
@@ -2567,6 +2567,10 @@
together. Instead, all transaction response
times are recorded individually.
+ http::>ha The adapted HTTP request headers.
+Optional header name argument on the format
+header[:[separator]element]
+
You can prefix adapt::*_trs format codes with adaptation
service name in curly braces to record response time(s) specific
to that service. For example: %{my_service}adapt::sum_trs
=== modified file 'src/client_side.cc'
--- src/client_side.cc 2010-01-13 01:13:17 +
+++ src/client_side.cc 2010-01-24 21:47:08 +
@@ -461,7 +461,23 @@
mb.init();
packerToMemInit(&p, &mb);
request->header.packInto(&p);
+#if USE_ADAPTATION
+ //if Adaptation this is the adapted request
+ aLogEntry->headers.adapted_request = xstrdup(mb.buf);
+#else
aLogEntry->headers.request = xstrdup(mb.buf);
+#endif
+
+#if USE_ADAPTATION
+ // if Adaptation then the virgin request is saved to aLogEntry->request
+ if (aLogEntry->request) {
+ packerClean(&p);
+ mb.reset();
+ packerToMemInit(&p, &mb);
+ aLogEntry->request->header.packInto(&p);
+ aLogEntry->headers.request = xstrdup(mb.buf);
+ }
+#endif
#if ICAP_CLIENT
packerClean(&p);
@@ -559,7 +575,11 @@
if (!Config.accessList.log || checklist->fastCheck()) {
if (request)
+#if USE_ADAPTATION
+al.adapted_request = HTTPMSGLOCK(request);
+#else
al.request = HTTPMSGLOCK(request);
+#endif
accessLogLog(&al, checklist);
updateCounters();
=== modified file 'src/client_side_request.cc'
--- src/client_side_request.cc 2010-01-22 01:13:11 +
+++ src/client_side_request.cc 2010-01-24 18:37:20 +
@@ -1266,6 +1266,9 @@
#if USE_ADAPTATION
if (!calloutContext->adaptation_acl_check_done) {
calloutContext->adaptation_acl_check_done = true;
+/*Save the original request for logging purposes*/
+calloutContext->http->al.request = HTTPMSGLOCK(request);
+
if (Adaptation::AccessCheck::Start(
Re: [PATCH] log virgin HTTP request headers
On Tue, 19 Jan 2010 17:48:20 -0700, Alex Rousskov wrote: > On 01/14/2010 03:28 PM, Tsantilas Christos wrote: >> 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? > > Can we clone once, before header_access and before adaptations? That > cloned pristine value is what should be logged with one of the >h* > options. The header value used for the final request to the server > should be used for the other >h* option. > > Personally, I do not care which >h* option is the default. All we need > to do is to provide a way to log from-the-client and to-the-server HTTP > request headers. > > Thank you, > > Alex. I don't see why not. Christos: It seems to me the place to add a clone is client_side_request.cc line 1263, between http_access testing and the first pre-cache adaptation. IIRC, the clone mechanism used to create the Squid->Server request is where header_replace is done. Much later in the processing. Amos
Re: [PATCH] log virgin HTTP request headers
On 01/14/2010 03:28 PM, Tsantilas Christos wrote: > 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? Can we clone once, before header_access and before adaptations? That cloned pristine value is what should be logged with one of the >h* options. The header value used for the final request to the server should be used for the other >h* option. Personally, I do not care which >h* option is the default. All we need to do is to provide a way to log from-the-client and to-the-server HTTP request headers. Thank you, Alex.
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
Re: [PATCH] log virgin HTTP request headers
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. 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. Amos -- Please be using Current Stable Squid 2.7.STABLE7 or 3.0.STABLE21 Current Beta Squid 3.1.0.15
Re: [PATCH] log virgin HTTP request headers
> 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 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? > > Likewise on replies. > > Amos > -- > Please be using >Current Stable Squid 2.7.STABLE7 or 3.0.STABLE21 >Current Beta Squid 3.1.0.15 >
Re: [PATCH] log virgin HTTP request headers
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. Likewise on replies. Amos -- Please be using Current Stable Squid 2.7.STABLE7 or 3.0.STABLE21 Current Beta Squid 3.1.0.15
[PATCH] log virgin HTTP request headers
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
=== modified file 'src/AccessLogEntry.h'
--- src/AccessLogEntry.h 2009-12-22 01:12:53 +
+++ src/AccessLogEntry.h 2010-01-12 18:54:44 +
@@ -47,7 +47,11 @@
{
public:
-AccessLogEntry() : url(NULL) , reply(NULL), request(NULL) {}
+AccessLogEntry() : url(NULL) , reply(NULL), request(NULL),
+#if USE_ADAPTATION
+ virgin_request(NULL)
+#endif
+{}
const char *url;
@@ -134,6 +138,10 @@
public:
Headers() : request(NULL),
+#if USE_ADAPTATION
+ virgin_request(NULL),
+#endif
+
#if ICAP_CLIENT
icap(NULL),
#endif
@@ -141,6 +149,11 @@
char *request;
+#if USE_ADAPTATION
+ char *virgin_request;
+#endif
+
+
#if ICAP_CLIENT
char * icap;///< last matching ICAP response header.
#endif
@@ -160,6 +173,10 @@
HierarchyLogEntry hier;
HttpReply *reply;
HttpRequest *request;
+#if USE_ADAPTATION
+HttpRequest *virgin_request;
+#endif
+
#if ICAP_CLIENT
/** \brief This subclass holds log info for ICAP part of request
=== modified file 'src/cf.data.pre'
--- src/cf.data.pre 2010-01-02 04:32:46 +
+++ src/cf.data.pre 2010-01-12 18:55:43 +
@@ -2547,7 +2547,7 @@
when multiple ICAP transactions per HTTP
transaction are supported.
- If adaptation is enabled the following two codes become available:
+ If adaptation is enabled the following codes become available:
adapt::sum_trs Summed adaptation transaction response
times recorded as a comma-separated list in
@@ -2567,6 +2567,10 @@
together. Instead, all transaction response
times are recorded individually.
+ http::>hv The original (non adapted) HTTP request header.
+Optional header name argument on the format
+header[:[separator]element]
+
You can prefix adapt::*_trs format codes with adaptation
service name in curly braces to record response time(s) specific
to that service. For example: %{my_service}adapt::sum_trs
=== modified file 'src/client_side.cc'
--- src/client_side.cc 2010-01-01 21:26:45 +
+++ src/client_side.cc 2010-01-12 18:54:44 +
@@ -463,6 +463,16 @@
request->header.packInto(&p);
aLogEntry->headers.request = xstrdup(mb.buf);
+#if USE_ADAPTATION
+ if (aLogEntry->virgin_request) {
+ packerClean(&p);
+ mb.reset();
+ packerToMemInit(&p, &mb);
+ aLogEntry->virgin_request->header.packInto(&p);
+ aLogEntry->headers.virgin_request = xstrdup(mb.buf);
+ }
+#endif
+
#if ICAP_CLIENT
packerClean(&p);
mb.reset();
=== modified file 'src/client_side_request.cc'
--- src/client_side_request.cc 2009-12-11 23:37:30 +
+++ src/client_side_request.cc 2010-01-12 18:54:44 +
@@ -1264,6 +1264,9 @@
#if USE_ADAPTATION
if (!calloutContext->adaptation_acl_check_done) {
calloutContext->adaptation_acl_check_done = true;
+/*Save the original request for logging purposes*/
+calloutContext->http->al.virgin_request = HTTPMSGLOCK(request);
+
if (Adaptation::AccessCheck::Start(
Adaptation::methodReqmod, Adaptation::pointPreCache,
request, NULL, adaptationAclCheckDoneWrapper, calloutContext))
=== modified file 'src/log/access_log.cc'
--- src/log/access_log.cc 2009-12-22 01:12:53 +
+++ src/log/access_log.cc 2010-01-12 18:55:13 +
@@ -359,6 +359,12 @@
LFT_REQUEST_HEADER_ELEM,
LFT_REQUEST_ALL_HEADERS,
+#if USE_ADAPTATION
+LFT_VIRGIN_REQUEST_HEADER,
+LFT_VIRGIN_REQUEST_HEADER_ELEM,
+LFT_VIRGIN_REQUEST_ALL_HEADERS,
+#endif
+
LFT_REPLY_HEADER,
LFT_REPLY_HEADER_ELEM,
LFT_REPLY_ALL_HEADERS,
@@ -512,6 +518,8 @@
{"hv", LFT_VIRGIN_REQUEST_HEADER},
+{">hv", LFT_VIRGIN_REQUEST_ALL_HEADERS},
{">h", LFT_REQUEST_HEADER},
{">h", LFT_REQUEST_ALL_HEADERS},
{"virgin_request)
+sb = al->virgin_request->header.getByName(fmt->data.header.header);
+
+out = sb.termedBuf();
+
+quote = 1;
+
+break;
+#endif
+
case LFT_REPLY_HEADER:
if (al->reply)
sb = al->reply->header.getByName(fmt->data.header.header);
@@ -954,6 +975,18 @@
break;
+#if USE_ADAPTATION
+case LFT_VIRGIN_REQUEST_HEADER_ELEM:
+if (al->virgin_request)
+sb = al->virgin_request->header.getByNameListMember(fmt->data.header.header, fmt->data.header.element, fmt->data.header.separator);
+
+out = sb.termedBuf();
+
+quote = 1;
+
+break;
+#endif
+
case LFT_REPLY_HEADER_ELEM:
if (al->reply)
sb = al->reply->header.getByNameListMember(fmt->data.header.header, fmt
