google / glog

C++ implementation of the Google logging module
http://google.github.io/glog/
BSD 3-Clause "New" or "Revised" License
7.01k stars 2.05k forks source link

OverrideLogCleanerFilePathPrefix to override the files the log cleane… #1083

Closed mturnock closed 6 months ago

mturnock commented 6 months ago

…r removes

google-cla[bot] commented 6 months ago

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 commented 6 months ago

My requirement was that I delete old log files, regardless of log level. I was looking to achieve something similar to https://github.com/google/glog/pull/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.

sergiud commented 6 months ago

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 commented 6 months ago

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_cleanuptime 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 commented 6 months ago

The suggestion from https://github.com/google/glog/pull/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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

OK, thanks. I've closed this one, and opened a new one to do the registration system approach. https://github.com/google/glog/pull/1086