Re: [PATCH] : Restore clearing t->logs.bytes

On Mon, 21 Jan 2008, Willy Tarreau wrote:

> Hi Krzysztof,
>
> On Mon, Jan 21, 2008 at 01:58:23AM +0100, Krzysztof Oledzki wrote:
>>> From 228127bcd843d9d9f354eeee6f164941a9d41343 Mon Sep 17 00:00:00 2001
>> From: Krzysztof Piotr Oledzki <ole#ans.pl>
>> Date: Sun, 20 Jan 2008 23:27:02 +0100
>> Subject: [BUG]: Restore clearing t->logs.bytes
>>
>> Commit 8b3977ffe36190e45fb974bf813bfbd935ac99a1 removed "t->logs.bytes_in = 0;"
>> but instead it should change it into "t->logs.bytes_out = 0;" as since
>> 583bc966064e248771e75c253dddec7351afd8a2 counters are incremented not set.
>> We need this to prevent counting headers twice.
>
> Are you really sure ? I thought it was a reminiscent of bytes -> bytes_in
> changes because I failed to find in what case it may be used.

Yes, as it was added by me in the
583bc966064e248771e75c253dddec7351afd8a2, mentioned above. However, as I just tested it, my above description is not right. You can find more correct description below.

> From what I see, it's only used if we want to log early (logasap). In
> this case, it looks like we set bytes_out to the header size to make
> it appear in the logs and then set it back to zero.

Exactly.

> But stupid question is how then you we account for this size ?

Like in the normale case, where there is no logasap enabled. It should be incremented in session_process_counters while sending data to a client:

 	bytes = s->rep->total - s->logs.bytes_out;
 	s->logs.bytes_out = s->rep->total;

However, if we increment (set) s->logs.bytes_out while processing "logasap", statistics get wrong values added for headers: 0 or even negative if haproxy adds some headers itself.

To test it, please enable logasap and download one empty file and look at stats. Without my fix information available on that page are invalid, for example:

# pxname,svname,qcur,qmax,scur,smax,slim,stot,bin,bout,dreq,dresp,ereq,econ,eresp,wretr,wredis,status,weight,act,bck,chkfail,chkdown,lastchg,downtime,qlimit,pid,iid,sid,throttle,lbtot, www,b,0,0,0,1,,1,24,-92,,0,,0,0,0,,UP,1,1,0,0,0,3121,0,,1,2,1,,1, www,BACKEND,0,0,0,1,0,1,24,-92,0,0,,0,0,0,0,UP,1,1,0,,0,3121,0,,1,2,0,,1,

> If it's really needed, then we need to add a comment next to it. The
> first time I saw it, I thought it was really a bug due to the fact
> it is assigned above. In fact, the comment should even be added at
> the first assignment, stating that it is temporary.

Yes, this is a very good idea.

> I'd like your opinion on this.

To be honest, counters need some more work as often, when haproxy adds/removes/changes headers, we need to add different values for servers (data received for a server), and frontends/backends (data sent to a client). It should not make a big difference but it is on my TODO list, however we really need the fix described above.

Best regards,

                                         Krzysztof Olêdzki Received on 2008/01/22 00:38