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

OverrideLogCleanerFilePathPrefix to override the files the log cleane… #1083

Closed
wants to merge 2 commits into from

Conversation

mturnock
Copy link

…r removes

Copy link

google-cla bot commented Mar 11, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@mturnock
Copy link
Author

mturnock commented Mar 11, 2024

My requirement was that I delete old log files, regardless of log level.
I was looking to achieve something similar to #1062, but I don't think that approach is very easy. So instead I extended the interface to override the files the log cleaner cleans, giving flexibility to users of the log cleaner on what they want cleaning.

I would be keen to hear any feedback.

@mturnock mturnock force-pushed the master branch 3 times, most recently from 05a994b to 10bb9cd Compare March 11, 2024 23:36
Add OverrideLogCleanerFilePathPrefix to override the files the log cleaner removes
@sergiud
Copy link
Collaborator

sergiud commented Mar 12, 2024

Thanks for the PR!

However, I'm not sure I understand why we need to extend the public API instead of fixing the underlying issue?

@mturnock
Copy link
Author

Thanks for the PR!

However, I'm not sure I understand why we need to extended the public API instead of fixing the underlying issue?

I don't think I have come up with a better alternative. I'm open to suggestions.

Some possible thoughts:

  1. Make next_cleanup_time_ a map, 1 entry per base_filename. Pro: no change to interface. Con: only deletes old log of a particular level when a new log line of that level is made.
  2. Add a registration system to log cleaner from each LogDestination. Con: seemed a bit gnarly.

@mturnock
Copy link
Author

The suggestion from #1062 is that we can fix IsLogFromCurrentProject, but I don't know if that is possible in general because the user can set any log name style they want with SetLogDestination

@sergiud
Copy link
Collaborator

sergiud commented Mar 12, 2024

I would prefer no additional methods if possible since using log cleaner will become less intuitive than it already is.

While the log destination can be different the suffix is always the same. Combined with the observation that log severities are known at compile time determining candidates for deletion should be doable.

@mturnock
Copy link
Author

Consider:

SetLogDestination(GLOG_INFO, "/path/to/logs/applog-carrots-");
SetLogDestination(GLOG_WARNING, "/path/to/logs/applog-peas-");
SetLogDestination(GLOG_ERROR, "/path/to/logs/applog-corn-");
SetLogFilenameExtension(".log");
EnableLogCleaner(std::chrono::days(7));

If we had a registration system, then that would work, but trying to fix IsLogFromCurrentProject seems futile.

@sergiud
Copy link
Collaborator

sergiud commented Mar 12, 2024

As I see it, an internal registration system is still a better solution than shifting the burden of managing log file paths onto users.

@mturnock mturnock closed this Mar 12, 2024
@mturnock
Copy link
Author

OK, thanks. I've closed this one, and opened a new one to do the registration system approach. #1086

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.

2 participants