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

Fixed IsLogFromCurrentProject to include all severities #1062

Closed
wants to merge 1 commit into from

Conversation

Mivekk
Copy link

@Mivekk Mivekk commented Jan 15, 2024

Previously when executing following code LogCleaner would only delete overdue Error log files:

google::EnableLogCleaner(1min);

LOG(ERROR) << "Error hello";
LOG(INFO) << "Info hello";
LOG(WARNING) << "Warning hello";

And if we changed the order to for example:

LOG(INFO) << "Info hello";
LOG(ERROR) << "Error hello";
LOG(WARNING) << "Warning hello";

LogCleaner would only delete overdue Info log files and overdue Error and Warning files were ignored.
To include all severities during LogCleaner's sweep I tested filepath for all severities in a IsLogFromCurrentProject function.

Copy link
Collaborator

@sergiud sergiud left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I have a couple of remarks.

There seem to be build failures caused by the changes.

Please also update the cleaner test cases by adding the corresponding log output.

src/logging.cc Outdated
Comment on lines 1426 to 1428
cleaned_base_filename.pop_back();
int dot_pos = cleaned_base_filename.find_last_of('.');
cleaned_base_filename.erase(dot_pos + 1, 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.

You do actually need to modify the string. Something like this should be sufficient:

Suggested change
cleaned_base_filename.pop_back();
int dot_pos = cleaned_base_filename.find_last_of('.');
cleaned_base_filename.erase(dot_pos + 1, std::string::npos);
const string::size_type dot_pos = cleaned_base_filename.find_last_of('.', cleaned_base_filename.size() - 1);

src/logging.cc Outdated
Comment on lines 1432 to 1434
bool found_file = false;
for (const auto& severity : LogSeverityNames) {
string cleaned_base_filename_with_severity = cleaned_base_filename + severity;
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
bool found_file = false;
for (const auto& severity : LogSeverityNames) {
string cleaned_base_filename_with_severity = cleaned_base_filename + severity;
```suggestion
const string base_filename = cleaned_base_filename.substring(0, dot_pos);
bool found_file = false;
for (const auto& severity : LogSeverityNames) {
string cleaned_base_filename_with_severity = base_filename + severity;

@Mivekk Mivekk force-pushed the fix-is-log-from-current branch 2 times, most recently from 692680d to 4e016e5 Compare January 16, 2024 09:14
@Mivekk
Copy link
Author

Mivekk commented Jan 16, 2024

I'm not sure how I should modify cleaner test cases unfortunately

@sergiud
Copy link
Collaborator

sergiud commented Jan 16, 2024

You can extend any of the cleanup_*_unittest.cc, for instance, here

for (unsigned i = 0; i < 1000; ++i) {
LOG(INFO) << "cleanup test";
}

by additionally logging at the missing severities.

Please note that the unit tests need to be updated either way because these are failing now with your changes.

@Mivekk Mivekk force-pushed the fix-is-log-from-current branch from 4e016e5 to 6ea4aea Compare January 16, 2024 10:01
@Mivekk
Copy link
Author

Mivekk commented Jan 16, 2024

Updated unit test and all checks should pass now hopefully

@sergiud
Copy link
Collaborator

sergiud commented Jan 16, 2024

Unfortunately, the cleanup tests are still failing.

@Mivekk Mivekk force-pushed the fix-is-log-from-current branch from 6ea4aea to 71a04a1 Compare January 31, 2024 11:40
@Mivekk
Copy link
Author

Mivekk commented Jan 31, 2024

Fixed an issue different way

@sergiud
Copy link
Collaborator

sergiud commented Feb 1, 2024

The unit tests are still failing though.

@sergiud
Copy link
Collaborator

sergiud commented Jun 11, 2024

Closing as abandoned. Please let me know if you want to continue working on this PR.

@sergiud sergiud closed this Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants