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

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.

> + /// 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".

> + 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().

> + 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.

> + /// 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.

> + // total message size
> + uint64_t total() const {return headerSz + payloadDataSz + payloadTeSz;}

Missing third "/" in the comment.

> +/**
> + * 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.

Thank you,

Alex.
Received on Thu Oct 31 2013 - 17:03:37 MDT