[Tarantool-patches] [PATCH v3 09/10] lua/log: allow to configure logging without a box
Oleg Babin
olegrok at tarantool.org
Wed May 27 15:59:29 MSK 2020
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 at 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,
> },
> }
>
>
More information about the Tarantool-patches
mailing list