From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-f195.google.com (mail-lj1-f195.google.com [209.85.208.195]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id F223A4429E1 for ; Wed, 17 Jun 2020 11:59:45 +0300 (MSK) Received: by mail-lj1-f195.google.com with SMTP id s1so1946295ljo.0 for ; Wed, 17 Jun 2020 01:59:45 -0700 (PDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\)) From: =?utf-8?B?0JjQu9GM0Y8g0JrQvtC90Y7RhdC+0LI=?= In-Reply-To: <6b8b0f63-0a3f-d3ae-7596-2611ea433d44@tarantool.org> Date: Wed, 17 Jun 2020 11:59:35 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <6D02837E-BBEB-4CB9-806F-D5D8E66014B1@gmail.com> References: <67c75c01-8503-2355-e1f7-9644def2179c@tarantool.org> <6b8b0f63-0a3f-d3ae-7596-2611ea433d44@tarantool.org> 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: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Hi, I=E2=80=99ve 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 = wrote: >=20 > Thanks for the fixes! >=20 > Generally fine. As a first set of simple stats. See a few > more comments below. >=20 > First is about why I wanted to collect the stat from C, in a > reactive way, rather than in a proactive way. >=20 > 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. >=20 > 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. =46rom 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. >=20 > 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. >=20 > See 4 comments below. >=20 >> 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 >>=20 >> +local function is_system_space(sp) >> + return box.schema.SYSTEM_ID_MIN <=3D sp.id and >> + sp.id <=3D box.schema.SYSTEM_ID_MAX >=20 > 1. Nit: please, align the second line by 'box' in the previous > line. Like this: >=20 > return box.schema.SYSTEM_ID_MIN <=3D sp.id and > sp.id <=3D box.schema.SYSTEM_ID_MAX done. >=20 >> +end >> + >> +local function is_jsonpath_index(idx) >> + for _, part in pairs(idx.parts) do >> + if part.path ~=3D nil then >> + return true >> + end >> + end >> + >> + return false >> +end >> + >> +local function is_functional_index(idx) >> + return idx.func ~=3D nil >> +end >> + >> local function fill_in_base_info(feedback) >> if box.info.status ~=3D "running" then >> return nil, "not running" >> @@ -65,9 +84,98 @@ local function fill_in_platform_info(feedback) >> feedback.is_docker =3D detect_docker_environment() >> end >>=20 >> +local function fill_in_indices_stats(space, stats) >> + if not space.index[0] then return end >=20 > 2. Looks like this condition is not necessary. The cycle anyway > shouldn't do any iterations if the table is empty. agree. fixed. >=20 >> + >> + for name, idx in pairs(space.index) do >> + if type(name) =3D=3D 'number' then >> + local idx_type =3D idx.type >> + if idx_type =3D=3D 'TREE' then >> + if is_functional_index(idx) then >> + stats.functional =3D stats.functional + 1 >> + elseif is_jsonpath_index(idx) then >> + stats.jsonpath =3D stats.jsonpath + 1 >> + end >> + stats.tree =3D stats.tree + 1 >> + elseif idx_type =3D=3D 'HASH' then >> + stats.hash =3D stats.hash + 1 >> + elseif idx_type =3D=3D 'RTREE' then >> + stats.rtree =3D stats.rtree + 1 >> + elseif idx_type =3D=3D 'BITSET' then >> + stats.bitset =3D stats.bitset + 1 >> + end >> + end >> + end >> +end >> + >> +local function fill_in_space_stats(features) >> + local spaces =3D { >> + memtx =3D 0, >> + vinyl =3D 0, >> + temporary =3D 0, >> + ['local'] =3D 0, >> + } >> + >> + local indices =3D { >> + hash =3D 0, >> + tree =3D 0, >> + rtree =3D 0, >> + bitset =3D 0, >> + jsonpath =3D 0, >> + functional =3D 0, >> + } >> + >> + for name, space in pairs(box.space) do >> + local is_system =3D is_system_space(space) >> + if not is_system and type(name) =3D=3D 'number' then >> + if space.engine =3D=3D 'vinyl' then >> + spaces.vinyl =3D spaces.vinyl + 1 >> + elseif space.engine =3D=3D 'memtx' then >> + if space.temporary ~=3D nil then >> + spaces.temporary =3D spaces.temporary + 1 >> + end >> + spaces.memtx =3D spaces.memtx + 1 >> + end >> + if space.is_local =3D=3D false then >=20 > 3. Are you sure? Looks like a typo. Perhaps you meant: >=20 > if space.is_local then >=20 > Without '=3D=3D 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. >=20 >> +local function fill_in_features_impl(features) >> + fill_in_space_stats(features) >> +end >> + >> +local cached_schema_version =3D 0 >> +local cached_feedback_features =3D {} >=20 > 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. >=20 > 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. >=20 > 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 =20 -local function fill_in_feedback(feedback) +local function detect_docker_environment_impl() + local fh =3D 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 =3D 4096 + local s =3D 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 =3D=3D nil then + cached_detect_docker_env =3D detect_docker_environment_impl() + end + + return cached_detect_docker_env +end + +local function is_system_space(sp) + return box.schema.SYSTEM_ID_MIN <=3D sp.id and + sp.id <=3D box.schema.SYSTEM_ID_MAX +end + +local function is_jsonpath_index(idx) + for _, part in pairs(idx.parts) do + if part.path ~=3D nil then + return true + end + end + + return false +end + +local function is_functional_index(idx) + return idx.func ~=3D nil +end + +local function fill_in_base_info(feedback) if box.info.status ~=3D "running" then return nil, "not running" end feedback.tarantool_version =3D box.info.version feedback.server_id =3D box.info.uuid feedback.cluster_id =3D box.info.cluster.uuid +end + +local function fill_in_platform_info(feedback) + feedback.os =3D jit.os + feedback.arch =3D jit.arch + feedback.is_docker =3D detect_docker_environment() +end + +local function fill_in_indices_stats(space, stats) + for name, idx in pairs(space.index) do + if type(name) =3D=3D 'number' then + local idx_type =3D idx.type + if idx_type =3D=3D 'TREE' then + if is_functional_index(idx) then + stats.functional =3D stats.functional + 1 + elseif is_jsonpath_index(idx) then + stats.jsonpath =3D stats.jsonpath + 1 + end + stats.tree =3D stats.tree + 1 + elseif idx_type =3D=3D 'HASH' then + stats.hash =3D stats.hash + 1 + elseif idx_type =3D=3D 'RTREE' then + stats.rtree =3D stats.rtree + 1 + elseif idx_type =3D=3D 'BITSET' then + stats.bitset =3D stats.bitset + 1 + end + end + end +end + +local function fill_in_space_stats_impl(features) + local spaces =3D { + memtx =3D 0, + vinyl =3D 0, + temporary =3D 0, + ['local'] =3D 0, + } + + local indices =3D { + hash =3D 0, + tree =3D 0, + rtree =3D 0, + bitset =3D 0, + jsonpath =3D 0, + functional =3D 0, + } + + for name, space in pairs(box.space) do + local is_system =3D is_system_space(space) + if not is_system and type(name) =3D=3D 'number' then + if space.engine =3D=3D 'vinyl' then + spaces.vinyl =3D spaces.vinyl + 1 + elseif space.engine =3D=3D 'memtx' then + if space.temporary then + spaces.temporary =3D spaces.temporary + 1 + end + spaces.memtx =3D spaces.memtx + 1 + end + if space.is_local then + spaces['local'] =3D spaces['local'] + 1 + end + fill_in_indices_stats(space, indices) + end + fiber.yield() + end + + for k, v in pairs(spaces) do + features[k..'_spaces'] =3D v + end + + for k, v in pairs(indices) do + features[k..'_indices'] =3D v + end +end + +local cached_schema_version =3D 0 +local cached_feedback_features =3D {} + +local function fill_in_space_stats(feedback) + local schema_version =3D box.internal.schema_version() + if cached_schema_version < schema_version then + local features =3D {} + fill_in_space_stats_impl(features) + cached_schema_version =3D schema_version + cached_feedback_features =3D features + end + feedback.features =3D 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 =20