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
prev parent 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