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

#28 SLF4J logging #89

Merged
merged 13 commits into from
Apr 3, 2015
6 changes: 6 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,12 @@
<artifactId>javax.el-api</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
<version>1.7.7</version>
<scope>compile</scope>
Copy link
Owner

Choose a reason for hiding this comment

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

we should add <optional>true</optional> here, in case a user doesn't want to use SLF4J logging, this dependency won't be in his project

</dependency>
</dependencies>
<build>
<plugins>
Expand Down
75 changes: 75 additions & 0 deletions src/main/java/org/takes/facets/slf4j/BkLogged.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/**
* The MIT License (MIT)
*
* Copyright (c) 2015 Yegor Bugayenko
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included
* in all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
* SOFTWARE.
*/

package org.takes.facets.slf4j;

import java.io.IOException;
import java.net.Socket;
import lombok.EqualsAndHashCode;
import org.takes.http.Back;

/**
* Logs Back.accept() calls.
*
* <p>The class is immutable and thread-safe.
* @author Dmitry Zaytsev (dmitry.zaytsev@gmail.com)
* @version $Id$
Copy link
Owner

Choose a reason for hiding this comment

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

let's add since 0.11

*/
@EqualsAndHashCode(of = "origin", callSuper = false)
public final class BkLogged extends LogWrap implements Back {
/**
* Original back.
*/
private final transient Back origin;

/**
* Ctor.
* @param back Original
*/
public BkLogged(final Back back) {
this(back, LogWrap.Level.TRACE);
}

/**
* Ctor.
* @param back Original
* @param lvl Log level
*/
public BkLogged(final Back back, final LogWrap.Level lvl) {
super(back.getClass(), lvl);
this.origin = back;
}

@Override
public void accept(final Socket socket) throws IOException {
final long started = System.currentTimeMillis();
this.origin.accept(socket);
this.log(
"[%s] #accept(%s) in [%d] ms",
this.origin,
socket,
System.currentTimeMillis() - started
);
}
}
73 changes: 73 additions & 0 deletions src/main/java/org/takes/facets/slf4j/FtLogged.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/**
* The MIT License (MIT)
*
* Copyright (c) 2015 Yegor Bugayenko
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included
* in all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
* SOFTWARE.
*/

package org.takes.facets.slf4j;

import java.io.IOException;
import lombok.EqualsAndHashCode;
import org.takes.http.Exit;
import org.takes.http.Front;

/**
* Logs Front.start() calls.
*
* <p>The class is immutable and thread-safe.
* @author Dmitry Zaytsev (dmitry.zaytsev@gmail.com)
* @version $Id$
*/
@EqualsAndHashCode(of = "origin", callSuper = false)
public final class FtLogged extends LogWrap implements Front {
/**
* Original front.
*/
private final transient Front origin;

/**
* Ctor.
* @param front Original
*/
public FtLogged(final Front front) {
this(front, LogWrap.Level.TRACE);

Choose a reason for hiding this comment

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

are you sure we need TRACE level by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@longtimeago I think logs every method's call with parameters and return values will be too big. It could be used for debug only. Level.DEBUIG will be better ?

Choose a reason for hiding this comment

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

I would store default level inside the LogWrap and make it DEBUIG instead of TRACE

}

/**
* Ctor.
* @param front Original
* @param lvl Log level
*/
public FtLogged(final Front front, final LogWrap.Level lvl) {
super(front.getClass(), lvl);
this.origin = front;
}

@Override
public void start(final Exit exit) throws IOException {
this.log(
"[%s] #start(%s)",
this.origin,
exit
);
this.origin.start(exit);
}
}
115 changes: 115 additions & 0 deletions src/main/java/org/takes/facets/slf4j/LogWrap.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
/**
* The MIT License (MIT)
*
* Copyright (c) 2015 Yegor Bugayenko
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included
* in all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
* SOFTWARE.
*/

package org.takes.facets.slf4j;

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

/**
* Wrap slf4j functionality.
*
* <p>The class is immutable and thread-safe.
* @author Dmitry Zaytsev (dmitry.zaytsev@gmail.com)
* @version $Id$
* @since 0.11
*/
@SuppressWarnings({ "PMD.CyclomaticComplexity", "PMD.LoggerIsNotStaticFinal" })
public class LogWrap {
/**
* Log levels.
*/
public enum Level {
/**
* Trace level.
*/
TRACE,
/**
* Debug level.
*/
DEBUG,
/**
* Info level.
*/
INFO,
/**
* Warn level.
*/
WARN,
/**
* Severe level.
*/
SEVERE
};

/**
* Log level.
*/
private final transient LogWrap.Level level;

/**
* Logger.
*/
private final transient Logger logger;

/**
* Ctor.
* @param clazz Logger class
* @param lvl Log level
*/
public LogWrap(final Class<?> clazz, final LogWrap.Level lvl) {
this.level = lvl;
this.logger = LoggerFactory.getLogger(clazz);
}

/**
* Log message.
* @param format Format string
* @param param Parameters
*/
// @checkstyle JavadocLocationCheck (1 line)

Choose a reason for hiding this comment

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

you can put suppression to the javadoc itself

/**
* Log message.
* @param format Format string
* @param param Parameters
* @checkstyle CyclomaticComplexityCheck (1 line)
*/

and remove JavadocLocationCheck suppression

// @checkstyle CyclomaticComplexityCheck (1 line)
public final void log(final String format, final Object... param) {

Choose a reason for hiding this comment

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

first of all, I'm not sure we need to check if some level is enabled for all levels. In general, check is meaningful for DEBUG and TRACE as almost all application have default log level INFO. Take a look at how it could look like:

    public final void log(final String format, final Object... param) {
        switch (this.level) {
            case TRACE :
                if (this.logger.isTraceEnabled()) {
                    this.logger.trace(String.format(format, param));
                }
                break;
            case DEBUG:
                if (this.logger.isDebugEnabled()) {
                    this.logger.debug(String.format(format, param));
                }
                break;
            case INFO:
                this.logger.info(String.format(format, param));
                break;
            case WARN:
                this.logger.warn(String.format(format, param));
                break;
            case SEVERE:
                this.logger.error(String.format(format, param));
                break;
            default:
                throw new IllegalArgumentException("Unknown log level");

        }
    }

what do you think?

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 think we need to check for all levels.
for example application sets INFO level, but in some condition it will generate huge logs. We could disable logs via log4.properties, but String.format() for a lot objects may be too expensive. We need check level before call log.info() to avoid cpu's overhead.

Choose a reason for hiding this comment

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

well ... there is more clever solution - delegate formatting to the underlying logger:

        switch (this.level) {
            case TRACE :
                this.logger.trace(format, param);
                break;
            case DEBUG:
                this.logger.debug(format, param);
                break;
            case INFO:
                this.logger.info(format, param);
                break;
            case WARN:
                this.logger.warn(format, param);
                break;
            case SEVERE:
                this.logger.error(format, param);
                break;
            default:
                throw new IllegalArgumentException("Unknown log level");
        }

Choose a reason for hiding this comment

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

I also propose to use ERROR instead of SEVERE to be consistent with SLF4J

if (this.level == LogWrap.Level.TRACE

Choose a reason for hiding this comment

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

consider using switch by LogWrap.Level with default 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.

@longtimeago I think switch with if in each block will be less readable. isn't it?

&& this.logger.isTraceEnabled()) {
this.logger.trace(String.format(format, param));
} else if (
this.level == LogWrap.Level.DEBUG
&& this.logger.isDebugEnabled()) {
this.logger.debug(String.format(format, param));
} else if (
this.level == LogWrap.Level.INFO
&& this.logger.isInfoEnabled()) {
this.logger.trace(String.format(format, param));
} else if (
this.level == LogWrap.Level.WARN
&& this.logger.isWarnEnabled()) {
this.logger.warn(String.format(format, param));
} else if (
this.level == LogWrap.Level.SEVERE
&& this.logger.isErrorEnabled()) {
this.logger.error(String.format(format, param));
}
}
}
107 changes: 107 additions & 0 deletions src/main/java/org/takes/facets/slf4j/PsLogged.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
/**
* The MIT License (MIT)
*
* Copyright (c) 2015 Yegor Bugayenko
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included
* in all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
* SOFTWARE.
*/

package org.takes.facets.slf4j;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Iterator;
import lombok.EqualsAndHashCode;
import org.takes.Request;
import org.takes.Response;
import org.takes.facets.auth.Identity;
import org.takes.facets.auth.Pass;

/**
* Logs Pass methods calls..
*
* <p>The class is immutable and thread-safe.
* @author Dmitry Zaytsev (dmitry.zaytsev@gmail.com)
* @version $Id$
*/
@EqualsAndHashCode(of = "origin", callSuper = false)
public final class PsLogged extends LogWrap implements Pass {
/**
* Original pass.
*/
private final transient Pass origin;

/**
* Ctor.
* @param pass Original
*/
public PsLogged(final Pass pass) {
this(pass, LogWrap.Level.TRACE);
}

/**
* Ctor.
* @param pass Original
* @param lvl Log level
*/
public PsLogged(final Pass pass, final LogWrap.Level lvl) {
super(pass.getClass(), lvl);
this.origin = pass;
}

@Override
public Iterator<Identity> enter(final Request request) throws IOException {
final long started = System.currentTimeMillis();
final Iterator<Identity> iterator = this.origin.enter(request);
final Collection<Identity> resp = new ArrayList<Identity>(1);
final StringBuilder sbr = new StringBuilder("[");
if (iterator.hasNext()) {
final Identity next = iterator.next();
sbr.append(next);
sbr.append(',');
resp.add(next);
}
sbr.append(']');
this.log(
"[%s] #enter(%s) return [%s] in [%d] ms",
this.origin,
request,
sbr.toString(),
System.currentTimeMillis() - started
);
return resp.iterator();
}

@Override
public Response exit(final Response response, final Identity identity)
throws IOException {
final long started = System.currentTimeMillis();
final Response resp = this.origin.exit(response, identity);
this.log(
"[%s] #exit(%s,%s) return [%s] in [%d] ms",
this.origin,
response,
identity,
resp,
System.currentTimeMillis() - started
);
return resp;
}
}
Loading