Skip to content

Commit

Permalink
admin: improve test coverage and increase the coverage-percent thresh…
Browse files Browse the repository at this point in the history 
…old (#20025)

Adds a missing test for recent lookups now that there are no more fake symbol tables. Adds tests for a variety of override methods defined in admin.h that were previously hard to hit.

Adds a benchmark test to establish a baseline for the speedups in #19693

Signed-off-by: Joshua Marantz <[email protected]>
  • Loading branch information
@jmarantz
jmarantz authored Mar 1, 2022
1 parent 9358689 commit 541dc48
Show file tree
Hide file tree
Showing 8 changed files with 276 additions and 47 deletions.
17 changes: 6 additions & 11 deletions source/server/admin/admin.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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.
*/
Expand Down Expand Up @@ -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; }
Expand All @@ -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; }
Expand Down
40 changes: 24 additions & 16 deletions source/server/admin/stats_handler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
15 changes: 10 additions & 5 deletions source/server/admin/stats_handler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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&);
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion test/per_file_coverage.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down
13 changes: 13 additions & 0 deletions test/server/admin/BUILD
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
load(
"//bazel:envoy_build_system.bzl",
"envoy_cc_benchmark_binary",
"envoy_cc_test",
"envoy_cc_test_library",
"envoy_package",
Expand Down Expand Up @@ -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"],
Expand Down
68 changes: 66 additions & 2 deletions test/server/admin/admin_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"};
Expand All @@ -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"};
Expand Down Expand Up @@ -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
Loading

0 comments on commit 541dc48

Please sign in to comment.