From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <olegrok@tarantool.org>
Received: from smtp37.i.mail.ru (smtp37.i.mail.ru [94.100.177.97])
 (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 3F5E6469711
 for <tarantool-patches@dev.tarantool.org>;
 Thu, 28 May 2020 13:42:17 +0300 (MSK)
From: Oleg Babin <olegrok@tarantool.org>
References: <20200528100738.221911-1-gorcunov@gmail.com>
 <20200528100738.221911-8-gorcunov@gmail.com>
Message-ID: <cc54fa2f-7e18-a409-2ab7-2e6fefb4af73@tarantool.org>
Date: Thu, 28 May 2020 13:42:14 +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-GB
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [PATCH v4 7/8] lua/log: allow to configure
	logging without a box
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Cyrill Gorcunov <gorcunov@gmail.com>, tml <tarantool-patches@dev.tarantool.org>

Hi! Thanks for your patch. See one comment 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 <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 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
> +
>   --
>   -- 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

Seems we could easily add an ability to specify log level as string.
log.cfg{log_level = 'info'} - it will be really more user-friendly than 
currently.

Also it's possible to specify "invalid" numbers:
```
tarantool> require('log').cfg({log_level = 0/0})
---
...

tarantool> require('log').cfg
---
- log_format: json
   log_level: nan
   log_nonblock: false
...
```
I think we could restict levels from 0/1 to 7.
All available log_levels are exposed as `enum say_level` and you can use 
it as you've done it for `say_format`s.