[Tarantool-patches] [PATCH v4 0/8] lua/log: add an ability to setup logger without box.cfg{}

[Tarantool-patches] [PATCH v4 0/8] lua/log: add an ability to setup logger without box.cfg{}

[Cyrill Gorcunov]

In the series we add an ability to configure logger early
without calling box.cfg{}. The syntax is the same as
in box.cfg{} call.

There was an idea to implement something similar via triggers but
I think this will require a way more efforts and code redesign,
so at first lets stick to simplier solution.

v2 by Oleg:
 - hide box_api symbols from users
 - initialize logger via log.cfg() call to look similar to box.cfg
v3 by Oleg:
 - add parametesr verification
 - allow to reconfig via log.cfg() call
v4 by Oleg:
 - lua style fixes
 - extent test (I didn't find a way to test wrong params verification
   inside tap test, so I did it manually. Still we might extent the
   test on top of the series).

branch gorcunov/gh-689-logger-4
issue https://github.com/tarantool/tarantool/issues/689

Cyrill Gorcunov (8):
  core/say: do not reconfig early set up logger
  lua/log: declare say_logger_init and say_logger_initialized
  lua/log: put string constants to map
  lua/log: do not allow to set json for boot logger
  lua/log: declare log as separate variable
  lua/log: use log module settings inside box.cfg
  lua/log: allow to configure logging without a box
  test: use direct log module

 src/box/lua/load_cfg.lua     |  24 +++-
 src/lib/core/say.c           |  13 +++
 src/lib/core/say.h           |   4 +
 src/lua/log.lua              | 216 +++++++++++++++++++++++++++++++----
 test/app-tap/logger.test.lua |  28 ++++-
 5 files changed, 255 insertions(+), 30 deletions(-)

-- 
2.26.2

[Tarantool-patches] [PATCH v4 1/8] core/say: do not reconfig early set up logger

[Cyrill Gorcunov]

We gonna support logger configuration on its own
without requirement to call `box.cfg{}`. Thus lets
say_logger_init() to skip processing if we already
did.

Note it is preparatory for next patches. Currently
the init called once.

Part-of #689

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/core/say.c | 13 +++++++++++++
 src/lib/core/say.h |  4 ++++
 2 files changed, 17 insertions(+)

diff --git a/src/lib/core/say.c b/src/lib/core/say.c
index e22089aac..d8f59fb9b 100644
--- a/src/lib/core/say.c
+++ b/src/lib/core/say.c
@@ -684,10 +684,23 @@ log_create(struct log *log, const char *init_str, int nonblock)
 	return 0;
 }
 
+bool
+say_logger_initialized(void)
+{
+	return log_default == &log_std;
+}
+
 void
 say_logger_init(const char *init_str, int level, int nonblock,
 		const char *format, int background)
 {
+	/*
+	 * The logger may be early configured
+	 * by hands without configuing the whole box.
+	 */
+	if (say_logger_initialized())
+		return;
+
 	if (log_create(&log_std, init_str, nonblock) < 0)
 		goto fail;
 
diff --git a/src/lib/core/say.h b/src/lib/core/say.h
index c50d7bbf4..857145465 100644
--- a/src/lib/core/say.h
+++ b/src/lib/core/say.h
@@ -274,6 +274,10 @@ say_logger_init(const char *init_str,
 		const char *log_format,
 		int background);
 
+/** Test if logger is initialized. */
+bool
+say_logger_initialized(void);
+
 /** Free default logger */
 void
 say_logger_free();
-- 
2.26.2

Re: [Tarantool-patches] [PATCH v4 1/8] core/say: do not reconfig early set up logger

[Oleg Babin]

Hi! Thanks for your patch. LGTM.

On 28/05/2020 13:07, Cyrill Gorcunov wrote:
> We gonna support logger configuration on its own
> without requirement to call `box.cfg{}`. Thus lets
> say_logger_init() to skip processing if we already
> did.
> 
> Note it is preparatory for next patches. Currently
> the init called once.
> 
> Part-of #689
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/lib/core/say.c | 13 +++++++++++++
>   src/lib/core/say.h |  4 ++++
>   2 files changed, 17 insertions(+)
> 
> diff --git a/src/lib/core/say.c b/src/lib/core/say.c
> index e22089aac..d8f59fb9b 100644
> --- a/src/lib/core/say.c
> +++ b/src/lib/core/say.c
> @@ -684,10 +684,23 @@ log_create(struct log *log, const char *init_str, int nonblock)
>   	return 0;
>   }
>   
> +bool
> +say_logger_initialized(void)
> +{
> +	return log_default == &log_std;
> +}
> +
>   void
>   say_logger_init(const char *init_str, int level, int nonblock,
>   		const char *format, int background)
>   {
> +	/*
> +	 * The logger may be early configured
> +	 * by hands without configuing the whole box.
> +	 */
> +	if (say_logger_initialized())
> +		return;
> +
>   	if (log_create(&log_std, init_str, nonblock) < 0)
>   		goto fail;
>   
> diff --git a/src/lib/core/say.h b/src/lib/core/say.h
> index c50d7bbf4..857145465 100644
> --- a/src/lib/core/say.h
> +++ b/src/lib/core/say.h
> @@ -274,6 +274,10 @@ say_logger_init(const char *init_str,
>   		const char *log_format,
>   		int background);
>   
> +/** Test if logger is initialized. */
> +bool
> +say_logger_initialized(void);
> +
>   /** Free default logger */
>   void
>   say_logger_free();
> 

Re: [Tarantool-patches] [PATCH v4 1/8] core/say: do not reconfig early set up logger

[lvasiliev]

Hi! Thank you for the patch.
I have a minor comment that can be skipped silently.

> diff --git a/src/lib/core/say.h b/src/lib/core/say.h
> index c50d7bbf4..857145465 100644
> --- a/src/lib/core/say.h
> +++ b/src/lib/core/say.h
> @@ -274,6 +274,10 @@ say_logger_init(const char *init_str,
>   		const char *log_format,
>   		int background);
>   
> +/** Test if logger is initialized. */
> +bool
> +say_logger_initialized(void);
> +

I think this function may be used for the check in say_logger_free()
instead log_default == &log_std.
>   /** Free default logger */
>   void
>   say_logger_free();
> 

Re: [Tarantool-patches] [PATCH v4 1/8] core/say: do not reconfig early set up logger

[Cyrill Gorcunov]

On Thu, May 28, 2020 at 01:42:01PM +0300, lvasiliev wrote:
> Hi! Thank you for the patch.
> I have a minor comment that can be skipped silently.
> 
> > diff --git a/src/lib/core/say.h b/src/lib/core/say.h
> > index c50d7bbf4..857145465 100644
> > --- a/src/lib/core/say.h
> > +++ b/src/lib/core/say.h
> > @@ -274,6 +274,10 @@ say_logger_init(const char *init_str,
> >   		const char *log_format,
> >   		int background);
> > +/** Test if logger is initialized. */
> > +bool
> > +say_logger_initialized(void);
> > +
> 
> I think this function may be used for the check in say_logger_free()
> instead log_default == &log_std.

True, I'll update. Thanks!

[Tarantool-patches] [PATCH v4 2/8] lua/log: declare say_logger_init and say_logger_initialized

[Cyrill Gorcunov]

We gonna use it to provide a way to initialize
logger subsystem early.

Part-of #689

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lua/log.lua | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/lua/log.lua b/src/lua/log.lua
index 312c14d5e..9830b0886 100644
--- a/src/lua/log.lua
+++ b/src/lua/log.lua
@@ -1,5 +1,7 @@
 -- log.lua
 --
+-- vim: ts=4 sw=4 et
+
 local ffi = require('ffi')
 ffi.cdef[[
     typedef void (*sayfunc_t)(int level, const char *filename, int line,
@@ -22,6 +24,12 @@ ffi.cdef[[
     void
     say_set_log_format(enum say_format format);
 
+    extern void
+    say_logger_init(const char *init_str, int level, int nonblock,
+                    const char *format, int background);
+
+    extern bool
+    say_logger_initialized(void);
 
     extern sayfunc_t _say;
     extern struct ev_loop;
-- 
2.26.2

Re: [Tarantool-patches] [PATCH v4 2/8] lua/log: declare say_logger_init and say_logger_initialized

[Oleg Babin]

Hi! Thanks for your patch. LGTM.

On 28/05/2020 13:07, Cyrill Gorcunov wrote:
> We gonna use it to provide a way to initialize
> logger subsystem early.
> 
> Part-of #689
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/lua/log.lua | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/src/lua/log.lua b/src/lua/log.lua
> index 312c14d5e..9830b0886 100644
> --- a/src/lua/log.lua
> +++ b/src/lua/log.lua
> @@ -1,5 +1,7 @@
>   -- log.lua
>   --
> +-- vim: ts=4 sw=4 et
> +
>   local ffi = require('ffi')
>   ffi.cdef[[
>       typedef void (*sayfunc_t)(int level, const char *filename, int line,
> @@ -22,6 +24,12 @@ ffi.cdef[[
>       void
>       say_set_log_format(enum say_format format);
>   
> +    extern void
> +    say_logger_init(const char *init_str, int level, int nonblock,
> +                    const char *format, int background);
> +
> +    extern bool
> +    say_logger_initialized(void);
>   
>       extern sayfunc_t _say;
>       extern struct ev_loop;
> 

Re: [Tarantool-patches] [PATCH v4 2/8] lua/log: declare say_logger_init and say_logger_initialized

[lvasiliev]

Hi! Thank you for the patch.
LGTM, except a comment below.

On 28.05.2020 13:07, Cyrill Gorcunov wrote:
> We gonna use it to provide a way to initialize
> logger subsystem early.
> 
> Part-of #689
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/lua/log.lua | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/src/lua/log.lua b/src/lua/log.lua
> index 312c14d5e..9830b0886 100644
> --- a/src/lua/log.lua
> +++ b/src/lua/log.lua
> @@ -1,5 +1,7 @@
>   -- log.lua
>   --
> +-- vim: ts=4 sw=4 et

Elders must decide whether or not to use vim ...

> +
>   local ffi = require('ffi')
>   ffi.cdef[[
>       typedef void (*sayfunc_t)(int level, const char *filename, int line,
> @@ -22,6 +24,12 @@ ffi.cdef[[
>       void
>       say_set_log_format(enum say_format format);
>   
> +    extern void
> +    say_logger_init(const char *init_str, int level, int nonblock,
> +                    const char *format, int background);
> +
> +    extern bool
> +    say_logger_initialized(void);
>   
>       extern sayfunc_t _say;
>       extern struct ev_loop;
> 

Re: [Tarantool-patches] [PATCH v4 2/8] lua/log: declare say_logger_init and say_logger_initialized

[Cyrill Gorcunov]

On Thu, May 28, 2020 at 02:12:26PM +0300, lvasiliev wrote:
> Hi! Thank you for the patch.
> LGTM, except a comment below.
> 
> On 28.05.2020 13:07, Cyrill Gorcunov wrote:
> > We gonna use it to provide a way to initialize
> > logger subsystem early.
> > 
> > Part-of #689
> > 
> > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> > ---
> >   src/lua/log.lua | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> > 
> > diff --git a/src/lua/log.lua b/src/lua/log.lua
> > index 312c14d5e..9830b0886 100644
> > --- a/src/lua/log.lua
> > +++ b/src/lua/log.lua
> > @@ -1,5 +1,7 @@
> >   -- log.lua
> >   --
> > +-- vim: ts=4 sw=4 et
> 
> Elders must decide whether or not to use vim ...

I'll drop it (since it put pants in fire fore some people ;)

[Tarantool-patches] [PATCH v4 3/8] lua/log: put string constants to map

[Cyrill Gorcunov]

This allows us to reuse them instead of copying
opencoded constants. They are highly bound to
ones compiled into the C code.

Part-of #689

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lua/log.lua | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/src/lua/log.lua b/src/lua/log.lua
index 9830b0886..d1d712cab 100644
--- a/src/lua/log.lua
+++ b/src/lua/log.lua
@@ -83,6 +83,20 @@ local special_fields = {
     "error_msg"
 }
 
+--
+-- Map format number to string.
+local fmt_num2str = {
+    [ffi.C.SF_PLAIN]    = "plain",
+    [ffi.C.SF_JSON]     = "json",
+}
+
+--
+-- Map format string to number.
+local fmt_str2num = {
+    ["plain"]           = ffi.C.SF_PLAIN,
+    ["json"]            = ffi.C.SF_JSON,
+}
+
 local function say(level, fmt, ...)
     if ffi.C.log_level < level then
         -- don't waste cycles on debug.getinfo()
@@ -104,7 +118,7 @@ local function say(level, fmt, ...)
         fmt = json.encode(fmt)
         if ffi.C.log_format == ffi.C.SF_JSON then
             -- indicate that message is already encoded in JSON
-            format = "json"
+            format = fmt_num2str[ffi.C.SF_JSON]
         end
     elseif type_fmt ~= 'string' then
         fmt = tostring(fmt)
@@ -135,16 +149,23 @@ local function log_level(level)
     return ffi.C.say_set_log_level(level)
 end
 
-local function log_format(format_name)
-    if format_name == "json" then
+local function log_format(name)
+    if not fmt_str2num[name] then
+        local keyset = {}
+        for k in pairs(fmt_str2num) do
+            keyset[#keyset + 1] = k
+        end
+        local m = "log_format: expected %s"
+        error(m:format(table.concat(keyset, ',')))
+    end
+
+    if fmt_str2num[name] == ffi.C.SF_JSON then
         if ffi.C.log_type() == ffi.C.SAY_LOGGER_SYSLOG then
             error("log_format: 'json' can't be used with syslog logger")
         end
         ffi.C.say_set_log_format(ffi.C.SF_JSON)
-    elseif format_name == "plain" then
-        ffi.C.say_set_log_format(ffi.C.SF_PLAIN)
     else
-        error("log_format: expected 'json' or 'plain'")
+        ffi.C.say_set_log_format(ffi.C.SF_PLAIN)
     end
 end
 
-- 
2.26.2

Re: [Tarantool-patches] [PATCH v4 3/8] lua/log: put string constants to map

[Oleg Babin]

Hi! Thanks for your patch. LGTM.

On 28/05/2020 13:07, Cyrill Gorcunov wrote:
> This allows us to reuse them instead of copying
> opencoded constants. They are highly bound to
> ones compiled into the C code.
> 
> Part-of #689
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/lua/log.lua | 33 +++++++++++++++++++++++++++------
>   1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/src/lua/log.lua b/src/lua/log.lua
> index 9830b0886..d1d712cab 100644
> --- a/src/lua/log.lua
> +++ b/src/lua/log.lua
> @@ -83,6 +83,20 @@ local special_fields = {
>       "error_msg"
>   }
>   
> +--
> +-- Map format number to string.
> +local fmt_num2str = {
> +    [ffi.C.SF_PLAIN]    = "plain",
> +    [ffi.C.SF_JSON]     = "json",
> +}
> +
> +--
> +-- Map format string to number.
> +local fmt_str2num = {
> +    ["plain"]           = ffi.C.SF_PLAIN,
> +    ["json"]            = ffi.C.SF_JSON,
> +}
> +
>   local function say(level, fmt, ...)
>       if ffi.C.log_level < level then
>           -- don't waste cycles on debug.getinfo()
> @@ -104,7 +118,7 @@ local function say(level, fmt, ...)
>           fmt = json.encode(fmt)
>           if ffi.C.log_format == ffi.C.SF_JSON then
>               -- indicate that message is already encoded in JSON
> -            format = "json"
> +            format = fmt_num2str[ffi.C.SF_JSON]
>           end
>       elseif type_fmt ~= 'string' then
>           fmt = tostring(fmt)
> @@ -135,16 +149,23 @@ local function log_level(level)
>       return ffi.C.say_set_log_level(level)
>   end
>   
> -local function log_format(format_name)
> -    if format_name == "json" then
> +local function log_format(name)
> +    if not fmt_str2num[name] then
> +        local keyset = {}
> +        for k in pairs(fmt_str2num) do
> +            keyset[#keyset + 1] = k
> +        end
> +        local m = "log_format: expected %s"
> +        error(m:format(table.concat(keyset, ',')))
> +    end
> +
> +    if fmt_str2num[name] == ffi.C.SF_JSON then
>           if ffi.C.log_type() == ffi.C.SAY_LOGGER_SYSLOG then
>               error("log_format: 'json' can't be used with syslog logger")
>           end
>           ffi.C.say_set_log_format(ffi.C.SF_JSON)
> -    elseif format_name == "plain" then
> -        ffi.C.say_set_log_format(ffi.C.SF_PLAIN)
>       else
> -        error("log_format: expected 'json' or 'plain'")
> +        ffi.C.say_set_log_format(ffi.C.SF_PLAIN)
>       end
>   end
>   
> 

Re: [Tarantool-patches] [PATCH v4 3/8] lua/log: put string constants to map

[lvasiliev]

Hi! Thank you for the patch.
LGTM.

[Tarantool-patches] [PATCH v4 4/8] lua/log: do not allow to set json for boot logger

[Cyrill Gorcunov]

For some reason we've missed that say_set_log_format
doesn't support json format not only for syslog but
for boottime logging as well.

Part-of #689

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lua/log.lua              | 7 +++++--
 test/app-tap/logger.test.lua | 1 -
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/lua/log.lua b/src/lua/log.lua
index d1d712cab..2491501f7 100644
--- a/src/lua/log.lua
+++ b/src/lua/log.lua
@@ -160,8 +160,11 @@ local function log_format(name)
     end
 
     if fmt_str2num[name] == ffi.C.SF_JSON then
-        if ffi.C.log_type() == ffi.C.SAY_LOGGER_SYSLOG then
-            error("log_format: 'json' can't be used with syslog logger")
+        if ffi.C.log_type() == ffi.C.SAY_LOGGER_SYSLOG or
+            ffi.C.log_type() == ffi.C.SAY_LOGGER_BOOT then
+            local m = "log_format: %s can't be used with " ..
+                    "syslog or boot-time logger"
+            error(m:format(fmt_num2str[ffi.C.SF_JSON]))
         end
         ffi.C.say_set_log_format(ffi.C.SF_JSON)
     else
diff --git a/test/app-tap/logger.test.lua b/test/app-tap/logger.test.lua
index 492d5ea0b..7bfa06e80 100755
--- a/test/app-tap/logger.test.lua
+++ b/test/app-tap/logger.test.lua
@@ -5,7 +5,6 @@ test:plan(24)
 
 -- gh-3946: Assertion failure when using log_format() before box.cfg()
 local log = require('log')
-log.log_format('json')
 log.log_format('plain')
 
 --
-- 
2.26.2

Re: [Tarantool-patches] [PATCH v4 4/8] lua/log: do not allow to set json for boot logger

[Oleg Babin]

Hi! Thanks for your patch see one comment below.

On 28/05/2020 13:07, Cyrill Gorcunov wrote:
> For some reason we've missed that say_set_log_format
> doesn't support json format not only for syslog but
> for boottime logging as well.
> 
> Part-of #689
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/lua/log.lua              | 7 +++++--
>   test/app-tap/logger.test.lua | 1 -
>   2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/src/lua/log.lua b/src/lua/log.lua
> index d1d712cab..2491501f7 100644
> --- a/src/lua/log.lua
> +++ b/src/lua/log.lua
> @@ -160,8 +160,11 @@ local function log_format(name)
>       end
>   
>       if fmt_str2num[name] == ffi.C.SF_JSON then
> -        if ffi.C.log_type() == ffi.C.SAY_LOGGER_SYSLOG then
> -            error("log_format: 'json' can't be used with syslog logger")
> +        if ffi.C.log_type() == ffi.C.SAY_LOGGER_SYSLOG or
> +            ffi.C.log_type() == ffi.C.SAY_LOGGER_BOOT then
> +            local m = "log_format: %s can't be used with " ..
> +                    "syslog or boot-time logger"
> +            error(m:format(fmt_num2str[ffi.C.SF_JSON]))
>           end
>           ffi.C.say_set_log_format(ffi.C.SF_JSON)
>       else
> diff --git a/test/app-tap/logger.test.lua b/test/app-tap/logger.test.lua
> index 492d5ea0b..7bfa06e80 100755
> --- a/test/app-tap/logger.test.lua
> +++ b/test/app-tap/logger.test.lua
> @@ -5,7 +5,6 @@ test:plan(24)
>   
>   -- gh-3946: Assertion failure when using log_format() before box.cfg()
>   local log = require('log')
> -log.log_format('json')

I think you can change it to "pcall(log.log_format, 'json')" and check 
that it returns an error. Not simply drop a test case.

>   log.log_format('plain')
>   
>   --
> 

Re: [Tarantool-patches] [PATCH v4 4/8] lua/log: do not allow to set json for boot logger

[Cyrill Gorcunov]

On Thu, May 28, 2020 at 01:40:25PM +0300, Oleg Babin wrote:
> Hi! Thanks for your patch see one comment below.
> 
> > diff --git a/test/app-tap/logger.test.lua b/test/app-tap/logger.test.lua
> > index 492d5ea0b..7bfa06e80 100755
> > --- a/test/app-tap/logger.test.lua
> > +++ b/test/app-tap/logger.test.lua
> > @@ -5,7 +5,6 @@ test:plan(24)
> >   -- gh-3946: Assertion failure when using log_format() before box.cfg()
> >   local log = require('log')
> > -log.log_format('json')
> 
> I think you can change it to "pcall(log.log_format, 'json')" and check that
> it returns an error. Not simply drop a test case.

Good catch! Thanks, will update!

Re: [Tarantool-patches] [PATCH v4 4/8] lua/log: do not allow to set json for boot logger

[lvasiliev]

Hi! Thank you for the patch.
See comment below.

On 28.05.2020 13:07, Cyrill Gorcunov wrote:
> For some reason we've missed that say_set_log_format
> doesn't support json format not only for syslog but
> for boottime logging as well.
> 
> Part-of #689
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/lua/log.lua              | 7 +++++--
>   test/app-tap/logger.test.lua | 1 -
>   2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/src/lua/log.lua b/src/lua/log.lua
> index d1d712cab..2491501f7 100644
> --- a/src/lua/log.lua
> +++ b/src/lua/log.lua
> @@ -160,8 +160,11 @@ local function log_format(name)
>       end
>   
>       if fmt_str2num[name] == ffi.C.SF_JSON then
> -        if ffi.C.log_type() == ffi.C.SAY_LOGGER_SYSLOG then
> -            error("log_format: 'json' can't be used with syslog logger")
> +        if ffi.C.log_type() == ffi.C.SAY_LOGGER_SYSLOG or
> +            ffi.C.log_type() == ffi.C.SAY_LOGGER_BOOT then
> +            local m = "log_format: %s can't be used with " ..
> +                    "syslog or boot-time logger" > +            error(m:format(fmt_num2str[ffi.C.SF_JSON]))
Why do you use the format if the message is always the same?
>           end
>           ffi.C.say_set_log_format(ffi.C.SF_JSON)
>       else
> diff --git a/test/app-tap/logger.test.lua b/test/app-tap/logger.test.lua
> index 492d5ea0b..7bfa06e80 100755
> --- a/test/app-tap/logger.test.lua
> +++ b/test/app-tap/logger.test.lua
> @@ -5,7 +5,6 @@ test:plan(24)
>   
>   -- gh-3946: Assertion failure when using log_format() before box.cfg()
>   local log = require('log')
> -log.log_format('json')
>   log.log_format('plain')
>   
>   --
> 

Re: [Tarantool-patches] [PATCH v4 4/8] lua/log: do not allow to set json for boot logger

[Cyrill Gorcunov]

On Thu, May 28, 2020 at 02:49:04PM +0300, lvasiliev wrote:
> > diff --git a/src/lua/log.lua b/src/lua/log.lua
> > index d1d712cab..2491501f7 100644
> > --- a/src/lua/log.lua
> > +++ b/src/lua/log.lua
> > @@ -160,8 +160,11 @@ local function log_format(name)
> >       end
> >       if fmt_str2num[name] == ffi.C.SF_JSON then
> > -        if ffi.C.log_type() == ffi.C.SAY_LOGGER_SYSLOG then
> > -            error("log_format: 'json' can't be used with syslog logger")
> > +        if ffi.C.log_type() == ffi.C.SAY_LOGGER_SYSLOG or
> > +            ffi.C.log_type() == ffi.C.SAY_LOGGER_BOOT then
> > +            local m = "log_format: %s can't be used with " ..
> > +                    "syslog or boot-time logger" > +            error(m:format(fmt_num2str[ffi.C.SF_JSON]))
> Why do you use the format if the message is always the same?

To eliminate opencoded 'json' constant.

[Tarantool-patches] [PATCH v4 5/8] lua/log: declare log as separate variable

[Cyrill Gorcunov]

We gonna hide some internal symbols in sake
of communication with box module, thus lets
make it clear.

Part-of #689

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lua/log.lua | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/src/lua/log.lua b/src/lua/log.lua
index 2491501f7..4b078d598 100644
--- a/src/lua/log.lua
+++ b/src/lua/log.lua
@@ -187,16 +187,20 @@ local compat_v16 = {
     end;
 }
 
-return setmetatable({
-    warn = say_closure(S_WARN);
-    info = say_closure(S_INFO);
-    verbose = say_closure(S_VERBOSE);
-    debug = say_closure(S_DEBUG);
-    error = say_closure(S_ERROR);
-    rotate = log_rotate;
-    pid = log_pid;
-    level = log_level;
-    log_format = log_format;
-}, {
+local log = {
+    warn = say_closure(S_WARN),
+    info = say_closure(S_INFO),
+    verbose = say_closure(S_VERBOSE),
+    debug = say_closure(S_DEBUG),
+    error = say_closure(S_ERROR),
+    rotate = log_rotate,
+    pid = log_pid,
+    level = log_level,
+    log_format = log_format,
+}
+
+setmetatable(log, {
     __index = compat_v16;
 })
+
+return log
-- 
2.26.2

Re: [Tarantool-patches] [PATCH v4 5/8] lua/log: declare log as separate variable

[Oleg Babin]

Hi! Thanks for your patch. LGTM.

On 28/05/2020 13:07, Cyrill Gorcunov wrote:
> We gonna hide some internal symbols in sake
> of communication with box module, thus lets
> make it clear.
> 
> Part-of #689
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/lua/log.lua | 26 +++++++++++++++-----------
>   1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/src/lua/log.lua b/src/lua/log.lua
> index 2491501f7..4b078d598 100644
> --- a/src/lua/log.lua
> +++ b/src/lua/log.lua
> @@ -187,16 +187,20 @@ local compat_v16 = {
>       end;
>   }
>   
> -return setmetatable({
> -    warn = say_closure(S_WARN);
> -    info = say_closure(S_INFO);
> -    verbose = say_closure(S_VERBOSE);
> -    debug = say_closure(S_DEBUG);
> -    error = say_closure(S_ERROR);
> -    rotate = log_rotate;
> -    pid = log_pid;
> -    level = log_level;
> -    log_format = log_format;
> -}, {
> +local log = {
> +    warn = say_closure(S_WARN),
> +    info = say_closure(S_INFO),
> +    verbose = say_closure(S_VERBOSE),
> +    debug = say_closure(S_DEBUG),
> +    error = say_closure(S_ERROR),
> +    rotate = log_rotate,
> +    pid = log_pid,
> +    level = log_level,
> +    log_format = log_format,
> +}
> +
> +setmetatable(log, {
>       __index = compat_v16;
>   })
> +
> +return log
> 

Re: [Tarantool-patches] [PATCH v4 5/8] lua/log: declare log as separate variable

[lvasiliev]

Hi! Thank you for the patch.
it's seems like the patch don't change anything, but actually the code
look more pretty. I don't mind. LGTM

[Tarantool-patches] [PATCH v4 6/8] lua/log: use log module settings inside box.cfg

[Cyrill Gorcunov]

Since we're going to implement early setup of
logging module we need both.

Part-of #689

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/lua/load_cfg.lua | 21 +++++++++++++++------
 src/lua/log.lua          | 37 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index 5d818addf..9aef12840 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -1,4 +1,6 @@
 -- load_cfg.lua - internal file
+--
+-- vim: ts=4 sw=4 et
 
 local log = require('log')
 local json = require('json')
@@ -59,10 +61,6 @@ local default_cfg = {
     vinyl_range_size          = nil, -- set automatically
     vinyl_page_size           = 8 * 1024,
     vinyl_bloom_fpr           = 0.05,
-    log                 = nil,
-    log_nonblock        = nil,
-    log_level           = 5,
-    log_format          = "plain",
     io_collect_interval = nil,
     readahead           = 16320,
     snap_io_rate_limit  = nil, -- no limit
@@ -233,8 +231,8 @@ end
 local dynamic_cfg = {
     listen                  = private.cfg_set_listen,
     replication             = private.cfg_set_replication,
-    log_level               = private.cfg_set_log_level,
-    log_format              = private.cfg_set_log_format,
+    log_level               = log.box_api.set_log_level,
+    log_format              = log.box_api.set_log_format,
     io_collect_interval     = private.cfg_set_io_collect_interval,
     readahead               = private.cfg_set_readahead,
     too_long_threshold      = private.cfg_set_too_long_threshold,
@@ -470,6 +468,16 @@ local function apply_default_cfg(cfg, default_cfg)
     end
 end
 
+local function apply_default_modules_cfg(cfg)
+    --
+    -- logging
+    for k,v in pairs(log.box_api.cfg) do
+        if cfg[k] == nil then
+            cfg[k] = v
+        end
+    end
+end
+
 -- Return true if two configurations are equivalent.
 local function compare_cfg(cfg1, cfg2)
     if type(cfg1) ~= type(cfg2) then
@@ -554,6 +562,7 @@ local function load_cfg(cfg)
     cfg = upgrade_cfg(cfg, translate_cfg)
     cfg = prepare_cfg(cfg, default_cfg, template_cfg, modify_cfg)
     apply_default_cfg(cfg, default_cfg);
+    apply_default_modules_cfg(cfg)
     -- Save new box.cfg
     box.cfg = cfg
     if not pcall(private.cfg_check)  then
diff --git a/src/lua/log.lua b/src/lua/log.lua
index 4b078d598..ce546f21e 100644
--- a/src/lua/log.lua
+++ b/src/lua/log.lua
@@ -97,6 +97,22 @@ local fmt_str2num = {
     ["json"]            = ffi.C.SF_JSON,
 }
 
+--
+-- Default options. The keys are part of
+-- user API, so change with caution.
+local default_cfg = {
+    log             = nil,
+    log_nonblock    = nil,
+    log_level       = S_INFO,
+    log_format      = fmt_num2str[ffi.C.SF_PLAIN],
+}
+
+local log_cfg = setmetatable(default_cfg, {
+    __newindex = function()
+        error('log: Attempt to modify a read-only table')
+    end,
+})
+
 local function say(level, fmt, ...)
     if ffi.C.log_level < level then
         -- don't waste cycles on debug.getinfo()
@@ -146,7 +162,8 @@ local function log_rotate()
 end
 
 local function log_level(level)
-    return ffi.C.say_set_log_level(level)
+    ffi.C.say_set_log_level(level)
+    rawset(log_cfg, 'log_level', level)
 end
 
 local function log_format(name)
@@ -170,6 +187,7 @@ local function log_format(name)
     else
         ffi.C.say_set_log_format(ffi.C.SF_PLAIN)
     end
+    rawset(log_cfg, 'log_format', name)
 end
 
 local function log_pid()
@@ -197,9 +215,26 @@ local log = {
     pid = log_pid,
     level = log_level,
     log_format = log_format,
+    --
+    -- Internal API to box module, not for users,
+    -- names can be changed.
+    box_api = {
+        cfg = log_cfg,
+        set_log_level = function()
+            log_level(box.cfg.log_level)
+        end,
+        set_log_format = function()
+            log_format(box.cfg.log_format)
+        end,
+    },
 }
 
 setmetatable(log, {
+    __serialize = function(self)
+        local res = table.copy(self)
+        res.box_api = nil
+        return setmetatable(res, {})
+    end,
     __index = compat_v16;
 })
 
-- 
2.26.2

Re: [Tarantool-patches] [PATCH v4 6/8] lua/log: use log module settings inside box.cfg

[Oleg Babin]

Hi! Thanks for your patch! Looks good but I still think that log 
prefixes are redundant. Is it possible to drop them?

E.g. yaml.cfg example (no "yaml_" prefixes):
```
tarantool> require('yaml').cfg
---
- encode_load_metatables: true
   encode_invalid_numbers: true
   encode_use_tostring: false
   decode_max_depth: 128
   encode_number_precision: 14
   encode_sparse_convert: true
   encode_max_depth: 128
   decode_invalid_numbers: true
   encode_sparse_ratio: 2
   encode_invalid_as_nil: false
   encode_sparse_safe: 10
   encode_deep_as_nil: false
   decode_save_metatables: true
...
```

```
tarantool> require('log').cfg
---
- log_format: json
   log_level: 1000
   log_nonblock: false
...
```

On 28/05/2020 13:07, Cyrill Gorcunov wrote:
> Since we're going to implement early setup of
> logging module we need both.
> 
> Part-of #689
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/box/lua/load_cfg.lua | 21 +++++++++++++++------
>   src/lua/log.lua          | 37 ++++++++++++++++++++++++++++++++++++-
>   2 files changed, 51 insertions(+), 7 deletions(-)
> 
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index 5d818addf..9aef12840 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -1,4 +1,6 @@
>   -- load_cfg.lua - internal file
> +--
> +-- vim: ts=4 sw=4 et
>   
>   local log = require('log')
>   local json = require('json')
> @@ -59,10 +61,6 @@ local default_cfg = {
>       vinyl_range_size          = nil, -- set automatically
>       vinyl_page_size           = 8 * 1024,
>       vinyl_bloom_fpr           = 0.05,
> -    log                 = nil,
> -    log_nonblock        = nil,
> -    log_level           = 5,
> -    log_format          = "plain",
>       io_collect_interval = nil,
>       readahead           = 16320,
>       snap_io_rate_limit  = nil, -- no limit
> @@ -233,8 +231,8 @@ end
>   local dynamic_cfg = {
>       listen                  = private.cfg_set_listen,
>       replication             = private.cfg_set_replication,
> -    log_level               = private.cfg_set_log_level,
> -    log_format              = private.cfg_set_log_format,
> +    log_level               = log.box_api.set_log_level,
> +    log_format              = log.box_api.set_log_format,
>       io_collect_interval     = private.cfg_set_io_collect_interval,
>       readahead               = private.cfg_set_readahead,
>       too_long_threshold      = private.cfg_set_too_long_threshold,
> @@ -470,6 +468,16 @@ local function apply_default_cfg(cfg, default_cfg)
>       end
>   end
>   
> +local function apply_default_modules_cfg(cfg)
> +    --
> +    -- logging
> +    for k,v in pairs(log.box_api.cfg) do
> +        if cfg[k] == nil then
> +            cfg[k] = v
> +        end
> +    end
> +end
> +
>   -- Return true if two configurations are equivalent.
>   local function compare_cfg(cfg1, cfg2)
>       if type(cfg1) ~= type(cfg2) then
> @@ -554,6 +562,7 @@ local function load_cfg(cfg)
>       cfg = upgrade_cfg(cfg, translate_cfg)
>       cfg = prepare_cfg(cfg, default_cfg, template_cfg, modify_cfg)
>       apply_default_cfg(cfg, default_cfg);
> +    apply_default_modules_cfg(cfg)
>       -- Save new box.cfg
>       box.cfg = cfg
>       if not pcall(private.cfg_check)  then
> diff --git a/src/lua/log.lua b/src/lua/log.lua
> index 4b078d598..ce546f21e 100644
> --- a/src/lua/log.lua
> +++ b/src/lua/log.lua
> @@ -97,6 +97,22 @@ local fmt_str2num = {
>       ["json"]            = ffi.C.SF_JSON,
>   }
>   
> +--
> +-- Default options. The keys are part of
> +-- user API, so change with caution.
> +local default_cfg = {
> +    log             = nil,
> +    log_nonblock    = nil,
> +    log_level       = S_INFO,
> +    log_format      = fmt_num2str[ffi.C.SF_PLAIN],
> +}
> +
> +local log_cfg = setmetatable(default_cfg, {
> +    __newindex = function()
> +        error('log: Attempt to modify a read-only table')
> +    end,
> +})
> +
>   local function say(level, fmt, ...)
>       if ffi.C.log_level < level then
>           -- don't waste cycles on debug.getinfo()
> @@ -146,7 +162,8 @@ local function log_rotate()
>   end
>   
>   local function log_level(level)
> -    return ffi.C.say_set_log_level(level)
> +    ffi.C.say_set_log_level(level)
> +    rawset(log_cfg, 'log_level', level)
>   end
>   
>   local function log_format(name)
> @@ -170,6 +187,7 @@ local function log_format(name)
>       else
>           ffi.C.say_set_log_format(ffi.C.SF_PLAIN)
>       end
> +    rawset(log_cfg, 'log_format', name)
>   end
>   
>   local function log_pid()
> @@ -197,9 +215,26 @@ local log = {
>       pid = log_pid,
>       level = log_level,
>       log_format = log_format,
> +    --
> +    -- Internal API to box module, not for users,
> +    -- names can be changed.
> +    box_api = {
> +        cfg = log_cfg,
> +        set_log_level = function()
> +            log_level(box.cfg.log_level)
> +        end,
> +        set_log_format = function()
> +            log_format(box.cfg.log_format)
> +        end,
> +    },
>   }
>   
>   setmetatable(log, {
> +    __serialize = function(self)
> +        local res = table.copy(self)
> +        res.box_api = nil
> +        return setmetatable(res, {})
> +    end,
>       __index = compat_v16;
>   })
>   
> 

Re: [Tarantool-patches] [PATCH v4 6/8] lua/log: use log module settings inside box.cfg

[Cyrill Gorcunov]

On Thu, May 28, 2020 at 01:41:54PM +0300, Oleg Babin wrote:
> Hi! Thanks for your patch! Looks good but I still think that log prefixes
> are redundant. Is it possible to drop them?
> 
> E.g. yaml.cfg example (no "yaml_" prefixes):
> ```
> tarantool> require('yaml').cfg
> ---
> - encode_load_metatables: true
>   encode_invalid_numbers: true
>   encode_use_tostring: false
>   decode_max_depth: 128
>   encode_number_precision: 14
>   encode_sparse_convert: true
>   encode_max_depth: 128
>   decode_invalid_numbers: true
>   encode_sparse_ratio: 2
>   encode_invalid_as_nil: false
>   encode_sparse_safe: 10
>   encode_deep_as_nil: false
>   decode_save_metatables: true
> ...
> ```
> 
> ```
> tarantool> require('log').cfg
> ---
> - log_format: json
>   log_level: 1000
>   log_nonblock: false
> ...
> ```

Somehow managed to miss this comment in on of yours
previous replies, sorry. I'll take a look.

Re: [Tarantool-patches] [PATCH v4 6/8] lua/log: use log module settings inside box.cfg

[lvasiliev]

Hi! Thank you for the patch.
See some comments below.

On 28.05.2020 13:07, Cyrill Gorcunov wrote:
> Since we're going to implement early setup of
> logging module we need both.
> 
> Part-of #689
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/box/lua/load_cfg.lua | 21 +++++++++++++++------
>   src/lua/log.lua          | 37 ++++++++++++++++++++++++++++++++++++-
>   2 files changed, 51 insertions(+), 7 deletions(-)
> 
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index 5d818addf..9aef12840 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -1,4 +1,6 @@
>   -- load_cfg.lua - internal file
> +--
> +-- vim: ts=4 sw=4 et
>   
>   local log = require('log')
>   local json = require('json')
> @@ -59,10 +61,6 @@ local default_cfg = {
>       vinyl_range_size          = nil, -- set automatically
>       vinyl_page_size           = 8 * 1024,
>       vinyl_bloom_fpr           = 0.05,
> -    log                 = nil,
> -    log_nonblock        = nil,
> -    log_level           = 5,
> -    log_format          = "plain",
>       io_collect_interval = nil,
>       readahead           = 16320,
>       snap_io_rate_limit  = nil, -- no limit
> @@ -233,8 +231,8 @@ end
>   local dynamic_cfg = {
>       listen                  = private.cfg_set_listen,
>       replication             = private.cfg_set_replication,
> -    log_level               = private.cfg_set_log_level,
> -    log_format              = private.cfg_set_log_format,
> +    log_level               = log.box_api.set_log_level,
> +    log_format              = log.box_api.set_log_format,
>       io_collect_interval     = private.cfg_set_io_collect_interval,
>       readahead               = private.cfg_set_readahead,
>       too_long_threshold      = private.cfg_set_too_long_threshold,
> @@ -470,6 +468,16 @@ local function apply_default_cfg(cfg, default_cfg)
>       end
>   end
>   
> +local function apply_default_modules_cfg(cfg)

Looks like apply_default_cfg(cfg, log.box_api.cfg).
Can we avoid code duplication?

> +    --
> +    -- logging
> +    for k,v in pairs(log.box_api.cfg) do

Add a space before v.

> +        if cfg[k] == nil then
> +            cfg[k] = v
> +        end
> +    end
> +end
> +
>   -- Return true if two configurations are equivalent.
>   local function compare_cfg(cfg1, cfg2)
>       if type(cfg1) ~= type(cfg2) then
> @@ -554,6 +562,7 @@ local function load_cfg(cfg)
>       cfg = upgrade_cfg(cfg, translate_cfg)
>       cfg = prepare_cfg(cfg, default_cfg, template_cfg, modify_cfg)
>       apply_default_cfg(cfg, default_cfg);
> +    apply_default_modules_cfg(cfg)
>       -- Save new box.cfg
>       box.cfg = cfg
>       if not pcall(private.cfg_check)  then
> diff --git a/src/lua/log.lua b/src/lua/log.lua
> index 4b078d598..ce546f21e 100644
> --- a/src/lua/log.lua
> +++ b/src/lua/log.lua
> @@ -97,6 +97,22 @@ local fmt_str2num = {
>       ["json"]            = ffi.C.SF_JSON,
>   }
>   
> +--
> +-- Default options. The keys are part of
> +-- user API, so change with caution.
> +local default_cfg = {
> +    log             = nil,
> +    log_nonblock    = nil,
> +    log_level       = S_INFO,
> +    log_format      = fmt_num2str[ffi.C.SF_PLAIN],
> +}
> +
> +local log_cfg = setmetatable(default_cfg, {
> +    __newindex = function()
> +        error('log: Attempt to modify a read-only table')
> +    end,
> +})
> +
>   local function say(level, fmt, ...)
>       if ffi.C.log_level < level then
>           -- don't waste cycles on debug.getinfo()
> @@ -146,7 +162,8 @@ local function log_rotate()
>   end
>   
>   local function log_level(level)
> -    return ffi.C.say_set_log_level(level)
> +    ffi.C.say_set_log_level(level)
> +    rawset(log_cfg, 'log_level', level)

Maybe I'm wrong, but seems like after that change the ffi.C.log_level
is deprecated (log_cfg.log_level can be used instead). But now you
keep 'level' in two places (ffi.C.log_level and log_cfg.log_level).

>   end
>   
>   local function log_format(name)
> @@ -170,6 +187,7 @@ local function log_format(name)
>       else
>           ffi.C.say_set_log_format(ffi.C.SF_PLAIN)
>       end
> +    rawset(log_cfg, 'log_format', name)

Same as for log_level ^.

>   end
>   
>   local function log_pid()
> @@ -197,9 +215,26 @@ local log = {
>       pid = log_pid,
>       level = log_level,
>       log_format = log_format,
> +    --
> +    -- Internal API to box module, not for users,
> +    -- names can be changed.
> +    box_api = {
> +        cfg = log_cfg,
> +        set_log_level = function()
> +            log_level(box.cfg.log_level)
> +        end,
> +        set_log_format = function()
> +            log_format(box.cfg.log_format)
> +        end,
> +    },
>   }
>   
>   setmetatable(log, {
> +    __serialize = function(self)

Can you add a test on __serizlize?

> +        local res = table.copy(self)
> +        res.box_api = nil
> +        return setmetatable(res, {})
> +    end,
>       __index = compat_v16;
>   })
>   
> 

Re: [Tarantool-patches] [PATCH v4 6/8] lua/log: use log module settings inside box.cfg

[Cyrill Gorcunov]

On Thu, May 28, 2020 at 08:07:02PM +0300, lvasiliev wrote:
> > +local function apply_default_modules_cfg(cfg)
> 
> Looks like apply_default_cfg(cfg, log.box_api.cfg).
> Can we avoid code duplication?

The problem is keys naming. The module may have own mapping.
For example in my latest version we use variables without
log_ prefix but for box module sake we provide a map

log.lua
=======

-- Name mapping from box to log module.
-- Make sure all required fields are covered!
local log2box_keys = {
    ['log']         = 'log',
    ['nonblock']    = 'log_nonblock',
    ['level']       = 'log_level',
    ['format']      = 'log_format',
}

-- Apply defaut config to the box module
local function box_cfg_apply_default(box_cfg)
    for k, v in pairs(log_cfg) do
        if box_cfg[log2box_keys[k]] == nil then
            box_cfg[log2box_keys[k]] = v
        end
    end
end

load_cfg.lua
============

-- Fetch default settings from modules.
local function apply_default_modules_cfg(cfg)
    log.box_api.cfg_apply_default(cfg)
end

This isolate module specifics from box variables as
it should be I believe.

> 
> > +    --
> > +    -- logging
> > +    for k,v in pairs(log.box_api.cfg) do
> 
> Add a space before v.

already reworked, thanks!

> > @@ -146,7 +162,8 @@ local function log_rotate()
> >   end
> >   local function log_level(level)
> > -    return ffi.C.say_set_log_level(level)
> > +    ffi.C.say_set_log_level(level)
> > +    rawset(log_cfg, 'log_level', level)
> 
> Maybe I'm wrong, but seems like after that change the ffi.C.log_level
> is deprecated (log_cfg.log_level can be used instead). But now you
> keep 'level' in two places (ffi.C.log_level and log_cfg.log_level).

Not sure I follow here. You mean to drop their usage in Lua's
"function say()" in this module?

> >   setmetatable(log, {
> > +    __serialize = function(self)
> 
> Can you add a test on __serizlize?

Good point, will do.

Re: [Tarantool-patches] [PATCH v4 6/8] lua/log: use log module settings inside box.cfg

[Leonid Vasiliev]

Hi!

On 28.05.2020 20:34, Cyrill Gorcunov wrote:
> On Thu, May 28, 2020 at 08:07:02PM +0300, lvasiliev wrote:
>>> +local function apply_default_modules_cfg(cfg)
>>
>> Looks like apply_default_cfg(cfg, log.box_api.cfg).
>> Can we avoid code duplication?
> 
> The problem is keys naming. The module may have own mapping.
> For example in my latest version we use variables without
> log_ prefix but for box module sake we provide a map
> 
> log.lua
> =======
> 
> -- Name mapping from box to log module.
> -- Make sure all required fields are covered!
> local log2box_keys = {
>      ['log']         = 'log',
>      ['nonblock']    = 'log_nonblock',
>      ['level']       = 'log_level',
>      ['format']      = 'log_format',
> }
> 
> -- Apply defaut config to the box module
> local function box_cfg_apply_default(box_cfg)
>      for k, v in pairs(log_cfg) do
>          if box_cfg[log2box_keys[k]] == nil then
>              box_cfg[log2box_keys[k]] = v
>          end
>      end
> end
> 
> load_cfg.lua
> ============
> 
> -- Fetch default settings from modules.
> local function apply_default_modules_cfg(cfg)
>      log.box_api.cfg_apply_default(cfg)
> end
> 
> This isolate module specifics from box variables as
> it should be I believe.
> 

Ok.

>>
>>> +    --
>>> +    -- logging
>>> +    for k,v in pairs(log.box_api.cfg) do
>>
>> Add a space before v.
> 
> already reworked, thanks!
> 
>>> @@ -146,7 +162,8 @@ local function log_rotate()
>>>    end
>>>    local function log_level(level)
>>> -    return ffi.C.say_set_log_level(level)
>>> +    ffi.C.say_set_log_level(level)
>>> +    rawset(log_cfg, 'log_level', level)
>>
>> Maybe I'm wrong, but seems like after that change the ffi.C.log_level
>> is deprecated (log_cfg.log_level can be used instead). But now you
>> keep 'level' in two places (ffi.C.log_level and log_cfg.log_level).
> 
> Not sure I follow here. You mean to drop their usage in Lua's
> "function say()" in this module?
> 

Yes.

>>>    setmetatable(log, {
>>> +    __serialize = function(self)
>>
>> Can you add a test on __serizlize?
> 
> Good point, will do.
> 

[Tarantool-patches] [PATCH v4 7/8] lua/log: allow to configure logging without a box

[Cyrill Gorcunov]

In the commit we allow to configure logger without
calling box.cfg{}.

Fixes #689

@TarantoolBot document
Title: Module log

Module log allows to setup logger early without calling ``box.cfg()``.
Configuration parameters are same as in ``box.cfg()`` call.

Example
```
> log = require('log')
> log.cfg({log='filename', log_format='plain', log_level=6})
...
> log.cfg({log_format='json', log_level=5})
```

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/lua/load_cfg.lua |  13 +++--
 src/lua/log.lua          | 117 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 119 insertions(+), 11 deletions(-)

diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index 9aef12840..00e424156 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -471,11 +471,13 @@ end
 local function apply_default_modules_cfg(cfg)
     --
     -- logging
-    for k,v in pairs(log.box_api.cfg) do
-        if cfg[k] == nil then
-            cfg[k] = v
-        end
-    end
+    log.box_api.cfg_apply_default(cfg)
+end
+
+local function update_modules_cfg(cfg)
+    --
+    -- logging
+    log.box_api.cfg_update(cfg)
 end
 
 -- Return true if two configurations are equivalent.
@@ -597,6 +599,7 @@ local function load_cfg(cfg)
             end
         end
     end
+    update_modules_cfg(cfg)
     if not box.cfg.read_only and not box.cfg.replication then
         box.schema.upgrade{auto = true}
     end
diff --git a/src/lua/log.lua b/src/lua/log.lua
index ce546f21e..31a80e18a 100644
--- a/src/lua/log.lua
+++ b/src/lua/log.lua
@@ -97,6 +97,14 @@ local fmt_str2num = {
     ["json"]            = ffi.C.SF_JSON,
 }
 
+local function fmt_list()
+    local keyset = {}
+    for k in pairs(fmt_str2num) do
+        keyset[#keyset + 1] = k
+    end
+    return table.concat(keyset, ',')
+end
+
 --
 -- Default options. The keys are part of
 -- user API, so change with caution.
@@ -162,18 +170,17 @@ local function log_rotate()
 end
 
 local function log_level(level)
+    if type(level) ~= 'number' then
+        error("log: 'level' (or 'log_level' option) expects a number")
+    end
     ffi.C.say_set_log_level(level)
     rawset(log_cfg, 'log_level', level)
 end
 
 local function log_format(name)
     if not fmt_str2num[name] then
-        local keyset = {}
-        for k in pairs(fmt_str2num) do
-            keyset[#keyset + 1] = k
-        end
         local m = "log_format: expected %s"
-        error(m:format(table.concat(keyset, ',')))
+        error(m:format(fmt_list()))
     end
 
     if fmt_str2num[name] == ffi.C.SF_JSON then
@@ -194,6 +201,100 @@ local function log_pid()
     return tonumber(ffi.C.log_pid)
 end
 
+local cfg_params_once = {
+    'log',
+    'log_nonblock',
+}
+
+--
+-- Setup dynamic parameters.
+local function dynamic_cfg(cfg)
+
+    for _, k in pairs(cfg_params_once) do
+        if cfg[k] ~= nil and cfg[k] ~= log_cfg[k] then
+            local m = "log: '%s' can't be set dynamically"
+            error(m:format(k))
+        end
+    end
+
+    if cfg.log_level ~= nil then
+        log_level(cfg.log_level)
+    end
+
+    if cfg.log_format ~= nil then
+        log_format(cfg.log_format)
+    end
+end
+
+--
+-- Load or reload configuration via log.cfg({})
+--
+-- The syntax is the same as for box.cfg({}).
+local function load_cfg(oldcfg, cfg)
+    cfg = cfg or {}
+
+    if cfg.log_format ~= nil then
+        if fmt_str2num[cfg.log_format] == nil then
+            local m = "log: 'log_format' must be %s"
+            error(m:format(fmt_list()))
+        end
+    end
+
+    if cfg.log_level ~= nil then
+        if type(cfg.log_level) ~= 'number' then
+            error("log: 'log_level' option must be a number")
+        end
+    end
+
+    if cfg.log_nonblock ~= nil then
+        if type(cfg.log_nonblock) ~= 'boolean' then
+            error("log: 'log_nonblock' option must be 'true' or 'false'")
+        end
+    end
+
+    if ffi.C.say_logger_initialized() == true then
+        return dynamic_cfg(cfg)
+    end
+
+    cfg.log_level = cfg.log_level or log_cfg.log_level
+    cfg.log_format = cfg.log_format or log_cfg.log_format
+    cfg.log_nonblock = cfg.log_nonblock or (log_cfg.log_nonblock or false)
+
+    --
+    -- We never allow confgure the logger in background
+    -- mode since we don't know how the box will be configured
+    -- later.
+    ffi.C.say_logger_init(cfg.log, cfg.log_level,
+                          cfg.log_nonblock, cfg.log_format, 0)
+
+    --
+    -- Update log_cfg vars to show them in module
+    -- configuration output.
+    rawset(log_cfg, 'log', cfg.log)
+    rawset(log_cfg, 'log_level', cfg.log_level)
+    rawset(log_cfg, 'log_nonblock', cfg.log_nonblock)
+    rawset(log_cfg, 'log_format', cfg.log_format)
+end
+
+--
+-- Apply defaut config to the box module
+local function box_cfg_apply_default(box_cfg)
+    for k, v in pairs(log_cfg) do
+        if box_cfg[k] == nil then
+            box_cfg[k] = v
+        end
+    end
+end
+
+--
+-- Reflect the changes made by box.cfg interface
+local function box_cfg_update(box_cfg)
+    rawset(log_cfg, 'log', box_cfg.log)
+    rawset(log_cfg, 'log_level', box_cfg.log_level)
+    rawset(log_cfg, 'log_nonblock', box_cfg.log_nonblock)
+    rawset(log_cfg, 'log_format', box_cfg.log_format)
+end
+
 local compat_warning_said = false
 local compat_v16 = {
     logger_pid = function()
@@ -215,17 +316,21 @@ local log = {
     pid = log_pid,
     level = log_level,
     log_format = log_format,
+    cfg = setmetatable(log_cfg, {
+        __call = load_cfg,
+    }),
     --
     -- Internal API to box module, not for users,
     -- names can be changed.
     box_api = {
-        cfg = log_cfg,
         set_log_level = function()
             log_level(box.cfg.log_level)
         end,
         set_log_format = function()
             log_format(box.cfg.log_format)
         end,
+        cfg_apply_default = box_cfg_apply_default,
+        cfg_update = box_cfg_update,
     },
 }
 
-- 
2.26.2

Re: [Tarantool-patches] [PATCH v4 7/8] lua/log: allow to configure logging without a box

[Oleg Babin]

Hi! Thanks for your patch. See one comment below.

On 28/05/2020 13:07, Cyrill Gorcunov wrote:
> In the commit we allow to configure logger without
> calling box.cfg{}.
> 
> Fixes #689
> 
> @TarantoolBot document
> Title: Module log
> 
> Module log allows to setup logger early without calling ``box.cfg()``.
> Configuration parameters are same as in ``box.cfg()`` call.
> 
> Example
> ```
>> log = require('log')
>> log.cfg({log='filename', log_format='plain', log_level=6})
> ...
>> log.cfg({log_format='json', log_level=5})
> ```
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/box/lua/load_cfg.lua |  13 +++--
>   src/lua/log.lua          | 117 +++++++++++++++++++++++++++++++++++++--
>   2 files changed, 119 insertions(+), 11 deletions(-)
> 
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index 9aef12840..00e424156 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -471,11 +471,13 @@ end
>   local function apply_default_modules_cfg(cfg)
>       --
>       -- logging
> -    for k,v in pairs(log.box_api.cfg) do
> -        if cfg[k] == nil then
> -            cfg[k] = v
> -        end
> -    end
> +    log.box_api.cfg_apply_default(cfg)
> +end
> +
> +local function update_modules_cfg(cfg)
> +    --
> +    -- logging
> +    log.box_api.cfg_update(cfg)
>   end
>   
>   -- Return true if two configurations are equivalent.
> @@ -597,6 +599,7 @@ local function load_cfg(cfg)
>               end
>           end
>       end
> +    update_modules_cfg(cfg)
>       if not box.cfg.read_only and not box.cfg.replication then
>           box.schema.upgrade{auto = true}
>       end
> diff --git a/src/lua/log.lua b/src/lua/log.lua
> index ce546f21e..31a80e18a 100644
> --- a/src/lua/log.lua
> +++ b/src/lua/log.lua
> @@ -97,6 +97,14 @@ local fmt_str2num = {
>       ["json"]            = ffi.C.SF_JSON,
>   }
>   
> +local function fmt_list()
> +    local keyset = {}
> +    for k in pairs(fmt_str2num) do
> +        keyset[#keyset + 1] = k
> +    end
> +    return table.concat(keyset, ',')
> +end
> +
>   --
>   -- Default options. The keys are part of
>   -- user API, so change with caution.
> @@ -162,18 +170,17 @@ local function log_rotate()
>   end
>   
>   local function log_level(level)
> +    if type(level) ~= 'number' then
> +        error("log: 'level' (or 'log_level' option) expects a number")
> +    end

Seems we could easily add an ability to specify log level as string.
log.cfg{log_level = 'info'} - it will be really more user-friendly than 
currently.

Also it's possible to specify "invalid" numbers:
```
tarantool> require('log').cfg({log_level = 0/0})
---
...

tarantool> require('log').cfg
---
- log_format: json
   log_level: nan
   log_nonblock: false
...
```
I think we could restict levels from 0/1 to 7.
All available log_levels are exposed as `enum say_level` and you can use 
it as you've done it for `say_format`s.

Re: [Tarantool-patches] [PATCH v4 7/8] lua/log: allow to configure logging without a box

[Leonid Vasiliev]

Hi! Thank you for the patch.
See some comments below.

On 28.05.2020 13:07, Cyrill Gorcunov wrote:
> In the commit we allow to configure logger without
> calling box.cfg{}.
> 
> Fixes #689
> 
> @TarantoolBot document
> Title: Module log
> 
> Module log allows to setup logger early without calling ``box.cfg()``.
> Configuration parameters are same as in ``box.cfg()`` call.
> 
> Example
> ```
>> log = require('log')
>> log.cfg({log='filename', log_format='plain', log_level=6})
> ...
>> log.cfg({log_format='json', log_level=5})
> ```
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/box/lua/load_cfg.lua |  13 +++--
>   src/lua/log.lua          | 117 +++++++++++++++++++++++++++++++++++++--
>   2 files changed, 119 insertions(+), 11 deletions(-)
> 
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index 9aef12840..00e424156 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -471,11 +471,13 @@ end
>   local function apply_default_modules_cfg(cfg)
>       --
>       -- logging
> -    for k,v in pairs(log.box_api.cfg) do
> -        if cfg[k] == nil then
> -            cfg[k] = v
> -        end
> -    end
> +    log.box_api.cfg_apply_default(cfg)
> +end
> +
> +local function update_modules_cfg(cfg)
> +    --
> +    -- logging
> +    log.box_api.cfg_update(cfg)
>   end
>   
>   -- Return true if two configurations are equivalent.
> @@ -597,6 +599,7 @@ local function load_cfg(cfg)
>               end
>           end
>       end
> +    update_modules_cfg(cfg)
>       if not box.cfg.read_only and not box.cfg.replication then
>           box.schema.upgrade{auto = true}
>       end
> diff --git a/src/lua/log.lua b/src/lua/log.lua
> index ce546f21e..31a80e18a 100644
> --- a/src/lua/log.lua
> +++ b/src/lua/log.lua
> @@ -97,6 +97,14 @@ local fmt_str2num = {
>       ["json"]            = ffi.C.SF_JSON,
>   }
>   
> +local function fmt_list()
> +    local keyset = {}
> +    for k in pairs(fmt_str2num) do
> +        keyset[#keyset + 1] = k
> +    end
> +    return table.concat(keyset, ',')
> +end

Can this be moved to "lua/log: put string constants to map"?

> +
>   --
>   -- Default options. The keys are part of
>   -- user API, so change with caution.
> @@ -162,18 +170,17 @@ local function log_rotate()
>   end
>   
>   local function log_level(level)
> +    if type(level) ~= 'number' then
> +        error("log: 'level' (or 'log_level' option) expects a number")
> +    end
>       ffi.C.say_set_log_level(level)
>       rawset(log_cfg, 'log_level', level)
>   end
>   
>   local function log_format(name)
>       if not fmt_str2num[name] then
> -        local keyset = {}
> -        for k in pairs(fmt_str2num) do
> -            keyset[#keyset + 1] = k
> -        end
>           local m = "log_format: expected %s"
> -        error(m:format(table.concat(keyset, ',')))
> +        error(m:format(fmt_list()))
>       end
>   
>       if fmt_str2num[name] == ffi.C.SF_JSON then
> @@ -194,6 +201,100 @@ local function log_pid()
>       return tonumber(ffi.C.log_pid)
>   end
>   
> +local cfg_params_once = {
> +    'log',
> +    'log_nonblock',
> +}
> +
> +--
> +-- Setup dynamic parameters.
> +local function dynamic_cfg(cfg)
> +
> +    for _, k in pairs(cfg_params_once) do
> +        if cfg[k] ~= nil and cfg[k] ~= log_cfg[k] then
> +            local m = "log: '%s' can't be set dynamically"
> +            error(m:format(k))
> +        end
> +    end
> +
> +    if cfg.log_level ~= nil then
> +        log_level(cfg.log_level)
> +    end
> +
> +    if cfg.log_format ~= nil then
> +        log_format(cfg.log_format)
> +    end
> +end
> +
> +--
> +-- Load or reload configuration via log.cfg({})
> +--
> +-- The syntax is the same as for box.cfg({}).
> +local function load_cfg(oldcfg, cfg)

oldcfg - unused argument. I think Sergey will be upset.
https://github.com/tarantool/tarantool/issues/4681

> +    cfg = cfg or {}
> +
> +    if cfg.log_format ~= nil then
> +        if fmt_str2num[cfg.log_format] == nil then
> +            local m = "log: 'log_format' must be %s"
> +            error(m:format(fmt_list()))
> +        end
> +    end
> +
> +    if cfg.log_level ~= nil then
> +        if type(cfg.log_level) ~= 'number' then
> +            error("log: 'log_level' option must be a number")
> +        end
> +    end
> +
> +    if cfg.log_nonblock ~= nil then
> +        if type(cfg.log_nonblock) ~= 'boolean' then
> +            error("log: 'log_nonblock' option must be 'true' or 'false'")
> +        end
> +    end
> +
> +    if ffi.C.say_logger_initialized() == true then
> +        return dynamic_cfg(cfg)
> +    end
> +
> +    cfg.log_level = cfg.log_level or log_cfg.log_level
> +    cfg.log_format = cfg.log_format or log_cfg.log_format
> +    cfg.log_nonblock = cfg.log_nonblock or (log_cfg.log_nonblock or false)
> +
> +    --
> +    -- We never allow confgure the logger in background
> +    -- mode since we don't know how the box will be configured
> +    -- later.
> +    ffi.C.say_logger_init(cfg.log, cfg.log_level,
> +                          cfg.log_nonblock, cfg.log_format, 0)
> +
> +    --
> +    -- Update log_cfg vars to show them in module
> +    -- configuration output.
> +    rawset(log_cfg, 'log', cfg.log)
> +    rawset(log_cfg, 'log_level', cfg.log_level)
> +    rawset(log_cfg, 'log_nonblock', cfg.log_nonblock)
> +    rawset(log_cfg, 'log_format', cfg.log_format)
> +end
> +
> +--
> +-- Apply defaut config to the box module
> +local function box_cfg_apply_default(box_cfg)
> +    for k, v in pairs(log_cfg) do
> +        if box_cfg[k] == nil then
> +            box_cfg[k] = v
> +        end
> +    end
> +end
> +
> +--
> +-- Reflect the changes made by box.cfg interface
> +local function box_cfg_update(box_cfg)
> +    rawset(log_cfg, 'log', box_cfg.log)
> +    rawset(log_cfg, 'log_level', box_cfg.log_level)
> +    rawset(log_cfg, 'log_nonblock', box_cfg.log_nonblock)
> +    rawset(log_cfg, 'log_format', box_cfg.log_format)
> +end
> +
>   local compat_warning_said = false
>   local compat_v16 = {
>       logger_pid = function()
> @@ -215,17 +316,21 @@ local log = {
>       pid = log_pid,
>       level = log_level,
>       log_format = log_format,
> +    cfg = setmetatable(log_cfg, {
> +        __call = load_cfg,
> +    }),
>       --
>       -- Internal API to box module, not for users,
>       -- names can be changed.
>       box_api = {
> -        cfg = log_cfg,
>           set_log_level = function()
>               log_level(box.cfg.log_level)
>           end,
>           set_log_format = function()
>               log_format(box.cfg.log_format)
>           end,
> +        cfg_apply_default = box_cfg_apply_default,
> +        cfg_update = box_cfg_update,
>       },

Why should we have a special box_module API?
Maybe we can do all box.cfg dirty stuff in load_cfg.lua.
>   }
>   
> 
Is it ok and we shouldn't use log.cfg() after box.cfg()?

└──╼ tarantool 
                                                               [70/1585]
Tarantool 2.5.0-90-g39a946b
type 'help' for interactive help
tarantool> log = require("log")
---
...

tarantool> box.cfg{log_level = 3}
2020-05-29 11:33:09.901 [6926] main/103/interactive C> Tarantool 
2.5.0-90-g39a946b
2020-05-29 11:33:09.901 [6926] main/103/interactive C> log level 3
2020-05-29 11:33:09.974 [6926] main/103/interactive C> leaving orphan mode
---
...

tarantool> log.cfg{log_level = 2}
---
...

tarantool> box.cfg.log_level
---
- 3
...

tarantool> log.cfg.log_level
---
- 2
...

Re: [Tarantool-patches] [PATCH v4 7/8] lua/log: allow to configure logging without a box

[Oleg Babin]

Hi! It's ok I think. box.cfg table stores user-defined values.
But it's a good point. There is an example of similar behaviour[1]:

```
tarantool> box.cfg{listen = 0}
---
...

tarantool> box.cfg.listen
---
- '0'
...

tarantool> box.info.listen
---
- '[::]:64991'
...
```

box.cfg.listen is "0" because you acually specify it by hands. 
box.info.listen is real value of property.

My point in such case don't try to support consistency between box.cfg 
and log.cfg. But:
   - log.cfg should always store an actual value of parameters
   - box.info (maybe, but not box.cfg) should also do it.

I hope it's appropriate stay it as is but file an issue to fix such it 
in future. It won't breaking change for existing installations (that 
doesn't use log.cfg{})

[1] https://github.com/tarantool/tarantool/issues/4620

On 29/05/2020 11:41, Leonid Vasiliev wrote:
 > Hi! Thank you for the patch.
 > See some comments below.

...

 >
 > tarantool> log.cfg{log_level = 2}
 > ---
 > ...
 >
 > tarantool> box.cfg.log_level
 > ---
 > - 3
 > ...
 >
 > tarantool> log.cfg.log_level
 > ---
 > - 2
 > ...

Re: [Tarantool-patches] [PATCH v4 7/8] lua/log: allow to configure logging without a box

[Leonid Vasiliev]

Hi!
Maybe we need to apply something like the "observer" pattern.
When changes in "log" occur, it alerts subscribers.

On 29.05.2020 11:53, Oleg Babin wrote:
> Hi! It's ok I think. box.cfg table stores user-defined values.
> But it's a good point. There is an example of similar behaviour[1]:
> 
> ```
> tarantool> box.cfg{listen = 0}
> ---
> ...
> 
> tarantool> box.cfg.listen
> ---
> - '0'
> ...
> 
> tarantool> box.info.listen
> ---
> - '[::]:64991'
> ...
> ```
> 
> box.cfg.listen is "0" because you acually specify it by hands. 
> box.info.listen is real value of property.
> 
> My point in such case don't try to support consistency between box.cfg 
> and log.cfg. But:
>    - log.cfg should always store an actual value of parameters
>    - box.info (maybe, but not box.cfg) should also do it.
> 
> I hope it's appropriate stay it as is but file an issue to fix such it 
> in future. It won't breaking change for existing installations (that 
> doesn't use log.cfg{})
> 
> [1] https://github.com/tarantool/tarantool/issues/4620
> 
> On 29/05/2020 11:41, Leonid Vasiliev wrote:
>  > Hi! Thank you for the patch.
>  > See some comments below.
> 
> ...
> 
>  >
>  > tarantool> log.cfg{log_level = 2}
>  > ---
>  > ...
>  >
>  > tarantool> box.cfg.log_level
>  > ---
>  > - 3
>  > ...
>  >
>  > tarantool> log.cfg.log_level
>  > ---
>  > - 2
>  > ...

Re: [Tarantool-patches] [PATCH v4 7/8] lua/log: allow to configure logging without a box

[Cyrill Gorcunov]

On Fri, May 29, 2020 at 12:16:08PM +0300, Leonid Vasiliev wrote:
> Hi!
> Maybe we need to apply something like the "observer" pattern.
> When changes in "log" occur, it alerts subscribers.

Seriously, guys, I'm not going to bring some watchdog here,
at least in this series 'cause it become over the bounds already.

I agree that we need a general solution with proper notification
queues and etc but lets postpone it for better times. I think
box_api interface is enough for now. It might be not that elegant
as it could though. After all we will be able to easily extend it
on top.

I'm preparing the v5 now and will try to address all your comments.
At least most of them :-)

Re: [Tarantool-patches] [PATCH v4 7/8] lua/log: allow to configure logging without a box

[Oleg Babin]

I completely agree with you.
We shouldn't overcomplicate code for such simple goal.

On 29/05/2020 12:49, Cyrill Gorcunov wrote:
> On Fri, May 29, 2020 at 12:16:08PM +0300, Leonid Vasiliev wrote:
>> Hi!
>> Maybe we need to apply something like the "observer" pattern.
>> When changes in "log" occur, it alerts subscribers.
> 
> Seriously, guys, I'm not going to bring some watchdog here,
> at least in this series 'cause it become over the bounds already.
> 
> I agree that we need a general solution with proper notification
> queues and etc but lets postpone it for better times. I think
> box_api interface is enough for now. It might be not that elegant
> as it could though. After all we will be able to easily extend it
> on top.
> 
> I'm preparing the v5 now and will try to address all your comments.
> At least most of them :-)
> 

Re: [Tarantool-patches] [PATCH v4 7/8] lua/log: allow to configure logging without a box

[Leonid Vasiliev]

I don't object, but why it can't be implemented by simple callback (we 
have only one recipient), which will be set in load_cfg.lua and called 
in case of updating.

Perhaps, we can don't copy the value to the box.cfg and always return 
the value from "log"?

On 29.05.2020 12:49, Cyrill Gorcunov wrote:
> On Fri, May 29, 2020 at 12:16:08PM +0300, Leonid Vasiliev wrote:
>> Hi!
>> Maybe we need to apply something like the "observer" pattern.
>> When changes in "log" occur, it alerts subscribers.
> 
> Seriously, guys, I'm not going to bring some watchdog here,
> at least in this series 'cause it become over the bounds already.
> 
> I agree that we need a general solution with proper notification
> queues and etc but lets postpone it for better times. I think
> box_api interface is enough for now. It might be not that elegant
> as it could though. After all we will be able to easily extend it
> on top.
> 
> I'm preparing the v5 now and will try to address all your comments.
> At least most of them :-)
> 

Re: [Tarantool-patches] [PATCH v4 7/8] lua/log: allow to configure logging without a box

[Cyrill Gorcunov]

On Fri, May 29, 2020 at 01:22:51PM +0300, Leonid Vasiliev wrote:
> I don't object, but why it can't be implemented by simple callback (we have
> only one recipient), which will be set in load_cfg.lua and called in case of
> updating.

Is not it the same as we do now?

box.load_cfg
  apply_default_modules_cfg
    log.box_api.cfg_apply_default(cfg)

box.reload_cfg
  update_modules_cfg
    log.box_api.cfg_update

The thing is that if we create some kind of geneal callbacks
we need to provide that named "stages" and hook-in modules
when they're loaded in.

If you have some other code in mind please show it as a pseudocode

> 
> Perhaps, we can don't copy the value to the box.cfg and always return the
> value from "log"?

Re: [Tarantool-patches] [PATCH v4 7/8] lua/log: allow to configure logging without a box

[Leonid Vasiliev]

On 29.05.2020 13:38, Cyrill Gorcunov wrote:
> On Fri, May 29, 2020 at 01:22:51PM +0300, Leonid Vasiliev wrote:
>> I don't object, but why it can't be implemented by simple callback (we have
>> only one recipient), which will be set in load_cfg.lua and called in case of
>> updating.
> 
> Is not it the same as we do now?
> 
> box.load_cfg
>    apply_default_modules_cfg
>      log.box_api.cfg_apply_default(cfg)
> 
> box.reload_cfg
>    update_modules_cfg
>      log.box_api.cfg_update
> 
> The thing is that if we create some kind of geneal callbacks
> we need to provide that named "stages" and hook-in modules
> when they're loaded in.
> 
> If you have some other code in mind please show it as a pseudocode
> 

I mean something like that:
in load_cfg.lua:

     local function on_log_module_update()
         -- set updated values to box.cfg
         ....
     end

     local function load_cfg()
         ....
         log.cfg.on_log_module_update = on_log_module_update
         ...
     end

in log.lua:

     local function load_cfg()
         ...
         -- Check is callback set.
         if log_cfg['on_log_module_update'] then
             log_cfg.on_log_module_update()
         end
         ...
     end

Re: [Tarantool-patches] [PATCH v4 7/8] lua/log: allow to configure logging without a box

[Cyrill Gorcunov]

OK, I see what you mean. But regular load and reload are
different. That's why I want to keep the helpers in box_api.
Leonid, lets make a deal -- I'll finish fixing my _current_
implementation and then we will try to make a patches on top.
Sounds ok?

Re: [Tarantool-patches] [PATCH v4 7/8] lua/log: allow to configure logging without a box

[Leonid Vasiliev]

Ok

On 29.05.2020 14:32, Cyrill Gorcunov wrote:
> OK, I see what you mean. But regular load and reload are
> different. That's why I want to keep the helpers in box_api.
> Leonid, lets make a deal -- I'll finish fixing my _current_
> implementation and then we will try to make a patches on top.
> Sounds ok?
> 

Re: [Tarantool-patches] [PATCH v4 7/8] lua/log: allow to configure logging without a box

[Cyrill Gorcunov]

On Fri, May 29, 2020 at 11:41:41AM +0300, Leonid Vasiliev wrote:
> > +local function fmt_list()
> > +    local keyset = {}
> > +    for k in pairs(fmt_str2num) do
> > +        keyset[#keyset + 1] = k
> > +    end
> > +    return table.concat(keyset, ',')
> > +end
> 
> Can this be moved to "lua/log: put string constants to map"?

It will be unused there because only in this patch we start
to print the complete list.

> > +--
> > +-- Load or reload configuration via log.cfg({})
> > +--
> > +-- The syntax is the same as for box.cfg({}).
> > +local function load_cfg(oldcfg, cfg)
> 
> oldcfg - unused argument. I think Sergey will be upset.
> https://github.com/tarantool/tarantool/issues/4681

As far as I understand it is __call semantic, no? IOW,
it is passed when log.cfg{} called as a function.

> >       --
> >       -- Internal API to box module, not for users,
> >       -- names can be changed.
> >       box_api = {
> > -        cfg = log_cfg,
> >           set_log_level = function()
> >               log_level(box.cfg.log_level)
> >           end,
> >           set_log_format = function()
> >               log_format(box.cfg.log_format)
> >           end,
> > +        cfg_apply_default = box_cfg_apply_default,
> > +        cfg_update = box_cfg_update,
> >       },
> 
> Why should we have a special box_module API?
> Maybe we can do all box.cfg dirty stuff in load_cfg.lua.

If you mean to modify log.cfg inside load_cfg.lua directly,
I think it is a bad idea. The variables of a separate module
must never be touched straightforward, this will make code
less maintainable.

> >   }
> > 
> Is it ok and we shouldn't use log.cfg() after box.cfg()?
> 
> └──╼ tarantool
> [70/1585]
> Tarantool 2.5.0-90-g39a946b
> type 'help' for interactive help
> tarantool> log = require("log")
> ---
> ...
> 
> tarantool> box.cfg{log_level = 3}
> 2020-05-29 11:33:09.901 [6926] main/103/interactive C> Tarantool
> 2.5.0-90-g39a946b
> 2020-05-29 11:33:09.901 [6926] main/103/interactive C> log level 3
> 2020-05-29 11:33:09.974 [6926] main/103/interactive C> leaving orphan mode
...

Thanks, will take a look.

[Tarantool-patches] [PATCH v4 8/8] test: use direct log module

[Cyrill Gorcunov]

To test if we can setup logging module before the box/cfg{}.

Part-of #689

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 test/app-tap/logger.test.lua | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/test/app-tap/logger.test.lua b/test/app-tap/logger.test.lua
index 7bfa06e80..410220494 100755
--- a/test/app-tap/logger.test.lua
+++ b/test/app-tap/logger.test.lua
@@ -1,23 +1,44 @@
 #!/usr/bin/env tarantool
 
 local test = require('tap').test('log')
-test:plan(24)
+test:plan(27)
 
 -- gh-3946: Assertion failure when using log_format() before box.cfg()
 local log = require('log')
 log.log_format('plain')
 
+--
+-- gh-689: Operate with logger via log module without calling box.cfg{}
+local json = require('json')
+local filename = "1.log"
+
+log.cfg({log=filename, log_format='json', log_level=5})
+local m = "info message"
+
+local file = io.open(filename)
+while file:read() do
+end
+
+log.info(m)
+local line = file:read()
+local message = json.decode(line)
+file:close()
+
+test:is(type(message), 'table', "(log) json valid in log.info")
+test:is(message.level, "INFO", "(log) check type info")
+test:is(message.message, m, "(log) check message content")
+log.log_format('plain')
+
 --
 -- Check that Tarantool creates ADMIN session for #! script
 --
-local filename = "1.log"
 local message = "Hello, World!"
 box.cfg{
     log=filename,
+    log_format='plain',
     memtx_memory=107374182,
 }
 local fio = require('fio')
-local json = require('json')
 local fiber = require('fiber')
 local file = io.open(filename)
 while file:read() do
-- 
2.26.2

Re: [Tarantool-patches] [PATCH v4 8/8] test: use direct log module

[Oleg Babin]

Hi! Thanks for your patch. See several comments below.

On 28/05/2020 13:07, Cyrill Gorcunov wrote:
> To test if we can setup logging module before the box/cfg{}.

nit: `box.cfg{}`, not `box/cfg{}`
> 
> Part-of #689
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   test/app-tap/logger.test.lua | 27 ++++++++++++++++++++++++---
>   1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/test/app-tap/logger.test.lua b/test/app-tap/logger.test.lua
> index 7bfa06e80..410220494 100755
> --- a/test/app-tap/logger.test.lua
> +++ b/test/app-tap/logger.test.lua
> @@ -1,23 +1,44 @@
>   #!/usr/bin/env tarantool
>   
>   local test = require('tap').test('log')
> -test:plan(24)
> +test:plan(27)
>   
>   -- gh-3946: Assertion failure when using log_format() before box.cfg()
>   local log = require('log')
>   log.log_format('plain')
>   
> +--
> +-- gh-689: Operate with logger via log module without calling box.cfg{}

As we've discussed it's better to add "negative" cases to tests.
There are examples, e.g. 
https://github.com/tarantool/tarantool/blob/master/test/app-tap/yaml.test.lua#L86

> +local json = require('json')
> +local filename = "1.log"
> +
> +log.cfg({log=filename, log_format='json', log_level=5})
> +local m = "info message"
> +
> +local file = io.open(filename)
> +while file:read() do
> +end
> +
> +log.info(m)
> +local line = file:read()
> +local message = json.decode(line)
> +file:close()
> +
> +test:is(type(message), 'table', "(log) json valid in log.info")
> +test:is(message.level, "INFO", "(log) check type info")
> +test:is(message.message, m, "(log) check message content")
> +log.log_format('plain')
> +
>   --
>   -- Check that Tarantool creates ADMIN session for #! script
>   --
> -local filename = "1.log"
>   local message = "Hello, World!"
>   box.cfg{
>       log=filename,
> +    log_format='plain',
>       memtx_memory=107374182,
>   }
>   local fio = require('fio')
> -local json = require('json')
>   local fiber = require('fiber')
>   local file = io.open(filename)
>   while file:read() do
> 

Re: [Tarantool-patches] [PATCH v4 8/8] test: use direct log module

[Cyrill Gorcunov]

On Thu, May 28, 2020 at 01:42:32PM +0300, Oleg Babin wrote:
> Hi! Thanks for your patch. See several comments below.
> 
> On 28/05/2020 13:07, Cyrill Gorcunov wrote:
> > To test if we can setup logging module before the box/cfg{}.
> 
> nit: `box.cfg{}`, not `box/cfg{}`

Thanks!

> > +--
> > +-- gh-689: Operate with logger via log module without calling box.cfg{}
> 
> As we've discussed it's better to add "negative" cases to tests.
> There are examples, e.g. https://github.com/tarantool/tarantool/blob/master/test/app-tap/yaml.test.lua#L86

Yes, thanks a huge!

Re: [Tarantool-patches] [PATCH v4 8/8] test: use direct log module

[Leonid Vasiliev]

Hi! Thank you for the patch.
I agree with the comments of Oleg.

Re: [Tarantool-patches] [PATCH v4 0/8] lua/log: add an ability to setup logger without box.cfg{}

[Leonid Vasiliev]

Hi! Thank you for the patchset.
Please, add @ChangeLog

On 28.05.2020 13:07, Cyrill Gorcunov wrote:
> In the series we add an ability to configure logger early
> without calling box.cfg{}. The syntax is the same as
> in box.cfg{} call.
> 
> There was an idea to implement something similar via triggers but
> I think this will require a way more efforts and code redesign,
> so at first lets stick to simplier solution.
> 
> v2 by Oleg:
>   - hide box_api symbols from users
>   - initialize logger via log.cfg() call to look similar to box.cfg
> v3 by Oleg:
>   - add parametesr verification
>   - allow to reconfig via log.cfg() call
> v4 by Oleg:
>   - lua style fixes
>   - extent test (I didn't find a way to test wrong params verification
>     inside tap test, so I did it manually. Still we might extent the
>     test on top of the series).
> 
> branch gorcunov/gh-689-logger-4
> issue https://github.com/tarantool/tarantool/issues/689
> 
> Cyrill Gorcunov (8):
>    core/say: do not reconfig early set up logger
>    lua/log: declare say_logger_init and say_logger_initialized
>    lua/log: put string constants to map
>    lua/log: do not allow to set json for boot logger
>    lua/log: declare log as separate variable
>    lua/log: use log module settings inside box.cfg
>    lua/log: allow to configure logging without a box
>    test: use direct log module
> 
>   src/box/lua/load_cfg.lua     |  24 +++-
>   src/lib/core/say.c           |  13 +++
>   src/lib/core/say.h           |   4 +
>   src/lua/log.lua              | 216 +++++++++++++++++++++++++++++++----
>   test/app-tap/logger.test.lua |  28 ++++-
>   5 files changed, 255 insertions(+), 30 deletions(-)
>