[Tarantool-patches] [PATCH 0/4] send feedback on tarantool start

[Tarantool-patches] [PATCH 0/4] send feedback on tarantool start

[Serge Petrenko via Tarantool-patches]

Send the first feedback_daemon report as soon as initial box configuration
finishes.

https://github.com/tarantool/tarantool/issues/5750
https://github.com/tarantool/tarantool/tree/sp/gh-5750-feedback-on-start

Serge Petrenko (4):
  feedback_daemon: include server uptime in the report
  feedback_daemon: rename `send_test` to `send`
  feedback_daemon: send feedback on server start
  feedback_daemon: generate report right before sending

 src/box/lua/feedback_daemon.lua       | 12 ++++++----
 src/box/lua/load_cfg.lua              | 34 ++++++++++++++++++---------
 test/box-tap/feedback_daemon.test.lua | 29 +++++++++++++----------
 3 files changed, 47 insertions(+), 28 deletions(-)

-- 
2.24.3 (Apple Git-128)

[Tarantool-patches] [PATCH 1/4] feedback_daemon: include server uptime in the report

[Serge Petrenko via Tarantool-patches]

We are going to send feedback right after initial `box.cfg{}` call, so
include server uptime in the report to filter out short-living CI
instances.

Also, while we're at it, fix a typo in feedback_daemon test.

Prerequisite #5750
---
 src/box/lua/feedback_daemon.lua       |  3 ++-
 test/box-tap/feedback_daemon.test.lua | 13 +++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/box/lua/feedback_daemon.lua b/src/box/lua/feedback_daemon.lua
index d54d75414..8820bcde5 100644
--- a/src/box/lua/feedback_daemon.lua
+++ b/src/box/lua/feedback_daemon.lua
@@ -108,6 +108,7 @@ local function fill_in_base_info(feedback)
     feedback.tarantool_version = box.info.version
     feedback.server_id         = box.info.uuid
     feedback.cluster_id        = box.info.cluster.uuid
+    feedback.uptime            = box.info.uptime
 end
 
 local function fill_in_platform_info(feedback)
@@ -400,7 +401,7 @@ setmetatable(daemon, {
         end,
         -- this function is used in saving feedback in file
         generate_feedback = function()
-            return fill_in_feedback({ feedback_version = 5 })
+            return fill_in_feedback({ feedback_version = 6 })
         end,
         start = function()
             start(daemon)
diff --git a/test/box-tap/feedback_daemon.test.lua b/test/box-tap/feedback_daemon.test.lua
index 8cfbf31d7..e1c450d2b 100755
--- a/test/box-tap/feedback_daemon.test.lua
+++ b/test/box-tap/feedback_daemon.test.lua
@@ -70,7 +70,7 @@ if not ok then
     os.exit(0)
 end
 
-test:plan(27)
+test:plan(28)
 
 local function check(message)
     while feedback_count < 1 do
@@ -123,9 +123,11 @@ local fio = require("fio")
 local fh = fio.open("feedback.json")
 test:ok(fh, "file is created")
 local file_data = fh:read()
--- Ignore the report time. The data should be equal other than that.
-feedback_save = string.gsub(feedback_save, '"time":(%d+)', 'time:0')
-file_data = string.gsub(file_data, '"time":(%d+)', 'time:0')
+-- Ignore the report time and uptime. The data should be equal other than that.
+feedback_save = string.gsub(feedback_save, '"uptime":(%d+)', '"uptime":0')
+file_data = string.gsub(file_data, '"uptime":(%d+)', '"uptime":0')
+feedback_save = string.gsub(feedback_save, '"time":(%d+)', '"time":0')
+file_data = string.gsub(file_data, '"time":(%d+)', '"time":0')
 test:is(file_data, feedback_save, "data is equal")
 fh:close()
 fio.unlink("feedback.json")
@@ -288,5 +290,8 @@ test:is(fiber.time64(), actual.stats.time, "Time of report generation is correct
 
 check_stats(actual.stats)
 
+actual = daemon.generate_feedback()
+test:is(box.info.uptime, actual.uptime, "Server uptime is reported and is correct.")
+
 test:check()
 os.exit(0)
-- 
2.24.3 (Apple Git-128)

[Tarantool-patches] [PATCH 2/4] feedback_daemon: rename `send_test` to `send`

[Serge Petrenko via Tarantool-patches]

feedback_daemon.send() will come in handy once we implement triggers to
dispatch feedback after some events, for example, right on initial
instance configuration.

So, it's not a testing method anymore, hence the new name.

Part of #5750
---
 src/box/lua/feedback_daemon.lua       |  2 +-
 test/box-tap/feedback_daemon.test.lua | 16 ++++++++--------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/box/lua/feedback_daemon.lua b/src/box/lua/feedback_daemon.lua
index 8820bcde5..d00eedf39 100644
--- a/src/box/lua/feedback_daemon.lua
+++ b/src/box/lua/feedback_daemon.lua
@@ -412,7 +412,7 @@ setmetatable(daemon, {
         reload = function()
             reload(daemon)
         end,
-        send_test = function()
+        send = function()
             if daemon.control ~= nil then
                 daemon.control:put("send")
             end
diff --git a/test/box-tap/feedback_daemon.test.lua b/test/box-tap/feedback_daemon.test.lua
index e1c450d2b..3d6bf1d9b 100755
--- a/test/box-tap/feedback_daemon.test.lua
+++ b/test/box-tap/feedback_daemon.test.lua
@@ -93,28 +93,28 @@ feedback_reset()
 errinj.set("ERRINJ_HTTPC", false)
 check('feedback received after errinj')
 
-daemon.send_test()
+daemon.send()
 check("feedback received after explicit sending")
 
 box.cfg{feedback_enabled = false}
 feedback_reset()
-daemon.send_test()
+daemon.send()
 test:ok(feedback_count == 0, "no feedback after disabling")
 
 box.cfg{feedback_enabled = true}
-daemon.send_test()
+daemon.send()
 check("feedback after start")
 
 daemon.stop()
 feedback_reset()
-daemon.send_test()
+daemon.send()
 test:ok(feedback_count == 0, "no feedback after stop")
 
 daemon.start()
-daemon.send_test()
+daemon.send()
 check("feedback after start")
-daemon.send_test()
-check("feedback after feedback send_test")
+daemon.send()
+check("feedback after feedback send")
 
 daemon.stop()
 
@@ -136,7 +136,7 @@ server:close()
 -- check it does not fail without server
 daemon = box.internal.feedback_daemon
 daemon.start()
-daemon.send_test()
+daemon.send()
 daemon.stop()
 
 --
-- 
2.24.3 (Apple Git-128)

[Tarantool-patches] [PATCH 3/4] feedback_daemon: send feedback on server start

[Serge Petrenko via Tarantool-patches]

Send the first report as soon as instance's initial configuration
finishes.

Closes #5750
---
 src/box/lua/feedback_daemon.lua |  2 +-
 src/box/lua/load_cfg.lua        | 34 ++++++++++++++++++++++-----------
 2 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/src/box/lua/feedback_daemon.lua b/src/box/lua/feedback_daemon.lua
index d00eedf39..2ce768642 100644
--- a/src/box/lua/feedback_daemon.lua
+++ b/src/box/lua/feedback_daemon.lua
@@ -345,7 +345,7 @@ local function guard_loop(self)
     while true do
 
         if get_fiber_id(self.fiber) == 0 then
-            self.fiber = fiber.create(feedback_loop, self)
+            self.fiber = fiber.new(feedback_loop, self)
             log.verbose("%s restarted", PREFIX)
         end
         local st = pcall(fiber.sleep, self.interval)
diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index 44bb95ed1..885a0cac1 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -626,6 +626,27 @@ setmetatable(box, {
      end
 })
 
+-- A trigger on initial box.cfg{} call. Used to perform some checks and
+-- send feedback report once instance is fully configured.
+local function on_initial_config()
+    -- Check if schema version matches Tarantool version and print
+    -- warning if it's not (in case user forgot to call
+    -- box.schema.upgrade()).
+    local needs, schema_version_str = private.schema_needs_upgrade()
+    if needs then
+        local msg = string.format(
+            'Your schema version is %s while Tarantool %s requires a more'..
+            ' recent schema version. Please, consider using box.'..
+            'schema.upgrade().', schema_version_str, box.info.version)
+        log.warn(msg)
+    end
+
+    -- Send feedback as soon as instance is configured.
+    if private.feedback_daemon ~= nil then
+        private.feedback_daemon.send()
+    end
+end
+
 -- Whether box is loaded.
 --
 -- `false` when box is not configured or when the initialization
@@ -716,17 +737,8 @@ local function load_cfg(cfg)
 
     box_is_configured = true
 
-    -- Check if schema version matches Tarantool version and print
-    -- warning if it's not (in case user forgot to call
-    -- box.schema.upgrade()).
-    local needs, schema_version_str = private.schema_needs_upgrade()
-    if needs then
-        local msg = string.format(
-            'Your schema version is %s while Tarantool %s requires a more'..
-            ' recent schema version. Please, consider using box.'..
-            'schema.upgrade().', schema_version_str, box.info.version)
-        log.warn(msg)
-    end
+    on_initial_config()
+
 end
 box.cfg = locked(load_cfg)
 
-- 
2.24.3 (Apple Git-128)

Re: [Tarantool-patches] [PATCH 3/4] feedback_daemon: send feedback on server start

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Thanks for the patch!

On 02.04.2021 16:58, Serge Petrenko wrote:
> Send the first report as soon as instance's initial configuration
> finishes.
> 
> Closes #5750
> ---
> diff --git a/src/box/lua/feedback_daemon.lua b/src/box/lua/feedback_daemon.lua
> index d00eedf39..2ce768642 100644
> --- a/src/box/lua/feedback_daemon.lua
> +++ b/src/box/lua/feedback_daemon.lua
> @@ -345,7 +345,7 @@ local function guard_loop(self)
>      while true do
>  
>          if get_fiber_id(self.fiber) == 0 then
> -            self.fiber = fiber.create(feedback_loop, self)
> +            self.fiber = fiber.new(feedback_loop, self)

Why?

Re: [Tarantool-patches] [PATCH 3/4] feedback_daemon: send feedback on server start

[Serge Petrenko via Tarantool-patches]

05.04.2021 16:18, Vladislav Shpilevoy пишет:
> Hi! Thanks for the patch!
>
> On 02.04.2021 16:58, Serge Petrenko wrote:
>> Send the first report as soon as instance's initial configuration
>> finishes.
>>
>> Closes #5750
>> ---
>> diff --git a/src/box/lua/feedback_daemon.lua b/src/box/lua/feedback_daemon.lua
>> index d00eedf39..2ce768642 100644
>> --- a/src/box/lua/feedback_daemon.lua
>> +++ b/src/box/lua/feedback_daemon.lua
>> @@ -345,7 +345,7 @@ local function guard_loop(self)
>>       while true do
>>   
>>           if get_fiber_id(self.fiber) == 0 then
>> -            self.fiber = fiber.create(feedback_loop, self)
>> +            self.fiber = fiber.new(feedback_loop, self)
> Why?

With fiber.create() feedback daemon proceeds to sending the "initial 
report" even before
lua's box.cfg() exits. This shouldn't break anything, as far as I 
understand, but
`tarantoolctl.test.lua` failed on my machine without this change.



-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH 3/4] feedback_daemon: send feedback on server start

[Vladislav Shpilevoy via Tarantool-patches]

On 05.04.2021 16:05, Serge Petrenko wrote:
> 
> 
> 05.04.2021 16:18, Vladislav Shpilevoy пишет:
>> Hi! Thanks for the patch!
>>
>> On 02.04.2021 16:58, Serge Petrenko wrote:
>>> Send the first report as soon as instance's initial configuration
>>> finishes.
>>>
>>> Closes #5750
>>> ---
>>> diff --git a/src/box/lua/feedback_daemon.lua b/src/box/lua/feedback_daemon.lua
>>> index d00eedf39..2ce768642 100644
>>> --- a/src/box/lua/feedback_daemon.lua
>>> +++ b/src/box/lua/feedback_daemon.lua
>>> @@ -345,7 +345,7 @@ local function guard_loop(self)
>>>       while true do
>>>             if get_fiber_id(self.fiber) == 0 then
>>> -            self.fiber = fiber.create(feedback_loop, self)
>>> +            self.fiber = fiber.new(feedback_loop, self)
>> Why?
> 
> With fiber.create() feedback daemon proceeds to sending the "initial report" even before
> lua's box.cfg() exits. This shouldn't break anything, as far as I understand, but
> `tarantoolctl.test.lua` failed on my machine without this change.

But box.cfg yields. Does it mean it still ends before box.cfg ends too,
if box.cfg yields?

Re: [Tarantool-patches] [PATCH 3/4] feedback_daemon: send feedback on server start

[Serge Petrenko via Tarantool-patches]

05.04.2021 19:11, Vladislav Shpilevoy пишет:
>
> On 05.04.2021 16:05, Serge Petrenko wrote:
>>
>> 05.04.2021 16:18, Vladislav Shpilevoy пишет:
>>> Hi! Thanks for the patch!
>>>
>>> On 02.04.2021 16:58, Serge Petrenko wrote:
>>>> Send the first report as soon as instance's initial configuration
>>>> finishes.
>>>>
>>>> Closes #5750
>>>> ---
>>>> diff --git a/src/box/lua/feedback_daemon.lua b/src/box/lua/feedback_daemon.lua
>>>> index d00eedf39..2ce768642 100644
>>>> --- a/src/box/lua/feedback_daemon.lua
>>>> +++ b/src/box/lua/feedback_daemon.lua
>>>> @@ -345,7 +345,7 @@ local function guard_loop(self)
>>>>        while true do
>>>>              if get_fiber_id(self.fiber) == 0 then
>>>> -            self.fiber = fiber.create(feedback_loop, self)
>>>> +            self.fiber = fiber.new(feedback_loop, self)
>>> Why?
>> With fiber.create() feedback daemon proceeds to sending the "initial report" even before
>> lua's box.cfg() exits. This shouldn't break anything, as far as I understand, but
>> `tarantoolctl.test.lua` failed on my machine without this change.
> But box.cfg yields. Does it mean it still ends before box.cfg ends too,
> if box.cfg yields?
Probably, yes.
I reverted this change after all. It didn't help much, and looks like the
problem was deeper than I thought. It wasn't about box.cfg{} exiting too
early. The problem is with some curl initializations on the first ever
request, which happens to be the feedback daemon report.

I worked this around by sleeping for 10 seconds before proceeding
to the dispatch. Fixes tests both locally and in CI.

Please find v2 in your mailbox.

-- 
Serge Petrenko

[Tarantool-patches] [PATCH 4/4] feedback_daemon: generate report right before sending

[Serge Petrenko via Tarantool-patches]

Feedback daemon used to generate report before waiting (for an hour by
default) until it's time to send it. Better actualize the reports and
generate them right when it's time to send them.

Follow-up #5750
---
 src/box/lua/feedback_daemon.lua | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/box/lua/feedback_daemon.lua b/src/box/lua/feedback_daemon.lua
index 2ce768642..0aab189b6 100644
--- a/src/box/lua/feedback_daemon.lua
+++ b/src/box/lua/feedback_daemon.lua
@@ -327,12 +327,13 @@ local function feedback_loop(self)
     fiber.name(PREFIX, { truncate = true })
 
     while true do
-        local feedback = self:generate_feedback()
         local msg = self.control:get(self.interval)
         -- if msg == "send" then we simply send feedback
         if msg == "stop" then
             break
-        elseif feedback ~= nil then
+        end
+        local feedback = self:generate_feedback()
+        if feedback ~= nil then
             pcall(http.post, self.host, json.encode(feedback), {timeout=1})
         end
     end
-- 
2.24.3 (Apple Git-128)

Re: [Tarantool-patches] [PATCH 4/4] feedback_daemon: generate report right before sending

[Serge Petrenko via Tarantool-patches]

02.04.2021 17:58, Serge Petrenko пишет:
> Feedback daemon used to generate report before waiting (for an hour by
> default) until it's time to send it. Better actualize the reports and
> generate them right when it's time to send them.
>
> Follow-up #5750
> ---
>   src/box/lua/feedback_daemon.lua | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/box/lua/feedback_daemon.lua b/src/box/lua/feedback_daemon.lua
> index 2ce768642..0aab189b6 100644
> --- a/src/box/lua/feedback_daemon.lua
> +++ b/src/box/lua/feedback_daemon.lua
> @@ -327,12 +327,13 @@ local function feedback_loop(self)
>       fiber.name(PREFIX, { truncate = true })
>   
>       while true do
> -        local feedback = self:generate_feedback()
>           local msg = self.control:get(self.interval)
>           -- if msg == "send" then we simply send feedback
>           if msg == "stop" then
>               break
> -        elseif feedback ~= nil then
> +        end
> +        local feedback = self:generate_feedback()
> +        if feedback ~= nil then
>               pcall(http.post, self.host, json.encode(feedback), {timeout=1})
>           end
>       end
Pushed a new commit on top:

=====================================

Bump `feedback_version` to 7 and introduce a new field: `feedback.events`.
It holds a counter for every event we may choose to register later on.

Currently the possible events are "create_space", "drop_space",
"create_index", "drop_index".

All the registered events and corresponding counters are sent in a
report in `feedback.events` field.

Also, the first registered event triggers the report sending right away.
So, we may follow such events like "first space/index created/dropped"

Closes #5750
---
  src/box/lua/feedback_daemon.lua       | 26 ++++++++++++++++++++++++--
  src/box/lua/schema.lua                | 16 ++++++++++++++++
  test/box-tap/feedback_daemon.test.lua | 23 ++++++++++++++++++++++-
  3 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/src/box/lua/feedback_daemon.lua 
b/src/box/lua/feedback_daemon.lua
index 0aab189b6..ca93b7574 100644
--- a/src/box/lua/feedback_daemon.lua
+++ b/src/box/lua/feedback_daemon.lua
@@ -312,13 +312,18 @@ local function fill_in_stats(feedback)
      feedback.stats = stats
  end

-local function fill_in_feedback(feedback)
+local function fill_in_events(self, feedback)
+    feedback.events = self.cached_events
+end
+
+local function fill_in_feedback(self, feedback)
      fill_in_base_info(feedback)
      fill_in_platform_info(feedback)
      fill_in_repo_url(feedback)
      fill_in_features(feedback)
      fill_in_options(feedback)
      fill_in_stats(feedback)
+    fill_in_events(self, feedback)

      return feedback
  end
@@ -358,12 +363,26 @@ local function guard_loop(self)
      self.shutdown:put("stopped")
  end

+local function save_event(self, event)
+    if type(event) ~= 'string' then
+        error("Usage: box.internal.feedback_daemon.save_event(string)")
+    end
+    local cnt = self.cached_events[event] or 0
+    self.cached_events[event] = (self.cached_events[event] or 0) + 1
+    if self.cached_events[event] == 1 then
+        -- The first occurred event of this type triggers report dispatch
+        -- immediately.
+        self.send()
+    end
+end
+
  -- these functions are used for test purposes only
  local function start(self)
      self:stop()
      if self.enabled then
          self.control = fiber.channel()
          self.shutdown = fiber.channel()
+        self.cached_events = {}
          self.guard = fiber.create(guard_loop, self)
      end
      log.verbose("%s started", PREFIX)
@@ -402,7 +421,7 @@ setmetatable(daemon, {
          end,
          -- this function is used in saving feedback in file
          generate_feedback = function()
-            return fill_in_feedback({ feedback_version = 6 })
+            return fill_in_feedback(daemon, { feedback_version = 7 })
          end,
          start = function()
              start(daemon)
@@ -413,6 +432,9 @@ setmetatable(daemon, {
          reload = function()
              reload(daemon)
          end,
+        save_event = function(event)
+            save_event(daemon, event)
+        end,
          send = function()
              if daemon.control ~= nil then
                  daemon.control:put("send")
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 96503a50e..cf1b070af 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -462,6 +462,10 @@ box.schema.space.create = function(name, options)
      })
      _space:insert{id, uid, name, options.engine, options.field_count,
          space_options, format}
+
+    if internal.feedback_daemon ~= nil then
+        internal.feedback_daemon.save_event("create_space")
+    end
      return box.space[id], "created"
  end

@@ -531,6 +535,9 @@ box.schema.space.drop = function(space_id, 
space_name, opts)
              box.error(box.error.NO_SUCH_SPACE, space_name)
          end
      end
+    if internal.feedback_daemon ~= nil then
+        internal.feedback_daemon.save_event("drop_space")
+    end
  end

  box.schema.space.rename = function(space_id, space_name)
@@ -1237,6 +1244,11 @@ box.schema.index.create = function(space_id, 
name, options)
          local _func_index = box.space[box.schema.FUNC_INDEX_ID]
          _func_index:insert{space_id, iid, index_opts.func}
      end
+
+    if internal.feedback_daemon ~= nil then
+        internal.feedback_daemon.save_event("create_index")
+    end
+
      return space.index[name]
  end

@@ -1257,6 +1269,10 @@ box.schema.index.drop = function(space_id, index_id)
          _func_index:delete({v.space_id, v.index_id})
      end
      _index:delete{space_id, index_id}
+
+    if internal.feedback_daemon ~= nil then
+        internal.feedback_daemon.save_event("drop_index")
+    end
  end

  box.schema.index.rename = function(space_id, index_id, name)
diff --git a/test/box-tap/feedback_daemon.test.lua 
b/test/box-tap/feedback_daemon.test.lua
index 3d6bf1d9b..a2e041649 100755
--- a/test/box-tap/feedback_daemon.test.lua
+++ b/test/box-tap/feedback_daemon.test.lua
@@ -70,7 +70,7 @@ if not ok then
      os.exit(0)
  end

-test:plan(28)
+test:plan(30)

  local function check(message)
      while feedback_count < 1 do
@@ -293,5 +293,26 @@ check_stats(actual.stats)
  actual = daemon.generate_feedback()
  test:is(box.info.uptime, actual.uptime, "Server uptime is reported and 
is correct.")

+daemon.reload()
+actual = daemon.generate_feedback()
+
+local events_expected = {}
+test:is_deeply(actual.events, events_expected, "Events are empty 
initially.")
+
+box.schema.space.create('test')
+box.space.test:create_index('pk')
+box.space.test.index.pk:drop()
+box.space.test:drop()
+
+actual = daemon.generate_feedback()
+events_expected = {
+    create_space = 1,
+    create_index = 1,
+    drop_space = 1,
+    drop_index = 1,
+}
+
+test:is_deeply(actual.events, events_expected, "Events are counted 
correctly")
+
  test:check()
  os.exit(0)
-- 
2.24.3 (Apple Git-128)


-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH 4/4] feedback_daemon: generate report right before sending

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for the patch!

The patch is mostly fine, but I don't see why would we need it, as well
as why do we need to know when a first space is created. I don't argue,
only making a note. Unless it would affect the perf or normal code base
complexity anyhow.

See 2 comments below.

> diff --git a/src/box/lua/feedback_daemon.lua b/src/box/lua/feedback_daemon.lua
> index 0aab189b6..ca93b7574 100644
> --- a/src/box/lua/feedback_daemon.lua
> +++ b/src/box/lua/feedback_daemon.lua
> @@ -358,12 +363,26 @@ local function guard_loop(self)
>      self.shutdown:put("stopped")
>  end
> 
> +local function save_event(self, event)
> +    if type(event) ~= 'string' then
> +        error("Usage: box.internal.feedback_daemon.save_event(string)")
> +    end
> +    local cnt = self.cached_events[event] or 0

1. cnt is unused. Did it pass luacheck? I suppose you wanted something like
this:

====================
@@ -367,9 +367,9 @@ local function save_event(self, event)
     if type(event) ~= 'string' then
         error("Usage: box.internal.feedback_daemon.save_event(string)")
     end
-    local cnt = self.cached_events[event] or 0
-    self.cached_events[event] = (self.cached_events[event] or 0) + 1
-    if self.cached_events[event] == 1 then
+    local cnt = (self.cached_events[event] + 1) or 1
+    self.cached_events[event] = cnt
+    if cnt == 1 then
         -- The first occurred event of this type triggers report dispatch
         -- immediately.
         self.send()
====================

> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index 96503a50e..cf1b070af 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -462,6 +462,10 @@ box.schema.space.create = function(name, options)
>      })
>      _space:insert{id, uid, name, options.engine, options.field_count,
>          space_options, format}
> +
> +    if internal.feedback_daemon ~= nil then
> +        internal.feedback_daemon.save_event("create_space")

2. To make it less ugly you could save feedback_daemon into a local
variable in this file, and access it as an upvalue in all the
usages. Or wrap the entire 'save_event' thing here into a function
which checks that the daemon is not nil, and then call its
save_event function.

Re: [Tarantool-patches] [PATCH 4/4] feedback_daemon: generate report right before sending

[Serge Petrenko via Tarantool-patches]

05.04.2021 19:23, Vladislav Shpilevoy пишет:
> Thanks for the patch!
>
> The patch is mostly fine, but I don't see why would we need it, as well
> as why do we need to know when a first space is created. I don't argue,
> only making a note. Unless it would affect the perf or normal code base
> complexity anyhow.

Well, it was requested in the ticked, and I've asked Artur personally.
They want to monitor user's progress in tutorials or something alike.

So, they want to receive feedback as soon as the user starts tarantool
(calls box.cfg in our case), creates the first space, creates the first 
index
and so on.

I've discussed this with Mons, and he suggested to count such events always.

This patch adds all the needed machinery for that task.
>
> See 2 comments below.
>
>> diff --git a/src/box/lua/feedback_daemon.lua b/src/box/lua/feedback_daemon.lua
>> index 0aab189b6..ca93b7574 100644
>> --- a/src/box/lua/feedback_daemon.lua
>> +++ b/src/box/lua/feedback_daemon.lua
>> @@ -358,12 +363,26 @@ local function guard_loop(self)
>>       self.shutdown:put("stopped")
>>   end
>>
>> +local function save_event(self, event)
>> +    if type(event) ~= 'string' then
>> +        error("Usage: box.internal.feedback_daemon.save_event(string)")
>> +    end
>> +    local cnt = self.cached_events[event] or 0
> 1. cnt is unused. Did it pass luacheck? I suppose you wanted something like
> this:

Oops, I forgot to drop this line. Sorry.

>
> ====================
> @@ -367,9 +367,9 @@ local function save_event(self, event)
>       if type(event) ~= 'string' then
>           error("Usage: box.internal.feedback_daemon.save_event(string)")
>       end
> -    local cnt = self.cached_events[event] or 0
> -    self.cached_events[event] = (self.cached_events[event] or 0) + 1
> -    if self.cached_events[event] == 1 then
> +    local cnt = (self.cached_events[event] + 1) or 1
> +    self.cached_events[event] = cnt
> +    if cnt == 1 then
>           -- The first occurred event of this type triggers report dispatch
>           -- immediately.
>           self.send()
> ====================
>
>> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
>> index 96503a50e..cf1b070af 100644
>> --- a/src/box/lua/schema.lua
>> +++ b/src/box/lua/schema.lua
>> @@ -462,6 +462,10 @@ box.schema.space.create = function(name, options)
>>       })
>>       _space:insert{id, uid, name, options.engine, options.field_count,
>>           space_options, format}
>> +
>> +    if internal.feedback_daemon ~= nil then
>> +        internal.feedback_daemon.save_event("create_space")
> 2. To make it less ugly you could save feedback_daemon into a local
> variable in this file, and access it as an upvalue in all the
> usages. Or wrap the entire 'save_event' thing here into a function
> which checks that the daemon is not nil, and then call its
> save_event function.

Sure, let's wrap it in a function.
Incremental diff below.

===================================================

diff --git a/src/box/lua/feedback_daemon.lua 
b/src/box/lua/feedback_daemon.lua
index ca93b7574..fb0e0df35 100644
--- a/src/box/lua/feedback_daemon.lua
+++ b/src/box/lua/feedback_daemon.lua
@@ -367,7 +367,6 @@ local function save_event(self, event)
      if type(event) ~= 'string' then
          error("Usage: box.internal.feedback_daemon.save_event(string)")
      end
-    local cnt = self.cached_events[event] or 0
      self.cached_events[event] = (self.cached_events[event] or 0) + 1
      if self.cached_events[event] == 1 then
          -- The first occurred event of this type triggers report dispatch
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index cf1b070af..93ecf7440 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -321,6 +321,12 @@ local function update_param_table(table, defaults)
      return new_table
  end

+local function feedback_save_event(event)
+    if internal.feedback_daemon ~= nil then
+        internal.feedback_daemon.save_event(event)
+    end
+end
+
  box.begin = function()
      if builtin.box_txn_begin() == -1 then
          box.error()
@@ -463,9 +469,7 @@ box.schema.space.create = function(name, options)
      _space:insert{id, uid, name, options.engine, options.field_count,
          space_options, format}

-    if internal.feedback_daemon ~= nil then
-        internal.feedback_daemon.save_event("create_space")
-    end
+    feedback_save_event('create_space')
      return box.space[id], "created"
  end

@@ -535,9 +539,8 @@ box.schema.space.drop = function(space_id, 
space_name, opts)
              box.error(box.error.NO_SUCH_SPACE, space_name)
          end
      end
-    if internal.feedback_daemon ~= nil then
-        internal.feedback_daemon.save_event("drop_space")
-    end
+
+    feedback_save_event('drop_space')
  end

  box.schema.space.rename = function(space_id, space_name)
@@ -1245,10 +1248,7 @@ box.schema.index.create = function(space_id, 
name, options)
          _func_index:insert{space_id, iid, index_opts.func}
      end

-    if internal.feedback_daemon ~= nil then
-        internal.feedback_daemon.save_event("create_index")
-    end
-
+    feedback_save_event('create_index')
      return space.index[name]
  end

@@ -1270,9 +1270,7 @@ box.schema.index.drop = function(space_id, index_id)
      end
      _index:delete{space_id, index_id}

-    if internal.feedback_daemon ~= nil then
-        internal.feedback_daemon.save_event("drop_index")
-    end
+    feedback_save_event('drop_index')
  end

  box.schema.index.rename = function(space_id, index_id, name)

-- 
Serge Petrenko