Merge 'Make tracing test run again, simplify backend registry and few related cleanups' from Pavel Emelyanov

It turned out that boost/tracing test is not run because its name doesn't match the *_test.cc pattern. While fixing it it turned out that the test cannot even start, because it uses future<>.get() calls outside of seastar::thread context. While patching this place the trace-backend registry was removed for simplicity. And, while at it, few more cleanups "while at it"

Closes #11779

* github.com:scylladb/scylladb:
  tracing: Wire tracing test back
  tracing: Indentation fix after previous patch
  tracing: Move test into thread
  tracing: Dismantle trace-backend registry
  tracing: Use class-registrator for backends
  tracing: Add constraint to trace_state::begin()
  tracing: Remove copy-n-paste comments from test
  tracing: Outline may_create_new_session

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',
@@ -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',

main.cc

@@ -50,7 +50,6 @@
 #include <sys/resource.h>
 #include <sys/prctl.h>
 #include "tracing/tracing.hh"
-#include "tracing/tracing_backend_registry.hh"
 #include <seastar/core/prometheus.hh>
 #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();
             });

test/boost/tracing_test.cc

@@ -10,30 +10,23 @@
 
 #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<future<>(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();
+    return do_with_cql_env_thread([func](auto &env) {
+        tracing::tracing::create_tracing("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 &reg) {
-            return 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();
-                });
+        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));
 }
 

tracing/trace_keyspace_helper.cc

@@ -10,7 +10,6 @@
 #include <seastar/core/metrics.hh>
 #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"
@@ -19,6 +18,7 @@
 #include "types/map.hh"
 #include "utils/UUID_gen.hh"
 #include "utils/fb_utilities.hh"
+#include "utils/class_registrator.hh"
 
 namespace tracing {
 
@@ -466,8 +466,7 @@ std::unique_ptr<backend_session_state_base> trace_keyspace_helper::allocate_sess
     return std::make_unique<trace_keyspace_backend_sesssion_state>();
 }
 
-void register_tracing_keyspace_backend(backend_registry& tbr) {
-    tbr.register_backend<trace_keyspace_helper>("trace_keyspace_helper");
-}
+using registry_default = class_registrator<i_tracing_backend_helper, trace_keyspace_helper, tracing&>;
+static registry_default registrator_default("trace_keyspace_helper");
 
 }

tracing/trace_state.hh

@@ -276,6 +276,7 @@ private:
     }
 
     template <typename Func>
+    requires std::is_invocable_r_v<sstring, Func>
     void begin(const seastar::lazy_eval<Func>& lf, gms::inet_address client) {
         begin(lf(), client);
     }

tracing/tracing.cc

@@ -10,7 +10,7 @@
 #include <seastar/core/metrics.hh>
 #include "tracing/tracing.hh"
 #include "tracing/trace_state.hh"
-#include "tracing/tracing_backend_registry.hh"
+#include "utils/class_registrator.hh"
 
 namespace tracing {
 
@@ -25,10 +25,9 @@ std::vector<sstring> 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()))
-        , _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)
@@ -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<cql3::query_processor>& qp) {
@@ -84,6 +83,26 @@ future<> tracing::stop_tracing() {
     });
 }
 
+bool tracing::may_create_new_session(const std::optional<utils::UUID>& 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;
@@ -127,8 +146,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<i_tracing_backend_helper>(_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 (...) {

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,
@@ -377,7 +376,6 @@ private:
     bool _ignore_trace_events = false;
     std::unique_ptr<i_tracing_backend_helper> _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
@@ -414,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<cql3::query_processor>& 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);
@@ -637,25 +635,7 @@ private:
      *
      * @return TRUE if conditions are allowing creating a new tracing session
      */
-    bool may_create_new_session(const std::optional<utils::UUID>& 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<utils::UUID>& session_id = std::nullopt);
 };
 
 void one_session_records::set_pending_for_write() {

tracing/tracing_backend_registry.cc

@@ -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<nonstatic_class_registry<i_tracing_backend_helper, tracing&>>()) {
-}
-
-void
-backend_registry::register_backend_creator(sstring name, std::function<std::unique_ptr<i_tracing_backend_helper> (tracing&)> creator) {
-    _impl->register_class(std::move(name), std::move(creator));
-}
-
-std::unique_ptr<i_tracing_backend_helper>
-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();
-    }
-}
-
-}

tracing/tracing_backend_registry.hh

@@ -1,51 +0,0 @@
-/*
- * Copyright 2018-present ScyllaDB
- */
-
-/*
- * SPDX-License-Identifier: AGPL-3.0-or-later
- */
-
-#pragma once
-
-#include <seastar/core/sstring.hh>
-#include <functional>
-#include <memory>
-#include <exception>
-#include "seastarx.hh"
-
-template <typename BaseType, typename... Args>
-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<nonstatic_class_registry<i_tracing_backend_helper, tracing&>> _impl;
-private:
-    void register_backend_creator(sstring name, std::function<std::unique_ptr<i_tracing_backend_helper> (tracing&)> creator);
-public:
-    backend_registry();
-    std::unique_ptr<i_tracing_backend_helper> create_backend(const sstring& name, tracing& t) const; // may throw no_such_tracing_backend
-    template <typename Backend>
-    void register_backend(sstring name);
-};
-
-template <typename Backend>
-void backend_registry::register_backend(sstring name) {
-    return register_backend_creator(name, [] (tracing& t) {
-        return std::make_unique<Backend>(t);
-    });
-}
-
-void register_tracing_keyspace_backend(backend_registry&);
-
-}