admin: improve test coverage and increase the coverage-percent threshold

source/server/admin/admin.h

@@ -200,19 +200,12 @@ class AdminImpl : public Admin,
                      Http::ResponseHeaderMap& response_headers, std::string& body) override;
   void closeSocket();
   void addListenerToHandler(Network::ConnectionHandler* handler) override;
-  Server::Instance& server() { return server_; }
 
   GenHandlerCb createHandlerFunction() {
     return [this](absl::string_view path_and_query, AdminStream& admin_stream) -> HandlerPtr {
       return findHandler(path_and_query, admin_stream);
     };
   }
-  AdminFilter::AdminServerCallbackFunction createCallbackFunction() {
-    return [this](absl::string_view path_and_query, Http::ResponseHeaderMap& response_headers,
-                  Buffer::OwnedImpl& response, AdminFilter& filter) -> Http::Code {
-      return runCallback(path_and_query, response_headers, response, filter);
-    };
-  }
   uint64_t maxRequestsPerConnection() const override { return 0; }
   const HttpConnectionManagerProto::ProxyStatusConfig* proxyStatusConfig() const override {
     return proxy_status_config_.get();
@@ -227,6 +220,8 @@ class AdminImpl : public Admin,
   static HandlerPtr makeStaticTextHandler(absl::string_view response_text, Http::Code code);
 
 private:
+  friend class AdminTestingPeer;
+
   /**
    * Individual admin handler including prefix, help text, and callback.
    */
@@ -295,8 +290,8 @@ class AdminImpl : public Admin,
    * OverloadManager keeps the admin interface accessible even when the proxy is overloaded.
    */
   struct NullOverloadManager : public OverloadManager {
-    struct NullThreadLocalOverloadState : public ThreadLocalOverloadState {
-      NullThreadLocalOverloadState(Event::Dispatcher& dispatcher) : dispatcher_(dispatcher) {}
+    struct OverloadState : public ThreadLocalOverloadState {
+      OverloadState(Event::Dispatcher& dispatcher) : dispatcher_(dispatcher) {}
       const OverloadActionState& getState(const std::string&) override { return inactive_; }
       bool tryAllocateResource(OverloadProactiveResourceName, int64_t) override { return false; }
       bool tryDeallocateResource(OverloadProactiveResourceName, int64_t) override { return false; }
@@ -310,12 +305,12 @@ class AdminImpl : public Admin,
 
     void start() override {
       tls_->set([](Event::Dispatcher& dispatcher) -> ThreadLocal::ThreadLocalObjectSharedPtr {
-        return std::make_shared<NullThreadLocalOverloadState>(dispatcher);
+        return std::make_shared<OverloadState>(dispatcher);
       });
     }
 
     ThreadLocalOverloadState& getThreadLocalOverloadState() override {
-      return tls_->getTyped<NullThreadLocalOverloadState>();
+      return tls_->getTyped<OverloadState>();
     }
 
     Event::ScaledRangeTimerManagerFactory scaledTimerFactory() override { return nullptr; }

source/server/admin/stats_handler.cc

@@ -86,52 +86,60 @@ Http::Code StatsHandler::handlerStats(absl::string_view url,
   }
 
   const absl::optional<std::string> format_value = Utility::formatParam(params);
-  if (format_value.has_value() && format_value.value() == "prometheus") {
-    return handlerPrometheusStats(url, response_headers, response, admin_stream);
+  bool json = false;
+  if (format_value.has_value()) {
+    if (format_value.value() == "prometheus") {
+      return handlerPrometheusStats(url, response_headers, response, admin_stream);
+    } else if (format_value.value() == "json") {
+      json = true;
+    } else {
+      response.add("usage: /stats?format=json  or /stats?format=prometheus \n");
+      response.add("\n");
+      return Http::Code::NotFound;
+    }
   }
+  return handlerStats(server_.stats(), used_only, json, regex, response_headers, response);
+}
 
+Http::Code StatsHandler::handlerStats(Stats::Store& stats, bool used_only, bool json,
+                                      const absl::optional<std::regex>& regex,
+                                      Http::ResponseHeaderMap& response_headers,
+                                      Buffer::Instance& response) {
   std::map<std::string, uint64_t> all_stats;
-  for (const Stats::CounterSharedPtr& counter : server_.stats().counters()) {
+  for (const Stats::CounterSharedPtr& counter : stats.counters()) {
     if (shouldShowMetric(*counter, used_only, regex)) {
       all_stats.emplace(counter->name(), counter->value());
     }
   }
 
-  for (const Stats::GaugeSharedPtr& gauge : server_.stats().gauges()) {
+  for (const Stats::GaugeSharedPtr& gauge : stats.gauges()) {
     if (shouldShowMetric(*gauge, used_only, regex)) {
       ASSERT(gauge->importMode() != Stats::Gauge::ImportMode::Uninitialized);
       all_stats.emplace(gauge->name(), gauge->value());
     }
   }
 
   std::map<std::string, std::string> text_readouts;
-  for (const auto& text_readout : server_.stats().textReadouts()) {
+  for (const auto& text_readout : stats.textReadouts()) {
     if (shouldShowMetric(*text_readout, used_only, regex)) {
       text_readouts.emplace(text_readout->name(), text_readout->value());
     }
   }
 
-  Stats::Store& stats = server_.stats();
   std::vector<Stats::ParentHistogramSharedPtr> histograms = stats.histograms();
   stats.symbolTable().sortByStatNames<Stats::ParentHistogramSharedPtr>(
       histograms.begin(), histograms.end(),
       [](const Stats::ParentHistogramSharedPtr& a) -> Stats::StatName { return a->statName(); });
 
-  if (!format_value.has_value()) {
+  if (!json) {
     // Display plain stats if format query param is not there.
     statsAsText(all_stats, text_readouts, histograms, used_only, regex, response);
     return Http::Code::OK;
   }
 
-  if (format_value.value() == "json") {
-    response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Json);
-    response.add(statsAsJson(all_stats, text_readouts, histograms, used_only, regex));
-    return Http::Code::OK;
-  }
-
-  response.add("usage: /stats?format=json  or /stats?format=prometheus \n");
-  response.add("\n");
-  return Http::Code::NotFound;
+  response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Json);
+  response.add(statsAsJson(all_stats, text_readouts, histograms, used_only, regex));
+  return Http::Code::OK;
 }
 
 Http::Code StatsHandler::handlerPrometheusStats(absl::string_view path_and_query,

source/server/admin/stats_handler.h

@@ -40,6 +40,11 @@ class StatsHandler : public HandlerContextBase {
   Http::Code handlerStats(absl::string_view path_and_query,
                           Http::ResponseHeaderMap& response_headers, Buffer::Instance& response,
                           AdminStream&);
+  static Http::Code handlerStats(Stats::Store& stats, bool used_only, bool json,
+                                 const absl::optional<std::regex>& filter,
+                                 Http::ResponseHeaderMap& response_headers,
+                                 Buffer::Instance& response);
+
   Http::Code handlerPrometheusStats(absl::string_view path_and_query,
                                     Http::ResponseHeaderMap& response_headers,
                                     Buffer::Instance& response, AdminStream&);
@@ -63,11 +68,11 @@ class StatsHandler : public HandlerContextBase {
                                  bool used_only, const absl::optional<std::regex>& regex,
                                  bool pretty_print = false);
 
-  void statsAsText(const std::map<std::string, uint64_t>& all_stats,
-                   const std::map<std::string, std::string>& text_readouts,
-                   const std::vector<Stats::ParentHistogramSharedPtr>& all_histograms,
-                   bool used_only, const absl::optional<std::regex>& regex,
-                   Buffer::Instance& response);
+  static void statsAsText(const std::map<std::string, uint64_t>& all_stats,
+                          const std::map<std::string, std::string>& text_readouts,
+                          const std::vector<Stats::ParentHistogramSharedPtr>& all_histograms,
+                          bool used_only, const absl::optional<std::regex>& regex,
+                          Buffer::Instance& response);
 };
 
 } // namespace Server

test/per_file_coverage.sh

@@ -82,7 +82,7 @@ declare -a KNOWN_LOW_COVERAGE=(
 "source/extensions/watchdog:83.3" # Death tests within extensions
 "source/extensions/watchdog/profile_action:83.3"
 "source/server:93.3" # flaky: be careful adjusting. See https://github.com/envoyproxy/envoy/issues/15239
-"source/server/admin:95.3"
+"source/server/admin:97.6"
 "source/server/config_validation:74.8"
 )
 

test/server/admin/BUILD

@@ -1,5 +1,6 @@
 load(
     "//bazel:envoy_build_system.bzl",
+    "envoy_cc_benchmark_binary",
     "envoy_cc_test",
     "envoy_cc_test_library",
     "envoy_package",
@@ -68,6 +69,18 @@ envoy_cc_test(
     ],
 )
 
+envoy_cc_benchmark_binary(
+    name = "stats_handler_speed_test",
+    srcs = ["stats_handler_speed_test.cc"],
+    deps = [
+        "//source/common/buffer:buffer_lib",
+        "//source/common/http:header_map_lib",
+        "//source/common/stats:thread_local_store_lib",
+        "//source/server/admin:stats_handler_lib",
+        "//test/mocks/server:admin_stream_mocks",
+    ],
+)
+
 envoy_cc_test(
     name = "runtime_handler_test",
     srcs = ["runtime_handler_test.cc"],

test/server/admin/admin_test.cc

@@ -66,7 +66,7 @@ TEST_P(AdminInstanceTest, WriteAddressToFile) {
 }
 
 TEST_P(AdminInstanceTest, AdminAddress) {
-  std::string address_out_path = TestEnvironment::temporaryPath("admin.address");
+  const std::string address_out_path = TestEnvironment::temporaryPath("admin.address");
   AdminImpl admin_address_out_path(cpu_profile_path_, server_, false);
   std::list<AccessLog::InstanceSharedPtr> access_logs;
   Filesystem::FilePathAndType file_info{Filesystem::DestinationType::File, "/dev/null"};
@@ -81,7 +81,8 @@ TEST_P(AdminInstanceTest, AdminAddress) {
 }
 
 TEST_P(AdminInstanceTest, AdminBadAddressOutPath) {
-  std::string bad_path = TestEnvironment::temporaryPath("some/unlikely/bad/path/admin.address");
+  const std::string bad_path =
+      TestEnvironment::temporaryPath("some/unlikely/bad/path/admin.address");
   AdminImpl admin_bad_address_out_path(cpu_profile_path_, server_, false);
   std::list<AccessLog::InstanceSharedPtr> access_logs;
   Filesystem::FilePathAndType file_info{Filesystem::DestinationType::File, "/dev/null"};
@@ -220,5 +221,68 @@ TEST_P(AdminInstanceTest, HelpUsesFormForMutations) {
   EXPECT_NE(-1, response.search(stats_href.data(), stats_href.size(), 0, 0));
 }
 
+class AdminTestingPeer {
+public:
+  AdminTestingPeer(AdminImpl& admin)
+      : admin_(admin), server_(admin.server_), listener_scope_(server_.stats().createScope("")) {}
+  AdminImpl::NullRouteConfigProvider& routeConfigProvider() { return route_config_provider_; }
+  AdminImpl::NullScopedRouteConfigProvider& scopedRouteConfigProvider() {
+    return scoped_route_config_provider_;
+  }
+  AdminImpl::NullOverloadManager& overloadManager() { return admin_.null_overload_manager_; }
+  AdminImpl::NullOverloadManager::OverloadState& overloadState() { return overload_state_; }
+  AdminImpl::AdminListenSocketFactory& socketFactory() { return socket_factory_; }
+  AdminImpl::AdminListener& listener() { return listener_; }
+
+private:
+  AdminImpl& admin_;
+  Server::Instance& server_;
+  AdminImpl::NullRouteConfigProvider route_config_provider_{server_.timeSource()};
+  AdminImpl::NullScopedRouteConfigProvider scoped_route_config_provider_{server_.timeSource()};
+  AdminImpl::NullOverloadManager::OverloadState overload_state_{server_.dispatcher()};
+  AdminImpl::AdminListenSocketFactory socket_factory_{nullptr};
+  Stats::ScopeSharedPtr listener_scope_;
+  AdminImpl::AdminListener listener_{admin_, std::move(listener_scope_)};
+};
+
+// Covers override methods for admin-specific implementations of classes in
+// admin.h, reducing a major source of reduced coverage-percent expectations in
+// source/server/admin. There remain a few uncovered lines that are somewhat
+// harder to construct tests for.
+TEST_P(AdminInstanceTest, Overrides) {
+  admin_.http1Settings();
+  admin_.originalIpDetectionExtensions();
+
+  AdminTestingPeer peer(admin_);
+
+  peer.routeConfigProvider().config();
+  peer.routeConfigProvider().configInfo();
+  peer.routeConfigProvider().lastUpdated();
+  peer.routeConfigProvider().onConfigUpdate();
+
+  peer.scopedRouteConfigProvider().lastUpdated();
+  peer.scopedRouteConfigProvider().getConfigProto();
+  peer.scopedRouteConfigProvider().getConfigVersion();
+  peer.scopedRouteConfigProvider().getConfig();
+  peer.scopedRouteConfigProvider().apiType();
+  peer.scopedRouteConfigProvider().getConfigProtos();
+
+  auto overload_name = Server::OverloadProactiveResourceName::GlobalDownstreamMaxConnections;
+  peer.overloadState().tryAllocateResource(overload_name, 0);
+  peer.overloadState().tryDeallocateResource(overload_name, 0);
+  peer.overloadState().isResourceMonitorEnabled(overload_name);
+
+  peer.overloadManager().scaledTimerFactory();
+
+  peer.socketFactory().clone();
+  peer.socketFactory().closeAllSockets();
+  peer.socketFactory().doFinalPreWorkerInit();
+
+  peer.listener().name();
+  peer.listener().udpListenerConfig();
+  peer.listener().direction();
+  peer.listener().tcpBacklogSize();
+}
+
 } // namespace Server
 } // namespace Envoy