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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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

> +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.

> +
> +    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?

> +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.