refactor clients/http: use MonotonicByLabelStorage for metrics

commit_hash:96a83e13d97eb6419431cf2b01f819a9d4a2c736

Author: Anton3

core/include/userver/clients/http/request.hpp

@@ -96,7 +96,7 @@ class Request final {
     explicit Request(
         impl::EasyWrapper&&,
         RequestStats&& req_stats,
-        const std::shared_ptr<DestinationStatistics>& dest_stats,
+        DestinationStatistics& dest_stats,
         clients::dns::Resolver* resolver,
         const tracing::TracingManagerBase& tracing_manager
     );

core/include/userver/utils/statistics/by_label_storage.hpp

@@ -243,7 +243,7 @@ struct ByLabelEntryEqual {
 /// @tparam Labels An aggregate type whose fields are convertible to `std::string_view`.
 /// @tparam Metric The metric type. Must support `DumpMetric(Writer&, const Metric&)`.
 template <typename Labels, typename Metric>
-requires impl::LabelsAggregate<Labels> && impl::DumpableMetric<Metric>
+requires impl::LabelsAggregate<Labels>
 class MonotonicByLabelStorage final {
 public:
     /// @brief Create an empty storage.
@@ -298,7 +298,9 @@ class MonotonicByLabelStorage final {
     }
 
     /// @brief Dump all metrics to a Writer, using label names from `Labels` fields.
-    friend void DumpMetric(Writer& writer, const MonotonicByLabelStorage& storage) {
+    friend void DumpMetric(Writer& writer, const MonotonicByLabelStorage& storage)
+    requires impl::DumpableMetric<Metric>
+    {
         static constexpr auto kLabelNames = impl::GetLabelNames<Labels>();
         static constexpr std::size_t kFieldCount = boost::pfr::tuple_size_v<Labels>;
 

core/src/clients/http/client_core.cpp

@@ -117,8 +117,8 @@ Request ClientCore::CreateRequest() {
             auto wrapper = impl::EasyWrapper{std::move(easy), *this};
             return Request{
                 std::move(wrapper),
-                statistics_[idx].CreateRequestStats(),
-                destination_statistics_,
+                RequestStats{statistics_[idx]},
+                *destination_statistics_,
                 resolver_,
                 *tracing_manager_.GetBase()
             };
@@ -133,8 +133,8 @@ Request ClientCore::CreateRequest() {
                     }).Get();
                 return Request{
                     std::move(wrapper),
-                    statistics_[i].CreateRequestStats(),
-                    destination_statistics_,
+                    RequestStats{statistics_[i]},
+                    *destination_statistics_,
                     resolver_,
                     *tracing_manager_.GetBase()
                 };

core/src/clients/http/destination_statistics.cpp

@@ -7,37 +7,14 @@ USERVER_NAMESPACE_BEGIN
 
 namespace clients::http {
 
-std::shared_ptr<RequestStats> DestinationStatistics::GetStatisticsForDestination(const std::string& destination) {
-    auto ptr = GetExistingStatisticsForDestination(destination);
-    if (ptr) {
-        return ptr;
-    }
-
-    return CreateStatisticsForDestination(destination);
+RequestStats DestinationStatistics::GetStatisticsForDestination(std::string_view destination) {
+    return RequestStats{metrics_[{.http_destination = destination}]};
 }
 
-std::shared_ptr<RequestStats> DestinationStatistics::CreateStatisticsForDestination(const std::string& destination) {
-    return std::make_shared<RequestStats>(*rcu_map_[destination]);
-}
-
-std::shared_ptr<RequestStats> DestinationStatistics::GetExistingStatisticsForDestination(const std::string& destination
-) {
-    /*
-     * It's safe to return RequestStats holding a reference to Statistics as
-     * RequestStats lifetime is less than clients::http::ClientCore's one.
-     */
-    auto stats = rcu_map_.Get(destination);
+std::optional<RequestStats> DestinationStatistics::GetStatisticsForDestinationAuto(std::string_view destination) {
+    auto* const stats = metrics_.GetIfExists({.http_destination = destination});
     if (stats) {
-        return std::make_shared<RequestStats>(*stats);
-    } else {
-        return {};
-    }
-}
-
-std::shared_ptr<RequestStats> DestinationStatistics::GetStatisticsForDestinationAuto(const std::string& destination) {
-    auto ptr = GetExistingStatisticsForDestination(destination);
-    if (ptr) {
-        return ptr;
+        return std::optional<RequestStats>{std::in_place, *stats};
     }
 
     // atomic [current++ iff current < max]
@@ -57,22 +34,26 @@ std::shared_ptr<RequestStats> DestinationStatistics::GetStatisticsForDestination
     } while (!current_auto_destinations_
                   .compare_exchange_strong(current_auto_destinations, current_auto_destinations + 1));
 
-    return CreateStatisticsForDestination(destination);
+    return GetStatisticsForDestination(destination);
 }
 
 void DestinationStatistics::SetAutoMaxSize(size_t max_auto_destinations) {
     max_auto_destinations_ = max_auto_destinations;
 }
 
-DestinationStatistics::DestinationsMap::ConstIterator DestinationStatistics::begin() const { return rcu_map_.begin(); }
-
-DestinationStatistics::DestinationsMap::ConstIterator DestinationStatistics::end() const { return rcu_map_.end(); }
+void DestinationStatistics::VisitAllDebug(utils::function_ref<void(const DestinationLabels&, const Statistics&)> func
+) const {
+    metrics_.VisitAll(func);
+}
 
 void DumpMetric(utils::statistics::Writer& writer, const DestinationStatistics& stats) {
-    for (const auto& [url, stat_ptr] : stats) {
-        const InstanceStatistics instance_stat{*stat_ptr};
-        writer.ValueWithLabels(DestinationStatisticsView{instance_stat}, {{"http_destination", url}, {"version", "2"}});
-    }
+    stats.metrics_.VisitAll([&writer](const DestinationLabels& labels, const Statistics& destination_stats) {
+        const InstanceStatistics instance_stat{destination_stats};
+        writer.ValueWithLabels(
+            DestinationStatisticsView{instance_stat},
+            {{"http_destination", labels.http_destination}, {"version", "2"}}
+        );
+    });
 }
 
 }  // namespace clients::http

core/src/clients/http/destination_statistics.hpp

@@ -1,9 +1,13 @@
 #pragma once
 
-#include <memory>
-#include <unordered_map>
-
-#include <userver/rcu/rcu_map.hpp>
+#include <atomic>
+#include <cstddef>
+#include <optional>
+#include <string_view>
+
+#include <userver/utils/function_ref.hpp>
+#include <userver/utils/required.hpp>
+#include <userver/utils/statistics/by_label_storage.hpp>
 #include <userver/utils/statistics/fwd.hpp>
 
 #include <clients/http/statistics.hpp>
@@ -12,30 +16,29 @@ USERVER_NAMESPACE_BEGIN
 
 namespace clients::http {
 
+struct DestinationLabels final {
+    utils::Required<std::string_view> http_destination;
+};
+
 class DestinationStatistics final {
 public:
-    // Return pointer to related RequestStats
-    std::shared_ptr<RequestStats> GetStatisticsForDestination(const std::string& destination);
+    // It's safe to return RequestStats holding a reference to Statistics as
+    // RequestStats lifetime is less than clients::http::Client's one.
+    RequestStats GetStatisticsForDestination(std::string_view destination);
 
-    // If max_auto_destinations reached, return nullptr, pointer to related
-    // RequestStats otherwise
-    std::shared_ptr<RequestStats> GetStatisticsForDestinationAuto(const std::string& destination);
+    // If max_auto_destinations is reached, returns nullopt.
+    std::optional<RequestStats> GetStatisticsForDestinationAuto(std::string_view destination);
 
     void SetAutoMaxSize(size_t max_auto_destinations);
 
-    using DestinationsMap = rcu::RcuMap<std::string, Statistics>;
+    void VisitAllDebug(utils::function_ref<void(const DestinationLabels&, const Statistics&)> func) const;
 
-    DestinationsMap::ConstIterator begin() const;
-    DestinationsMap::ConstIterator end() const;
+    friend void DumpMetric(utils::statistics::Writer& writer, const DestinationStatistics& stats);
 
 private:
-    std::shared_ptr<RequestStats> GetExistingStatisticsForDestination(const std::string& destination);
-
-    std::shared_ptr<RequestStats> CreateStatisticsForDestination(const std::string& destination);
-
-    rcu::RcuMap<std::string, Statistics> rcu_map_;
-    size_t max_auto_destinations_{0};
-    std::atomic<size_t> current_auto_destinations_{0};
+    utils::statistics::MonotonicByLabelStorage<DestinationLabels, Statistics> metrics_;
+    std::size_t max_auto_destinations_{0};
+    std::atomic<std::size_t> current_auto_destinations_{0};
 };
 
 void DumpMetric(utils::statistics::Writer& writer, const DestinationStatistics& stats);

core/src/clients/http/destination_statistics_test.cpp

@@ -14,7 +14,6 @@ USERVER_NAMESPACE_BEGIN
 
 using HttpResponse = utest::SimpleServer::Response;
 using HttpRequest = utest::SimpleServer::Request;
-using HttpCallback = utest::SimpleServer::OnRequest;
 
 static HttpResponse Callback(int code, const HttpRequest& request) {
     LOG_INFO() << "HTTP Server receive: " << request;
@@ -30,7 +29,10 @@ static HttpResponse Callback(int code, const HttpRequest& request) {
 UTEST(DestinationStatistics, Empty) {
     auto client = utest::impl::CreateHttpClientCore();
 
-    EXPECT_EQ(client->GetDestinationStatistics().begin(), client->GetDestinationStatistics().end());
+    client->GetDestinationStatistics()
+        .VisitAllDebug([](const clients::http::DestinationLabels&, const clients::http::Statistics&) {
+            ADD_FAILURE() << "Should be empty";
+        });
 }
 
 UTEST(DestinationStatistics, Ok) {
@@ -41,23 +43,23 @@ UTEST(DestinationStatistics, Ok) {
 
     auto response = client->CreateRequest().post(url).retry(1).timeout(std::chrono::milliseconds(100)).perform();
 
-    const auto& dest_stats = client->GetDestinationStatistics();
     size_t size = 0;
-    for (const auto& [stat_url, stat_ptr] : dest_stats) {
+
+    const auto visitor = [&](const clients::http::DestinationLabels& labels, const clients::http::Statistics& stat) {
         ASSERT_EQ(1, ++size);
 
-        EXPECT_EQ(url, stat_url);
-        ASSERT_NE(nullptr, stat_ptr);
+        EXPECT_EQ(url, *labels.http_destination);
 
-        auto stats = clients::http::InstanceStatistics(*stat_ptr);
+        auto stats = clients::http::InstanceStatistics(stat);
         auto ok = static_cast<size_t>(clients::http::Statistics::ErrorGroup::kOk);
         EXPECT_EQ(utils::statistics::Rate{1}, stats.error_count[ok]);
         for (size_t i = 0; i < clients::http::Statistics::kErrorGroupCount; i++) {
             if (i != ok) {
                 EXPECT_EQ(utils::statistics::Rate{0}, stats.error_count[i]);
             }
         }
-    }
+    };
+    client->GetDestinationStatistics().VisitAllDebug(visitor);
 }
 
 UTEST(DestinationStatistics, CancelledFuture) {
@@ -99,25 +101,23 @@ UTEST(DestinationStatistics, Multiple) {
     auto response = client->CreateRequest().post(url).retry(1).timeout(std::chrono::milliseconds(100)).perform();
     response = client->CreateRequest().post(url2).retry(1).timeout(std::chrono::milliseconds(100)).perform();
 
-    const auto& dest_stats = client->GetDestinationStatistics();
     size_t size = 0;
     std::unordered_set<std::string> expected_urls{url, url2};
-    for (const auto& [stat_url, stat_ptr] : dest_stats) {
+    const auto visitor = [&](const clients::http::DestinationLabels& labels, const clients::http::Statistics& stat) {
         ASSERT_LE(++size, 2);
 
-        EXPECT_EQ(1, expected_urls.erase(stat_url));
+        EXPECT_EQ(1, expected_urls.erase(std::string{labels.http_destination}));
 
-        ASSERT_NE(nullptr, stat_ptr);
-
-        auto stats = clients::http::InstanceStatistics(*stat_ptr);
+        auto stats = clients::http::InstanceStatistics(stat);
         auto ok = static_cast<size_t>(clients::http::Statistics::ErrorGroup::kOk);
         EXPECT_EQ(utils::statistics::Rate{1}, stats.error_count[ok]);
         for (size_t i = 0; i < clients::http::Statistics::kErrorGroupCount; i++) {
             if (i != ok) {
                 EXPECT_EQ(utils::statistics::Rate{0}, stats.error_count[i]) << i << " errors must be zero";
             }
         }
-    }
+    };
+    client->GetDestinationStatistics().VisitAllDebug(visitor);
 }
 
 USERVER_NAMESPACE_END

core/src/clients/http/request.cpp

@@ -204,7 +204,7 @@ ProxyAuthType ProxyAuthTypeFromString(std::string_view auth_name) {
 Request::Request(
     impl::EasyWrapper&& wrapper,
     RequestStats&& req_stats,
-    const std::shared_ptr<DestinationStatistics>& dest_stats,
+    DestinationStatistics& dest_stats,
     clients::dns::Resolver* resolver,
     const tracing::TracingManagerBase& tracing_manager
 )

core/src/clients/http/request_state.cpp

@@ -240,7 +240,7 @@ bool IsHttp11WithCompleteBody(const std::shared_ptr<Response> response) {
 RequestState::RequestState(
     impl::EasyWrapper&& wrapper,
     RequestStats&& req_stats,
-    const std::shared_ptr<DestinationStatistics>& dest_stats,
+    DestinationStatistics& dest_stats,
     clients::dns::Resolver* resolver,
     const tracing::TracingManagerBase& tracing_manager
 )
@@ -420,7 +420,7 @@ void RequestState::SetDestinationMetricNameAuto(std::string destination) {
 }
 
 void RequestState::SetDestinationMetricName(const std::string& destination) {
-    dest_req_stats_ = dest_stats_->GetStatisticsForDestination(destination);
+    dest_req_stats_ = dest_stats_.GetStatisticsForDestination(destination);
 }
 
 void RequestState::SetTestsuiteConfig(const std::shared_ptr<const TestsuiteConfig>& config) {
@@ -1215,7 +1215,7 @@ void RequestState::StartNewSpan(utils::impl::SourceLocation location) {
 
 void RequestState::StartStats() {
     if (!dest_req_stats_) {
-        dest_req_stats_ = dest_stats_->GetStatisticsForDestinationAuto(destination_metric_name_);
+        dest_req_stats_ = dest_stats_.GetStatisticsForDestinationAuto(destination_metric_name_);
     }
 
     WithRequestStats([](RequestStats& stats) { stats.Start(); });

core/src/clients/http/request_state.hpp

@@ -51,7 +51,7 @@ class RequestState : public std::enable_shared_from_this<RequestState> {
     RequestState(
         impl::EasyWrapper&&,
         RequestStats&& req_stats,
-        const std::shared_ptr<DestinationStatistics>& dest_stats,
+        DestinationStatistics& dest_stats,
         clients::dns::Resolver* resolver,
         const tracing::TracingManagerBase& tracing_manager
     );
@@ -214,10 +214,10 @@ class RequestState : public std::enable_shared_from_this<RequestState> {
     /// curl handler wrapper
     impl::EasyWrapper easy_;
     RequestStats stats_;
-    std::shared_ptr<RequestStats> dest_req_stats_;
+    std::optional<RequestStats> dest_req_stats_;
     CancellationPolicy cancellation_policy_{CancellationPolicy::kCancel};
 
-    std::shared_ptr<DestinationStatistics> dest_stats_;
+    DestinationStatistics& dest_stats_;
     std::string destination_metric_name_;
 
     std::shared_ptr<const TestsuiteConfig> testsuite_config_;

core/src/clients/http/statistics.cpp

@@ -40,6 +40,14 @@ RequestStats::~RequestStats() {
 
 RequestStats::RequestStats(RequestStats&& other) noexcept : stats_{std::exchange(other.stats_, nullptr)} {}
 
+RequestStats& RequestStats::operator=(RequestStats&& other) noexcept {
+    if (this != &other) {
+        stats_ = std::exchange(other.stats_, nullptr);
+        start_time_ = other.start_time_;
+    }
+    return *this;
+}
+
 void RequestStats::Start() { start_time_ = std::chrono::steady_clock::now(); }
 
 void RequestStats::FinishOk(int code, unsigned int attempts) noexcept {