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

Replacing log4j 1 with a modern, supported logging framework #49

Open
Neutius opened this issue Mar 3, 2022 · 9 comments
Open

Replacing log4j 1 with a modern, supported logging framework #49

Neutius opened this issue Mar 3, 2022 · 9 comments

Comments

@Neutius
Copy link
Contributor

Neutius commented Mar 3, 2022

OpenJUMP is currently still using log4j 1 as a logging implementation. This logging implementation has been end of life and unsupported since 2015. The recently published security vulnerability probably doesn't affect OpenJUMP at all, but it might still be a good reminder that outdated dependencies might pose a risk.

Me and my team would like to donate some of our time to help remove log4j 1 from OpenJUMP. Our company has been using OpenJUMP for years. We have recently removed log4j 1 from our own legacy application and we'd like to volunteer our newly gained experience.

The direct usage of log4j 1 code in the OpenJUMP code base is mostly isolated/concentrated in the com.vividsolutions.jump.workbench.Logger class. There are 6 other classes handling (part of) their own logging, which should probably all be using the centralized Logger class instead. The raw amount of work seems pretty limited.

In general, it's best practice to use a logging API like Apache Commons Logging or SLF4J. There's a TODO comment above the Logger class mentioning Apache Commons Logging, my team has more experience with SLF4J, but I'm sure either one is perfectly fine.

However, the Logger class does directly manipulate and query the logging implementation. We have dealt with a similar situation, and found it unfeasible to do this via SLF4J. Our solution was to use log4j 2 (which is still being supported) and manipulate that logging implementation instead.

Should we investigate the possibility of using SLF4J or Apache Commons Logging in OpenJUMP? Or would it be enough of an improvement to upgrade the logging implementation to log4j 2?

@edeso
Copy link
Member

edeso commented Mar 3, 2022

hey Neutius,

thanks for the generous offer. any contribution is greatly appreciated.

totally agree with your assessment above. everything should use the central Logger, which in turn bundles the implementation details for whichever logger implementation is actually used.

needs we had for now were Log Levels, Console and Logfile Printing. nothing fancy really. ignore the comment from 2016, as times have changed and implementations with it.

reads as if the advantage of using SLF4J for API separation disappeared, which was probably the reason for the Commons-Logging comment as well
https://stackoverflow.com/questions/41498021/is-it-worth-to-use-slf4j-with-log4j2/41500347#41500347

but as we separate API via Logger already, every solution fitting in there should do the trick. we particularily try not to pull in to big dependencies, so maybe that is something to look out for as well.

@jratike80
Copy link
Member

jratike80 commented Mar 3, 2022

Geoserver is just changing away from Log4J 1 by this plan https://github.com/geoserver/geoserver/wiki/GSIP-167. We did not make a large evaluation about the alternatives but we did find this comparison table http://www.java-logging.com/comparison/ but log4j2 was selected because a) we got a maker for log4j2 switch and b) I think we was shown results about log4j2 being much faster in some use scenario of Geoserver. I wonder what it was, this table shows different numbers https://logback.qos.ch/performance.html but what ever they are I suppose that speed is not critical for OpenJUMP.

One developer was a bit in favor of logback because it lacks features that have made log4j2 vulnerable and because the upgrade from log4j 1 maybe would have been easier.

I found also these planning notes https://github.com/geoserver/geoserver/wiki/Update-or-replace-Log4J-1-library.

@Neutius
Copy link
Contributor Author

Neutius commented Mar 3, 2022

Thanks for the quick responses!

we separate API via Logger already

That's a good point, actually. The current setup already provides a separation of API and implementation, while also allowing the manipulation of the underlying implementation. Introducing SLF4J might even be over-engineering in this case.

I'll discuss with my team and we'll hopefully be able to offer a pull request soon. I'll propose two steps:

  1. Moving all interactions with the logging implementation to the OpenJUMP Logger class.
  2. Upgrading to log4j 2 (while keeping the same behaviour and configuration).

@edeso
Copy link
Member

edeso commented Mar 3, 2022

  • Moving all interactions with the logging implementation to the OpenJUMP Logger class.
  • Upgrading to log4j 2 (while keeping the same behaviour and configuration).

sound good to me. thanks again!

@edeso
Copy link
Member

edeso commented May 1, 2022

hey @Neutius,

thanks for the PR. i'll have a look in the course of the coming week. regards ede

@Neutius
Copy link
Contributor Author

Neutius commented May 2, 2022

Hi @edeso,

Thanks, sounds great!

I started work on the second part - the actual upgrade to log4j 2. I hope to open a second PR later this week.

@Neutius
Copy link
Contributor Author

Neutius commented May 5, 2022

I'm busy working on the actual upgrade to Log4j2.

I currently have a version that uses both Log4j 1 and 2, using both versions to write to 2 different files and to the same console. Feel free to take a peak or try it out: rigd-loxia#1

I'm not 100% sure I have maintained the same behaviour between log4j 1 and 2, so I'll look into that some more.

Next step will be to re-write all interactions with Log4j in the com.vividsolutions.jump.workbench.Logger class, e.g. concerning log levels and log files.
When that is done, all Log4j 1 stuff can be taken out.

@Neutius
Copy link
Contributor Author

Neutius commented May 6, 2022

@edeso I have fully implemented and configured Log4j2 - both Log4j 1 and 2 are being used. See rigd-loxia#1

I'll remove Log4j 1 next week. Let me know if you'd like to test out the changes, I can keep this version around on a separate branch.

@edeso
Copy link
Member

edeso commented May 6, 2022

@Neutius no need to keep Log4J v1 around. we can easily compare with older builds when the PR was merged.
i'll have a more detailed look then.

thanks!

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

No branches or pull requests

3 participants