Guard most logging with the verbose logging option #36

igorpeshansky · 2017-10-12T16:42:09Z

Adds command-line flag support as well.

bmoyles0117

I would generally vote for log levels instead of having conditionals based on the flag everywhere. I think this is a good interim solution for reducing log spam, but the complexity could be greatly reduced with log levels.

bmoyles0117 · 2017-10-13T17:42:32Z

src/api_server.cc

      // frequent, and could be promoted to ERROR.
-      LOG(WARNING) << "No matching resource for " << id;
+      if (agent_.config_.VerboseLogging()) {
+        LOG(WARNING) << "No matching resource for " << id;


I would consider logging this at least once per resource instead of guarding by verbose logging, as I would agree repetitively spamming an undiscovered resource would be spammy, even though this would happen within the 60 second poll interval for newly created docker containers.

Logging this once isn't something that's easy to do with the current logging framework.
Besides, this is a handler for the incoming requests, so it would log every time an agent wants to translate a resource, which is a lot more frequent than once a minute.

bmoyles0117 · 2017-10-13T17:45:06Z

src/configuration.cc

+  boost::program_options::options_description flags_desc;
+  flags_desc.add_options()
+      ("help", "Print help message")
+      ("verbose", boost::program_options::value<bool>(&verbose_logging_),


Will this support shorthand for the first letter as well? It would be nice to have -v as well as --verbose.

bmoyles0117 · 2017-10-13T17:47:00Z

src/environment.cc

 };

-json::value ReadCredentials(const std::string& credentials_file)
+json::value ReadCredentials(const std::string& credentials_file, bool verbose)


s/verbose/verbose_logging/g for consistency.

igorpeshansky

Thanks. PTAL.

igorpeshansky · 2017-10-13T18:32:09Z

src/configuration.cc

+  boost::program_options::options_description flags_desc;
+  flags_desc.add_options()
+      ("help", "Print help message")
+      ("verbose", boost::program_options::value<bool>(&verbose_logging_),


igorpeshansky · 2017-10-13T18:33:03Z

src/environment.cc

 };

-json::value ReadCredentials(const std::string& credentials_file)
+json::value ReadCredentials(const std::string& credentials_file, bool verbose)


igorpeshansky · 2017-10-13T18:34:58Z

src/api_server.cc

      // frequent, and could be promoted to ERROR.
-      LOG(WARNING) << "No matching resource for " << id;
+      if (agent_.config_.VerboseLogging()) {
+        LOG(WARNING) << "No matching resource for " << id;


Logging this once isn't something that's easy to do with the current logging framework.
Besides, this is a handler for the incoming requests, so it would log every time an agent wants to translate a resource, which is a lot more frequent than once a minute.

bmoyles0117 · 2017-10-19T15:54:05Z

src/configuration.cc

+          .options(all_desc).positional(positional_desc).run(), flags);
+  boost::program_options::notify(flags);
+
+  if (flags.count("help")) {


Will this work if -h is passed in instead of --help?

Yes, it will.

bmoyles0117

LGTM

dhrupadb · 2017-10-19T17:49:05Z

src/configuration.cc

+      ("verbose", boost::program_options::value<bool>(&verbose_logging_),
+           "Enable verbose logging")
+      ;
+  boost::program_options::options_description hidden_desc;


Could you explain why this is hidden as opposed to just setup to have a default value?

I didn't want it to show up in the --help output. Maybe that was overkill. I went back and forth on that (and also tried an approach where the config file could only be specified as a positional parameter, but quickly reverted that). But it's harmless to leave it as-is for now.

Sounds good to me just wanted the rationale behind it. Thanks.

dhrupadb · 2017-10-19T17:56:12Z

src/api_server.cc

      // As we add more resource mappings, these will become less and less
      // frequent, and could be promoted to ERROR.
-      LOG(WARNING) << "No matching resource for " << id;
+      if (agent_.config_.VerboseLogging()) {


Is this the only way to do it? Seems like if conditionals to guard logging defeats the purpose of using log levels like INFO and DEBUG. Also elsewhere.

Right, the two are orthogonal. With verbose logging, we can get messages at all 4 levels (ERROR, WARNING, INFO, DEBUG). You're right that ideally this would be built into the logging framework itself, but that's a much larger undertaking.

I think we all agree here, I had spoken with @igorpeshansky offline and we came to the conclusion that the goal for now is to reduce log spam with a simple flag and implement full support for log levels later on down the road.

I see. Alright. Deferring for now.

dhrupadb

LGTM 👌

dhrupadb · 2017-10-19T18:53:21Z

src/configuration.cc

+      ("verbose", boost::program_options::value<bool>(&verbose_logging_),
+           "Enable verbose logging")
+      ;
+  boost::program_options::options_description hidden_desc;


Sounds good to me just wanted the rationale behind it. Thanks.

dhrupadb

LGTM 👌

igorpeshansky added 2 commits October 12, 2017 12:39

Add command-line options support.

9828f01

Guard most logging with the verbose logging option.

55be0f4

igorpeshansky requested review from bmoyles0117, dhrupadb and qingling128 October 12, 2017 16:42

bmoyles0117 reviewed Oct 13, 2017

View reviewed changes

Make --verbose a switch (no argument); add shorthand; fixup.

c88b197

igorpeshansky commented Oct 13, 2017

View reviewed changes

bmoyles0117 reviewed Oct 19, 2017

View reviewed changes

bmoyles0117 approved these changes Oct 19, 2017

View reviewed changes

dhrupadb suggested changes Oct 19, 2017

View reviewed changes

dhrupadb approved these changes Oct 19, 2017

View reviewed changes

igorpeshansky and others added 2 commits October 20, 2017 16:10

Add missing include.

6e73a0e

Add libboost-program-options to runtime dependencies. (#38)

acb8d79

dhrupadb approved these changes Nov 7, 2017

View reviewed changes

igorpeshansky merged commit c9d1816 into master Nov 7, 2017

igorpeshansky deleted the igorp-verbose-logging branch November 7, 2017 18:00

Guard most logging with the verbose logging option #36

Guard most logging with the verbose logging option #36

Uh oh!

Conversation

igorpeshansky commented Oct 12, 2017

Uh oh!

bmoyles0117 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!

bmoyles0117 Oct 13, 2017 • edited by igorpeshansky Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmoyles0117 Oct 13, 2017 • edited by igorpeshansky Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky 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!

bmoyles0117 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!

dhrupadb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhrupadb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bmoyles0117 Oct 13, 2017 •

edited by igorpeshansky

Loading

bmoyles0117 Oct 13, 2017 •

edited by igorpeshansky

Loading

dhrupadb left a comment •

edited

Loading