From 79820c2006c22c29be7250e70855b4b264621e8e Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Thu, 13 Oct 2022 11:56:12 +0300 Subject: [PATCH 1/8] tracing: Outline may_create_new_session It's a private method used purely in tracing.cc, no need in compiling it every time the header is met somewhere else. Signed-off-by: Pavel Emelyanov --- tracing/tracing.cc | 20 ++++++++++++++++++++ tracing/tracing.hh | 20 +------------------- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/tracing/tracing.cc b/tracing/tracing.cc index bc556c1d8b..30efba1e5d 100644 --- a/tracing/tracing.cc +++ b/tracing/tracing.cc @@ -84,6 +84,26 @@ future<> tracing::stop_tracing() { }); } +bool tracing::may_create_new_session(const std::optional& session_id) { + // Don't create a session if its records are likely to be dropped + if (!have_records_budget(exp_trace_events_per_session) || _active_sessions >= max_pending_sessions + write_event_sessions_threshold) { + if (session_id) { + tracing_logger.trace("{}: Too many outstanding tracing records or sessions. Dropping a secondary session", *session_id); + } else { + tracing_logger.trace("Too many outstanding tracing records or sessions. Dropping a primary session"); + } + + if (++stats.dropped_sessions % tracing::log_warning_period == 1) { + tracing_logger.warn("Dropped {} sessions: open_sessions {}, cached_records {} pending_for_write_records {}, flushing_records {}", + stats.dropped_sessions, _active_sessions, _cached_records, _pending_for_write_records_count, _flushing_records); + } + + return false; + } + + return true; +} + trace_state_ptr tracing::create_session(trace_type type, trace_state_props_set props) noexcept { if (!started()) { return nullptr; diff --git a/tracing/tracing.hh b/tracing/tracing.hh index 5b87ece604..117df376a4 100644 --- a/tracing/tracing.hh +++ b/tracing/tracing.hh @@ -637,25 +637,7 @@ private: * * @return TRUE if conditions are allowing creating a new tracing session */ - bool may_create_new_session(const std::optional& session_id = std::nullopt) { - // Don't create a session if its records are likely to be dropped - if (!have_records_budget(exp_trace_events_per_session) || _active_sessions >= max_pending_sessions + write_event_sessions_threshold) { - if (session_id) { - tracing_logger.trace("{}: Too many outstanding tracing records or sessions. Dropping a secondary session", *session_id); - } else { - tracing_logger.trace("Too many outstanding tracing records or sessions. Dropping a primary session"); - } - - if (++stats.dropped_sessions % tracing::log_warning_period == 1) { - tracing_logger.warn("Dropped {} sessions: open_sessions {}, cached_records {} pending_for_write_records {}, flushing_records {}", - stats.dropped_sessions, _active_sessions, _cached_records, _pending_for_write_records_count, _flushing_records); - } - - return false; - } - - return true; - } + bool may_create_new_session(const std::optional& session_id = std::nullopt); }; void one_session_records::set_pending_for_write() { From 0a6a5a242e9c7498c5d1f290dcbf6c5ab590d3c9 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Thu, 13 Oct 2022 11:58:37 +0300 Subject: [PATCH 2/8] tracing: Remove copy-n-paste comments from test Tests don't have supervisor, so there's no sense in keeping these bits Signed-off-by: Pavel Emelyanov --- test/boost/tracing.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/boost/tracing.cc b/test/boost/tracing.cc index 3f191910d8..ba7ffab2e9 100644 --- a/test/boost/tracing.cc +++ b/test/boost/tracing.cc @@ -17,12 +17,10 @@ future<> do_with_tracing_env(std::function(cql_test_env&)> func, cql_test_config cfg_in = {}) { return do_with_cql_env([func](auto &env) { - // supervisor::notify("creating tracing"); tracing::backend_registry tracing_backend_registry; tracing::register_tracing_keyspace_backend(tracing_backend_registry); tracing::tracing::create_tracing(tracing_backend_registry, "trace_keyspace_helper").get(); - // supervisor::notify("starting tracing"); tracing::tracing::start_tracing(env.qp()).get(); return do_with(std::move(tracing_backend_registry), [func, &env](auto ®) { From 1adb2c8cc36e04414a60d8b72407ad39173f71e4 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Thu, 13 Oct 2022 12:30:56 +0300 Subject: [PATCH 3/8] tracing: Add constraint to trace_state::begin() It expects that the function is (void) and returns back a string Signed-off-by: Pavel Emelyanov --- tracing/trace_state.hh | 1 + 1 file changed, 1 insertion(+) diff --git a/tracing/trace_state.hh b/tracing/trace_state.hh index 217d8ed6ad..df7cd61f5d 100644 --- a/tracing/trace_state.hh +++ b/tracing/trace_state.hh @@ -276,6 +276,7 @@ private: } template + requires std::is_invocable_r_v void begin(const seastar::lazy_eval& lf, gms::inet_address client) { begin(lf(), client); } From fe7d38661c61f11d662be2446cb5a62e728d9248 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Thu, 13 Oct 2022 17:18:55 +0300 Subject: [PATCH 4/8] tracing: Use class-registrator for backends Currently the code uses its own class registration engine, but there's a generic one in utils/ that applies here too. In fact, the tracing backend registry is just a transparent wrapper over the generic one :\ Signed-off-by: Pavel Emelyanov --- tracing/trace_keyspace_helper.cc | 4 ++++ tracing/tracing.cc | 6 +++--- tracing/tracing.hh | 1 - 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/tracing/trace_keyspace_helper.cc b/tracing/trace_keyspace_helper.cc index aed0b6c688..e2c28405c9 100644 --- a/tracing/trace_keyspace_helper.cc +++ b/tracing/trace_keyspace_helper.cc @@ -19,6 +19,7 @@ #include "types/map.hh" #include "utils/UUID_gen.hh" #include "utils/fb_utilities.hh" +#include "utils/class_registrator.hh" namespace tracing { @@ -470,4 +471,7 @@ void register_tracing_keyspace_backend(backend_registry& tbr) { tbr.register_backend("trace_keyspace_helper"); } +using registry_default = class_registrator; +static registry_default registrator_default("trace_keyspace_helper"); + } diff --git a/tracing/tracing.cc b/tracing/tracing.cc index 30efba1e5d..7a0cfbe875 100644 --- a/tracing/tracing.cc +++ b/tracing/tracing.cc @@ -11,6 +11,7 @@ #include "tracing/tracing.hh" #include "tracing/trace_state.hh" #include "tracing/tracing_backend_registry.hh" +#include "utils/class_registrator.hh" namespace tracing { @@ -28,7 +29,6 @@ std::vector trace_type_names = { tracing::tracing(const backend_registry& br, sstring tracing_backend_helper_class_name) : _write_timer([this] { write_timer_callback(); }) , _thread_name(seastar::format("shard {:d}", this_shard_id())) - , _backend_registry(br) , _tracing_backend_helper_class_name(std::move(tracing_backend_helper_class_name)) , _gen(std::random_device()()) , _slow_query_duration_threshold(default_slow_query_duraion_threshold) @@ -147,8 +147,8 @@ trace_state_ptr tracing::create_session(const trace_info& secondary_session_info future<> tracing::start(cql3::query_processor& qp) { try { - _tracing_backend_helper_ptr = _backend_registry.create_backend(_tracing_backend_helper_class_name, *this); - } catch (no_such_tracing_backend& e) { + _tracing_backend_helper_ptr = create_object(_tracing_backend_helper_class_name, *this); + } catch (no_such_class& e) { tracing_logger.error("Can't create tracing backend helper {}: not supported", _tracing_backend_helper_class_name); throw; } catch (...) { diff --git a/tracing/tracing.hh b/tracing/tracing.hh index 117df376a4..00bc849e0c 100644 --- a/tracing/tracing.hh +++ b/tracing/tracing.hh @@ -377,7 +377,6 @@ private: bool _ignore_trace_events = false; std::unique_ptr _tracing_backend_helper_ptr; sstring _thread_name; - const backend_registry& _backend_registry; sstring _tracing_backend_helper_class_name; seastar::metrics::metric_groups _metrics; double _trace_probability = 0.0; // keep this one for querying purposes From 5c8a61ace280a7fea8642485d4dad62cb6c5f040 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Thu, 13 Oct 2022 17:30:01 +0300 Subject: [PATCH 5/8] tracing: Dismantle trace-backend registry It's not used any longer Signed-off-by: Pavel Emelyanov --- configure.py | 1 - main.cc | 5 +-- test/boost/tracing.cc | 7 +--- tracing/trace_keyspace_helper.cc | 5 --- tracing/tracing.cc | 7 ++-- tracing/tracing.hh | 5 ++- tracing/tracing_backend_registry.cc | 36 -------------------- tracing/tracing_backend_registry.hh | 51 ----------------------------- 8 files changed, 7 insertions(+), 110 deletions(-) delete mode 100644 tracing/tracing_backend_registry.cc delete mode 100644 tracing/tracing_backend_registry.hh diff --git a/configure.py b/configure.py index 4590d97992..5ee021d27b 100755 --- a/configure.py +++ b/configure.py @@ -994,7 +994,6 @@ scylla_core = (['message/messaging_service.cc', 'tracing/tracing.cc', 'tracing/trace_keyspace_helper.cc', 'tracing/trace_state.cc', - 'tracing/tracing_backend_registry.cc', 'tracing/traced_file.cc', 'table_helper.cc', 'range_tombstone.cc', diff --git a/main.cc b/main.cc index 9bc75913a8..9a28d6498e 100644 --- a/main.cc +++ b/main.cc @@ -50,7 +50,6 @@ #include #include #include "tracing/tracing.hh" -#include "tracing/tracing_backend_registry.hh" #include #include "message/messaging_service.hh" #include "db/sstables-format-selector.hh" @@ -744,9 +743,7 @@ To start the scylla server proper, simply invoke as: scylla server (or just scyl // }); supervisor::notify("creating tracing"); - tracing::backend_registry tracing_backend_registry; - tracing::register_tracing_keyspace_backend(tracing_backend_registry); - tracing::tracing::create_tracing(tracing_backend_registry, "trace_keyspace_helper").get(); + tracing::tracing::create_tracing("trace_keyspace_helper").get(); auto destroy_tracing = defer_verbose_shutdown("tracing instance", [] { tracing::tracing::tracing_instance().stop().get(); }); diff --git a/test/boost/tracing.cc b/test/boost/tracing.cc index ba7ffab2e9..e0fd043fd9 100644 --- a/test/boost/tracing.cc +++ b/test/boost/tracing.cc @@ -10,20 +10,16 @@ #include "tracing/tracing.hh" #include "tracing/trace_state.hh" -#include "tracing/tracing_backend_registry.hh" #include "utils/class_registrator.hh" #include "test/lib/cql_test_env.hh" future<> do_with_tracing_env(std::function(cql_test_env&)> func, cql_test_config cfg_in = {}) { return do_with_cql_env([func](auto &env) { - tracing::backend_registry tracing_backend_registry; - tracing::register_tracing_keyspace_backend(tracing_backend_registry); - tracing::tracing::create_tracing(tracing_backend_registry, "trace_keyspace_helper").get(); + tracing::tracing::create_tracing("trace_keyspace_helper").get(); tracing::tracing::start_tracing(env.qp()).get(); - return do_with(std::move(tracing_backend_registry), [func, &env](auto ®) { return func(env).finally([]() { return tracing::tracing::tracing_instance().invoke_on_all([](tracing::tracing &local_tracing) { return local_tracing.shutdown(); @@ -31,7 +27,6 @@ future<> do_with_tracing_env(std::function(cql_test_env&)> func, cql_te return tracing::tracing::tracing_instance().stop(); }); }); - }); }, std::move(cfg_in)); } diff --git a/tracing/trace_keyspace_helper.cc b/tracing/trace_keyspace_helper.cc index e2c28405c9..4ae0762ec3 100644 --- a/tracing/trace_keyspace_helper.cc +++ b/tracing/trace_keyspace_helper.cc @@ -10,7 +10,6 @@ #include #include "types.hh" #include "tracing/trace_keyspace_helper.hh" -#include "tracing/tracing_backend_registry.hh" #include "cql3/statements/batch_statement.hh" #include "cql3/statements/modification_statement.hh" #include "cql3/query_processor.hh" @@ -467,10 +466,6 @@ std::unique_ptr trace_keyspace_helper::allocate_sess return std::make_unique(); } -void register_tracing_keyspace_backend(backend_registry& tbr) { - tbr.register_backend("trace_keyspace_helper"); -} - using registry_default = class_registrator; static registry_default registrator_default("trace_keyspace_helper"); diff --git a/tracing/tracing.cc b/tracing/tracing.cc index 7a0cfbe875..ae552e1517 100644 --- a/tracing/tracing.cc +++ b/tracing/tracing.cc @@ -10,7 +10,6 @@ #include #include "tracing/tracing.hh" #include "tracing/trace_state.hh" -#include "tracing/tracing_backend_registry.hh" #include "utils/class_registrator.hh" namespace tracing { @@ -26,7 +25,7 @@ std::vector trace_type_names = { "REPAIR" }; -tracing::tracing(const backend_registry& br, sstring tracing_backend_helper_class_name) +tracing::tracing(sstring tracing_backend_helper_class_name) : _write_timer([this] { write_timer_callback(); }) , _thread_name(seastar::format("shard {:d}", this_shard_id())) , _tracing_backend_helper_class_name(std::move(tracing_backend_helper_class_name)) @@ -67,8 +66,8 @@ tracing::tracing(const backend_registry& br, sstring tracing_backend_helper_clas }); } -future<> tracing::create_tracing(const backend_registry& br, sstring tracing_backend_class_name) { - return tracing_instance().start(std::ref(br), std::move(tracing_backend_class_name)); +future<> tracing::create_tracing(sstring tracing_backend_class_name) { + return tracing_instance().start(std::move(tracing_backend_class_name)); } future<> tracing::start_tracing(sharded& qp) { diff --git a/tracing/tracing.hh b/tracing/tracing.hh index 00bc849e0c..e5ca3e7546 100644 --- a/tracing/tracing.hh +++ b/tracing/tracing.hh @@ -32,7 +32,6 @@ extern logging::logger tracing_logger; class trace_state_ptr; class tracing; -class backend_registry; enum class trace_type : uint8_t { NONE, @@ -413,10 +412,10 @@ public: return !_down; } - static future<> create_tracing(const backend_registry& br, sstring tracing_backend_helper_class_name); + static future<> create_tracing(sstring tracing_backend_helper_class_name); static future<> start_tracing(sharded& qp); static future<> stop_tracing(); - tracing(const backend_registry& br, sstring tracing_backend_helper_class_name); + tracing(sstring tracing_backend_helper_class_name); // Initialize a tracing backend (e.g. tracing_keyspace or logstash) future<> start(cql3::query_processor& qp); diff --git a/tracing/tracing_backend_registry.cc b/tracing/tracing_backend_registry.cc deleted file mode 100644 index dc913529ba..0000000000 --- a/tracing/tracing_backend_registry.cc +++ /dev/null @@ -1,36 +0,0 @@ -/* - * Copyright 2018-present ScyllaDB - */ - -/* - * SPDX-License-Identifier: AGPL-3.0-or-later - */ - -#include "tracing_backend_registry.hh" -#include "tracing/tracing.hh" -#include "utils/class_registrator.hh" - -namespace tracing { - -no_such_tracing_backend::no_such_tracing_backend() : std::runtime_error("no such tracing backend") { -} - -backend_registry::backend_registry() - : _impl(std::make_unique>()) { -} - -void -backend_registry::register_backend_creator(sstring name, std::function (tracing&)> creator) { - _impl->register_class(std::move(name), std::move(creator)); -} - -std::unique_ptr -backend_registry::create_backend(const sstring& name, tracing& t) const { - try { - return _impl->create(name, t); - } catch (no_such_class&) { - throw no_such_tracing_backend(); - } -} - -} diff --git a/tracing/tracing_backend_registry.hh b/tracing/tracing_backend_registry.hh deleted file mode 100644 index e22a4f38a4..0000000000 --- a/tracing/tracing_backend_registry.hh +++ /dev/null @@ -1,51 +0,0 @@ -/* - * Copyright 2018-present ScyllaDB - */ - -/* - * SPDX-License-Identifier: AGPL-3.0-or-later - */ - -#pragma once - -#include -#include -#include -#include -#include "seastarx.hh" - -template -class nonstatic_class_registry; - -namespace tracing { - -class i_tracing_backend_helper; -class trace_keyspace_helper; -class tracing; - -class no_such_tracing_backend : public std::runtime_error { -public: - no_such_tracing_backend(); -}; - -class backend_registry { - std::unique_ptr> _impl; -private: - void register_backend_creator(sstring name, std::function (tracing&)> creator); -public: - backend_registry(); - std::unique_ptr create_backend(const sstring& name, tracing& t) const; // may throw no_such_tracing_backend - template - void register_backend(sstring name); -}; - -template -void backend_registry::register_backend(sstring name) { - return register_backend_creator(name, [] (tracing& t) { - return std::make_unique(t); - }); -} - -void register_tracing_keyspace_backend(backend_registry&); - -} From 53ac8536f1e41541cb547e8c40a330c97559c512 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Thu, 13 Oct 2022 17:50:32 +0300 Subject: [PATCH 6/8] tracing: Move test into thread The test calls future<>.get()'s in its lambda which is only allowed in seastar threads. It's not stepped upon because (surprise, surprise) this test is not run at all. Next patch fixes it. Meanwhile, the fix is in using cql_env_thread thing for the whole lambda which runs in it seastar::async() context Signed-off-by: Pavel Emelyanov --- test/boost/tracing.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/boost/tracing.cc b/test/boost/tracing.cc index e0fd043fd9..60d9324693 100644 --- a/test/boost/tracing.cc +++ b/test/boost/tracing.cc @@ -15,18 +15,18 @@ #include "test/lib/cql_test_env.hh" future<> do_with_tracing_env(std::function(cql_test_env&)> func, cql_test_config cfg_in = {}) { - return do_with_cql_env([func](auto &env) { + return do_with_cql_env_thread([func](auto &env) { tracing::tracing::create_tracing("trace_keyspace_helper").get(); tracing::tracing::start_tracing(env.qp()).get(); - return func(env).finally([]() { + func(env).finally([]() { return tracing::tracing::tracing_instance().invoke_on_all([](tracing::tracing &local_tracing) { return local_tracing.shutdown(); }).finally([]() { return tracing::tracing::tracing_instance().stop(); }); - }); + }).get(); }, std::move(cfg_in)); } From 5b67a2a87634d4eb262df8b1ec3990e03e1867da Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Thu, 13 Oct 2022 17:50:43 +0300 Subject: [PATCH 7/8] tracing: Indentation fix after previous patch Signed-off-by: Pavel Emelyanov --- test/boost/tracing.cc | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/boost/tracing.cc b/test/boost/tracing.cc index 60d9324693..72105ab91c 100644 --- a/test/boost/tracing.cc +++ b/test/boost/tracing.cc @@ -20,13 +20,13 @@ future<> do_with_tracing_env(std::function(cql_test_env&)> func, cql_te tracing::tracing::start_tracing(env.qp()).get(); - func(env).finally([]() { - return tracing::tracing::tracing_instance().invoke_on_all([](tracing::tracing &local_tracing) { - return local_tracing.shutdown(); - }).finally([]() { - return tracing::tracing::tracing_instance().stop(); - }); - }).get(); + func(env).finally([]() { + return tracing::tracing::tracing_instance().invoke_on_all([](tracing::tracing &local_tracing) { + return local_tracing.shutdown(); + }).finally([]() { + return tracing::tracing::tracing_instance().stop(); + }); + }).get(); }, std::move(cfg_in)); } From 707efb6dfb4127465ab347b71bb1734b324f27e6 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Thu, 13 Oct 2022 17:54:43 +0300 Subject: [PATCH 8/8] tracing: Wire tracing test back The boost/tracing test is not run, because test.py boost suite collects tests that match *_test.cc pattern. The tracing one apparently doesn't Signed-off-by: Pavel Emelyanov --- configure.py | 2 +- test/boost/{tracing.cc => tracing_test.cc} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename test/boost/{tracing.cc => tracing_test.cc} (100%) diff --git a/configure.py b/configure.py index 5ee021d27b..437b4fdf1b 100755 --- a/configure.py +++ b/configure.py @@ -448,7 +448,7 @@ scylla_tests = set([ 'test/boost/schema_change_test', 'test/boost/schema_registry_test', 'test/boost/secondary_index_test', - 'test/boost/tracing', + 'test/boost/tracing_test', 'test/boost/index_with_paging_test', 'test/boost/serialization_test', 'test/boost/serialized_action_test', diff --git a/test/boost/tracing.cc b/test/boost/tracing_test.cc similarity index 100% rename from test/boost/tracing.cc rename to test/boost/tracing_test.cc