HAProxy ratelimit based on bandwidth

[Sangameshwar Babu]

Hello Team,

I would like to get some suggestions on setting up ratelimit on HAProxy 1.8
version, my current setup is as below.

1000+ rsyslog clients(TCP)   >  HAProxy (TCP mode)>  backend
centralized rsyslog server.

I have the below stick table and acl's through which I am able to mark a
source as "abuse" if the client crosses the limit post which all new
connections from the same client are rejected until stick table timer
expires.

haproxy.cfg
-
stick-table type ip size 200k expire 2m store
gpc0,conn_rate(2s),bytes_in_rate(1s),bytes_in_cnt

acl data_rate_abuse  sc1_bytes_in_rate ge 100
acl data_size_abuse  sc1_kbytes_in ge 1

   tcp-request connection silent-drop if data_rate_abuse
tcp-request connection reject if data_size_abuse

However I would like to configure in such a way that once a client sends
about "x bytes" of data the connection should be closed instantly instead
of marking it abuse and simultaneous connections being rejected.

Kindly let me know if the above can be configured with HAProxy version 1.8.

BR
Sangam

Re: [PATCH] MINOR: abort() on my_unreachable() when DEBUG_USE_ABORT is set.

[Илья Шипицин]

there's another one not reported coverity finding in src/hlua.c

I tried to suppress it by adding DEFINE=-DDEBUG_USE_ABORT to coverity build
(please notice BUG_ON(...) which is not recognized by coverity).
but I did something wrong and it did not help :)


4155lua_settable(L, -3);
4156
4157htx = htxbuf(&s->req.buf);

6. returned_null: htx_get_first_blk returns NULL (checked 17 out of 18
times). [show details

]

7. var_assigned: Assigning: blk = NULL return value from htx_get_first_blk.
4158blk = htx_get_first_blk(htx);
4159BUG_ON(!blk || htx_get_blk_type(blk) != HTX_BLK_REQ_SL);

CID 1401718 (#1 of 1): Dereference null return value (NULL_RETURNS)8.
dereference: Dereferencing a pointer that might be NULL blk when calling
htx_get_blk_ptr. [show details

]
4160sl = htx_get_blk_ptr(htx, blk);
4161
4162/* Stores the request method. */



пн, 25 янв. 2021 г. в 21:51, Tim Duesterhus :

> Hopefully this helps static analysis tools detecting that the code after
> that
> call is unreachable.
>
> See GitHub Issue #1075.
> ---
>  include/haproxy/compiler.h | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/include/haproxy/compiler.h b/include/haproxy/compiler.h
> index e5fae3e27..fba6dc358 100644
> --- a/include/haproxy/compiler.h
> +++ b/include/haproxy/compiler.h
> @@ -66,11 +66,15 @@
>   * above which can more aggressively detect null dereferences. The builtin
>   * below was introduced in gcc 4.5, and before it we didn't care.
>   */
> +#ifdef DEBUG_USE_ABORT
> +#define my_unreachable() abort()
> +#else
>  #if __GNUC__ >= 5 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 5)
>  #define my_unreachable() __builtin_unreachable()
>  #else
>  #define my_unreachable()
>  #endif
> +#endif
>
>  /* This macro may be used to block constant propagation that lets the
> compiler
>   * detect a possible NULL dereference on a variable resulting from an
> explicit
> --
> 2.29.0
>
>

[PATCH] MINOR: abort() on my_unreachable() when DEBUG_USE_ABORT is set.

[Tim Duesterhus]

Hopefully this helps static analysis tools detecting that the code after that
call is unreachable.

See GitHub Issue #1075.
---
 include/haproxy/compiler.h | 4 
 1 file changed, 4 insertions(+)

diff --git a/include/haproxy/compiler.h b/include/haproxy/compiler.h
index e5fae3e27..fba6dc358 100644
--- a/include/haproxy/compiler.h
+++ b/include/haproxy/compiler.h
@@ -66,11 +66,15 @@
  * above which can more aggressively detect null dereferences. The builtin
  * below was introduced in gcc 4.5, and before it we didn't care.
  */
+#ifdef DEBUG_USE_ABORT
+#define my_unreachable() abort()
+#else
 #if __GNUC__ >= 5 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 5)
 #define my_unreachable() __builtin_unreachable()
 #else
 #define my_unreachable()
 #endif
+#endif
 
 /* This macro may be used to block constant propagation that lets the compiler
  * detect a possible NULL dereference on a variable resulting from an explicit
-- 
2.29.0

[PATCH v2 3/4] MEDIUM: stats: allow to select one field in `stats_fill_sv_stats`

[William Dauchy]

prometheus approach requires to output all values for a given metric
name; meaning we iterate through all metrics, and then iterate in the
inner loop on all objects for this metric.
In order to allow more code reuse, adapt the stats API to be able to
select one field or fill them all otherwise.
This patch follows what has already been done on frontend and backend
side.
>From this patch it should be possible to remove most of the duplicate
code on prometheuse side for the server.

A few things to note though:
- state require prior calculation, so I moved that to a sort of helper
  `stats_fill_be_stats_computestate`.
- all ST_F*TIME fields requires some minor compute, so I moved it at te
  beginning of the function under a condition.

Signed-off-by: William Dauchy 
---
 include/haproxy/stats.h |   2 +-
 src/hlua_fcn.c  |   3 +-
 src/stats.c | 585 +---
 3 files changed, 368 insertions(+), 222 deletions(-)

diff --git a/include/haproxy/stats.h b/include/haproxy/stats.h
index 6c04f2761..6115dca8f 100644
--- a/include/haproxy/stats.h
+++ b/include/haproxy/stats.h
@@ -51,7 +51,7 @@ int stats_fill_fe_stats(struct proxy *px, struct field 
*stats, int len,
 int stats_fill_li_stats(struct proxy *px, struct listener *l, int flags,
 struct field *stats, int len);
 int stats_fill_sv_stats(struct proxy *px, struct server *sv, int flags,
-struct field *stats, int len);
+struct field *stats, int len, enum stat_field 
*selected_field);
 int stats_fill_be_stats(struct proxy *px, int flags, struct field *stats, int 
len,
enum stat_field *selected_field);
 
diff --git a/src/hlua_fcn.c b/src/hlua_fcn.c
index d13c5344f..6dd9efb21 100644
--- a/src/hlua_fcn.c
+++ b/src/hlua_fcn.c
@@ -922,7 +922,8 @@ int hlua_server_get_stats(lua_State *L)
return 1;
}
 
-   stats_fill_sv_stats(srv->proxy, srv, STAT_SHLGNDS, stats, STATS_LEN);
+   stats_fill_sv_stats(srv->proxy, srv, STAT_SHLGNDS, stats,
+   STATS_LEN, NULL);
 
lua_newtable(L);
for (i=0; i with the server statistics.  is
- * preallocated array of length . The length of the array
- * must be at least ST_F_TOTAL_FIELDS. If this length is less
- * then this value, the function returns 0, otherwise, it
- * returns 1.  can take the value STAT_SHLGNDS.
+/* Compute server state helper
  */
-int stats_fill_sv_stats(struct proxy *px, struct server *sv, int flags,
-struct field *stats, int len)
+static void stats_fill_sv_stats_computestate(struct server *sv, struct server 
*ref,
+enum srv_stats_state *state)
 {
-   struct server *via, *ref;
-   char str[INET6_ADDRSTRLEN];
-   struct buffer *out = get_trash_chunk();
-   enum srv_stats_state state;
-   char *fld_status;
-   long long srv_samples_counter;
-   unsigned int srv_samples_window = TIME_STATS_SAMPLES;
-
-   if (len < ST_F_TOTAL_FIELDS)
-   return 0;
-
-   /* we have "via" which is the tracked server as described in the 
configuration,
-* and "ref" which is the checked server and the end of the chain.
-*/
-   via = sv->track ? sv->track : sv;
-   ref = via;
-   while (ref->track)
-   ref = ref->track;
-
if (sv->cur_state == SRV_ST_RUNNING || sv->cur_state == 
SRV_ST_STARTING) {
if ((ref->check.state & CHK_ST_ENABLED) &&
(ref->check.health < ref->check.rise + ref->check.fall - 
1)) {
-   state = SRV_STATS_STATE_UP_GOING_DOWN;
+   *state = SRV_STATS_STATE_UP_GOING_DOWN;
} else {
-   state = SRV_STATS_STATE_UP;
+   *state = SRV_STATS_STATE_UP;
}
 
if (sv->cur_admin & SRV_ADMF_DRAIN) {
if (ref->agent.state & CHK_ST_ENABLED)
-   state = SRV_STATS_STATE_DRAIN_AGENT;
-   else if (state == SRV_STATS_STATE_UP_GOING_DOWN)
-   state = SRV_STATS_STATE_DRAIN_GOING_DOWN;
+   *state = SRV_STATS_STATE_DRAIN_AGENT;
+   else if (*state == SRV_STATS_STATE_UP_GOING_DOWN)
+   *state = SRV_STATS_STATE_DRAIN_GOING_DOWN;
else
-   state = SRV_STATS_STATE_DRAIN;
+   *state = SRV_STATS_STATE_DRAIN;
}
 
-   if (state == SRV_STATS_STATE_UP && !(ref->check.state & 
CHK_ST_ENABLED)) {
-   state = SRV_STATS_STATE_NO_CHECK;
+   if (*state == SRV_STATS_STATE_UP && !(ref->check.state & 
CHK_ST_ENABLED)) {
+   *state = SRV_STATS_STATE_NO_CHECK;
}
}
else if (sv->cur_sta

[PATCH v2 2/4] MINOR: contrib/prometheus-exporter: use fill_be_stats for backend dump

[William Dauchy]

use `stats_fill_be_stats` when possible to avoid duplicating code; make
use of field selector to get the needed field only.

the only difference is on `haproxy_backend_downtime_seconds_total` as
stats.c is testing `px->srv`. This behaviour is present since commit
7344f4789321ef8ce2ce17cf73dabd672f7c8c6 ("MINOR: stats: only report
backend's down time if it has servers"). The end result is a NaN instead
of a zero when no server are present.

Signed-off-by: William Dauchy 
---
 .../prometheus-exporter/service-prometheus.c  | 163 ++
 1 file changed, 15 insertions(+), 148 deletions(-)

diff --git a/contrib/prometheus-exporter/service-prometheus.c 
b/contrib/prometheus-exporter/service-prometheus.c
index bc9ebf3fc..12fe3ee90 100644
--- a/contrib/prometheus-exporter/service-prometheus.c
+++ b/contrib/prometheus-exporter/service-prometheus.c
@@ -685,8 +685,8 @@ static int promex_dump_back_metrics(struct appctx *appctx, 
struct htx *htx)
struct channel *chn = si_ic(appctx->owner);
struct ist out = ist2(trash.area, 0);
size_t max = htx_get_max_blksz(htx, channel_htx_recv_max(chn, htx));
+   struct field *stats = stat_l[STATS_DOMAIN_PROXY];
int ret = 1;
-   uint32_t weight;
double secs;
 
for (;appctx->st2 < ST_F_TOTAL_FIELDS; appctx->st2++) {
@@ -700,46 +700,13 @@ static int promex_dump_back_metrics(struct appctx 
*appctx, struct htx *htx)
if (px->disabled || px->uuid <= 0 || !(px->cap & 
PR_CAP_BE))
goto next_px;
 
+   if (!stats_fill_be_stats(px, 0, stats, 
ST_F_TOTAL_FIELDS, &(appctx->st2)))
+   return -1;
+
switch (appctx->st2) {
case ST_F_STATUS:
val = mkf_u32(FO_STATUS, 
(px->lbprm.tot_weight > 0 || !px->srv) ? 1 : 0);
break;
-   case ST_F_SCUR:
-   val = mkf_u32(0, px->beconn);
-   break;
-   case ST_F_SMAX:
-   val = mkf_u32(FN_MAX, 
px->be_counters.conn_max);
-   break;
-   case ST_F_SLIM:
-   val = mkf_u32(FO_CONFIG|FN_LIMIT, 
px->fullconn);
-   break;
-   case ST_F_STOT:
-   val = mkf_u64(FN_COUNTER, 
px->be_counters.cum_conn);
-   break;
-   case ST_F_RATE_MAX:
-   val = mkf_u32(0, 
px->be_counters.sps_max);
-   break;
-   case ST_F_LASTSESS:
-   val = mkf_s32(FN_AGE, 
be_lastsession(px));
-   break;
-   case ST_F_QCUR:
-   val = mkf_u32(0, px->nbpend);
-   break;
-   case ST_F_QMAX:
-   val = mkf_u32(FN_MAX, 
px->be_counters.nbpend_max);
-   break;
-   case ST_F_CONNECT:
-   val = mkf_u64(FN_COUNTER, 
px->be_counters.connect);
-   break;
-   case ST_F_REUSE:
-   val = mkf_u64(FN_COUNTER, 
px->be_counters.reuse);
-   break;
-   case ST_F_BIN:
-   val = mkf_u64(FN_COUNTER, 
px->be_counters.bytes_in);
-   break;
-   case ST_F_BOUT:
-   val = mkf_u64(FN_COUNTER, 
px->be_counters.bytes_out);
-   break;
case ST_F_QTIME:
secs = 
(double)swrate_avg(px->be_counters.q_time, TIME_STATS_SAMPLES) / 1000.0;
val = mkf_flt(FN_AVG, secs);
@@ -772,139 +739,39 @@ static int promex_dump_back_metrics(struct appctx 
*appctx, struct htx *htx)
secs = 
(double)px->be_counters.ttime_max / 1000.0;
val = mkf_flt(FN_MAX, secs);
break;
-   case ST_F_DREQ:
-   val = mkf_u64(FN_COUNTER, 
px->be_counters.denied_req);
-   break;
-   case ST_F_DRESP:
-   val = mkf_u6

[PATCH v2 4/4] MINOR: contrib/prometheus-exporter: use fill_sv_stats for server dump

[William Dauchy]

use `stats_fill_sv_stats` when possible to avoid duplicating code.

the following metrics have a change of behaviour:

haproxy_server_limit_sessions
haproxy_server_queue_limit
haproxy_server_check_failures_total
haproxy_server_check_up_down_total
haproxy_server_downtime_seconds_total
haproxy_server_current_throttle
haproxy_server_idle_connections_limit

depending on cases, if the limit was not configured or enabled, NaN is
returned instead. It should not be an issue for users, even better than
before as it provides more precise info.

Signed-off-by: William Dauchy 
---
 .../prometheus-exporter/service-prometheus.c  | 143 +-
 1 file changed, 7 insertions(+), 136 deletions(-)

diff --git a/contrib/prometheus-exporter/service-prometheus.c 
b/contrib/prometheus-exporter/service-prometheus.c
index 12fe3ee90..364d4b6d1 100644
--- a/contrib/prometheus-exporter/service-prometheus.c
+++ b/contrib/prometheus-exporter/service-prometheus.c
@@ -799,8 +799,8 @@ static int promex_dump_srv_metrics(struct appctx *appctx, 
struct htx *htx)
struct channel *chn = si_ic(appctx->owner);
struct ist out = ist2(trash.area, 0);
size_t max = htx_get_max_blksz(htx, channel_htx_recv_max(chn, htx));
+   struct field *stats = stat_l[STATS_DOMAIN_PROXY];
int ret = 1;
-   uint32_t weight;
double secs;
 
for (;appctx->st2 < ST_F_TOTAL_FIELDS; appctx->st2++) {
@@ -817,6 +817,9 @@ static int promex_dump_srv_metrics(struct appctx *appctx, 
struct htx *htx)
while (appctx->ctx.stats.obj2) {
sv = appctx->ctx.stats.obj2;
 
+   if (!stats_fill_sv_stats(px, sv, 0, stats, 
ST_F_TOTAL_FIELDS, &(appctx->st2)))
+   return -1;
+
if ((appctx->ctx.stats.flags & 
PROMEX_FL_NO_MAINT_SRV) && (sv->cur_admin & SRV_ADMF_MAINT))
goto next_sv;
 
@@ -824,39 +827,6 @@ static int promex_dump_srv_metrics(struct appctx *appctx, 
struct htx *htx)
case ST_F_STATUS:
val = mkf_u32(FO_STATUS, 
promex_srv_status(sv));
break;
-   case ST_F_SCUR:
-   val = mkf_u32(0, sv->cur_sess);
-   break;
-   case ST_F_SMAX:
-   val = mkf_u32(FN_MAX, 
sv->counters.cur_sess_max);
-   break;
-   case ST_F_SLIM:
-   val = 
mkf_u32(FO_CONFIG|FN_LIMIT, sv->maxconn);
-   break;
-   case ST_F_STOT:
-   val = mkf_u64(FN_COUNTER, 
sv->counters.cum_sess);
-   break;
-   case ST_F_RATE_MAX:
-   val = mkf_u32(FN_MAX, 
sv->counters.sps_max);
-   break;
-   case ST_F_LASTSESS:
-   val = mkf_s32(FN_AGE, 
srv_lastsession(sv));
-   break;
-   case ST_F_QCUR:
-   val = mkf_u32(0, sv->nbpend);
-   break;
-   case ST_F_QMAX:
-   val = mkf_u32(FN_MAX, 
sv->counters.nbpend_max);
-   break;
-   case ST_F_QLIMIT:
-   val = 
mkf_u32(FO_CONFIG|FS_SERVICE, sv->maxqueue);
-   break;
-   case ST_F_BIN:
-   val = mkf_u64(FN_COUNTER, 
sv->counters.bytes_in);
-   break;
-   case ST_F_BOUT:
-   val = mkf_u64(FN_COUNTER, 
sv->counters.bytes_out);
-   break;
case ST_F_QTIME:
secs = 
(double)swrate_avg(sv->counters.q_time, TIME_STATS_SAMPLES) / 1000.0;
val = mkf_flt(FN_AVG, secs);
@@ -889,43 +859,6 @@ static int promex_dump_srv_metrics(struct appctx *appctx, 
struct htx *htx)
secs = 
(double)sv->counters.ttime_max

[PATCH v2 1/4] MEDIUM: stats: allow to select one field in `stats_fill_be_stats`

[William Dauchy]

prometheus approach requires to output all values for a given metric
name; meaning we iterate through all metrics, and then iterate in the
inner loop on all objects for this metric.
In order to allow more code reuse, adapt the stats API to be able to
select one field or fill them all otherwise.
This patch follows what has already been done on frontend side.
>From this patch it should be possible to remove most of the duplicate
code on prometheuse side for the backend

A few things to note though:
- status and uweight field requires prior compute, so I moved that to a
  sort of helper `stats_fill_be_stats_computesrv`.
- all ST_F*TIME fields requires some minor compute, so I moved it at te
  beginning of the function under a condition.

Signed-off-by: William Dauchy 
---
 include/haproxy/stats.h |   3 +-
 src/hlua_fcn.c  |   2 +-
 src/stats.c | 368 
 3 files changed, 266 insertions(+), 107 deletions(-)

diff --git a/include/haproxy/stats.h b/include/haproxy/stats.h
index 8210367ac..6c04f2761 100644
--- a/include/haproxy/stats.h
+++ b/include/haproxy/stats.h
@@ -52,7 +52,8 @@ int stats_fill_li_stats(struct proxy *px, struct listener *l, 
int flags,
 struct field *stats, int len);
 int stats_fill_sv_stats(struct proxy *px, struct server *sv, int flags,
 struct field *stats, int len);
-int stats_fill_be_stats(struct proxy *px, int flags, struct field *stats, int 
len);
+int stats_fill_be_stats(struct proxy *px, int flags, struct field *stats, int 
len,
+   enum stat_field *selected_field);
 
 void stats_io_handler(struct stream_interface *si);
 int stats_emit_raw_data_field(struct buffer *out, const struct field *f);
diff --git a/src/hlua_fcn.c b/src/hlua_fcn.c
index aab864370..d13c5344f 100644
--- a/src/hlua_fcn.c
+++ b/src/hlua_fcn.c
@@ -1341,7 +1341,7 @@ int hlua_proxy_get_stats(lua_State *L)
 
px = hlua_check_proxy(L, 1);
if (px->cap & PR_CAP_BE)
-   stats_fill_be_stats(px, STAT_SHLGNDS, stats, STATS_LEN);
+   stats_fill_be_stats(px, STAT_SHLGNDS, stats, STATS_LEN, NULL);
else
stats_fill_fe_stats(px, stats, STATS_LEN, NULL);
lua_newtable(L);
diff --git a/src/stats.c b/src/stats.c
index dcb99f674..949b8f696 100644
--- a/src/stats.c
+++ b/src/stats.c
@@ -2260,130 +2260,288 @@ static int stats_dump_sv_stats(struct 
stream_interface *si, struct proxy *px, st
return stats_dump_one_line(stats, stats_count, appctx);
 }
 
-/* Fill  with the backend statistics.  is
- * preallocated array of length . The length of the array
- * must be at least ST_F_TOTAL_FIELDS. If this length is less
- * then this value, the function returns 0, otherwise, it
- * returns 1.  can take the value STAT_SHLGNDS.
+/* Helper to compute srv values for a given backend
  */
-int stats_fill_be_stats(struct proxy *px, int flags, struct field *stats, int 
len)
+static void stats_fill_be_stats_computesrv(struct proxy *px, int *nbup, int 
*nbsrv, int *totuw)
 {
-   long long be_samples_counter;
-   unsigned int be_samples_window = TIME_STATS_SAMPLES;
-   struct buffer *out = get_trash_chunk();
+   int nbup_tmp, nbsrv_tmp, totuw_tmp;
const struct server *srv;
-   int nbup, nbsrv;
-   int totuw;
-   char *fld;
-
-   if (len < ST_F_TOTAL_FIELDS)
-   return 0;
 
-   totuw = 0;
-   nbup = nbsrv = 0;
+   nbup_tmp = nbsrv_tmp = totuw_tmp = 0;
for (srv = px->srv; srv; srv = srv->next) {
if (srv->cur_state != SRV_ST_STOPPED) {
-   nbup++;
+   nbup_tmp++;
if (srv_currently_usable(srv) &&
(!px->srv_act ^ !(srv->flags & SRV_F_BACKUP)))
-   totuw += srv->uweight;
+   totuw_tmp += srv->uweight;
}
-   nbsrv++;
+   nbsrv_tmp++;
}
 
HA_RWLOCK_RDLOCK(LBPRM_LOCK, &px->lbprm.lock);
if (!px->srv_act && px->lbprm.fbck)
-   totuw = px->lbprm.fbck->uweight;
+   totuw_tmp = px->lbprm.fbck->uweight;
HA_RWLOCK_RDUNLOCK(LBPRM_LOCK, &px->lbprm.lock);
 
-   stats[ST_F_PXNAME]   = mkf_str(FO_KEY|FN_NAME|FS_SERVICE, px->id);
-   stats[ST_F_SVNAME]   = mkf_str(FO_KEY|FN_NAME|FS_SERVICE, "BACKEND");
-   stats[ST_F_MODE] = mkf_str(FO_CONFIG|FS_SERVICE, 
proxy_mode_str(px->mode));
-   stats[ST_F_QCUR] = mkf_u32(0, px->nbpend);
-   stats[ST_F_QMAX] = mkf_u32(FN_MAX, px->be_counters.nbpend_max);
-   stats[ST_F_SCUR] = mkf_u32(0, px->beconn);
-   stats[ST_F_SMAX] = mkf_u32(FN_MAX, px->be_counters.conn_max);
-   stats[ST_F_SLIM] = mkf_u32(FO_CONFIG|FN_LIMIT, px->fullconn);
-   stats[ST_F_STOT] = mkf_u64(FN_COUNTER, px->be_counters.cum_conn);
-   stats[ST_F_BIN]  = mkf_u64(FN_COUNTER, px

Re: [PATCH 5/6] MEDIUM: stats: allow to select one field in `stats_fill_sv_stats`

[William Dauchy]

Hello Christopher,

Thanks for the review.

On Mon, Jan 25, 2021 at 4:33 PM Christopher Faulet  wrote:
> The "via" parameter is unused. No reason to pass it.

good catch. surprised gcc did not triggered it
fixed in v2

> Same comment than for stats_fill_b_stats. The metric variable must be 
> initialized.

fixed in v2.

--
William

Re: [PATCH 3/6] MEDIUM: stats: allow to select one field in `stats_fill_be_stats`

[William Dauchy]

Hi Christopher,

Thanks for the review.

On Mon, Jan 25, 2021 at 4:26 PM Christopher Faulet  wrote:
> Here, you must take care to increment "*nbup" and "*nbsrv" in the for loop. 
> But
> changing it this way gcc then complains these variables are now unused. Thus 
> to
> make it happy, I must use local variables.

ah yes, I forgot the * indeed.
I did modify the code to use local variables but it somehow looks
uglier than before.
see in v2.

> Take a look at the commit 8596bfbaf ("BUG/MINOR: stats: Init the metric 
> variable
> when frontend stats are filled"). The metric variable must be initialized.

fixed in v2


--
William

Re: [PATCH 5/6] MEDIUM: stats: allow to select one field in `stats_fill_sv_stats`

[Christopher Faulet]

Le 22/01/2021 à 21:09, William Dauchy a écrit :

prometheus approach requires to output all values for a given metric
name; meaning we iterate through all metrics, and then iterate in the
inner loop on all objects for this metric.
In order to allow more code reuse, adapt the stats API to be able to
select one field or fill them all otherwise.
This patch follows what has already been done on frontend and backend
side.
 From this patch it should be possible to remove most of the duplicate
code on prometheuse side for the server.

A few things to note though:
- state require prior calculation, so I moved that to a sort of helper
   `stats_fill_be_stats_computestate`.
- all ST_F*TIME fields requires some minor compute, so I moved it at te
   beginning of the function under a condition.



William,

Here too, some comments inlined.

[...]


-/* Fill  with the server statistics.  is
- * preallocated array of length . The length of the array
- * must be at least ST_F_TOTAL_FIELDS. If this length is less
- * then this value, the function returns 0, otherwise, it
- * returns 1.  can take the value STAT_SHLGNDS.
+/* Compute server state helper
   */
-int stats_fill_sv_stats(struct proxy *px, struct server *sv, int flags,
-struct field *stats, int len)
+static void stats_fill_sv_stats_computestate(struct server *sv, struct server 
*via,
+struct server *ref, enum 
srv_stats_state *state)
  {


The "via" parameter is unused. No reason to pass it.


+/* Fill  with the backend statistics.  is preallocated array of
+ * length . If  is != NULL, only fill this one. The length
+ * of the array must be at least ST_F_TOTAL_FIELDS. If this length is less than
+ * this value, or if the selected field is not implemented for servers, the
+ * function returns 0, otherwise, it returns 1.  can take the value
+ * STAT_SHLGNDS.
+ */
+int stats_fill_sv_stats(struct proxy *px, struct server *sv, int flags,
+struct field *stats, int len,
+   enum stat_field *selected_field)
+{
+   enum stat_field current_field = (selected_field != NULL ? 
*selected_field : 0);
+   struct field metric;
+   struct server *via = sv->track ? sv->track : sv;
+   struct server *ref = via;
+   enum srv_stats_state state = 0;
+   char str[INET6_ADDRSTRLEN];
+   struct buffer *out = get_trash_chunk();
+   char *fld_status;
+   long long srv_samples_counter;
+   unsigned int srv_samples_window = TIME_STATS_SAMPLES;
  


Same comment than for stats_fill_b_stats. The metric variable must be 
initialized.

--
Christopher Faulet

Re: [PATCH 3/6] MEDIUM: stats: allow to select one field in `stats_fill_be_stats`

[Christopher Faulet]

Le 22/01/2021 à 21:09, William Dauchy a écrit :

prometheus approach requires to output all values for a given metric
name; meaning we iterate through all metrics, and then iterate in the
inner loop on all objects for this metric.
In order to allow more code reuse, adapt the stats API to be able to
select one field or fill them all otherwise.
This patch follows what has already been done on frontend side.
 From this patch it should be possible to remove most of the duplicate
code on prometheuse side for the backend

A few things to note though:
- status and uweight field requires prior compute, so I moved that to a
   sort of helper `stats_fill_be_stats_computesrv`.
- all ST_F*TIME fields requires some minor compute, so I moved it at te
   beginning of the function under a condition.



William,

Some comments inlined. I can handle the changes if you want by amending your 
patch. But I can wait a new version if you prefer to handle the changes 
yourself. Just let me know.


[...]


+/* Helper to compute srv values for a given backend
   */
-int stats_fill_be_stats(struct proxy *px, int flags, struct field *stats, int 
len)
+static void stats_fill_be_stats_computesrv(struct proxy *px, int *nbup, int 
*nbsrv, int *totuw)
  {
-   long long be_samples_counter;
-   unsigned int be_samples_window = TIME_STATS_SAMPLES;
-   struct buffer *out = get_trash_chunk();
const struct server *srv;
-   int nbup, nbsrv;
-   int totuw;
-   char *fld;
-
-   if (len < ST_F_TOTAL_FIELDS)
-   return 0;
  
-	totuw = 0;

-   nbup = nbsrv = 0;
+   *nbup = *nbsrv = *totuw = 0;
for (srv = px->srv; srv; srv = srv->next) {
if (srv->cur_state != SRV_ST_STOPPED) {
nbup++;
if (srv_currently_usable(srv) &&
(!px->srv_act ^ !(srv->flags & SRV_F_BACKUP)))
-   totuw += srv->uweight;
+   *totuw += srv->uweight;
}
nbsrv++;
}
  
  	HA_RWLOCK_RDLOCK(LBPRM_LOCK, &px->lbprm.lock);

if (!px->srv_act && px->lbprm.fbck)
-   totuw = px->lbprm.fbck->uweight;
+   *totuw = px->lbprm.fbck->uweight;
HA_RWLOCK_RDUNLOCK(LBPRM_LOCK, &px->lbprm.lock);
+}
  


Here, you must take care to increment "*nbup" and "*nbsrv" in the for loop. But 
changing it this way gcc then complains these variables are now unused. Thus to 
make it happy, I must use local variables.


[...]


+/* Fill  with the backend statistics.  is preallocated array of
+ * length . If  is != NULL, only fill this one. The length
+ * of the array must be at least ST_F_TOTAL_FIELDS. If this length is less than
+ * this value, or if the selected field is not implemented for backends, the
+ * function returns 0, otherwise, it returns 1.  can take the value
+ * STAT_SHLGNDS.
+ */
+int stats_fill_be_stats(struct proxy *px, int flags, struct field *stats, int 
len,
+   enum stat_field *selected_field)
+{
+   enum stat_field current_field = (selected_field != NULL ? 
*selected_field : 0);
+   struct field metric;
+   long long be_samples_counter;
+   unsigned int be_samples_window = TIME_STATS_SAMPLES;
+   struct buffer *out = get_trash_chunk();
+   int nbup, nbsrv, totuw = 0;
+   char *fld;
  


Take a look at the commit 8596bfbaf ("BUG/MINOR: stats: Init the metric variable 
when frontend stats are filled"). The metric variable must be initialized.



--
Christopher Faulet

Re: [PATCH 2/6] CLEANUP: stats: improve field selection for frontend http fields

[Christopher Faulet]

Le 22/01/2021 à 21:09, William Dauchy a écrit :

while working on backend/servers I realised I could have written that in
a better way and avoid one extra break. This is slightly improving
readiness.
also while being here, fix function declaration which was not 100%
accurate.

this patch does not change the behaviour of the code.

Signed-off-by: William Dauchy 
---
  include/haproxy/stats.h |  2 +-
  src/stats.c | 45 +
  2 files changed, 19 insertions(+), 28 deletions(-)

diff --git a/include/haproxy/stats.h b/include/haproxy/stats.h
index eb72446ae..8210367ac 100644
--- a/include/haproxy/stats.h
+++ b/include/haproxy/stats.h
@@ -47,7 +47,7 @@ int stats_dump_one_line(const struct field *stats, size_t 
stats_count, struct ap
  
  int stats_fill_info(struct field *info, int len);

  int stats_fill_fe_stats(struct proxy *px, struct field *stats, int len,
-   enum stat_field *field);
+   enum stat_field *selected_field);
  int stats_fill_li_stats(struct proxy *px, struct listener *l, int flags,
  struct field *stats, int len);
  int stats_fill_sv_stats(struct proxy *px, struct server *sv, int flags,
diff --git a/src/stats.c b/src/stats.c
index e1b350a44..161fc6548 100644
--- a/src/stats.c
+++ b/src/stats.c
@@ -1712,49 +1712,40 @@ int stats_fill_fe_stats(struct proxy *px, struct field 
*stats, int len,
metric = mkf_u64(FN_COUNTER, 
px->fe_counters.internal_errors);
break;
case ST_F_HRSP_1XX:
-   if (px->mode != PR_MODE_HTTP)
-   break;
-   metric = mkf_u64(FN_COUNTER, 
px->fe_counters.p.http.rsp[1]);
+   if (px->mode == PR_MODE_HTTP)
+   metric = mkf_u64(FN_COUNTER, 
px->fe_counters.p.http.rsp[1]);
break;
case ST_F_HRSP_2XX:
-   if (px->mode != PR_MODE_HTTP)
-   break;
-   metric = mkf_u64(FN_COUNTER, 
px->fe_counters.p.http.rsp[2]);
+   if (px->mode == PR_MODE_HTTP)
+   metric = mkf_u64(FN_COUNTER, 
px->fe_counters.p.http.rsp[2]);
break;
case ST_F_HRSP_3XX:
-   if (px->mode != PR_MODE_HTTP)
-   break;
-   metric = mkf_u64(FN_COUNTER, 
px->fe_counters.p.http.rsp[3]);
+   if (px->mode == PR_MODE_HTTP)
+   metric = mkf_u64(FN_COUNTER, 
px->fe_counters.p.http.rsp[3]);
break;
case ST_F_HRSP_4XX:
-   if (px->mode != PR_MODE_HTTP)
-   break;
-   metric = mkf_u64(FN_COUNTER, 
px->fe_counters.p.http.rsp[4]);
+   if (px->mode == PR_MODE_HTTP)
+   metric = mkf_u64(FN_COUNTER, 
px->fe_counters.p.http.rsp[4]);
break;
case ST_F_HRSP_5XX:
-   if (px->mode != PR_MODE_HTTP)
-   break;
-   metric = mkf_u64(FN_COUNTER, 
px->fe_counters.p.http.rsp[5]);
+   if (px->mode == PR_MODE_HTTP)
+   metric = mkf_u64(FN_COUNTER, 
px->fe_counters.p.http.rsp[5]);
break;
case ST_F_HRSP_OTHER:
-   if (px->mode != PR_MODE_HTTP)
-   break;
-   metric = mkf_u64(FN_COUNTER, 
px->fe_counters.p.http.rsp[0]);
+   if (px->mode == PR_MODE_HTTP)
+   metric = mkf_u64(FN_COUNTER, 
px->fe_counters.p.http.rsp[0]);
break;
case ST_F_INTERCEPTED:
-   if (px->mode != PR_MODE_HTTP)
-   break;
-   metric = mkf_u64(FN_COUNTER, 
px->fe_counters.intercepted_req);
+   if (px->mode == PR_MODE_HTTP)
+   metric = mkf_u64(FN_COUNTER, 
px->fe_counters.intercepted_req);
break;
case ST_F_CACHE_LOOKUPS:
-   if (px->mode != PR_MODE_HTTP)
-   break;
-   metric = mkf_u64(FN_COUNTER, 
px->fe_counters.p.http.cache_lookups);
+ 

Re: [PATCH 1/6] MINOR: contrib/prometheus-exporter: better output of Not-a-Number

[Christopher Faulet]

Le 22/01/2021 à 21:09, William Dauchy a écrit :

Not necessarily mandatory but I saw a few prometheus client parsing only
`NaN`. Also most librarries do output `NaN`

Signed-off-by: William Dauchy 
---
  contrib/prometheus-exporter/service-prometheus.c | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/contrib/prometheus-exporter/service-prometheus.c 
b/contrib/prometheus-exporter/service-prometheus.c
index aa72382d9..bc9ebf3fc 100644
--- a/contrib/prometheus-exporter/service-prometheus.c
+++ b/contrib/prometheus-exporter/service-prometheus.c
@@ -415,7 +415,7 @@ static int promex_srv_status(struct server *sv)
  
  /* Convert a field to its string representation and write it in , followed

   * by a newline, if there is enough space. non-numeric value are converted in
- * "Nan" because Prometheus only support numerical values (but it is 
unexepceted
+ * "NaN" because Prometheus only support numerical values (but it is 
unexepceted
   * to process this kind of value). It returns 1 on success. Otherwise, it
   * returns 0. The buffer's length must not exceed  value.
   */
@@ -424,14 +424,14 @@ static int promex_metric_to_str(struct buffer *out, 
struct field *f, size_t max)
int ret = 0;
  
  	switch (field_format(f, 0)) {

-   case FF_EMPTY: ret = chunk_strcat(out,  "Nan\n"); break;
+   case FF_EMPTY: ret = chunk_strcat(out,  "NaN\n"); break;
case FF_S32:   ret = chunk_appendf(out, "%d\n", f->u.s32); 
break;
case FF_U32:   ret = chunk_appendf(out, "%u\n", f->u.u32); 
break;
case FF_S64:   ret = chunk_appendf(out, "%lld\n", (long 
long)f->u.s64); break;
case FF_U64:   ret = chunk_appendf(out, "%llu\n", (unsigned long 
long)f->u.u64); break;
case FF_FLT:   ret = chunk_appendf(out, "%f\n", f->u.flt); 
break;
-   case FF_STR:   ret = chunk_strcat(out,  "Nan\n"); break;
-   default:   ret = chunk_strcat(out,  "Nan\n"); break;
+   case FF_STR:   ret = chunk_strcat(out,  "NaN\n"); break;
+   default:   ret = chunk_strcat(out,  "NaN\n"); break;
}
if (!ret || out->data > max)
return 0;



This one is merged now. Thanks !

--
Christopher Faulet