Skip to content
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.

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

add map between severity and overdue minutes #1144

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
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
39 changes: 35 additions & 4 deletions src/logging.cc
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
#include <tuple>
#include <type_traits>
#include <utility>
#include <unordered_map>

#include "config.h"
#include "glog/platform.h"
Expand Down Expand Up @@ -337,6 +338,16 @@ const char* GetLogSeverityName(LogSeverity severity) {
return LogSeverityNames[severity];
}

LogSeverity FindFilepathLogSeverity(const std::string& filepath) {
for (int i = GLOG_INFO; i < NUM_SEVERITIES; i++) {
if (filepath.find(GetLogSeverityName(static_cast<LogSeverity>(i))) != std::string::npos) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (filepath.find(GetLogSeverityName(static_cast<LogSeverity>(i))) != std::string::npos) {
if (filepath.rfind(GetLogSeverityName(static_cast<LogSeverity>(i))) != std::string::npos) {

Search from right since the severity is used as a suffix.

return static_cast<LogSeverity>(i);
}
}
// assume that the file has the INFO severity
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do not assume anything and simply return a not found sentinel.

return GLOG_INFO;
}

static bool SendEmailInternal(const char* dest, const char* subject,
const char* body, bool use_logging);

Expand Down Expand Up @@ -437,6 +448,9 @@ class LogCleaner {

// Setting overdue to 0 days will delete all logs.
void Enable(const std::chrono::minutes& overdue);

void Enable(std::unordered_map<LogSeverity, std::chrono::minutes>& overdue_per_severity_);

void Disable();

void Run(const std::chrono::system_clock::time_point& current_time,
Expand All @@ -460,8 +474,7 @@ class LogCleaner {
const std::chrono::system_clock::time_point& current_time) const;

bool enabled_{false};
std::chrono::minutes overdue_{
std::chrono::duration<int, std::ratio<kSecondsInWeek>>{1}};
std::unordered_map<LogSeverity, std::chrono::minutes> overdue_per_severity_;
std::chrono::system_clock::time_point
next_cleanup_time_; // cycle count at which to clean overdue log
};
Expand Down Expand Up @@ -1286,7 +1299,14 @@ LogCleaner::LogCleaner() = default;

void LogCleaner::Enable(const std::chrono::minutes& overdue) {
enabled_ = true;
overdue_ = overdue;
for (int i = GLOG_INFO; i < NUM_SEVERITIES; i++) {
overdue_per_severity_[static_cast<LogSeverity>(i)] = overdue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fill the map only with the severity set by the user.

}
}

void LogCleaner::Enable(std::unordered_map<LogSeverity, std::chrono::minutes>& overdue_per_severity) {
enabled_ = true;
overdue_per_severity_ = overdue_per_severity;
}

void LogCleaner::Disable() { enabled_ = false; }
Expand Down Expand Up @@ -1471,7 +1491,14 @@ bool LogCleaner::IsLogLastModifiedOver(
const auto last_modified_time =
std::chrono::system_clock::from_time_t(file_stat.st_mtime);
const auto diff = current_time - last_modified_time;
return diff >= overdue_;

LogSeverity severity = FindFilepathLogSeverity(filepath);
auto overdue_it = overdue_per_severity_.find(severity);
// if there is no overdue for this severity, do not delete it
if(overdue_it == overdue_per_severity_.end()){
return false;
}
return diff >= overdue_it->second;
}

// If failed to get file stat, don't return true!
Expand Down Expand Up @@ -2627,6 +2654,10 @@ void EnableLogCleaner(const std::chrono::minutes& overdue) {
log_cleaner.Enable(overdue);
}

void EnableLogCleaner(std::unordered_map<LogSeverity, std::chrono::minutes>& overdue_per_severity) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Exposing the map as a parameter makes the calls verbose and the API usage unnecessarily complicated. Instead use the following signature:

Suggested change
void EnableLogCleaner(std::unordered_map<LogSeverity, std::chrono::minutes>& overdue_per_severity) {
void EnableLogCleaner(LogSeverity severity, const std::chrono::minutes& overdue) {

log_cleaner.Enable(overdue_per_severity);
}

void DisableLogCleaner() { log_cleaner.Disable(); }
Copy link
Collaborator

Choose a reason for hiding this comment

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

For symmetry, there should be an API call for disabling the log cleaner for specific severity.


LogMessageTime::LogMessageTime() = default;
Expand Down