Issue 596: Add profiler path to configuration

include/envoy/server/configuration.h

@@ -146,6 +146,11 @@ class Admin {
    */
   virtual const std::string& accessLogPath() PURE;
 
+  /**
+   * @return const std::string& profiler output path.
+   */
+  virtual const std::string& profilePath() PURE;
+
   /**
    * @return Network::Address::InstanceConstSharedPtr the server address.
    */

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"],

source/common/profiler/profiler.cc

@@ -9,7 +9,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 +29,7 @@ void Heap::forceLink() {
 namespace Profiler {
 
 bool Cpu::profilerEnabled() { return false; }
-void Cpu::startProfiler(const std::string&) {}
+bool Cpu::startProfiler(const std::string&) { return false; }
 void Cpu::stopProfiler() {}
 
 } // Profiler

source/common/profiler/profiler.h

@@ -14,8 +14,9 @@ class Cpu {
 
   /**
    * Start the profiler and write to the specified path.
+   * @return bool whether the call to start profiler succeeded.
    */
-  static void startProfiler(const std::string& output_path);
+  static bool startProfiler(const std::string& output_path);
 
   /**
    * Stop the profiler.

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")) {

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& profilePath() override { return profile_path_; }
     Network::Address::InstanceConstSharedPtr address() override { return address_; }
 
     std::string access_log_path_;
+    std::string profile_path_;
     Network::Address::InstanceConstSharedPtr address_;
   };
 

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(profile_path_)) {
+      response.add("failure to start the profiler");
+      return Http::Code::InternalServerError;
+    }
+
   } else if (!enable && Profiler::Cpu::profilerEnabled()) {
     Profiler::Cpu::stopProfiler();
   }
@@ -295,9 +299,10 @@ void AdminFilter::onComplete() {
 AdminImpl::NullRouteConfigProvider::NullRouteConfigProvider()
     : config_(new Router::NullConfigImpl()) {}
 
-AdminImpl::AdminImpl(const std::string& access_log_path,
+AdminImpl::AdminImpl(const std::string& access_log_path, const std::string& profile_path,
                      Network::Address::InstanceConstSharedPtr 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())),
@@ -317,7 +322,6 @@ AdminImpl::AdminImpl(const std::string& access_log_path,
           {"/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()));

source/server/http/admin.h

@@ -22,8 +22,8 @@ class AdminImpl : public Admin,
                   public Http::ConnectionManagerConfig,
                   Logger::Loggable<Logger::Id::admin> {
 public:
-  AdminImpl(const std::string& access_log_path, Network::Address::InstanceConstSharedPtr address,
-            Server::Instance& server);
+  AdminImpl(const std::string& access_log_path, const std::string& profiler_path,
+            Network::Address::InstanceConstSharedPtr address, Server::Instance& server);
 
   Http::Code runCallback(const std::string& path, Buffer::Instance& response);
   Network::ListenSocket& socket() { return *socket_; }
@@ -113,6 +113,7 @@ class AdminImpl : public Admin,
 
   Server::Instance& server_;
   std::list<Http::AccessLog::InstanceSharedPtr> access_logs_;
+  std::string profile_path_;
   Network::ListenSocketPtr socket_;
   Http::ConnectionManagerStats stats_;
   Http::ConnectionManagerTracingStats tracing_stats_;

source/server/server.cc

@@ -170,7 +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().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());

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",

test/integration/integration_admin_test.cc

@@ -105,28 +105,39 @@ TEST_F(IntegrationTest, Admin) {
   EXPECT_TRUE(response->complete());
   EXPECT_STREQ("400", response->headers().Status()->value().c_str());
 
-  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("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

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"
@@ -12,8 +13,10 @@ 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", Network::Utility::resolveUrl("tcp://127.0.0.1:9002"), server_),
+      : admin_("/dev/null", "/tmp/envoy.prof", Network::Utility::resolveUrl("tcp://127.0.0.1:9002"),
+               server_),
         filter_(admin_), request_headers_{{":path", "/"}} {
     filter_.setDecoderFilterCallbacks(callbacks_);
   }
@@ -45,4 +48,27 @@ 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);
+  EXPECT_TRUE(Profiler::Cpu::profilerEnabled());
+  admin_.runCallback("/cpuprofiler?enable=n", data);
+  EXPECT_FALSE(Profiler::Cpu::profilerEnabled());
+}
+
+#endif
+
+TEST_F(AdminFilterTest, AdminBadProfiler) {
+  Buffer::OwnedImpl 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());
+}
+
 } // namespace Server