[Tarantool-patches] [PATCH v3 00/10] lua/log: add an ability to setup logger without box.cfg{}

[Tarantool-patches] [PATCH v3 00/10] 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

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

Cyrill Gorcunov (10):
  core/say: drop redundant declarations
  core/say: drop vsay declaration
  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           |  18 ++-
 src/lib/core/say.h           |   8 +-
 src/lua/log.lua              | 216 +++++++++++++++++++++++++++++++----
 test/app-tap/logger.test.lua |  28 ++++-
 5 files changed, 255 insertions(+), 39 deletions(-)

-- 
2.26.2

[Tarantool-patches] [PATCH v3 01/10] core/say: drop redundant declarations

[Cyrill Gorcunov]

Part-of #689

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/core/say.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/src/lib/core/say.c b/src/lib/core/say.c
index 7116883d4..e22089aac 100644
--- a/src/lib/core/say.c
+++ b/src/lib/core/say.c
@@ -244,11 +244,6 @@ say_format_by_name(const char *format)
 	return STR2ENUM(say_format, format);
 }
 
-static void
-write_to_file(struct log *log, int total);
-static void
-write_to_syslog(struct log *log, int total);
-
 /**
  * Sets O_NONBLOCK flag in case if lognonblock is set.
  */
-- 
2.26.2

[Tarantool-patches] [PATCH v3 02/10] core/say: drop vsay declaration

[Cyrill Gorcunov]

The implementation has been removed in
commit ef2c171d3e8f1d675c1cab548838a03ae67a2a66

Part-of #689

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/core/say.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/src/lib/core/say.h b/src/lib/core/say.h
index 43883f0da..c50d7bbf4 100644
--- a/src/lib/core/say.h
+++ b/src/lib/core/say.h
@@ -278,10 +278,6 @@ say_logger_init(const char *init_str,
 void
 say_logger_free();
 
-CFORMAT(printf, 5, 0) void
-vsay(int level, const char *filename, int line, const char *error,
-     const char *format, va_list ap);
-
 /** \cond public */
 typedef void (*sayfunc_t)(int, const char *, int, const char *,
 		    const char *, ...);
-- 
2.26.2

[Tarantool-patches] [PATCH v3 03/10] 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

[Tarantool-patches] [PATCH v3 04/10] 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

[Tarantool-patches] [PATCH v3 05/10] 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..3fceb1d0e 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

[Tarantool-patches] [PATCH v3 06/10] 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 3fceb1d0e..c6e03ad78 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

[Tarantool-patches] [PATCH v3 07/10] 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 c6e03ad78..bee2851c8 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

[Tarantool-patches] [PATCH v3 08/10] 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 bee2851c8..25c919412 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

[Tarantool-patches] [PATCH v3 09/10] 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 25c919412..c14769e84 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

[Tarantool-patches] [PATCH v3 10/10] 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 v3 00/10] lua/log: add an ability to setup logger without box.cfg{}

[Oleg Babin]

Hi! Thanks for your changes.

I think that it's a bit redundant to have "log" prefixes for log.cfg 
options.

log.cfg({format = ..., level = ..., non_block = ...}) looks better IMO.


Also it's still possible to setup invalid value for log_level.
```
tarantool> log.cfg{log_format = 'invalid'}
---
- error: 'builtin/log.lua:239: log: ''log_format'' must be json,plain'
...

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

tarantool> log.cfg
---
- log_format: json
   log_level: -1
   log_nonblock: false
...
```

Also I have several comments for specific commits. I'll send them 
separately.

On 27/05/2020 14:13, 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
> 
> branch gorcunov/gh-689-logger-3
> issue https://github.com/tarantool/tarantool/issues/689
> 
> Cyrill Gorcunov (10):
>    core/say: drop redundant declarations
>    core/say: drop vsay declaration
>    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           |  18 ++-
>   src/lib/core/say.h           |   8 +-
>   src/lua/log.lua              | 216 +++++++++++++++++++++++++++++++----
>   test/app-tap/logger.test.lua |  28 ++++-
>   5 files changed, 255 insertions(+), 39 deletions(-)
> 

Re: [Tarantool-patches] [PATCH v3 05/10] lua/log: put string constants to map

[Oleg Babin]

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

On 27/05/2020 14:14, 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..3fceb1d0e 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

nit: I think `_` could be omitted.

> +            keyset[#keyset+1] = k

nit: To follow Tarantool lua style guide spaces around `+` should be 
added (https://www.tarantool.io/en/doc/2.2/dev_guide/lua_style_guide)
> +        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 v3 07/10] lua/log: declare log as separate variable

[Oleg Babin]

Hi! Thanks for your patch see 1 comment below.

On 27/05/2020 14:14, 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 c6e03ad78..bee2851c8 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 ={

nit: Please, add space between `=` and `{`

> +    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 v3 09/10] lua/log: allow to configure logging without a box

[Oleg Babin]

Thanks for your patch. See several comments below. It's just nitpicks, 
in some places I only write how it should be changed to respect 
Tarantool lua style guide.

On 27/05/2020 14:14, 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 25c919412..c14769e84 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
nit: spaces around `+` are required by tarantool lua style guide 
(https://www.tarantool.io/en/doc/2.2/dev_guide/lua_style_guide)
> +    end
> +    return table.concat(keyset,',')
nit: space between a comma and argument required is required by lua 
style guide
> +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
nit: `_,k` -> `_, k`
> +        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
nit: `~=nil` -> `~= nil`

> +        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,
>       },
>   }
>   
> 

Re: [Tarantool-patches] [PATCH v3 10/10] test: use direct log module

[Oleg Babin]

Thanks for your patch. I think that there are some cases that should be 
also covered:

- Invalid parameters: log_format/log_level
- It's possible to call log.cfg several times with different set of 
parameters.

On 27/05/2020 14:14, Cyrill Gorcunov wrote:
> 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
> 

Re: [Tarantool-patches] [PATCH v3 01/10] core/say: drop redundant declarations

[Kirill Yukhin]

Hello,

On 27 май 14:13, Cyrill Gorcunov wrote:
> Part-of #689
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

Checked into master as obvious.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH v3 02/10] core/say: drop vsay declaration

[Kirill Yukhin]

Hello,

On 27 май 14:13, Cyrill Gorcunov wrote:
> The implementation has been removed in
> commit ef2c171d3e8f1d675c1cab548838a03ae67a2a66
> 
> Part-of #689
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

Checked into master as obvious.

--
Regards, Kirill Yukhin