openmrs-core: replace log4j 1 which is EOL

log4j 2.x async seems to outperform log4j and logback according to their tests at https://logging.apache.org/log4j/2.x/manual/async.html#Performance

It would be nice to make use of it at some point.

Migration to log4j 2.x seems equally easy as logback since both have sl4j bindings, https://logging.apache.org/log4j/2.0/log4j-slf4j-impl/index.html

Thanks!

Do you think this change is valid for release 2.2.0?

I mean modules that depend on openmrs-core just need to ensure that they use the facade slf4j and not the implementations commons-logging and log4j (like we will do in the core as well lets clean up logging).

@teleivo @darius since 2.1.0 is still in alpha can this feature be sneaked into it, as it means that it is available for general testing and provides time for migrating all the modules in Ref App 2.7

1 Like

I would very much welcome that we replace a very outdated library and would also volunteer to work on this in openmrs-core and help modules to make the change :slight_smile: But this will probably affect all modules.

I mean every module

  1. will probably have a api/src/test/resources/log4j.xml which needs to be replaced with a log4j2.xml (of course we can provide a working sample)
  2. needs to replace all its uses of log4j and apache commons with the facade of slf4j (also an easy task but still work :wink:

If more agree and you want us to do this I’ll set up a wiki for the migration. In fact I already added a how to log https://wiki.openmrs.org/display/docs/Java+Conventions#JavaConventions-loggingLogging

The 2.1.0 version is closed for major changes after alpha release so it would have to go in 2.2.0.

@teleivo, I’d verify if the bridge mentioned at https://logging.apache.org/log4j/2.0/log4j-1.2-api/ works for us since as I understand it doesn’t require config changes.

It would be great to use this opportunity to rethink how modules configure logging. Including log4j.xml is a bad practice as it may mess up/overwrite the global configuration and it introduces a dependency on the log4j version as we just learned the hard way…

@teleivo this is our documentation concerning including log4j.xml in modules: https://wiki.openmrs.org/display/docs/Module+Conventions#ModuleConventions-Logging

Updating to its successor in platform 2.2.0 looks right.

thank you for pointing me to it!!

but I think its already favorable to rely on the facade slf4j instead on the specific logging implementation as thats the reason for having it in the first place.

Are you ok with adapting the module wiki page like so?

One problem that always comes up with this–is it possible to make modules compatible with both the “before” and “after” of this change without forking the module? We generally do our utmost to avoid changes that would require forking modules to maintain compatibility with different versions of Core.

Take care, Mark

Yes am ok with changing the module wiki page as you suggest. :slight_smile:

Hi Mark,

Regarding simple log.info("message") expressions

If every module makes sure that they only use the slf4j API to log messages than it would not need to worry about what logging implementation openmrs-core uses as implementation. So no compatibility changes from before/after. The slf4j facade lib is already in openmrs-core. I dont know why it is not used exclusively (see clean up logging).

One caveat: slf4j does not support the log level FATAL. So it does not provide log.fatal() and thus such calls have to be changed into info.error("message", exception) or using markers info.error(MarkerFactory.getMarker("FATAL"), "message", exception)

see https://www.slf4j.org/faq.html#fatal

log4j.xml

I see that the https://wiki.openmrs.org/display/docs/Module+Conventions discourages modules from adding a log4j.xml (except for testing) so they dont override the cores config. So I hope that nobody did that :wink:

Changing logging configuration programmatically

If modules do change logging config via log4j 1 APIs than this is of course this can be an issue.

Ill try out the bridge from log4j 1 to 2 https://logging.apache.org/log4j/2.0/log4j-1.2-api/ which @raff pointed me to, this might help.

But if you have example modules for me which do more than just call log.info/debug/… please tell me and I’ll check what this would mean for them!

We really should only be using logging for the basic info/debug/etc. I suspect as we start testing against modules we may find some bad practices that may need to be cleaned up, but from what you say, it sounds more like it will be tedious rather than conceptually difficult. And it sounds like after any cleanup, it should work fine before/after which is great.

So, thanks, I don’t have major concerns then.

I’d suggest we start testing with some of the core modules bundled with the reference app and see if we come into any unexpected issues.

Take care, Mark

IMHO yes, if it breaks compatibility (for a good cause) the modules affected should be migrated to be in synch with core 2.2. Or else they can stay unmodified as long as they run with core < 2.2. I guess it’s a matter of lack of resources, if there was a team responsible of each module the migration could be done in parallel in a few days.

I’m not so much concerned with the migration effort (though that does need to be considered). But having to maintain two branches of modules, one compatible with < 2.2 and one 2.2+ is something I’d be very reluctant to do. However, it sounds like @teleivo thinks this won’t be an issue.

Take care, Mark

But we already have that right? I assume release branches of modules only work with certains version of core, not with the latest. Also the branching model we use is trunk-based, and only important fixes are backported by cherry picking, not new developments or refactorings. Maybe I’m wrong.

One way you could do this is have a Logger class where you can register loggers – the modules just use that class.

Are the needed changes predictable enough that we could provide a script to convert the module? When we switched to using Maven, Ben made a shell script that converted a standard module layout to the new maven organization, added the pom, etc. That made it much easier for modules to migrate.

Could we back port support for slf4j? Or maybe make a separate module that injects slf4j support in oldest systems such that other modules could use it. This way, a module migrated to use slf4j could still run on older versions.

Thats already taken care of by the logging facade slf4j which is in core and everyone (core and modules) should use it instead of the logging implementation (be it log4j version 1 or 2).

I’m not saying to remove slf4j, I am saying we should use it :slight_smile: There is no need to inject its already in core a long time.

So as first step modules have to:

Replace their calls to commons-logging or log4j version 1 with calls to slf4j, so they are not tied to the logging implementation openmrs uses! Sample commit for the radiology module here. This can be done today on any branch. The only consequence when doing this switch is that:

slf4j does not support the log level FATAL. So it does not provide log.fatal() and thus such calls have to be changed into info.error(“message”, exception) or using markers info.error(MarkerFactory.getMarker(“FATAL”), “message”, exception)

I can think of a script/IDE automation to support module devs and am volunteering to do this for a handful.

As a background info for those who havent read it lets clean up logging - #5 by dkayiwa This step is independent of switching logging implementations and should be done regardless of whether we switch and when, it can be done ASAP.

As a second step modules have to:

Exchange their log4j.xml file that they include in the src/test/resources for testing purposes with a log4j2-text.xml. We will of course supply the file.

My 2 cents: We use log4j2 on several projects where I work and it is great. It performs very well, has lots of features, is quite flexible in both API and configuration. I suggest log4j2 and nothing else (i.e. slf4j).

Lee

Since we have always had slf4j, then i do not see a backwards compatibility problem for modules. :slight_smile:

Sounds like a plan!

As a side note @lluismf generally the master branches of modules work with much earlier versions of core. We are running, more or less, the latest releases (or even snapshots) of most of the reference app modules, plus some other modules, with OpenMRS 1.10.x. It’s rare that we have branches of modules targeted to certain versions of core. There’s limitations caused by maintaining this, of course, and there’s a chance we may have to change that in the future, but it would require some thought.

Take care, Mark