[PATCH] Added debug_random directive
J Carter
jordanc.carter at outlook.com
Sat Aug 26 18:13:59 UTC 2023
Hello,
Thanks for the review.
> On Mon, Jul 24, 2023 at 05:24:04AM +0100, J Carter wrote:
>
> [..]
>
> > # HG changeset patch
> > # User J Carter <jordanc.carter at outlook.com>
> > # Date 1690171706 -3600
> > # Mon Jul 24 05:08:26 2023 +0100
> > # Node ID ea91b9aa69d8ce9dc9878209a83b7d538e6bc7e1
> > # Parent 77c1418916f7817a0d020f28d8a08f278a12fe43
> > Added debug_random directive
>
> This line needs a prefix, for example:
>
> Events: debug_random directive.
>
Understood, fixed.
> See "hg log" for common prefixes used in nginx commits.
>
> > More bug fixes and style changes.
> >
> > diff -r 77c1418916f7 -r ea91b9aa69d8 src/event/ngx_event.c
> > --- a/src/event/ngx_event.c Thu Jun 08 14:58:01 2023 +0400
> > +++ b/src/event/ngx_event.c Mon Jul 24 05:08:26 2023 +0100
> > @@ -30,6 +30,8 @@
> > static char *ngx_event_use(ngx_conf_t *cf, ngx_command_t *cmd, void *conf);
> > static char *ngx_event_debug_connection(ngx_conf_t *cf, ngx_command_t *cmd,
> > void *conf);
> > +static char *ngx_event_debug_random(ngx_conf_t *cf, ngx_command_t *cmd,
> > + void *conf);
> >
> > static void *ngx_event_core_create_conf(ngx_cycle_t *cycle);
> > static char *ngx_event_core_init_conf(ngx_cycle_t *cycle, void *conf);
> > @@ -162,6 +164,13 @@
> > 0,
> > NULL },
> >
> > + { ngx_string("debug_random"),
> > + NGX_EVENT_CONF|NGX_CONF_TAKE1,
> > + ngx_event_debug_random,
> > + 0,
> > + 0,
> > + NULL },
> > +
> > ngx_null_command
> > };
> >
> > @@ -496,6 +505,7 @@
> > size_t size, cl;
> > ngx_shm_t shm;
> > ngx_time_t *tp;
> > + ngx_cidr_t *cidr;
> > ngx_core_conf_t *ccf;
> > ngx_event_conf_t *ecf;
> >
> > @@ -507,6 +517,33 @@
> > "using the \"%s\" event method", ecf->name);
> > }
> >
> > + if (ecf->debug_connection.nelts == 0
> > + && ecf->debug_scaled_pct > 0)
> > + {
> > + cidr = ngx_array_push(&ecf->debug_connection);
> > + if (cidr == NULL) {
> > + return NGX_ERROR;
> > + }
> > + /*0.0.0.0/0*/
> > + ngx_memzero(cidr, sizeof(ngx_cidr_t));
> > + cidr->family = AF_INET;
> > +
> > +#ifdef NGX_HAVE_INET6
> > + cidr = ngx_array_push(&ecf->debug_connection);
> > + if (cidr == NULL) {
> > + return NGX_ERROR;
> > + }
> > + /*::/0*/
> > + ngx_memzero(cidr, sizeof(ngx_cidr_t));
> > + cidr->family = AF_INET6;
> > +#endif
> > +
> > + } else if (ecf->debug_connection.nelts > 0
> > + && ecf->debug_scaled_pct == 0)
> > + {
> > + ecf->debug_scaled_pct = NGX_MAX_INT32_VALUE;
> > + }
> > +
>
> Why do we need this code? It looks to me like everything can be done in
> ngx_debug_accepted_connection().
>
The previous attempt (further back in this thread) had this logic in
ngx_debug_accepted_connection(), however I changed it to this style as it made
the logic of that function very messy, and introduced new special cases.
With this method, I am simply utilizing the standard mechanisms
(same as a user would use, debug_connections) to add mark all IPV4/V6
connections as candidates for selection.
In any case - the "else if" section must be done in this function.
> > ccf = (ngx_core_conf_t *) ngx_get_conf(cycle->conf_ctx, ngx_core_module);
> >
> > ngx_timer_resolution = ccf->timer_resolution;
> > @@ -1254,6 +1291,55 @@
> > }
> >
> >
> > +static char *
> > +ngx_event_debug_random(ngx_conf_t *cf, ngx_command_t *cmd, void *conf)
> > +{
> > +#if (NGX_DEBUG)
> > + ngx_event_conf_t *ecf = conf;
> > +
> > + u_char *c;
> > + ngx_int_t pct;
> > + ngx_uint_t len;
> > + ngx_str_t *value;
> > +
> > + if (ecf->debug_scaled_pct != NGX_CONF_UNSET_UINT) {
> > + return "is duplicate";
> > + }
> > +
> > + value = cf->args->elts;
> > + c = value[1].data;
> > + len = value[1].len;
> > +
> > + if (c[len-1] != '%') {
> > + ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> > + "%V missing '%%'",
> > + &value[1]);
> > + return NGX_CONF_ERROR;
> > + }
> > +
> > + pct = ngx_atofp(c, len-1, 2);
> > +
> > + if (pct == NGX_ERROR || pct == 0 || pct > 9999) {
> > + ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> > + "%V is an invalid value",
> > + &value[1]);
> > + return NGX_CONF_ERROR;
> > + }
> > +
> > + ecf->debug_scaled_pct = NGX_MAX_INT32_VALUE / 10000 * pct;
>
> We already have a similar handler that parses percent argument, see
> ngx_stream_split_clients(). Style-wise it's a good idea to follow the
> style/naming of existing code.
>
Understood, I have updated the parsing logic to more closely match
the style of split clients.
> Also, why not store 0-10000 value range in configuration and use
> ngx_random() % 10000 in runtime?
>
By scaling this to INT_MAX range at configuration load time I avoid the
overhead of division for every connection accept (and I can't see a
disadvantage to doing this).
I notice in in ngx_http_split_clients (line 192) it appears to also do
prescaling for the ranges (to uint32 max in that case) and so it also
avoids division when comparing the ranges to hashed input at runtime.
> > +#else
> > +
> > + ngx_conf_log_error(NGX_LOG_WARN, cf, 0,
> > + "\"debug_random\" is ignored, you need to rebuild "
> > + "nginx using --with-debug option to enable it");
> > +
> > +#endif
> > +
> > + return NGX_CONF_OK;
> > +}
> > +
> > +
> > static void *
> > ngx_event_core_create_conf(ngx_cycle_t *cycle)
> > {
> > @@ -1279,6 +1365,8 @@
> > return NULL;
> > }
> >
> > + ecf->debug_scaled_pct = NGX_CONF_UNSET_UINT;
> > +
> > #endif
> >
> > return ecf;
> > @@ -1369,5 +1457,7 @@
> > ngx_conf_init_value(ecf->accept_mutex, 0);
> > ngx_conf_init_msec_value(ecf->accept_mutex_delay, 500);
> >
> > + ngx_conf_init_uint_value(ecf->debug_scaled_pct, 0);
> > +
> > return NGX_CONF_OK;
> > }
> > diff -r 77c1418916f7 -r ea91b9aa69d8 src/event/ngx_event.h
> > --- a/src/event/ngx_event.h Thu Jun 08 14:58:01 2023 +0400
> > +++ b/src/event/ngx_event.h Mon Jul 24 05:08:26 2023 +0100
> > @@ -438,6 +438,7 @@
> > u_char *name;
> >
> > #if (NGX_DEBUG)
> > + ngx_uint_t debug_scaled_pct;
> > ngx_array_t debug_connection;
> > #endif
> > } ngx_event_conf_t;
> > diff -r 77c1418916f7 -r ea91b9aa69d8 src/event/ngx_event_accept.c
> > --- a/src/event/ngx_event_accept.c Thu Jun 08 14:58:01 2023 +0400
> > +++ b/src/event/ngx_event_accept.c Mon Jul 24 05:08:26 2023 +0100
> > @@ -523,6 +523,7 @@
> > struct sockaddr_in6 *sin6;
> > ngx_uint_t n;
> > #endif
> > + ngx_uint_t r = ngx_random();
>
> Style: we do not initialize non-static local variables like that.
> Instead, a variable is initialized explicitly in the code after declaration.
>
Understood, fixed.
> >
> > cidr = ecf->debug_connection.elts;
> > for (i = 0; i < ecf->debug_connection.nelts; i++) {
> > @@ -548,6 +549,7 @@
> >
> > #if (NGX_HAVE_UNIX_DOMAIN)
> > case AF_UNIX:
> > + r = 0;
> > break;
> > #endif
> >
> > @@ -561,7 +563,10 @@
> > break;
> > }
> >
> > - c->log->log_level = NGX_LOG_DEBUG_CONNECTION|NGX_LOG_DEBUG_ALL;
> > + if (r <= ecf->debug_scaled_pct) {
> > + c->log->log_level = NGX_LOG_DEBUG_CONNECTION|NGX_LOG_DEBUG_ALL;
> > + }
> > +
> > break;
> >
> > next:
> >
I've added some additional style fixes & preprocess fixes
(missing #if(debug)s), and also updated the commit message all into one.
# HG changeset patch
# User J Carter <jordanc.carter at outlook.com>
# Date 1693056585 -3600
# Sat Aug 26 14:29:45 2023 +0100
# Node ID 2f95db29f38476e76cdfc5e2d0a59a0c569b74f5
# Parent 77c1418916f7817a0d020f28d8a08f278a12fe43
Events: debug_random directive
This directive enforces for EITHER a percentage of total connections
OR a percentage of connections matched by debug_connection CIDRs
to have debug logging enabled.
This is not performed for AF_UNIX connections, previous
debug_connections handling still applies.
This directive is useful for debugging nginx when under high load
(production) - where logging all connections with debug error log is not
possible without disrupting traffic.
This directive takes a value between 0.00%-100.00% exclusive.
Example usage:
events {
worker_connections 1024;
#if uncommented, the percentage applies to connection from lo.
#debug_connection 127.0.0.0/8;
debug_random 1%;
}
diff -r 77c1418916f7 -r 2f95db29f384 src/event/ngx_event.c
--- a/src/event/ngx_event.c Thu Jun 08 14:58:01 2023 +0400
+++ b/src/event/ngx_event.c Sat Aug 26 14:29:45 2023 +0100
@@ -30,6 +30,8 @@
static char *ngx_event_use(ngx_conf_t *cf, ngx_command_t *cmd, void *conf);
static char *ngx_event_debug_connection(ngx_conf_t *cf, ngx_command_t *cmd,
void *conf);
+static char *ngx_event_debug_random(ngx_conf_t *cf, ngx_command_t *cmd,
+ void *conf);
static void *ngx_event_core_create_conf(ngx_cycle_t *cycle);
static char *ngx_event_core_init_conf(ngx_cycle_t *cycle, void *conf);
@@ -162,6 +164,13 @@
0,
NULL },
+ { ngx_string("debug_random"),
+ NGX_EVENT_CONF|NGX_CONF_TAKE1,
+ ngx_event_debug_random,
+ 0,
+ 0,
+ NULL },
+
ngx_null_command
};
@@ -498,6 +507,9 @@
ngx_time_t *tp;
ngx_core_conf_t *ccf;
ngx_event_conf_t *ecf;
+#if (NGX_DEBUG)
+ ngx_cidr_t *cidr;
+#endif
cf = ngx_get_conf(cycle->conf_ctx, ngx_events_module);
ecf = (*cf)[ngx_event_core_module.ctx_index];
@@ -507,6 +519,35 @@
"using the \"%s\" event method", ecf->name);
}
+#if (NGX_DEBUG)
+ if (ecf->debug_connection.nelts == 0
+ && ecf->debug_percent > 0)
+ {
+ cidr = ngx_array_push(&ecf->debug_connection);
+ if (cidr == NULL) {
+ return NGX_ERROR;
+ }
+ /*0.0.0.0/0*/
+ ngx_memzero(cidr, sizeof(ngx_cidr_t));
+ cidr->family = AF_INET;
+
+ #if (NGX_HAVE_INET6)
+ cidr = ngx_array_push(&ecf->debug_connection);
+ if (cidr == NULL) {
+ return NGX_ERROR;
+ }
+ /*::/0*/
+ ngx_memzero(cidr, sizeof(ngx_cidr_t));
+ cidr->family = AF_INET6;
+ #endif
+
+ } else if (ecf->debug_connection.nelts > 0
+ && ecf->debug_percent == 0)
+ {
+ ecf->debug_percent = NGX_MAX_INT32_VALUE;
+ }
+#endif
+
ccf = (ngx_core_conf_t *) ngx_get_conf(cycle->conf_ctx, ngx_core_module);
ngx_timer_resolution = ccf->timer_resolution;
@@ -1254,6 +1295,53 @@
}
+static char *
+ngx_event_debug_random(ngx_conf_t *cf, ngx_command_t *cmd, void *conf)
+{
+#if (NGX_DEBUG)
+ ngx_event_conf_t *ecf = conf;
+
+ ngx_int_t n;
+ ngx_str_t *value;
+
+ if (ecf->debug_percent != NGX_CONF_UNSET_UINT) {
+ return "is duplicate";
+ }
+
+ value = cf->args->elts;
+
+ if (value[1].len == 0 || value[1].data[value[1].len -1] != '%') {
+ goto invalid;
+ }
+
+ n = ngx_atofp(value[1].data, value[1].len - 1, 2);
+
+ if (n == NGX_ERROR || n == 0 || n > 9999) {
+ goto invalid;
+ }
+
+ ecf->debug_percent = (ngx_uint_t)
+ (n * (uint64_t) NGX_MAX_INT32_VALUE / 10000);
+
+ return NGX_CONF_OK;
+
+invalid:
+
+ ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
+ "invalid percent value \"%V\"", &value[1]);
+
+#else
+
+ ngx_conf_log_error(NGX_LOG_WARN, cf, 0,
+ "\"debug_random\" is ignored, you need to rebuild "
+ "nginx using --with-debug option to enable it");
+
+#endif
+
+ return NGX_CONF_ERROR;
+}
+
+
static void *
ngx_event_core_create_conf(ngx_cycle_t *cycle)
{
@@ -1279,6 +1367,8 @@
return NULL;
}
+ ecf->debug_percent = NGX_CONF_UNSET_UINT;
+
#endif
return ecf;
@@ -1369,5 +1459,9 @@
ngx_conf_init_value(ecf->accept_mutex, 0);
ngx_conf_init_msec_value(ecf->accept_mutex_delay, 500);
+#if (NGX_DEBUG)
+ ngx_conf_init_uint_value(ecf->debug_percent, 0);
+#endif
+
return NGX_CONF_OK;
}
diff -r 77c1418916f7 -r 2f95db29f384 src/event/ngx_event.h
--- a/src/event/ngx_event.h Thu Jun 08 14:58:01 2023 +0400
+++ b/src/event/ngx_event.h Sat Aug 26 14:29:45 2023 +0100
@@ -438,6 +438,7 @@
u_char *name;
#if (NGX_DEBUG)
+ ngx_uint_t debug_percent;
ngx_array_t debug_connection;
#endif
} ngx_event_conf_t;
diff -r 77c1418916f7 -r 2f95db29f384 src/event/ngx_event_accept.c
--- a/src/event/ngx_event_accept.c Thu Jun 08 14:58:01 2023 +0400
+++ b/src/event/ngx_event_accept.c Sat Aug 26 14:29:45 2023 +0100
@@ -518,12 +518,14 @@
{
struct sockaddr_in *sin;
ngx_cidr_t *cidr;
- ngx_uint_t i;
+ ngx_uint_t i, r;
#if (NGX_HAVE_INET6)
struct sockaddr_in6 *sin6;
ngx_uint_t n;
#endif
+ r = ngx_random();
+
cidr = ecf->debug_connection.elts;
for (i = 0; i < ecf->debug_connection.nelts; i++) {
if (cidr[i].family != (ngx_uint_t) c->sockaddr->sa_family) {
@@ -548,6 +550,7 @@
#if (NGX_HAVE_UNIX_DOMAIN)
case AF_UNIX:
+ r = 0;
break;
#endif
@@ -561,7 +564,10 @@
break;
}
- c->log->log_level = NGX_LOG_DEBUG_CONNECTION|NGX_LOG_DEBUG_ALL;
+ if (r <= ecf->debug_percent) {
+ c->log->log_level = NGX_LOG_DEBUG_CONNECTION|NGX_LOG_DEBUG_ALL;
+ }
+
break;
next:
-------------- next part --------------
A non-text attachment was scrubbed...
Name: hgexport
Type: application/octet-stream
Size: 6009 bytes
Desc: not available
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20230826/67155e16/attachment.obj>
More information about the nginx-devel
mailing list