From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id D73B642EF5D for ; Thu, 11 Jun 2020 22:32:47 +0300 (MSK) References: <67c75c01-8503-2355-e1f7-9644def2179c@tarantool.org> From: Vladislav Shpilevoy Message-ID: <6b8b0f63-0a3f-d3ae-7596-2611ea433d44@tarantool.org> Date: Thu, 11 Jun 2020 21:32:46 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 2/2] feedback: collect db engines and index features List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?0JjQu9GM0Y8g0JrQvtC90Y7RhdC+0LI=?= Cc: tarantool-patches@dev.tarantool.org, alexander.turenko@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.