feat metrics: add UINVARIANT() on metrics registration & fix bugs

commit_hash:ed536be2effdaabe227756ce4e6fe229b309be6b

Author: segoon

core/src/engine/task/task_processor.cpp

@@ -133,8 +133,8 @@ TaskProcessor::TaskProcessor(TaskProcessorConfig config, std::shared_ptr<impl::T
     : task_queue_(MakeTaskQueue(config)),
       task_counter_(config.worker_threads),
       config_(config),
-      pools_(std::move(pools)),
       plugin_manager_(*this, config.worker_threads),
+      pools_(std::move(pools)),
       trace_plugin_(config.worker_threads)
 {
     utils::impl::FinishStaticRegistration();

core/src/engine/task/task_processor.hpp

@@ -137,6 +137,7 @@ class TaskProcessor final {
     impl::TaskCounter task_counter_;
 
     const TaskProcessorConfig config_;
+    PluginManager plugin_manager_;
     const std::shared_ptr<impl::TaskProcessorPools> pools_;
     std::vector<std::thread> workers_;
     logging::LoggerPtr task_trace_logger_{nullptr};
@@ -154,7 +155,6 @@ class TaskProcessor final {
     std::unique_ptr<utils::statistics::ThreadPoolCpuStatsStorage> cpu_stats_storage_{nullptr};
     TaskProcessor* fs_task_processor_{nullptr};
 
-    PluginManager plugin_manager_;
     // TracePlugin must start before any task is created to account it
     TracePlugin trace_plugin_;
 };

core/src/utils/statistics/storage.cpp

@@ -43,29 +43,48 @@ class FakeFormatBuilder final : public BaseFormatBuilder {
     void HandleMetric(std::string_view, LabelsSpan, const MetricValue&) override {}
 };
 
+void WriteWithFakeFormat(impl::MetricsSource& source)
+{
+    static constexpr std::string_view fake_prefix = "fake_prefix";
+    if (source.writer) {
+        FakeFormatBuilder builder;
+        const Request request;
+        impl::WriterState state{builder, request, {}, {}};
+        auto writer = Writer{&state}[fake_prefix];
+        source.writer(writer);
+    }
+    if (source.extender) {
+        source.extender(StatisticsRequest{});
+    }
+}
+
+[[maybe_unused]] void CheckDataUsedByCallbackIsValidBeforeRegistering(impl::MetricsSource& source) noexcept {
+    try {
+        WriteWithFakeFormat(source);
+    } catch (const std::exception& e) {
+        utils::AbortWithStacktrace(fmt::format(
+            "Unhandled exception while statistics holder {} is registering: {}",
+            source.prefix_path,
+            e.what()
+        ));
+    }
+}
+
 // During the `Entry::Unregister` call or destruction of `Entry`, all variables
 // used by the writer or extender callback must be valid (must not be
 // destroyed). A common cause of crashes in this place: there is no manual call
 // to `Unregister`. In this case, check the lifetime of the data used by the
 // callback.
 [[maybe_unused]] void CheckDataUsedByCallbackHasNotBeenDestroyedBeforeUnregistering(impl::MetricsSource& source
 ) noexcept {
-    static constexpr std::string_view fake_prefix = "fake_prefix";
     try {
-        if (source.writer) {
-            FakeFormatBuilder builder;
-            const Request request;
-            impl::WriterState state{builder, request, {}, {}};
-            auto writer = Writer{&state}[fake_prefix];
-            source.writer(writer);
-        }
-        if (source.extender) {
-            source.extender(StatisticsRequest{});
-        }
+        WriteWithFakeFormat(source);
     } catch (const std::exception& e) {
-        LOG_ERROR()
-            << "Unhandled exception while statistics holder " << source.prefix_path
-            << " is unregistering automatically: " << e.what();
+        utils::AbortWithStacktrace(fmt::format(
+            "Unhandled exception while statistics holder {} is unregistering automatically: {}",
+            source.prefix_path,
+            e.what()
+        ));
     }
 }
 
@@ -185,6 +204,9 @@ Entry Storage::DoRegisterExtender(impl::MetricsSource&& source) {
     );
 
     const std::lock_guard lock(mutex_);
+    if constexpr (impl::kCheckSubscriptionUB) {
+        CheckDataUsedByCallbackIsValidBeforeRegistering(source);
+    }
     const auto res = metrics_sources_.insert(metrics_sources_.end(), std::move(source));
     return Entry(Entry::Impl{this, res});
 }

core/src/utils/statistics/writer_test.cpp

@@ -365,6 +365,11 @@ UTEST(MetricsWriter, LabeledMultiple) {
 }
 
 UTEST(MetricsWriter, CustomTypesOptimization) {
+    if constexpr (utils::statistics::impl::kCheckSubscriptionUB) {
+        return;
+    }
+    // Check for "not called" only if call on registration is not forced
+
     Storage storage;
     auto holder = storage.RegisterWriter("skip", [](Writer& writer) {
         writer = MetricTypeThatMustBeSkipped{"at 'skip' path"};
@@ -432,7 +437,7 @@ UTEST(MetricsWriter, AutomaticUnsubscribingCheckWriterData) {
     }
 
     if constexpr (utils::statistics::impl::kCheckSubscriptionUB) {
-        EXPECT_EQ(counter, 1);
+        EXPECT_EQ(counter, 3);
     } else {
         EXPECT_EQ(counter, 0);
     }
@@ -452,7 +457,7 @@ UTEST(MetricsWriter, AutomaticUnsubscribingCheckExtenderData) {
     }
 
     if constexpr (utils::statistics::impl::kCheckSubscriptionUB) {
-        EXPECT_EQ(counter, 1);
+        EXPECT_EQ(counter, 3);
     } else {
         EXPECT_EQ(counter, 0);
     }