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

Implements a slf4j logger impl as default logger in azure core #7298

Merged
merged 13 commits into from
Jan 16, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@

<!-- Any code in any package, it should never be a 'throw' keyword in the client library codebase except for in the client logger -->
<suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck" files=".*[/\\]com[/\\]azure[/\\]core[/\\]util[/\\]logging[/\\]*"/>
<suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck" files=".*[/\\]com[/\\]azure[/\\]core[/\\]implementation[/\\]logging[/\\]*"/>
Copy link
Member

Choose a reason for hiding this comment

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

Now that all methods are implemented in DefaultLogger this exception is no longer required.


<!-- Suppress IO exception for now, which need code owner's attention -->
<suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck" files="com.azure.storage.blob.BlobInputStream.java"/>
Expand Down Expand Up @@ -184,8 +185,10 @@
<!-- CodeSnippet Suppression for now, which need code owner's attention -->
<suppress checks="com.azure.tools.checkstyle.checks.JavadocCodeSnippetCheck" files="com.azure.data.appconfiguration.ConfigurationAsyncClient.java"/>

<!-- ClientLogger class suppression -->
<!-- Logger class suppression -->
<suppress checks="com.azure.tools.checkstyle.checks.GoodLoggingCheck" files="ClientLogger.java"/>
<suppress checks="com.azure.tools.checkstyle.checks.GoodLoggingCheck" files="DefaultLogger.java"/>
<suppress checks="com.azure.tools.checkstyle.checks.GoodLoggingCheck" files="DefaultLoggerBase.java"/>
Copy link
Member

Choose a reason for hiding this comment

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

This line can now be removed as DefaultLoggerBase does not exist.


<!-- Use the logger in Utility static method. -->
<suppress checks="com.azure.tools.checkstyle.checks.GoodLoggingCheck" files="com.azure.storage.common.Utility.java"/>
Expand Down
6 changes: 0 additions & 6 deletions sdk/core/azure-core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -120,12 +120,6 @@
<version>2.2</version> <!-- {x-version-update;org.hamcrest:hamcrest-library;external_dependency} -->
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-simple</artifactId>
<version>1.7.25</version> <!-- {x-version-update;org.slf4j:slf4j-simple;external_dependency} -->
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,366 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

package com.azure.core.implementation.logging;

import com.azure.core.util.logging.LogLevel;
import java.io.PrintWriter;
import java.io.StringWriter;
import java.time.LocalDateTime;
import java.time.format.DateTimeFormatter;
import org.slf4j.helpers.FormattingTuple;
import org.slf4j.helpers.MarkerIgnoringBase;
import org.slf4j.helpers.MessageFormatter;

/**
* This class is an internal implementation of slf4j logger.
*/
public final class DefaultLogger extends MarkerIgnoringBase {
private static final long serialVersionUID = -144261058636441630L;

private String classPath;
private static final DateTimeFormatter DATE_FORMAT = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm");

// The template forms the log message in a format:
// YYYY-MM-DD HH:MM [thread] [level] classpath - message
// E.g: 2020-01-09 12:35 [main] [WARNING] com.azure.core.DefaultLogger - This is my log message.
private static final String MESSAGE_TEMPLATE = "%s [%s] [%s] %s - %s";

/**
* Construct DefaultLogger for the given class.
*
* @param clazz Class creating the logger.
*/
public DefaultLogger(Class<?> clazz) {
this(clazz.getName());
}

/**
* Construct DefaultLogger for the given class name.
*
* @param className Class name creating the logger.
* @throws RuntimeException it is an error.
Copy link
Member

Choose a reason for hiding this comment

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

Will this throw an exception now that you have a catch block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not logger throw an exception. Good catch. Will update the JavaDoc

*/
public DefaultLogger(String className) {
try {
this.classPath = Class.forName(className).getCanonicalName();
} catch (ClassNotFoundException e) {
this.classPath = className;
}
}

/**
* {@inheritDoc}
* @param format The formattable message to log.
* @param args Arguments for the message. If an exception is being logged, the last argument should be the
* {@link Throwable}.
*/
@Override
public void debug(String format, Object... args) {
logFromFormat(LogLevel.VERBOSE, format, args);
}

/**
* {@inheritDoc}
* @param format The formattable message to log.
* @param args Arguments for the message. If an exception is being logged, the last argument should be the
* {@link Throwable}.
*/
@Override
public void info(String format, Object... args) {
logFromFormat(LogLevel.INFORMATIONAL, format, args);
}

/**
* {@inheritDoc}
* @param format The formattable message to log.
Copy link
Member

Choose a reason for hiding this comment

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

Combination of inheritdoc and @param docs don't work correctly. Just use @inheritDoc and skip @param docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

* @param args Arguments for the message. If an exception is being logged, the last argument should be the
* {@link Throwable}.
*/
@Override
public void warn(String format, Object... args) {
logFromFormat(LogLevel.WARNING, format, args);
}

/**
* {@inheritDoc}
* @param format The formattable message to log.
* @param args Arguments for the message. If an exception is being logged, the last argument should be the
* {@link Throwable}.
*/
@Override
public void error(String format, Object... args) {
logFromFormat(LogLevel.ERROR, format, args);
}

/**
* Format and write the message according to the {@code MESSAGE_TEMPLATE}.
*
* @param level The level to log.
* @param format The log message format.
* @param arguments a list of arbitrary arguments taken in by format.
*/
private void logFromFormat(LogLevel level, String format, Object... arguments) {
FormattingTuple tp = MessageFormatter.arrayFormat(format, arguments);
log(level, tp.getMessage(), tp.getThrowable());
}

/**
* Format and write the message according to the {@code MESSAGE_TEMPLATE}.
*
* @param level log level
* @param message The message itself
* @param t The exception whose stack trace should be logged
*/
private void log(LogLevel level, String message, Throwable t) {
String dateTime = getFormattedDate();
String threadName = Thread.currentThread().getName();
String levelName = level.name();
StringBuilder buf = new StringBuilder(32);
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to instantiate the StringBuilder with an internal capacity of 32? Instead could it be instantiated with a capacity large enough to hold the initial string being written to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is inherited from simple logger.
The message we know definitely will be printed out is:
2020-01-09 12:35 [main] [Informational]

This is 39 character long. I can put 64 instead of 32 characters to minimize capacity increase.

buf.append(String.format(MESSAGE_TEMPLATE, dateTime, threadName, levelName, classPath, message));
writeWithThrowable(buf, t);
}

/**
* Get the current time in Local time zone.
*
* @return The current time in {@code DATE_FORMAT}
*/
private String getFormattedDate() {
LocalDateTime now = LocalDateTime.now();
return DATE_FORMAT.format(now);
}

/**
* Write the log message with throwable stack trace if any.
*
* @param buf Take the log messages.
* @param t The exception whose stack trace should be logged
*/
void writeWithThrowable(StringBuilder buf, Throwable t) {
if (t != null) {
StringWriter sw = new StringWriter();
try (PrintWriter pw = new PrintWriter(sw)) {
t.printStackTrace(pw);
buf.append(sw.toString());
}
}
System.out.println(buf.toString());
}

/**
* {@inheritDoc}
*/
@Override
public String getName() {
throw new UnsupportedOperationException();
Copy link
Member

Choose a reason for hiding this comment

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

This should be supported. We should return the name of this logger instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Talked offline. Will create an issue and put Up-for-grabs label.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

github.com//issues/7341

}

/**
* {@inheritDoc}
*/
@Override
public boolean isTraceEnabled() {
throw new UnsupportedOperationException();
}

/**
* {@inheritDoc}
*/
@Override
public void trace(final String msg) {
throw new UnsupportedOperationException();
}

/**
* {@inheritDoc}
*/
@Override
public void trace(final String format, final Object arg) {
throw new UnsupportedOperationException();
}

/**
* {@inheritDoc}
*/
@Override
public void trace(final String format, final Object arg1, final Object arg2) {
throw new UnsupportedOperationException();
}

/**
* {@inheritDoc}
*/
@Override
public void trace(final String format, final Object... arguments) {
throw new UnsupportedOperationException();
}

/**
* {@inheritDoc}
*/
@Override
public void trace(final String msg, final Throwable t) {
Copy link
Member

Choose a reason for hiding this comment

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

Why overload all of these methods to throw an exception when the base class has an actual useful implementation? Look at https://github.com/qos-ch/slf4j/blob/master/slf4j-api/src/main/java/org/slf4j/helpers/MarkerIgnoringBase.java

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

throw new UnsupportedOperationException();
}

/**
* {@inheritDoc}
*/
@Override
public boolean isDebugEnabled() {
throw new UnsupportedOperationException();
Copy link
Member

Choose a reason for hiding this comment

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

This should be supported since we have an implementation for debug(String format, Object.. argArray) on line 61.

}

/**
* {@inheritDoc}
*/
@Override
public void debug(final String msg) {
throw new UnsupportedOperationException();
Copy link
Member

Choose a reason for hiding this comment

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

This should be supported too as we have support for debug overload with formatted strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I enabled the ones used by ClientLogger.

}

/**
* {@inheritDoc}
*/
@Override
public void debug(final String format, final Object arg) {
throw new UnsupportedOperationException();
}

/**
* {@inheritDoc}
*/
@Override
public void debug(final String format, final Object arg1, final Object arg2) {
throw new UnsupportedOperationException();
}

/**
* {@inheritDoc}
*/
@Override
public void debug(final String msg, final Throwable t) {
throw new UnsupportedOperationException();
}

/**
* {@inheritDoc}
*/
@Override
public boolean isInfoEnabled() {
throw new UnsupportedOperationException();
Copy link
Member

Choose a reason for hiding this comment

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

All is*Enabled() methods should be supported.

}

/**
* {@inheritDoc}
*/
@Override
public void info(final String msg) {
throw new UnsupportedOperationException();
Copy link
Member

Choose a reason for hiding this comment

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

These should be implemented too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above

}

/**
* {@inheritDoc}
*/
@Override
public void info(final String format, final Object arg) {
throw new UnsupportedOperationException();
}

/**
* {@inheritDoc}
*/
@Override
public void info(final String format, final Object arg1, final Object arg2) {
throw new UnsupportedOperationException();
}

/**
* {@inheritDoc}
*/
@Override
public void info(final String msg, final Throwable t) {
throw new UnsupportedOperationException();
}

/**
* {@inheritDoc}
*/
@Override
public boolean isWarnEnabled() {
throw new UnsupportedOperationException();
}

/**
* {@inheritDoc}
*/
@Override
public void warn(final String msg) {
throw new UnsupportedOperationException();
}

/**
* {@inheritDoc}
*/
@Override
public void warn(final String format, final Object arg) {
throw new UnsupportedOperationException();
}

/**
* {@inheritDoc}
*/
@Override
public void warn(final String format, final Object arg1, final Object arg2) {
throw new UnsupportedOperationException();
}

/**
* {@inheritDoc}
*/
@Override
public void warn(final String msg, final Throwable t) {
throw new UnsupportedOperationException();
}

/**
* {@inheritDoc}
*/
@Override
public boolean isErrorEnabled() {
throw new UnsupportedOperationException();
}

/**
* {@inheritDoc}
*/
@Override
public void error(final String msg) {
throw new UnsupportedOperationException();
}

/**
* {@inheritDoc}
*/
@Override
public void error(final String format, final Object arg) {
throw new UnsupportedOperationException();
}

/**
* {@inheritDoc}
*/
@Override
public void error(final String format, final Object arg1, final Object arg2) {
throw new UnsupportedOperationException();
}

/**
* {@inheritDoc}
*/
@Override
public void error(final String msg, final Throwable t) {
throw new UnsupportedOperationException();
}
}
Loading