From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <tarantool-patches-bounces@dev.tarantool.org>
Received: from [87.239.111.99] (localhost [127.0.0.1])
	by dev.tarantool.org (Postfix) with ESMTP id 2F2D56EC5D;
	Tue,  6 Apr 2021 11:18:37 +0300 (MSK)
DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2F2D56EC5D
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev;
	t=1617697117; bh=iIVMtH0cg0HSXsXh9bSXdyBa2+eLj0pI4l56a+HsWrs=;
	h=To:Cc:References:Date:In-Reply-To:Subject:List-Id:
	 List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe:
	 From:Reply-To:From;
	b=HC8dY2pQwbyohjdcH28xRuGu0O+8saJ5qC6wXwG5mcfUmnaKXEQOnusPalQrPfDnK
	 h1MszVMY6OiOSdHHo5OYbuvuVsTavBL6nkIBPryhvh70stmrmLF4c73Rv7SCcM7h6w
	 nYs4ykdHxAcQQ70OGXukw4pTvt0v76JHKEGqnxW8=
Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108])
 (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 C45A06EC5D
 for <tarantool-patches@dev.tarantool.org>;
 Tue,  6 Apr 2021 11:18:35 +0300 (MSK)
DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C45A06EC5D
Received: by smtp48.i.mail.ru with esmtpa (envelope-from
 <sergepetrenko@tarantool.org>)
 id 1lTguw-0004QU-Uy; Tue, 06 Apr 2021 11:18:35 +0300
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>,
 alexander.turenko@tarantool.org
Cc: tarantool-patches@dev.tarantool.org
References: <cover.1617375300.git.sergepetrenko@tarantool.org>
 <44f4b260e0ac435fe78282c1f6f4607159914935.1617375300.git.sergepetrenko@tarantool.org>
 <6216debc-5807-ede1-df21-823d60a1e70b@tarantool.org>
 <36dca5a2-9e18-e517-78d2-827b85cbc3e1@tarantool.org>
Message-ID: <55ccac3e-1c39-46b4-2278-d101aac1f08f@tarantool.org>
Date: Tue, 6 Apr 2021 11:18:34 +0300
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.9.0
MIME-Version: 1.0
In-Reply-To: <36dca5a2-9e18-e517-78d2-827b85cbc3e1@tarantool.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-GB
X-7564579A: 646B95376F6C166E
X-77F55803: 4F1203BC0FB41BD9ED7173E37F4E32947A0146560F8BA70927CAA5B950F38D9F182A05F538085040C7918A5EF374E64AF10A5FBD267416A4967253BB86F48043E593992D54DCF510
X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE79B4AFF82F0254274EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637D919B403F35891598638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95C16EE06F5A270FE6A6109B72B0CE018C55D1248891792501CA471835C12D1D9774AD6D5ED66289B5278DA827A17800CE70F3DDF2BBF19B93A9FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3A12191B5F2BB8629117882F4460429728AD0CFFFB425014E868A13BD56FB6657D81D268191BDAD3DC09775C1D3CA48CFEEA082C9A12FE45576E601842F6C81A12EF20D2F80756B5F7E9C4E3C761E06A776E601842F6C81A127C277FBC8AE2E8BB4A6D9B00E37F4B33AA81AA40904B5D9DBF02ECDB25306B2201CA6A4E26CD07C3BBE47FD9DD3FB595F5C1EE8F4F765FC72CEEB2601E22B093A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4931B544F03EFBC4D57FC839A7D10C5E1E9C4224003CC83647689D4C264860C145E
X-C1DE0DAB: 0D63561A33F958A5245E6BD1ED39F6F629390FADDFF5EB29461A71EE16DCEBCED59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0
X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34D8C933888226C8412159C95B001BED35C5C8E1FAFBA749358B880F6FB38E212FFCDEA2F31813315A1D7E09C32AA3244C62EAA3CE9C44F06AC12CDCB3F349B463D08D48398F32B4A6FACE5A9C96DEB163
X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojM00ve/f+0ome6mZFGmOnUQ==
X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A769310BB5A2F8068DC9F10A5FBD267416A40D95629463F16A6A32C2A64043BFB05F66FEC6BF5C9C28D9D2F9AC31ED4B18F0B80F102789B70DF27402F9BA4338D657ED14614B50AE0675
X-Mras: Ok
Subject: Re: [Tarantool-patches] [PATCH 4/4] feedback_daemon: generate
 report right before sending
X-BeenThere: tarantool-patches@dev.tarantool.org
X-Mailman-Version: 2.1.34
Precedence: list
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>
From: Serge Petrenko via Tarantool-patches
 <tarantool-patches@dev.tarantool.org>
Reply-To: Serge Petrenko <sergepetrenko@tarantool.org>
Errors-To: tarantool-patches-bounces@dev.tarantool.org
Sender: "Tarantool-patches" <tarantool-patches-bounces@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