Mike Kolesnik has uploaded a new change for review. Change subject: engine: Fix VDS command logging to use @Logged ......................................................................
engine: Fix VDS command logging to use @Logged Currently VDS command executions are logged using @Logged, but the exception is not because it's handled internally in VDSCommandBase. Replaced internal handling with standard handling using @Logged, which allows standard and flexible control of how the exception will be logged. Since stacktrace is not always desirable, added a way to set it's log level on the @Logged annotation. The default is DEBUG which is what was used to log the trace, but can be changed for individual commands if need be. Change-Id: I2d7101ccd26509d1afdb8d606a6114aa7c5a06a7 Signed-off-by: Mike Kolesnik <[email protected]> --- M backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/log/Logged.java M backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/log/LoggedUtils.java M backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/log/LoggedUtilsTest.java M backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VDSCommandBase.java 4 files changed, 51 insertions(+), 9 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/18/17318/1 diff --git a/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/log/Logged.java b/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/log/Logged.java index 3579556..718bd94 100644 --- a/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/log/Logged.java +++ b/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/log/Logged.java @@ -104,4 +104,18 @@ * <b>Default:</b> {@link LogLevel#INFO}. */ LogLevel returnLevel() default LogLevel.INFO; + + /** + * The stack trace level determines when should the command's exception stack trace be printed. The + * {@link Logged#errorLevel()} is used as a logical upper-bound for this parameter, so it makes no sense to set it + * higher than that.<br> + * If the log level is lower than {@link Logged#errorLevel()}, and the {@link Logged#errorLevel()} is enabled in the + * log, then: + * <ul> + * <li>If the stack trace level is enabled in the log, then the exception stack trace is printed.</li> + * <li>Otherwise, the exception stack trace is not printed, only the error message.</li> + * </ul> + * <b>Default:</b> {@link LogLevel#DEBUG}. + */ + LogLevel stackTraceLevel() default LogLevel.DEBUG; } diff --git a/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/log/LoggedUtils.java b/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/log/LoggedUtils.java index 137368f..4687422 100644 --- a/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/log/LoggedUtils.java +++ b/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/log/LoggedUtils.java @@ -111,7 +111,10 @@ /** * Log error of a command to the given log, using the specification in the {@link Logged} annotation on the * object's. If there's no {@link Logged} annotation present, or if the log level isn't sufficient to log, then - * nothing happens. + * nothing happens.<br> + * <br> + * The {@link Logged#errorLevel()} field will determine if the log should be printed at all, while + * {@link Logged#stackTraceLevel()} will determine is the stacktrace will be printed to log. * * @param log * The log to log to. @@ -122,9 +125,21 @@ */ public static void logError(Log log, String id, Object obj, Throwable t) { Logged logged = getAnnotation(obj); - if (logged != null && isLogLevelOn(log, logged.errorLevel())) { - log(log, logged.errorLevel(), ERROR_LOG, determineMessage(log, logged, obj), + if (logged == null) { + return; + } + + LogLevel logLevel = logged.errorLevel(); + if (!isLogLevelOn(log, logLevel)) { + return; + } + + if (isLogLevelOn(log, LogLevel.getMinimalLevel(logLevel, logged.stackTraceLevel()))) { + log(log, logLevel, ERROR_LOG, determineMessage(log, logged, obj), ExceptionUtils.getMessage(t), id, t); + } else { + log(log, logLevel, ERROR_LOG, determineMessage(log, logged, obj), + ExceptionUtils.getMessage(t), id); } } diff --git a/backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/log/LoggedUtilsTest.java b/backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/log/LoggedUtilsTest.java index 3cc3ad4..d1871e8 100644 --- a/backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/log/LoggedUtilsTest.java +++ b/backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/log/LoggedUtilsTest.java @@ -42,7 +42,7 @@ } @Logged(executionLevel = LogLevel.DEBUG, errorLevel = LogLevel.WARN, - parametersLevel = LogLevel.FATAL, returnLevel = LogLevel.FATAL) + parametersLevel = LogLevel.FATAL, returnLevel = LogLevel.FATAL, stackTraceLevel = LogLevel.FATAL) public class LoggedOverridingSubclass extends LoggedClass { // Intentionally empty - a stub class for testing } @@ -54,6 +54,11 @@ @Logged(returnLevel = LogLevel.DEBUG) public class LoggedOverridingSubclassNoReturn extends LoggedClass { + // Intentionally empty - a stub class for testing + } + + @Logged(stackTraceLevel = LogLevel.DEBUG) + public class LoggedOverridingSubclassNoStackTrace extends LoggedClass { // Intentionally empty - a stub class for testing } @@ -333,6 +338,17 @@ verify(log).warnFormat(eq(LoggedUtils.ERROR_LOG), anyObject(), anyObject(), eq(id), anyObject()); } + @Test + public void testLogErrorLogsNoStackTrace() throws Exception { + String id = ""; + Log log = mock(Log.class); + + when(log.isErrorEnabled()).thenReturn(true); + + LoggedUtils.logError(log, id, new LoggedOverridingSubclassNoStackTrace(), new Exception()); + verify(log).errorFormat(eq(LoggedUtils.ERROR_LOG), anyObject(), anyObject(), eq(id)); + } + /* --- Helper methods --- */ /** diff --git a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VDSCommandBase.java b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VDSCommandBase.java index afffcbe..245db03 100644 --- a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VDSCommandBase.java +++ b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VDSCommandBase.java @@ -1,10 +1,10 @@ package org.ovirt.engine.core.vdsbroker; import org.apache.commons.lang.StringUtils; -import org.apache.commons.lang.exception.ExceptionUtils; import org.ovirt.engine.core.common.vdscommands.VDSParametersBase; import org.ovirt.engine.core.common.vdscommands.VDSReturnValue; import org.ovirt.engine.core.dal.VdcCommandBase; +import org.ovirt.engine.core.utils.log.LoggedUtils; import org.ovirt.engine.core.vdsbroker.vdsbroker.VDSExceptionBase; public abstract class VDSCommandBase<P extends VDSParametersBase> extends VdcCommandBase { @@ -79,10 +79,7 @@ } private void logException(RuntimeException ex) { - log.errorFormat("Command {0} execution failed. Exception: {1}", getCommandName(), ExceptionUtils.getMessage(ex)); - if (log.isDebugEnabled()) { - log.debugFormat("Detailed stacktrace:", ex); - } + LoggedUtils.logError(log, LoggedUtils.getObjectId(this), this, ex); } protected String getAdditionalInformation() { -- To view, visit http://gerrit.ovirt.org/17318 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I2d7101ccd26509d1afdb8d606a6114aa7c5a06a7 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Mike Kolesnik <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
