-
Notifications
You must be signed in to change notification settings - Fork 11
Guard most logging with the verbose logging option #36
Changes from all commits
9828f01
55be0f4
c88b197
6e73a0e
acb8d79
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,23 +109,29 @@ void MetadataApiServer::Handler::operator()(const HttpServer::request& request, | |
| static const std::string kPrefix = "/monitoredResource/"; | ||
| // The format for the local metadata API request is: | ||
| // {host}:{port}/monitoredResource/{id} | ||
| LOG(INFO) << "Handler called: " << request.method | ||
| << " " << request.destination | ||
| << " headers: " << request.headers | ||
| << " body: " << request.body; | ||
| if (agent_.config_.VerboseLogging()) { | ||
| LOG(INFO) << "Handler called: " << request.method | ||
| << " " << request.destination | ||
| << " headers: " << request.headers | ||
| << " body: " << request.body; | ||
| } | ||
| if (request.method == "GET" && request.destination.find(kPrefix) == 0) { | ||
| std::string id = request.destination.substr(kPrefix.size()); | ||
| const auto result = agent_.resource_map_.find(id); | ||
| if (result == agent_.resource_map_.end()) { | ||
| // TODO: This could be considered log spam. | ||
| // 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()) { | ||
| LOG(WARNING) << "No matching resource for " << id; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Logging this once isn't something that's easy to do with the current logging framework. |
||
| } | ||
| response = HttpServer::response::stock_reply( | ||
| HttpServer::response::not_found, ""); | ||
| } else { | ||
| const MonitoredResource& resource = result->second; | ||
| LOG(INFO) << "Found resource for " << id << ": " << resource; | ||
| if (agent_.config_.VerboseLogging()) { | ||
| LOG(INFO) << "Found resource for " << id << ": " << resource; | ||
| } | ||
| response = HttpServer::response::stock_reply( | ||
| HttpServer::response::ok, resource.ToJSON()->ToString()); | ||
| } | ||
|
|
@@ -176,9 +182,13 @@ void MetadataReporter::ReportMetadata() { | |
| std::this_thread::sleep_for(std::chrono::seconds(3)); | ||
| // TODO: Do we need to be able to stop this? | ||
| while (true) { | ||
| LOG(INFO) << "Sending metadata request to server"; | ||
| if (agent_.config_.VerboseLogging()) { | ||
| LOG(INFO) << "Sending metadata request to server"; | ||
| } | ||
| SendMetadata(agent_.GetMetadataMap()); | ||
| LOG(INFO) << "Metadata request sent successfully"; | ||
| if (agent_.config_.VerboseLogging()) { | ||
| LOG(INFO) << "Metadata request sent successfully"; | ||
| } | ||
| std::this_thread::sleep_for(period_); | ||
| } | ||
| //LOG(INFO) << "Metadata reporter exiting"; | ||
|
|
@@ -188,13 +198,16 @@ namespace { | |
|
|
||
| void SendMetadataRequest(std::vector<json::value>&& entries, | ||
| const std::string& endpoint, | ||
| const std::string& auth_header) { | ||
| const std::string& auth_header, | ||
| bool verbose_logging) { | ||
| json::value update_metadata_request = json::object({ | ||
| {"entries", json::array(std::move(entries))}, | ||
| }); | ||
|
|
||
| LOG(INFO) << "About to send request: POST " << endpoint | ||
| << " " << *update_metadata_request; | ||
| if (verbose_logging) { | ||
| LOG(INFO) << "About to send request: POST " << endpoint | ||
| << " " << *update_metadata_request; | ||
| } | ||
|
|
||
| http::client client; | ||
| http::client::request request(endpoint); | ||
|
|
@@ -205,7 +218,9 @@ void SendMetadataRequest(std::vector<json::value>&& entries, | |
| request << boost::network::header("Authorization", auth_header); | ||
| request << boost::network::body(request_body); | ||
| http::client::response response = client.post(request); | ||
| LOG(INFO) << "Server responded with " << body(response); | ||
| if (verbose_logging) { | ||
| LOG(INFO) << "Server responded with " << body(response); | ||
| } | ||
| // TODO: process response. | ||
| } | ||
|
|
||
|
|
@@ -214,11 +229,15 @@ void SendMetadataRequest(std::vector<json::value>&& entries, | |
| void MetadataReporter::SendMetadata( | ||
| std::map<MonitoredResource, MetadataAgent::Metadata>&& metadata) { | ||
| if (metadata.empty()) { | ||
| LOG(INFO) << "No data to send"; | ||
| if (agent_.config_.VerboseLogging()) { | ||
| LOG(INFO) << "No data to send"; | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| LOG(INFO) << "Sending request to the server"; | ||
| if (agent_.config_.VerboseLogging()) { | ||
| LOG(INFO) << "Sending request to the server"; | ||
| } | ||
| const std::string project_id = environment_.NumericProjectId(); | ||
| // The endpoint template is expected to be of the form | ||
| // "https://stackdriver.googleapis.com/.../projects/{{project_id}}/...". | ||
|
|
@@ -261,15 +280,17 @@ void MetadataReporter::SendMetadata( | |
| continue; | ||
| } | ||
| if (total_size + size > limit_bytes) { | ||
| SendMetadataRequest(std::move(entries), endpoint, auth_header); | ||
| SendMetadataRequest(std::move(entries), endpoint, auth_header, | ||
| agent_.config_.VerboseLogging()); | ||
| entries.clear(); | ||
| total_size = empty_size; | ||
| } | ||
| entries.emplace_back(std::move(metadata_entry)); | ||
| total_size += size; | ||
| } | ||
| if (!entries.empty()) { | ||
| SendMetadataRequest(std::move(entries), endpoint, auth_header); | ||
| SendMetadataRequest(std::move(entries), endpoint, auth_header, | ||
| agent_.config_.VerboseLogging()); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -285,17 +306,22 @@ void MetadataAgent::UpdateResource(const std::vector<std::string>& resource_ids, | |
| // TODO: How do we handle deleted resources? | ||
| // TODO: Do we care if the value was already there? | ||
| for (const std::string& id : resource_ids) { | ||
| LOG(INFO) << "Updating resource map '" << id << "'->" << resource; | ||
| if (config_.VerboseLogging()) { | ||
| LOG(INFO) << "Updating resource map '" << id << "'->" << resource; | ||
| } | ||
| resource_map_.emplace(id, resource); | ||
| } | ||
| LOG(INFO) << "Updating metadata map " << resource << "->{" | ||
| << "version: " << entry.version << ", " | ||
| << "is_deleted: " << entry.is_deleted << ", " | ||
| << "created_at: " << rfc3339::ToString(entry.created_at) << ", " | ||
| << "collected_at: " << rfc3339::ToString(entry.collected_at) << ", " | ||
| << "metadata: " << *entry.metadata << ", " | ||
| << "ignore: " << entry.ignore | ||
| << "}"; | ||
| if (config_.VerboseLogging()) { | ||
| LOG(INFO) << "Updating metadata map " << resource << "->{" | ||
| << "version: " << entry.version << ", " | ||
| << "is_deleted: " << entry.is_deleted << ", " | ||
| << "created_at: " << rfc3339::ToString(entry.created_at) << ", " | ||
| << "collected_at: " << rfc3339::ToString(entry.collected_at) | ||
| << ", " | ||
| << "metadata: " << *entry.metadata << ", " | ||
| << "ignore: " << entry.ignore | ||
| << "}"; | ||
| } | ||
| // Force value update. The repeated search is inefficient, but shouldn't | ||
| // be a huge deal. | ||
| metadata_map_.erase(resource); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,13 +16,16 @@ | |
|
|
||
| #include "configuration.h" | ||
|
|
||
| #include <boost/program_options.hpp> | ||
| #include <iostream> | ||
| #include <map> | ||
|
|
||
| #include <yaml-cpp/yaml.h> | ||
|
|
||
| namespace google { | ||
|
|
||
| namespace { | ||
| constexpr const char kConfigFileFlag[] = "config-file"; | ||
|
|
||
| constexpr const char kDefaultProjectId[] = ""; | ||
| constexpr const char kDefaultCredentialsFile[] = ""; | ||
|
|
@@ -51,6 +54,7 @@ constexpr const char kDefaultInstanceZone[] = ""; | |
| MetadataAgentConfiguration::MetadataAgentConfiguration() | ||
| : project_id_(kDefaultProjectId), | ||
| credentials_file_(kDefaultCredentialsFile), | ||
| verbose_logging_(false), | ||
| metadata_api_num_threads_(kMetadataApiDefaultNumThreads), | ||
| metadata_api_port_(kMetadataApiDefaultPort), | ||
| metadata_reporter_interval_seconds_( | ||
|
|
@@ -71,9 +75,41 @@ MetadataAgentConfiguration::MetadataAgentConfiguration() | |
| instance_id_(kDefaultInstanceId), | ||
| instance_zone_(kDefaultInstanceZone) {} | ||
|
|
||
| MetadataAgentConfiguration::MetadataAgentConfiguration( | ||
| const std::string& filename) : MetadataAgentConfiguration() | ||
| { | ||
| int MetadataAgentConfiguration::ParseArguments(int ac, char** av) { | ||
| std::string config_file; | ||
| boost::program_options::options_description flags_desc; | ||
| flags_desc.add_options() | ||
| ("help,h", "Print help message") | ||
| ("verbose,v", boost::program_options::bool_switch(&verbose_logging_), | ||
| "Enable verbose logging") | ||
| ; | ||
| boost::program_options::options_description hidden_desc; | ||
| hidden_desc.add_options() | ||
| (kConfigFileFlag, | ||
| boost::program_options::value<std::string>(&config_file) | ||
| ->default_value(""), | ||
| "Configuration file location") | ||
| ; | ||
| boost::program_options::options_description all_desc; | ||
| all_desc.add(flags_desc).add(hidden_desc); | ||
| boost::program_options::positional_options_description positional_desc; | ||
| positional_desc.add(kConfigFileFlag, 1); | ||
| boost::program_options::variables_map flags; | ||
| boost::program_options::store( | ||
| boost::program_options::command_line_parser(ac, av) | ||
| .options(all_desc).positional(positional_desc).run(), flags); | ||
| boost::program_options::notify(flags); | ||
|
|
||
| if (flags.count("help")) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this work if -h is passed in instead of --help?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it will. |
||
| std::cout << flags_desc << std::endl; | ||
| return 1; | ||
| } | ||
|
|
||
| ParseConfigFile(config_file); | ||
| return 0; | ||
| } | ||
|
|
||
| void MetadataAgentConfiguration::ParseConfigFile(const std::string& filename) { | ||
| std::lock_guard<std::mutex> lock(mutex_); | ||
| if (filename.empty()) return; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the only way to do it? Seems like
ifconditionals to guard logging defeats the purpose of using log levels likeINFOandDEBUG. Also elsewhere.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Alright. Deferring for now.