Replace logback by log4j 2

[fabiocarvalho777]

Replace logback by log4j 2

[badalsarkar]

Hi @fabiocarvalho777 ,
I can work on this issue.

Thanks,

[fabiocarvalho777]

Sure, let's conclude the other one first. Notice I added some comments to you PR. Thanks!!

[badalsarkar]

Hi @fabiocarvalho777 ,
Sorry for being late.
I was looking through the code base. Before I start working, I am sharing my findings, just to check I am in the right direction.

For this issue to be resolved, the following tasks must be done-

1. Create necessary configuration for Log4j2
2. Replace LogBackLogConfigurator with new implementation for Log4j2
3. Make modification in LogBackLogConfiguratorTest
4. Make changes in LogFileDefiner as it implements PropertyDefiner from LogBack
5. Change or add new tests

I am still going through the code base and may find new things to be done. But if I have missed anything please let me know.

Thanks,

[fabiocarvalho777]

Hello Badal,

Thanks. Yes, your approach seems to be correct, but let's do these modifications in this plan:

1. Let's leave LogBackLogConfigurator, LogBackLogConfiguratorTest and LogFileDefiner contents as is, but delete these 3 files (we want to get a clean commit history with deletions, instead of modifications for these files)
2. Add Log4j2LogConfigurator and Log4j2LogConfiguratorTest classes
3. Remove logback SLF4J implementation
4. Add Log4J 2 SLF4J implementation

Does this make sense?

Thanks!!

[badalsarkar]

Yes for number 1 and 2.
Just to clarify Number 3 and 4, these basically means that I need to update dependency to add Log4J2 with SLF4J implementation. As Butterfly calls SLF4J api and Log4J2 support SLF4J api, all the calls to logger should work.

Am I correct?

[fabiocarvalho777]

Yes, you are.

However, logback is brought to Butterfly transitively. You need to find out who is bringing it (probably Spring Boot) and exclude it, similarly to what we did here.

Also, make sure to remove this, add Log4j 2 there, and replace this by Log4j 2.

[badalsarkar]

Great. I am on it. Thanks

[fabiocarvalho777]

Thanks!

[badalsarkar]

Hi Fábio,
I have done some progress. The framework is implemented. But HelpIT test case is failing.

I will explain the test case in steps-
Step 1:
new ButterflyCliApp().run(); at this line. And the assertion passes.

Step 2:
Next, we are reassigning System.out here to a temporary file for testing the log output.

Step 3:
After that, we are calling new ButterflyCliApp().run(); again here. And this is where problem begins.

Because we have reassigned System.out to file, the log message should go to the file. But it is still going to the console.

However, if I only do step 2 and step 3 it works fine. It prints everything to the file. But when we are reassigning System.out in the middle of the process, the loggers are still logging to previous System.out.

I have tried to see if there is any configuration error, but could not find any. You can take a look at my repo https://github.com/badalsarkar/butterfly/tree/issue%23314.

My suspect is that it has something to do with Log4j2 LoggerContext.

Thanks.

[fabiocarvalho777]

Hello Badal,

Thanks for sharing this information in detail.
There are two things that I believe could help in this investigation.

1. The reason why we decided to move to Log4J 2 is to take advantage of its Async nature. Because of that, there is a chance the log is not really flushed at the time the test needs to assert it. Maybe it would be necessary to block and wait for the thread that does so. Or, change Log4J 2 somehow (only during that test) to work sequentially, on the same thread that runs the test.
2. Other possibility is the default logger is not using an appender directed to the console. Or, even if it is, the "console" is not using System.out as output stream. That sounds very unlikely but maybe that should be double checked.

I hope this helps. Let me know how it goes. Thanks!!

[badalsarkar]

Issue solved. Log4j2 has a specific attribute for ConsoleAppender to activate reassignment of System.out. I just found it out. Now all tests are passing. I will open a PR soon.

[fabiocarvalho777]

Nice!! :-)

[fabiocarvalho777]

@badalsarkar just merged your PR. Thanks again!!
Let me know which other issue you want to work with, or I can find some to suggest too if you prefer.