Skip to content

Issue 596: Add profiler path to configuration #603

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattklein123 merged 8 commits into from
Mar 22, 2017
Merged

Issue 596: Add profiler path to configuration #603

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6a3b748
Update handlerCpuProfiler and add profiler_path
hennna Mar 21, 2017
2059a8b
Clean up profiler and admin_test files
hennna Mar 21, 2017
45eeba9
Fix address asan build fail in Cpu::startProfiler()
hennna Mar 22, 2017
f9968bb
Clean up profiler and admin_test files v2
hennna Mar 22, 2017
9e3ae3d
Edit addressing comments
hennna Mar 22, 2017
a92b095
Merge remote-tracking branch 'upstream/master' into issue-596
hennna Mar 22, 2017
0aa7ac9
Allow admin profile_path JSON configurability
hennna Mar 22, 2017
8dd2764
Address comments for JSON configurability
hennna Mar 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions include/envoy/server/configuration.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,11 @@ class Admin {
*/
virtual const std::string& accessLogPath() PURE;

/**
* @return const std::string& the admin profiler path.

@htuch htuch Mar 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it might be more semantically correct to call this profilePath (and similar renames elsewhere). It's where the profile is emitted, not the profiler itself. The comment might then be "@return const std::string& profiler output path."

*/
virtual const std::string& profilerPath() PURE;

/**
* @return Network::Address::InstancePtr the server address.
*/
Expand Down
1 change: 1 addition & 0 deletions source/common/json/config_schemas.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"},

@htuch htuch Mar 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto etc.

@htuch htuch Mar 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you change config_schemas.cc, it's usually also necessary to update the user configuration docs. In this case, the file to update is https://github.com/lyft/envoy/blob/master/docs/configuration/overview/admin.rst.

@htuch htuch Mar 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also defer the config_schemas.cc change to your next PR, since it's not used yet here.

"address" : {"type" : "string"}
},
"required" : ["access_log_path", "address"],
Expand Down
6 changes: 4 additions & 2 deletions source/common/profiler/profiler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(); }

Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion source/common/profiler/profiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

@htuch htuch Mar 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add some return value documentation.


/**
* Stop the profiler.
Expand Down
2 changes: 2 additions & 0 deletions source/server/configuration_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
};

Expand Down
12 changes: 8 additions & 4 deletions source/server/http/admin.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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=<y|n>\n");

@htuch htuch Mar 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a more meaningful response would indicate there was a failure to start the profiler.

return Http::Code::BadRequest;

@htuch htuch Mar 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This returns a 400. I think it's not actually a bad client formed request, it's an internal server error, so you want to return a 500.

}

} else if (!enable && Profiler::Cpu::profilerEnabled()) {
Profiler::Cpu::stopProfiler();
}
Expand Down Expand Up @@ -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.",
Expand All @@ -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)}} {

@htuch htuch Mar 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: unrelated line deletion?

profiler_path_ = profiler_path;

@htuch htuch Mar 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nicer to initialize this in the above constructor member initialization list, i.e.

: server_(server), ..., profiler_path_(profiler_path)

access_logs_.emplace_back(new Http::AccessLog::InstanceImpl(
access_log_path, {}, Http::AccessLog::AccessLogFormatUtils::defaultAccessLogFormatter(),
server.accessLogManager()));
Expand Down
7 changes: 5 additions & 2 deletions source/server/http/admin.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::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_; }
Expand Down Expand Up @@ -111,6 +111,9 @@ class AdminImpl : public Admin,

Server::Instance& server_;
std::list<Http::AccessLog::InstancePtr> 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_;
Expand Down
1 change: 1 addition & 0 deletions source/server/server.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_,
Expand Down
4 changes: 3 additions & 1 deletion test/integration/integration_admin_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@htuch htuch Mar 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convention used in Envoy is TODO(hennna): foo bar.

// 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);
Expand Down
21 changes: 20 additions & 1 deletion test/server/http/admin_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
Expand All @@ -13,13 +14,17 @@ 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/envoy.prof", Network::Utility::resolveUrl("tcp://127.0.0.1:9002"),

@htuch htuch Mar 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a fault in this PR (it's a widespread issue in Envoy today), but using /tmp is kind of dangerous, since you may conflict with other users or test runs in a shared environment. You can keep the code as is, since we don't have a better solution yet, but add a TODO(htuch): Use proper temporary path allocation method. here?

@htuch htuch Mar 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've filed #605 to track this work.

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<MockInstance> server_;
AdminImpl admin_;
AdminImpl admin_bad_profiler_path_;

@htuch htuch Mar 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this needs to be a member of AdminFilterTest, it can be just allocated and instantiated locally in AdminBadProfiler.

AdminFilter filter_;
NiceMock<Http::MockStreamDecoderFilterCallbacks> callbacks_;
Http::TestHeaderMapImpl request_headers_;
Expand All @@ -45,4 +50,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