[Tarantool-patches] [PATCH v2 0/5] send feedback on start and on key events

[Tarantool-patches] [PATCH v2 0/5] send feedback on start and on key events

[Serge Petrenko via Tarantool-patches]

Changes in v2:
  - add event reporting in patch 5
  - add a workaround for failing tests:
    sleep for 10 seconds before the first ever report dispatch.
  - minor review fixes

https://github.com/tarantool/tarantool/issues/5750
https://github.com/tarantool/tarantool/tree/sp/gh-5750-feedback-on-start

Serge Petrenko (5):
  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
  feedback_daemon: count and report some events

 src/box/lua/feedback_daemon.lua       | 49 ++++++++++++++++++++++----
 src/box/lua/load_cfg.lua              | 34 ++++++++++++------
 src/box/lua/schema.lua                | 14 ++++++++
 test/box-tap/feedback_daemon.test.lua | 50 ++++++++++++++++++++-------
 4 files changed, 118 insertions(+), 29 deletions(-)

-- 
2.24.3 (Apple Git-128)

[Tarantool-patches] [PATCH v2 1/5] feedback_daemon: include server uptime in the report

[Serge Petrenko via 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)

[Tarantool-patches] [PATCH v2 2/5] feedback_daemon: rename `send_test` to `send`

[Serge Petrenko via 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)

[Tarantool-patches] [PATCH v2 3/5] feedback_daemon: send feedback on server start

[Serge Petrenko via Tarantool-patches]

Send the first report as soon as instance's initial configuration
finishes.

Part of #5750
---
 src/box/lua/feedback_daemon.lua | 10 ++++++++++
 src/box/lua/load_cfg.lua        | 34 ++++++++++++++++++++++-----------
 2 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/src/box/lua/feedback_daemon.lua b/src/box/lua/feedback_daemon.lua
index d00eedf39..43bbc1fa2 100644
--- a/src/box/lua/feedback_daemon.lua
+++ b/src/box/lua/feedback_daemon.lua
@@ -323,6 +323,12 @@ local function fill_in_feedback(feedback)
     return feedback
 end
 
+-- fixme: remove this hack.
+-- It's here to prevent too early feedback sending.
+-- This leads to problems with thread sanitization after fork() on Mac OS.
+-- Google objc_initializeAfterForkError for details.
+local is_first_send = true
+
 local function feedback_loop(self)
     fiber.name(PREFIX, { truncate = true })
 
@@ -333,6 +339,10 @@ local function feedback_loop(self)
         if msg == "stop" then
             break
         elseif feedback ~= nil then
+            if is_first_send then
+                fiber.sleep(10)
+                is_first_send = nil
+            end
             pcall(http.post, self.host, json.encode(feedback), {timeout=1})
         end
     end
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)

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

[Serge Petrenko via 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.

Part of #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 43bbc1fa2..93bbb2a11 100644
--- a/src/box/lua/feedback_daemon.lua
+++ b/src/box/lua/feedback_daemon.lua
@@ -333,12 +333,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
             if is_first_send then
                 fiber.sleep(10)
                 is_first_send = nil
-- 
2.24.3 (Apple Git-128)

[Tarantool-patches] [PATCH v2 5/5] feedback_daemon: count and report some events

[Serge Petrenko via Tarantool-patches]

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       | 31 ++++++++++++++++++++++++---
 src/box/lua/schema.lua                | 14 ++++++++++++
 test/box-tap/feedback_daemon.test.lua | 23 +++++++++++++++++++-
 3 files changed, 64 insertions(+), 4 deletions(-)

diff --git a/src/box/lua/feedback_daemon.lua b/src/box/lua/feedback_daemon.lua
index 93bbb2a11..660ef8403 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
@@ -368,12 +373,29 @@ 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
+    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()
+        -- There may be up to 5 fibers triggering a send during bootstrap or
+        -- shortly after it. And maybe more to come. Do not make anyone wait for
+        -- feedback daemon to process the incoming events, and set channel size
+        -- to 10 just in case.
+        self.control = fiber.channel(10)
         self.shutdown = fiber.channel()
+        self.cached_events = {}
         self.guard = fiber.create(guard_loop, self)
     end
     log.verbose("%s started", PREFIX)
@@ -412,7 +434,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)
@@ -423,6 +445,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..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()
@@ -462,6 +468,8 @@ box.schema.space.create = function(name, options)
     })
     _space:insert{id, uid, name, options.engine, options.field_count,
         space_options, format}
+
+    feedback_save_event('create_space')
     return box.space[id], "created"
 end
 
@@ -531,6 +539,8 @@ box.schema.space.drop = function(space_id, space_name, opts)
             box.error(box.error.NO_SUCH_SPACE, space_name)
         end
     end
+
+    feedback_save_event('drop_space')
 end
 
 box.schema.space.rename = function(space_id, space_name)
@@ -1237,6 +1247,8 @@ 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
+
+    feedback_save_event('create_index')
     return space.index[name]
 end
 
@@ -1257,6 +1269,8 @@ 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}
+
+    feedback_save_event('drop_index')
 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)

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

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for the patch!

On 08.04.2021 15:38, Serge Petrenko via Tarantool-patches wrote:
> 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.

Now there is the 10 seconds delay for the first send. Maybe worth
generating the feedback after the sleep on Mac.

Re: [Tarantool-patches] [PATCH v2 3/5] feedback_daemon: send feedback on server start

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Thanks for the patchset!

On 08.04.2021 15:38, Serge Petrenko via Tarantool-patches wrote:
> Send the first report as soon as instance's initial configuration
> finishes.
> 
> Part of #5750
> ---
>  src/box/lua/feedback_daemon.lua | 10 ++++++++++
>  src/box/lua/load_cfg.lua        | 34 ++++++++++++++++++++++-----------
>  2 files changed, 33 insertions(+), 11 deletions(-)
> 
> diff --git a/src/box/lua/feedback_daemon.lua b/src/box/lua/feedback_daemon.lua
> index d00eedf39..43bbc1fa2 100644
> --- a/src/box/lua/feedback_daemon.lua
> +++ b/src/box/lua/feedback_daemon.lua
> @@ -323,6 +323,12 @@ local function fill_in_feedback(feedback)
>      return feedback
>  end
>  
> +-- fixme: remove this hack.
> +-- It's here to prevent too early feedback sending.
> +-- This leads to problems with thread sanitization after fork() on Mac OS.
> +-- Google objc_initializeAfterForkError for details.
> +local is_first_send = true

Have you tried this?
https://stackoverflow.com/questions/50168647/multiprocessing-causes-python-to-crash-and-gives-an-error-may-have-been-in-progr

We could add it to the CI jobs so the env variable would be
set. And set it manually on our own machines.

You can also use jit.os to start it without a delay on
non-Mac machines. On Mac jit.os == 'OSX'. I see we use it
in some places, so it looks legal and working.

Re: [Tarantool-patches] [PATCH v2 3/5] feedback_daemon: send feedback on server start

[Serge Petrenko via Tarantool-patches]

10.04.2021 00:53, Vladislav Shpilevoy пишет:
> Hi! Thanks for the patchset!

Thanks for the review!

>
> On 08.04.2021 15:38, Serge Petrenko via Tarantool-patches wrote:
>> Send the first report as soon as instance's initial configuration
>> finishes.
>>
>> Part of #5750
>> ---
>>   src/box/lua/feedback_daemon.lua | 10 ++++++++++
>>   src/box/lua/load_cfg.lua        | 34 ++++++++++++++++++++++-----------
>>   2 files changed, 33 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/box/lua/feedback_daemon.lua b/src/box/lua/feedback_daemon.lua
>> index d00eedf39..43bbc1fa2 100644
>> --- a/src/box/lua/feedback_daemon.lua
>> +++ b/src/box/lua/feedback_daemon.lua
>> @@ -323,6 +323,12 @@ local function fill_in_feedback(feedback)
>>       return feedback
>>   end
>>   
>> +-- fixme: remove this hack.
>> +-- It's here to prevent too early feedback sending.
>> +-- This leads to problems with thread sanitization after fork() on Mac OS.
>> +-- Google objc_initializeAfterForkError for details.
>> +local is_first_send = true
> Have you tried this?
> https://stackoverflow.com/questions/50168647/multiprocessing-causes-python-to-crash-and-gives-an-error-may-have-been-in-progr

Yes, I have. I was worried there are some test failures in CI caused by
curl's too early initialization, which wouldn't be solved by setting that
env variable. Actually there is such a problem in CI. I've tried a patch
with sleeping only on Mac, and release_asan_clang
job has a lot of failed tests, each reporting a leak from
CRYPTO_memdup/CRYPTO_strdup/CRYPTO_strndup:
https://github.com/tarantool/tarantool/runs/2313054777?check_suite_focus=true

Moreover, I'm afraid that setting that env variable we may miss any further
alerts from Mac's thread/fork sanitizer.

Long story short, let's leave this hack as is for now.

Here's the patch I was trying to apply:

diff --git a/src/box/lua/feedback_daemon.lua 
b/src/box/lua/feedback_daemon.lua
index 43bbc1fa2..cc4e24559 100644
--- a/src/box/lua/feedback_daemon.lua
+++ b/src/box/lua/feedback_daemon.lua
@@ -327,7 +327,7 @@ end
  -- It's here to prevent too early feedback sending.
  -- This leads to problems with thread sanitization after fork() on Mac OS.
  -- Google objc_initializeAfterForkError for details.
-local is_first_send = true
+local is_first_send = jit.os == 'OSX' and true or nil

  local function feedback_loop(self)
      fiber.name(PREFIX, { truncate = true })
>
> We could add it to the CI jobs so the env variable would be
> set. And set it manually on our own machines.
>
> You can also use jit.os to start it without a delay on
> non-Mac machines. On Mac jit.os == 'OSX'. I see we use it
> in some places, so it looks legal and working.

-- 
Serge Petrenko

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

[Serge Petrenko via Tarantool-patches]

10.04.2021 00:53, Vladislav Shpilevoy пишет:
> Thanks for the patch!
>
> On 08.04.2021 15:38, Serge Petrenko via Tarantool-patches wrote:
>> 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.
> Now there is the 10 seconds delay for the first send. Maybe worth
> generating the feedback after the sleep on Mac.
Sure, thanks for noticing!

Incremental diff:
=====================================

diff --git a/src/box/lua/feedback_daemon.lua 
b/src/box/lua/feedback_daemon.lua
index 93bbb2a11..64460f5f4 100644
--- a/src/box/lua/feedback_daemon.lua
+++ b/src/box/lua/feedback_daemon.lua
@@ -338,12 +338,12 @@ local function feedback_loop(self)
          if msg == "stop" then
              break
          end
+        if is_first_send then
+            fiber.sleep(10)
+            is_first_send = nil
+        end
          local feedback = self:generate_feedback()
          if feedback ~= nil then
-            if is_first_send then
-                fiber.sleep(10)
-                is_first_send = nil
-            end
              pcall(http.post, self.host, json.encode(feedback), 
{timeout=1})
          end
      end

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH v2 0/5] send feedback on start and on key events

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for the patchset, Sergey! Technically the patchset looks bearable.
Below text is for the people who forced us to do this ticket.


Disclaimer: this is not toxic. I mostly just ask questions and express
my opinion.

I don't like the patch. The hack to workaround some CI issues with a sleep,
and the behaviour we introduce on the whole. I don't think creation of a
first space/index makes any sense to track.

The reason about "going through a tutorial" is a very weak argument. Spaces
are created not only as a part of a tutorial obviously. And I see no other
reason for sending these "events".

This rant is for Mons, Artur, and whoever else pushed sending of that useless
information and introduction of a **known bug** which we hack with 10s sleep. I
bet they have no idea how to really use it **exactly**, not in some abstract
way they will try to figure out later, and they just wanted to send "something"
to close some story-points.

We on purpose **push buggy code** only to send these feedback events we don't
know what to do with.

Here is what I see in the issue:

	In order for us to assess the real situation for the product, it is
	necessary to send feedback from the product immediately after the
	launch and after the key actions are completed.

How the hell is it related to a "real situation for the product"? How is it
"necessary"? What was wrong with the feedback we have now? How are the
instances working less than 1 hour are so important? And why do you need to
know exactly when a first space/index was created? Well, even if we need it,
we could remember the timestamps and send them with the next regular report.

Another cite:

	If Tarantool was installed using a script from the Download page,
	then you need to send feedback immediately and further after the
	following events are completed:

	...

	The rest of Tarantool that were downloaded outside of the Download
	page does not contain a special meta-file and should be considered as
	internal usage and no feedback sent from them. These are usually CI
	instances.

What is that "metafile"? We send the feedback anyway regardless of some metafile
in this patch, and from where the executable file was downloaded. Which again
renders this "feature" absolutely useless. Besides, creation of a first index
and space won't cover instances which don't create any indexes and spaces. For
example, vshard routers.

Additionally, implementation of this code stole time from the scaling team on
writing the code, doing the reviews, doing the review fixes. How is it even
related to the scaling team at all? Why was it so urgent and more important than
finishing a fix for a serious bug in the synchronous replication? I would propose
Artur to send a PR for that next time. Tickets like that should go to the
wishlist right away, instead of the known crashes and optimizations.

I hope we will drop these feedback events in the future when will realize it
does not help anyhow with a problem which can't even be properly formulated,
and because it introduces a bug we don't know for sure why is happening, and
how can be fixed except the 10s sleep hack.

Re: [Tarantool-patches] [PATCH v2 0/5] send feedback on start and on key events

[Kirill Yukhin via Tarantool-patches]

Hello,

On 11 апр 16:15, Vladislav Shpilevoy via Tarantool-patches wrote:
> Thanks for the patchset, Sergey! Technically the patchset looks bearable.
> Below text is for the people who forced us to do this ticket.
> 
> 
> Disclaimer: this is not toxic. I mostly just ask questions and express
> my opinion.
> 
> I don't like the patch. The hack to workaround some CI issues with a sleep,
> and the behaviour we introduce on the whole. I don't think creation of a
> first space/index makes any sense to track.
> 
> The reason about "going through a tutorial" is a very weak argument. Spaces
> are created not only as a part of a tutorial obviously. And I see no other
> reason for sending these "events".
> 
> This rant is for Mons, Artur, and whoever else pushed sending of that useless
> information and introduction of a **known bug** which we hack with 10s sleep. I
> bet they have no idea how to really use it **exactly**, not in some abstract
> way they will try to figure out later, and they just wanted to send "something"
> to close some story-points.
> 
> We on purpose **push buggy code** only to send these feedback events we don't
> know what to do with.
> 
> Here is what I see in the issue:
> 
> 	In order for us to assess the real situation for the product, it is
> 	necessary to send feedback from the product immediately after the
> 	launch and after the key actions are completed.
> 
> How the hell is it related to a "real situation for the product"? How is it
> "necessary"? What was wrong with the feedback we have now? How are the
> instances working less than 1 hour are so important? And why do you need to
> know exactly when a first space/index was created? Well, even if we need it,
> we could remember the timestamps and send them with the next regular report.
> 
> Another cite:
> 
> 	If Tarantool was installed using a script from the Download page,
> 	then you need to send feedback immediately and further after the
> 	following events are completed:
> 
> 	...
> 
> 	The rest of Tarantool that were downloaded outside of the Download
> 	page does not contain a special meta-file and should be considered as
> 	internal usage and no feedback sent from them. These are usually CI
> 	instances.
> 
> What is that "metafile"? We send the feedback anyway regardless of some metafile
> in this patch, and from where the executable file was downloaded. Which again
> renders this "feature" absolutely useless. Besides, creation of a first index
> and space won't cover instances which don't create any indexes and spaces. For
> example, vshard routers.
> 
> Additionally, implementation of this code stole time from the scaling team on
> writing the code, doing the reviews, doing the review fixes. How is it even
> related to the scaling team at all? Why was it so urgent and more important than
> finishing a fix for a serious bug in the synchronous replication? I would propose
> Artur to send a PR for that next time. Tickets like that should go to the
> wishlist right away, instead of the known crashes and optimizations.
> 
> I hope we will drop these feedback events in the future when will realize it
> does not help anyhow with a problem which can't even be properly formulated,
> and because it introduces a bug we don't know for sure why is happening, and
> how can be fixed except the 10s sleep hack.

This was our commitement, so it should be done. I'll escalate this to our PMs.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH v2 0/5] send feedback on start and on key events

[Kirill Yukhin via Tarantool-patches]

Hello,

On 08 апр 16:38, Serge Petrenko via Tarantool-patches wrote:
> Changes in v2:
>   - add event reporting in patch 5
>   - add a workaround for failing tests:
>     sleep for 10 seconds before the first ever report dispatch.
>   - minor review fixes
> 
> https://github.com/tarantool/tarantool/issues/5750
> https://github.com/tarantool/tarantool/tree/sp/gh-5750-feedback-on-start
> 
> Serge Petrenko (5):
>   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
>   feedback_daemon: count and report some events
> 
>  src/box/lua/feedback_daemon.lua       | 49 ++++++++++++++++++++++----
>  src/box/lua/load_cfg.lua              | 34 ++++++++++++------
>  src/box/lua/schema.lua                | 14 ++++++++
>  test/box-tap/feedback_daemon.test.lua | 50 ++++++++++++++++++++-------
>  4 files changed, 118 insertions(+), 29 deletions(-)

I've checked your patchset into master.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH v2 5/5] feedback_daemon: count and report some events

[Alexander Turenko via Tarantool-patches]

> @@ -462,6 +468,8 @@ box.schema.space.create = function(name, options)
>      })
>      _space:insert{id, uid, name, options.engine, options.field_count,
>          space_options, format}
> +
> +    feedback_save_event('create_space')
>      return box.space[id], "created"
>  end

What the counter will show, if I bootstrap an instance from an existing
snapshot with N spaces and add one more?

Will it count spaces that starts from the underscore symbol (queue and
memcached modules create such spaces)?

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH v2 0/5] send feedback on start and on key events

[Alexander Turenko via Tarantool-patches]

> This was our commitement, so it should be done. I'll escalate this to
> our PMs.

Can you share results of the discussion with PMs here?

I agree with Vlad: it is hard to imagine how to extract something useful
from the reported information.

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH v2 5/5] feedback_daemon: count and report some events

[Serge Petrenko via Tarantool-patches]

13.04.2021 11:31, Alexander Turenko пишет:
>> @@ -462,6 +468,8 @@ box.schema.space.create = function(name, options)
>>       })
>>       _space:insert{id, uid, name, options.engine, options.field_count,
>>           space_options, format}
>> +
>> +    feedback_save_event('create_space')
>>       return box.space[id], "created"
>>   end
> What the counter will show, if I bootstrap an instance from an existing
> snapshot with N spaces and add one more?
The counter will be 1 again. It shows 'how many events have occured 
since tarantool
startup`. An event here is user calling `box.schema.space.create`
>
> Will it count spaces that starts from the underscore symbol (queue and
> memcached modules create such spaces)?

Yes, it counts every space which was created with `box.schema.space.create`.

If you do `_space:insert{new_space_id, new_space_name, ...}` (I don't 
remember
the exact format but you get the idea), such an event won't
be counted.
>
> WBR, Alexander Turenko.

-- 
Serge Petrenko