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 purge of old logs file #576

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

PierreF
Copy link
Contributor

@PierreF PierreF commented Jun 19, 2017

It seems that currently nothing purge old logs file. Which means that after a while a lots of files may be present in /var/log/jmxtrans and consume disk space.

I'm not sure logs older that few weeks are still useful.

The PR add a daily cron script that remove all archived logs older than 32 days.

@gehel
Copy link
Member

gehel commented Jun 19, 2017

Thanks for the PR! For log rotation, I'd much prefer have this managed by the logging framework (log4j / logback, yes, there is a mess there). That's one less system dependency...

@PierreF
Copy link
Contributor Author

PierreF commented Jun 19, 2017

I'm not a Java developer, so I've a very little knowledge of log4j 1.x/log4j 2.x/logback and co.
But I initially tried to look at handling this on logging framework (currently log4j 1.x) and it seems that it does only support it with size based rotation.
With a time based rotation logback seems to support it. And (I'm not sure about this one) log4j 2.x.

So for me, if we want to support this in logging framework, we need to add logback and/or upgrade to log4j 2.x.

Should I try to look at upgrading log4j or add logback ?

@gehel
Copy link
Member

gehel commented Jun 19, 2017

Since you seem to be managing Java application, some knowledge of the standard logging framework will probably help you in the long run :) (sorry I could not resist).

The situation is a bit complex... We have a mess of both logback and log4j. The way to go is definitely logback, but there are some OutputWriters that depend directly on log4j, which makes removing it (or upgrading it) a giant pain. Not that there is a time based rolling policy in log4j-extra, which is already bundled with jmxtrans. This could be a short term approach.

@PierreF
Copy link
Contributor Author

PierreF commented Jun 22, 2017

From my search the logger that support deletion of history are:

For the last solution, I don't see how to do it. jmxtrans-output-log4j use SLF4F and its log4j12 binding. From SLF4J documentation we should "drop one and only one binding [...] onto the [...] class path". Therefor I can't add logback binding without providing two binding.

The second solution means dropping usage of SLF4J (or we have the same problem as described above). It should be much more than just changing all logger creation. It also means that SLF4J + log4j 1.2 will still be used but jmxtrans-output-log4j. But since log4j 2 use another package namespace (org.apache.logging.log4j vs org.apache.log4j it should be conflict).

The first solution is just configuration change. But it means that rotation will be size based and no longer time based.

@gehel
Copy link
Member

gehel commented Jun 22, 2017

Another solution would be to use https://logging.apache.org/log4j/extras/apidocs/org/apache/log4j/rolling/TimeBasedRollingPolicy.html with log4j. The class is already on the classpath.

@PierreF
Copy link
Contributor Author

PierreF commented Jun 22, 2017

I don't see how it help. It should already be used by log4j.xml and don't seem to have any option to limit history size.
If you are thinking to https://logging.apache.org/log4j/extras/apidocs/org/apache/log4j/rolling/RollingFileAppender.html (which is also used by log4j.xml config) and its MaxBackupIndex option, it's a unused option. Code confirm that it's only a JavaDoc comment.

@PierreF PierreF closed this Jun 22, 2017
@PierreF PierreF reopened this Jun 22, 2017
@PierreF
Copy link
Contributor Author

PierreF commented Jun 22, 2017

(closed my mistake, sry)

@gehel
Copy link
Member

gehel commented Jun 22, 2017

In any case, the "right" solution would be to deprecate the log4j based output writers (keep them as an optional module) and move all logging to logback...

@gquintana
Copy link
Member

I also tried to replace the Log4J1 dependency by a Log4J2 dependency for similar need.
But the jmxtrans-output-log4j module has a strong dependency on Log4J1.

@gehel
Copy link
Member

gehel commented Dec 11, 2017

Yeah, those log4j appenders are a mess. We should deprecate them and replace them by slf4j output writer, which can do the same things, but driven by logging configuration. This would require writing some docs on how to use it, remove the log4j appender from the final build (we could keep them around, just in case someone really likes them). My preference would be to migrate to logback more than log4j2, but I'm open to alternatives.

@gquintana
Copy link
Member

Since v270 (maybe v269), JMXTrans uses Logback library. Its RollingFileAppender supports a TimeBasedRollingPolicy which supports both time and size rotation.

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.

None yet

3 participants