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

On Tue, Jan 22, 2008 at 12:38:27AM +0100, Krzysztof Oledzki wrote:
> >>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.

OK

> >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,

OK thanks for the test and example.

> >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'll do this.

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

OK fine. I'll merge your fix then, and possibly enhance the description and add a comment.

Thanks,
Willy Received on 2008/01/22 00:19