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

fix: switch from log4j to SLF4J #610

Merged
merged 6 commits into from
Jan 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 7 additions & 5 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -93,11 +93,13 @@ Twilio.setEdge("sydney");
This will result in the `hostname` transforming from `api.twilio.com` to `api.sydney.au1.twilio.com`.

### Enable Debug Logging
In order to enable debug logging, create a configuration file named log4j2.xml that defines the logger at the root level to at least 'debug'. An example of the configuration file can be found [here](src/main/java/com/twilio/example/log4j2.xml). For more configuration options please see the log4j configuration [documentation](https://logging.apache.org/log4j/2.x/manual/configuration.html).
```java
Twilio.init(accountSid, authToken);
Twilio.setLoggerConfiguration("path/to/log4j2.xml");
```

This library uses SLF4J for logging. Consult the SFL4J documentation
for information about logging configuration:

http://slf4j.org/docs.html

For example, if you are using `log4j`, make sure you have `slf4j-log4j12-1.X.XX.jar`, `log4j-1.X.XX.jar` and `slf4j-api-1.X.XX.jar` in your classpath and a `log4j.properties` file in the root of your project.

### Environment Variables

Expand Down
27 changes: 20 additions & 7 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,11 @@
<artifactId>jackson-datatype-jsr310</artifactId>
<version>${jackson.version}</version>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
<version>1.7.30</version>
</dependency>
<dependency>
<groupId>javax.xml.bind</groupId>
<artifactId>jaxb-api</artifactId>
Expand All @@ -231,6 +236,12 @@
<artifactId>archunit</artifactId>
<version>0.14.1</version>
<scope>test</scope>
<exclusions>
thinkingserious marked this conversation as resolved.
Show resolved Hide resolved
<exclusion>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>nl.jqno.equalsverifier</groupId>
Expand All @@ -240,13 +251,15 @@
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
thinkingserious marked this conversation as resolved.
Show resolved Hide resolved
<artifactId>log4j-api</artifactId>
<version>2.14.0</version>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-core</artifactId>
<artifactId>log4j-slf4j-impl</artifactId>
<version>2.14.0</version>
<scope>test</scope>
<exclusions>
<exclusion>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</exclusion>
</exclusions>
</dependency>
</dependencies>
<build>
Expand Down Expand Up @@ -392,4 +405,4 @@
<artifactId>oss-parent</artifactId>
<version>7</version>
</parent>
</project>
</project>
16 changes: 0 additions & 16 deletions src/main/java/com/twilio/Twilio.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,6 @@
import java.util.concurrent.Executors;
import java.io.File;

import org.apache.logging.log4j.core.LoggerContext;
import org.apache.logging.log4j.LogManager;

/**
* Singleton class to initialize Twilio environment.
*/
Expand Down Expand Up @@ -154,19 +151,6 @@ public static synchronized void setEdge(final String edge) {
Twilio.edge = edge;
}

/**
* Set the logger configuration file path.
*
* @param filePath path to logging configuration file
* @param loggerContext defaults to false to get the appropriate logger context for the caller.
*/
public static synchronized void setLoggerConfiguration(final String filePath, final boolean... loggerContext) {
thinkingserious marked this conversation as resolved.
Show resolved Hide resolved
boolean logContext = (loggerContext.length >= 1) ? loggerContext[0] : false;
LoggerContext context = (org.apache.logging.log4j.core.LoggerContext) LogManager.getContext(logContext);
File file = new File(filePath);
context.setConfigLocation(file.toURI());
}

/**
* Returns (and initializes if not initialized) the Twilio Rest Client.
*
Expand Down
55 changes: 30 additions & 25 deletions src/main/java/com/twilio/http/TwilioRestClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,13 @@
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.util.function.Predicate;
import java.util.Map;
import java.util.List;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.core.LoggerContext;

public class TwilioRestClient {

public static final int HTTP_STATUS_CODE_CREATED = 201;
Expand All @@ -25,7 +24,7 @@ public class TwilioRestClient {
private final String region;
private final String edge;
private final HttpClient httpClient;
private static final Logger logger = LogManager.getLogger();
private static final Logger logger = LoggerFactory.getLogger(TwilioRestClient.class);

private TwilioRestClient(Builder b) {
this.username = b.username;
Expand Down Expand Up @@ -59,11 +58,14 @@ public Response request(final Request request) {

logRequest(request);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Response response = httpClient.reliableRequest(request);
logger.debug("status code: " + response.getStatusCode());
org.apache.http.Header[] responseHeaders = response.getHeaders();
logger.debug("response headers:");
for (int i = 0; i < responseHeaders.length; i++) {
logger.debug(responseHeaders[i]);

if (logger.isDebugEnabled()) {
logger.debug("status code: {}", response.getStatusCode());
org.apache.http.Header[] responseHeaders = response.getHeaders();
logger.debug("response headers:");
for (int i = 0; i < responseHeaders.length; i++) {
logger.debug("responseHeader: {}", responseHeaders[i]);
}
}

return response;
Expand Down Expand Up @@ -146,25 +148,28 @@ public TwilioRestClient build() {
* Logging debug information about HTTP request.
*/
public void logRequest(final Request request) {
logger.debug("-- BEGIN Twilio API Request --");
logger.debug("request method: " + request.getMethod());
logger.debug("request URL: " + request.getUrl());
final Map<String, List<String>> queryParams = request.getQueryParams();
final Map<String, List<String>> headerParams = request.getHeaderParams();

if (!queryParams.isEmpty()) {
logger.debug("query parameters: " + queryParams);
}
if (logger.isDebugEnabled()) {
logger.debug("-- BEGIN Twilio API Request --");
logger.debug("request method: " + request.getMethod());
logger.debug("request URL: " + request.getUrl());
final Map<String, List<String>> queryParams = request.getQueryParams();
final Map<String, List<String>> headerParams = request.getHeaderParams();

if (queryParams != null && !queryParams.isEmpty()) {
logger.debug("query parameters: " + queryParams);
}

if (!headerParams.isEmpty()) {
logger.debug("header parameters: ");
for (String key : headerParams.keySet()) {
if (!key.toLowerCase().contains("authorization")) {
logger.debug(key + ": " + headerParams.get(key));
if (headerParams != null && !headerParams.isEmpty()) {
logger.debug("header parameters: ");
for (String key : headerParams.keySet()) {
if (!key.toLowerCase().contains("authorization")) {
logger.debug(key + ": " + headerParams.get(key));
}
}
}

logger.debug("-- END Twilio API Request --");
}
logger.debug("-- END Twilio API Request --");
}

}
7 changes: 2 additions & 5 deletions src/test/java/com/twilio/http/LoggingTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import com.twilio.Twilio;
import com.twilio.rest.Domains;
import com.twilio.rest.api.v2010.account.Message;
import org.junit.Before;
import org.junit.Test;
import org.junit.Assert;
Expand All @@ -11,8 +10,8 @@
import java.io.PrintStream;

public class LoggingTest {
private static final PrintStream ORIGINAL_SYSTEM_OUT = System.out;
private ByteArrayOutputStream output;
private PrintStream originalStream;

@Before
public void setUp() throws Exception {
Expand All @@ -22,21 +21,19 @@ public void setUp() throws Exception {
public void logCapturingSetup() {
output = new ByteArrayOutputStream();
PrintStream outputStream = new PrintStream(output);
originalStream = System.out;
System.setOut(outputStream);
}

public void finishLogCapturingSetup(Request request) {
TwilioRestClient twilioRestClient = Twilio.getRestClient();
twilioRestClient.logRequest(request);
System.out.flush();
System.setOut(originalStream);
System.setOut(ORIGINAL_SYSTEM_OUT);
}

@Test
public void testDebugLogging() {
logCapturingSetup();
Twilio.setLoggerConfiguration("src/main/java/com/twilio/example/log4j2.xml");
final Request request = new Request(HttpMethod.GET, Domains.API.toString(),
"/2010-04-01/Accounts/AC123/Messages/MM123.json");
request.addHeaderParam("Authorization", "authorization value");
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
<?xml version="1.0" encoding="UTF-8"?>
thinkingserious marked this conversation as resolved.
Show resolved Hide resolved
<!--logging configuration file example-->
<Configuration status="warn" name="MyApp" packages="">
<Appenders>
<Console name="STDOUT" target="SYSTEM_OUT">
Expand Down