Tarantool development patches archive
 help / color / mirror / Atom feed
From: Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>,
	alexander.turenko@tarantool.org
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 4/4] feedback_daemon: generate report right before sending
Date: Tue, 6 Apr 2021 11:18:34 +0300	[thread overview]
Message-ID: <55ccac3e-1c39-46b4-2278-d101aac1f08f@tarantool.org> (raw)
In-Reply-To: <36dca5a2-9e18-e517-78d2-827b85cbc3e1@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


      reply	other threads:[~2021-04-06  8:18 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-02 14:57 [Tarantool-patches] [PATCH 0/4] send feedback on tarantool start Serge Petrenko via Tarantool-patches
2021-04-02 14:58 ` [Tarantool-patches] [PATCH 1/4] feedback_daemon: include server uptime in the report Serge Petrenko via Tarantool-patches
2021-04-02 14:58 ` [Tarantool-patches] [PATCH 2/4] feedback_daemon: rename `send_test` to `send` Serge Petrenko via Tarantool-patches
2021-04-02 14:58 ` [Tarantool-patches] [PATCH 3/4] feedback_daemon: send feedback on server start Serge Petrenko via Tarantool-patches
2021-04-05 13:18   ` Vladislav Shpilevoy via Tarantool-patches
2021-04-05 14:05     ` Serge Petrenko via Tarantool-patches
2021-04-05 16:11       ` Vladislav Shpilevoy via Tarantool-patches
2021-04-08 13:41         ` Serge Petrenko via Tarantool-patches
2021-04-02 14:58 ` [Tarantool-patches] [PATCH 4/4] feedback_daemon: generate report right before sending Serge Petrenko via Tarantool-patches
2021-04-05 14:03   ` Serge Petrenko via Tarantool-patches
2021-04-05 16:23     ` Vladislav Shpilevoy via Tarantool-patches
2021-04-06  8:18       ` Serge Petrenko via Tarantool-patches [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=55ccac3e-1c39-46b4-2278-d101aac1f08f@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=alexander.turenko@tarantool.org \
    --cc=sergepetrenko@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 4/4] feedback_daemon: generate report right before sending' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox