Re: [Tarantool-patches] [PATCH 2/2] feedback: collect db engines and index features

["Илья Конюхов" <runsfor@gmail.com>]

Hi,

I’ve worked on your comments and fixed lapses. See comments below.

Besides that, are you ok with pushing this (Lua) patch forward? I understand your thoughts on implementing more robust solution in C, which would also cover some statistics gathering goals. I agree with it, but I think we need to design it first. Let me branch out an issue on this topic.

> On 11 Jun 2020, at 22:32, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Thanks for the fixes!
> 
> Generally fine. As a first set of simple stats. See a few
> more comments below.
> 
> First is about why I wanted to collect the stat from C, in a
> reactive way, rather than in a proactive way.
> 
> The main reason - C is cheaper. Also the collected stats could be
> useful for users, if we could design it in a good way. And then it
> could be collected more often than once in an hour.
> 
> The other reason - the schema cache should help in most of the
> cases, but I once saw a customer, who used spaces as temporary
> objects. From what I saw, it was code of a game, and spaces were
> lobbies. In such installation the space count may be huge, and
> schema change rate as well. So the cache becomes useless.
> 
> A reactive C implementation could just increment a set of int counters
> in a global struct in alter.cc right when a space/index is
> created/changed/dropped. It would cost nothing for everyone. And would
> be a good base for wider set of statistics. Which we clearly need.
> For example, Cyrill was struggling with lack of internal statistics
> when tried to bench relay threads performance.
> 
> See 4 comments below.
> 
>> diff --git a/src/box/lua/feedback_daemon.lua b/src/box/lua/feedback_daemon.lua
>> index 21e69d511..1f177a204 100644
>> --- a/src/box/lua/feedback_daemon.lua
>> +++ b/src/box/lua/feedback_daemon.lua
>> @@ -50,6 +50,25 @@ local function detect_docker_environment()
>>     return cached_detect_docker_env
>> end
>> 
>> +local function is_system_space(sp)
>> +    return box.schema.SYSTEM_ID_MIN <= sp.id and
>> +            sp.id <= box.schema.SYSTEM_ID_MAX
> 
> 1. Nit: please, align the second line by 'box' in the previous
> line. Like this:
> 
>    return box.schema.SYSTEM_ID_MIN <= sp.id and
>           sp.id <= box.schema.SYSTEM_ID_MAX

done.

> 
>> +end
>> +
>> +local function is_jsonpath_index(idx)
>> +    for _, part in pairs(idx.parts) do
>> +        if part.path ~= nil then
>> +            return true
>> +        end
>> +    end
>> +
>> +    return false
>> +end
>> +
>> +local function is_functional_index(idx)
>> +    return idx.func ~= nil
>> +end
>> +
>> local function fill_in_base_info(feedback)
>>     if box.info.status ~= "running" then
>>         return nil, "not running"
>> @@ -65,9 +84,98 @@ local function fill_in_platform_info(feedback)
>>     feedback.is_docker = detect_docker_environment()
>> end
>> 
>> +local function fill_in_indices_stats(space, stats)
>> +    if not space.index[0] then return end
> 
> 2. Looks like this condition is not necessary. The cycle anyway
> shouldn't do any iterations if the table is empty.

agree. fixed.

> 
>> +
>> +    for name, idx in pairs(space.index) do
>> +        if type(name) == 'number' then
>> +            local idx_type = idx.type
>> +            if idx_type == 'TREE' then
>> +                if is_functional_index(idx) then
>> +                    stats.functional = stats.functional + 1
>> +                elseif is_jsonpath_index(idx) then
>> +                    stats.jsonpath = stats.jsonpath + 1
>> +                end
>> +                stats.tree = stats.tree + 1
>> +            elseif idx_type == 'HASH' then
>> +                stats.hash = stats.hash + 1
>> +            elseif idx_type == 'RTREE' then
>> +                stats.rtree = stats.rtree + 1
>> +            elseif idx_type == 'BITSET' then
>> +                stats.bitset = stats.bitset + 1
>> +            end
>> +        end
>> +    end
>> +end
>> +
>> +local function fill_in_space_stats(features)
>> +    local spaces = {
>> +        memtx     = 0,
>> +        vinyl     = 0,
>> +        temporary = 0,
>> +        ['local'] = 0,
>> +    }
>> +
>> +    local indices = {
>> +        hash       = 0,
>> +        tree       = 0,
>> +        rtree      = 0,
>> +        bitset     = 0,
>> +        jsonpath   = 0,
>> +        functional = 0,
>> +    }
>> +
>> +    for name, space in pairs(box.space) do
>> +        local is_system = is_system_space(space)
>> +        if not is_system and type(name) == 'number' then
>> +            if space.engine == 'vinyl' then
>> +                spaces.vinyl = spaces.vinyl + 1
>> +            elseif space.engine == 'memtx' then
>> +                if space.temporary ~= nil then
>> +                    spaces.temporary = spaces.temporary + 1
>> +                end
>> +                spaces.memtx = spaces.memtx + 1
>> +            end
>> +            if space.is_local == false then
> 
> 3. Are you sure? Looks like a typo. Perhaps you meant:
> 
>    if space.is_local then
> 
> Without '== false'. Otherwise you count non-local spaces.
> How does it pass the tests now?

My bad. Thanks, fixed. Test happen to pass, because there were 2 spaces. 1 local and 1 non-local. Adjusted it.

> 
>> +local function fill_in_features_impl(features)
>> +    fill_in_space_stats(features)
>> +end
>> +
>> +local cached_schema_version = 0
>> +local cached_feedback_features = {}
> 
> 4. I would better move the cache handling into fill_in_space_stats().
> Because when you will add more features, not related to the schema,
> they won't relate to schema version.
> 
> fill_in_features() should not use any caches. Does not depend on schema.
> fill_in_space_stats() can use the cache. Because fetches info from the
> schema.
> 
> I mean, it works now, but we would need to rewrite that mostly, when
> more features will be collected.

Agree. Extracted out caching into fill_in_space_stats(), but kept it in fill_in_features(), since space_stats is semantically features.



diff --git a/src/box/lua/feedback_daemon.lua b/src/box/lua/feedback_daemon.lua
index 95130d696..111a70fca 100644
--- a/src/box/lua/feedback_daemon.lua
+++ b/src/box/lua/feedback_daemon.lua
@@ -26,13 +26,154 @@ local function get_fiber_id(f)
     return fid
 end
 
-local function fill_in_feedback(feedback)
+local function detect_docker_environment_impl()
+    local fh = fio.open('/proc/1/cgroup', {'O_RDONLY'})
+    if not fh then
+        return false
+    end
+
+    -- fh:read() doesn't read empty "proc" files
+    local big_enough_chunk = 4096
+    local s = fh:read(big_enough_chunk)
+    fh:close()
+
+    return s and s:find('docker') and true or false
+end
+
+local cached_detect_docker_env
+
+local function detect_docker_environment()
+    if cached_detect_docker_env == nil then
+        cached_detect_docker_env = detect_docker_environment_impl()
+    end
+
+    return cached_detect_docker_env
+end
+
+local function is_system_space(sp)
+    return box.schema.SYSTEM_ID_MIN <= sp.id and
+           sp.id <= box.schema.SYSTEM_ID_MAX
+end
+
+local function is_jsonpath_index(idx)
+    for _, part in pairs(idx.parts) do
+        if part.path ~= nil then
+            return true
+        end
+    end
+
+    return false
+end
+
+local function is_functional_index(idx)
+    return idx.func ~= nil
+end
+
+local function fill_in_base_info(feedback)
     if box.info.status ~= "running" then
         return nil, "not running"
     end
     feedback.tarantool_version = box.info.version
     feedback.server_id         = box.info.uuid
     feedback.cluster_id        = box.info.cluster.uuid
+end
+
+local function fill_in_platform_info(feedback)
+    feedback.os        = jit.os
+    feedback.arch      = jit.arch
+    feedback.is_docker = detect_docker_environment()
+end
+
+local function fill_in_indices_stats(space, stats)
+    for name, idx in pairs(space.index) do
+        if type(name) == 'number' then
+            local idx_type = idx.type
+            if idx_type == 'TREE' then
+                if is_functional_index(idx) then
+                    stats.functional = stats.functional + 1
+                elseif is_jsonpath_index(idx) then
+                    stats.jsonpath = stats.jsonpath + 1
+                end
+                stats.tree = stats.tree + 1
+            elseif idx_type == 'HASH' then
+                stats.hash = stats.hash + 1
+            elseif idx_type == 'RTREE' then
+                stats.rtree = stats.rtree + 1
+            elseif idx_type == 'BITSET' then
+                stats.bitset = stats.bitset + 1
+            end
+        end
+    end
+end
+
+local function fill_in_space_stats_impl(features)
+    local spaces = {
+        memtx     = 0,
+        vinyl     = 0,
+        temporary = 0,
+        ['local'] = 0,
+    }
+
+    local indices = {
+        hash       = 0,
+        tree       = 0,
+        rtree      = 0,
+        bitset     = 0,
+        jsonpath   = 0,
+        functional = 0,
+    }
+
+    for name, space in pairs(box.space) do
+        local is_system = is_system_space(space)
+        if not is_system and type(name) == 'number' then
+            if space.engine == 'vinyl' then
+                spaces.vinyl = spaces.vinyl + 1
+            elseif space.engine == 'memtx' then
+                if space.temporary then
+                    spaces.temporary = spaces.temporary + 1
+                end
+                spaces.memtx = spaces.memtx + 1
+            end
+            if space.is_local then
+                spaces['local'] = spaces['local'] + 1
+            end
+            fill_in_indices_stats(space, indices)
+        end
+        fiber.yield()
+    end
+
+    for k, v in pairs(spaces) do
+        features[k..'_spaces'] = v
+    end
+
+    for k, v in pairs(indices) do
+        features[k..'_indices'] = v
+    end
+end
+
+local cached_schema_version = 0
+local cached_feedback_features = {}
+
+local function fill_in_space_stats(feedback)
+    local schema_version = box.internal.schema_version()
+    if cached_schema_version < schema_version then
+        local features = {}
+        fill_in_space_stats_impl(features)
+        cached_schema_version = schema_version
+        cached_feedback_features = features
+    end
+    feedback.features = cached_feedback_features
+end
+
+local function fill_in_features(feedback)
+    fill_in_space_stats(feedback)
+end
+
+local function fill_in_feedback(feedback)
+    fill_in_base_info(feedback)
+    fill_in_platform_info(feedback)
+    fill_in_features(feedback)
+
     return feedback
 end