Re: [PATCH] MINOR: lua: Add a flag to disable logging to stderr
On Tue, Oct 24, 2023 at 11:41:56AM +, Tristan wrote: > > On 23/10/2023 14:38, Tristan wrote: > > Thanks both for your time helping me with it :) > > > > On 23/10/2023 14:34, Aurelien DARRAGON wrote: > > > Just a side note regarding the comment from the 2nd patch: it's not > > > useful to mention the commit ID from the previous patch given that the > > > effective commit ID will change when the patch will be applied on top of > > > haproxy master branch. > > > > Oh, I thought patch commit hashes carried through when applied. Well, TIL. > > On that note, I assume the easiest is to let Willy alter the commit message > to update (or remove) the commit reference when merging? Yeah I just did that. One method we commonly practice with such series is to simply mention 'previous commit "xxx" ...'. It's even more convenient for those reading the logs later because instead of stopping and copy- pasting a commit ID in another window, they know they just need to scroll a little bit more. > Asking just in case you were waiting for me to send an amended patch for it. No don't worry, I'm not for wasting anyone's time respinning patches for a few words I can edit with commit --amend. These are the occasional details we can and do routinely adjust when merging for optimal use of everyone's time (unless the patch is signed). Thanks! Willy
Re: [PATCH] MINOR: lua: Add a flag to disable logging to stderr
> On that note, I assume the easiest is to let Willy alter the commit message > to update (or remove) the commit reference when merging? > > Asking just in case you were waiting for me to send an amended patch for it. Yeah I think so, and even if it's not amended, it's not dramatic: the commit ID simply won't resolve, but we still have its name :) Aurelien
Re: [PATCH] MINOR: lua: Add a flag to disable logging to stderr
On 23/10/2023 14:38, Tristan wrote: Thanks both for your time helping me with it :) On 23/10/2023 14:34, Aurelien DARRAGON wrote: Just a side note regarding the comment from the 2nd patch: it's not useful to mention the commit ID from the previous patch given that the effective commit ID will change when the patch will be applied on top of haproxy master branch. Oh, I thought patch commit hashes carried through when applied. Well, TIL. On that note, I assume the easiest is to let Willy alter the commit message to update (or remove) the commit reference when merging? Asking just in case you were waiting for me to send an amended patch for it. Tristan
Re: [PATCH] MINOR: lua: Add a flag to disable logging to stderr
Thanks both for your time helping me with it :) On 23/10/2023 14:34, Aurelien DARRAGON wrote: Just a side note regarding the comment from the 2nd patch: it's not useful to mention the commit ID from the previous patch given that the effective commit ID will change when the patch will be applied on top of haproxy master branch. Oh, I thought patch commit hashes carried through when applied. Well, TIL. Tristan
Re: [PATCH] MINOR: lua: Add a flag to disable logging to stderr
> Overall it looks good to me as is. I'll let Aurélien have a quick look > as well in case he sees anything but I'm personally fine with merging > this. Looks good to me as well, nice job Tristan :) Just a side note regarding the comment from the 2nd patch: it's not useful to mention the commit ID from the previous patch given that the effective commit ID will change when the patch will be applied on top of haproxy master branch. Regards, Aurelien
Re: [PATCH] MINOR: lua: Add a flag to disable logging to stderr
On Mon, Oct 23, 2023 at 01:07:37PM +, Tristan wrote: > Hi Willy, > > On 23/10/2023 10:16, Willy Tarreau wrote: > > No more comments, overall this looks good to me. Thus in summary, let's > > try to avoid the ambiguity of "tune.lua.log" alone, and re-adjust the > > bitfields. By the way, if we're having the same prefix "tune.lua.log" > > for both options, it's an indication that we should likely have a dot > > after to enable loggers (or any other name) and stderr: > > > > tune.lua.log.loggers on|off > > tune.lua.log.stderr on|auto|off > > > > One being prefix of the other was also not great. So even if "log.loggers" > is a little awkward, this is probably for the best after all. Yeah I share the same feeling, not being proud of that name either :-) > So here's the 2 patches again. Hopefully they match what you wanted. > > Two notes: > - I saw various forms of enum declarations in the codebase (naming it or not > and typedef-ing or not) and I don't practice C enough to have an opinion on > the matter, so I just picked the one I think looks nicest... > - For the regtests, if we don't want them to tee stderr to check it, then > there's little point, and chance of breaking this all is also rather low > anyway as you said, so none added Overall it looks good to me as is. I'll let Aurélien have a quick look as well in case he sees anything but I'm personally fine with merging this. Thank you! Willy
Re: [PATCH] MINOR: lua: Add a flag to disable logging to stderr
Hi Willy,
On 23/10/2023 10:16, Willy Tarreau wrote:
No more comments, overall this looks good to me. Thus in summary, let's
try to avoid the ambiguity of "tune.lua.log" alone, and re-adjust the
bitfields. By the way, if we're having the same prefix "tune.lua.log"
for both options, it's an indication that we should likely have a dot
after to enable loggers (or any other name) and stderr:
tune.lua.log.loggers on|off
tune.lua.log.stderr on|auto|off
One being prefix of the other was also not great. So even if
"log.loggers" is a little awkward, this is probably for the best after all.
So here's the 2 patches again. Hopefully they match what you wanted.
Two notes:
- I saw various forms of enum declarations in the codebase (naming it or
not and typedef-ing or not) and I don't practice C enough to have an
opinion on the matter, so I just picked the one I think looks nicest...
- For the regtests, if we don't want them to tee stderr to check it,
then there's little point, and chance of breaking this all is also
rather low anyway as you said, so none added
Regards,
Tristan
From 7ef333b8803c213ab1bd0a33a73faa30336ab55e Mon Sep 17 00:00:00 2001
From: Tristan
Date: Mon, 23 Oct 2023 13:07:39 +0100
Subject: [PATCH] MINOR: lua: Add flags to configure logging behaviour
Until now, messages printed from LUA log functions were sent both to
the any logger configured for the current proxy, and additionally to
stderr (in most cases)
This introduces two flags to configure LUA log handling:
- tune.lua.log.loggers to use standard loggers or not
- tune.lua.log.stderr to use stderr, or not, or only conditionally
This addresses github feature request #2316
This can be backported to 2.8 as it doesn't change previous behaviour.
---
doc/configuration.txt | 22
doc/lua-api/index.rst | 12 ---
doc/lua.txt | 4 +++
src/hlua.c| 80 +--
4 files changed, 111 insertions(+), 7 deletions(-)
diff --git a/doc/configuration.txt b/doc/configuration.txt
index bb599bca9..433f15baf 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -1196,6 +1196,8 @@ The following keywords are supported in the "global"
section :
- tune.lua.service-timeout
- tune.lua.session-timeout
- tune.lua.task-timeout
+ - tune.lua.log.loggers
+ - tune.lua.log.stderr
- tune.max-checks-per-thread
- tune.maxaccept
- tune.maxpollevents
@@ -3192,6 +3194,26 @@ tune.lua.task-timeout
remain alive during of the lifetime of HAProxy. For example, a task used to
check servers.
+tune.lua.log.loggers { on | off }
+ Enables ('on') or disables ('off') logging the output of LUA scripts via the
+ loggers applicable to the current proxy, if any.
+
+ Defaults to 'on'.
+
+tune.lua.log.stderr { on | auto | off }
+ Enables ('on') or disables ('off') logging the output of LUA scripts via
+ stderr.
+ When set to 'auto', logging via stderr is conditionally 'on' if any of:
+
+- tune.lua.log.loggers is set to 'off'
+- the script is executed in a non-proxy context with no global logger
+- the script is executed in a proxy context with no logger attached
+
+ Please note that, when enabled, this logging is in addition to the logging
+ configured via tune.lua.log.loggers.
+
+ Defaults to 'on'.
+
tune.max-checks-per-thread
Sets the number of active checks per thread above which a thread will
actively try to search a less loaded thread to run the health check, or
diff --git a/doc/lua-api/index.rst b/doc/lua-api/index.rst
index 8a29bc5b5..90802cfd4 100644
--- a/doc/lua-api/index.rst
+++ b/doc/lua-api/index.rst
@@ -267,8 +267,10 @@ Core class
**context**: body, init, task, action, sample-fetch, converter
This function sends a log. The log is sent, according with the HAProxy
- configuration file, on the default syslog server if it is configured and on
- the stderr if it is allowed.
+ configuration file, to the loggers relevant to the current context and
+ to stderr if it is allowed.
+
+ The exact behaviour depends on tune.lua.log.loggers and tune.lua.log.stderr.
:param integer loglevel: Is the log level associated with the message. It is
a
number between 0 and 7.
@@ -2648,8 +2650,10 @@ TXN class
.. js:function:: TXN.log(TXN, loglevel, msg)
This function sends a log. The log is sent, according with the HAProxy
- configuration file, on the default syslog server if it is configured and on
- the stderr if it is allowed.
+ configuration file, to the loggers relevant to the current context and
+ to stderr if it is allowed.
+
+ The exact behaviour depends on tune.lua.log.loggers and tune.lua.log.stderr.
:param class_txn txn: The class txn object containing the data.
:param integer loglevel: Is the log level associated with the message. It is
diff --git a/doc/lua.txt b/doc/lua.txt
index 8d5561668..25db8a304 100644
--- a/doc/lua.txt
+++ b/doc/lua.txt
@@ -630,6 +630,10 @@ It displays
Re: [PATCH] MINOR: lua: Add a flag to disable logging to stderr
Hi Tristan,
On Fri, Oct 20, 2023 at 04:19:34PM +, Tristan wrote:
> Hi all again,
>
> Here is the updated patch set after changes based on feedback received.
Thanks for doing this work.
> The change is now split across 2 patches.
>
> Patch 0001 adding:
> - tune.lua.log { on | off } (defaults to 'on') for usage of loggers
> - tune.lua.log-stderr { on | auto | off } (defaults to 'on') for usage of
> stderr
I'm still finding these a bit confusing, due to the naming. It seems to
me that the first one is sufficient to disable everything including the
second (due to the name only, given that you explained it's not the case).
I got your point regarding my perception of "alerts" vs "logs". I thought
these were used by alerts and in fact I noticed from your patch on the doc
that it applies to txn.log(), core.log() and so on, so that's definitely
related to logging.
Just an idea, not necessarily the best one, but maybe "tune.lua.log"
should be called "tune.lua.log-loggers" instead ? otherwise we can go
back to a single multi-combinations option (but this leaves less room
for later extensions if other ways ever appear such as rings or anything
else).
> Effectively no change to current behaviour, hence fit for backport.
>
> Patch 0002 changing:
> - tune.lua.log-stderr default from 'on' to 'auto'
>
> Changes behaviour, hence no backport.
>
> I guess my only remaining question is whether we want regtests for this?
>
> I'm be happy to write some if yes, but I know regtests runtime is a concern
> and it'd require a bit of test infra work to make capturing stderr output,
> so I just want to be sure it is desirable before :-)
I'd say it's as you want. We can't test stderr in regtests so the test
will be limited anyway. Also I don't expect such a thing to be broken
by accident, which is the first purpose of regtests.
> diff --git a/src/hlua.c b/src/hlua.c
> index e94325727..70a25e762 100644
> --- a/src/hlua.c
> +++ b/src/hlua.c
> @@ -69,6 +69,20 @@
> #include
> #include
>
> +/* Global LUA flags */
> +/* log handling */
> +#define HLUA_TUNE_LOG 0x0001 /* send to current logger */
> +#define HLUA_TUNE_LOG_STDERR_ON 0x0010 /* send to stderr */
> +#define HLUA_TUNE_LOG_STDERR_AUTO 0x0020 /* send to stderr if no logger
> in use */
> +#define HLUA_TUNE_LOG_STDERR_OFF 0x0040 /* never send to stderr */
> +#define HLUA_TUNE_LOG_STDERR_MASK 0x0070
Please do not add flags for "off", and in general, flags are supposed to
be cumulable. By this I mean that above you could have ON|OFF|AUTO, which
will not make much sense in terms of configuration. The right approach
when you deal with multiple values like this for a same setting is to
group them within a bit mask as an enum. I also suggest that the OFF
value is the zero one, it simplifies lots of tests and more importantly
makes the code look more intuitive. Thus you would have for example:
#define HLUA_TUNE_LOG_STDERR_OFF 0x /* never send to stderr */
#define HLUA_TUNE_LOG_STDERR_ON 0x0010 /* send to stderr */
#define HLUA_TUNE_LOG_STDERR_AUTO 0x0020 /* send to stderr if no logger
in use */
#define HLUA_TUNE_LOG_STDERR_MASK 0x0030 /* mask to retrieve stderr
logging */
Then you'll read a value like this:
(hlua_tune_flags & HLUA_TUNE_LOG_STDERR_MASK) == HLUA_TUNE_LOG_STDERR_xxx
In order to set such a value when parsing the config, you can just do:
hlua_tune_flags &= ~HLUA_TUNE_LOG_STDERR_MASK;
hlua_tune_flags |= HLUA_TUNE_LOG_STDERR_xxx;
And I'm seeing later in your patch that it's exactly what you did for
these, which is fine.
Also please do not leave holes in bit fields; I know it's tempting to
leave room for later extensions but in the end it leaves more smaller
holes which are harder to fill, so you can safely regroup them on bits
1 and 2 (thus values 0,2,4,6).
> +/* default flag values */
> +#define HLUA_TUNE_FLAGS_DFLT 0x0011
This one can be quite a trap: if flags are later reordered, you can be
sure this one will not be updated. Better set the value based on the
field names.
> +/* flags made of HLUA_TUNE_* */
> +static uint hlua_tune_flags = HLUA_TUNE_FLAGS_DFLT;
> +
> /* Lua uses longjmp to perform yield or throwing errors. This
> * macro is used only for identifying the function that can
> * not return because a longjmp is executed.
> @@ -1366,8 +1380,9 @@ const char *hlua_show_current_location(const char *pfx)
> return NULL;
> }
>
> -/* This function is used to send logs. It try to send on screen (stderr)
> - * and on the default syslog server.
> +/* This function is used to send logs. It tries to send them to:
> + * - the log target applicable in the current context, AND
> + * - stderr if not in quiet mode or explicitly disabled
> */
> static inline void hlua_sendlog(struct proxy *px, int level, const char *msg)
> {
> @@ -1392,8 +1407,28 @@ static inline void hlua_sendlog(struct proxy *px, int
> level, const char *msg)
> }
> *p = '\0';
Re: [PATCH] MINOR: lua: Add a flag to disable logging to stderr
Following up on Aurélien's remarks (thanks for catching my forgetting
it!), here's an additional patch to update the LUA-specific documentation.
Could be kept standalone or merged into the first patch, but to avoid
re-submitting the patchset already, here it is standalone for now.
Tristan
From 67c3177c51044d288036044f7d17f6f037ac4f55 Mon Sep 17 00:00:00 2001
From: Tristan
Date: Fri, 20 Oct 2023 18:59:03 +0100
Subject: [PATCH] DOC: lua: update core.log and TXN.log documentation
Mention that their behaviour depends on the tunables
introduced in commit 45e6f27140 ("MINOR: lua: Add
flags to configure logging behaviour")
Rather than being entirely precise here and risk drift, simply
refer to the relevant tunables for details.
This should be backported wherever the aforementionned commit is
---
doc/lua-api/index.rst | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/doc/lua-api/index.rst b/doc/lua-api/index.rst
index 8a29bc5b5..9b9dcbd2c 100644
--- a/doc/lua-api/index.rst
+++ b/doc/lua-api/index.rst
@@ -267,8 +267,10 @@ Core class
**context**: body, init, task, action, sample-fetch, converter
This function sends a log. The log is sent, according with the HAProxy
- configuration file, on the default syslog server if it is configured and on
- the stderr if it is allowed.
+ configuration file, to the loggers relevant to the current context and/or
+ stderr.
+
+ The exact behaviour depends on tune.lua.log and tune.lua.log-stderr tunables.
:param integer loglevel: Is the log level associated with the message. It is
a
number between 0 and 7.
@@ -2648,8 +2650,10 @@ TXN class
.. js:function:: TXN.log(TXN, loglevel, msg)
This function sends a log. The log is sent, according with the HAProxy
- configuration file, on the default syslog server if it is configured and on
- the stderr if it is allowed.
+ configuration file, to the loggers relevant to the current context and/or
+ stderr.
+
+ The exact behaviour depends on tune.lua.log and tune.lua.log-stderr tunables.
:param class_txn txn: The class txn object containing the data.
:param integer loglevel: Is the log level associated with the message. It is
--
2.41.0
Re: [PATCH] MINOR: lua: Add a flag to disable logging to stderr
> https://www.arpalert.org/haproxy-api.html (related txn:log() > documentation: > https://www.arpalert.org/src/haproxy-lua-api/2.9/index.html#core.log) Forgot https://www.arpalert.org/src/haproxy-lua-api/2.9/index.html#TXN.log as well (both txn:log(), core:log() and friends with explicit log level in the name such as txn.Alert() end up using the hlua_sendlog() function internally)
Re: [PATCH] MINOR: lua: Add a flag to disable logging to stderr
Hi Tristan, Thanks for the nice work :) Just my 2 cents, in the second patch, since you change the default behavior, you forgot to update your comment from the 1st patch in Lua's doc according to the new behavior: > diff --git a/doc/lua.txt b/doc/lua.txt > index 8d5561668..8d244ab3a 100644 > --- a/doc/lua.txt > +++ b/doc/lua.txt > @@ -630,6 +630,10 @@ It displays a log during the HAProxy startup: > > [alert] 285/083533 (14465) : Hello World ! > > +Note: By default, logs originating from a LUA script are sent to the loggers > +applicable to the current context, if any, and additionally to stderr. See > +tune.lua.log and tune.lua.log-stderr for more information. Note that the lua.txt file is not used much now, and mainly serves as documentation for developers, not very useful for users. If you're willing to reach more people you should probably maintain the doc/lua-api/index.rst file which is used to generate the online documentation at https://www.arpalert.org/haproxy-api.html (related txn:log() documentation: https://www.arpalert.org/src/haproxy-lua-api/2.9/index.html#core.log) For the rest, I'll leave it to Willy :) Regards, Aurelien
Re: [PATCH] MINOR: lua: Add a flag to disable logging to stderr
Hi all again,
Here is the updated patch set after changes based on feedback received.
The change is now split across 2 patches.
Patch 0001 adding:
- tune.lua.log { on | off } (defaults to 'on') for usage of loggers
- tune.lua.log-stderr { on | auto | off } (defaults to 'on') for usage
of stderr
Effectively no change to current behaviour, hence fit for backport.
Patch 0002 changing:
- tune.lua.log-stderr default from 'on' to 'auto'
Changes behaviour, hence no backport.
I guess my only remaining question is whether we want regtests for this?
I'm be happy to write some if yes, but I know regtests runtime is a
concern and it'd require a bit of test infra work to make capturing
stderr output, so I just want to be sure it is desirable before :-)
Regards,
Tristan
From 45e6f271404d20479284fc3e0e1f7448e260e016 Mon Sep 17 00:00:00 2001
From: Tristan
Date: Fri, 20 Oct 2023 16:31:58 +0100
Subject: [PATCH] MINOR: lua: Add flags to configure logging behaviour
Until now, messages printed from LUA log functions were sent both to
the any logger configured for the current proxy, and additionally to
stderr (in most cases)
This introduces two flags to configure LUA log handling:
- tune.lua.log to use standard loggers or not
- tune.lua.log-stderr to use stderr, or not, or only conditionally
This addresses github feature request #2316
This can be backported to 2.8 as it doesn't change previous behaviour.
---
doc/configuration.txt | 22
doc/lua.txt | 4 +++
src/hlua.c| 80 +--
3 files changed, 103 insertions(+), 3 deletions(-)
diff --git a/doc/configuration.txt b/doc/configuration.txt
index 50fe882d0..7c4a585ec 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -1196,6 +1196,8 @@ The following keywords are supported in the "global"
section :
- tune.lua.service-timeout
- tune.lua.session-timeout
- tune.lua.task-timeout
+ - tune.lua.log
+ - tune.lua.log-stderr
- tune.max-checks-per-thread
- tune.maxaccept
- tune.maxpollevents
@@ -3192,6 +3194,26 @@ tune.lua.task-timeout
remain alive during of the lifetime of HAProxy. For example, a task used to
check servers.
+tune.lua.log { on | off }
+ Enables ('on') or disables ('off') logging the output of LUA scripts via the
+ loggers applicable to the current proxy, if any.
+
+ Defaults to 'on'.
+
+tune.lua.log-stderr { on | auto | off }
+ Enables ('on') or disables ('off') logging the output of LUA scripts via
+ stderr.
+ When set to 'auto', logging via stderr is conditionally 'on' if any of:
+
+- tune.lua.log is set to 'off'
+- the script is executed in a non-proxy context with no global logger
+- the script is executed in a proxy context with no logger attached
+
+ Please note that, when enabled, this logging is in addition to the logging
+ configured via tune.lua.log.
+
+ Defaults to 'on'.
+
tune.max-checks-per-thread
Sets the number of active checks per thread above which a thread will
actively try to search a less loaded thread to run the health check, or
diff --git a/doc/lua.txt b/doc/lua.txt
index 8d5561668..8d244ab3a 100644
--- a/doc/lua.txt
+++ b/doc/lua.txt
@@ -630,6 +630,10 @@ It displays a log during the HAProxy startup:
[alert] 285/083533 (14465) : Hello World !
+Note: By default, logs originating from a LUA script are sent to the loggers
+applicable to the current context, if any, and additionally to stderr. See
+tune.lua.log and tune.lua.log-stderr for more information.
+
Default path and libraries
--
diff --git a/src/hlua.c b/src/hlua.c
index e94325727..70a25e762 100644
--- a/src/hlua.c
+++ b/src/hlua.c
@@ -69,6 +69,20 @@
#include
#include
+/* Global LUA flags */
+/* log handling */
+#define HLUA_TUNE_LOG 0x0001 /* send to current logger */
+#define HLUA_TUNE_LOG_STDERR_ON 0x0010 /* send to stderr */
+#define HLUA_TUNE_LOG_STDERR_AUTO 0x0020 /* send to stderr if no logger in
use */
+#define HLUA_TUNE_LOG_STDERR_OFF 0x0040 /* never send to stderr */
+#define HLUA_TUNE_LOG_STDERR_MASK 0x0070
+
+/* default flag values */
+#define HLUA_TUNE_FLAGS_DFLT 0x0011
+
+/* flags made of HLUA_TUNE_* */
+static uint hlua_tune_flags = HLUA_TUNE_FLAGS_DFLT;
+
/* Lua uses longjmp to perform yield or throwing errors. This
* macro is used only for identifying the function that can
* not return because a longjmp is executed.
@@ -1366,8 +1380,9 @@ const char *hlua_show_current_location(const char *pfx)
return NULL;
}
-/* This function is used to send logs. It try to send on screen (stderr)
- * and on the default syslog server.
+/* This function is used to send logs. It tries to send them to:
+ * - the log target applicable in the current context, AND
+ * - stderr if not in quiet mode or explicitly disabled
*/
static inline void hlua_sendlog(struct proxy *px, int level, const char *msg)
{
@@ -1392,8 +1407,28 @@
Re: [PATCH] MINOR: lua: Add a flag to disable logging to stderr
On 20/10/2023 15:30, Tristan wrote:
ie in this snippet (hlua.c:1387):
static inline void hlua_sendlog(struct proxy *px, ...)
{
...
if (... && (!LIST_ISEMPTY(&px->loggers)))
return;
has the following results:
- locally from source => compiles happily
- locally from clone + patch => compile error
- in CI from clone + patch => compile error
When it fails, this is what I get back:
src/hlua.c:1417:86: error: ‘struct proxy’ has no member named ‘loggers’
1417 | if ((hlua_tune_flags & (HLUA_TUNE_LOG))
&& (!LIST_ISEMPTY(&px->loggers)))
| ^~
include/haproxy/list.h:102:28: note: in definition of macro ‘LIST_ISEMPTY’
102 | #define LIST_ISEMPTY(lh) ((lh)->n == (lh))
| ^~
Which would make sense if struct proxy had no such member, but it most
certainly has one...
I must be missing something obvious, which hopefully someone can point
out to me...
Well, Aurélien replied off-list and turns out my build scripts were not
using the same exact commit I was writing it against...
https://github.com/mangadex-pub/haproxy/commit/46c15afd27876c3e260fc0c70234e0aff70422de#r130521133
Thanks Aurélien, and :facepalm: to me
Tristan
Re: [PATCH] MINOR: lua: Add a flag to disable logging to stderr
Hi again Willy,
On 18/10/2023 07:47, Willy Tarreau wrote:
[...]
maybe we can have a 3rd value "auto" which would be the default
and which would only log to stderr if there's no other logger ? I
don't know if we have this info where it's used, though. Hmmm at
first glance we seem to have it by testing if either px->loggers
is non-empty or global.loggers is non-empty. Thus it could be the
nicest approach, having "on" by default in 2.8 and older and "auto"
on 2.9 maybe ?
I did go that route and thus split it into 2 patches (one for the flags
+ 'on' for that, and one that swaps from 'on' to 'auto').
Also from my reading of the config parser it seems to me like the global
loggers are always appended to the proxy's loggers if 'log global' is used.
So I opted to only check the proxy's loggers since I can see no
situations where one has 1) some global loggers and 2) a proxy that
expressly doesn't inherit log global and 3) nor any other proxy-specific
logger target.
Anyway, that small question aside, I'm seeing some pretty strange
discrepancy when compiling in different places.
ie in this snippet (hlua.c:1387):
static inline void hlua_sendlog(struct proxy *px, ...)
{
...
if (... && (!LIST_ISEMPTY(&px->loggers)))
return;
has the following results:
- locally from source => compiles happily
- locally from clone + patch => compile error
- in CI from clone + patch => compile error
When it fails, this is what I get back:
src/hlua.c:1417:86: error: ‘struct proxy’ has no member named ‘loggers’
1417 | if ((hlua_tune_flags & (HLUA_TUNE_LOG))
&& (!LIST_ISEMPTY(&px->loggers)))
|
^~
include/haproxy/list.h:102:28: note: in definition of macro ‘LIST_ISEMPTY’
102 | #define LIST_ISEMPTY(lh) ((lh)->n == (lh))
|^~
Which would make sense if struct proxy had no such member, but it most
certainly has one...
I must be missing something obvious, which hopefully someone can point
out to me...
Regards,
Tristan
nb: don't want to waste your time by forcing to review the patches
before I had a chance to properly test them, but if seeing them helps
more than the error, they are here atm:
https://github.com/mangadex-pub/haproxy/tree/93c92592bc693b53a27305152de80f544664f49d/haproxy/patches-dev
Re: [PATCH] MINOR: lua: Add a flag to disable logging to stderr
Hi Tristan, On Wed, Oct 18, 2023 at 04:25:47PM +, Tristan wrote: > > > > ... > > > One last thing, SEND_ERR() reports to stderr through ha_alert() and > > > hlua_sendlog() does it through fprintf(stderr, ) by appending a static > > > header containing the log level, the date and the pid: maybe we should > > > try to unify those outputs as well? > > > > I'm not sure anyone really *wants* to have to configure all that stuff > > for error reporting. Maybe the format is not great, maybe it should use > > send_log() etc, quite frankly I don't know as I have not visited that > > part. [...] > > To be honest here I'm somewhat on Aurélien's side. I would actually > appreciate a lua-log-format sort of configurable option. > > Mainly so I can prefix (for example) with the timestamp and request id, like > I do for HTTP traffic. > > Since raw output means it looks a bit odd in logs, and enforcing prefixes > inside scripts is a bit of a maintenance burden (and error prone + > expensive). So if it's a prefix it's not "log-format". I prefer to insist on this because "log-format" defines the payload, and there are other elements for the header format, such as the "format" keyword on the "log" line which lets one choose what the log's envelope will look like: http://docs.haproxy.org/dev/configuration.html#4.2-log%20global It supports "local", "rfc3164", "rfc5424", "priority", "short", "timed", "iso", "raw". Some have the date and pid, others not etc. Normally these are common to all logs emitted on the same target. Log-formats on the opposite use expressions that heavily depend on streams, such as request time, queue time, response time, frontend, backend, server used, server cookie, captures, request etc, which have nothing to do with an error or alert that Lua might want to emit. Willy
Re: [PATCH] MINOR: lua: Add a flag to disable logging to stderr
By the way, since we're talking about it, I think that actually it's not
logs that we're sending to stderr, it's Lua's alert messages that we're
sending both to stderr and logs.
Well I am no expert in what qualifies as "logs" vs "alerts", but the
messages sent from (for example) txn:Info() are included in this logic
(which is why I cared in the first place).
If that is an alert, then it's fine, but I would not have expected it.
So maybe it would be more convenient with these two choices:
tune.lua.alerts-log { on | off }
tune.lua.alerts-stderr { on | off }
Or with this single one:
tune.lua.alerts { off | stderr | log | both }
But please keep in mind the default that would change between 2.8 and 2.9
and that would need to be placed at the right location. The last one makes
it less easy in this regard.
I feel like the 2-different-flags approach is a little bit better here,
so I'll go with that. Since it also gives the option if ever needed to
relatively cleanly extend things later. For example:
- tune.lua.log { off | on [log $logger] [log-format $logfmt] }
- tune.lua.log-stderr { off | on | [min-level $level] }
Not that I necessarily plan either, but the first one would be pretty
neat, potentially.
Do what's easier for you. Really. Make sure the final version contains
a patch and a commit message that apply easily (either all inline or
attached, as you see fit). The only thing to avoid is to send multiple
series for review before getting feedback, that's extremely confusing,
but fortunately this almost never happens here.
Ok. Probably will reply to first message of this thread with 2 diff
files (one for 2.8 and one for 2.9 to account for default config
difference) then.
Tristan
Re: [PATCH] MINOR: lua: Add a flag to disable logging to stderr
On Wed, Oct 18, 2023 at 04:23:06PM +, Tristan wrote:
> Hi Willy,
>
>
> On 18/10/2023 07:47, Willy Tarreau wrote:
> > Hi Tristan,
> >
> > ...
> >
> > I'm fine with the general approach, but I'm having two comments:
> >
> >- using the word "also" in the option name is really not welcome
> > ("tune.lua.also-log-to-stderr"), and actually more confusing than
> > without because one might wonder "why also, where is it also sent".
>
> Funny you say that, because that's exactly what I intended by using "also".
> Since what is surprising to me about this behavior is less that it prints to
> stderr and more that it ADDITIONALLY prints to stderr. Which is why I
> thought emphasizing it made sense.
But that would be the *only* exception. For example the "log" option does
*add* logs, and it's not called "also-log". Here the principle is that we'd
disable it by default and if you want to enable stderr logging you'd enable
log-stderr (for example).
> > Most of our options are compatible and cumulable, and not exclusive,
> > so it should be seen as a boolean like any other one. As such I
> > would just call it "tune.lua.log-stderr" and be done with it. That
> > may even allow you not to resize the keywords array, which could
> > help with backports ;-)
>
> Not resizing the array is ideal ideed, but I'd argue that "log-stderr" is a
> little bit misleading, since it's really not a switch between two output
> streams, but rather a switch for a second one (stderr) being also written
> to.
That's precisely because it's not a switch that I find it more logical.
If it was a switch it would get the value after the name:
tune.lua.log { stderr | log }
If you make it a boolean, it doesn't exclude others and only concerns this
specific output:
tune.lua.log-stderr { on | off }
And if next year someone wants to send another patch to disable sending logs
from Lua they can produce another option which only handles that one without
making "also-stderr" bizarre.
By the way, since we're talking about it, I think that actually it's not
logs that we're sending to stderr, it's Lua's alert messages that we're
sending both to stderr and logs. So maybe it would be more convenient with
these two choices:
tune.lua.alerts-log { on | off }
tune.lua.alerts-stderr { on | off }
Or with this single one:
tune.lua.alerts { off | stderr | log | both }
But please keep in mind the default that would change between 2.8 and 2.9
and that would need to be placed at the right location. The last one makes
it less easy in this regard.
> Though frankly I don't care THAT much so if everyone's fine with it, I also
> am.
I don't care as much about the name as about the logic. It's extremely
difficult to find extensive options that will not conflict with new use
cases, so we always want to reserve some degrees of freedom for later
additions. We failed on some of them in the past (the log directive, the
mode in the proxies that can be put at the end, the almost-unordered
global section etc). And sometimes it really prevents new features from
being added.
> For the record, I agree it is ugly wording though,
No worries for ugly wording, for most of us it's not our native language
and some well-sounding terms just don't naturally come to our minds. It's
like this and that's also why discussing with others about names does help
a lot.
> but I looked for what
> similar flags existed and how other software defined them, and
> "alsologtostderr" seems to be the common approach these days (though imo
> such a flag shouldn't even exist in the first place).
No, we also try hard to preserve word delimitation. Those in a single
block are extremely old. I don't remember exactly but we used to have
some whose reading was ambiguous, that's when we stopped that ;-)
> > Or if
> > the concern is to lose the messages when no logs are configured,
> > maybe we can have a 3rd value "auto" which would be the default
> > and which would only log to stderr if there's no other logger ? I
> > don't know if we have this info where it's used, though. Hmmm at
> > first glance we seem to have it by testing if either px->loggers
> > is non-empty or global.loggers is non-empty. Thus it could be the
> > nicest approach, having "on" by default in 2.8 and older and "auto"
> > on 2.9 maybe ?
>
> I suppose that work too. I actually never ran HAProxy without any log config
> and thought defaults would be equivalent to:
>
> global
> log stderr format raw local0
>
> default
> log global
>
> (ie that at runtime logger fields were always set)
I agree that most users do have logs, but *some* do not (and start to
add them after their first issue report :-)).
> > A few comments on the patch:
> >
> > > [...]
> > >
> > > +static int hlua_tune_flags = HLUA_TUNE_ALSO_LOG_TO_STDERR;
> >
> > When you're using such arrays of flags, please precede them with a short
> > comm
Re: [PATCH] MINOR: lua: Add a flag to disable logging to stderr
... One last thing, SEND_ERR() reports to stderr through ha_alert() and hlua_sendlog() does it through fprintf(stderr, ) by appending a static header containing the log level, the date and the pid: maybe we should try to unify those outputs as well? I'm not sure anyone really *wants* to have to configure all that stuff for error reporting. Maybe the format is not great, maybe it should use send_log() etc, quite frankly I don't know as I have not visited that part. [...] To be honest here I'm somewhat on Aurélien's side. I would actually appreciate a lua-log-format sort of configurable option. Mainly so I can prefix (for example) with the timestamp and request id, like I do for HTTP traffic. Since raw output means it looks a bit odd in logs, and enforcing prefixes inside scripts is a bit of a maintenance burden (and error prone + expensive). That said, I'd definitely do that in a follow-up patch. Tristan
Re: [PATCH] MINOR: lua: Add a flag to disable logging to stderr
Hi Willy,
On 18/10/2023 07:47, Willy Tarreau wrote:
Hi Tristan,
...
I'm fine with the general approach, but I'm having two comments:
- using the word "also" in the option name is really not welcome
("tune.lua.also-log-to-stderr"), and actually more confusing than
without because one might wonder "why also, where is it also sent".
Funny you say that, because that's exactly what I intended by using
"also". Since what is surprising to me about this behavior is less that
it prints to stderr and more that it ADDITIONALLY prints to stderr.
Which is why I thought emphasizing it made sense.
Most of our options are compatible and cumulable, and not exclusive,
so it should be seen as a boolean like any other one. As such I
would just call it "tune.lua.log-stderr" and be done with it. That
may even allow you not to resize the keywords array, which could
help with backports ;-)
Not resizing the array is ideal ideed, but I'd argue that "log-stderr"
is a little bit misleading, since it's really not a switch between two
output streams, but rather a switch for a second one (stderr) being also
written to.
Though frankly I don't care THAT much so if everyone's fine with it, I
also am.
For the record, I agree it is ugly wording though, but I looked for what
similar flags existed and how other software defined them, and
"alsologtostderr" seems to be the common approach these days (though imo
such a flag shouldn't even exist in the first place).
- should we really stick to "on" as the default behavior in 2.9 ? I
I'd be up for that, but I expected to be told off. If no one objects I'm
more than happy to make it default. Or rather call it
"tune.lua.log-stderr" as you propose, and default that one to "off".
sense that basically nobody wants that by default anymore, and if
we want to change the default, it will only be in an odd version,
hence 3.1 for the next one. Maybe now's the right moment ? Or if
the concern is to lose the messages when no logs are configured,
maybe we can have a 3rd value "auto" which would be the default
and which would only log to stderr if there's no other logger ? I
don't know if we have this info where it's used, though. Hmmm at
first glance we seem to have it by testing if either px->loggers
is non-empty or global.loggers is non-empty. Thus it could be the
nicest approach, having "on" by default in 2.8 and older and "auto"
on 2.9 maybe ?
I suppose that work too. I actually never ran HAProxy without any log
config and thought defaults would be equivalent to:
global
log stderr format raw local0
default
log global
(ie that at runtime logger fields were always set)
A few comments on the patch:
[...]
+static int hlua_tune_flags = HLUA_TUNE_ALSO_LOG_TO_STDERR;
When you're using such arrays of flags, please precede them with a short
comment saying "flags made of HLUA_TUNE_*" or something like this so that
if it ever grows and more stuff gets inserted in between, it remains easy
to figure (that's one issue that has affected all other ones over time).
Fair point. Feared it'd be a bit repetitive with the comment just above,
but I see what you mean.
To be honest I wasn't even sure where in the file I should put it. Or if
it was preferable to add it in global.tune.options.
But I saw above that struct a comment saying it needed to be redone
"properly" so I figured I shouldn't add to it.
Also for bit fields, I'd prefer to use an unsigned int (we have "uint"
as a short form) so that when you see them in gdb you don't get negative
values that are even more annoying to copy-paste and decode.
Ah, sure.
Thought so too, but guessed that there was a reason for signed ints
since global.tune.options uses that.
+static int hlua_also_log_to_stderr(char **args, int section_type, struct proxy
*curpx,
+ const struct proxy *defpx, const char
*file, int line,
+ char **err)
It's not obvious at all that this function is there to parse a keyword,
I'm seeing it as the one which will actually log. Almost all of our
keyword parsing functions (others historically have "check"). I'm
seeing that it's not the case for the majority of the historic Lua
functions, but this should not be a reason for not fixing it (and
maxmem was fixed already). Better call it "hlua_parse_log_stderr" or
something like this that reminds the keyword.
Got it. Definitely agree, and only went that route for consistency at
first 👍
Please have a look at these points (or comment if you disagree), and
I'll happily merge it!
I'll follow up with the requested changes, however I'm not familiar with
patches over mailing lists. Should the updated patch be sent in its own
thread or as a reply of this with the diff file in attachment?
Tristan
Re: [PATCH] MINOR: lua: Add a flag to disable logging to stderr
Hi Aurélien, On Wed, Oct 18, 2023 at 09:32:19AM +0200, Aurelien DARRAGON wrote: > Hi Guys, > > I also have a suggestion, while at it: > > SEND_ERR() which is used to report unexpected Lua errors (because of > improper API usage, or due to external factors such as IO/memory issues) > currently does a stderr duplication as in hlua_sendlog() > > I'm thinking that it could be useful to make this configurable too, but > maybe independently from the txn:log() (hlua_sendlog()) logic since in > this case for errors we might be more interested in stderr reporting by > default instead of regular logging? I don't know. I tend to think that keeping technical errors (allocation, internal bugs etc) on stderr is not a bad thing, as they should never flood. However user manipulation errors, data processing errors and API errors (which can result from a bug in a lua script) should not be sent there by default. Thus I don't think we need an extra option, maybe we should just recheck which call places use SEND_ERR() and maybe some of them should just use ha_alert() without considering the option and other ones should consider the option. > One last thing, SEND_ERR() reports to stderr through ha_alert() and > hlua_sendlog() does it through fprintf(stderr, ) by appending a static > header containing the log level, the date and the pid: maybe we should > try to unify those outputs as well? I'm not sure anyone really *wants* to have to configure all that stuff for error reporting. Maybe the format is not great, maybe it should use send_log() etc, quite frankly I don't know as I have not visited that part. But my gut feeling is that it shouldbe as little specific as possible and use the standard features, mechanisms and formats that other subsystems already use. For example when a proxy is stopped it emits alerts, on stderr and in logs. I think that there's no reason Lua would use a different approach. Thanks, Willy
Re: [PATCH] MINOR: lua: Add a flag to disable logging to stderr
Hi Guys, I also have a suggestion, while at it: SEND_ERR() which is used to report unexpected Lua errors (because of improper API usage, or due to external factors such as IO/memory issues) currently does a stderr duplication as in hlua_sendlog() I'm thinking that it could be useful to make this configurable too, but maybe independently from the txn:log() (hlua_sendlog()) logic since in this case for errors we might be more interested in stderr reporting by default instead of regular logging? One last thing, SEND_ERR() reports to stderr through ha_alert() and hlua_sendlog() does it through fprintf(stderr, ) by appending a static header containing the log level, the date and the pid: maybe we should try to unify those outputs as well? Aurelien
Re: [PATCH] MINOR: lua: Add a flag to disable logging to stderr
Hi Tristan,
On Tue, Oct 17, 2023 at 06:19:57PM +, Tristan wrote:
> By default, messages printed from LUA log functions are sent both to
> the configured log target and additionally to stderr (in most cases).
> This introduces tune.lua.also-log-to-stderr for disabling that
> second copy of the message being sent to stderr.
>
> Addresses https://github.com/haproxy/haproxy/issues/2316
>
> This could be backported if wanted, since it preserves the behaviour
> that existed prior to it.
I agree with this, it has already annoyed me a few times in the past
when trying to debug something else. I have vague memories of the Lua
integration having been done with lots of fprintf() for debugging and
that finally it was cleaned up and considered good enough for a first
version. Since it was quite new by then, feedback and debugging were
expected so it was not a problem. But it has ossified into a default
behavior which is not longer desirable in my opinion.
I'm fine with the general approach, but I'm having two comments:
- using the word "also" in the option name is really not welcome
("tune.lua.also-log-to-stderr"), and actually more confusing than
without because one might wonder "why also, where is it also sent".
Most of our options are compatible and cumulable, and not exclusive,
so it should be seen as a boolean like any other one. As such I
would just call it "tune.lua.log-stderr" and be done with it. That
may even allow you not to resize the keywords array, which could
help with backports ;-)
- should we really stick to "on" as the default behavior in 2.9 ? I
sense that basically nobody wants that by default anymore, and if
we want to change the default, it will only be in an odd version,
hence 3.1 for the next one. Maybe now's the right moment ? Or if
the concern is to lose the messages when no logs are configured,
maybe we can have a 3rd value "auto" which would be the default
and which would only log to stderr if there's no other logger ? I
don't know if we have this info where it's used, though. Hmmm at
first glance we seem to have it by testing if either px->loggers
is non-empty or global.loggers is non-empty. Thus it could be the
nicest approach, having "on" by default in 2.8 and older and "auto"
on 2.9 maybe ?
A few comments on the patch:
> diff --git a/src/hlua.c b/src/hlua.c
> index c686f222a..261aee763 100644
> --- a/src/hlua.c
> +++ b/src/hlua.c
> @@ -69,6 +69,12 @@
> #include
> #include
>
> +/* Global LUA on/off flags */
> +/* if on, LUA-originating logs are duplicated to stderr */
> +#define HLUA_TUNE_ALSO_LOG_TO_STDERR (1<<0)
Please leave several spaces between the name and the value so that other
names do not require to realign everything. For settings, it tends to be
more convenient to use hex values (padded left, e.g. 0x0001) because
it allows masks and combined values to be represented as well in a visual
way, something that's basically unreadable when dealing with shifts. If
we implement "off", "auto", "on" here it will already be the case.
> +
> +static int hlua_tune_flags = HLUA_TUNE_ALSO_LOG_TO_STDERR;
When you're using such arrays of flags, please precede them with a short
comment saying "flags made of HLUA_TUNE_*" or something like this so that
if it ever grows and more stuff gets inserted in between, it remains easy
to figure (that's one issue that has affected all other ones over time).
Also for bit fields, I'd prefer to use an unsigned int (we have "uint"
as a short form) so that when you see them in gdb you don't get negative
values that are even more annoying to copy-paste and decode.
> +static int hlua_also_log_to_stderr(char **args, int section_type, struct
> proxy *curpx,
> + const struct proxy *defpx, const char
> *file, int line,
> + char **err)
It's not obvious at all that this function is there to parse a keyword,
I'm seeing it as the one which will actually log. Almost all of our
keyword parsing functions (others historically have "check"). I'm
seeing that it's not the case for the majority of the historic Lua
functions, but this should not be a reason for not fixing it (and
maxmem was fixed already). Better call it "hlua_parse_log_stderr" or
something like this that reminds the keyword.
Please have a look at these points (or comment if you disagree), and
I'll happily merge it!
Thanks,
Willy
