Re: RFC: continous counters (fwd)

Hi Krzysztof,

On Sun, Nov 04, 2007 at 08:09:39PM +0100, Krzysztof Oledzki wrote:
> Hello,
>
> Currently counters get increased when session finishes. It works quite well
> when serving small objects, but with big ones (for example large images or
> archives) or with A/V streaming, a grap generated from haproxy counters
> looks like a hedgehog. ;)

Yes, I've already observed that too. Interestingly, this only happens with moderate loads (few users with large objects). Because as soon as you have thousands of users, you reach a point where your stats become smooth.

> This RFC-quality patch implements a contcnt (continous counters) option,
> when set makes counters incremented continuously during the whole session.
> As recounting touches a hotpath directly I decided to make it optional, per
> a frontend.

I agree with not making it mandatory, for the same reason. I thought this should become a stats option (after all, its only purpose is only to get continuous stats). But the fact is that stats are often reported by different proxies than the ones you want to monitor.

However, there's something which puzzles me. I think you should not update the stats as long as the backend has not been assigned or the session closed (flags & BE_ASSIGNED I believe). The reason is that with current patch, the backend will never be updated for the size of the request headers.

> BTW: It think that PR_O_* defines seems to be in the wrong place. We should
> move everyting from types/backend.h to types/proxy.h, shouldn't we?

We could. The defines are the only remains of what has moved from backend.h to proxy.h. What I wanted first was to separate proxy options between frontend-specific and backend-specific. It would still make sense given that we use nearly all bits now. But this is still a big change.

BTW, I think that your option should recall the "stats" keyword rather than "counters". Maybe "contstats" or something like this ? The more options we have, the harder it is to remind them all, and to remember how to write them. That's why I'd prefer avoiding something like "cnt" for "counters".

Thanks,
Willy

> diff -Nur haproxy-1.3.13-orig/include/proto/session.h
> haproxy-1.3.13-contiguous-cunters/include/proto/session.h
> --- haproxy-1.3.13-orig/include/proto/session.h 2007-10-18
> 22:38:22.000000000 +0200
> +++ haproxy-1.3.13-contiguous-cunters/include/proto/session.h 2007-11-04
> 16:46:49.000000000 +0100
> @@ -33,6 +33,8 @@
> /* perform minimal intializations, report 0 in case of error, 1 if OK. */
> int init_session();
>
> +void session_process_counters(struct session *s);
> +
> #endif /* _PROTO_SESSION_H */
>
> /*
> diff -Nur haproxy-1.3.13-orig/include/types/backend.h
> haproxy-1.3.13-contiguous-cunters/include/types/backend.h
> --- haproxy-1.3.13-orig/include/types/backend.h 2007-10-18
> 22:38:22.000000000 +0200
> +++ haproxy-1.3.13-contiguous-cunters/include/types/backend.h 2007-11-04
> 16:35:56.000000000 +0100
> @@ -61,6 +61,7 @@
> #define PR_O_BALANCE (PR_O_BALANCE_RR | PR_O_BALANCE_SH |
> PR_O_BALANCE_UH)
> #define PR_O_SMTP_CHK 0x20000000 /* use SMTP EHLO check for server
> health - pvandijk#vision6.com.au */
> #define PR_O_TCP_NOLING 0x40000000 /* disable lingering on client and
> server connections */
> +#define PR_O_CONTCNT 0x80000000 /* continous counters */
>
>
> #endif /* _TYPES_BACKEND_H */
> diff -Nur haproxy-1.3.13-orig/src/cfgparse.c
> haproxy-1.3.13-contiguous-cunters/src/cfgparse.c
> --- haproxy-1.3.13-orig/src/cfgparse.c 2007-11-02 01:12:24.000000000 +0100
> +++ haproxy-1.3.13-contiguous-cunters/src/cfgparse.c 2007-11-04
> 16:36:09.000000000 +0100
> @@ -95,6 +95,7 @@
> { "httpclose", PR_O_HTTP_CLOSE, PR_CAP_FE | PR_CAP_BE, 0 },
> { "nolinger", PR_O_TCP_NOLING, PR_CAP_FE | PR_CAP_BE, 0 },
> { "logasap", PR_O_LOGASAP, PR_CAP_FE, 0 },
> + { "contcnt", PR_O_CONTCNT, PR_CAP_FE, 0 },
> { "abortonclose", PR_O_ABRT_CLOSE, PR_CAP_BE, 0 },
> { "checkcache", PR_O_CHK_CACHE, PR_CAP_BE, 0 },
> { "dontlognull", PR_O_NULLNOLOG, PR_CAP_FE, 0 },
> diff -Nur haproxy-1.3.13-orig/src/proto_http.c
> haproxy-1.3.13-contiguous-cunters/src/proto_http.c
> --- haproxy-1.3.13-orig/src/proto_http.c 2007-10-18
> 22:38:22.000000000 +0200
> +++ haproxy-1.3.13-contiguous-cunters/src/proto_http.c 2007-11-04
> 16:41:12.000000000 +0100
> @@ -613,6 +613,10 @@
> } while (fsm_resync);
>
> if (likely(s->cli_state != CL_STCLOSE || s->srv_state !=
> SV_STCLOSE)) {
> +
> + if (s->fe->options & PR_O_CONTCNT)
> + session_process_counters(s);
> +
> s->req->flags &= BF_CLEAR_READ & BF_CLEAR_WRITE;
> s->rep->flags &= BF_CLEAR_READ & BF_CLEAR_WRITE;
>
> @@ -651,21 +655,7 @@
> }
>
> s->logs.t_close = tv_ms_elapsed(&s->logs.tv_accept, &now);
> - if (s->req != NULL)
> - s->logs.bytes_in = s->req->total;
> - if (s->rep != NULL)
> - s->logs.bytes_out = s->rep->total;
> -
> - s->fe->bytes_in += s->logs.bytes_in;
> - s->fe->bytes_out += s->logs.bytes_out;
> - if (s->be != s->fe) {
> - s->be->bytes_in += s->logs.bytes_in;
> - s->be->bytes_out += s->logs.bytes_out;
> - }
> - if (s->srv) {
> - s->srv->bytes_in += s->logs.bytes_in;
> - s->srv->bytes_out += s->logs.bytes_out;
> - }
> + session_process_counters(s);
>
> /* let's do a final log if we need it */
> if (s->logs.logwait &&
> @@ -3094,6 +3084,7 @@
> http_sess_log(t);
> else
> tcp_sess_log(t);
> + t->logs.bytes_in = 0;
> }
>
> /* Note: we must not try to cheat by jumping directly to
> DATA,
> @@ -3389,6 +3380,7 @@
> /* dump server statistics */
> int ret = stats_dump_http(s, s->be->uri_auth,
> (s->flags & SN_STAT_FMTCSV) ? 0 :
> STAT_FMT_HTML);
> +
> if (ret >= 0)
> return ret;
> /* -1 indicates an error */
> diff -Nur haproxy-1.3.13-orig/src/proto_uxst.c
> haproxy-1.3.13-contiguous-cunters/src/proto_uxst.c
> --- haproxy-1.3.13-orig/src/proto_uxst.c 2007-10-18
> 22:38:22.000000000 +0200
> +++ haproxy-1.3.13-contiguous-cunters/src/proto_uxst.c 2007-11-04
> 16:39:58.000000000 +0100
> @@ -1238,6 +1238,10 @@
> } while (fsm_resync);
>
> if (likely(s->cli_state != CL_STCLOSE || s->srv_state !=
> SV_STCLOSE)) {
> +
> + if (s->fe->options & PR_O_CONTCNT)
> + session_process_counters(s);
> +
> s->req->flags &= BF_CLEAR_READ & BF_CLEAR_WRITE;
> s->rep->flags &= BF_CLEAR_READ & BF_CLEAR_WRITE;
>
> @@ -1270,23 +1274,7 @@
> }
>
> s->logs.t_close = tv_ms_elapsed(&s->logs.tv_accept, &now);
> - if (s->req != NULL)
> - s->logs.bytes_in = s->req->total;
> - if (s->rep != NULL)
> - s->logs.bytes_out = s->rep->total;
> -
> - if (s->fe) {
> - s->fe->bytes_in += s->logs.bytes_in;
> - s->fe->bytes_out += s->logs.bytes_out;
> - }
> - if (s->be && (s->be != s->fe)) {
> - s->be->bytes_in += s->logs.bytes_in;
> - s->be->bytes_out += s->logs.bytes_out;
> - }
> - if (s->srv) {
> - s->srv->bytes_in += s->logs.bytes_in;
> - s->srv->bytes_out += s->logs.bytes_out;
> - }
> + session_process_counters(s);
>
> /* let's do a final log if we need it */
> if (s->logs.logwait &&
> diff -Nur haproxy-1.3.13-orig/src/session.c
> haproxy-1.3.13-contiguous-cunters/src/session.c
> --- haproxy-1.3.13-orig/src/session.c 2007-10-18 22:38:22.000000000 +0200
> +++ haproxy-1.3.13-contiguous-cunters/src/session.c 2007-11-04
> 16:24:23.000000000 +0100
> @@ -102,6 +102,38 @@
> return pool2_session != NULL;
> }
>
> +void session_process_counters(struct session *s) {
> +
> + unsigned long long bytes;
> +
> + if (s->req && s->req->total != s->logs.bytes_in) {
> + bytes = s->req->total - s->logs.bytes_in;
> +
> + s->fe->bytes_in += bytes;
> +
> + if (s->be != s->fe)
> + s->be->bytes_in += bytes;
> +
> + if (s->srv)
> + s->srv->bytes_in += bytes;
> +
> + s->logs.bytes_in = s->req->total;
> + }
> +
> + if (s->rep && s->rep->total != s->logs.bytes_out) {
> + bytes = s->rep->total - s->logs.bytes_out;
> +
> + s->fe->bytes_out += bytes;
> +
> + if (s->be != s->fe)
> + s->be->bytes_out += bytes;
> +
> + if (s->srv)
> + s->srv->bytes_out += bytes;
> +
> + s->logs.bytes_out = s->rep->total;
> + }
> +}
>
> /*
> * Local variables:
>
> Best regards,
>
> Krzysztof Ol?dzki
Received on 2007/11/04 21:44