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

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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