From 6a3b74835bdd7de7faa38e97cc17f778dc55b025 Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Tue, 21 Mar 2017 17:23:40 -0400 Subject: [PATCH 1/7] Update handlerCpuProfiler and add profiler_path --- include/envoy/server/configuration.h | 5 +++++ source/common/json/config_schemas.cc | 1 + source/common/profiler/profiler.cc | 7 +++++-- source/common/profiler/profiler.h | 2 +- source/server/configuration_impl.h | 2 ++ source/server/http/admin.cc | 12 ++++++++---- source/server/http/admin.h | 7 +++++-- source/server/server.cc | 1 + test/integration/integration_admin_test.cc | 4 +++- test/server/http/admin_test.cc | 22 +++++++++++++++++++++- 10 files changed, 52 insertions(+), 11 deletions(-) diff --git a/include/envoy/server/configuration.h b/include/envoy/server/configuration.h index dffa7a34720c8..3fa04d49a0248 100644 --- a/include/envoy/server/configuration.h +++ b/include/envoy/server/configuration.h @@ -146,6 +146,11 @@ class Admin { */ virtual const std::string& accessLogPath() PURE; + /** + * @return const std::string& the admin profiler path. + */ + virtual const std::string& profilerPath() PURE; + /** * @return Network::Address::InstancePtr the server address. */ diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index f40baa6fa7155..8753eb7180695 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -899,6 +899,7 @@ const std::string Json::Schema::TOP_LEVEL_CONFIG_SCHEMA(R"EOF( "type" : "object", "properties" : { "access_log_path" : {"type" : "string"}, + "profiler_path" : {"type" : "string"}, "address" : {"type" : "string"} }, "required" : ["access_log_path", "address"], diff --git a/source/common/profiler/profiler.cc b/source/common/profiler/profiler.cc index d930e9fb37c02..19dfbae9e396c 100644 --- a/source/common/profiler/profiler.cc +++ b/source/common/profiler/profiler.cc @@ -2,6 +2,7 @@ #ifdef TCMALLOC +#include "common/common/assert.h" #include "gperftools/heap-profiler.h" #include "gperftools/profiler.h" @@ -9,7 +10,9 @@ namespace Profiler { bool Cpu::profilerEnabled() { return ProfilingIsEnabledForAllThreads(); } -void Cpu::startProfiler(const std::string& output_path) { ProfilerStart(output_path.c_str()); } +bool Cpu::startProfiler(const std::string& output_path) { + return ProfilerStart(output_path.c_str()); +} void Cpu::stopProfiler() { ProfilerStop(); } @@ -27,7 +30,7 @@ void Heap::forceLink() { namespace Profiler { bool Cpu::profilerEnabled() { return false; } -void Cpu::startProfiler(const std::string&) {} +bool Cpu::startProfiler(const std::string&) {} void Cpu::stopProfiler() {} } // Profiler diff --git a/source/common/profiler/profiler.h b/source/common/profiler/profiler.h index fca61797d4c13..824a64577e1d2 100644 --- a/source/common/profiler/profiler.h +++ b/source/common/profiler/profiler.h @@ -15,7 +15,7 @@ class Cpu { /** * Start the profiler and write to the specified path. */ - static void startProfiler(const std::string& output_path); + static bool startProfiler(const std::string& output_path); /** * Stop the profiler. diff --git a/source/server/configuration_impl.h b/source/server/configuration_impl.h index 99c1c7ec27466..173e76c987c91 100644 --- a/source/server/configuration_impl.h +++ b/source/server/configuration_impl.h @@ -157,9 +157,11 @@ class InitialImpl : public Initial { struct AdminImpl : public Admin { // Server::Configuration::Initial::Admin const std::string& accessLogPath() override { return access_log_path_; } + const std::string& profilerPath() override { return profiler_path_; } Network::Address::InstancePtr address() override { return address_; } std::string access_log_path_; + std::string profiler_path_; Network::Address::InstancePtr address_; }; diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index bb06f442a5b83..4c634d25dd218 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -163,7 +163,11 @@ Http::Code AdminImpl::handlerCpuProfiler(const std::string& url, Buffer::Instanc bool enable = query_params.begin()->second == "y"; if (enable && !Profiler::Cpu::profilerEnabled()) { - Profiler::Cpu::startProfiler("/var/log/envoy/envoy.prof"); + if (!Profiler::Cpu::startProfiler(profiler_path_)) { + response.add("?enable=\n"); + return Http::Code::BadRequest; + } + } else if (!enable && Profiler::Cpu::profilerEnabled()) { Profiler::Cpu::stopProfiler(); } @@ -295,8 +299,8 @@ void AdminFilter::onComplete() { AdminImpl::NullRouteConfigProvider::NullRouteConfigProvider() : config_(new Router::NullConfigImpl()) {} -AdminImpl::AdminImpl(const std::string& access_log_path, Network::Address::InstancePtr address, - Server::Instance& server) +AdminImpl::AdminImpl(const std::string& access_log_path, const std::string& profiler_path, + Network::Address::InstancePtr address, Server::Instance& server) : server_(server), socket_(new Network::TcpListenSocket(address, true)), stats_(Http::ConnectionManagerImpl::generateStats("http.admin.", server_.stats())), tracing_stats_(Http::ConnectionManagerImpl::generateTracingStats("http.admin.tracing.", @@ -317,7 +321,7 @@ AdminImpl::AdminImpl(const std::string& access_log_path, Network::Address::Insta {"/server_info", "print server version/status information", MAKE_HANDLER(handlerServerInfo)}, {"/stats", "print server stats", MAKE_HANDLER(handlerStats)}} { - + profiler_path_ = profiler_path; access_logs_.emplace_back(new Http::AccessLog::InstanceImpl( access_log_path, {}, Http::AccessLog::AccessLogFormatUtils::defaultAccessLogFormatter(), server.accessLogManager())); diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 6e56a907ebdd3..1c8815e86568e 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -22,8 +22,8 @@ class AdminImpl : public Admin, public Http::ConnectionManagerConfig, Logger::Loggable { public: - AdminImpl(const std::string& access_log_path, Network::Address::InstancePtr address, - Server::Instance& server); + AdminImpl(const std::string& access_log_path, const std::string& profiler_path, + Network::Address::InstancePtr address, Server::Instance& server); Http::Code runCallback(const std::string& path, Buffer::Instance& response); Network::ListenSocket& socket() { return *socket_; } @@ -111,6 +111,9 @@ class AdminImpl : public Admin, Server::Instance& server_; std::list access_logs_; + // TO DO: Remove default value after making profiler_path a configurable + // parameter via the Envoy config JSON + std::string profiler_path_ = "/var/log/envoy/envoy.prof"; Network::ListenSocketPtr socket_; Http::ConnectionManagerStats stats_; Http::ConnectionManagerTracingStats tracing_stats_; diff --git a/source/server/server.cc b/source/server/server.cc index fdacabf9dfa8b..fe9209f3027f0 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -170,6 +170,7 @@ void InstanceImpl::initialize(Options& options, TestHooks& hooks, drain_manager_->startParentShutdownSequence(); original_start_time_ = info.original_start_time_; admin_.reset(new AdminImpl(initial_config.admin().accessLogPath(), + initial_config.admin().profilerPath(), initial_config.admin().address(), *this)); admin_scope_ = stats_store_.createScope("listener.admin."); handler_.addListener(*admin_, admin_->socket(), *admin_scope_, diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc index 16190ae0ae9d4..906c2cd4b63f3 100644 --- a/test/integration/integration_admin_test.cc +++ b/test/integration/integration_admin_test.cc @@ -105,10 +105,12 @@ TEST_F(IntegrationTest, Admin) { EXPECT_TRUE(response->complete()); EXPECT_STREQ("400", response->headers().Status()->value().c_str()); + // TO DO: Change to expect 200 when profiler path is a configurable parameter + // via the Envoy config JSON. Currently call returns 400 due to inaccessable profiler path response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/cpuprofiler?enable=y", "", Http::CodecClient::Type::HTTP1); EXPECT_TRUE(response->complete()); - EXPECT_STREQ("200", response->headers().Status()->value().c_str()); + EXPECT_STREQ("400", response->headers().Status()->value().c_str()); response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/cpuprofiler?enable=n", "", Http::CodecClient::Type::HTTP1); diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index 1cb183cc682e0..5952c8a89f2ac 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -1,4 +1,5 @@ #include "common/http/message_impl.h" +#include "common/profiler/profiler.h" #include "server/http/admin.h" #include "test/mocks/server/mocks.h" @@ -13,16 +14,21 @@ class AdminFilterTest : public testing::Test { public: // TODO(mattklein123): Switch to mocks and do not bind to a real port. AdminFilterTest() - : admin_("/dev/null", Network::Utility::resolveUrl("tcp://127.0.0.1:9002"), server_), + : admin_("/dev/null", "/tmp/enovy.prof", Network::Utility::resolveUrl("tcp://127.0.0.1:9002"), + server_), + admin_bad_profiler_path_("/dev/null", "/var/log/envoy/envoy.prof", + Network::Utility::resolveUrl("tcp://127.0.0.1:9002"), server_), filter_(admin_), request_headers_{{":path", "/"}} { filter_.setDecoderFilterCallbacks(callbacks_); } NiceMock server_; AdminImpl admin_; + AdminImpl admin_bad_profiler_path_; AdminFilter filter_; NiceMock callbacks_; Http::TestHeaderMapImpl request_headers_; + Http::TestHeaderMapImpl profiler_headers_; }; TEST_F(AdminFilterTest, HeaderOnly) { @@ -45,4 +51,18 @@ TEST_F(AdminFilterTest, Trailers) { filter_.decodeTrailers(request_headers_); } +TEST_F(AdminFilterTest, AdminProfiler) { + Buffer::OwnedImpl data; + admin_.runCallback("/cpuprofiler?enable=y", data); + EXPECT_TRUE(Profiler::Cpu::profilerEnabled()); + admin_.runCallback("/cpuprofiler?enable=n", data); + EXPECT_FALSE(Profiler::Cpu::profilerEnabled()); +} + +TEST_F(AdminFilterTest, AdminBadProfiler) { + Buffer::OwnedImpl data; + admin_bad_profiler_path_.runCallback("/cpuprofiler?enable=y", data); + EXPECT_FALSE(Profiler::Cpu::profilerEnabled()); +} + } // namespace Server From 2059a8bbfe482b8df2ded6dc08569ec4ef69e1f1 Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Tue, 21 Mar 2017 18:36:22 -0400 Subject: [PATCH 2/7] Clean up profiler and admin_test files --- source/common/profiler/profiler.cc | 1 - test/server/http/admin_test.cc | 1 - 2 files changed, 2 deletions(-) diff --git a/source/common/profiler/profiler.cc b/source/common/profiler/profiler.cc index 19dfbae9e396c..735a8b34d03c0 100644 --- a/source/common/profiler/profiler.cc +++ b/source/common/profiler/profiler.cc @@ -2,7 +2,6 @@ #ifdef TCMALLOC -#include "common/common/assert.h" #include "gperftools/heap-profiler.h" #include "gperftools/profiler.h" diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index 5952c8a89f2ac..bee04bbb3c6c6 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -28,7 +28,6 @@ class AdminFilterTest : public testing::Test { AdminFilter filter_; NiceMock callbacks_; Http::TestHeaderMapImpl request_headers_; - Http::TestHeaderMapImpl profiler_headers_; }; TEST_F(AdminFilterTest, HeaderOnly) { From 45eeba92b6f532b94edf1fd0e0c6ddd4edea8f3b Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Tue, 21 Mar 2017 20:04:59 -0400 Subject: [PATCH 3/7] Fix address asan build fail in Cpu::startProfiler() --- source/common/profiler/profiler.cc | 3 ++- test/server/http/admin_test.cc | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/source/common/profiler/profiler.cc b/source/common/profiler/profiler.cc index 735a8b34d03c0..15261f56f6557 100644 --- a/source/common/profiler/profiler.cc +++ b/source/common/profiler/profiler.cc @@ -2,6 +2,7 @@ #ifdef TCMALLOC +#include "common/common/assert.h" #include "gperftools/heap-profiler.h" #include "gperftools/profiler.h" @@ -29,7 +30,7 @@ void Heap::forceLink() { namespace Profiler { bool Cpu::profilerEnabled() { return false; } -bool Cpu::startProfiler(const std::string&) {} +bool Cpu::startProfiler(const std::string&) { return false; } void Cpu::stopProfiler() {} } // Profiler diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index bee04bbb3c6c6..131629253d4b0 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -14,7 +14,7 @@ class AdminFilterTest : public testing::Test { public: // TODO(mattklein123): Switch to mocks and do not bind to a real port. AdminFilterTest() - : admin_("/dev/null", "/tmp/enovy.prof", Network::Utility::resolveUrl("tcp://127.0.0.1:9002"), + : admin_("/dev/null", "/tmp/envoy.prof", Network::Utility::resolveUrl("tcp://127.0.0.1:9002"), server_), admin_bad_profiler_path_("/dev/null", "/var/log/envoy/envoy.prof", Network::Utility::resolveUrl("tcp://127.0.0.1:9002"), server_), @@ -28,6 +28,7 @@ class AdminFilterTest : public testing::Test { AdminFilter filter_; NiceMock callbacks_; Http::TestHeaderMapImpl request_headers_; + Http::TestHeaderMapImpl profiler_headers_; }; TEST_F(AdminFilterTest, HeaderOnly) { From f9968bbb712825889b9e310e26318e9cc8483fb3 Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Tue, 21 Mar 2017 21:25:06 -0400 Subject: [PATCH 4/7] Clean up profiler and admin_test files v2 --- source/common/profiler/profiler.cc | 1 - test/server/http/admin_test.cc | 1 - 2 files changed, 2 deletions(-) diff --git a/source/common/profiler/profiler.cc b/source/common/profiler/profiler.cc index 15261f56f6557..bf7f6279ef2a3 100644 --- a/source/common/profiler/profiler.cc +++ b/source/common/profiler/profiler.cc @@ -2,7 +2,6 @@ #ifdef TCMALLOC -#include "common/common/assert.h" #include "gperftools/heap-profiler.h" #include "gperftools/profiler.h" diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index 131629253d4b0..e854c6d681fe8 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -28,7 +28,6 @@ class AdminFilterTest : public testing::Test { AdminFilter filter_; NiceMock callbacks_; Http::TestHeaderMapImpl request_headers_; - Http::TestHeaderMapImpl profiler_headers_; }; TEST_F(AdminFilterTest, HeaderOnly) { From 9e3ae3de734361e822d75129c09da5c24d474609 Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Wed, 22 Mar 2017 12:56:04 -0400 Subject: [PATCH 5/7] Edit addressing comments --- include/envoy/server/configuration.h | 4 ++-- source/common/json/config_schemas.cc | 1 - source/common/profiler/profiler.h | 1 + source/server/configuration_impl.h | 4 ++-- source/server/http/admin.cc | 12 ++++++------ source/server/http/admin.h | 4 ++-- source/server/server.cc | 4 ++-- test/integration/integration_admin_test.cc | 4 ++-- test/server/http/admin_test.cc | 15 +++++++++++---- 9 files changed, 28 insertions(+), 21 deletions(-) diff --git a/include/envoy/server/configuration.h b/include/envoy/server/configuration.h index 3fa04d49a0248..55b620e686b29 100644 --- a/include/envoy/server/configuration.h +++ b/include/envoy/server/configuration.h @@ -147,9 +147,9 @@ class Admin { virtual const std::string& accessLogPath() PURE; /** - * @return const std::string& the admin profiler path. + * @return const std::string& profiler output path. */ - virtual const std::string& profilerPath() PURE; + virtual const std::string& profilePath() PURE; /** * @return Network::Address::InstancePtr the server address. diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index 8753eb7180695..f40baa6fa7155 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -899,7 +899,6 @@ const std::string Json::Schema::TOP_LEVEL_CONFIG_SCHEMA(R"EOF( "type" : "object", "properties" : { "access_log_path" : {"type" : "string"}, - "profiler_path" : {"type" : "string"}, "address" : {"type" : "string"} }, "required" : ["access_log_path", "address"], diff --git a/source/common/profiler/profiler.h b/source/common/profiler/profiler.h index 824a64577e1d2..072e726602043 100644 --- a/source/common/profiler/profiler.h +++ b/source/common/profiler/profiler.h @@ -14,6 +14,7 @@ class Cpu { /** * Start the profiler and write to the specified path. + * @return bool whether the call to start profiler succeeded. */ static bool startProfiler(const std::string& output_path); diff --git a/source/server/configuration_impl.h b/source/server/configuration_impl.h index 173e76c987c91..b4af922748b28 100644 --- a/source/server/configuration_impl.h +++ b/source/server/configuration_impl.h @@ -157,11 +157,11 @@ class InitialImpl : public Initial { struct AdminImpl : public Admin { // Server::Configuration::Initial::Admin const std::string& accessLogPath() override { return access_log_path_; } - const std::string& profilerPath() override { return profiler_path_; } + const std::string& profilePath() override { return profile_path_; } Network::Address::InstancePtr address() override { return address_; } std::string access_log_path_; - std::string profiler_path_; + std::string profile_path_; Network::Address::InstancePtr address_; }; diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index 4c634d25dd218..bec2cbaf53405 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -163,9 +163,9 @@ Http::Code AdminImpl::handlerCpuProfiler(const std::string& url, Buffer::Instanc bool enable = query_params.begin()->second == "y"; if (enable && !Profiler::Cpu::profilerEnabled()) { - if (!Profiler::Cpu::startProfiler(profiler_path_)) { - response.add("?enable=\n"); - return Http::Code::BadRequest; + if (!Profiler::Cpu::startProfiler(profile_path_)) { + response.add("failure to start the profiler"); + return Http::Code::InternalServerError; } } else if (!enable && Profiler::Cpu::profilerEnabled()) { @@ -299,9 +299,10 @@ void AdminFilter::onComplete() { AdminImpl::NullRouteConfigProvider::NullRouteConfigProvider() : config_(new Router::NullConfigImpl()) {} -AdminImpl::AdminImpl(const std::string& access_log_path, const std::string& profiler_path, +AdminImpl::AdminImpl(const std::string& access_log_path, const std::string& profile_path, Network::Address::InstancePtr address, Server::Instance& server) - : server_(server), socket_(new Network::TcpListenSocket(address, true)), + : server_(server), profile_path_(profile_path), + socket_(new Network::TcpListenSocket(address, true)), stats_(Http::ConnectionManagerImpl::generateStats("http.admin.", server_.stats())), tracing_stats_(Http::ConnectionManagerImpl::generateTracingStats("http.admin.tracing.", server_.stats())), @@ -321,7 +322,6 @@ AdminImpl::AdminImpl(const std::string& access_log_path, const std::string& prof {"/server_info", "print server version/status information", MAKE_HANDLER(handlerServerInfo)}, {"/stats", "print server stats", MAKE_HANDLER(handlerStats)}} { - profiler_path_ = profiler_path; access_logs_.emplace_back(new Http::AccessLog::InstanceImpl( access_log_path, {}, Http::AccessLog::AccessLogFormatUtils::defaultAccessLogFormatter(), server.accessLogManager())); diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 1c8815e86568e..010d056efd131 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -111,9 +111,9 @@ class AdminImpl : public Admin, Server::Instance& server_; std::list access_logs_; - // TO DO: Remove default value after making profiler_path a configurable + // TODO(hennna): Remove default value after making profile_path a configurable // parameter via the Envoy config JSON - std::string profiler_path_ = "/var/log/envoy/envoy.prof"; + std::string profile_path_ = "/var/log/envoy/envoy.prof"; Network::ListenSocketPtr socket_; Http::ConnectionManagerStats stats_; Http::ConnectionManagerTracingStats tracing_stats_; diff --git a/source/server/server.cc b/source/server/server.cc index fe9209f3027f0..ca977c6cf0b05 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -170,8 +170,8 @@ void InstanceImpl::initialize(Options& options, TestHooks& hooks, drain_manager_->startParentShutdownSequence(); original_start_time_ = info.original_start_time_; admin_.reset(new AdminImpl(initial_config.admin().accessLogPath(), - initial_config.admin().profilerPath(), - initial_config.admin().address(), *this)); + initial_config.admin().profilePath(), initial_config.admin().address(), + *this)); admin_scope_ = stats_store_.createScope("listener.admin."); handler_.addListener(*admin_, admin_->socket(), *admin_scope_, Network::ListenerOptions::listenerOptionsWithBindToPort()); diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc index 906c2cd4b63f3..66b5153cac209 100644 --- a/test/integration/integration_admin_test.cc +++ b/test/integration/integration_admin_test.cc @@ -105,12 +105,12 @@ TEST_F(IntegrationTest, Admin) { EXPECT_TRUE(response->complete()); EXPECT_STREQ("400", response->headers().Status()->value().c_str()); - // TO DO: Change to expect 200 when profiler path is a configurable parameter + // TODO(hennna): Change to expect 200 when profile path is a configurable parameter // via the Envoy config JSON. Currently call returns 400 due to inaccessable profiler path response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/cpuprofiler?enable=y", "", Http::CodecClient::Type::HTTP1); EXPECT_TRUE(response->complete()); - EXPECT_STREQ("400", response->headers().Status()->value().c_str()); + EXPECT_STREQ("500", response->headers().Status()->value().c_str()); response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/cpuprofiler?enable=n", "", Http::CodecClient::Type::HTTP1); diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index e854c6d681fe8..37896c2fba045 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -13,18 +13,16 @@ namespace Server { class AdminFilterTest : public testing::Test { public: // TODO(mattklein123): Switch to mocks and do not bind to a real port. + // TODO(htuch): Use proper temporary path allocation method. AdminFilterTest() : admin_("/dev/null", "/tmp/envoy.prof", Network::Utility::resolveUrl("tcp://127.0.0.1:9002"), server_), - admin_bad_profiler_path_("/dev/null", "/var/log/envoy/envoy.prof", - Network::Utility::resolveUrl("tcp://127.0.0.1:9002"), server_), filter_(admin_), request_headers_{{":path", "/"}} { filter_.setDecoderFilterCallbacks(callbacks_); } NiceMock server_; AdminImpl admin_; - AdminImpl admin_bad_profiler_path_; AdminFilter filter_; NiceMock callbacks_; Http::TestHeaderMapImpl request_headers_; @@ -50,6 +48,11 @@ TEST_F(AdminFilterTest, Trailers) { filter_.decodeTrailers(request_headers_); } +// Can only get code coverage of AdminImpl::handlerCpuProfiler stopProfiler with +// a real profiler linked in (successful call to startProfiler). startProfiler +// requies tcmalloc. +#ifdef TCMALLOC + TEST_F(AdminFilterTest, AdminProfiler) { Buffer::OwnedImpl data; admin_.runCallback("/cpuprofiler?enable=y", data); @@ -58,9 +61,13 @@ TEST_F(AdminFilterTest, AdminProfiler) { EXPECT_FALSE(Profiler::Cpu::profilerEnabled()); } +#endif + TEST_F(AdminFilterTest, AdminBadProfiler) { Buffer::OwnedImpl data; - admin_bad_profiler_path_.runCallback("/cpuprofiler?enable=y", data); + AdminImpl admin_bad_profile_path("/dev/null", "/var/log/envoy/envoy.prof", + Network::Utility::resolveUrl("tcp://127.0.0.1:9002"), server_); + admin_bad_profile_path.runCallback("/cpuprofiler?enable=y", data); EXPECT_FALSE(Profiler::Cpu::profilerEnabled()); } From 0aa7ac93e203c527b6803abd755ef117fb9f23ee Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Wed, 22 Mar 2017 17:17:25 -0400 Subject: [PATCH 6/7] Allow admin profile_path JSON configurability --- source/common/json/config_schemas.cc | 1 + source/server/configuration_impl.cc | 1 + source/server/http/admin.h | 4 +--- test/config/integration/server.json | 2 +- test/integration/integration_admin_test.cc | 25 +++++++++++++++------- 5 files changed, 21 insertions(+), 12 deletions(-) diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index f40baa6fa7155..00f1269f92794 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -899,6 +899,7 @@ const std::string Json::Schema::TOP_LEVEL_CONFIG_SCHEMA(R"EOF( "type" : "object", "properties" : { "access_log_path" : {"type" : "string"}, + "profile_path" : {"type" : "string"}, "address" : {"type" : "string"} }, "required" : ["access_log_path", "address"], diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index 1ffbbb2c9a91d..0e4ce71c355d7 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -185,6 +185,7 @@ bool MainImpl::ListenerConfig::createFilterChain(Network::Connection& connection InitialImpl::InitialImpl(const Json::Object& json) { Json::ObjectPtr admin = json.getObject("admin"); admin_.access_log_path_ = admin->getString("access_log_path"); + admin_.profile_path_ = admin->getString("profile_path", "/var/log/envoy/envoy.prof"); admin_.address_ = Network::Utility::resolveUrl(admin->getString("address")); if (json.hasObject("flags_path")) { diff --git a/source/server/http/admin.h b/source/server/http/admin.h index b007d27f78aa4..8bc124ac56c54 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -113,9 +113,7 @@ class AdminImpl : public Admin, Server::Instance& server_; std::list access_logs_; - // TODO(hennna): Remove default value after making profile_path a configurable - // parameter via the Envoy config JSON - std::string profile_path_ = "/var/log/envoy/envoy.prof"; + std::string profile_path_; Network::ListenSocketPtr socket_; Http::ConnectionManagerStats stats_; Http::ConnectionManagerTracingStats tracing_stats_; diff --git a/test/config/integration/server.json b/test/config/integration/server.json index ce3e987809931..b7c38a1e656ca 100644 --- a/test/config/integration/server.json +++ b/test/config/integration/server.json @@ -244,7 +244,7 @@ }] }], - "admin": { "access_log_path": "/dev/null", "address": "tcp://127.0.0.1:10003" }, + "admin": { "access_log_path": "/dev/null", "profile_path": "/tmp/envoy.prof", "address": "tcp://127.0.0.1:10003" }, "flags_path": "/invalid_flags", "statsd_local_udp_port": 8125, "statsd_tcp_cluster_name": "statsd", diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc index 66b5153cac209..721f335cc7ab8 100644 --- a/test/integration/integration_admin_test.cc +++ b/test/integration/integration_admin_test.cc @@ -105,30 +105,39 @@ TEST_F(IntegrationTest, Admin) { EXPECT_TRUE(response->complete()); EXPECT_STREQ("400", response->headers().Status()->value().c_str()); - // TODO(hennna): Change to expect 200 when profile path is a configurable parameter - // via the Envoy config JSON. Currently call returns 400 due to inaccessable profiler path - response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/cpuprofiler?enable=y", "", + response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/hot_restart_version", "", Http::CodecClient::Type::HTTP1); EXPECT_TRUE(response->complete()); - EXPECT_STREQ("500", response->headers().Status()->value().c_str()); + EXPECT_STREQ("200", response->headers().Status()->value().c_str()); - response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/cpuprofiler?enable=n", "", + response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/reset_counters", "", Http::CodecClient::Type::HTTP1); EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); - response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/hot_restart_version", "", + response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/certs", "", Http::CodecClient::Type::HTTP1); EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); +} - response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/reset_counters", "", +// Successful call to startProfiler requires tcmalloc. +#ifdef TCMALLOC + +TEST_F(IntegrationTest, AdminCpuProfilerStart) { + BufferingStreamDecoderPtr response = IntegrationUtil::makeSingleRequest( + ADMIN_PORT, "GET", "/", "", Http::CodecClient::Type::HTTP1); + EXPECT_TRUE(response->complete()); + EXPECT_STREQ("404", response->headers().Status()->value().c_str()); + + response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/cpuprofiler?enable=y", "", Http::CodecClient::Type::HTTP1); EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); - response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/certs", "", + response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/cpuprofiler?enable=n", "", Http::CodecClient::Type::HTTP1); EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); } +#endif From 8dd27645fda389b701b01e5e5681b5b5bf1520bf Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Wed, 22 Mar 2017 18:27:44 -0400 Subject: [PATCH 7/7] Address comments for JSON configurability --- source/common/profiler/profiler.h | 2 +- source/server/http/admin.cc | 1 + source/server/http/admin.h | 2 +- test/integration/integration_admin_test.cc | 7 +------ test/server/http/admin_test.cc | 2 +- 5 files changed, 5 insertions(+), 9 deletions(-) diff --git a/source/common/profiler/profiler.h b/source/common/profiler/profiler.h index 072e726602043..1944ac27d1dd1 100644 --- a/source/common/profiler/profiler.h +++ b/source/common/profiler/profiler.h @@ -14,7 +14,7 @@ class Cpu { /** * Start the profiler and write to the specified path. - * @return bool whether the call to start profiler succeeded. + * @return bool whether the call to start the profiler succeeded. */ static bool startProfiler(const std::string& output_path); diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index 3264f603fbdd4..387a535cbe6fe 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -322,6 +322,7 @@ AdminImpl::AdminImpl(const std::string& access_log_path, const std::string& prof {"/server_info", "print server version/status information", MAKE_HANDLER(handlerServerInfo)}, {"/stats", "print server stats", MAKE_HANDLER(handlerStats)}} { + access_logs_.emplace_back(new Http::AccessLog::InstanceImpl( access_log_path, {}, Http::AccessLog::AccessLogFormatUtils::defaultAccessLogFormatter(), server.accessLogManager())); diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 8bc124ac56c54..b6b29f9a7a806 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -113,7 +113,7 @@ class AdminImpl : public Admin, Server::Instance& server_; std::list access_logs_; - std::string profile_path_; + const std::string profile_path_; Network::ListenSocketPtr socket_; Http::ConnectionManagerStats stats_; Http::ConnectionManagerTracingStats tracing_stats_; diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc index 721f335cc7ab8..ead6c6d90c7c3 100644 --- a/test/integration/integration_admin_test.cc +++ b/test/integration/integration_admin_test.cc @@ -126,12 +126,7 @@ TEST_F(IntegrationTest, Admin) { TEST_F(IntegrationTest, AdminCpuProfilerStart) { BufferingStreamDecoderPtr response = IntegrationUtil::makeSingleRequest( - ADMIN_PORT, "GET", "/", "", Http::CodecClient::Type::HTTP1); - EXPECT_TRUE(response->complete()); - EXPECT_STREQ("404", response->headers().Status()->value().c_str()); - - response = IntegrationUtil::makeSingleRequest(ADMIN_PORT, "GET", "/cpuprofiler?enable=y", "", - Http::CodecClient::Type::HTTP1); + ADMIN_PORT, "GET", "/cpuprofiler?enable=y", "", Http::CodecClient::Type::HTTP1); EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index 37896c2fba045..f2deb3b850f38 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -65,7 +65,7 @@ TEST_F(AdminFilterTest, AdminProfiler) { TEST_F(AdminFilterTest, AdminBadProfiler) { Buffer::OwnedImpl data; - AdminImpl admin_bad_profile_path("/dev/null", "/var/log/envoy/envoy.prof", + AdminImpl admin_bad_profile_path("/dev/null", "/some/unlikely/bad/path.prof", Network::Utility::resolveUrl("tcp://127.0.0.1:9002"), server_); admin_bad_profile_path.runCallback("/cpuprofiler?enable=y", data); EXPECT_FALSE(Profiler::Cpu::profilerEnabled());