Bug 61105

Summary: Roll log files by default
Product: Tomcat 9 Reporter: Mark Thomas <markt>
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: enhancement    
Priority: P2    
Version: unspecified   
Target Milestone: -----   
Hardware: All   
OS: All   

Description Mark Thomas 2017-05-18 19:09:05 UTC
From a discussion at TomcatCon, it would be a better default if Tomcat rolled log files by default to avoid filling disks. We probably need to err on the side of caution regarding how long to keep the files for.
Comment 1 Violeta Georgieva 2017-06-05 19:41:12 UTC
Hi,

Currently the log files (except catalina.out which is available on OSes != Windows) are rotated by default based on the date.
We can introduce in addition a configuration for the file size and also how many files to keep. Do we want to archive files when rotate them or just a simple renaming is enough?

What about catalina.out file? Do we want a rotation for this file also? There is already enhancement about something similar https://bz.apache.org/bugzilla/show_bug.cgi?id=53930

What about the log files created by the Tomcat service on Windows? Do we want to have something similar for them also?

Thanks,
Violeta
Comment 2 Mark Thomas 2017-06-05 19:44:56 UTC
The conversation at TomcatCon was around putting a (relatively large) limit on the number of files that are kept by default. Picking a number of of thin air, how does 90 days sound?

There was no concern expressed about the log files that are currently not rolled (generally, I suspect, because well written apps won;t trigger content to those files).
Comment 3 Violeta Georgieva 2017-06-06 11:01:13 UTC
Hi,

What do you think about this approach?
https://github.com/apache/tomcat/pull/60

Thanks,
Violeta
Comment 4 Violeta Georgieva 2017-06-06 18:31:13 UTC
Hi,

In the PR [1] there is a proposal for merging the proposed functionality with https://github.com/apache/tomee/blob/master/tomee/tomee-juli/src/main/java/org/apache/tomee/jul/handler/rotating/LocalFileHandler.java

I checked the TomEE's LocalFileHandler and it provides many useful features.

If there is a demand I can port it to Tomcat.

Regards,
Violeta

[1] https://github.com/apache/tomcat/pull/60
Comment 5 Konstantin Kolinko 2017-06-06 22:53:45 UTC
(In reply to Violeta Georgieva from comment #3)
> Hi,
> 
> What do you think about this approach?
> https://github.com/apache/tomcat/pull/60
> 

+    public static final int DEFAULT_MAX_DAYS = 90;
+    private int maxDays = DEFAULT_MAX_DAYS;

I do not like the idea of built-in default limit in java code.

I am open to discuss whether it is feasible for Tomcat 9,
but such built-in limit cannot be backported to stable versions (8.5 and earlier).

I think it is better to add limits explicitly to the default logging.properties configuration.


+        String sMaxDays = getProperty(className + ".maxDays", String.valueOf(DEFAULT_MAX_DAYS));
+        if (maxDays <= 0) {
+            try {
+                maxDays = Integer.parseInt(sMaxDays);
+            } catch (NumberFormatException ignore) {
+                // no-op
+            }
+        }

I think the above try/catch block is never executed, as "if (maxDays <= 0)" is always false, as maxDays is "90" by default.

+    private DirectoryStream<Path> streamFilesForDelete() throws IOException {
+        FileTime maxDaysOffset = FileTime.from(Instant.now().minus(maxDays, ChronoUnit.DAYS));
+        return Files.newDirectoryStream(new File(directory).toPath(), path -> {
+            String fileName = path.getFileName().toString();
+            return fileName.startsWith(prefix) && fileName.endsWith(suffix)
+                    && Files.getLastModifiedTime(path).compareTo(maxDaysOffset) < 0;
+        });
+    }

I do not like the above.

1. "fileName.startsWith(prefix)" will result in false positives.

2. I'd prefer to test the date in the file name, not file modification date.


BTW, for access logs I usually have an empty prefix, grouping the files into separate directories by month:
fileDateFormat="yyyy-MM'/webappname.'yyyy-MM-dd"
prefix=""
suffix=".access.log"

Such feature is not implemented for JULI logging yet. If it were, the "fileName.startsWith(prefix)" here would be true for every file.
Comment 6 Huxing Zhang 2017-06-09 01:49:53 UTC
Hi,

> There was no concern expressed about the log files that are currently not
> rolled (generally, I suspect, because well written apps won;t trigger content
> to those files).

We do have concerns about rotate the output to stdout/stderr.
In most of our cases, this is due to logging framework conflict between log4j and logback in a web application.
The default behavior is that all the logging content are eventually gone to catalina.out.
Most of the users even won't be aware of it, until being alerted by running out of the disk space (The web application may run for months).

To avoid this, we actually have implemented a feature in Tomcat to rotate catalina.out on a daily basis.
Under the hood we use a customized PrintStream to replace System.out/System.err, capture the content, and output to JULI.
Since it is rotated by day, it make us easier to keep the latest N files.

I know the best solution will be solving the conflict, but according to our experience, most of the user don't know there is a conflict.

In there any interest in adding this feature to Tomcat?
Comment 7 Violeta Georgieva 2017-06-09 20:32:48 UTC
Hi,

(In reply to Konstantin Kolinko from comment #5)
> (In reply to Violeta Georgieva from comment #3)
> > Hi,
> > 
> > What do you think about this approach?
> > https://github.com/apache/tomcat/pull/60
> > 
> 
> +    public static final int DEFAULT_MAX_DAYS = 90;
> +    private int maxDays = DEFAULT_MAX_DAYS;
> 
> I do not like the idea of built-in default limit in java code.
> 
> I am open to discuss whether it is feasible for Tomcat 9,
> but such built-in limit cannot be backported to stable versions (8.5 and
> earlier).
> 
> I think it is better to add limits explicitly to the default
> logging.properties configuration.
> 
> 
> +        String sMaxDays = getProperty(className + ".maxDays",
> String.valueOf(DEFAULT_MAX_DAYS));
> +        if (maxDays <= 0) {
> +            try {
> +                maxDays = Integer.parseInt(sMaxDays);
> +            } catch (NumberFormatException ignore) {
> +                // no-op
> +            }
> +        }
> 
> I think the above try/catch block is never executed, as "if (maxDays <= 0)"
> is always false, as maxDays is "90" by default.
> 
> +    private DirectoryStream<Path> streamFilesForDelete() throws IOException
> {
> +        FileTime maxDaysOffset = FileTime.from(Instant.now().minus(maxDays,
> ChronoUnit.DAYS));
> +        return Files.newDirectoryStream(new File(directory).toPath(), path
> -> {
> +            String fileName = path.getFileName().toString();
> +            return fileName.startsWith(prefix) && fileName.endsWith(suffix)
> +                    &&
> Files.getLastModifiedTime(path).compareTo(maxDaysOffset) < 0;
> +        });
> +    }
> 
> I do not like the above.
> 
> 1. "fileName.startsWith(prefix)" will result in false positives.
> 
> 2. I'd prefer to test the date in the file name, not file modification date.
> 
> 
> BTW, for access logs I usually have an empty prefix, grouping the files into
> separate directories by month:
> fileDateFormat="yyyy-MM'/webappname.'yyyy-MM-dd"
> prefix=""
> suffix=".access.log"
> 
> Such feature is not implemented for JULI logging yet. If it were, the
> "fileName.startsWith(prefix)" here would be true for every file.

Thanks for the review. I prepared a new patch where I applied all your recommendations. Can you take a look at it?

Violeta
Comment 8 Violeta Georgieva 2017-06-16 11:10:17 UTC
Any comments?
Comment 9 Konstantin Kolinko 2017-06-16 14:38:51 UTC
(In reply to Violeta Georgieva from comment #8)
> Any comments?

Generally: I like it.

1. Typo in method name: obtainDateFormPath  s/Form/From/

2. Building a pattern,

> pattern = Pattern.compile("^(" + prefix + ")\\d{4}-\\d{1,2}-\\d{1,2}(" + suffix + ")$");

This should use  (Pattern.quote(prefix) + "..." + Pattern.quote(suffix))

Prefix and suffix can contain special characters, e.g. '.' = any character. Wrapping them with Pattern.quote() solves this issue.

3. Temporary directory handling in unit test

There is a base Test class that provides support for temporary directories,

https://github.com/apache/tomcat/blob/trunk/test/org/apache/catalina/startup/LoggingBaseTest.java

Differences:
- It respects system property "tomcat.test.temp"
- It uses creates a random directory for the test, to allow running several tests in parallel
tempDir = Files.createTempDirectory(tempBasePath, "test").toFile();

Maybe it is not a good idea to use LoggingBaseTest directly as a base class,
as it initializes logging and this test tests logging, but it can be used to copy some code.
Comment 10 Violeta Georgieva 2017-06-16 19:29:35 UTC
(In reply to Konstantin Kolinko from comment #9)
> (In reply to Violeta Georgieva from comment #8)
> > Any comments?
> 
> Generally: I like it.
> 
> 1. Typo in method name: obtainDateFormPath  s/Form/From/

Fixed

> 2. Building a pattern,
> 
> > pattern = Pattern.compile("^(" + prefix + ")\\d{4}-\\d{1,2}-\\d{1,2}(" + suffix + ")$");
> 
> This should use  (Pattern.quote(prefix) + "..." + Pattern.quote(suffix))
> 
> Prefix and suffix can contain special characters, e.g. '.' = any character.
> Wrapping them with Pattern.quote() solves this issue.
>

Fixed, added a test also
 
> 3. Temporary directory handling in unit test
> 
> There is a base Test class that provides support for temporary directories,
> 
> https://github.com/apache/tomcat/blob/trunk/test/org/apache/catalina/startup/
> LoggingBaseTest.java
> 
> Differences:
> - It respects system property "tomcat.test.temp"
> - It uses creates a random directory for the test, to allow running several
> tests in parallel
> tempDir = Files.createTempDirectory(tempBasePath, "test").toFile();
> 
> Maybe it is not a good idea to use LoggingBaseTest directly as a base class,
> as it initializes logging and this test tests logging, but it can be used to
> copy some code.

Fixed

Thanks for the review,
Violeta
Comment 11 Violeta Georgieva 2017-06-16 19:34:24 UTC
The change is applied and will be available in 9.0.0.M22 onwards.
The log files will be kept by default 90 days.

Do we want this change in the previous versions? May be with different default value?

Thanks,
Violeta
Comment 12 Mark Thomas 2017-06-17 09:30:05 UTC
Happy to see it back-ported. I'd disable by default though so there is no change of behaviour in stable versions.
Comment 13 Matt Farwell 2017-06-17 17:08:00 UTC
It would great to see this functionality backported.  Bringing it into Tomcat 8.5 and set it as disabled by default makes sense to me.
Comment 14 Violeta Georgieva 2017-06-19 10:25:24 UTC
Hi,

The new functionality was back ported to:
- 8.5.x for 8.5.16 onwards
- 8.0.x for 8.0.45 onwards
- 7.0.x for 7.0.79 onwards

The default will be: keep the log files forever.

Regards,
Violeta