[Tarantool-patches] [PATCH] lua/log: accept symbolic logging levels

[Tarantool-patches] [PATCH] lua/log: accept symbolic logging levels

[Cyrill Gorcunov via Tarantool-patches]

Currently `log` module accepts only numeric values of
logging levels. I turn `box.cfg` interface supports
symbolic names (such as 'fatat', 'crit' and etc).

Thus we should support the same in `log` module.

Closes #5882

Reported-by: Alexander Turenko <alexander.turenko@tarantool.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
issue https://github.com/tarantool/tarantool/issues/5882
branch gorcunov/gh-5882-logger-strings

 changelogs/unreleased/gh-5882-log-levels.md | 13 +++++++++++++
 src/lua/log.lua                             |  6 ++++++
 2 files changed, 19 insertions(+)
 create mode 100644 changelogs/unreleased/gh-5882-log-levels.md

diff --git a/changelogs/unreleased/gh-5882-log-levels.md b/changelogs/unreleased/gh-5882-log-levels.md
new file mode 100644
index 000000000..08f9595be
--- /dev/null
+++ b/changelogs/unreleased/gh-5882-log-levels.md
@@ -0,0 +1,13 @@
+## feature/lua/log
+
+ * Implemented support of symbolic log levels representation
+   in `log` module (gh-5882). Now it is possible to specify
+   levels the same way as in `box.cfg{}` call. For example
+   instead of
+   ``` Lua
+   require('log').cfg{level = 6}
+   ```
+   One can use
+   ``` Lua
+   require('log').cfg{level = 'verbose'}
+   ```
diff --git a/src/lua/log.lua b/src/lua/log.lua
index 62ea61f2d..788560722 100644
--- a/src/lua/log.lua
+++ b/src/lua/log.lua
@@ -499,6 +499,12 @@ local function load_cfg(self, cfg)
             local m = "log.cfg: \'%s\' %s"
             error(m:format('level', msg))
         end
+        -- Convert level to a numeric value since
+        -- low level api operates with numbers only.
+        if type(cfg.level) == 'string' then
+            assert(log_level_keys[cfg.level] ~= nil)
+            cfg.level = log_level_keys[cfg.level]
+        end
     end
 
     if cfg.nonblock ~= nil then
-- 
2.30.2

Re: [Tarantool-patches] [PATCH] lua/log: accept symbolic logging levels

[Cyrill Gorcunov via Tarantool-patches]

On Fri, May 07, 2021 at 02:13:05PM +0300, Cyrill Gorcunov wrote:
> Currently `log` module accepts only numeric values of
> logging levels. I turn `box.cfg` interface supports
> symbolic names (such as 'fatat', 'crit' and etc).
                           ^^^^^
Typo, should be 'fatal'. Force pushed an update.

Re: [Tarantool-patches] [PATCH] lua/log: accept symbolic logging levels

[Serge Petrenko via Tarantool-patches]

07.05.2021 14:13, Cyrill Gorcunov via Tarantool-patches пишет:
> Currently `log` module accepts only numeric values of
> logging levels. I turn `box.cfg` interface supports
> symbolic names (such as 'fatat', 'crit' and etc).
>
> Thus we should support the same in `log` module.
>
> Closes #5882
>
> Reported-by: Alexander Turenko <alexander.turenko@tarantool.org>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
> issue https://github.com/tarantool/tarantool/issues/5882
> branch gorcunov/gh-5882-logger-strings
>
>   changelogs/unreleased/gh-5882-log-levels.md | 13 +++++++++++++
>   src/lua/log.lua                             |  6 ++++++
>   2 files changed, 19 insertions(+)
>   create mode 100644 changelogs/unreleased/gh-5882-log-levels.md
>
> diff --git a/changelogs/unreleased/gh-5882-log-levels.md b/changelogs/unreleased/gh-5882-log-levels.md
> new file mode 100644
> index 000000000..08f9595be
> --- /dev/null
> +++ b/changelogs/unreleased/gh-5882-log-levels.md
> @@ -0,0 +1,13 @@
> +## feature/lua/log
> +
> + * Implemented support of symbolic log levels representation
> +   in `log` module (gh-5882). Now it is possible to specify
> +   levels the same way as in `box.cfg{}` call. For example
> +   instead of
> +   ``` Lua
> +   require('log').cfg{level = 6}
> +   ```
> +   One can use
> +   ``` Lua
> +   require('log').cfg{level = 'verbose'}
> +   ```
> diff --git a/src/lua/log.lua b/src/lua/log.lua
> index 62ea61f2d..788560722 100644
> --- a/src/lua/log.lua
> +++ b/src/lua/log.lua
> @@ -499,6 +499,12 @@ local function load_cfg(self, cfg)
>               local m = "log.cfg: \'%s\' %s"
>               error(m:format('level', msg))
>           end
> +        -- Convert level to a numeric value since
> +        -- low level api operates with numbers only.
> +        if type(cfg.level) == 'string' then
> +            assert(log_level_keys[cfg.level] ~= nil)
> +            cfg.level = log_level_keys[cfg.level]
> +        end
>       end
>   
>       if cfg.nonblock ~= nil then

Hi! Thanks for the patch!
LGTM.

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH] lua/log: accept symbolic logging levels

[Alexander Turenko via Tarantool-patches]

Sorry for the late response.

Looks nice! LGTM.

If you'll have a free time, it would be good to add a simple test case:
just to automatically verify it for regressions in a future.

WBR, Alexander Turenko.

On Fri, May 07, 2021 at 02:13:05PM +0300, Cyrill Gorcunov wrote:
> Currently `log` module accepts only numeric values of
> logging levels. I turn `box.cfg` interface supports
> symbolic names (such as 'fatal', 'crit' and etc).
> 
> Thus we should support the same in `log` module.
> 
> Closes #5882
> 
> Reported-by: Alexander Turenko <alexander.turenko@tarantool.org>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
> issue https://github.com/tarantool/tarantool/issues/5882
> branch gorcunov/gh-5882-logger-strings
>
> <...>

Re: [Tarantool-patches] [PATCH] lua/log: accept symbolic logging levels

[Cyrill Gorcunov via Tarantool-patches]

On Thu, May 27, 2021 at 01:04:54AM +0300, Alexander Turenko wrote:
> Sorry for the late response.
> 
> Looks nice! LGTM.
> 
> If you'll have a free time, it would be good to add a simple test case:
> just to automatically verify it for regressions in a future.
> 

Thanks! I force pushed an update where a test added (we can extend
it in future).

branch gorcunov/gh-5882-logger-strings
issue https://github.com/tarantool/tarantool/issues/5882
---
From: Cyrill Gorcunov <gorcunov@gmail.com>
Date: Fri, 7 May 2021 13:59:43 +0300
Subject: [PATCH] lua/log: accept symbolic logging levels

Currently `log` module accepts only numeric values of
logging levels. I turn `box.cfg` interface supports
symbolic names (such as 'fatal', 'crit' and etc).

Thus we should support the same in `log` module.

Closes #5882

Reported-by: Alexander Turenko <alexander.turenko@tarantool.org>
Acked-by: Alexander Turenko <alexander.turenko@tarantool.org>
Acked-by: Serge Petrenko <sergepetrenko@tarantool.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 changelogs/unreleased/gh-5882-log-levels.md | 13 ++++++++
 src/lua/log.lua                             |  6 ++++
 test/app-tap/logmod.test.lua                | 36 +++++++++++++++++++++
 3 files changed, 55 insertions(+)
 create mode 100644 changelogs/unreleased/gh-5882-log-levels.md
 create mode 100755 test/app-tap/logmod.test.lua

diff --git a/changelogs/unreleased/gh-5882-log-levels.md b/changelogs/unreleased/gh-5882-log-levels.md
new file mode 100644
index 000000000..08f9595be
--- /dev/null
+++ b/changelogs/unreleased/gh-5882-log-levels.md
@@ -0,0 +1,13 @@
+## feature/lua/log
+
+ * Implemented support of symbolic log levels representation
+   in `log` module (gh-5882). Now it is possible to specify
+   levels the same way as in `box.cfg{}` call. For example
+   instead of
+   ``` Lua
+   require('log').cfg{level = 6}
+   ```
+   One can use
+   ``` Lua
+   require('log').cfg{level = 'verbose'}
+   ```
diff --git a/src/lua/log.lua b/src/lua/log.lua
index 62ea61f2d..788560722 100644
--- a/src/lua/log.lua
+++ b/src/lua/log.lua
@@ -499,6 +499,12 @@ local function load_cfg(self, cfg)
             local m = "log.cfg: \'%s\' %s"
             error(m:format('level', msg))
         end
+        -- Convert level to a numeric value since
+        -- low level api operates with numbers only.
+        if type(cfg.level) == 'string' then
+            assert(log_level_keys[cfg.level] ~= nil)
+            cfg.level = log_level_keys[cfg.level]
+        end
     end
 
     if cfg.nonblock ~= nil then
diff --git a/test/app-tap/logmod.test.lua b/test/app-tap/logmod.test.lua
new file mode 100755
index 000000000..68508ce5b
--- /dev/null
+++ b/test/app-tap/logmod.test.lua
@@ -0,0 +1,36 @@
+#!/usr/bin/env tarantool
+
+local test = require('tap').test('log')
+local log = require('log')
+
+test:plan(8)
+
+log.log_format('plain')
+
+-- Test symbolic names for loglevels
+_, err = pcall(log.cfg, {level='fatal'})
+test:ok(err == nil and log.cfg.level == 0, 'both got fatal')
+
+_, err = pcall(log.cfg, {level='syserror'})
+test:ok(err == nil and log.cfg.level == 1, 'got syserror')
+
+_, err = pcall(log.cfg, {level='error'})
+test:ok(err == nil and log.cfg.level == 2, 'got error')
+
+_, err = pcall(log.cfg, {level='crit'})
+test:ok(err == nil and log.cfg.level == 3, 'got crit')
+
+_, err = pcall(log.cfg, {level='warn'})
+test:ok(err == nil and log.cfg.level == 4, 'got warn')
+
+_, err = pcall(log.cfg, {level='info'})
+test:ok(err == nil and log.cfg.level == 5, 'got info')
+
+_, err = pcall(log.cfg, {level='verbose'})
+test:ok(err == nil and log.cfg.level == 6, 'got verbose')
+
+_, err = pcall(log.cfg, {level='debug'})
+test:ok(err == nil and log.cfg.level == 7, 'got debug')
+
+test:check()
+os.exit()
-- 
2.31.1

Re: [Tarantool-patches] [PATCH] lua/log: accept symbolic logging levels

[Alexander Turenko via Tarantool-patches]

On Thu, May 27, 2021 at 10:53:32AM +0300, Cyrill Gorcunov wrote:
> On Thu, May 27, 2021 at 01:04:54AM +0300, Alexander Turenko wrote:
> > Sorry for the late response.
> > 
> > Looks nice! LGTM.
> > 
> > If you'll have a free time, it would be good to add a simple test case:
> > just to automatically verify it for regressions in a future.
> > 
> 
> Thanks! I force pushed an update where a test added (we can extend
> it in future).

> +-- Test symbolic names for loglevels
> +_, err = pcall(log.cfg, {level='fatal'})
> +test:ok(err == nil and log.cfg.level == 0, 'both got fatal')

Now I noticed one difference from box.cfg() behaviour:

 | tarantool> box.cfg{log_level = 'verbose'}
 | tarantool> box.cfg.log_level
 | ---
 | - verbose
 | ...

 | tarantool> log = require('log')
 | tarantool> log.cfg{level = 'verbose'}
 | tarantool> log.cfg.level
 | ---
 | - 6
 | ...

AFAIU, box.cfg.foo generally returns the same that a user provides in
box.cfg({foo = <...>}), without normalization.

The only exception from this rule, known to me, is convertion from a
number to a string for box.cfg.listen:

 | tarantool> box.cfg{listen = 3301}
 | tarantool> box.cfg.listen
 | ---
 | - '3301'
 | ...

I don't know, to be honest, whether the general rule was part of design
or becomes so unintentionally. However, when we add listening on zero
port ([1]), we added a real port into box.info.listen and didn't perform
normalization in box.cfg.listen.

Not a blocker, IMHO, but it worth to think about. I'll add Vlad into the
discussion. Maybe he knows more than me.

[1]: https://github.com/tarantool/tarantool/issues/4620

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH] lua/log: accept symbolic logging levels

[Cyrill Gorcunov via Tarantool-patches]

On Thu, May 27, 2021 at 12:04:31PM +0300, Alexander Turenko wrote:
> 
> > +-- Test symbolic names for loglevels
> > +_, err = pcall(log.cfg, {level='fatal'})
> > +test:ok(err == nil and log.cfg.level == 0, 'both got fatal')
> 
> Now I noticed one difference from box.cfg() behaviour:
> 
>  | tarantool> box.cfg{log_level = 'verbose'}
>  | tarantool> box.cfg.log_level
>  | ---
>  | - verbose
>  | ...
> 
>  | tarantool> log = require('log')
>  | tarantool> log.cfg{level = 'verbose'}
>  | tarantool> log.cfg.level
>  | ---
>  | - 6
>  | ...
> 
> AFAIU, box.cfg.foo generally returns the same that a user provides in
> box.cfg({foo = <...>}), without normalization.

Well, as to me this looks somehow inconvenient and worth to implement
symbolic output for log leves I think. As a separate feature.

Re: [Tarantool-patches] [PATCH] lua/log: accept symbolic logging levels

[Kirill Yukhin via Tarantool-patches]

Hello,

On 07 май 14:13, Cyrill Gorcunov wrote:
> Currently `log` module accepts only numeric values of
> logging levels. I turn `box.cfg` interface supports
> symbolic names (such as 'fatat', 'crit' and etc).
> 
> Thus we should support the same in `log` module.
> 
> Closes #5882
> 
> Reported-by: Alexander Turenko <alexander.turenko@tarantool.org>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

I've checked your patch into 2.8 and master.

--
Regards, Kirill Yukhin