Re: [Tarantool-patches] [PATCH v3 09/10] lua/log: allow to configure logging without a box

[Oleg Babin <olegrok@tarantool.org>]

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