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 7 commits into
base: master
Choose a base branch
from

Conversation

HNOONa-0
Copy link
Contributor

I have added an unordered_map that can store a severity and it's corresponding overdue time to the LogCleaner object. The modifications should be backwards compatible. If this idea is correct, we can preform some optimizations, like keeping a cache of filepaths whose severities have been computed.

Fixes: #882

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 and apologies for the late reply.

The changes are going into the right direction. Please add some unit tests. For this, you can extend the existing log cleaner tests.

src/logging.cc Outdated
@@ -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.

@HNOONa-0 HNOONa-0 force-pushed the feat/overdue-per-severity branch from cb1b4aa to 55e6f47 Compare February 21, 2025 11:31
@HNOONa-0
Copy link
Contributor Author

Hey @sergiud , I have made some improvements, I switched from a map to a vector for better lookup speed and cleaner implementation. I also addressed all the issues you pointed out. PTAL!

@codecov-commenter
Copy link

codecov-commenter commented Feb 21, 2025

Codecov Report

Attention: Patch coverage is 70.58824% with 10 lines in your changes missing coverage. Please review.

Project coverage is 61.95%. Comparing base (6c5c692) to head (314903e).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
src/logging.cc 70.58% 9 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1144      +/-   ##
==========================================
- Coverage   62.22%   61.95%   -0.27%     
==========================================
  Files          19       19              
  Lines        2324     2363      +39     
  Branches      845      856      +11     
==========================================
+ Hits         1446     1464      +18     
- Misses        596      616      +20     
- Partials      282      283       +1     
Files with missing lines Coverage Δ
src/glog/logging.h 83.60% <ø> (ø)
src/logging.cc 68.63% <70.58%> (-0.58%) ⬇️

... and 1 file with indirect coverage changes

@HNOONa-0 HNOONa-0 force-pushed the feat/overdue-per-severity branch from f0e5b47 to 314903e Compare February 22, 2025 17:25
@HNOONa-0
Copy link
Contributor Author

Hey @sergiud, I initially made the wrong assumptions, I thought log files must have severity in their names, but that’s not the case. I have added methods to enable, disable logging for both types of files (with and without severity in their names) and exported those functions. I also added a test file that checks for deferred cleanup logic, but I haven’t integrated it with CMake yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set different overduedays for different levels of logs
3 participants