[Bug 61105] New: Roll log files by default

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

[Bug 61105] New: Roll log files by default

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61105

            Bug ID: 61105
           Summary: Roll log files by default
           Product: Tomcat 9
           Version: unspecified
          Hardware: All
                OS: All
            Status: NEW
          Severity: enhancement
          Priority: P2
         Component: Catalina
          Assignee: [hidden email]
          Reporter: [hidden email]
  Target Milestone: -----

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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61105] Roll log files by default

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61105

--- Comment #1 from Violeta Georgieva <[hidden email]> ---
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

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61105] Roll log files by default

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61105

--- Comment #2 from Mark Thomas <[hidden email]> ---
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).

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61105] Roll log files by default

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61105

--- Comment #3 from Violeta Georgieva <[hidden email]> ---
Hi,

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

Thanks,
Violeta

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61105] Roll log files by default

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61105

--- Comment #4 from Violeta Georgieva <[hidden email]> ---
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

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61105] Roll log files by default

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61105

--- Comment #5 from Konstantin Kolinko <[hidden email]> ---
(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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [Bug 61105] Roll log files by default

Huxing Zhang
In reply to this post by Bugzilla from bugzilla@apache.org
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?

------------------------------------------------------------------
From:bugzilla <[hidden email]>
Time:2017 Jun 6 (Tue) 03:45
To:dev <[hidden email]>
Subject:[Bug 61105] Roll log files by default


https://bz.apache.org/bugzilla/show_bug.cgi?id=61105

--- Comment #2 from Mark Thomas <[hidden email]> ---
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).

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61105] Roll log files by default

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61105

--- Comment #6 from Huxing Zhang <[hidden email]> ---
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?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61105] Roll log files by default

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61105

--- Comment #7 from Violeta Georgieva <[hidden email]> ---
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

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61105] Roll log files by default

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61105

--- Comment #8 from Violeta Georgieva <[hidden email]> ---
Any comments?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61105] Roll log files by default

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61105

--- Comment #9 from Konstantin Kolinko <[hidden email]> ---
(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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61105] Roll log files by default

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61105

--- Comment #10 from Violeta Georgieva <[hidden email]> ---
(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

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61105] Roll log files by default

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61105

--- Comment #11 from Violeta Georgieva <[hidden email]> ---
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

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61105] Roll log files by default

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61105

--- Comment #12 from Mark Thomas <[hidden email]> ---
Happy to see it back-ported. I'd disable by default though so there is no
change of behaviour in stable versions.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61105] Roll log files by default

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61105

--- Comment #13 from Matt Farwell <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61105] Roll log files by default

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61105

Violeta Georgieva <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #14 from Violeta Georgieva <[hidden email]> ---
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

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]