Issue 596: Add profiler path to configuration by hennna · Pull Request #603 · envoyproxy/envoy

hennna · 2017-03-21T22:54:19Z

Step 1

Added profiler_path_ member variable to AdminImpl class. Allows profiler path to be configurable rather than hard coded.
Return Http::Code::BadRequest when call to Cpu::startProfiler(...) fails.

Step 2

Update admin profiler_path JSON configuration

Issue link: #596

@htuch for comments.

htuch

Overall, looks great. Mostly minor comments.

htuch · 2017-03-22T13:24:58Z

include/envoy/server/configuration.h

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

+  /**


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

htuch · 2017-03-22T13:25:10Z

source/common/json/config_schemas.cc

        "type" : "object",
        "properties" : {
          "access_log_path" : {"type" : "string"},
+          "profiler_path" : {"type" : "string"},


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.

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

htuch · 2017-03-22T13:26:39Z

source/common/profiler/profiler.h

   * 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);


Add some return value documentation.

htuch · 2017-03-22T13:29:48Z

source/server/http/admin.cc

           MAKE_HANDLER(handlerServerInfo)},
          {"/stats", "print server stats", MAKE_HANDLER(handlerStats)}} {
-
+  profiler_path_ = profiler_path;


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

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

htuch · 2017-03-22T13:30:59Z

test/integration/integration_admin_test.cc

  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


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

htuch · 2017-03-22T13:34:00Z

source/server/http/admin.cc

-    Profiler::Cpu::startProfiler("/var/log/envoy/envoy.prof");
+    if (!Profiler::Cpu::startProfiler(profiler_path_)) {
+      response.add("?enable=<y|n>\n");
+      return Http::Code::BadRequest;


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.

htuch · 2017-03-22T13:35:05Z

source/server/http/admin.cc

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


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

htuch · 2017-03-22T13:36:22Z

test/server/http/admin_test.cc


  NiceMock<MockInstance> server_;
  AdminImpl admin_;
+  AdminImpl admin_bad_profiler_path_;


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

htuch · 2017-03-22T13:38:49Z

test/server/http/admin_test.cc

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


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?

I've filed #605 to track this work.

mattklein123 · 2017-03-22T20:31:21Z

@hennna please sign CLA if you didn't already.

hennna · 2017-03-22T21:21:38Z

@mattklein123 just signed CLA

mattklein123 · 2017-03-22T21:54:31Z

source/common/profiler/profiler.h


  /**
   * Start the profiler and write to the specified path.
+   * @return bool whether the call to start profiler succeeded.


nit: "start the profiler"

mattklein123 · 2017-03-22T21:55:56Z

source/server/http/admin.h


  Server::Instance& server_;
  std::list<Http::AccessLog::InstanceSharedPtr> access_logs_;
+  std::string profile_path_;


mattklein123 · 2017-03-22T21:57:33Z

test/integration/integration_admin_test.cc

+#ifdef TCMALLOC
+
+TEST_F(IntegrationTest, AdminCpuProfilerStart) {
+  BufferingStreamDecoderPtr response = IntegrationUtil::makeSingleRequest(


I don't think you meant to move this request ("/") inside here. Should go back in other section.

mattklein123 · 2017-03-22T21:59:38Z

test/server/http/admin_test.cc

+
+TEST_F(AdminFilterTest, AdminBadProfiler) {
+  Buffer::OwnedImpl data;
+  AdminImpl admin_bad_profile_path("/dev/null", "/var/log/envoy/envoy.prof",


This is not going to fail in all envs. I would use a totally bogus path here.

htuch · 2017-03-22T21:52:43Z

source/server/http/admin.cc

          {"/server_info", "print server version/status information",
           MAKE_HANDLER(handlerServerInfo)},
          {"/stats", "print server stats", MAKE_HANDLER(handlerStats)}} {
-


Nit: unrelated line deletion?

htuch · 2017-03-22T21:56:17Z

test/integration/integration_admin_test.cc

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


What's going on with all these handler path changes? They don't seem related.

I moved the two enable=y and enable=n into it's own integration test called AdminCpuProfilerStart. I think this has to do with line order changes.

htuch · 2017-03-22T22:01:05Z

test/server/http/admin_test.cc

+
+TEST_F(AdminFilterTest, AdminBadProfiler) {
+  Buffer::OwnedImpl data;
+  AdminImpl admin_bad_profile_path("/dev/null", "/var/log/envoy/envoy.prof",


/var/log/envoy/envoy.prof might be a valid path in some environments. Maybe use /some/unlikely/bad/path.prof for now, we should deal with this better when cleaning up the temp file story. Another approach would be to mock file handling, but I think this is a bit of clutter and overhead to put this dependency injection into AdminImpl.

htuch

LGTM.

Automatic merge from submit-queue [DO NOT MERGE] Auto PR to update dependencies of proxy This PR will be merged automatically once checks are successful. ```release-note none ```

Signed-off-by: John Plevyak <jplevyak@gmail.com>

@theatrus

Description: currently apps would fail in IPv6 only networks. This configuration change makes it so that the default setting is used, which is AUTO. Risk Level: low Testing: will test on device Thanks to @theatrus for reporting! Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

@theatrus

Description: currently apps would fail in IPv6 only networks. This configuration change makes it so that the default setting is used, which is AUTO. Risk Level: low Testing: will test on device Thanks to @theatrus for reporting! Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

hennna added 2 commits March 21, 2017 18:39

Update handlerCpuProfiler and add profiler_path

6a3b748

Clean up profiler and admin_test files

2059a8b

hennna closed this Mar 22, 2017

hennna deleted the issue-596 branch March 22, 2017 00:15

hennna restored the issue-596 branch March 22, 2017 00:22

hennna added 2 commits March 21, 2017 20:50

Fix address asan build fail in Cpu::startProfiler()

45eeba9

Clean up profiler and admin_test files v2

f9968bb

hennna reopened this Mar 22, 2017

htuch reviewed Mar 22, 2017

View reviewed changes

hennna added 2 commits March 22, 2017 12:56

Edit addressing comments

9e3ae3d

Merge remote-tracking branch 'upstream/master' into issue-596

a92b095

Allow admin profile_path JSON configurability

0aa7ac9

mattklein123 reviewed Mar 22, 2017

View reviewed changes

htuch reviewed Mar 22, 2017

View reviewed changes

Address comments for JSON configurability

8dd2764

htuch reviewed Mar 22, 2017

View reviewed changes

mattklein123 approved these changes Mar 22, 2017

View reviewed changes

mattklein123 merged commit 16f197c into envoyproxy:master Mar 22, 2017

hennna mentioned this pull request Mar 23, 2017

Update admin.rst to include profile_path #611

Merged

hennna deleted the issue-596 branch March 23, 2017 21:52

PiotrSikora pushed a commit to PiotrSikora/envoy that referenced this pull request Aug 7, 2020

Move onResolveDns to the root context. (envoyproxy#603)

e7e95d7

Signed-off-by: John Plevyak <jplevyak@gmail.com>

Conversation

hennna commented Mar 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Mar 22, 2017

Uh oh!

hennna commented Mar 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hennna commented Mar 21, 2017 •

edited

Loading