Re: /bzr/squid3/trunk/ r13074: Cleanup transaction message size logging

On 1/11/2013 6:03 a.m., Alex Rousskov wrote:
> On 10/28/2013 08:24 PM, Amos Jeffries wrote:
>
>> + /// counters for the response sent to client
>> + MessageCounters adaptedReply;
> Please clarify by picking one: "response sent to client" or [received]
> "adapted reply". The two are usually not the same because Squid adds its
> own headers to the "adapted reply". For example, the "adapted reply" may
> be a 304 response without a body received from an adaptation service
> while the "response sent to the client" for the same transaction is a
> 200 OK response with a body that never went through any adaptation service.

You have a need to account for bytes received from adapters?
I'm accepting this and going with "clientReply" variable renaming since
the documentation states the intention. I was conceiving the adapted I/O
bytes would be under icap.* AccessLogEntry sub-class and this would be
the fully adapted reply/request after all ICAP. URL-rewrite and Squid
internal adaptations were done.

>
>> + /// total size of extra bytes added by transfer encoding
>> + uint64_t payloadTeSz;
>> + /// total payload size including transfer encoding overheads
>> + uint64_t payloadTotal() const {return payloadDataSz + payloadTeSz;}
> Please remove as unused (and of questionable potential/direction in its
> current implementation). If removed, payloadDataSz should be renamed to
> "payload".

Not unused for long. We have log codes explicitly documented as
with/without chunked encoding bytes and I have followup patches underway
doing logical changes that calculate all these metrics individually. The
sponsoring client use-case is for very specific byte level accounting of
everything, and backward compatibility calls for the more vague
payloadData only output.

>
>> + uint64_t headerSz;
>> + uint64_t payloadDataSz;
>> + uint64_t payloadTeSz;
> The "Sz" suffix should be dropped since the whole class is about sizes
> (per class description and implementation). If you disagree, then the
> total() method should be renamed to totalSz().

Done.

>
>> + MessageCounters clientRequest;
>> + MessageCounters adaptedReply;
> While these should probably have a suffix like Sz or Sizes because the
> are other stats and information related to client request and adapted
> reply in the ALE, these two do not encapsulate all of that info.

Done.

>
>> + /// size of message header block (if any)
>> + uint64_t headerSz;
> You may want to mention that this includes Start-Line (Status-Line or
> Request-Line). Technically, RFC 2616 does not consider Start-Line a part
> of a message headers.

Done.

>
>> + // total message size
>> + uint64_t total() const {return headerSz + payloadDataSz + payloadTeSz;}
> Missing third "/" in the comment.
Fixed.

>
>> +/**
>> + * Counters used to collate the traffic size measurements
>> + * for a transaction message.
>> + */
>> +class MessageCounters
> Based on the description and implementation, it would be better to call
> this class MessageSizes, but it is probably too late for that.

Okay. Yes that makes sense. Done.

Anything else, or can I go ahead and commit these updated to trunk
befroe I go onto the measurement patches?

Amos
Received on Mon Nov 04 2013 - 03:27:48 MST