From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id ABD89469710 for ; Fri, 29 May 2020 11:41:42 +0300 (MSK) References: <20200528100738.221911-1-gorcunov@gmail.com> <20200528100738.221911-8-gorcunov@gmail.com> From: Leonid Vasiliev Message-ID: <19d91eba-a988-0ee8-6c1b-1b83dca9fce8@tarantool.org> Date: Fri, 29 May 2020 11:41:41 +0300 MIME-Version: 1.0 In-Reply-To: <20200528100738.221911-8-gorcunov@gmail.com> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH v4 7/8] lua/log: allow to configure logging without a box List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov , tml 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 > --- > 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 ...