fix: switch from log4j to SLF4J #610

sullis · 2020-12-06T07:00:59Z

Checklist

I acknowledge that all my contributions will be made under the project's license

eshanholtz · 2020-12-08T00:11:25Z

@sullis We recently added log4j as our logging dependency. Any reason why we should be using SLF4J instead?

lex-erik · 2020-12-17T15:34:40Z

Hi @eshanholtz, below I've written up a few reasons for using Slf4j.

Log4j is an implementation, Slf4j is a logging abstraction and not an actual implementation. This is important, as it allows consumers of your library to choose their own flavor of logging implementation.

By forcing Log4j unto your consumers,

the Twilio SDK is possibly breaking applications by including an additional logging implementation
when consumers use a different logging implementation, then logs generated by Twilio do not follow the rules and may not be shipped to ElasticSearch or Splunk unless I create a log4j version of the logging configuration.

To hit it home further, I'm currently running into one of these problems. The Twilio SDK is currently conflicting with server frameworks such as Spring and Micronaut that rely on classpath scanning and detect which logging library is on the classpath. See for example here where Micronaut detects the logging implementation, however, when it finds multiple Logging implementations it will refuse to start (as it should). Spring works in a similar fashion.

I believe it's considered a rule of thumb that as SDK developers you must use Slf4j and leave logging implementation choices to your consumer. SDK developers can chose to include log4j as runtime dependency when they develop, but it should never become a compile time dependency. Slf4j is exactly created to solve these type of problems.

My $.02.

lex-erik · 2020-12-17T15:45:47Z

As FYI, for whoever is running into similar issues, Twilio 7.3.0 is the latest release before log4j became a compile dependency. Reverting back to Twilio SDK 7.3.0 worked for me.

thinkingserious

Thanks for the PR @sullis!

Please take a look at my comments and let us know what you think. Thanks again!

pom.xml

thinkingserious · 2020-12-17T22:49:25Z

README.md

-Twilio.setLoggerConfiguration("path/to/log4j2.xml");
-```
+
+This library uses SFL4J for logging. Consult the SFL4J documentation


We need to update the example (perhaps java.util.logging, logback or log4j).

pom.xml

src/main/java/com/twilio/Twilio.java

This is a proof of concept. It demonstrates how SLF4J could be used as the Logging API. SFL4J is the standard logging API for open source Java libraries.

eshanholtz · 2021-01-05T00:25:42Z

@sullis Your changes look good. Can you just address the last few comments from @thinkingserious and we can get this PR merged. Thanks!

thinkingserious · 2021-01-26T18:15:20Z

Hello @sullis,

Would you like us to take this PR from here and get this PR closed out? Please let me know what you think. Thanks again!

With best regards,

Elmer

sullis · 2021-01-26T18:30:38Z

Would you like us to take this PR from here and get this PR closed out? Please let me know what you think. Thanks again!

Please take ownership of this PR and/or open a replacement PR. I think it is best for you to own it.

thinkingserious · 2021-01-26T18:56:32Z

Thank you @sullis! We appreciate your contribution to help us get this update merged!

thinkingserious · 2021-01-28T05:36:09Z

Here is how I tested this, from this branch:

make install (from twilio-java root)
touch LogExample.java

import com.twilio.Twilio;
import com.twilio.rest.api.v2010.account.Message;
import com.twilio.type.PhoneNumber;

public class LogExample {
    public static void main(String[] args) {
        String accountSid = System.getenv("TWILIO_ACCOUNT_SID");
		String authToken = System.getenv("TWILIO_AUTH_TOKEN");
        Twilio.init(accountSid, authToken);
        Message message = Message.creator(
        new PhoneNumber(System.getenv("TWILIO_TO_NUMBER");),
        new PhoneNumber(System.getenv("TWILIO_FROM_NUMBER");),
            "Hello logging world!"
        ).create();

        System.out.println(message.getSid());
    }
}

touch log4j.properties

log4j.rootLogger=DEBUG,stdout
log4j.logger.com.endeca=INFO
log4j.logger.com.endeca.itl.web.metrics=INFO

log4j.appender.stdout=org.apache.log4j.ConsoleAppender
log4j.appender.stdout.layout=org.apache.log4j.PatternLayout
log4j.appender.stdout.layout.ConversionPattern=%p\t%d{ISO8601}\t%r\t%c\t[%t]\t%m%n

Download the following jars: slf4j-log4j12-1.7.28.jar, log4j-1.2.17.jar, slf4j-api-1.7.28.jar
javac -classpath ./slf4j-log4j12-1.7.28.jar:./log4j-1.2.17.jar:./slf4j-api-1.7.28.jar:./target/twilio-8.6.0-jar-with-dependencies.jar:. LogExample.java && java -classpath ./slf4j-log4j12-1.7.28.jar:./log4j-1.2.17.jar:./slf4j-api-1.7.28.jar:./target/twilio-8.6.0-jar-with-dependencies.jar:. LogExample

Then, you should see something like:

...
DEBUG	2021-01-27 21:15:06,759	784	com.twilio.http.TwilioRestClient	[main]	responseHeader: X-Shenanigans: none
...

JenniferMah · 2021-01-28T18:01:33Z

src/main/java/com/twilio/http/TwilioRestClient.java

@@ -59,11 +58,14 @@ public Response request(final Request request) {

        logRequest(request);


We need to add a check either within this function or before this function is called to make sure logger.isDebugEnabled(). Within the logRequest() function can we add some additional logic to make sure headerParams and queryParams are not null before we log them?

Great catch! isEmpty() does not catch null values.

src/test/resources/log4j2.xml

…java into slf4j-logging-api

JenniferMah

Did you test this with Java 9 or later to make sure that we don't see any issues with the build artifact?

thinkingserious · 2021-01-29T20:29:48Z

I tested with Java 11 and did not observe any issues.

JenniferMah

🎆

eshanholtz added the status: waiting for feedback waiting for feedback from the submitter label Dec 8, 2020

thinkingserious reviewed Dec 17, 2020

View reviewed changes

sullis force-pushed the slf4j-logging-api branch from 309da69 to 3032865 Compare December 24, 2020 18:09

SLF4J logging api

15271f9

This is a proof of concept. It demonstrates how SLF4J could be used as the Logging API. SFL4J is the standard logging API for open source Java libraries.

sullis force-pushed the slf4j-logging-api branch from 3032865 to 15271f9 Compare December 24, 2020 18:11

thinkingserious added the type: community enhancement feature request not on Twilio's roadmap label Dec 28, 2020

eshanholtz changed the title ~~SLF4J logging api~~ fix: SLF4J logging api Jan 5, 2021

shwetha-manvinkurke mentioned this pull request Jan 20, 2021

log4j integration broke compatibility with Keycloak #615

Closed

Merge branch 'main' into slf4j-logging-api

ae5d725

add simple example

dc1777b

Merge branch 'main' into slf4j-logging-api

ba2b873

thinkingserious marked this pull request as ready for review January 28, 2021 05:36

JenniferMah reviewed Jan 28, 2021

View reviewed changes

thinkingserious added 2 commits January 29, 2021 06:30

add null check and debug logic

2a21429

Merge branch 'slf4j-logging-api' of https://github.com/sullis/twilio-…

b75a9bf

…java into slf4j-logging-api

JenniferMah reviewed Jan 29, 2021

View reviewed changes

JenniferMah approved these changes Jan 29, 2021

View reviewed changes

thinkingserious changed the title ~~fix: SLF4J logging api~~ fix: switch to SLF4J Jan 29, 2021

thinkingserious changed the title ~~fix: switch to SLF4J~~ fix: switch from log4j to SLF4J Jan 29, 2021

thinkingserious merged commit 69e449f into twilio:main Jan 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: switch from log4j to SLF4J #610

fix: switch from log4j to SLF4J #610

sullis commented Dec 6, 2020 •

edited by thinkingserious

Loading

eshanholtz commented Dec 8, 2020

lex-erik commented Dec 17, 2020

lex-erik commented Dec 17, 2020

thinkingserious left a comment

thinkingserious Dec 17, 2020

eshanholtz commented Jan 5, 2021

thinkingserious commented Jan 26, 2021

sullis commented Jan 26, 2021

thinkingserious commented Jan 26, 2021

thinkingserious commented Jan 28, 2021 •

edited

Loading

JenniferMah Jan 28, 2021 •

edited

Loading

thinkingserious Jan 29, 2021

JenniferMah left a comment

thinkingserious commented Jan 29, 2021

JenniferMah left a comment

		@@ -59,11 +58,14 @@ public Response request(final Request request) {

		logRequest(request);

fix: switch from log4j to SLF4J #610

fix: switch from log4j to SLF4J #610

Conversation

sullis commented Dec 6, 2020 • edited by thinkingserious Loading

Checklist

eshanholtz commented Dec 8, 2020

lex-erik commented Dec 17, 2020

lex-erik commented Dec 17, 2020

thinkingserious left a comment

Choose a reason for hiding this comment

thinkingserious Dec 17, 2020

Choose a reason for hiding this comment

eshanholtz commented Jan 5, 2021

thinkingserious commented Jan 26, 2021

sullis commented Jan 26, 2021

thinkingserious commented Jan 26, 2021

thinkingserious commented Jan 28, 2021 • edited Loading

JenniferMah Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

thinkingserious Jan 29, 2021

Choose a reason for hiding this comment

JenniferMah left a comment

Choose a reason for hiding this comment

thinkingserious commented Jan 29, 2021

JenniferMah left a comment

Choose a reason for hiding this comment

sullis commented Dec 6, 2020 •

edited by thinkingserious

Loading

thinkingserious commented Jan 28, 2021 •

edited

Loading

JenniferMah Jan 28, 2021 •

edited

Loading