[squid-dev] [PATCH] keep track of the size of uploads done with CONNECT

[Alex Rousskov]

On 09/14/2015 02:22 PM, Aymeric Vincent wrote:
> Alex Rousskov <rousskov at measurement-factory.com> writes:
>> You are adding a new ClientHttpRequest in_size field dedicated to
>> client-sent "payload" sizes in tunnels, but Squid seems to be using
>> AccessLogEntry's http.clientRequestSz.payloadData for similar purposes
>> in non-tunnel areas of the code. Would it be possible to teach tunnel.cc
>> code to update http.clientRequestSz.payloadData (directly or indirectly)
>> instead of adding a new field?


> Here is a quick patch doing what you suggest. However:
> 
> - it doesn't match what happens in client_side.cc where log data gets
>   filled in after the fact (see most notably req_sz field) but this may
>   need fixing too after all

I agree that the after-the-fact setting vs. runtime updates difference
is unfortunate. The suggestions below eliminate that difference (but
since many AccessLogEntry (ALE) fields are set and updated throughout
the transaction lifetime so we were not setting a new bad precedent here).

Overall, take2 is slightly better, but the _pointer_ cast is indeed ugly
(not to mention the need to maintain these pointers). I would recommend
the following changes:

1a. Since you are polishing tunnelStart() profile anyway, delete the
status_ptr parameter as well. AFAICT, that parameter is as unneeded as
the size_ptr parameter you have already deleted. Use al->http.code
inside tunnelStart() instead. This will leave you with two parameters:
ClientHttpRequest and AccessLogEntry pointers.

1b. Use (1a) logic above to delete the AccessLogEntry::Pointer parameter
as well. Initialize TunnelStateData::al with http->al.

2. Convert TunnelStateData::Connection::size_ptr into an "uint64_t
bytesSent" or a similar data member (not a pointer). It will still be
updated by TunnelStateData::Connection::dataSent().

3. In TunnelStateData destructor, update ALE "al" as needed, using
client.bytesSent and server.bytesSent values. Test that you have guessed
which one is which correctly -- they are confusing.


> - it currently requires a cast because tunnelStart() expects (signed)
>   int64_t's whereas payloadData is (unsigned) uint64_t, and switching to
>   uint64_t everywhere requires an audit because out.size gets compared
>   to some other signed quantity (expectedLength) in at least one place.
>   This pulls strings in many places where int64_t's are used for size
>   quantities.
> 
>   The cast is harmless because this integer gets only added to, but it's
>   still a cast
> 
> If you are not satisfied with any of the two patches, please tell me
> which direction you prefer and I can work on a more extensive patch
> tomorrow. IMHO the advantage of the first patch is its blending with the
> current code, while other nicer solutions will require changing a few
> types here and there, which could be done as a second, wider, cleaning
> step.

The problem with the first take is that it adds a data field that is
dedicated to the tunnel code but does not live in the tunnel class. It
moves an already ugly code a tiny step in the same wrong direction by
adding yet another hard-to-track ClientHttpRequest:TunnelStateData
dependency. ALE is supposed to hold statistics for logging. It is
guaranteed to exist. The tunnel code should just update it.

Your patch, even after a few takes, will not solve all the upload size
logging problems, of course, but I think that encapsulating tunnel
upload size accounting code in tunnel.cc is the right step. Eventually,
similar changes may be done to the non-tunneling code, completely
removing upload size updates from prepareLogWithRequestDetails().


Thank you,

Alex.