* [Tarantool-patches] [PATCH 0/4] send feedback on tarantool start @ 2021-04-02 14:57 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 ` (3 more replies) 0 siblings, 4 replies; 12+ messages in thread From: Serge Petrenko via Tarantool-patches @ 2021-04-02 14:57 UTC (permalink / raw) To: v.shpilevoy, alexander.turenko; +Cc: 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) ^ permalink raw reply [flat|nested] 12+ messages in thread
* [Tarantool-patches] [PATCH 1/4] feedback_daemon: include server uptime in the report 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 ` 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 ` (2 subsequent siblings) 3 siblings, 0 replies; 12+ messages in thread From: Serge Petrenko via Tarantool-patches @ 2021-04-02 14:58 UTC (permalink / raw) To: v.shpilevoy, alexander.turenko; +Cc: 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) ^ permalink raw reply [flat|nested] 12+ messages in thread
* [Tarantool-patches] [PATCH 2/4] feedback_daemon: rename `send_test` to `send` 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 ` 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-02 14:58 ` [Tarantool-patches] [PATCH 4/4] feedback_daemon: generate report right before sending Serge Petrenko via Tarantool-patches 3 siblings, 0 replies; 12+ messages in thread From: Serge Petrenko via Tarantool-patches @ 2021-04-02 14:58 UTC (permalink / raw) To: v.shpilevoy, alexander.turenko; +Cc: 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) ^ permalink raw reply [flat|nested] 12+ messages in thread
* [Tarantool-patches] [PATCH 3/4] feedback_daemon: send feedback on server start 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 ` Serge Petrenko via Tarantool-patches 2021-04-05 13:18 ` Vladislav Shpilevoy 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 3 siblings, 1 reply; 12+ messages in thread From: Serge Petrenko via Tarantool-patches @ 2021-04-02 14:58 UTC (permalink / raw) To: v.shpilevoy, alexander.turenko; +Cc: 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) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/4] feedback_daemon: send feedback on server start 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 0 siblings, 1 reply; 12+ messages in thread From: Vladislav Shpilevoy via Tarantool-patches @ 2021-04-05 13:18 UTC (permalink / raw) To: Serge Petrenko, alexander.turenko; +Cc: 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? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/4] feedback_daemon: send feedback on server start 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 0 siblings, 1 reply; 12+ messages in thread From: Serge Petrenko via Tarantool-patches @ 2021-04-05 14:05 UTC (permalink / raw) To: Vladislav Shpilevoy, alexander.turenko; +Cc: 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/4] feedback_daemon: send feedback on server start 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 0 siblings, 1 reply; 12+ messages in thread From: Vladislav Shpilevoy via Tarantool-patches @ 2021-04-05 16:11 UTC (permalink / raw) To: Serge Petrenko, alexander.turenko; +Cc: 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? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/4] feedback_daemon: send feedback on server start 2021-04-05 16:11 ` Vladislav Shpilevoy via Tarantool-patches @ 2021-04-08 13:41 ` Serge Petrenko via Tarantool-patches 0 siblings, 0 replies; 12+ messages in thread From: Serge Petrenko via Tarantool-patches @ 2021-04-08 13:41 UTC (permalink / raw) To: Vladislav Shpilevoy, alexander.turenko; +Cc: 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [Tarantool-patches] [PATCH 4/4] feedback_daemon: generate report right before sending 2021-04-02 14:57 [Tarantool-patches] [PATCH 0/4] send feedback on tarantool start Serge Petrenko via Tarantool-patches ` (2 preceding siblings ...) 2021-04-02 14:58 ` [Tarantool-patches] [PATCH 3/4] feedback_daemon: send feedback on server start Serge Petrenko via Tarantool-patches @ 2021-04-02 14:58 ` Serge Petrenko via Tarantool-patches 2021-04-05 14:03 ` Serge Petrenko via Tarantool-patches 3 siblings, 1 reply; 12+ messages in thread From: Serge Petrenko via Tarantool-patches @ 2021-04-02 14:58 UTC (permalink / raw) To: v.shpilevoy, alexander.turenko; +Cc: 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) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/4] feedback_daemon: generate report right before sending 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 0 siblings, 1 reply; 12+ messages in thread From: Serge Petrenko via Tarantool-patches @ 2021-04-05 14:03 UTC (permalink / raw) To: v.shpilevoy, alexander.turenko; +Cc: 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/4] feedback_daemon: generate report right before sending 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 0 siblings, 1 reply; 12+ messages in thread From: Vladislav Shpilevoy via Tarantool-patches @ 2021-04-05 16:23 UTC (permalink / raw) To: Serge Petrenko, alexander.turenko; +Cc: 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Tarantool-patches] [PATCH 4/4] feedback_daemon: generate report right before sending 2021-04-05 16:23 ` Vladislav Shpilevoy via Tarantool-patches @ 2021-04-06 8:18 ` Serge Petrenko via Tarantool-patches 0 siblings, 0 replies; 12+ messages in thread From: Serge Petrenko via Tarantool-patches @ 2021-04-06 8:18 UTC (permalink / raw) To: Vladislav Shpilevoy, alexander.turenko; +Cc: 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-04-08 13:43 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox