From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (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 6A99A469710 for ; Wed, 3 Jun 2020 10:03:29 +0300 (MSK) From: Oleg Babin References: <20200602221817.645015-1-gorcunov@gmail.com> <20200602221817.645015-11-gorcunov@gmail.com> Message-ID: <2ce5c32b-f0fc-3593-7617-de1b0de8f5e8@tarantool.org> Date: Wed, 3 Jun 2020 10:03:24 +0300 MIME-Version: 1.0 In-Reply-To: <20200602221817.645015-11-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v8 10/12] log/lua: allow to specify logging level as a string List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov , tml Hi! Thanks for changes, see one comment below. On 03/06/2020 01:18, Cyrill Gorcunov wrote: > Allow to specify logging level as a string in log.cfg() call. > > @TarantoolBot document > Title: Module log > > The `log.cfg({level='string'})` accepts not only number but > symbolic representation of loglevels such as: `fatal`, `syserror`, > `error`, `crit`, `warn`, `info`, `verbose` and `debug`. > > Part-of #689 > > Suggested-by: Oleg Babin > Signed-off-by: Cyrill Gorcunov > --- > src/lua/log.lua | 36 ++++++++++++++++++++++++++++++++++-- > test/app-tap/logger.test.lua | 29 ++++++++++++++++++++++++++++- > 2 files changed, 62 insertions(+), 3 deletions(-) > > diff --git a/src/lua/log.lua b/src/lua/log.lua > index 33f7a5892..50c96a8d2 100644 > --- a/src/lua/log.lua > +++ b/src/lua/log.lua > @@ -140,6 +140,26 @@ local function update_box_cfg(k) > end > end > > +-- Logging levels symbolic representation. > +local log_level_keys = { > + ['fatal'] = ffi.C.S_FATAL, > + ['syserror'] = ffi.C.S_SYSERROR, > + ['error'] = ffi.C.S_ERROR, > + ['crit'] = ffi.C.S_CRIT, > + ['warn'] = ffi.C.S_WARN, > + ['info'] = ffi.C.S_INFO, > + ['verbose'] = ffi.C.S_VERBOSE, > + ['debug'] = ffi.C.S_DEBUG, > +} > + > +local function log_level_list() > + local keyset = {} > + for k in pairs(log_level_keys) do > + keyset[#keyset + 1] = k > + end > + return table.concat(keyset, ',') > +end > + > local function say(level, fmt, ...) > if ffi.C.log_level < level then > -- don't waste cycles on debug.getinfo() > @@ -303,8 +323,20 @@ local function load_cfg(oldcfg, cfg) > end > > if cfg.level ~= nil then > - if type(cfg.level) ~= 'number' then > - error("log: 'level' option must be a number") > + if type(cfg.level) == 'number' then > + if cfg.level < ffi.C.S_FATAL or > + cfg.level > ffi.C.S_DEBUG then > + local m = "log: 'level' must be in range [%d;%d]" > + error(m:format(ffi.C.S_FATAL, ffi.C.S_DEBUG)) > + end > + elseif type(cfg.level) == 'string' then > + if log_level_keys[cfg.level] == nil then > + local m = "log: 'level' must be one of [%s]" > + error(m:format(log_level_list())) > + end > + cfg.level = log_level_keys[cfg.level] > + else > + error("log: 'level' must be a number or string") > end > end > > diff --git a/test/app-tap/logger.test.lua b/test/app-tap/logger.test.lua > index bc301d720..48dc8af3e 100755 > --- a/test/app-tap/logger.test.lua > +++ b/test/app-tap/logger.test.lua > @@ -1,7 +1,7 @@ > #!/usr/bin/env tarantool > > local test = require('tap').test('log') > -test:plan(34) > +test:plan(44) > > -- gh-3946: Assertion failure when using log_format() before box.cfg() > local log = require('log') > @@ -60,6 +60,33 @@ log.cfg({format='plain', level=5}) > test:is(box.cfg.log_format, 'plain', 'box sees plain format') > test:is(box.cfg.log_level, 5, 'box sees level change') > > +-- Test symbolic names for loglevels > +log.cfg({level='fatal'}) > +test:ok(log.cfg.level == 0 and box.cfg.log_level == 0, 'both got fatal') > +log.cfg({level='syserror'}) > +test:ok(log.cfg.level == 1 and box.cfg.log_level == 1, 'both got syserror') > +log.cfg({level='error'}) > +test:ok(log.cfg.level == 2 and box.cfg.log_level == 2, 'both got error') > +log.cfg({level='crit'}) > +test:ok(log.cfg.level == 3 and box.cfg.log_level == 3, 'both got crit') > +log.cfg({level='warn'}) > +test:ok(log.cfg.level == 4 and box.cfg.log_level == 4, 'both got warn') > +log.cfg({level='info'}) > +test:ok(log.cfg.level == 5 and box.cfg.log_level == 5, 'both got info') > +log.cfg({level='verbose'}) > +test:ok(log.cfg.level == 6 and box.cfg.log_level == 6, 'both got verbose') > +log.cfg({level='debug'}) > +test:ok(log.cfg.level == 7 and box.cfg.log_level == 7, 'both got debug') > + > +_,err = pcall(log, {level = -1}) > +test:ok(err ~= nil, 'level = -1 error') > + > +_,err = pcall(log, {level = 'unknown'}) > +test:ok(err ~= nil, "level = 'unknown' error") > + ``` tarantool> pcall(log, {level = -1}) --- - false - attempt to call a table value ... tarantool> pcall(log, {level = 'unknown'}) --- - false - attempt to call a table value ... ``` It should be pcall(log.cfg, ...). Also it would be nice to check that error message is "log: 'level' must be one of...". > +-- restore to info for next tests > +log.cfg({level='info'}) > + > -- > -- Check that Tarantool creates ADMIN session for #! script > -- >